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

[Stefan Hajnoczi <stefanha@gmail.com>]

On Wed, Jul 23, 2025 at 8:44 AM Albert Esteve <aesteve@redhat.com> 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.

I had similar thoughts to Phil when reading the commit description. It
says what the patch is doing but does not really describe why.

Please update the commit description to state why RAM regions need
different refcount semantics.

Stefan