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.od...@daynix.com> > > > Reviewed-by: Peter Xu <pet...@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.. -- Peter Xu
