Re: [RFC] memory.c: improve refcounting for RAM vs MMIO regions

[David Hildenbrand <david@redhat.com>]

On 23.07.25 14:45, David Hildenbrand wrote:
> On 23.07.25 14:42, Albert Esteve wrote:
>> On Wed, Jul 23, 2025 at 2:32 PM Philippe Mathieu-Daudé
>> <philmd@linaro.org> wrote:
>>>
>>> Hi,
>>>
>>> On 23/7/25 14:19, Albert Esteve wrote:
>>>> In the last version of the SHMEM MAP/UNMAP [1] there was a
>>>> comment [2] from Stefan about the lifecycle of the memory
>>>> regions.
>>>>
>>>> After some discussion, David Hildenbrand proposed
>>>> to detect RAM regions and handle refcounting differently
>>>> to clear the initial concern. This RFC patch is
>>>> meant for gathering feedback from others
>>>> (i.e., Paolo Bonzini and Peter Xu).
>>>>
>>>> [1] https://patchwork.ozlabs.org/project/qemu-devel/list/?series=460121
>>>> [2] https://patchwork.ozlabs.org/comment/3528600/
>>>>
>>>> ---
>>>>
>>>> This patch enhances memory_region_ref() and memory_region_unref()
>>>> to handle RAM and MMIO memory regions differently, improving
>>>> reference counting semantics.
>>>>
>>>> RAM regions now reference/unreference the memory region object
>>>> itself, while MMIO continue to reference/unreference the owner
>>>> device as before.
>>>>
>>>> An additional qtest has been added to stress the memory
>>>> lifecycle. All tests pass as these changes keep backward
>>>> compatibility (prior behaviour is kept for MMIO regions).
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com >
>>>> Signed-off-by: Albert Esteve <aesteve@redhat.com>
>>>> ---
>>>>     system/memory.c            | 22 +++++++++++++----
>>>>     tests/qtest/ivshmem-test.c | 50 ++++++++++++++++++++++++++++++++++++++
>>>>     2 files changed, 67 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/system/memory.c b/system/memory.c
>>>> index 5646547940..48ab6e5592 100644
>>>> --- a/system/memory.c
>>>> +++ b/system/memory.c
>>>> @@ -1826,6 +1826,14 @@ Object *memory_region_owner(MemoryRegion *mr)
>>>>
>>>>     void memory_region_ref(MemoryRegion *mr)
>>>>     {
>>>> +    /* Regions without an owner are considered static. */
>>>> +    if (!mr || !mr->owner) {
>>>> +        return;
>>>> +    }
>>>> +    if (mr->ram) {
>>>> +        object_ref(OBJECT(mr));
>>>> +        return;
>>>> +    }
>>>>         /* 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.
>>>> @@ -1836,16 +1844,20 @@ void memory_region_ref(MemoryRegion *mr)
>>>>          * Memory regions without an owner are supposed to never go away;
>>>
>>> What are the use cases for MRs without QOM owner?
>>
>> Not sure if you are asking about the logic or the actual usecases
>> where these MRs would make sense.
>>
>> Regarding the logic, note the early return at the beginning of the
>> function, so that this comment is kept valid. In short, nothing
>> changes.
>>
>> Regarding the usecases for these type of memories, I can think of
>> system memory or container regions as examples. But there are
>> certainly more experienced people in this thread that can answer you
>> better than me.
> 
> The thing is: these MRs have an owner, but to make the limitation
> spelled out in the doc (see my comment) work, we must refcount the MR
> itself.
> 
> We could likely ref both (RAM region and the owner), but it's documented
> that that results in a performance problem.

Correction: we can't easily, because of the object_unparent IIRC.

-- 
Cheers,

David / dhildenb