Re: [PATCH v7 1/2] memory: Update inline documentation

[Peter Xu]

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..

-- 
Peter Xu