| Message ID | 20250109-san-v7-1-93c432a73024@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix check-qtest-ppc64 sanitizer errors | expand |
On Thu, 9 Jan 2025, Akihiko Odaki wrote: > Do not refer to "memory region's reference count" > ------------------------------------------------- > > Now MemoryRegions do have their own reference counts, but they will not > be used when their owners are not themselves. However, the documentation > of memory_region_ref() says it adds "1 to a memory region's reference > count", which is confusing. Avoid referring to "memory region's > reference count" and just say: "Add a reference to a memory region". > Make a similar change to memory_region_unref() too. > > Refer to docs/devel/memory.rst for "owner" > ------------------------------------------ > > memory_region_ref() and memory_region_unref() used to have their own > descriptions of "owner", but they are somewhat out-of-date and > misleading. > > In particular, they say "whenever memory regions are accessed outside > the BQL, they need to be preserved against hot-unplug", but protecting > against hot-unplug is not mandatory if it is known that they will never > be hot-unplugged. They also say "MemoryRegions actually do not have > their own reference count", but they actually do. They just will not be > used unless their owners are not themselves. > > Refer to docs/devel/memory.rst as the single source of truth instead of > maintaining duplicate descriptions of "owner". > > Clarify that owner may be missing > > --------------------------------- > A memory region may not have an owner, and memory_region_ref() and > memory_region_unref() do nothing for such. > > memory: Clarify owner must not call memory_region_ref() > -------------------------------------------------------- > > The owner must not call this function as it results in a circular > reference. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > Reviewed-by: Peter Xu <peterx@redhat.com> > --- > include/exec/memory.h | 59 ++++++++++++++++++++++++--------------------------- > 1 file changed, 28 insertions(+), 31 deletions(-) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 9458e2801d50..ca247343f433 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s); > * memory_region_add_subregion() to add subregions. > * > * @mr: the #MemoryRegion to be initialized > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @name: used for debugging; not visible to the user or ABI > * @size: size of the region; any subregions beyond this size will be clipped > */ > @@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr, > uint64_t size); > > /** > - * memory_region_ref: Add 1 to a memory region's reference count > + * memory_region_ref: Add a reference to the owner of a memory region > * > - * Whenever memory regions are accessed outside the BQL, they need to be > - * preserved against hot-unplug. MemoryRegions actually do not have their > - * own reference count; they piggyback on a QOM object, their "owner". > - * This function adds a reference to the owner. > - * > - * All MemoryRegions must have an owner if they can disappear, even if the > - * device they belong to operates exclusively under the BQL. This is because > - * the region could be returned at any time by memory_region_find, and this > - * is usually under guest control. > + * This function adds a reference to the owner of a memory region to keep the > + * memory region alive. It does nothing if the owner is not present as a memory > + * region without owner will never die. > + * For references internal to the owner, use object_ref() instead to avoid a > + * circular reference. Reading this again I'm still confused by this last sentence. Do you mean references internal to the memory region should use object_ref on the memory region or that other references to the owner should use object_ref on the owner? This sentence is still not clear about that. Regards, BALATON Zoltan > + * See docs/devel/memory.rst to know about owner. > * > * @mr: the #MemoryRegion > */ > void memory_region_ref(MemoryRegion *mr); > > /** > - * memory_region_unref: Remove 1 to a memory region's reference count > + * memory_region_unref: Remove a reference to the memory region of the owner > * > - * Whenever memory regions are accessed outside the BQL, they need to be > - * preserved against hot-unplug. MemoryRegions actually do not have their > - * own reference count; they piggyback on a QOM object, their "owner". > - * This function removes a reference to the owner and possibly destroys it. > + * This function removes a reference to the owner of a memory region and > + * possibly destroys the owner along with the memory region. It does nothing if > + * the owner is not present. > + * See docs/devel/memory.rst to know about owner. > * > * @mr: the #MemoryRegion > */ > @@ -1255,7 +1252,7 @@ void memory_region_unref(MemoryRegion *mr); > * if @size is nonzero, subregions will be clipped to @size. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @ops: a structure containing read and write callbacks to be used when > * I/O is performed on the region. > * @opaque: passed to the read and write callbacks of the @ops structure. > @@ -1275,7 +1272,7 @@ void memory_region_init_io(MemoryRegion *mr, > * directly. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @name: Region name, becomes part of RAMBlock name used in migration stream > * must be unique within any device > * @size: size of the region. > @@ -1298,7 +1295,7 @@ bool memory_region_init_ram_nomigrate(MemoryRegion *mr, > * modify memory directly. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @name: Region name, becomes part of RAMBlock name used in migration stream > * must be unique within any device > * @size: size of the region. > @@ -1328,7 +1325,7 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr, > * canceled. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @name: Region name, becomes part of RAMBlock name used in migration stream > * must be unique within any device > * @size: used size of the region. > @@ -1357,7 +1354,7 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr, > * mmap-ed backend. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @name: Region name, becomes part of RAMBlock name used in migration stream > * must be unique within any device > * @size: size of the region. > @@ -1390,7 +1387,7 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr, > * mmap-ed backend. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @name: the name of the region. > * @size: size of the region. > * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM, > @@ -1421,7 +1418,7 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr, > * region will modify memory directly. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @name: Region name, becomes part of RAMBlock name used in migration stream > * must be unique within any device > * @size: size of the region. > @@ -1449,7 +1446,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr, > * skip_dump flag. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @name: the name of the region. > * @size: size of the region. > * @ptr: memory to be mapped; must contain at least @size bytes. > @@ -1469,7 +1466,7 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr, > * part of another memory region. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @name: used for debugging; not visible to the user or ABI > * @orig: the region to be referenced; @mr will be equivalent to > * @orig between @offset and @offset + @size - 1. > @@ -1495,7 +1492,7 @@ void memory_region_init_alias(MemoryRegion *mr, > * of the caller. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @name: Region name, becomes part of RAMBlock name used in migration stream > * must be unique within any device > * @size: size of the region. > @@ -1518,7 +1515,7 @@ bool memory_region_init_rom_nomigrate(MemoryRegion *mr, > * of the caller. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @ops: callbacks for write access handling (must not be NULL). > * @opaque: passed to the read and write callbacks of the @ops structure. > * @name: Region name, becomes part of RAMBlock name used in migration stream > @@ -1554,7 +1551,7 @@ bool memory_region_init_rom_device_nomigrate(MemoryRegion *mr, > * @_iommu_mr: the #IOMMUMemoryRegion to be initialized > * @instance_size: the IOMMUMemoryRegion subclass instance size > * @mrtypename: the type name of the #IOMMUMemoryRegion > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @name: used for debugging; not visible to the user or ABI > * @size: size of the region. > */ > @@ -1570,7 +1567,7 @@ void memory_region_init_iommu(void *_iommu_mr, > * region will modify memory directly. > * > * @mr: the #MemoryRegion to be initialized > - * @owner: the object that tracks the region's reference count (must be > + * @owner: the object that keeps the region alive (must be > * TYPE_DEVICE or a subclass of TYPE_DEVICE, or NULL) > * @name: name of the memory region > * @size: size of the region in bytes > @@ -1616,7 +1613,7 @@ bool memory_region_init_ram_guest_memfd(MemoryRegion *mr, > * If you pass a non-NULL non-device @owner then we will assert. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @name: Region name, becomes part of RAMBlock name used in migration stream > * must be unique within any device > * @size: size of the region. > @@ -1647,7 +1644,7 @@ bool memory_region_init_rom(MemoryRegion *mr, > * If you pass a non-NULL non-device @owner then we will assert. > * > * @mr: the #MemoryRegion to be initialized. > - * @owner: the object that tracks the region's reference count > + * @owner: the object that keeps the region alive > * @ops: callbacks for write access handling (must not be NULL). > * @opaque: passed to the read and write callbacks of the @ops structure. > * @name: Region name, becomes part of RAMBlock name used in migration stream > >
On Thu, Jan 09, 2025 at 01:30:35PM +0100, BALATON Zoltan wrote: > On Thu, 9 Jan 2025, Akihiko Odaki wrote: > > Do not refer to "memory region's reference count" > > ------------------------------------------------- > > > > Now MemoryRegions do have their own reference counts, but they will not > > be used when their owners are not themselves. However, the documentation > > of memory_region_ref() says it adds "1 to a memory region's reference > > count", which is confusing. Avoid referring to "memory region's > > reference count" and just say: "Add a reference to a memory region". > > Make a similar change to memory_region_unref() too. > > > > Refer to docs/devel/memory.rst for "owner" > > ------------------------------------------ > > > > memory_region_ref() and memory_region_unref() used to have their own > > descriptions of "owner", but they are somewhat out-of-date and > > misleading. > > > > In particular, they say "whenever memory regions are accessed outside > > the BQL, they need to be preserved against hot-unplug", but protecting > > against hot-unplug is not mandatory if it is known that they will never > > be hot-unplugged. They also say "MemoryRegions actually do not have > > their own reference count", but they actually do. They just will not be > > used unless their owners are not themselves. > > > > Refer to docs/devel/memory.rst as the single source of truth instead of > > maintaining duplicate descriptions of "owner". > > > > Clarify that owner may be missing > > > > --------------------------------- > > A memory region may not have an owner, and memory_region_ref() and > > memory_region_unref() do nothing for such. > > > > memory: Clarify owner must not call memory_region_ref() > > -------------------------------------------------------- > > > > The owner must not call this function as it results in a circular > > reference. > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > Reviewed-by: Peter Xu <peterx@redhat.com> > > --- > > include/exec/memory.h | 59 ++++++++++++++++++++++++--------------------------- > > 1 file changed, 28 insertions(+), 31 deletions(-) > > > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > index 9458e2801d50..ca247343f433 100644 > > --- a/include/exec/memory.h > > +++ b/include/exec/memory.h > > @@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s); > > * memory_region_add_subregion() to add subregions. > > * > > * @mr: the #MemoryRegion to be initialized > > - * @owner: the object that tracks the region's reference count > > + * @owner: the object that keeps the region alive > > * @name: used for debugging; not visible to the user or ABI > > * @size: size of the region; any subregions beyond this size will be clipped > > */ > > @@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr, > > uint64_t size); > > > > /** > > - * memory_region_ref: Add 1 to a memory region's reference count > > + * memory_region_ref: Add a reference to the owner of a memory region > > * > > - * Whenever memory regions are accessed outside the BQL, they need to be > > - * preserved against hot-unplug. MemoryRegions actually do not have their > > - * own reference count; they piggyback on a QOM object, their "owner". > > - * This function adds a reference to the owner. > > - * > > - * All MemoryRegions must have an owner if they can disappear, even if the > > - * device they belong to operates exclusively under the BQL. This is because > > - * the region could be returned at any time by memory_region_find, and this > > - * is usually under guest control. > > + * This function adds a reference to the owner of a memory region to keep the > > + * memory region alive. It does nothing if the owner is not present as a memory > > + * region without owner will never die. > > + * For references internal to the owner, use object_ref() instead to avoid a > > + * circular reference. > > Reading this again I'm still confused by this last sentence. Do you mean > references internal to the memory region should use object_ref on the memory > region or that other references to the owner should use object_ref on the > owner? This sentence is still not clear about that. Having two refcounts are definitely confusing.. especially IIRC all MRs' obj->free==NULL, so the MR's refcount isn't working. Dynamic MR's needs its g_free() on its own. I acked both patches, but maybe it could indeed be slightly better we drop this sentence, meanwhile in patch 2 we can drop the object_ref() too: it means for parent/child MRs that share the same owner, QEMU does nothing on the child MRs when add subregion, because it assumes the child MR will never go away when the parent is there who shares the owner. So maybe we try not to touch MR's refcount manually, but fix what can be problematic for owner->ref only.
On Thu, Jan 09, 2025 at 02:29:21PM -0500, Peter Xu wrote: > On Thu, Jan 09, 2025 at 01:30:35PM +0100, BALATON Zoltan wrote: > > On Thu, 9 Jan 2025, Akihiko Odaki wrote: > > > Do not refer to "memory region's reference count" > > > ------------------------------------------------- > > > > > > Now MemoryRegions do have their own reference counts, but they will not > > > be used when their owners are not themselves. However, the documentation > > > of memory_region_ref() says it adds "1 to a memory region's reference > > > count", which is confusing. Avoid referring to "memory region's > > > reference count" and just say: "Add a reference to a memory region". > > > Make a similar change to memory_region_unref() too. > > > > > > Refer to docs/devel/memory.rst for "owner" > > > ------------------------------------------ > > > > > > memory_region_ref() and memory_region_unref() used to have their own > > > descriptions of "owner", but they are somewhat out-of-date and > > > misleading. > > > > > > In particular, they say "whenever memory regions are accessed outside > > > the BQL, they need to be preserved against hot-unplug", but protecting > > > against hot-unplug is not mandatory if it is known that they will never > > > be hot-unplugged. They also say "MemoryRegions actually do not have > > > their own reference count", but they actually do. They just will not be > > > used unless their owners are not themselves. > > > > > > Refer to docs/devel/memory.rst as the single source of truth instead of > > > maintaining duplicate descriptions of "owner". > > > > > > Clarify that owner may be missing > > > > > > --------------------------------- > > > A memory region may not have an owner, and memory_region_ref() and > > > memory_region_unref() do nothing for such. > > > > > > memory: Clarify owner must not call memory_region_ref() > > > -------------------------------------------------------- > > > > > > The owner must not call this function as it results in a circular > > > reference. > > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > Reviewed-by: Peter Xu <peterx@redhat.com> > > > --- > > > include/exec/memory.h | 59 ++++++++++++++++++++++++--------------------------- > > > 1 file changed, 28 insertions(+), 31 deletions(-) > > > > > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > > index 9458e2801d50..ca247343f433 100644 > > > --- a/include/exec/memory.h > > > +++ b/include/exec/memory.h > > > @@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s); > > > * memory_region_add_subregion() to add subregions. > > > * > > > * @mr: the #MemoryRegion to be initialized > > > - * @owner: the object that tracks the region's reference count > > > + * @owner: the object that keeps the region alive > > > * @name: used for debugging; not visible to the user or ABI > > > * @size: size of the region; any subregions beyond this size will be clipped > > > */ > > > @@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr, > > > uint64_t size); > > > > > > /** > > > - * memory_region_ref: Add 1 to a memory region's reference count > > > + * memory_region_ref: Add a reference to the owner of a memory region > > > * > > > - * Whenever memory regions are accessed outside the BQL, they need to be > > > - * preserved against hot-unplug. MemoryRegions actually do not have their > > > - * own reference count; they piggyback on a QOM object, their "owner". > > > - * This function adds a reference to the owner. > > > - * > > > - * All MemoryRegions must have an owner if they can disappear, even if the > > > - * device they belong to operates exclusively under the BQL. This is because > > > - * the region could be returned at any time by memory_region_find, and this > > > - * is usually under guest control. > > > + * This function adds a reference to the owner of a memory region to keep the > > > + * memory region alive. It does nothing if the owner is not present as a memory > > > + * region without owner will never die. > > > + * For references internal to the owner, use object_ref() instead to avoid a > > > + * circular reference. > > > > Reading this again I'm still confused by this last sentence. Do you mean > > references internal to the memory region should use object_ref on the memory > > region or that other references to the owner should use object_ref on the > > owner? This sentence is still not clear about that. > > Having two refcounts are definitely confusing.. especially IIRC all MRs' > obj->free==NULL, so the MR's refcount isn't working. Dynamic MR's needs > its g_free() on its own. > > I acked both patches, but maybe it could indeed be slightly better we drop > this sentence, meanwhile in patch 2 we can drop the object_ref() too: it > means for parent/child MRs that share the same owner, QEMU does nothing on > the child MRs when add subregion, because it assumes the child MR will > never go away when the parent is there who shares the owner. > > So maybe we try not to touch MR's refcount manually, but fix what can be > problematic for owner->ref only. As an attached comment: I may have forgot some context on this issue, but I still remember I used to have a patch that simply detach either parent or child MR links when finalize(). It's here: https://lore.kernel.org/all/ZsenKpu1czQGYz7m@x1n/ I see this issue was there for a long time so maybe we want to fix it one way or another. I don't strongly feel which way to go, but personally I still prefer that way (I assume that can fix the same issue), and it doesn't have MR's refcount involved at all, meanwhile I don't see an issue yet with it..
On 2025/01/10 4:37, Peter Xu wrote: > On Thu, Jan 09, 2025 at 02:29:21PM -0500, Peter Xu wrote: >> On Thu, Jan 09, 2025 at 01:30:35PM +0100, BALATON Zoltan wrote: >>> On Thu, 9 Jan 2025, Akihiko Odaki wrote: >>>> Do not refer to "memory region's reference count" >>>> ------------------------------------------------- >>>> >>>> Now MemoryRegions do have their own reference counts, but they will not >>>> be used when their owners are not themselves. However, the documentation >>>> of memory_region_ref() says it adds "1 to a memory region's reference >>>> count", which is confusing. Avoid referring to "memory region's >>>> reference count" and just say: "Add a reference to a memory region". >>>> Make a similar change to memory_region_unref() too. >>>> >>>> Refer to docs/devel/memory.rst for "owner" >>>> ------------------------------------------ >>>> >>>> memory_region_ref() and memory_region_unref() used to have their own >>>> descriptions of "owner", but they are somewhat out-of-date and >>>> misleading. >>>> >>>> In particular, they say "whenever memory regions are accessed outside >>>> the BQL, they need to be preserved against hot-unplug", but protecting >>>> against hot-unplug is not mandatory if it is known that they will never >>>> be hot-unplugged. They also say "MemoryRegions actually do not have >>>> their own reference count", but they actually do. They just will not be >>>> used unless their owners are not themselves. >>>> >>>> Refer to docs/devel/memory.rst as the single source of truth instead of >>>> maintaining duplicate descriptions of "owner". >>>> >>>> Clarify that owner may be missing >>>> >>>> --------------------------------- >>>> A memory region may not have an owner, and memory_region_ref() and >>>> memory_region_unref() do nothing for such. >>>> >>>> memory: Clarify owner must not call memory_region_ref() >>>> -------------------------------------------------------- >>>> >>>> The owner must not call this function as it results in a circular >>>> reference. >>>> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>> Reviewed-by: Peter Xu <peterx@redhat.com> >>>> --- >>>> include/exec/memory.h | 59 ++++++++++++++++++++++++--------------------------- >>>> 1 file changed, 28 insertions(+), 31 deletions(-) >>>> >>>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>>> index 9458e2801d50..ca247343f433 100644 >>>> --- a/include/exec/memory.h >>>> +++ b/include/exec/memory.h >>>> @@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s); >>>> * memory_region_add_subregion() to add subregions. >>>> * >>>> * @mr: the #MemoryRegion to be initialized >>>> - * @owner: the object that tracks the region's reference count >>>> + * @owner: the object that keeps the region alive >>>> * @name: used for debugging; not visible to the user or ABI >>>> * @size: size of the region; any subregions beyond this size will be clipped >>>> */ >>>> @@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr, >>>> uint64_t size); >>>> >>>> /** >>>> - * memory_region_ref: Add 1 to a memory region's reference count >>>> + * memory_region_ref: Add a reference to the owner of a memory region >>>> * >>>> - * Whenever memory regions are accessed outside the BQL, they need to be >>>> - * preserved against hot-unplug. MemoryRegions actually do not have their >>>> - * own reference count; they piggyback on a QOM object, their "owner". >>>> - * This function adds a reference to the owner. >>>> - * >>>> - * All MemoryRegions must have an owner if they can disappear, even if the >>>> - * device they belong to operates exclusively under the BQL. This is because >>>> - * the region could be returned at any time by memory_region_find, and this >>>> - * is usually under guest control. >>>> + * This function adds a reference to the owner of a memory region to keep the >>>> + * memory region alive. It does nothing if the owner is not present as a memory >>>> + * region without owner will never die. >>>> + * For references internal to the owner, use object_ref() instead to avoid a >>>> + * circular reference. >>> >>> Reading this again I'm still confused by this last sentence. Do you mean >>> references internal to the memory region should use object_ref on the memory >>> region or that other references to the owner should use object_ref on the >>> owner? This sentence is still not clear about that. >> >> Having two refcounts are definitely confusing.. especially IIRC all MRs' >> obj->free==NULL, so the MR's refcount isn't working. Dynamic MR's needs >> its g_free() on its own. We still have instance_finalize that will fire when the MR's refcount gets zero so it has its own use cases. >> >> I acked both patches, but maybe it could indeed be slightly better we drop >> this sentence, meanwhile in patch 2 we can drop the object_ref() too: it >> means for parent/child MRs that share the same owner, QEMU does nothing on >> the child MRs when add subregion, because it assumes the child MR will >> never go away when the parent is there who shares the owner. >> >> So maybe we try not to touch MR's refcount manually, but fix what can be >> problematic for owner->ref only. > > As an attached comment: I may have forgot some context on this issue, but I > still remember I used to have a patch that simply detach either parent or > child MR links when finalize(). It's here: > > https://lore.kernel.org/all/ZsenKpu1czQGYz7m@x1n/ > > I see this issue was there for a long time so maybe we want to fix it one > way or another. I don't strongly feel which way to go, but personally I > still prefer that way (I assume that can fix the same issue), and it > doesn't have MR's refcount involved at all, meanwhile I don't see an issue > yet with it.. > For this particular topic I have somewhat a strong opinion that we should care the two reference counters. Indeed, dealing with two reference counters is not fun, but sometimes it is necessary to do reference counting correctly. Your patch is to avoid reference counting for tracking dependencies among regions with the same owner, and it does so by ignoring the reference from container to subregion. I prefer to keep reference counting correct instead of having an additional ad-hoc measure that breaks reference relationships.
On Fri, Jan 10, 2025 at 05:43:15PM +0900, Akihiko Odaki wrote: > On 2025/01/10 4:37, Peter Xu wrote: > > On Thu, Jan 09, 2025 at 02:29:21PM -0500, Peter Xu wrote: > > > On Thu, Jan 09, 2025 at 01:30:35PM +0100, BALATON Zoltan wrote: > > > > On Thu, 9 Jan 2025, Akihiko Odaki wrote: > > > > > Do not refer to "memory region's reference count" > > > > > ------------------------------------------------- > > > > > > > > > > Now MemoryRegions do have their own reference counts, but they will not > > > > > be used when their owners are not themselves. However, the documentation > > > > > of memory_region_ref() says it adds "1 to a memory region's reference > > > > > count", which is confusing. Avoid referring to "memory region's > > > > > reference count" and just say: "Add a reference to a memory region". > > > > > Make a similar change to memory_region_unref() too. > > > > > > > > > > Refer to docs/devel/memory.rst for "owner" > > > > > ------------------------------------------ > > > > > > > > > > memory_region_ref() and memory_region_unref() used to have their own > > > > > descriptions of "owner", but they are somewhat out-of-date and > > > > > misleading. > > > > > > > > > > In particular, they say "whenever memory regions are accessed outside > > > > > the BQL, they need to be preserved against hot-unplug", but protecting > > > > > against hot-unplug is not mandatory if it is known that they will never > > > > > be hot-unplugged. They also say "MemoryRegions actually do not have > > > > > their own reference count", but they actually do. They just will not be > > > > > used unless their owners are not themselves. > > > > > > > > > > Refer to docs/devel/memory.rst as the single source of truth instead of > > > > > maintaining duplicate descriptions of "owner". > > > > > > > > > > Clarify that owner may be missing > > > > > > > > > > --------------------------------- > > > > > A memory region may not have an owner, and memory_region_ref() and > > > > > memory_region_unref() do nothing for such. > > > > > > > > > > memory: Clarify owner must not call memory_region_ref() > > > > > -------------------------------------------------------- > > > > > > > > > > The owner must not call this function as it results in a circular > > > > > reference. > > > > > > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > > > Reviewed-by: Peter Xu <peterx@redhat.com> > > > > > --- > > > > > include/exec/memory.h | 59 ++++++++++++++++++++++++--------------------------- > > > > > 1 file changed, 28 insertions(+), 31 deletions(-) > > > > > > > > > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > > > > index 9458e2801d50..ca247343f433 100644 > > > > > --- a/include/exec/memory.h > > > > > +++ b/include/exec/memory.h > > > > > @@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s); > > > > > * memory_region_add_subregion() to add subregions. > > > > > * > > > > > * @mr: the #MemoryRegion to be initialized > > > > > - * @owner: the object that tracks the region's reference count > > > > > + * @owner: the object that keeps the region alive > > > > > * @name: used for debugging; not visible to the user or ABI > > > > > * @size: size of the region; any subregions beyond this size will be clipped > > > > > */ > > > > > @@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr, > > > > > uint64_t size); > > > > > > > > > > /** > > > > > - * memory_region_ref: Add 1 to a memory region's reference count > > > > > + * memory_region_ref: Add a reference to the owner of a memory region > > > > > * > > > > > - * Whenever memory regions are accessed outside the BQL, they need to be > > > > > - * preserved against hot-unplug. MemoryRegions actually do not have their > > > > > - * own reference count; they piggyback on a QOM object, their "owner". > > > > > - * This function adds a reference to the owner. > > > > > - * > > > > > - * All MemoryRegions must have an owner if they can disappear, even if the > > > > > - * device they belong to operates exclusively under the BQL. This is because > > > > > - * the region could be returned at any time by memory_region_find, and this > > > > > - * is usually under guest control. > > > > > + * This function adds a reference to the owner of a memory region to keep the > > > > > + * memory region alive. It does nothing if the owner is not present as a memory > > > > > + * region without owner will never die. > > > > > + * For references internal to the owner, use object_ref() instead to avoid a > > > > > + * circular reference. > > > > > > > > Reading this again I'm still confused by this last sentence. Do you mean > > > > references internal to the memory region should use object_ref on the memory > > > > region or that other references to the owner should use object_ref on the > > > > owner? This sentence is still not clear about that. > > > > > > Having two refcounts are definitely confusing.. especially IIRC all MRs' > > > obj->free==NULL, so the MR's refcount isn't working. Dynamic MR's needs > > > its g_free() on its own. > > We still have instance_finalize that will fire when the MR's refcount gets > zero so it has its own use cases. > > > > > > > I acked both patches, but maybe it could indeed be slightly better we drop > > > this sentence, meanwhile in patch 2 we can drop the object_ref() too: it > > > means for parent/child MRs that share the same owner, QEMU does nothing on > > > the child MRs when add subregion, because it assumes the child MR will > > > never go away when the parent is there who shares the owner. > > > > > > So maybe we try not to touch MR's refcount manually, but fix what can be > > > problematic for owner->ref only. > > > > As an attached comment: I may have forgot some context on this issue, but I > > still remember I used to have a patch that simply detach either parent or > > child MR links when finalize(). It's here: > > > > https://lore.kernel.org/all/ZsenKpu1czQGYz7m@x1n/ > > > > I see this issue was there for a long time so maybe we want to fix it one > > way or another. I don't strongly feel which way to go, but personally I > > still prefer that way (I assume that can fix the same issue), and it > > doesn't have MR's refcount involved at all, meanwhile I don't see an issue > > yet with it.. > > > > For this particular topic I have somewhat a strong opinion that we should > care the two reference counters. > > Indeed, dealing with two reference counters is not fun, but sometimes it is > necessary to do reference counting correctly. Your patch is to avoid > reference counting for tracking dependencies among regions with the same > owner, and it does so by ignoring the reference from container to subregion. I don't think so? When with that patch, container will reference subregion the same way as others, which is to take a refcount on the owner. That kept at least the refcount behavior consistent within memory_region_ref(). That patch removes the circular reference by always properly release the circular reference due to sub-regioning internally. > > I prefer to keep reference counting correct instead of having an additional > ad-hoc measure that breaks reference relationships. Your patch added more complexity to me on refcounting, meanwhile it's also not always "correct". It can boil down to how you define "correct" - if you mean one should always boost a refcount somewhere if it references one MR, then it's still not 100% correct at least when mr->owner==NULL. We never yet did it alright, so to me it's a matter of working around current sanitizer issue, and that's only about it yet so far. Meanwhile I _think_ adding such complexity also means MR's finalize() will be called in specific order when parent/child MRs belong to the same owner. In my patch the order shouldn't matter, IIUC, which I preferred because that reduces details that we may not care much (or I could have overlooked why we need to care about it). Basically that's simpler to maintain to me, but again, I don't feel strongly until someone would like / be able to rework MR refcounting completely. Thanks,
On 2025/01/11 0:18, Peter Xu wrote: > On Fri, Jan 10, 2025 at 05:43:15PM +0900, Akihiko Odaki wrote: >> On 2025/01/10 4:37, Peter Xu wrote: >>> On Thu, Jan 09, 2025 at 02:29:21PM -0500, Peter Xu wrote: >>>> On Thu, Jan 09, 2025 at 01:30:35PM +0100, BALATON Zoltan wrote: >>>>> On Thu, 9 Jan 2025, Akihiko Odaki wrote: >>>>>> Do not refer to "memory region's reference count" >>>>>> ------------------------------------------------- >>>>>> >>>>>> Now MemoryRegions do have their own reference counts, but they will not >>>>>> be used when their owners are not themselves. However, the documentation >>>>>> of memory_region_ref() says it adds "1 to a memory region's reference >>>>>> count", which is confusing. Avoid referring to "memory region's >>>>>> reference count" and just say: "Add a reference to a memory region". >>>>>> Make a similar change to memory_region_unref() too. >>>>>> >>>>>> Refer to docs/devel/memory.rst for "owner" >>>>>> ------------------------------------------ >>>>>> >>>>>> memory_region_ref() and memory_region_unref() used to have their own >>>>>> descriptions of "owner", but they are somewhat out-of-date and >>>>>> misleading. >>>>>> >>>>>> In particular, they say "whenever memory regions are accessed outside >>>>>> the BQL, they need to be preserved against hot-unplug", but protecting >>>>>> against hot-unplug is not mandatory if it is known that they will never >>>>>> be hot-unplugged. They also say "MemoryRegions actually do not have >>>>>> their own reference count", but they actually do. They just will not be >>>>>> used unless their owners are not themselves. >>>>>> >>>>>> Refer to docs/devel/memory.rst as the single source of truth instead of >>>>>> maintaining duplicate descriptions of "owner". >>>>>> >>>>>> Clarify that owner may be missing >>>>>> >>>>>> --------------------------------- >>>>>> A memory region may not have an owner, and memory_region_ref() and >>>>>> memory_region_unref() do nothing for such. >>>>>> >>>>>> memory: Clarify owner must not call memory_region_ref() >>>>>> -------------------------------------------------------- >>>>>> >>>>>> The owner must not call this function as it results in a circular >>>>>> reference. >>>>>> >>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>>>> Reviewed-by: Peter Xu <peterx@redhat.com> >>>>>> --- >>>>>> include/exec/memory.h | 59 ++++++++++++++++++++++++--------------------------- >>>>>> 1 file changed, 28 insertions(+), 31 deletions(-) >>>>>> >>>>>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>>>>> index 9458e2801d50..ca247343f433 100644 >>>>>> --- a/include/exec/memory.h >>>>>> +++ b/include/exec/memory.h >>>>>> @@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s); >>>>>> * memory_region_add_subregion() to add subregions. >>>>>> * >>>>>> * @mr: the #MemoryRegion to be initialized >>>>>> - * @owner: the object that tracks the region's reference count >>>>>> + * @owner: the object that keeps the region alive >>>>>> * @name: used for debugging; not visible to the user or ABI >>>>>> * @size: size of the region; any subregions beyond this size will be clipped >>>>>> */ >>>>>> @@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr, >>>>>> uint64_t size); >>>>>> >>>>>> /** >>>>>> - * memory_region_ref: Add 1 to a memory region's reference count >>>>>> + * memory_region_ref: Add a reference to the owner of a memory region >>>>>> * >>>>>> - * Whenever memory regions are accessed outside the BQL, they need to be >>>>>> - * preserved against hot-unplug. MemoryRegions actually do not have their >>>>>> - * own reference count; they piggyback on a QOM object, their "owner". >>>>>> - * This function adds a reference to the owner. >>>>>> - * >>>>>> - * All MemoryRegions must have an owner if they can disappear, even if the >>>>>> - * device they belong to operates exclusively under the BQL. This is because >>>>>> - * the region could be returned at any time by memory_region_find, and this >>>>>> - * is usually under guest control. >>>>>> + * This function adds a reference to the owner of a memory region to keep the >>>>>> + * memory region alive. It does nothing if the owner is not present as a memory >>>>>> + * region without owner will never die. >>>>>> + * For references internal to the owner, use object_ref() instead to avoid a >>>>>> + * circular reference. >>>>> >>>>> Reading this again I'm still confused by this last sentence. Do you mean >>>>> references internal to the memory region should use object_ref on the memory >>>>> region or that other references to the owner should use object_ref on the >>>>> owner? This sentence is still not clear about that. >>>> >>>> Having two refcounts are definitely confusing.. especially IIRC all MRs' >>>> obj->free==NULL, so the MR's refcount isn't working. Dynamic MR's needs >>>> its g_free() on its own. >> >> We still have instance_finalize that will fire when the MR's refcount gets >> zero so it has its own use cases. >> >>>> >>>> I acked both patches, but maybe it could indeed be slightly better we drop >>>> this sentence, meanwhile in patch 2 we can drop the object_ref() too: it >>>> means for parent/child MRs that share the same owner, QEMU does nothing on >>>> the child MRs when add subregion, because it assumes the child MR will >>>> never go away when the parent is there who shares the owner. >>>> >>>> So maybe we try not to touch MR's refcount manually, but fix what can be >>>> problematic for owner->ref only. >>> >>> As an attached comment: I may have forgot some context on this issue, but I >>> still remember I used to have a patch that simply detach either parent or >>> child MR links when finalize(). It's here: >>> >>> https://lore.kernel.org/all/ZsenKpu1czQGYz7m@x1n/ >>> >>> I see this issue was there for a long time so maybe we want to fix it one >>> way or another. I don't strongly feel which way to go, but personally I >>> still prefer that way (I assume that can fix the same issue), and it >>> doesn't have MR's refcount involved at all, meanwhile I don't see an issue >>> yet with it.. >>> >> >> For this particular topic I have somewhat a strong opinion that we should >> care the two reference counters. >> >> Indeed, dealing with two reference counters is not fun, but sometimes it is >> necessary to do reference counting correctly. Your patch is to avoid >> reference counting for tracking dependencies among regions with the same >> owner, and it does so by ignoring the reference from container to subregion. > > I don't think so? When with that patch, container will reference subregion > the same way as others, which is to take a refcount on the owner. That > kept at least the refcount behavior consistent within memory_region_ref(). memory_region_ref() is not the only place that is responsible for reference management. memory_region_do_init() also calls object_property_add_child(), which in turn calls object_ref() to create a reference from the owner to the memory region. We should keep using object_ref() for object references originating from the owner. > > That patch removes the circular reference by always properly release the > circular reference due to sub-regioning internally. Calling memory_region_del_subregion() is not consistent with the direction of object references. A container references its subregion so the container should remove references to its subregion when appropriate. A subregion should not remove the reference its container holds. > >> >> I prefer to keep reference counting correct instead of having an additional >> ad-hoc measure that breaks reference relationships. > > Your patch added more complexity to me on refcounting, meanwhile it's also > not always "correct". It can boil down to how you define "correct" - if > you mean one should always boost a refcount somewhere if it references one > MR, then it's still not 100% correct at least when mr->owner==NULL. We > never yet did it alright, so to me it's a matter of working around current > sanitizer issue, and that's only about it yet so far. mr->owner == NULL is an exceptional case that we allow for performance reasons. We have luxury to spend more time in our case. > > Meanwhile I _think_ adding such complexity also means MR's finalize() will > be called in specific order when parent/child MRs belong to the same owner. > In my patch the order shouldn't matter, IIUC, which I preferred because > that reduces details that we may not care much (or I could have overlooked > why we need to care about it). Basically that's simpler to maintain to me, > but again, I don't feel strongly until someone would like / be able to > rework MR refcounting completely. We need to take care of the semantics of subregion. A container needs its subregions to satisfy accesses to the memory it represents. So it refers to the subregions, and the reference must keep the subregions alive; that's why we must keep the ordering. Regards, Akihiko Odaki
On Sat, Jan 11, 2025 at 01:15:24PM +0900, Akihiko Odaki wrote: > On 2025/01/11 0:18, Peter Xu wrote: > > On Fri, Jan 10, 2025 at 05:43:15PM +0900, Akihiko Odaki wrote: > > > On 2025/01/10 4:37, Peter Xu wrote: > > > > On Thu, Jan 09, 2025 at 02:29:21PM -0500, Peter Xu wrote: > > > > > On Thu, Jan 09, 2025 at 01:30:35PM +0100, BALATON Zoltan wrote: > > > > > > On Thu, 9 Jan 2025, Akihiko Odaki wrote: > > > > > > > Do not refer to "memory region's reference count" > > > > > > > ------------------------------------------------- > > > > > > > > > > > > > > Now MemoryRegions do have their own reference counts, but they will not > > > > > > > be used when their owners are not themselves. However, the documentation > > > > > > > of memory_region_ref() says it adds "1 to a memory region's reference > > > > > > > count", which is confusing. Avoid referring to "memory region's > > > > > > > reference count" and just say: "Add a reference to a memory region". > > > > > > > Make a similar change to memory_region_unref() too. > > > > > > > > > > > > > > Refer to docs/devel/memory.rst for "owner" > > > > > > > ------------------------------------------ > > > > > > > > > > > > > > memory_region_ref() and memory_region_unref() used to have their own > > > > > > > descriptions of "owner", but they are somewhat out-of-date and > > > > > > > misleading. > > > > > > > > > > > > > > In particular, they say "whenever memory regions are accessed outside > > > > > > > the BQL, they need to be preserved against hot-unplug", but protecting > > > > > > > against hot-unplug is not mandatory if it is known that they will never > > > > > > > be hot-unplugged. They also say "MemoryRegions actually do not have > > > > > > > their own reference count", but they actually do. They just will not be > > > > > > > used unless their owners are not themselves. > > > > > > > > > > > > > > Refer to docs/devel/memory.rst as the single source of truth instead of > > > > > > > maintaining duplicate descriptions of "owner". > > > > > > > > > > > > > > Clarify that owner may be missing > > > > > > > > > > > > > > --------------------------------- > > > > > > > A memory region may not have an owner, and memory_region_ref() and > > > > > > > memory_region_unref() do nothing for such. > > > > > > > > > > > > > > memory: Clarify owner must not call memory_region_ref() > > > > > > > -------------------------------------------------------- > > > > > > > > > > > > > > The owner must not call this function as it results in a circular > > > > > > > reference. > > > > > > > > > > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > > > > > Reviewed-by: Peter Xu <peterx@redhat.com> > > > > > > > --- > > > > > > > include/exec/memory.h | 59 ++++++++++++++++++++++++--------------------------- > > > > > > > 1 file changed, 28 insertions(+), 31 deletions(-) > > > > > > > > > > > > > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > > > > > > index 9458e2801d50..ca247343f433 100644 > > > > > > > --- a/include/exec/memory.h > > > > > > > +++ b/include/exec/memory.h > > > > > > > @@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s); > > > > > > > * memory_region_add_subregion() to add subregions. > > > > > > > * > > > > > > > * @mr: the #MemoryRegion to be initialized > > > > > > > - * @owner: the object that tracks the region's reference count > > > > > > > + * @owner: the object that keeps the region alive > > > > > > > * @name: used for debugging; not visible to the user or ABI > > > > > > > * @size: size of the region; any subregions beyond this size will be clipped > > > > > > > */ > > > > > > > @@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr, > > > > > > > uint64_t size); > > > > > > > > > > > > > > /** > > > > > > > - * memory_region_ref: Add 1 to a memory region's reference count > > > > > > > + * memory_region_ref: Add a reference to the owner of a memory region > > > > > > > * > > > > > > > - * Whenever memory regions are accessed outside the BQL, they need to be > > > > > > > - * preserved against hot-unplug. MemoryRegions actually do not have their > > > > > > > - * own reference count; they piggyback on a QOM object, their "owner". > > > > > > > - * This function adds a reference to the owner. > > > > > > > - * > > > > > > > - * All MemoryRegions must have an owner if they can disappear, even if the > > > > > > > - * device they belong to operates exclusively under the BQL. This is because > > > > > > > - * the region could be returned at any time by memory_region_find, and this > > > > > > > - * is usually under guest control. > > > > > > > + * This function adds a reference to the owner of a memory region to keep the > > > > > > > + * memory region alive. It does nothing if the owner is not present as a memory > > > > > > > + * region without owner will never die. > > > > > > > + * For references internal to the owner, use object_ref() instead to avoid a > > > > > > > + * circular reference. > > > > > > > > > > > > Reading this again I'm still confused by this last sentence. Do you mean > > > > > > references internal to the memory region should use object_ref on the memory > > > > > > region or that other references to the owner should use object_ref on the > > > > > > owner? This sentence is still not clear about that. > > > > > > > > > > Having two refcounts are definitely confusing.. especially IIRC all MRs' > > > > > obj->free==NULL, so the MR's refcount isn't working. Dynamic MR's needs > > > > > its g_free() on its own. > > > > > > We still have instance_finalize that will fire when the MR's refcount gets > > > zero so it has its own use cases. > > > > > > > > > > > > > I acked both patches, but maybe it could indeed be slightly better we drop > > > > > this sentence, meanwhile in patch 2 we can drop the object_ref() too: it > > > > > means for parent/child MRs that share the same owner, QEMU does nothing on > > > > > the child MRs when add subregion, because it assumes the child MR will > > > > > never go away when the parent is there who shares the owner. > > > > > > > > > > So maybe we try not to touch MR's refcount manually, but fix what can be > > > > > problematic for owner->ref only. > > > > > > > > As an attached comment: I may have forgot some context on this issue, but I > > > > still remember I used to have a patch that simply detach either parent or > > > > child MR links when finalize(). It's here: > > > > > > > > https://lore.kernel.org/all/ZsenKpu1czQGYz7m@x1n/ > > > > > > > > I see this issue was there for a long time so maybe we want to fix it one > > > > way or another. I don't strongly feel which way to go, but personally I > > > > still prefer that way (I assume that can fix the same issue), and it > > > > doesn't have MR's refcount involved at all, meanwhile I don't see an issue > > > > yet with it.. > > > > > > > > > > For this particular topic I have somewhat a strong opinion that we should > > > care the two reference counters. > > > > > > Indeed, dealing with two reference counters is not fun, but sometimes it is > > > necessary to do reference counting correctly. Your patch is to avoid > > > reference counting for tracking dependencies among regions with the same > > > owner, and it does so by ignoring the reference from container to subregion. > > > > I don't think so? When with that patch, container will reference subregion > > the same way as others, which is to take a refcount on the owner. That > > kept at least the refcount behavior consistent within memory_region_ref(). > > memory_region_ref() is not the only place that is responsible for reference > management. memory_region_do_init() also calls object_property_add_child(), > which in turn calls object_ref() to create a reference from the owner to the > memory region. We should keep using object_ref() for object references > originating from the owner. What I meant is we keep the refcount behavior consistent whenever a caller uses memory_region_ref(), so that we always stick with 1 refcount for 99% of users. Yes, we have that property link that holds the MR's own refcount, but that's the whole point of what I was trying to propose: I want to keep that internal as of now so I hope 99% of the people may not even be aware that such refcount existed. I hope people stick with using memory_region_ref() to refcount any MRs, then we only have 1 refcount which is the owner's. And that easily makes sense because the MR is part of the owner object as a struct field. What your patch did is extending that single usage out to normal memory_region_ref() callers, which I personally not prefer. So far if with my proposal, the property link will be a solo point where the owner says "ok I'm going to be destroyed, let's notify all the children properties" and that includes the MR. So that my hope was mr->refcount was sololy for that purpose, and if for that purpose we do not need to have that refcount to be bigger than 1 at all and it can actually be a boolean saying whether the link existed. I'm not saying that we need to change that to bool but I was trying to express my point, that I want to limit mr->refcount to minimum usage, and we stick with one refcount model by default, rather than spreading the "there're two refcounts" idea all over. I still think functionally they're identical but trying to stick with 1 refcount is definitely easier to follow. > > > > > That patch removes the circular reference by always properly release the > > circular reference due to sub-regioning internally. > > Calling memory_region_del_subregion() is not consistent with the direction > of object references. A container references its subregion so the container > should remove references to its subregion when appropriate. A subregion > should not remove the reference its container holds. Call memory_region_del_subregion() from the child says "I'm the child, now my owner is leaving, so I need to go". As simple as that. Any future reference to parent MR will keep working but not finding that child MR anymore. I think it's like when a device is unplugged, then the device needs to report to its bus it's gone. We don't have such limitation that because a device is under a bus so only the bus can proactively unplug it. The device can also decide to go or being unplugged by a human. It's pretty common thing that notifications can come from bottom, no matter why the child needs to go. In reality, I don't think this path is needed at all if all the owner properly does all subregion removals.. It's more of a safety belt. Because if there's a cross-device subregion, it means the owner must not have been released its last refcount anyway, so the owner (together with this child MR) must be alive. As long as we stick with "always ref owner's refcount" idea with my patch, this path (of addition of my patch) can only happen when the subregion is on top of the owner's own parent MR. It means the link is owned by the owner and if the owner (across QEMU's tree..) does proper removal of subregion of itself, my that path can be removed. > > > > > > > > > I prefer to keep reference counting correct instead of having an additional > > > ad-hoc measure that breaks reference relationships. > > > > Your patch added more complexity to me on refcounting, meanwhile it's also > > not always "correct". It can boil down to how you define "correct" - if > > you mean one should always boost a refcount somewhere if it references one > > MR, then it's still not 100% correct at least when mr->owner==NULL. We > > never yet did it alright, so to me it's a matter of working around current > > sanitizer issue, and that's only about it yet so far. > > mr->owner == NULL is an exceptional case that we allow for performance > reasons. We have luxury to spend more time in our case. Fair enough. We don't need to add that into the current discussion. But if you see, what you're doing with this patch is actually not needed either: when the owner of parent/child is the same, it's destined that the added refcount on top of mr->refcount won't help to me, because the parent needs to be alive first and that means the owner needs to be alive too. In general, I do think any refcount within the owner object (against any of its own MRs as part of struct fields) do not help but waste some atomic cycles, there's only one makes sense which is the owner<->MR property link that takes the MR->refcount so far. > > > > > Meanwhile I _think_ adding such complexity also means MR's finalize() will > > be called in specific order when parent/child MRs belong to the same owner. > > In my patch the order shouldn't matter, IIUC, which I preferred because > > that reduces details that we may not care much (or I could have overlooked > > why we need to care about it). Basically that's simpler to maintain to me, > > but again, I don't feel strongly until someone would like / be able to > > rework MR refcounting completely. > > We need to take care of the semantics of subregion. A container needs its > subregions to satisfy accesses to the memory it represents. So it refers to > the subregions, and the reference must keep the subregions alive; that's why > we must keep the ordering. Again, we're only talking about when owner is the same between parent/child. I don't think that order matters, then, because in that case as long as the parent MR is alive, owner and child MR must alive. To me, it's still easier we always take a refcount on the owner whenever we want to take a reference on a MR (except the only case of owner<->MR property link), it is still easy to understand when there's the struct field relationship between the owner and the MRs under it. When taking MR->refcount into the picture of memory_region_ref(), it's much harder to understand and it's much harder to define what is MR->refcount. So I mentioned that I can ACK this patch, but only because it looks like no one yet agree with me, and I also agree at least with you that we should still fix it first when there's no quorum. I'm ok merging this one because the changeset is small - worst case is whoever rework refcount can revert it. But again, that's not my preference, and I'm not convinced this is better.. Thanks,
On 2025/01/14 0:57, Peter Xu wrote: > On Sat, Jan 11, 2025 at 01:15:24PM +0900, Akihiko Odaki wrote: >> On 2025/01/11 0:18, Peter Xu wrote: >>> On Fri, Jan 10, 2025 at 05:43:15PM +0900, Akihiko Odaki wrote: >>>> On 2025/01/10 4:37, Peter Xu wrote: >>>>> On Thu, Jan 09, 2025 at 02:29:21PM -0500, Peter Xu wrote: >>>>>> On Thu, Jan 09, 2025 at 01:30:35PM +0100, BALATON Zoltan wrote: >>>>>>> On Thu, 9 Jan 2025, Akihiko Odaki wrote: >>>>>>>> Do not refer to "memory region's reference count" >>>>>>>> ------------------------------------------------- >>>>>>>> >>>>>>>> Now MemoryRegions do have their own reference counts, but they will not >>>>>>>> be used when their owners are not themselves. However, the documentation >>>>>>>> of memory_region_ref() says it adds "1 to a memory region's reference >>>>>>>> count", which is confusing. Avoid referring to "memory region's >>>>>>>> reference count" and just say: "Add a reference to a memory region". >>>>>>>> Make a similar change to memory_region_unref() too. >>>>>>>> >>>>>>>> Refer to docs/devel/memory.rst for "owner" >>>>>>>> ------------------------------------------ >>>>>>>> >>>>>>>> memory_region_ref() and memory_region_unref() used to have their own >>>>>>>> descriptions of "owner", but they are somewhat out-of-date and >>>>>>>> misleading. >>>>>>>> >>>>>>>> In particular, they say "whenever memory regions are accessed outside >>>>>>>> the BQL, they need to be preserved against hot-unplug", but protecting >>>>>>>> against hot-unplug is not mandatory if it is known that they will never >>>>>>>> be hot-unplugged. They also say "MemoryRegions actually do not have >>>>>>>> their own reference count", but they actually do. They just will not be >>>>>>>> used unless their owners are not themselves. >>>>>>>> >>>>>>>> Refer to docs/devel/memory.rst as the single source of truth instead of >>>>>>>> maintaining duplicate descriptions of "owner". >>>>>>>> >>>>>>>> Clarify that owner may be missing >>>>>>>> >>>>>>>> --------------------------------- >>>>>>>> A memory region may not have an owner, and memory_region_ref() and >>>>>>>> memory_region_unref() do nothing for such. >>>>>>>> >>>>>>>> memory: Clarify owner must not call memory_region_ref() >>>>>>>> -------------------------------------------------------- >>>>>>>> >>>>>>>> The owner must not call this function as it results in a circular >>>>>>>> reference. >>>>>>>> >>>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>>>>>> Reviewed-by: Peter Xu <peterx@redhat.com> >>>>>>>> --- >>>>>>>> include/exec/memory.h | 59 ++++++++++++++++++++++++--------------------------- >>>>>>>> 1 file changed, 28 insertions(+), 31 deletions(-) >>>>>>>> >>>>>>>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>>>>>>> index 9458e2801d50..ca247343f433 100644 >>>>>>>> --- a/include/exec/memory.h >>>>>>>> +++ b/include/exec/memory.h >>>>>>>> @@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s); >>>>>>>> * memory_region_add_subregion() to add subregions. >>>>>>>> * >>>>>>>> * @mr: the #MemoryRegion to be initialized >>>>>>>> - * @owner: the object that tracks the region's reference count >>>>>>>> + * @owner: the object that keeps the region alive >>>>>>>> * @name: used for debugging; not visible to the user or ABI >>>>>>>> * @size: size of the region; any subregions beyond this size will be clipped >>>>>>>> */ >>>>>>>> @@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr, >>>>>>>> uint64_t size); >>>>>>>> >>>>>>>> /** >>>>>>>> - * memory_region_ref: Add 1 to a memory region's reference count >>>>>>>> + * memory_region_ref: Add a reference to the owner of a memory region >>>>>>>> * >>>>>>>> - * Whenever memory regions are accessed outside the BQL, they need to be >>>>>>>> - * preserved against hot-unplug. MemoryRegions actually do not have their >>>>>>>> - * own reference count; they piggyback on a QOM object, their "owner". >>>>>>>> - * This function adds a reference to the owner. >>>>>>>> - * >>>>>>>> - * All MemoryRegions must have an owner if they can disappear, even if the >>>>>>>> - * device they belong to operates exclusively under the BQL. This is because >>>>>>>> - * the region could be returned at any time by memory_region_find, and this >>>>>>>> - * is usually under guest control. >>>>>>>> + * This function adds a reference to the owner of a memory region to keep the >>>>>>>> + * memory region alive. It does nothing if the owner is not present as a memory >>>>>>>> + * region without owner will never die. >>>>>>>> + * For references internal to the owner, use object_ref() instead to avoid a >>>>>>>> + * circular reference. >>>>>>> >>>>>>> Reading this again I'm still confused by this last sentence. Do you mean >>>>>>> references internal to the memory region should use object_ref on the memory >>>>>>> region or that other references to the owner should use object_ref on the >>>>>>> owner? This sentence is still not clear about that. >>>>>> >>>>>> Having two refcounts are definitely confusing.. especially IIRC all MRs' >>>>>> obj->free==NULL, so the MR's refcount isn't working. Dynamic MR's needs >>>>>> its g_free() on its own. >>>> >>>> We still have instance_finalize that will fire when the MR's refcount gets >>>> zero so it has its own use cases. >>>> >>>>>> >>>>>> I acked both patches, but maybe it could indeed be slightly better we drop >>>>>> this sentence, meanwhile in patch 2 we can drop the object_ref() too: it >>>>>> means for parent/child MRs that share the same owner, QEMU does nothing on >>>>>> the child MRs when add subregion, because it assumes the child MR will >>>>>> never go away when the parent is there who shares the owner. >>>>>> >>>>>> So maybe we try not to touch MR's refcount manually, but fix what can be >>>>>> problematic for owner->ref only. >>>>> >>>>> As an attached comment: I may have forgot some context on this issue, but I >>>>> still remember I used to have a patch that simply detach either parent or >>>>> child MR links when finalize(). It's here: >>>>> >>>>> https://lore.kernel.org/all/ZsenKpu1czQGYz7m@x1n/ >>>>> >>>>> I see this issue was there for a long time so maybe we want to fix it one >>>>> way or another. I don't strongly feel which way to go, but personally I >>>>> still prefer that way (I assume that can fix the same issue), and it >>>>> doesn't have MR's refcount involved at all, meanwhile I don't see an issue >>>>> yet with it.. >>>>> >>>> >>>> For this particular topic I have somewhat a strong opinion that we should >>>> care the two reference counters. >>>> >>>> Indeed, dealing with two reference counters is not fun, but sometimes it is >>>> necessary to do reference counting correctly. Your patch is to avoid >>>> reference counting for tracking dependencies among regions with the same >>>> owner, and it does so by ignoring the reference from container to subregion. >>> >>> I don't think so? When with that patch, container will reference subregion >>> the same way as others, which is to take a refcount on the owner. That >>> kept at least the refcount behavior consistent within memory_region_ref(). >> >> memory_region_ref() is not the only place that is responsible for reference >> management. memory_region_do_init() also calls object_property_add_child(), >> which in turn calls object_ref() to create a reference from the owner to the >> memory region. We should keep using object_ref() for object references >> originating from the owner. > > What I meant is we keep the refcount behavior consistent whenever a caller > uses memory_region_ref(), so that we always stick with 1 refcount for 99% > of users. > > Yes, we have that property link that holds the MR's own refcount, but > that's the whole point of what I was trying to propose: I want to keep that > internal as of now so I hope 99% of the people may not even be aware that > such refcount existed. I hope people stick with using memory_region_ref() > to refcount any MRs, then we only have 1 refcount which is the owner's. > And that easily makes sense because the MR is part of the owner object as a > struct field. > > What your patch did is extending that single usage out to normal > memory_region_ref() callers, which I personally not prefer. > > So far if with my proposal, the property link will be a solo point where > the owner says "ok I'm going to be destroyed, let's notify all the children > properties" and that includes the MR. So that my hope was mr->refcount was > sololy for that purpose, and if for that purpose we do not need to have > that refcount to be bigger than 1 at all and it can actually be a boolean > saying whether the link existed. I'm not saying that we need to change > that to bool but I was trying to express my point, that I want to limit > mr->refcount to minimum usage, and we stick with one refcount model by > default, rather than spreading the "there're two refcounts" idea all over. > I still think functionally they're identical but trying to stick with 1 > refcount is definitely easier to follow. The advantage of my patch is it does not require further knowledge beyond the two reference counters. Even now, if someone is going to change system/memory.c, they need to know there are two reference counters, and that is sufficient to understand my patch too. However, if we add some workaround specific to subregion, they will need to understand the specifics of subregion as well. > >> >>> >>> That patch removes the circular reference by always properly release the >>> circular reference due to sub-regioning internally. >> >> Calling memory_region_del_subregion() is not consistent with the direction >> of object references. A container references its subregion so the container >> should remove references to its subregion when appropriate. A subregion >> should not remove the reference its container holds. > > Call memory_region_del_subregion() from the child says "I'm the child, now > my owner is leaving, so I need to go". As simple as that. Any future > reference to parent MR will keep working but not finding that child MR > anymore. I think it's like when a device is unplugged, then the device > needs to report to its bus it's gone. We don't have such limitation that > because a device is under a bus so only the bus can proactively unplug it. > The device can also decide to go or being unplugged by a human. It's > pretty common thing that notifications can come from bottom, no matter why > the child needs to go. memory_region_finalize() is not a function to tell the owner is leaving, but the memory region itself is being destroyed. It should not happen when a container is still referencing it. That is also why it has memory_region_ref(subregion) in memory_region_update_container_subregions() and assert(!mr->container) in memory_region_finalize(). > > In reality, I don't think this path is needed at all if all the owner > properly does all subregion removals.. It's more of a safety belt. > Because if there's a cross-device subregion, it means the owner must not > have been released its last refcount anyway, so the owner (together with > this child MR) must be alive. As long as we stick with "always ref owner's > refcount" idea with my patch, this path (of addition of my patch) can only > happen when the subregion is on top of the owner's own parent MR. It means > the link is owned by the owner and if the owner (across QEMU's tree..) does > proper removal of subregion of itself, my that path can be removed. I really do not want owners to remove subregions manually. We should automate the finalization procedure as much as possible. > >> >>> >>>> >>>> I prefer to keep reference counting correct instead of having an additional >>>> ad-hoc measure that breaks reference relationships. >>> >>> Your patch added more complexity to me on refcounting, meanwhile it's also >>> not always "correct". It can boil down to how you define "correct" - if >>> you mean one should always boost a refcount somewhere if it references one >>> MR, then it's still not 100% correct at least when mr->owner==NULL. We >>> never yet did it alright, so to me it's a matter of working around current >>> sanitizer issue, and that's only about it yet so far. >> >> mr->owner == NULL is an exceptional case that we allow for performance >> reasons. We have luxury to spend more time in our case. > > Fair enough. We don't need to add that into the current discussion. > > But if you see, what you're doing with this patch is actually not needed > either: when the owner of parent/child is the same, it's destined that the > added refcount on top of mr->refcount won't help to me, because the parent > needs to be alive first and that means the owner needs to be alive too. In > general, I do think any refcount within the owner object (against any of > its own MRs as part of struct fields) do not help but waste some atomic > cycles, there's only one makes sense which is the owner<->MR property link > that takes the MR->refcount so far. Let's keep the discussion away from the subtle details. The waste of cycles is virtually imperceptible in the slow path we are caring now. > >> >>> >>> Meanwhile I _think_ adding such complexity also means MR's finalize() will >>> be called in specific order when parent/child MRs belong to the same owner. >>> In my patch the order shouldn't matter, IIUC, which I preferred because >>> that reduces details that we may not care much (or I could have overlooked >>> why we need to care about it). Basically that's simpler to maintain to me, >>> but again, I don't feel strongly until someone would like / be able to >>> rework MR refcounting completely. >> >> We need to take care of the semantics of subregion. A container needs its >> subregions to satisfy accesses to the memory it represents. So it refers to >> the subregions, and the reference must keep the subregions alive; that's why >> we must keep the ordering. > > Again, we're only talking about when owner is the same between > parent/child. I don't think that order matters, then, because in that case > as long as the parent MR is alive, owner and child MR must alive. > > To me, it's still easier we always take a refcount on the owner whenever we > want to take a reference on a MR (except the only case of owner<->MR > property link), it is still easy to understand when there's the struct > field relationship between the owner and the MRs under it. When taking > MR->refcount into the picture of memory_region_ref(), it's much harder to > understand and it's much harder to define what is MR->refcount. You should take MR->refcount into the picture of memory_region_ref(). Otherwise you will not understand why memory regions get finalized during the finalization of the owner. The understanding of the timing is essential to understand why your patch is necessary too. I have no idea to save people from the trouble of understanding the owner and memory region objects when dealing with references and finalization. > > So I mentioned that I can ACK this patch, but only because it looks like no > one yet agree with me, and I also agree at least with you that we should > still fix it first when there's no quorum. I'm ok merging this one because > the changeset is small - worst case is whoever rework refcount can revert > it. But again, that's not my preference, and I'm not convinced this is > better.. > > Thanks, >
On Tue, Jan 14, 2025 at 05:43:09PM +0900, Akihiko Odaki wrote: > memory_region_finalize() is not a function to tell the owner is leaving, but > the memory region itself is being destroyed. It is when the lifecycle of the MR is the same as the owner. That holds true I suppose if without this patch, and that's why I don't prefer this patch because it makes that part more complicated. > It should not happen when a container is still referencing it. That is > also why it has memory_region_ref(subregion) in > memory_region_update_container_subregions() and assert(!mr->container) in > memory_region_finalize(). Again, the line I added was sololy for what you said "automation" elsewhere and only should work within MR-links within the same owner. Otherwise anyone referencing the MR would hold the owner ref then this finalize() will never happen. Now, if I could go back to your original purpose of this work, quotting from your cover letter: > I saw various sanitizer errors when running check-qtest-ppc64. While > I could just turn off sanitizers, I decided to tackle them this time. > > Unfortunately, GLib versions older than 2.81.0 do not free test data in > some cases so some sanitizer errors remain. All sanitizer errors will be > gone with this patch series combined with the following change for GLib: > https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120 Is check-qtest-ppc64 the only one that will trigger this issue? Does it mean that most of the devices will do proper removal of device-owned subregions (hence, not prone to circular reference of owner refcount) except some devices in ppc64?
On Tue, 14 Jan 2025 at 17:02, Peter Xu <peterx@redhat.com> wrote: > > On Tue, Jan 14, 2025 at 05:43:09PM +0900, Akihiko Odaki wrote: > > memory_region_finalize() is not a function to tell the owner is leaving, but > > the memory region itself is being destroyed. > > It is when the lifecycle of the MR is the same as the owner. That holds > true I suppose if without this patch, and that's why I don't prefer this > patch because it makes that part more complicated. > > > It should not happen when a container is still referencing it. That is > > also why it has memory_region_ref(subregion) in > > memory_region_update_container_subregions() and assert(!mr->container) in > > memory_region_finalize(). > > Again, the line I added was sololy for what you said "automation" elsewhere > and only should work within MR-links within the same owner. Otherwise > anyone referencing the MR would hold the owner ref then this finalize() > will never happen. > > Now, if I could go back to your original purpose of this work, quotting > from your cover letter: > > > I saw various sanitizer errors when running check-qtest-ppc64. While > > I could just turn off sanitizers, I decided to tackle them this time. > > > > Unfortunately, GLib versions older than 2.81.0 do not free test data in > > some cases so some sanitizer errors remain. All sanitizer errors will be > > gone with this patch series combined with the following change for GLib: > > https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120 > > Is check-qtest-ppc64 the only one that will trigger this issue? Does it > mean that most of the devices will do proper removal of device-owned > subregions (hence, not prone to circular reference of owner refcount) > except some devices in ppc64? There's at least one test in the arm qtests that will hit this. I suspect that you'll find that most architectures except x86 (where we don't have models of complex SoCs and the few machines we do have tend to be old code that is less QOMified) will hit similar issues. I think there's a general issue here, this isn't just "some particular ppc device is wrongly coded". -- PMM
On Tue, Jan 14, 2025 at 05:42:57PM +0000, Peter Maydell wrote: > On Tue, 14 Jan 2025 at 17:02, Peter Xu <peterx@redhat.com> wrote: > > > > On Tue, Jan 14, 2025 at 05:43:09PM +0900, Akihiko Odaki wrote: > > > memory_region_finalize() is not a function to tell the owner is leaving, but > > > the memory region itself is being destroyed. > > > > It is when the lifecycle of the MR is the same as the owner. That holds > > true I suppose if without this patch, and that's why I don't prefer this > > patch because it makes that part more complicated. > > > > > It should not happen when a container is still referencing it. That is > > > also why it has memory_region_ref(subregion) in > > > memory_region_update_container_subregions() and assert(!mr->container) in > > > memory_region_finalize(). > > > > Again, the line I added was sololy for what you said "automation" elsewhere > > and only should work within MR-links within the same owner. Otherwise > > anyone referencing the MR would hold the owner ref then this finalize() > > will never happen. > > > > Now, if I could go back to your original purpose of this work, quotting > > from your cover letter: > > > > > I saw various sanitizer errors when running check-qtest-ppc64. While > > > I could just turn off sanitizers, I decided to tackle them this time. > > > > > > Unfortunately, GLib versions older than 2.81.0 do not free test data in > > > some cases so some sanitizer errors remain. All sanitizer errors will be > > > gone with this patch series combined with the following change for GLib: > > > https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120 > > > > Is check-qtest-ppc64 the only one that will trigger this issue? Does it > > mean that most of the devices will do proper removal of device-owned > > subregions (hence, not prone to circular reference of owner refcount) > > except some devices in ppc64? > > There's at least one test in the arm qtests that will hit this. > I suspect that you'll find that most architectures except x86 > (where we don't have models of complex SoCs and the few > machines we do have tend to be old code that is less QOMified) > will hit similar issues. I think there's a general issue here, > this isn't just "some particular ppc device is wrongly coded". I see. Do you know how many of them would be important memory leaks that we should fix immediately? I mean, we have known memory leaks in QEMU in many places I assume. I am curious how important this problem is, and whether such would justify a memory API change that is not reaching a quorum state (and, imho, add complexity to memory core and of course that spreads to x86 too even if it was not affected) to be merged. Or perhaps we can fix the important ones first from the device model directly instead. It's not new to me that QEMU can leave some memory allocated for the whole lifecycle of the process. E.g. I just worked on something for migration that we could have UAF on migration object. I tried to provide a core fix via QOM singleton but unfortunately that didn't yet got accepted, so migration still face such UAF. It was not accepted because of some reasons and reviewer concerns, so I suppose that's fair that until we reach a consensus on an acceptable and clean general solution, we leave that issue be there if it's a corner case anyway - in migration that was the case. For this specific case, my current understanding is the important leaks are where it can e.g. get devices frequently plugged and unplugged with can cause QEMU to bloat till host OOM. For those cases I wonder whether (even if we want to provide a global sulution... but while before it settles..) we could fix them first by correctly detach the subregions just like what x86 does. Thanks,
On 2025/01/15 2:02, Peter Xu wrote: > On Tue, Jan 14, 2025 at 05:43:09PM +0900, Akihiko Odaki wrote: >> memory_region_finalize() is not a function to tell the owner is leaving, but >> the memory region itself is being destroyed. > > It is when the lifecycle of the MR is the same as the owner. That holds > true I suppose if without this patch, and that's why I don't prefer this > patch because it makes that part more complicated. The lifecycle of the MR is not the same as the owner. The MR gets finalized during the finalization of the owner, and the owner is still alive at the moment. It is something you should always care when having a child object. > >> It should not happen when a container is still referencing it. That is >> also why it has memory_region_ref(subregion) in >> memory_region_update_container_subregions() and assert(!mr->container) in >> memory_region_finalize(). > > Again, the line I added was sololy for what you said "automation" elsewhere > and only should work within MR-links within the same owner. Otherwise > anyone referencing the MR would hold the owner ref then this finalize() > will never happen. > > Now, if I could go back to your original purpose of this work, quotting > from your cover letter: > >> I saw various sanitizer errors when running check-qtest-ppc64. While >> I could just turn off sanitizers, I decided to tackle them this time. >> >> Unfortunately, GLib versions older than 2.81.0 do not free test data in >> some cases so some sanitizer errors remain. All sanitizer errors will be >> gone with this patch series combined with the following change for GLib: >> https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120 > > Is check-qtest-ppc64 the only one that will trigger this issue? Does it > mean that most of the devices will do proper removal of device-owned > subregions (hence, not prone to circular reference of owner refcount) > except some devices in ppc64? > Searching for memory_region_add_subregion() gives 1078 instances where there are 142 instances of memory_region_del_subregion(). This is a rough estimate but there are potentially 936 instances of subregions without explicit deletion. For example, hw/audio/intel-hda.c adds subregions immediately after their containers never deletes the subregions. I think that's fine because their lifetimes are obvious with reference counters.
On Wed, Jan 15, 2025 at 01:46:29PM +0900, Akihiko Odaki wrote: > On 2025/01/15 2:02, Peter Xu wrote: > > On Tue, Jan 14, 2025 at 05:43:09PM +0900, Akihiko Odaki wrote: > > > memory_region_finalize() is not a function to tell the owner is leaving, but > > > the memory region itself is being destroyed. > > > > It is when the lifecycle of the MR is the same as the owner. That holds > > true I suppose if without this patch, and that's why I don't prefer this > > patch because it makes that part more complicated. > > The lifecycle of the MR is not the same as the owner. The MR gets finalized > during the finalization of the owner, and the owner is still alive at the > moment. It is something you should always care when having a child object. What is the benefit of having such explicit layering of different lifecycle between the owner and the MRs that it owns? To ask in another way, what's the functional benefit that we order the destruction of MRs within the same owner, paying that with explicit two refcounts concept in memory core? AFAICT, that's the only purpose MR->refcount is servicing for in this patchset besides the property link. Currently, memory_region_ref() takes the refcount _only_ from the host. Considering that's the only memory API to take a reference on a MR, it kind of implies to everyone that the MR and the owner shares the lifetime. In reality, it's not 100% shared indeed, but almost. We even have those document for dynamic MRs to make sure that is true even there. Then it's about the "virtual lifecycle" which triggers a finalize(), or "real lifecycle" which triggers a free() that may make a difference to a MR. And that's the part on whether we should try to not expose too much at all on these. I want to keep the concept simple if possible that we stick with sharing lifetime between owner and all MRs underneath. I want to see whether we can avoid complicating that part. I can see why you want to clearly separate the lifetimes, because it's cleaner to you. But IMHO we already made a decision from that starting from when memory_region_ref() does not take MR->refcount, otherwise you should at least need something like this to make the lifecycle completely separate in your this patch: diff --git a/system/memory.c b/system/memory.c index b17b5538ff..d4b88c389a 100644 --- a/system/memory.c +++ b/system/memory.c @@ -1843,15 +1843,23 @@ void memory_region_ref(MemoryRegion *mr) * Memory regions without an owner are supposed to never go away; * we do not ref/unref them because it slows down DMA sensibly. */ - if (mr && mr->owner) { - object_ref(mr->owner); + if (mr) { + /* The MR has its own lifecycle.. even if in most cases, virtually */ + object_ref(mr); + if (mr->owner) { + object_ref(mr->owner); + } } } void memory_region_unref(MemoryRegion *mr) { - if (mr && mr->owner) { - object_unref(mr->owner); + if (mr) { + /* The MR has its own lifecycle.. even if in most cases, virtually */ + object_unref(mr); + if (mr->owner) { + object_unref(mr->owner); + } } } To me, QEMU already went the other way. So I sincerely don't know how that extra mr->refcount usage it could bring us. It only makes it harder to understand to me. > > > > > > It should not happen when a container is still referencing it. That is > > > also why it has memory_region_ref(subregion) in > > > memory_region_update_container_subregions() and assert(!mr->container) in > > > memory_region_finalize(). > > > > Again, the line I added was sololy for what you said "automation" elsewhere > > and only should work within MR-links within the same owner. Otherwise > > anyone referencing the MR would hold the owner ref then this finalize() > > will never happen. > > > > Now, if I could go back to your original purpose of this work, quotting > > from your cover letter: > > > > > I saw various sanitizer errors when running check-qtest-ppc64. While > > > I could just turn off sanitizers, I decided to tackle them this time. > > > > > > Unfortunately, GLib versions older than 2.81.0 do not free test data in > > > some cases so some sanitizer errors remain. All sanitizer errors will be > > > gone with this patch series combined with the following change for GLib: > > > https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120 > > > > Is check-qtest-ppc64 the only one that will trigger this issue? Does it > > mean that most of the devices will do proper removal of device-owned > > subregions (hence, not prone to circular reference of owner refcount) > > except some devices in ppc64? > > > > Searching for memory_region_add_subregion() gives 1078 instances where there > are 142 instances of memory_region_del_subregion(). This is a rough estimate > but there are potentially 936 instances of subregions without explicit > deletion. > > For example, hw/audio/intel-hda.c adds subregions immediately after their > containers never deletes the subregions. I think that's fine because their > lifetimes are obvious with reference counters. OK, let's try to figure out a best way to move forward then. Let me try to summarize the two approaches so far. So in general I think I don't prefer this patch because this patch is kind of in the middle of something. It neither provides 100% separation of MR lifecycle: as discussed above, on not referencing MR->refcount on memory_region_ref/unref at least yet so far together in this patch, but suddenly started considering it in MR links. To me, that's abuse if ordering of such finalize() is not justified. Nor it provides best efficiency: needing to take a MR->refcount when linking two MRs, even if we essentially don't need to guarded by the fact that owner must exist already, which must hold true anyway for QEMU to work so far. What I think the best is we either go one way or another: either we make MR lifecycle clearly separate, or we make it clearly efficient (meanwhile we still keep the concept easy, and we at least try to always stick with one refcount which is easier to maintain too). IMHO that's what the other older patch does (plus my fixup squashed in): https://lore.kernel.org/all/ZsenKpu1czQGYz7m@x1n/ That avoids taking a refcount for internal MRs, always stick with owner shares the same lifecycle with MRs, just like the same assumption we have already had in memory_region_ref(). The bad side effect is we need something slightly hackish in mr finalize(), but we can provide some better doc, and keep the comlexity there only (which I think is better than always having two refcounts all over). If we worry about removal of that container assertion, we could assert instead on the owner. I've attached a slightly modified full version of such alternative patch below, with the best comment I see suite. diff --git a/system/memory.c b/system/memory.c index b17b5538ff..7b2d91ca6b 100644 --- a/system/memory.c +++ b/system/memory.c @@ -1803,7 +1803,6 @@ static void memory_region_finalize(Object *obj) { MemoryRegion *mr = MEMORY_REGION(obj); - assert(!mr->container); /* We know the region is not visible in any address space (it * does not have a container and cannot be a root either because @@ -1813,6 +1812,17 @@ static void memory_region_finalize(Object *obj) */ mr->enabled = false; memory_region_transaction_begin(); + if (mr->container) { + /* + * If this happens, it must be when MRs share the same owner, + * because only share-owner-ed links doesn't take a refcount. In + * this specific case, we allow random order of finalize() on the + * MRs the owner owns, so it's possible the child finalize()s + * before a parent. When it happens, unlink from the child. + */ + assert(mr->container->owner == mr->owner); + memory_region_del_subregion(mr->container, mr); + } while (!QTAILQ_EMPTY(&mr->subregions)) { MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions); memory_region_del_subregion(mr, subregion); @@ -2644,7 +2654,15 @@ static void memory_region_update_container_subregions(MemoryRegion *subregion) memory_region_transaction_begin(); - memory_region_ref(subregion); + if (mr->owner != subregion->owner) { + /* + * MRs always have the same lifecycle of its owner, so that when + * adding a subregion that shares the same owner of the parent, we + * don't need any refcounting, because the two MRs share the + * lifecycle with owner, so they share between each other too. + */ + memory_region_ref(subregion); + } QTAILQ_FOREACH(other, &mr->subregions, subregions_link) { if (subregion->priority >= other->priority) { QTAILQ_INSERT_BEFORE(other, subregion, subregions_link); @@ -2702,7 +2720,10 @@ void memory_region_del_subregion(MemoryRegion *mr, assert(alias->mapped_via_alias >= 0); } QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link); - memory_region_unref(subregion); + /* See the corresponding comment in add subregion path */ + if (mr->owner != subregion->owner) { + memory_region_unref(subregion); + } memory_region_update_pending |= mr->enabled && subregion->enabled; memory_region_transaction_commit(); }
On 2025/01/15 22:43, Peter Xu wrote: > On Wed, Jan 15, 2025 at 01:46:29PM +0900, Akihiko Odaki wrote: >> On 2025/01/15 2:02, Peter Xu wrote: >>> On Tue, Jan 14, 2025 at 05:43:09PM +0900, Akihiko Odaki wrote: >>>> memory_region_finalize() is not a function to tell the owner is leaving, but >>>> the memory region itself is being destroyed. >>> >>> It is when the lifecycle of the MR is the same as the owner. That holds >>> true I suppose if without this patch, and that's why I don't prefer this >>> patch because it makes that part more complicated. >> >> The lifecycle of the MR is not the same as the owner. The MR gets finalized >> during the finalization of the owner, and the owner is still alive at the >> moment. It is something you should always care when having a child object. > > What is the benefit of having such explicit layering of different lifecycle > between the owner and the MRs that it owns? > > To ask in another way, what's the functional benefit that we order the > destruction of MRs within the same owner, paying that with explicit two > refcounts concept in memory core? > > AFAICT, that's the only purpose MR->refcount is servicing for in this > patchset besides the property link. > > Currently, memory_region_ref() takes the refcount _only_ from the host. > Considering that's the only memory API to take a reference on a MR, it kind > of implies to everyone that the MR and the owner shares the lifetime. > > In reality, it's not 100% shared indeed, but almost. We even have those > document for dynamic MRs to make sure that is true even there. > > Then it's about the "virtual lifecycle" which triggers a finalize(), or > "real lifecycle" which triggers a free() that may make a difference to a > MR. And that's the part on whether we should try to not expose too much at > all on these. I want to keep the concept simple if possible that we stick > with sharing lifetime between owner and all MRs underneath. I want to see > whether we can avoid complicating that part. I would rather avoid virtual or real lifecycles notions because it's more than free(). Memory regions constructed with functions like memory_region_init_io() and memory_region_init_ram_ptr() requires the owner to retain the backend resource to keep functioning. In other words, the memory region refers to the owner, and that is no different from other kind of references. The uniqueness of this relationship is that the owner also refers to the memory region. Memory regions avoid a circular reference by omitting the reference from them to the owner and instruct others to refer to the owner instead. > > I can see why you want to clearly separate the lifetimes, because it's > cleaner to you. But IMHO we already made a decision from that starting > from when memory_region_ref() does not take MR->refcount, otherwise you > should at least need something like this to make the lifecycle completely > separate in your this patch: > > diff --git a/system/memory.c b/system/memory.c > index b17b5538ff..d4b88c389a 100644 > --- a/system/memory.c > +++ b/system/memory.c > @@ -1843,15 +1843,23 @@ void memory_region_ref(MemoryRegion *mr) > * Memory regions without an owner are supposed to never go away; > * we do not ref/unref them because it slows down DMA sensibly. > */ > - if (mr && mr->owner) { > - object_ref(mr->owner); > + if (mr) { > + /* The MR has its own lifecycle.. even if in most cases, virtually */ > + object_ref(mr); > + if (mr->owner) { > + object_ref(mr->owner); > + } > } > } > > void memory_region_unref(MemoryRegion *mr) > { > - if (mr && mr->owner) { > - object_unref(mr->owner); > + if (mr) { > + /* The MR has its own lifecycle.. even if in most cases, virtually */ > + object_unref(mr); > + if (mr->owner) { > + object_unref(mr->owner); > + } > } > } > > To me, QEMU already went the other way. So I sincerely don't know how that > extra mr->refcount usage it could bring us. It only makes it harder to > understand to me. The owner refers to the memory region in turn so it is fine omitting object_ref(mr). If you draw an object graph that originates from the referrer, you can still reach the memory region. That is not true for your patch; you cannot reach to the subregion from the container. The separate lifetimes still matter even with your patch. In a hypothetical world that the lifetime of owner and memory regions completely match, the ordering of finalization of memory regions owned by one object simply does not happen because they occur simultaneously. It is simply not true, and even your patch does not make sense in such a hypothetical world. > >> >>> >>>> It should not happen when a container is still referencing it. That is >>>> also why it has memory_region_ref(subregion) in >>>> memory_region_update_container_subregions() and assert(!mr->container) in >>>> memory_region_finalize(). >>> >>> Again, the line I added was sololy for what you said "automation" elsewhere >>> and only should work within MR-links within the same owner. Otherwise >>> anyone referencing the MR would hold the owner ref then this finalize() >>> will never happen. >>> >>> Now, if I could go back to your original purpose of this work, quotting >>> from your cover letter: >>> >>>> I saw various sanitizer errors when running check-qtest-ppc64. While >>>> I could just turn off sanitizers, I decided to tackle them this time. >>>> >>>> Unfortunately, GLib versions older than 2.81.0 do not free test data in >>>> some cases so some sanitizer errors remain. All sanitizer errors will be >>>> gone with this patch series combined with the following change for GLib: >>>> https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120 >>> >>> Is check-qtest-ppc64 the only one that will trigger this issue? Does it >>> mean that most of the devices will do proper removal of device-owned >>> subregions (hence, not prone to circular reference of owner refcount) >>> except some devices in ppc64? >>> >> >> Searching for memory_region_add_subregion() gives 1078 instances where there >> are 142 instances of memory_region_del_subregion(). This is a rough estimate >> but there are potentially 936 instances of subregions without explicit >> deletion. >> >> For example, hw/audio/intel-hda.c adds subregions immediately after their >> containers never deletes the subregions. I think that's fine because their >> lifetimes are obvious with reference counters. > > OK, let's try to figure out a best way to move forward then. > > Let me try to summarize the two approaches so far. > > So in general I think I don't prefer this patch because this patch is kind > of in the middle of something. > > It neither provides 100% separation of MR lifecycle: as discussed above, on > not referencing MR->refcount on memory_region_ref/unref at least yet so far > together in this patch, but suddenly started considering it in MR links. > To me, that's abuse if ordering of such finalize() is not justified. > > Nor it provides best efficiency: needing to take a MR->refcount when > linking two MRs, even if we essentially don't need to guarded by the fact > that owner must exist already, which must hold true anyway for QEMU to work > so far. > > What I think the best is we either go one way or another: either we make MR > lifecycle clearly separate, or we make it clearly efficient (meanwhile we > still keep the concept easy, and we at least try to always stick with one > refcount which is easier to maintain too). > > IMHO that's what the other older patch does (plus my fixup squashed in): > > https://lore.kernel.org/all/ZsenKpu1czQGYz7m@x1n/ > > That avoids taking a refcount for internal MRs, always stick with owner > shares the same lifecycle with MRs, just like the same assumption we have > already had in memory_region_ref(). The bad side effect is we need > something slightly hackish in mr finalize(), but we can provide some better > doc, and keep the comlexity there only (which I think is better than always > having two refcounts all over). Again, please forget about efficiency. It does not matter and makes noises in our thoughts. > > If we worry about removal of that container assertion, we could assert > instead on the owner. I've attached a slightly modified full version of > such alternative patch below, with the best comment I see suite. This is better as it tells the lifetimes of memory regions need to be dealt with, but why don't you deal them with reference counters in that case? Reference counters are tools specifically designed for this. > > diff --git a/system/memory.c b/system/memory.c > index b17b5538ff..7b2d91ca6b 100644 > --- a/system/memory.c > +++ b/system/memory.c > @@ -1803,7 +1803,6 @@ static void memory_region_finalize(Object *obj) > { > MemoryRegion *mr = MEMORY_REGION(obj); > > - assert(!mr->container); > > /* We know the region is not visible in any address space (it > * does not have a container and cannot be a root either because > @@ -1813,6 +1812,17 @@ static void memory_region_finalize(Object *obj) > */ > mr->enabled = false; > memory_region_transaction_begin(); > + if (mr->container) { > + /* > + * If this happens, it must be when MRs share the same owner, > + * because only share-owner-ed links doesn't take a refcount. In > + * this specific case, we allow random order of finalize() on the > + * MRs the owner owns, so it's possible the child finalize()s > + * before a parent. When it happens, unlink from the child. > + */ > + assert(mr->container->owner == mr->owner); > + memory_region_del_subregion(mr->container, mr); > + } > while (!QTAILQ_EMPTY(&mr->subregions)) { > MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions); > memory_region_del_subregion(mr, subregion); > @@ -2644,7 +2654,15 @@ static void memory_region_update_container_subregions(MemoryRegion *subregion) > > memory_region_transaction_begin(); > > - memory_region_ref(subregion); > + if (mr->owner != subregion->owner) { > + /* > + * MRs always have the same lifecycle of its owner, so that when > + * adding a subregion that shares the same owner of the parent, we > + * don't need any refcounting, because the two MRs share the > + * lifecycle with owner, so they share between each other too. > + */ > + memory_region_ref(subregion); > + } > QTAILQ_FOREACH(other, &mr->subregions, subregions_link) { > if (subregion->priority >= other->priority) { > QTAILQ_INSERT_BEFORE(other, subregion, subregions_link); > @@ -2702,7 +2720,10 @@ void memory_region_del_subregion(MemoryRegion *mr, > assert(alias->mapped_via_alias >= 0); > } > QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link); > - memory_region_unref(subregion); > + /* See the corresponding comment in add subregion path */ > + if (mr->owner != subregion->owner) { > + memory_region_unref(subregion); > + } > memory_region_update_pending |= mr->enabled && subregion->enabled; > memory_region_transaction_commit(); > }
On Wed, Jan 15, 2025 at 11:54:56PM +0900, Akihiko Odaki wrote: > On 2025/01/15 22:43, Peter Xu wrote: > > On Wed, Jan 15, 2025 at 01:46:29PM +0900, Akihiko Odaki wrote: > > > On 2025/01/15 2:02, Peter Xu wrote: > > > > On Tue, Jan 14, 2025 at 05:43:09PM +0900, Akihiko Odaki wrote: > > > > > memory_region_finalize() is not a function to tell the owner is leaving, but > > > > > the memory region itself is being destroyed. > > > > > > > > It is when the lifecycle of the MR is the same as the owner. That holds > > > > true I suppose if without this patch, and that's why I don't prefer this > > > > patch because it makes that part more complicated. > > > > > > The lifecycle of the MR is not the same as the owner. The MR gets finalized > > > during the finalization of the owner, and the owner is still alive at the > > > moment. It is something you should always care when having a child object. > > > > What is the benefit of having such explicit layering of different lifecycle > > between the owner and the MRs that it owns? > > > > To ask in another way, what's the functional benefit that we order the > > destruction of MRs within the same owner, paying that with explicit two > > refcounts concept in memory core? [1] > > > > AFAICT, that's the only purpose MR->refcount is servicing for in this > > patchset besides the property link. > > > > Currently, memory_region_ref() takes the refcount _only_ from the host. > > Considering that's the only memory API to take a reference on a MR, it kind > > of implies to everyone that the MR and the owner shares the lifetime. > > > > In reality, it's not 100% shared indeed, but almost. We even have those > > document for dynamic MRs to make sure that is true even there. > > > > Then it's about the "virtual lifecycle" which triggers a finalize(), or > > "real lifecycle" which triggers a free() that may make a difference to a > > MR. And that's the part on whether we should try to not expose too much at > > all on these. I want to keep the concept simple if possible that we stick > > with sharing lifetime between owner and all MRs underneath. I want to see > > whether we can avoid complicating that part. > > I would rather avoid virtual or real lifecycles notions because it's more > than free(). Memory regions constructed with functions like > memory_region_init_io() and memory_region_init_ram_ptr() requires the owner > to retain the backend resource to keep functioning. In other words, the > memory region refers to the owner, and that is no different from other kind > of references. > > The uniqueness of this relationship is that the owner also refers to the > memory region. Memory regions avoid a circular reference by omitting the > reference from them to the owner and instruct others to refer to the owner > instead. > > > > > I can see why you want to clearly separate the lifetimes, because it's > > cleaner to you. But IMHO we already made a decision from that starting > > from when memory_region_ref() does not take MR->refcount, otherwise you > > should at least need something like this to make the lifecycle completely > > separate in your this patch: > > > diff --git a/system/memory.c b/system/memory.c > > index b17b5538ff..d4b88c389a 100644 > > --- a/system/memory.c > > +++ b/system/memory.c > > @@ -1843,15 +1843,23 @@ void memory_region_ref(MemoryRegion *mr) > > * Memory regions without an owner are supposed to never go away; > > * we do not ref/unref them because it slows down DMA sensibly. > > */ > > - if (mr && mr->owner) { > > - object_ref(mr->owner); > > + if (mr) { > > + /* The MR has its own lifecycle.. even if in most cases, virtually */ > > + object_ref(mr); > > + if (mr->owner) { > > + object_ref(mr->owner); > > + } > > } > > } > > void memory_region_unref(MemoryRegion *mr) > > { > > - if (mr && mr->owner) { > > - object_unref(mr->owner); > > + if (mr) { > > + /* The MR has its own lifecycle.. even if in most cases, virtually */ > > + object_unref(mr); > > + if (mr->owner) { > > + object_unref(mr->owner); > > + } > > } > > } > > > > To me, QEMU already went the other way. So I sincerely don't know how that > > extra mr->refcount usage it could bring us. It only makes it harder to > > understand to me. > > The owner refers to the memory region in turn so it is fine omitting > object_ref(mr). So you decided to "sometimes" take mr->refcount because it needs to, then "sometimes" don't take mr->refcounts because it doesn't need to.. Normally such complexity is ok, but to me it's ok only when it services, for example, a major performance improvements, so that it's justified to add complexity. The pay is done whoever going to maintain this code. In this case, no, I don't yet see how important this idea is yet to introduce such difference into mr refcounts, which is already complicated as hell.. We're paying such complexity with some "technical cleanest", while when with different treatment of mr->refcount in different context, it isn't that clean either. > If you draw an object graph that originates from the > referrer, you can still reach the memory region. That is not true for your > patch; you cannot reach to the subregion from the container. > > The separate lifetimes still matter even with your patch. In a hypothetical > world that the lifetime of owner and memory regions completely match, the > ordering of finalization of memory regions owned by one object simply does > not happen because they occur simultaneously. It is simply not true, and > even your patch does not make sense in such a hypothetical world. I hope that's obvious goal since start, yes, that patch will make finalize() in any order works for MRs under the same owner, as I don't know why that order matters.. taking that chance of almost still sticking with one refcount. I suppose you finally need to answer my above question [1] to say whether it makes sense. To me, it doesn't make sense only if there's a functional difference on that order of finalize(). > > > > > > > > > > > > > > > It should not happen when a container is still referencing it. That is > > > > > also why it has memory_region_ref(subregion) in > > > > > memory_region_update_container_subregions() and assert(!mr->container) in > > > > > memory_region_finalize(). > > > > > > > > Again, the line I added was sololy for what you said "automation" elsewhere > > > > and only should work within MR-links within the same owner. Otherwise > > > > anyone referencing the MR would hold the owner ref then this finalize() > > > > will never happen. > > > > > > > > Now, if I could go back to your original purpose of this work, quotting > > > > from your cover letter: > > > > > > > > > I saw various sanitizer errors when running check-qtest-ppc64. While > > > > > I could just turn off sanitizers, I decided to tackle them this time. > > > > > > > > > > Unfortunately, GLib versions older than 2.81.0 do not free test data in > > > > > some cases so some sanitizer errors remain. All sanitizer errors will be > > > > > gone with this patch series combined with the following change for GLib: > > > > > https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120 > > > > > > > > Is check-qtest-ppc64 the only one that will trigger this issue? Does it > > > > mean that most of the devices will do proper removal of device-owned > > > > subregions (hence, not prone to circular reference of owner refcount) > > > > except some devices in ppc64? > > > > > > > > > > Searching for memory_region_add_subregion() gives 1078 instances where there > > > are 142 instances of memory_region_del_subregion(). This is a rough estimate > > > but there are potentially 936 instances of subregions without explicit > > > deletion. > > > > > > For example, hw/audio/intel-hda.c adds subregions immediately after their > > > containers never deletes the subregions. I think that's fine because their > > > lifetimes are obvious with reference counters. > > > > OK, let's try to figure out a best way to move forward then. > > > > Let me try to summarize the two approaches so far. > > > > So in general I think I don't prefer this patch because this patch is kind > > of in the middle of something. > > > > It neither provides 100% separation of MR lifecycle: as discussed above, on > > not referencing MR->refcount on memory_region_ref/unref at least yet so far > > together in this patch, but suddenly started considering it in MR links. > > To me, that's abuse if ordering of such finalize() is not justified. > > > > Nor it provides best efficiency: needing to take a MR->refcount when > > linking two MRs, even if we essentially don't need to guarded by the fact > > that owner must exist already, which must hold true anyway for QEMU to work > > so far. > > > > What I think the best is we either go one way or another: either we make MR > > lifecycle clearly separate, or we make it clearly efficient (meanwhile we > > still keep the concept easy, and we at least try to always stick with one > > refcount which is easier to maintain too). > > > > IMHO that's what the other older patch does (plus my fixup squashed in): > > > > https://lore.kernel.org/all/ZsenKpu1czQGYz7m@x1n/ > > > > That avoids taking a refcount for internal MRs, always stick with owner > > shares the same lifecycle with MRs, just like the same assumption we have > > already had in memory_region_ref(). The bad side effect is we need > > something slightly hackish in mr finalize(), but we can provide some better > > doc, and keep the comlexity there only (which I think is better than always > > having two refcounts all over). > > Again, please forget about efficiency. It does not matter and makes noises > in our thoughts. It's not only about efficiency, that's pretty much side effect. It's more about how we should define refcount in the future, then if we stick with owner sharing lifetime with all MRs then taking that subregion refcount doesn't help anything except introducing a circular reference. It solves the circular reference with even a good side effect of reducing one atomic op from that pov, even if in a slow path. > > > > > If we worry about removal of that container assertion, we could assert > > instead on the owner. I've attached a slightly modified full version of > > such alternative patch below, with the best comment I see suite. > > This is better as it tells the lifetimes of memory regions need to be dealt > with, but why don't you deal them with reference counters in that case? We discussed plenty in this area, obviously you don't care about keep having two refcounts on MRs but I do my best to avoid it.. that's all about it so far.. > Reference counters are tools specifically designed for this. I hope I was trying to help. We could wait for a 2nd opinion. > > > > > diff --git a/system/memory.c b/system/memory.c > > index b17b5538ff..7b2d91ca6b 100644 > > --- a/system/memory.c > > +++ b/system/memory.c > > @@ -1803,7 +1803,6 @@ static void memory_region_finalize(Object *obj) > > { > > MemoryRegion *mr = MEMORY_REGION(obj); > > - assert(!mr->container); > > /* We know the region is not visible in any address space (it > > * does not have a container and cannot be a root either because > > @@ -1813,6 +1812,17 @@ static void memory_region_finalize(Object *obj) > > */ > > mr->enabled = false; > > memory_region_transaction_begin(); > > + if (mr->container) { > > + /* > > + * If this happens, it must be when MRs share the same owner, > > + * because only share-owner-ed links doesn't take a refcount. In > > + * this specific case, we allow random order of finalize() on the > > + * MRs the owner owns, so it's possible the child finalize()s > > + * before a parent. When it happens, unlink from the child. > > + */ > > + assert(mr->container->owner == mr->owner); > > + memory_region_del_subregion(mr->container, mr); > > + } > > while (!QTAILQ_EMPTY(&mr->subregions)) { > > MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions); > > memory_region_del_subregion(mr, subregion); > > @@ -2644,7 +2654,15 @@ static void memory_region_update_container_subregions(MemoryRegion *subregion) > > memory_region_transaction_begin(); > > - memory_region_ref(subregion); > > + if (mr->owner != subregion->owner) { > > + /* > > + * MRs always have the same lifecycle of its owner, so that when > > + * adding a subregion that shares the same owner of the parent, we > > + * don't need any refcounting, because the two MRs share the > > + * lifecycle with owner, so they share between each other too. > > + */ > > + memory_region_ref(subregion); > > + } > > QTAILQ_FOREACH(other, &mr->subregions, subregions_link) { > > if (subregion->priority >= other->priority) { > > QTAILQ_INSERT_BEFORE(other, subregion, subregions_link); > > @@ -2702,7 +2720,10 @@ void memory_region_del_subregion(MemoryRegion *mr, > > assert(alias->mapped_via_alias >= 0); > > } > > QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link); > > - memory_region_unref(subregion); > > + /* See the corresponding comment in add subregion path */ > > + if (mr->owner != subregion->owner) { > > + memory_region_unref(subregion); > > + } > > memory_region_update_pending |= mr->enabled && subregion->enabled; > > memory_region_transaction_commit(); > > } >
On 2025/01/16 0:40, Peter Xu wrote: > On Wed, Jan 15, 2025 at 11:54:56PM +0900, Akihiko Odaki wrote: >> On 2025/01/15 22:43, Peter Xu wrote: >>> On Wed, Jan 15, 2025 at 01:46:29PM +0900, Akihiko Odaki wrote: >>>> On 2025/01/15 2:02, Peter Xu wrote: >>>>> On Tue, Jan 14, 2025 at 05:43:09PM +0900, Akihiko Odaki wrote: >>>>>> memory_region_finalize() is not a function to tell the owner is leaving, but >>>>>> the memory region itself is being destroyed. >>>>> >>>>> It is when the lifecycle of the MR is the same as the owner. That holds >>>>> true I suppose if without this patch, and that's why I don't prefer this >>>>> patch because it makes that part more complicated. >>>> >>>> The lifecycle of the MR is not the same as the owner. The MR gets finalized >>>> during the finalization of the owner, and the owner is still alive at the >>>> moment. It is something you should always care when having a child object. >>> >>> What is the benefit of having such explicit layering of different lifecycle >>> between the owner and the MRs that it owns? >>> >>> To ask in another way, what's the functional benefit that we order the >>> destruction of MRs within the same owner, paying that with explicit two >>> refcounts concept in memory core? > > [1] > >>> >>> AFAICT, that's the only purpose MR->refcount is servicing for in this >>> patchset besides the property link. >>> >>> Currently, memory_region_ref() takes the refcount _only_ from the host. >>> Considering that's the only memory API to take a reference on a MR, it kind >>> of implies to everyone that the MR and the owner shares the lifetime. >>> >>> In reality, it's not 100% shared indeed, but almost. We even have those >>> document for dynamic MRs to make sure that is true even there. >>> >>> Then it's about the "virtual lifecycle" which triggers a finalize(), or >>> "real lifecycle" which triggers a free() that may make a difference to a >>> MR. And that's the part on whether we should try to not expose too much at >>> all on these. I want to keep the concept simple if possible that we stick >>> with sharing lifetime between owner and all MRs underneath. I want to see >>> whether we can avoid complicating that part. >> >> I would rather avoid virtual or real lifecycles notions because it's more >> than free(). Memory regions constructed with functions like >> memory_region_init_io() and memory_region_init_ram_ptr() requires the owner >> to retain the backend resource to keep functioning. In other words, the >> memory region refers to the owner, and that is no different from other kind >> of references. >> >> The uniqueness of this relationship is that the owner also refers to the >> memory region. Memory regions avoid a circular reference by omitting the >> reference from them to the owner and instruct others to refer to the owner >> instead. >> >>> >>> I can see why you want to clearly separate the lifetimes, because it's >>> cleaner to you. But IMHO we already made a decision from that starting >>> from when memory_region_ref() does not take MR->refcount, otherwise you >>> should at least need something like this to make the lifecycle completely >>> separate in your this patch: >>>> diff --git a/system/memory.c b/system/memory.c >>> index b17b5538ff..d4b88c389a 100644 >>> --- a/system/memory.c >>> +++ b/system/memory.c >>> @@ -1843,15 +1843,23 @@ void memory_region_ref(MemoryRegion *mr) >>> * Memory regions without an owner are supposed to never go away; >>> * we do not ref/unref them because it slows down DMA sensibly. >>> */ >>> - if (mr && mr->owner) { >>> - object_ref(mr->owner); >>> + if (mr) { >>> + /* The MR has its own lifecycle.. even if in most cases, virtually */ >>> + object_ref(mr); >>> + if (mr->owner) { >>> + object_ref(mr->owner); >>> + } >>> } >>> } >>> void memory_region_unref(MemoryRegion *mr) >>> { >>> - if (mr && mr->owner) { >>> - object_unref(mr->owner); >>> + if (mr) { >>> + /* The MR has its own lifecycle.. even if in most cases, virtually */ >>> + object_unref(mr); >>> + if (mr->owner) { >>> + object_unref(mr->owner); >>> + } >>> } >>> } >>> >>> To me, QEMU already went the other way. So I sincerely don't know how that >>> extra mr->refcount usage it could bring us. It only makes it harder to >>> understand to me. >> >> The owner refers to the memory region in turn so it is fine omitting >> object_ref(mr). > > So you decided to "sometimes" take mr->refcount because it needs to, then > "sometimes" don't take mr->refcounts because it doesn't need to.. > > Normally such complexity is ok, but to me it's ok only when it services, > for example, a major performance improvements, so that it's justified to > add complexity. The pay is done whoever going to maintain this code. > > In this case, no, I don't yet see how important this idea is yet to > introduce such difference into mr refcounts, which is already complicated > as hell.. We're paying such complexity with some "technical cleanest", > while when with different treatment of mr->refcount in different context, > it isn't that clean either. > >> If you draw an object graph that originates from the >> referrer, you can still reach the memory region. That is not true for your >> patch; you cannot reach to the subregion from the container. >> >> The separate lifetimes still matter even with your patch. In a hypothetical >> world that the lifetime of owner and memory regions completely match, the >> ordering of finalization of memory regions owned by one object simply does >> not happen because they occur simultaneously. It is simply not true, and >> even your patch does not make sense in such a hypothetical world. > > I hope that's obvious goal since start, yes, that patch will make > finalize() in any order works for MRs under the same owner, as I don't know > why that order matters.. taking that chance of almost still sticking with > one refcount. > > I suppose you finally need to answer my above question [1] to say whether > it makes sense. To me, it doesn't make sense only if there's a functional > difference on that order of finalize(). I have been discussed about semantics, not functionality and both of them matter. Functionally, the ordering of container/subregion finalization matters if some device tries to a container during finalization. In such a case, removing subregions from the container at random timing can result in an unexpected behavior. There is little chance to have such a scenario but we should stay the safe side if possible. > >> >>> >>>> >>>>> >>>>>> It should not happen when a container is still referencing it. That is >>>>>> also why it has memory_region_ref(subregion) in >>>>>> memory_region_update_container_subregions() and assert(!mr->container) in >>>>>> memory_region_finalize(). >>>>> >>>>> Again, the line I added was sololy for what you said "automation" elsewhere >>>>> and only should work within MR-links within the same owner. Otherwise >>>>> anyone referencing the MR would hold the owner ref then this finalize() >>>>> will never happen. >>>>> >>>>> Now, if I could go back to your original purpose of this work, quotting >>>>> from your cover letter: >>>>> >>>>>> I saw various sanitizer errors when running check-qtest-ppc64. While >>>>>> I could just turn off sanitizers, I decided to tackle them this time. >>>>>> >>>>>> Unfortunately, GLib versions older than 2.81.0 do not free test data in >>>>>> some cases so some sanitizer errors remain. All sanitizer errors will be >>>>>> gone with this patch series combined with the following change for GLib: >>>>>> https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120 >>>>> >>>>> Is check-qtest-ppc64 the only one that will trigger this issue? Does it >>>>> mean that most of the devices will do proper removal of device-owned >>>>> subregions (hence, not prone to circular reference of owner refcount) >>>>> except some devices in ppc64? >>>>> >>>> >>>> Searching for memory_region_add_subregion() gives 1078 instances where there >>>> are 142 instances of memory_region_del_subregion(). This is a rough estimate >>>> but there are potentially 936 instances of subregions without explicit >>>> deletion. >>>> >>>> For example, hw/audio/intel-hda.c adds subregions immediately after their >>>> containers never deletes the subregions. I think that's fine because their >>>> lifetimes are obvious with reference counters. >>> >>> OK, let's try to figure out a best way to move forward then. >>> >>> Let me try to summarize the two approaches so far. >>> >>> So in general I think I don't prefer this patch because this patch is kind >>> of in the middle of something. >>> >>> It neither provides 100% separation of MR lifecycle: as discussed above, on >>> not referencing MR->refcount on memory_region_ref/unref at least yet so far >>> together in this patch, but suddenly started considering it in MR links. >>> To me, that's abuse if ordering of such finalize() is not justified. >>> >>> Nor it provides best efficiency: needing to take a MR->refcount when >>> linking two MRs, even if we essentially don't need to guarded by the fact >>> that owner must exist already, which must hold true anyway for QEMU to work >>> so far. >>> >>> What I think the best is we either go one way or another: either we make MR >>> lifecycle clearly separate, or we make it clearly efficient (meanwhile we >>> still keep the concept easy, and we at least try to always stick with one >>> refcount which is easier to maintain too). >>> >>> IMHO that's what the other older patch does (plus my fixup squashed in): >>> >>> https://lore.kernel.org/all/ZsenKpu1czQGYz7m@x1n/ >>> >>> That avoids taking a refcount for internal MRs, always stick with owner >>> shares the same lifecycle with MRs, just like the same assumption we have >>> already had in memory_region_ref(). The bad side effect is we need >>> something slightly hackish in mr finalize(), but we can provide some better >>> doc, and keep the comlexity there only (which I think is better than always >>> having two refcounts all over). >> >> Again, please forget about efficiency. It does not matter and makes noises >> in our thoughts. > > It's not only about efficiency, that's pretty much side effect. > > It's more about how we should define refcount in the future, then if we > stick with owner sharing lifetime with all MRs then taking that subregion > refcount doesn't help anything except introducing a circular reference. It > solves the circular reference with even a good side effect of reducing one > atomic op from that pov, even if in a slow path. > >> >>> >>> If we worry about removal of that container assertion, we could assert >>> instead on the owner. I've attached a slightly modified full version of >>> such alternative patch below, with the best comment I see suite. >> >> This is better as it tells the lifetimes of memory regions need to be dealt >> with, but why don't you deal them with reference counters in that case? > > We discussed plenty in this area, obviously you don't care about keep > having two refcounts on MRs but I do my best to avoid it.. that's all about > it so far.. > >> Reference counters are tools specifically designed for this. > > I hope I was trying to help. We could wait for a 2nd opinion. > >> >>> >>> diff --git a/system/memory.c b/system/memory.c >>> index b17b5538ff..7b2d91ca6b 100644 >>> --- a/system/memory.c >>> +++ b/system/memory.c >>> @@ -1803,7 +1803,6 @@ static void memory_region_finalize(Object *obj) >>> { >>> MemoryRegion *mr = MEMORY_REGION(obj); >>> - assert(!mr->container); >>> /* We know the region is not visible in any address space (it >>> * does not have a container and cannot be a root either because >>> @@ -1813,6 +1812,17 @@ static void memory_region_finalize(Object *obj) >>> */ >>> mr->enabled = false; >>> memory_region_transaction_begin(); >>> + if (mr->container) { >>> + /* >>> + * If this happens, it must be when MRs share the same owner, >>> + * because only share-owner-ed links doesn't take a refcount. In >>> + * this specific case, we allow random order of finalize() on the >>> + * MRs the owner owns, so it's possible the child finalize()s >>> + * before a parent. When it happens, unlink from the child. >>> + */ >>> + assert(mr->container->owner == mr->owner); >>> + memory_region_del_subregion(mr->container, mr); >>> + } >>> while (!QTAILQ_EMPTY(&mr->subregions)) { >>> MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions); >>> memory_region_del_subregion(mr, subregion); >>> @@ -2644,7 +2654,15 @@ static void memory_region_update_container_subregions(MemoryRegion *subregion) >>> memory_region_transaction_begin(); >>> - memory_region_ref(subregion); >>> + if (mr->owner != subregion->owner) { >>> + /* >>> + * MRs always have the same lifecycle of its owner, so that when >>> + * adding a subregion that shares the same owner of the parent, we >>> + * don't need any refcounting, because the two MRs share the >>> + * lifecycle with owner, so they share between each other too. >>> + */ >>> + memory_region_ref(subregion); >>> + } >>> QTAILQ_FOREACH(other, &mr->subregions, subregions_link) { >>> if (subregion->priority >= other->priority) { >>> QTAILQ_INSERT_BEFORE(other, subregion, subregions_link); >>> @@ -2702,7 +2720,10 @@ void memory_region_del_subregion(MemoryRegion *mr, >>> assert(alias->mapped_via_alias >= 0); >>> } >>> QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link); >>> - memory_region_unref(subregion); >>> + /* See the corresponding comment in add subregion path */ >>> + if (mr->owner != subregion->owner) { >>> + memory_region_unref(subregion); >>> + } >>> memory_region_update_pending |= mr->enabled && subregion->enabled; >>> memory_region_transaction_commit(); >>> } >> >
On Thu, Jan 16, 2025 at 12:52:56AM +0900, Akihiko Odaki wrote: > Functionally, the ordering of container/subregion finalization matters if > some device tries to a container during finalization. In such a case, | ^ something is missing here, feel free to complete this. > removing subregions from the container at random timing can result in an > unexpected behavior. There is little chance to have such a scenario but we > should stay the safe side if possible. It sounds like a future feature, and I'm not sure we'll get there, so I don't worry that much. Keeping refcount core idea simple is still very attractive to me. I still prefer we have complete MR refcounting iff when necessary. It's also possible it'll never happen to QEMU.
On 2025/01/16 1:14, Peter Xu wrote: > On Thu, Jan 16, 2025 at 12:52:56AM +0900, Akihiko Odaki wrote: >> Functionally, the ordering of container/subregion finalization matters if >> some device tries to a container during finalization. In such a case, > | > ^ something is missing here, feel free to complete this. Oops, I meant: functionally, the ordering of container/subregion finalization matters if some device tries to use a container during finalization. > >> removing subregions from the container at random timing can result in an >> unexpected behavior. There is little chance to have such a scenario but we >> should stay the safe side if possible. > > It sounds like a future feature, and I'm not sure we'll get there, so I > don't worry that much. Keeping refcount core idea simple is still very > attractive to me. I still prefer we have complete MR refcounting iff when > necessary. It's also possible it'll never happen to QEMU. > It's not just about the future but also about compatibility with the current device implementations. I will not be surprised even if the random ordering of subregion finalization breaks one of dozens of devices we already have. We should pay attention the details as we are touching the core infrastructure.
On Thu, Jan 16, 2025 at 02:37:38PM +0900, Akihiko Odaki wrote: > On 2025/01/16 1:14, Peter Xu wrote: > > On Thu, Jan 16, 2025 at 12:52:56AM +0900, Akihiko Odaki wrote: > > > Functionally, the ordering of container/subregion finalization matters if > > > some device tries to a container during finalization. In such a case, > > | > > ^ something is missing here, feel free to complete this. > > Oops, I meant: functionally, the ordering of container/subregion > finalization matters if some device tries to use a container during > finalization. This is true, though if we keep the concept of "all the MRs share the same lifecycle of the owner" idea, another fix of such is simply moving the container access before any detachment of MRs. > > > > > > removing subregions from the container at random timing can result in an > > > unexpected behavior. There is little chance to have such a scenario but we > > > should stay the safe side if possible. > > > > It sounds like a future feature, and I'm not sure we'll get there, so I > > don't worry that much. Keeping refcount core idea simple is still very > > attractive to me. I still prefer we have complete MR refcounting iff when > > necessary. It's also possible it'll never happen to QEMU. > > > > It's not just about the future but also about compatibility with the current > device implementations. I will not be surprised even if the random ordering > of subregion finalization breaks one of dozens of devices we already have. > We should pay attention the details as we are touching the core > infrastructure. Yes, if we can find any such example that we must follow the order of MR destruction, I think that could justify your approach will be required but not optional. It's just that per my understanding there should be none, and even if there're very few outliers, it can still be trivially fixed as mentioned above. My gut feeling is when we need serious MR refcounting (I'd expect due to the current heavy code base it might be easier to start a new project if that's required.. that's why I was thinking maybe it will not happen.. but if it will..), we'll do more than your change, and that also means memory_region_ref() must start to refcount MRs, because a serious MR separate refcounting should mean MR can go on the fly before the owner.
On Tue, 14 Jan 2025 at 19:12, Peter Xu <peterx@redhat.com> wrote: > > On Tue, Jan 14, 2025 at 05:42:57PM +0000, Peter Maydell wrote: > > There's at least one test in the arm qtests that will hit this. > > I suspect that you'll find that most architectures except x86 > > (where we don't have models of complex SoCs and the few > > machines we do have tend to be old code that is less QOMified) > > will hit similar issues. I think there's a general issue here, > > this isn't just "some particular ppc device is wrongly coded". > > I see. Do you know how many of them would be important memory leaks that > we should fix immediately? None of these are important memory leaks, because the device is almost always present for the lifetime of the simulation. The only case you'd actually get a visible leak would be if you could hot-unplug the device, and even then you'd have to deliberately sit there doing hot-plug-then-unplug cycles to leak an interesting amount of memory. The main reason to want to fix them is that it lets us run "make check" under the sanitizer and catch other, more interesting leaks. > I mean, we have known memory leaks in QEMU in many places I assume. I am > curious how important this problem is, and whether such would justify a > memory API change that is not reaching a quorum state (and, imho, add > complexity to memory core and of course that spreads to x86 too even if it > was not affected) to be merged. Or perhaps we can fix the important ones > first from the device model directly instead. The problem is generic, and the problem is that we have not actually nailed down how this is supposed to work, i.e: * what are the reference counts counting? * if a device has this kind of memory region inside another, how is it supposed to be coded so as to not leak memory? If we can figure out how the lifecycle and memory management is supposed to work, then yes, we can fix the relevant device models so that they follow whatever the rules are. But it seems to me that at the moment we have not got a consensus on how this is supposed to work. Until we have that, there's no way to fix this at the device model level, because we don't know what changes we need to make. thanks -- PMM
On Thu, 16 Jan 2025, Peter Maydell wrote: > On Tue, 14 Jan 2025 at 19:12, Peter Xu <peterx@redhat.com> wrote: >> >> On Tue, Jan 14, 2025 at 05:42:57PM +0000, Peter Maydell wrote: >>> There's at least one test in the arm qtests that will hit this. >>> I suspect that you'll find that most architectures except x86 >>> (where we don't have models of complex SoCs and the few >>> machines we do have tend to be old code that is less QOMified) >>> will hit similar issues. I think there's a general issue here, >>> this isn't just "some particular ppc device is wrongly coded". >> >> I see. Do you know how many of them would be important memory leaks that >> we should fix immediately? > > None of these are important memory leaks, because the device is > almost always present for the lifetime of the simulation. The > only case you'd actually get a visible leak would be if you > could hot-unplug the device, and even then you'd have to > deliberately sit there doing hot-plug-then-unplug cycles to > leak an interesting amount of memory. > > The main reason to want to fix them is that it lets us run > "make check" under the sanitizer and catch other, more > interesting leaks. > >> I mean, we have known memory leaks in QEMU in many places I assume. I am >> curious how important this problem is, and whether such would justify a >> memory API change that is not reaching a quorum state (and, imho, add >> complexity to memory core and of course that spreads to x86 too even if it >> was not affected) to be merged. Or perhaps we can fix the important ones >> first from the device model directly instead. > > The problem is generic, and the problem is that we have not actually > nailed down how this is supposed to work, i.e: > * what are the reference counts counting? It would be very unintuitive if ref counts not counted the number of references to the object that contains this ref count. If I understood correctly that's the reason for this discussion that in this case the ref count of the owner is counting the MRs instead of its own references and the MR's ref count is not used which is confusing. > * if a device has this kind of memory region inside another, > how is it supposed to be coded so as to not leak memory? > > If we can figure out how the lifecycle and memory management > is supposed to work, then yes, we can fix the relevant device > models so that they follow whatever the rules are. But it seems If looking for rules to follow one proven and relatively simple set is what Cocoa uses on macOS. There's a very short introduction here: https://www.peachpit.com/articles/article.aspx?p=377302&seqNum=2 and a bit longer more complete here: https://www.tomdalling.com/blog/cocoa/an-in-depth-look-at-manual-memory-management-in-objective-c/ I think this case is one which is shortly mentioned at the end of the second link which is solved by not retaining the contained objects so this is closer to what PeterX suggests just dropping the refcounting on the owner and consider the MRs owned by the superregions once added there. This means add subregion would pass the reference to the owner and remove subregion passes it back to who called it so the MR's ref count needs no change but then who added the MR should not use it after passing it to the superregion and who called remove subregion should release it. But I don't really understans the problem so maybe it's more complex. Regards, BALATON Zoltan > to me that at the moment we have not got a consensus on how > this is supposed to work. Until we have that, there's no way to > fix this at the device model level, because we don't know what > changes we need to make. > > thanks > -- PMM > >
On Thu, Jan 16, 2025 at 02:50:26PM +0000, Peter Maydell wrote: > On Tue, 14 Jan 2025 at 19:12, Peter Xu <peterx@redhat.com> wrote: > > > > On Tue, Jan 14, 2025 at 05:42:57PM +0000, Peter Maydell wrote: > > > There's at least one test in the arm qtests that will hit this. > > > I suspect that you'll find that most architectures except x86 > > > (where we don't have models of complex SoCs and the few > > > machines we do have tend to be old code that is less QOMified) > > > will hit similar issues. I think there's a general issue here, > > > this isn't just "some particular ppc device is wrongly coded". > > > > I see. Do you know how many of them would be important memory leaks that > > we should fix immediately? > > None of these are important memory leaks, because the device is > almost always present for the lifetime of the simulation. The > only case you'd actually get a visible leak would be if you > could hot-unplug the device, and even then you'd have to > deliberately sit there doing hot-plug-then-unplug cycles to > leak an interesting amount of memory. > > The main reason to want to fix them is that it lets us run > "make check" under the sanitizer and catch other, more > interesting leaks. Right. > > > I mean, we have known memory leaks in QEMU in many places I assume. I am > > curious how important this problem is, and whether such would justify a > > memory API change that is not reaching a quorum state (and, imho, add > > complexity to memory core and of course that spreads to x86 too even if it > > was not affected) to be merged. Or perhaps we can fix the important ones > > first from the device model directly instead. > > The problem is generic, and the problem is that we have not actually > nailed down how this is supposed to work, i.e: > * what are the reference counts counting? I think we know how it works before this patch. And with that, I suppose Akihiko managed to figure out how it was not freed properly now for those due to the circular reference issue that this series is tracking. That's definitely one step forward to me. Said that, this is also the major controversial point so far on this patch on how we should implement such object-internal auto-detach mechanism for MRs even within the same object. My suggestion is sticking with the current simple model as much as possible on always using owner refcounts for MRs, while resolving the issue we're facing with that. I have a complete patch (which, I would say, should still be under authorship of Akihiko; what I provided is a fixup on top of his, and I slightly touched up some comments to provide the best readability): https://lore.kernel.org/qemu-devel/Z4e7gFSqdhcmJPYb@x1n So it's more of a preference that I prefer his older patch with the fixup, rather than this patch. Akihiko would prefer start using two refcounts, one almost dedicated for object internal MR refcounts so that we'll finalize internal MRs in order (which I questioned on the necessary), but not completely. > * if a device has this kind of memory region inside another, > how is it supposed to be coded so as to not leak memory? Is this question about one MR being struct field of one object, or specific to something else? > > If we can figure out how the lifecycle and memory management > is supposed to work, then yes, we can fix the relevant device > models so that they follow whatever the rules are. But it seems > to me that at the moment we have not got a consensus on how > this is supposed to work. Until we have that, there's no way to > fix this at the device model level, because we don't know what > changes we need to make. Yes, I think I understand slightly better on the issue we're facing now, thanks.
On 2025/01/17 1:13, BALATON Zoltan wrote: > On Thu, 16 Jan 2025, Peter Maydell wrote: >> On Tue, 14 Jan 2025 at 19:12, Peter Xu <peterx@redhat.com> wrote: >>> >>> On Tue, Jan 14, 2025 at 05:42:57PM +0000, Peter Maydell wrote: >>>> There's at least one test in the arm qtests that will hit this. >>>> I suspect that you'll find that most architectures except x86 >>>> (where we don't have models of complex SoCs and the few >>>> machines we do have tend to be old code that is less QOMified) >>>> will hit similar issues. I think there's a general issue here, >>>> this isn't just "some particular ppc device is wrongly coded". >>> >>> I see. Do you know how many of them would be important memory leaks >>> that >>> we should fix immediately? >> >> None of these are important memory leaks, because the device is >> almost always present for the lifetime of the simulation. The >> only case you'd actually get a visible leak would be if you >> could hot-unplug the device, and even then you'd have to >> deliberately sit there doing hot-plug-then-unplug cycles to >> leak an interesting amount of memory. >> >> The main reason to want to fix them is that it lets us run >> "make check" under the sanitizer and catch other, more >> interesting leaks. >> >>> I mean, we have known memory leaks in QEMU in many places I assume. >>> I am >>> curious how important this problem is, and whether such would justify a >>> memory API change that is not reaching a quorum state (and, imho, add >>> complexity to memory core and of course that spreads to x86 too even >>> if it >>> was not affected) to be merged. Or perhaps we can fix the important >>> ones >>> first from the device model directly instead. >> >> The problem is generic, and the problem is that we have not actually >> nailed down how this is supposed to work, i.e: >> * what are the reference counts counting? > > It would be very unintuitive if ref counts not counted the number of > references to the object that contains this ref count. If I understood > correctly that's the reason for this discussion that in this case the > ref count of the owner is counting the MRs instead of its own references > and the MR's ref count is not used which is confusing. > >> * if a device has this kind of memory region inside another, >> how is it supposed to be coded so as to not leak memory? >> >> If we can figure out how the lifecycle and memory management >> is supposed to work, then yes, we can fix the relevant device >> models so that they follow whatever the rules are. But it seems > > If looking for rules to follow one proven and relatively simple set is > what Cocoa uses on macOS. There's a very short introduction here: > https://www.peachpit.com/articles/article.aspx?p=377302&seqNum=2 > and a bit longer more complete here: > https://www.tomdalling.com/blog/cocoa/an-in-depth-look-at-manual-memory- > management-in-objective-c/ > > I think this case is one which is shortly mentioned at the end of the > second link which is solved by not retaining the contained objects so > this is closer to what PeterX suggests just dropping the refcounting on > the owner and consider the MRs owned by the superregions once added > there. This means add subregion would pass the reference to the owner > and remove subregion passes it back to who called it so the MR's ref > count needs no change but then who added the MR should not use it after > passing it to the superregion and who called remove subregion should > release it. But I don't really understans the problem so maybe it's more > complex. Let me explain my and Peter Xu's solutions by listing object references. It will abstract the details away and allow applying the analogy of other reference counting systems like Objective-C. "Real" reference relationship ----------------------------- First, I'll introduce one basic rule: Rule 1. If A needs B, let A refer to B. Now, There is an owner and a memory region. The owner needs the memory region to function. The memory region needs the owner to provide to allocate itself and e.g., to provide MemoryRegionOps for memory_region_init_io(). So we will make a bi-directional reference between the owner and the memory region. O (Owner) -> MR (Memory Region) MR -> O Now, let's think the case where there is a container and subregion, and they are owned by different objects. The container needs the subregion to satisfy its accesses, and the subregion needs the container for memory_region_find(). So this relationship is also bi-directional. CO (Container's Owner) -> C (Container) C -> CO SO (Subregions' Owner) -> S (Subregion) S -> SO C -> S S -> C Let's think someone took a reference to the container e.g., for DMA. This mean we add the following reference: DMA Controller -> Container This will ensure all of Container, Container's Owner, Subregion, Subregion's Owner are alive during the DMA. "Weak" references ----------------- Next, I'll impose the restriction that prevents circular references. To avoid circular references, we'll stop counting references for one direction. In other words, we'll make some references "weak". Weak references are prone to be dangling (in Objective-C or other platforms, such references will be usually replaced with nil-like values). Rule 2. Make a reference from a memory region to its owner weak Rule 3. Make a reference from a subregion to its container weak CO -> C C -> CO (weak) SO -> S S -> SO (weak) C -> S S -> C (weak) D (DMA Controller) -> C This will fix circular references, but unfortunately it will not work because DMA Controller requires weak references to reach to Container's Owner or Subregion's Owner with "strong" references; it means they can be dangling at any time. To solve this problem, we'll introduce another rule: Rule 4. Replace references to memory regions with references to their owners *except for the references from the owner to the memory region*. CO -> C C -> CO (weak) SO -> S S -> SO (weak) C -> SO S -> C (weak) D -> CO With this change, D can again reach all of CO, C, SO, and S. Problem ------- What if Container's Owner and Subregion's Owner are identical? O -> C C -> O (weak) O -> S S -> O (weak) C -> O S -> C (weak) D -> O Now there is a circular reference: O -> C -> O My patch -------- My patch amends rule 4: Rule 4'. Replace references to memory regions with references to their owners *unless the reference originates from the owner*. O -> C C -> O (weak) O -> S S -> O (weak) C -> S (this used to be C -> O) S -> C (weak) D -> O Now the circular reference is gone. Peter Xu's patch ---------------- Peter Xu's patch adds two new rules: Rule 5. Make references from a container to its subregion weak if they are owned by the same object. O -> C C -> O (weak) O -> S S -> O (weak) C -> O (weak [this used to be strong]) S -> C (weak) D -> O Now the circular reference is gone and D can still reach O, C, and S. However, there is one problem: C requires a weak reference to reach to S. More concretely the pointer from C to S will be dangling when the reference O -> S gets dropped during the finalization of O. Peter Xu's patch adds yet another rule to overcome this problem: Rule 6. When a subregion gets finalized, drop the weak reference from its container to itself. The dangling reference will be gone with this rule. Comparison between the two patches ---------------------------------- I'll compare them by showing pros and cons of Peter Xu's patch. Pro: a. Will not add yet another direct reference to memory regions where currently there can be only one reference from the owner to each memory region. Cons: b. It requires two new rules. c. C will see its reference to S to disappear at random timing due to a weak reference. So, which is better do you think?
On 2025/01/16 23:33, Peter Xu wrote: > On Thu, Jan 16, 2025 at 02:37:38PM +0900, Akihiko Odaki wrote: >> On 2025/01/16 1:14, Peter Xu wrote: >>> On Thu, Jan 16, 2025 at 12:52:56AM +0900, Akihiko Odaki wrote: >>>> Functionally, the ordering of container/subregion finalization matters if >>>> some device tries to a container during finalization. In such a case, >>> | >>> ^ something is missing here, feel free to complete this. >> >> Oops, I meant: functionally, the ordering of container/subregion >> finalization matters if some device tries to use a container during >> finalization. > > This is true, though if we keep the concept of "all the MRs share the same > lifecycle of the owner" idea, another fix of such is simply moving the > container access before any detachment of MRs. > >> >>> >>>> removing subregions from the container at random timing can result in an >>>> unexpected behavior. There is little chance to have such a scenario but we >>>> should stay the safe side if possible. >>> >>> It sounds like a future feature, and I'm not sure we'll get there, so I >>> don't worry that much. Keeping refcount core idea simple is still very >>> attractive to me. I still prefer we have complete MR refcounting iff when >>> necessary. It's also possible it'll never happen to QEMU. >>> >> >> It's not just about the future but also about compatibility with the current >> device implementations. I will not be surprised even if the random ordering >> of subregion finalization breaks one of dozens of devices we already have. >> We should pay attention the details as we are touching the core >> infrastructure. > > Yes, if we can find any such example that we must follow the order of MR > destruction, I think that could justify your approach will be required but > not optional. It's just that per my understanding there should be none, > and even if there're very few outliers, it can still be trivially fixed as > mentioned above. It can be fixed but that means we need auditing the code of devices or wait until we get a bug report. > > My gut feeling is when we need serious MR refcounting (I'd expect due to > the current heavy code base it might be easier to start a new project if > that's required.. that's why I was thinking maybe it will not happen.. but > if it will..), we'll do more than your change, and that also means > memory_region_ref() must start to refcount MRs, because a serious MR > separate refcounting should mean MR can go on the fly before the owner. Actually there is one example: virtio_gpu_virgl_map_resource_blob() in hw/display/virtio-gpu-virgl.c creates a MR that can be removed before the device. To make this possible, it specifies MRs themselves as their and let them refcount without help of the device.
On Fri, Jan 17, 2025 at 03:24:34PM +0900, Akihiko Odaki wrote: > On 2025/01/16 23:33, Peter Xu wrote: > > On Thu, Jan 16, 2025 at 02:37:38PM +0900, Akihiko Odaki wrote: > > > On 2025/01/16 1:14, Peter Xu wrote: > > > > On Thu, Jan 16, 2025 at 12:52:56AM +0900, Akihiko Odaki wrote: > > > > > Functionally, the ordering of container/subregion finalization matters if > > > > > some device tries to a container during finalization. In such a case, > > > > | > > > > ^ something is missing here, feel free to complete this. > > > > > > Oops, I meant: functionally, the ordering of container/subregion > > > finalization matters if some device tries to use a container during > > > finalization. > > > > This is true, though if we keep the concept of "all the MRs share the same > > lifecycle of the owner" idea, another fix of such is simply moving the > > container access before any detachment of MRs. > > > > > > > > > > > > > > removing subregions from the container at random timing can result in an > > > > > unexpected behavior. There is little chance to have such a scenario but we > > > > > should stay the safe side if possible. > > > > > > > > It sounds like a future feature, and I'm not sure we'll get there, so I > > > > don't worry that much. Keeping refcount core idea simple is still very > > > > attractive to me. I still prefer we have complete MR refcounting iff when > > > > necessary. It's also possible it'll never happen to QEMU. > > > > > > > > > > It's not just about the future but also about compatibility with the current > > > device implementations. I will not be surprised even if the random ordering > > > of subregion finalization breaks one of dozens of devices we already have. > > > We should pay attention the details as we are touching the core > > > infrastructure. > > > > Yes, if we can find any such example that we must follow the order of MR > > destruction, I think that could justify your approach will be required but > > not optional. It's just that per my understanding there should be none, > > and even if there're very few outliers, it can still be trivially fixed as > > mentioned above. > > It can be fixed but that means we need auditing the code of devices or wait > until we get a bug report. We'd better have a solid example. And for this specific question, IIUC we can have such problem even if internal-ref start to use MR refcounts. It's because we have a not very straightforward way of finalize() an object, which is freeing all properties before its own finalize().. static void object_finalize(void *data) { ... object_property_del_all(obj); object_deinit(obj, ti); ... } I think it used to be the other way round (which will be easier to understand to me..), but changed after 76a6e1cc7cc. It could boil down to two dependencies: (1) children's unparent() callback wanting to have the parent being present and valid, and (2) parent's finalize() callback wanting to have all children being present and valid. I guess we chose (1) as of now. So I suppose it means even with your patch, it won't help either as long as MRs are properties, and they can already all be gone in a device finalize() even with your new patch. From that POV, qdev unrealize() could be a good place for such cleanups while making sure all properties are present. > > > > > My gut feeling is when we need serious MR refcounting (I'd expect due to > > the current heavy code base it might be easier to start a new project if > > that's required.. that's why I was thinking maybe it will not happen.. but > > if it will..), we'll do more than your change, and that also means > > memory_region_ref() must start to refcount MRs, because a serious MR > > separate refcounting should mean MR can go on the fly before the owner. > > Actually there is one example: virtio_gpu_virgl_map_resource_blob() in > hw/display/virtio-gpu-virgl.c creates a MR that can be removed before the > device. To make this possible, it specifies MRs themselves as their and let > them refcount without help of the device. .. I am definitely surprised that we have code that assigns obj->parent to be itself. Putting the self parenting aside.. and to the topic: I don't think this is an example for internal-MR use case? When the owner is itself, then it's not sharing the owner of the parent MR (which is b->hostmem in this case, which should probably be owned by VirtIOGPU object instead). So IIUC no matter which way we choose on the current discussion, it won't affect the result. Not to mention, the MRs are always properly detached always when the resource is unmapped: virtio_gpu_virgl_unmap_resource_blob(): /* memory region owns self res->mr object and frees it by itself */ memory_region_set_enabled(mr, false); memory_region_del_subregion(&b->hostmem, mr); So from that POV it's a very good citizen.. it doesn't need anything to be auto detached. Below can be off topic, but since we're discussing this use case.. My gut feeling is that it can cause trouble at some point having MR itself as parent. For example, I will not be surprised that some day QMP command qom-list-properties provide a --deep pararmeter, then this will hang that command trying to look at child forever in a loop. It can easily break in other ways too, e.g. when we add an assertion anywhere for "obj->parent != obj" (which should really make sense.. from a object model POV), or when we want to take a ref to parent (for obj->parent reference) then it'll be a "real" circular reference, comparing to what we're discussing on auto-detach so far (which is IIUC a pretty "weak" circular ref; IOW, if del_subregion is always properly done, this patch not needed). Meanwhile the other free() overwrite: OBJECT(mr)->free = virtio_gpu_virgl_hostmem_region_free; I suppose it can be error prone too whenever we want to provide a common free() for MRs and this one can be easily overlooked.. I'm not familiar with GPU stuff, but reading it closer I do feel like it's kind of a slight abuse on all above.. If to stick with "owner shares the same lifecycle for its MRs" idea and fix all above, IMHO we could make virtio_gpu_virgl_hostmem_region to be a QOM object, then it can be a proper parent for the MR.
On 2025/01/18 2:46, Peter Xu wrote: > On Fri, Jan 17, 2025 at 03:24:34PM +0900, Akihiko Odaki wrote: >> On 2025/01/16 23:33, Peter Xu wrote: >>> On Thu, Jan 16, 2025 at 02:37:38PM +0900, Akihiko Odaki wrote: >>>> On 2025/01/16 1:14, Peter Xu wrote: >>>>> On Thu, Jan 16, 2025 at 12:52:56AM +0900, Akihiko Odaki wrote: >>>>>> Functionally, the ordering of container/subregion finalization matters if >>>>>> some device tries to a container during finalization. In such a case, >>>>> | >>>>> ^ something is missing here, feel free to complete this. >>>> >>>> Oops, I meant: functionally, the ordering of container/subregion >>>> finalization matters if some device tries to use a container during >>>> finalization. >>> >>> This is true, though if we keep the concept of "all the MRs share the same >>> lifecycle of the owner" idea, another fix of such is simply moving the >>> container access before any detachment of MRs. >>> >>>> >>>>> >>>>>> removing subregions from the container at random timing can result in an >>>>>> unexpected behavior. There is little chance to have such a scenario but we >>>>>> should stay the safe side if possible. >>>>> >>>>> It sounds like a future feature, and I'm not sure we'll get there, so I >>>>> don't worry that much. Keeping refcount core idea simple is still very >>>>> attractive to me. I still prefer we have complete MR refcounting iff when >>>>> necessary. It's also possible it'll never happen to QEMU. >>>>> >>>> >>>> It's not just about the future but also about compatibility with the current >>>> device implementations. I will not be surprised even if the random ordering >>>> of subregion finalization breaks one of dozens of devices we already have. >>>> We should pay attention the details as we are touching the core >>>> infrastructure. >>> >>> Yes, if we can find any such example that we must follow the order of MR >>> destruction, I think that could justify your approach will be required but >>> not optional. It's just that per my understanding there should be none, >>> and even if there're very few outliers, it can still be trivially fixed as >>> mentioned above. >> >> It can be fixed but that means we need auditing the code of devices or wait >> until we get a bug report. > > We'd better have a solid example. > > And for this specific question, IIUC we can have such problem even if > internal-ref start to use MR refcounts. > > It's because we have a not very straightforward way of finalize() an > object, which is freeing all properties before its own finalize().. > > static void object_finalize(void *data) > { > ... > object_property_del_all(obj); > object_deinit(obj, ti); > ... > } > > I think it used to be the other way round (which will be easier to > understand to me..), but changed after 76a6e1cc7cc. It could boil down to > two dependencies: (1) children's unparent() callback wanting to have the > parent being present and valid, and (2) parent's finalize() callback > wanting to have all children being present and valid. I guess we chose (1) > as of now. > > So I suppose it means even with your patch, it won't help either as long as > MRs are properties, and they can already all be gone in a device finalize() > even with your new patch. The owner can object_ref() to keep the memory region alive. > > From that POV, qdev unrealize() could be a good place for such cleanups > while making sure all properties are present.> >> >>> >>> My gut feeling is when we need serious MR refcounting (I'd expect due to >>> the current heavy code base it might be easier to start a new project if >>> that's required.. that's why I was thinking maybe it will not happen.. but >>> if it will..), we'll do more than your change, and that also means >>> memory_region_ref() must start to refcount MRs, because a serious MR >>> separate refcounting should mean MR can go on the fly before the owner. >> >> Actually there is one example: virtio_gpu_virgl_map_resource_blob() in >> hw/display/virtio-gpu-virgl.c creates a MR that can be removed before the >> device. To make this possible, it specifies MRs themselves as their and let >> them refcount without help of the device. > > .. I am definitely surprised that we have code that assigns obj->parent to > be itself. > > Putting the self parenting aside.. and to the topic: I don't think this is > an example for internal-MR use case? > > When the owner is itself, then it's not sharing the owner of the parent MR > (which is b->hostmem in this case, which should probably be owned by > VirtIOGPU object instead). So IIUC no matter which way we choose on the > current discussion, it won't affect the result. > > Not to mention, the MRs are always properly detached always when the > resource is unmapped: > > virtio_gpu_virgl_unmap_resource_blob(): > /* memory region owns self res->mr object and frees it by itself */ > memory_region_set_enabled(mr, false); > memory_region_del_subregion(&b->hostmem, mr); > > So from that POV it's a very good citizen.. it doesn't need anything to be > auto detached. > > Below can be off topic, but since we're discussing this use case.. > > My gut feeling is that it can cause trouble at some point having MR itself > as parent. > > For example, I will not be surprised that some day QMP command > qom-list-properties provide a --deep pararmeter, then this will hang that > command trying to look at child forever in a loop. > > It can easily break in other ways too, e.g. when we add an assertion > anywhere for "obj->parent != obj" (which should really make sense.. from a > object model POV), or when we want to take a ref to parent (for obj->parent > reference) then it'll be a "real" circular reference, comparing to what > we're discussing on auto-detach so far (which is IIUC a pretty "weak" > circular ref; IOW, if del_subregion is always properly done, this patch not > needed). > > Meanwhile the other free() overwrite: > > OBJECT(mr)->free = virtio_gpu_virgl_hostmem_region_free; > > I suppose it can be error prone too whenever we want to provide a common > free() for MRs and this one can be easily overlooked.. > > I'm not familiar with GPU stuff, but reading it closer I do feel like it's > kind of a slight abuse on all above.. > > If to stick with "owner shares the same lifecycle for its MRs" idea and fix > all above, IMHO we could make virtio_gpu_virgl_hostmem_region to be a QOM > object, then it can be a proper parent for the MR. >
On Sat, Jan 18, 2025 at 07:15:56PM +0900, Akihiko Odaki wrote: > On 2025/01/18 2:46, Peter Xu wrote: > > On Fri, Jan 17, 2025 at 03:24:34PM +0900, Akihiko Odaki wrote: > > > On 2025/01/16 23:33, Peter Xu wrote: > > > > On Thu, Jan 16, 2025 at 02:37:38PM +0900, Akihiko Odaki wrote: > > > > > On 2025/01/16 1:14, Peter Xu wrote: > > > > > > On Thu, Jan 16, 2025 at 12:52:56AM +0900, Akihiko Odaki wrote: > > > > > > > Functionally, the ordering of container/subregion finalization matters if > > > > > > > some device tries to a container during finalization. In such a case, > > > > > > | > > > > > > ^ something is missing here, feel free to complete this. > > > > > > > > > > Oops, I meant: functionally, the ordering of container/subregion > > > > > finalization matters if some device tries to use a container during > > > > > finalization. > > > > > > > > This is true, though if we keep the concept of "all the MRs share the same > > > > lifecycle of the owner" idea, another fix of such is simply moving the > > > > container access before any detachment of MRs. > > > > > > > > > > > > > > > > > > > > > > removing subregions from the container at random timing can result in an > > > > > > > unexpected behavior. There is little chance to have such a scenario but we > > > > > > > should stay the safe side if possible. > > > > > > > > > > > > It sounds like a future feature, and I'm not sure we'll get there, so I > > > > > > don't worry that much. Keeping refcount core idea simple is still very > > > > > > attractive to me. I still prefer we have complete MR refcounting iff when > > > > > > necessary. It's also possible it'll never happen to QEMU. > > > > > > > > > > > > > > > > It's not just about the future but also about compatibility with the current > > > > > device implementations. I will not be surprised even if the random ordering > > > > > of subregion finalization breaks one of dozens of devices we already have. > > > > > We should pay attention the details as we are touching the core > > > > > infrastructure. > > > > > > > > Yes, if we can find any such example that we must follow the order of MR > > > > destruction, I think that could justify your approach will be required but > > > > not optional. It's just that per my understanding there should be none, > > > > and even if there're very few outliers, it can still be trivially fixed as > > > > mentioned above. > > > > > > It can be fixed but that means we need auditing the code of devices or wait > > > until we get a bug report. > > > > We'd better have a solid example. > > > > And for this specific question, IIUC we can have such problem even if > > internal-ref start to use MR refcounts. > > > > It's because we have a not very straightforward way of finalize() an > > object, which is freeing all properties before its own finalize().. > > > > static void object_finalize(void *data) > > { > > ... > > object_property_del_all(obj); > > object_deinit(obj, ti); > > ... > > } > > > > I think it used to be the other way round (which will be easier to > > understand to me..), but changed after 76a6e1cc7cc. It could boil down to > > two dependencies: (1) children's unparent() callback wanting to have the > > parent being present and valid, and (2) parent's finalize() callback > > wanting to have all children being present and valid. I guess we chose (1) > > as of now. > > > > So I suppose it means even with your patch, it won't help either as long as > > MRs are properties, and they can already all be gone in a device finalize() > > even with your new patch. > > The owner can object_ref() to keep the memory region alive. Do you mean explicitly (rather by the add_subregion)? Why an owner need to do it at all, if it knows the MR is part of itself?
diff --git a/include/exec/memory.h b/include/exec/memory.h index 9458e2801d50..ca247343f433 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s); * memory_region_add_subregion() to add subregions. * * @mr: the #MemoryRegion to be initialized - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: used for debugging; not visible to the user or ABI * @size: size of the region; any subregions beyond this size will be clipped */ @@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr, uint64_t size); /** - * memory_region_ref: Add 1 to a memory region's reference count + * memory_region_ref: Add a reference to the owner of a memory region * - * Whenever memory regions are accessed outside the BQL, they need to be - * preserved against hot-unplug. MemoryRegions actually do not have their - * own reference count; they piggyback on a QOM object, their "owner". - * This function adds a reference to the owner. - * - * All MemoryRegions must have an owner if they can disappear, even if the - * device they belong to operates exclusively under the BQL. This is because - * the region could be returned at any time by memory_region_find, and this - * is usually under guest control. + * This function adds a reference to the owner of a memory region to keep the + * memory region alive. It does nothing if the owner is not present as a memory + * region without owner will never die. + * For references internal to the owner, use object_ref() instead to avoid a + * circular reference. + * See docs/devel/memory.rst to know about owner. * * @mr: the #MemoryRegion */ void memory_region_ref(MemoryRegion *mr); /** - * memory_region_unref: Remove 1 to a memory region's reference count + * memory_region_unref: Remove a reference to the memory region of the owner * - * Whenever memory regions are accessed outside the BQL, they need to be - * preserved against hot-unplug. MemoryRegions actually do not have their - * own reference count; they piggyback on a QOM object, their "owner". - * This function removes a reference to the owner and possibly destroys it. + * This function removes a reference to the owner of a memory region and + * possibly destroys the owner along with the memory region. It does nothing if + * the owner is not present. + * See docs/devel/memory.rst to know about owner. * * @mr: the #MemoryRegion */ @@ -1255,7 +1252,7 @@ void memory_region_unref(MemoryRegion *mr); * if @size is nonzero, subregions will be clipped to @size. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @ops: a structure containing read and write callbacks to be used when * I/O is performed on the region. * @opaque: passed to the read and write callbacks of the @ops structure. @@ -1275,7 +1272,7 @@ void memory_region_init_io(MemoryRegion *mr, * directly. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: Region name, becomes part of RAMBlock name used in migration stream * must be unique within any device * @size: size of the region. @@ -1298,7 +1295,7 @@ bool memory_region_init_ram_nomigrate(MemoryRegion *mr, * modify memory directly. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: Region name, becomes part of RAMBlock name used in migration stream * must be unique within any device * @size: size of the region. @@ -1328,7 +1325,7 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr, * canceled. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: Region name, becomes part of RAMBlock name used in migration stream * must be unique within any device * @size: used size of the region. @@ -1357,7 +1354,7 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr, * mmap-ed backend. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: Region name, becomes part of RAMBlock name used in migration stream * must be unique within any device * @size: size of the region. @@ -1390,7 +1387,7 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr, * mmap-ed backend. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: the name of the region. * @size: size of the region. * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM, @@ -1421,7 +1418,7 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr, * region will modify memory directly. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: Region name, becomes part of RAMBlock name used in migration stream * must be unique within any device * @size: size of the region. @@ -1449,7 +1446,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr, * skip_dump flag. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: the name of the region. * @size: size of the region. * @ptr: memory to be mapped; must contain at least @size bytes. @@ -1469,7 +1466,7 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr, * part of another memory region. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: used for debugging; not visible to the user or ABI * @orig: the region to be referenced; @mr will be equivalent to * @orig between @offset and @offset + @size - 1. @@ -1495,7 +1492,7 @@ void memory_region_init_alias(MemoryRegion *mr, * of the caller. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: Region name, becomes part of RAMBlock name used in migration stream * must be unique within any device * @size: size of the region. @@ -1518,7 +1515,7 @@ bool memory_region_init_rom_nomigrate(MemoryRegion *mr, * of the caller. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @ops: callbacks for write access handling (must not be NULL). * @opaque: passed to the read and write callbacks of the @ops structure. * @name: Region name, becomes part of RAMBlock name used in migration stream @@ -1554,7 +1551,7 @@ bool memory_region_init_rom_device_nomigrate(MemoryRegion *mr, * @_iommu_mr: the #IOMMUMemoryRegion to be initialized * @instance_size: the IOMMUMemoryRegion subclass instance size * @mrtypename: the type name of the #IOMMUMemoryRegion - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: used for debugging; not visible to the user or ABI * @size: size of the region. */ @@ -1570,7 +1567,7 @@ void memory_region_init_iommu(void *_iommu_mr, * region will modify memory directly. * * @mr: the #MemoryRegion to be initialized - * @owner: the object that tracks the region's reference count (must be + * @owner: the object that keeps the region alive (must be * TYPE_DEVICE or a subclass of TYPE_DEVICE, or NULL) * @name: name of the memory region * @size: size of the region in bytes @@ -1616,7 +1613,7 @@ bool memory_region_init_ram_guest_memfd(MemoryRegion *mr, * If you pass a non-NULL non-device @owner then we will assert. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: Region name, becomes part of RAMBlock name used in migration stream * must be unique within any device * @size: size of the region. @@ -1647,7 +1644,7 @@ bool memory_region_init_rom(MemoryRegion *mr, * If you pass a non-NULL non-device @owner then we will assert. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @ops: callbacks for write access handling (must not be NULL). * @opaque: passed to the read and write callbacks of the @ops structure. * @name: Region name, becomes part of RAMBlock name used in migration stream