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

[Peter Xu]

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,

-- 
Peter Xu