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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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.