On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote:
A memory region does not use their own reference counters, but instead
piggybacks on another QOM object, "owner" (unless the owner is not the
memory region itself). When creating a subregion, a new reference to the
owner of the container must be created. However, if the subregion is
owned by the same QOM object, this result in a self-reference, and make
the owner immortal. Avoid such a self-reference.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
system/memory.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index 74cd73ebc78b..949f5016a68d 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2638,7 +2638,10 @@ static void
memory_region_update_container_subregions(MemoryRegion *subregion)
memory_region_transaction_begin();
- memory_region_ref(subregion);
+ if (mr->owner != subregion->owner) {
+ memory_region_ref(subregion);
+ }
+
QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
if (subregion->priority >= other->priority) {
QTAILQ_INSERT_BEFORE(other, subregion, subregions_link);
@@ -2696,7 +2699,11 @@ void memory_region_del_subregion(MemoryRegion *mr,
assert(alias->mapped_via_alias >= 0);
}
QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
- memory_region_unref(subregion);
+
+ if (mr->owner != subregion->owner) {
+ memory_region_unref(subregion);
+ }
+
memory_region_update_pending |= mr->enabled && subregion->enabled;
memory_region_transaction_commit();
}
This does look like a real issue.. the patch looks reasonable to me, but I
wonder whether we should start to add some good comments in code to reflect
that complexity starting from this one. The MR refcount isn't easy to
understand to me.
It also lets me start to wonder how MR refcount went through until it looks
like today.. It's definitely not extremely intuitive to use mr->owner as
the object to do refcounting if mr itself does has its own QObject,
meanwhile it has other tricks around.
E.g. the first thing I stumbled over when looking was the optimization
where we will avoid refcounting the mr when there's no owner, and IIUC it
was for the case when the "guest memory" (which will never be freed) used
to have no owner so we can speedup DMA if we know it won't go away.
https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/
commit 612263cf33062f7441a5d0e3b37c65991fdc3210
Author: Paolo Bonzini <pbonzini@redhat.com>
Date: Wed Dec 9 11:44:25 2015 +0100
memory: avoid unnecessary object_ref/unref
For the common case of DMA into non-hotplugged RAM, it is unnecessary
but expensive to do object_ref/unref. Add back an owner field to
MemoryRegion, so that these memory regions can skip the reference
counting.
If so, it looks like it will stop working with memory-backends get
involved? As I think those MRs will have owner set always, and I wonder
whether memory-backends should be the major way to specify guest memory now
and in the future. So I'm not sure how important that optimization is as
of now, and whether we could "simplify" it back to always do the refcount
if the major scenarios will not adopt it.
The other issue is we used owner refcount from the start of
memory_region_ref() got introduced, since:
commit 46637be269aaaceb9867ffdf176e906401138fff
Author: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue May 7 09:06:00 2013 +0200
memory: add ref/unref
And we still have that in our document, even though I don't think it's true
anymore:
* ... 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.
It looks like what happened is when introduced the change, MR is not a QOM
object yet. But it later is..
I mentioned all these only because I found that _if_ we can keep mr
refcounting as simple as other objects:
memory_region_ref(mr)
{
object_ref(OBJECT(mr));
}
Then looks like this "recursive refcount" problem can also go away. I'm
curious whether you or anyone tried to explore that path, or whether above
doesn't make sense at all.