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