Re: [PATCH v2 09/15] memory: Do not create circular reference with subregion

[Peter Xu]

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.

Thanks,

-- 
Peter Xu