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();
 }

-- 
2.45.2

On Thu, 27 Jun 2024 at 14:40, Akihiko Odaki <akihiko.odaki@daynix.com> 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();
>  }

I was having another look at leaks this week, and I tried
this patch to see how many of the leaks I was seeing it
fixed. I found however that for arm it results in an
assertion when the device-introspection-test exercises
the "imx7.analog" device. By-hand repro:

$ ./build/asan/qemu-system-aarch64 -display none -machine none -accel
qtest -monitor stdio
==712838==WARNING: ASan doesn't fully support makecontext/swapcontext
functions and may produce false positives in some cases!
QEMU 9.0.92 monitor - type 'help' for more information
(qemu) device_add imx7.analog,help
qemu-system-aarch64: ../../system/memory.c:1777: void
memory_region_finalize(Object *): Assertion `!mr->container' failed.
Aborted (core dumped)

It may be well be that this is a preexisting bug that's only
exposed by this refcount change causing us to actually try
to dispose of the memory regions.

I think that what's happening here is that the device
object has multiple MemoryRegions, each of which is a child
QOM property. One of these MRs is a "container MR", and the
other three are actual-content MRs which the device put into
the container when it created them. When we deref the device,
we go through all the child QOM properties unparenting and
unreffing them. However, there's no particular ordering
here, and it happens that we try to unref one of the
actual-content MRs first. That MR is still inside the
container MR, so we hit the assert. If we had happened to
unref the container MR first then memory_region_finalize()
would have removed all the subregions from it, avoiding
the problem.

I'm not sure what the best fix would be here -- that assert
is there as a guard that the region isn't visible in
any address space, so maybe it needs to be made a bit
cleverer about the condition it checks? e.g. in this
example although mr->container is not NULL,
mr->container->container is NULL. Or we could check
whether the mr->container->owner is the same as the
mr->owner and allow a non-NULL mr->container in that case.
I don't know this subsystem well enough so I'm just
making random stabs here, though.

thanks
-- PMM

On Thu, Aug 22, 2024 at 06:10:43PM +0100, Peter Maydell wrote:
> On Thu, 27 Jun 2024 at 14:40, Akihiko Odaki <akihiko.odaki@daynix.com> 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();
> >  }
> 
> I was having another look at leaks this week, and I tried
> this patch to see how many of the leaks I was seeing it
> fixed. I found however that for arm it results in an
> assertion when the device-introspection-test exercises
> the "imx7.analog" device. By-hand repro:
> 
> $ ./build/asan/qemu-system-aarch64 -display none -machine none -accel
> qtest -monitor stdio
> ==712838==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> QEMU 9.0.92 monitor - type 'help' for more information
> (qemu) device_add imx7.analog,help
> qemu-system-aarch64: ../../system/memory.c:1777: void
> memory_region_finalize(Object *): Assertion `!mr->container' failed.
> Aborted (core dumped)
> 
> It may be well be that this is a preexisting bug that's only
> exposed by this refcount change causing us to actually try
> to dispose of the memory regions.
> 
> I think that what's happening here is that the device
> object has multiple MemoryRegions, each of which is a child
> QOM property. One of these MRs is a "container MR", and the
> other three are actual-content MRs which the device put into
> the container when it created them. When we deref the device,
> we go through all the child QOM properties unparenting and
> unreffing them. However, there's no particular ordering
> here, and it happens that we try to unref one of the
> actual-content MRs first. That MR is still inside the
> container MR, so we hit the assert. If we had happened to
> unref the container MR first then memory_region_finalize()
> would have removed all the subregions from it, avoiding
> the problem.
> 
> I'm not sure what the best fix would be here -- that assert
> is there as a guard that the region isn't visible in
> any address space, so maybe it needs to be made a bit
> cleverer about the condition it checks? e.g. in this
> example although mr->container is not NULL,
> mr->container->container is NULL.

If we keep looking at ->container we'll always see NULL, IIUC, because
either it's removed from its parent MR so it's NULL already, or at some
point it can start to point to a root mr of an address space, where should
also be NULL, afaiu.

> Or we could check whether the mr->container->owner is the same as the
> mr->owner and allow a non-NULL mr->container in that case.  I don't know
> this subsystem well enough so I'm just making random stabs here, though.

If with the assumption of this patch applied, then looks like it's pretty
legal a container MR and the child MRs be finalized in any order when the
owner device is being destroyed.

IIUC the MR should be destined to be invisible until this point, with or
without the fact that mr->container is NULL.  It's because anyone who
references the MR should do memory_region_ref() first, which takes the
owner's refcount.  Here if MR finalize() is reached I think it means the
owner refcount must be zero.  So it looks to me the only possible case when
mr->container is non-NULL is it's used internally like this.  Then it's
invisible and also safe to be detached even if container != NULL.

So.. I wonder whether below would make sense, on top of this existing
patch.

===8<===
diff --git a/system/memory.c b/system/memory.c
index 1c00df8305..54a9d9e5f9 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1771,16 +1771,23 @@ static void memory_region_finalize(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
 
-    assert(!mr->container);
-
-    /* We know the region is not visible in any address space (it
-     * does not have a container and cannot be a root either because
-     * it has no references, so we can blindly clear mr->enabled.
-     * memory_region_set_enabled instead could trigger a transaction
-     * and cause an infinite loop.
+    /*
+     * We know the region is not visible in any address space, because
+     * normally MR's finalize() should be invoked by finalize() of the
+     * owner, which will remove all the properties including the MRs, and
+     * that can only happen when prior memory_region_ref() of the MR (which
+     * will ultimately take the owner's refcount) from elsewhere got
+     * properly released.
+     *
+     * So we can blindly clear mr->enabled, unlink both the upper container
+     * or all subregions.  memory_region_set_enabled() won't work instead,
+     * as it could trigger a transaction and cause an infinite loop.
      */
     mr->enabled = false;
     memory_region_transaction_begin();
+    if (mr->container) {
+        memory_region_del_subregion(mr->container, mr);
+    }
     while (!QTAILQ_EMPTY(&mr->subregions)) {
         MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions);
===8<===

Thanks,

-- 
Peter Xu

On 2024/08/23 6:01, Peter Xu wrote:
> On Thu, Aug 22, 2024 at 06:10:43PM +0100, Peter Maydell wrote:
>> On Thu, 27 Jun 2024 at 14:40, Akihiko Odaki <akihiko.odaki@daynix.com> 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();
>>>   }
>>
>> I was having another look at leaks this week, and I tried
>> this patch to see how many of the leaks I was seeing it
>> fixed. I found however that for arm it results in an
>> assertion when the device-introspection-test exercises
>> the "imx7.analog" device. By-hand repro:
>>
>> $ ./build/asan/qemu-system-aarch64 -display none -machine none -accel
>> qtest -monitor stdio
>> ==712838==WARNING: ASan doesn't fully support makecontext/swapcontext
>> functions and may produce false positives in some cases!
>> QEMU 9.0.92 monitor - type 'help' for more information
>> (qemu) device_add imx7.analog,help
>> qemu-system-aarch64: ../../system/memory.c:1777: void
>> memory_region_finalize(Object *): Assertion `!mr->container' failed.
>> Aborted (core dumped)
>>
>> It may be well be that this is a preexisting bug that's only
>> exposed by this refcount change causing us to actually try
>> to dispose of the memory regions.
>>
>> I think that what's happening here is that the device
>> object has multiple MemoryRegions, each of which is a child
>> QOM property. One of these MRs is a "container MR", and the
>> other three are actual-content MRs which the device put into
>> the container when it created them. When we deref the device,
>> we go through all the child QOM properties unparenting and
>> unreffing them. However, there's no particular ordering
>> here, and it happens that we try to unref one of the
>> actual-content MRs first. That MR is still inside the
>> container MR, so we hit the assert. If we had happened to
>> unref the container MR first then memory_region_finalize()
>> would have removed all the subregions from it, avoiding
>> the problem.
>>
>> I'm not sure what the best fix would be here -- that assert
>> is there as a guard that the region isn't visible in
>> any address space, so maybe it needs to be made a bit
>> cleverer about the condition it checks? e.g. in this
>> example although mr->container is not NULL,
>> mr->container->container is NULL.
> 
> If we keep looking at ->container we'll always see NULL, IIUC, because
> either it's removed from its parent MR so it's NULL already, or at some
> point it can start to point to a root mr of an address space, where should
> also be NULL, afaiu.
> 
>> Or we could check whether the mr->container->owner is the same as the
>> mr->owner and allow a non-NULL mr->container in that case.  I don't know
>> this subsystem well enough so I'm just making random stabs here, though.
> 
> If with the assumption of this patch applied, then looks like it's pretty
> legal a container MR and the child MRs be finalized in any order when the
> owner device is being destroyed.
> 
> IIUC the MR should be destined to be invisible until this point, with or
> without the fact that mr->container is NULL.  It's because anyone who
> references the MR should do memory_region_ref() first, which takes the
> owner's refcount.  Here if MR finalize() is reached I think it means the
> owner refcount must be zero.  So it looks to me the only possible case when
> mr->container is non-NULL is it's used internally like this.  Then it's
> invisible and also safe to be detached even if container != NULL.

It is still nice if we can protect internal uses; it is theoretically 
possible for the owner to remove the subregion at runtime, and we want 
to ensure the subregion will be kept alive while its container is even 
in such a case. We can do so by creating a reference to the subregion 
itself instead of its owner. I sent v4 for this change.

Regards,
Akihiko Odaki

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.

Thanks,

-- 
Peter Xu

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.

Regards,
Akihiko Odaki

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

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

On 2024/07/09 1:40, Peter Xu wrote:
> 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.

We don't even need reference-counting memory regions if we are sure 
there would be no reference to a MR when the device owning it is gone 
because a MR is already alive when the device owning it is.

Unfortunately that is not the case. If you grep for memory_region_ref(), 
you will find lot of places that asynchronously calls 
memory_region_unref(). Devices can be removed whenever the BQL is 
unlocked so we certainly need to count references.

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

I think it is better to remove the reference counter from Object and 
create a subclass like RefCounted to add one for sub-struct-field cases. 
There are exceptional cases in virtio-gpu-gl that dynamically allocate 
MRs but such a device can create Objects that functions as owners of 
dynamically-allocated MRs.

Such a change is independent and out of the scope of this change though. 
It also requires to convert all other usage of Object to RefCounted, 
which is not really trivial.

Regards,
Akihiko Odaki

On 2024/07/08 17:06, 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).

I meant memory_region_update_container_subregions() is the only code 
that creates such a reference. The function itself is called in many 
code paths.