|
| From: | Akihiko Odaki |
| Subject: | Re: [PATCH v7 1/2] memory: Update inline documentation |
| Date: | Sat, 11 Jan 2025 13:15:24 +0900 |
| User-agent: | Mozilla Thunderbird |
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
| [Prev in Thread] | Current Thread | [Next in Thread] |