On Mon, Jul 08, 2024 at 10:06:44AM +0200, Philippe Mathieu-Daudé wrote:
On 6/7/24 13:59, Akihiko Odaki wrote:
On 2024/07/03 2:44, Peter Xu wrote:
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.
It unfortunately does not solve the problem.
The underlying problem is that the whole device must be kept alive while
its memory region are. Indeed MemoryRegions do have refcounts, but
incrementing them do not extend the lifetime of the devices (i.e., the
owners). The refcount of the owners must be incremented for correctness.
Referencing a subregion MemoryRegion from its container MemoryRegion
owned by the same device is an exceptional case. Incrementing the
refcount of the owner extends the owner's lifetime to forever.
Is it really an exceptional case?
What I'm seeing are a lot of devices creating MR and never bother
to destroy them, so indeed owner (device) refcount never reaches 0.
Most of the time when we create MR in .instance_init/.realize,
we neglect to implement the undo path (.instance_finalize or
.unrealize).
Right. The cross-reference of MR <-> device makes sense from the concept
level, but I'm not sure how much we rely on that from QEMU perspective
currently. It complicates refcount and looks like it needs some thoughts.
One proof is, when I replied I actually tested all qemu qtests with below
patch and nothing yet explode:
===8<===
diff --git a/system/memory.c b/system/memory.c
index 2d69521360..a1d2c1f808 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1801,26 +1801,12 @@ Object *memory_region_owner(MemoryRegion *mr)
void memory_region_ref(MemoryRegion *mr)
{
- /* MMIO callbacks most likely will access data that belongs
- * to the owner, hence the need to ref/unref the owner whenever
- * the memory region is in use.
- *
- * The memory region is a child of its owner. As long as the
- * owner doesn't call unparent itself on the memory region,
- * ref-ing the owner will also keep the memory region alive.
- * Memory regions without an owner are supposed to never go away;
- * we do not ref/unref them because it slows down DMA sensibly.
- */
- if (mr && mr->owner) {
- object_ref(mr->owner);
- }
+ object_ref(OBJECT(mr));
}
void memory_region_unref(MemoryRegion *mr)
{
- if (mr && mr->owner) {
- object_unref(mr->owner);
- }
+ object_unref(OBJECT(mr));
}
===8<===
I'm not sure how much it covers dynamic destruction of devices, though,
where the device creates internal MRs to the guest address space. I'm
actually wondering whether we have other barriers already to avoid having
any MR ops being invoked if a device was removed.
So I think above indeed would remove the cross-ref we used to have. It
might be a matter of whether that's fine for now to fix the cyclic ref
issue alone, and even if we still want some cross-ref.. whether we should
do it the old way. I mean, the cross-ref can also be done in other forms,
and it may not block MR has its own refcounts.
Even if we want to have dev<->MR refcount, I wonder whether we should avoid
having MR refcount the device, because it looks like "sub-object refs the
parent" which sounds like a good source of cyclic refcount issue. I wonder
whether device should ref the MR instead (when MR is dynamically created; I
am not sure whether we even need that if in Phil's comment where the MR
object is a sub-struct-field of the Device object). Then the device can
provide a proper destroy() function to release the MRs under it, perhaps by
disabling them first (then any ops should already fail after that), then
unref the MRs with the hope that they're the last ref holder then MRs
destroys there too.