system/memory.c | 22 +++++++++++++---- tests/qtest/ivshmem-test.c | 50 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 5 deletions(-)
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;
* we do not ref/unref them because it slows down DMA sensibly.
*/
- if (mr && mr->owner) {
- object_ref(mr->owner);
- }
+ object_ref(mr->owner);
}
void memory_region_unref(MemoryRegion *mr)
{
- if (mr && mr->owner) {
- object_unref(mr->owner);
+ /* Regions without an owner are considered static. */
+ if (!mr || !mr->owner) {
+ return;
+ }
+ if (mr->ram) {
+ object_unref(OBJECT(mr));
+ return;
}
+ object_unref(mr->owner);
}
uint64_t memory_region_size(MemoryRegion *mr)
diff --git a/tests/qtest/ivshmem-test.c b/tests/qtest/ivshmem-test.c
index fb45fdeb07..44f712e9ae 100644
--- a/tests/qtest/ivshmem-test.c
+++ b/tests/qtest/ivshmem-test.c
@@ -194,6 +194,55 @@ static void test_ivshmem_single(void)
cleanup_vm(s);
}
+static void test_memory_region_lifecycle(void)
+{
+ /* Device creation triggers memory region mapping (calls ref) */
+ IVState state1, state2;
+ uint32_t test_data, read_data;
+ int i;
+
+ setup_vm(&state1);
+
+ /* Basic verification that device works */
+ test_data = 0x12345678;
+ write_mem(&state1, 0, &test_data, sizeof(test_data));
+ read_mem(&state1, 0, &read_data, sizeof(read_data));
+ g_assert_cmpuint(read_data, ==, test_data);
+
+ /* Multiple devices stress test memory region ref counting */
+ setup_vm(&state2);
+
+ /* Verify both devices work independently */
+ test_data = 0xDEADBEEF;
+ write_mem(&state2, 4, &test_data, sizeof(test_data));
+ read_mem(&state2, 4, &read_data, sizeof(read_data));
+ g_assert_cmpuint(read_data, ==, test_data);
+
+ /* Device destruction triggers memory region unmapping (calls unref) */
+ cleanup_vm(&state1);
+
+ /* Verify remaining device still works after first device cleanup */
+ read_mem(&state2, 4, &read_data, sizeof(read_data));
+ g_assert_cmpuint(read_data, ==, test_data);
+
+ /* Final cleanup */
+ cleanup_vm(&state2);
+
+ /* Quick lifecycle stress test - multiple create/destroy cycles */
+ for (i = 0; i < 5; i++) {
+ IVState temp_state;
+ setup_vm(&temp_state);
+
+ /* Quick test to ensure device works */
+ test_data = 0x1000 + i;
+ write_mem(&temp_state, 0, &test_data, sizeof(test_data));
+ read_mem(&temp_state, 0, &read_data, sizeof(read_data));
+ g_assert_cmpuint(read_data, ==, test_data);
+
+ cleanup_vm(&temp_state);
+ }
+}
+
static void test_ivshmem_pair(void)
{
IVState state1, state2, *s1, *s2;
@@ -503,6 +552,7 @@ int main(int argc, char **argv)
tmpserver = g_strconcat(tmpdir, "/server", NULL);
qtest_add_func("/ivshmem/single", test_ivshmem_single);
+ qtest_add_func("/ivshmem/memory-lifecycle", test_memory_region_lifecycle);
qtest_add_func("/ivshmem/hotplug", test_ivshmem_hotplug);
qtest_add_func("/ivshmem/memdev", test_ivshmem_memdev);
if (g_test_slow()) {
--
2.49.0On 23.07.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(-) Did we discuss extending the doc as well, to clarify which scenario is now supported? -- Cheers, David / dhildenb
On Wed, Jul 23, 2025 at 2:43 PM David Hildenbrand <david@redhat.com> wrote: > > On 23.07.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(-) > > Did we discuss extending the doc as well, to clarify which scenario is > now supported? Not that I remember? But it is a good idea. I will update the docs for the next version of this patch. > > -- > Cheers, > > David / dhildenb >
On 23.07.25 14:48, Albert Esteve wrote:
> On Wed, Jul 23, 2025 at 2:43 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 23.07.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(-)
>>
>> Did we discuss extending the doc as well, to clarify which scenario is
>> now supported?
>
> Not that I remember? But it is a good idea. I will update the docs for
> the next version of this patch.
Maybe I never sent it to you while brainstorming. Here is what I had:
diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
index 57fb2aec76..5c4bc9ced5 100644
--- a/docs/devel/memory.rst
+++ b/docs/devel/memory.rst
@@ -143,11 +143,13 @@ Region lifecycle
----------------
A region is created by one of the memory_region_init*() functions and
-attached to an object, which acts as its owner or parent. QEMU ensures
-that the owner object remains alive as long as the region is visible to
-the guest, or as long as the region is in use by a virtual CPU or another
-device. For example, the owner object will not die between an
-address_space_map operation and the corresponding address_space_unmap.
+attached to an object, which acts as its owner or parent.
+
+For non-RAM regions, QEMU ensures that the owner object remains alive as
+long as the region is visible to the guest, or as long as the region is in
+use by a virtual CPU or another device. For example, the owner object will
+not die between an address_space_map operation and the corresponding
+address_space_unmap. For RAM regions, this is not guaranteed.
After creation, a region can be added to an address space or a
container with memory_region_add_subregion(), and removed using
@@ -174,7 +176,8 @@ callback. The dynamically allocated data structure that contains the
memory region then should obviously be freed in the instance_finalize
callback as well.
-If you break this rule, the following situation can happen:
+If you break this rule, the following situation can happen for non-RAM
+regions:
- the memory region's owner had a reference taken via memory_region_ref
(for example by address_space_map)
--
Cheers,
David / dhildenb
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?
> * we do not ref/unref them because it slows down DMA sensibly.
> */
> - if (mr && mr->owner) {
> - object_ref(mr->owner);
> - }
> + object_ref(mr->owner);
> }
>
> void memory_region_unref(MemoryRegion *mr)
> {
> - if (mr && mr->owner) {
> - object_unref(mr->owner);
> + /* Regions without an owner are considered static. */
> + if (!mr || !mr->owner) {
> + return;
> + }
> + if (mr->ram) {
> + object_unref(OBJECT(mr));
> + return;
> }
> + object_unref(mr->owner);
> }
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.
BR,
Albert.
>
> > * we do not ref/unref them because it slows down DMA sensibly.
> > */
> > - if (mr && mr->owner) {
> > - object_ref(mr->owner);
> > - }
> > + object_ref(mr->owner);
> > }
> >
> > void memory_region_unref(MemoryRegion *mr)
> > {
> > - if (mr && mr->owner) {
> > - object_unref(mr->owner);
> > + /* Regions without an owner are considered static. */
> > + if (!mr || !mr->owner) {
> > + return;
> > + }
> > + if (mr->ram) {
> > + object_unref(OBJECT(mr));
> > + return;
> > }
> > + object_unref(mr->owner);
> > }
>
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
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.
--
Cheers,
David / dhildenb
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
© 2016 - 2025 Red Hat, Inc.