[Qemu-devel] [PATCH] memory: Fix double unref of flatview

[Qemu-devel] [PATCH] memory: Fix double unref of flatview

[Matthew Rosato]

Since 374f2981d1 "memory: protect current_map by RCU",
address_space_update_topology unrefs the old_flatview twice,
once by call_rcu and once by direct call.  This patch removes
the direct call in favor of the call_rcu.  Fixes at least one
assertion failure seen in s390, where a ref count for a memory
region attempts to go negative during hot-unplug of guest memory.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 memory.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index 130152c..d08abe5 100644
--- a/memory.c
+++ b/memory.c
@@ -755,7 +755,6 @@ static void address_space_update_topology(AddressSpace *as)
 
     /* Writes are protected by the BQL.  */
     atomic_rcu_set(&as->current_map, new_view);
-    call_rcu(old_view, flatview_unref, rcu);
 
     /* Note that all the old MemoryRegions are still alive up to this
      * point.  This relieves most MemoryListeners from the need to
@@ -763,7 +762,7 @@ static void address_space_update_topology(AddressSpace *as)
      * outside the iothread mutex, in which case precise reference
      * counting is necessary.
      */
-    flatview_unref(old_view);
+    call_rcu(old_view, flatview_unref, rcu);
 
     address_space_update_ioeventfds(as);
 }
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] memory: Fix double unref of flatview

[Paolo Bonzini]

On 12/02/2015 17:21, Matthew Rosato wrote:
> Since 374f2981d1 "memory: protect current_map by RCU",
> address_space_update_topology unrefs the old_flatview twice,
> once by call_rcu and once by direct call.  This patch removes
> the direct call in favor of the call_rcu.  Fixes at least one
> assertion failure seen in s390, where a ref count for a memory
> region attempts to go negative during hot-unplug of guest memory.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

The two unrefs are correct.

One is needed to balance address_space_get_flatview; the other is 
needed because as->current_map does not point to old_view anymore.

You can remove them with something like this (your patch plus one
extra hunk):

diff --git a/memory.c b/memory.c
index a844ced..5add529 100644
--- a/memory.c
+++ b/memory.c
@@ -747,7 +747,7 @@ static void address_space_update_topology_pass(AddressSpace *as,
 
 static void address_space_update_topology(AddressSpace *as)
 {
-    FlatView *old_view = address_space_get_flatview(as);
+    FlatView *old_view = as->current_map;
     FlatView *new_view = generate_memory_topology(as->root);
 
     address_space_update_topology_pass(as, old_view, new_view, false);
@@ -755,7 +755,6 @@ static void address_space_update_topology(AddressSpace *as)
 
     /* Writes are protected by the BQL.  */
     atomic_rcu_set(&as->current_map, new_view);
-    call_rcu(old_view, flatview_unref, rcu);
 
     /* Note that all the old MemoryRegions are still alive up to this
      * point.  This relieves most MemoryListeners from the need to
@@ -763,7 +762,7 @@ static void address_space_update_topology(AddressSpace *as)
      * outside the iothread mutex, in which case precise reference
      * counting is necessary.
      */
-    flatview_unref(old_view);
+    call_rcu(old_view, flatview_unref, rcu);
 
     address_space_update_ioeventfds(as);
 }

but it wouldn't affect your bug.

Paolo

Re: [Qemu-devel] [PATCH] memory: Fix double unref of flatview

[Paolo Bonzini]

On 12/02/2015 17:21, Matthew Rosato wrote:
> Since 374f2981d1 "memory: protect current_map by RCU",
> address_space_update_topology unrefs the old_flatview twice,
> once by call_rcu and once by direct call.  This patch removes
> the direct call in favor of the call_rcu.  Fixes at least one
> assertion failure seen in s390, where a ref count for a memory
> region attempts to go negative during hot-unplug of guest memory.

This is buggy:

            MemoryRegion *standby_ram = g_new(MemoryRegion, 1);
            memory_region_init_ram(standby_ram, NULL, id, this_subregion_size, &error_abort);

...
                object_unparent(OBJECT(mr));
                g_free(mr);

Memory might not be released after object_unparent (and will not be
released if the RCU callbacks haven't run yet).

Your patch worked around it because it never freed the FlatView, and
thus the RCU callbacks never did anything on the memory regions.

Instead, you have to do this:

            MemoryRegion *standby_ram = g_new(MemoryRegion, 1);
            Object *obj = OBJECT(standby_ram);

            memory_region_init_ram(standby_ram, NULL, id, this_subregion_size, &error_abort);
            OBJECT(mr)->free = g_free;
...
            object_unparent(OBJECT(mr));

and the freeing will happen once the reference count goes to zero.

Paolo

> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  memory.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 130152c..d08abe5 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -755,7 +755,6 @@ static void address_space_update_topology(AddressSpace *as)
>  
>      /* Writes are protected by the BQL.  */
>      atomic_rcu_set(&as->current_map, new_view);
> -    call_rcu(old_view, flatview_unref, rcu);
>  
>      /* Note that all the old MemoryRegions are still alive up to this
>       * point.  This relieves most MemoryListeners from the need to
> @@ -763,7 +762,7 @@ static void address_space_update_topology(AddressSpace *as)
>       * outside the iothread mutex, in which case precise reference
>       * counting is necessary.
>       */
> -    flatview_unref(old_view);
> +    call_rcu(old_view, flatview_unref, rcu);
>  
>      address_space_update_ioeventfds(as);
>  }
> 

Re: [Qemu-devel] [PATCH] memory: Fix double unref of flatview

[Matthew Rosato]

On 02/12/2015 12:34 PM, Paolo Bonzini wrote:
> 
> 
> On 12/02/2015 17:21, Matthew Rosato wrote:
>> Since 374f2981d1 "memory: protect current_map by RCU",
>> address_space_update_topology unrefs the old_flatview twice,
>> once by call_rcu and once by direct call.  This patch removes
>> the direct call in favor of the call_rcu.  Fixes at least one
>> assertion failure seen in s390, where a ref count for a memory
>> region attempts to go negative during hot-unplug of guest memory.
> 
> This is buggy:
> 
>             MemoryRegion *standby_ram = g_new(MemoryRegion, 1);
>             memory_region_init_ram(standby_ram, NULL, id, this_subregion_size, &error_abort);
> 
> ...
>                 object_unparent(OBJECT(mr));
>                 g_free(mr);
> 
> Memory might not be released after object_unparent (and will not be
> released if the RCU callbacks haven't run yet).
> 
> Your patch worked around it because it never freed the FlatView, and
> thus the RCU callbacks never did anything on the memory regions.
> 
> Instead, you have to do this:
> 
>             MemoryRegion *standby_ram = g_new(MemoryRegion, 1);
>             Object *obj = OBJECT(standby_ram);
> 
>             memory_region_init_ram(standby_ram, NULL, id, this_subregion_size, &error_abort);
>             OBJECT(mr)->free = g_free;
> ...
>             object_unparent(OBJECT(mr));
> 
> and the freeing will happen once the reference count goes to zero.
> 
> Paolo
> 

Thanks Paolo, I like this change but it doesn't seem to help; still
hitting the same assertion with it applied.

Could it be that the order in which flatview_unref (and therefore
memory_region_unref) vs object_unparent(mr) matters (ie, object_unparent
should always happen last)?  Prior to RCUification, seems like the
old_view was always unreferenced prior to return from
memory_region_del_subregion (so, memory_region_unref always occurred
before the object_unparent(mr)).  Now, the two could happen in either
order -- and looking at memory_region_unref, it is specifically looking
at the existence of obj->parent.

Still investigating this, but I performed a litmus test:  if I directly
call flatview_unref twice in address_space_update_topology (rather than
1 direct & 1 via call_rcu), the assertion goes away.

Matt


>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>>  memory.c |    3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/memory.c b/memory.c
>> index 130152c..d08abe5 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -755,7 +755,6 @@ static void address_space_update_topology(AddressSpace *as)
>>  
>>      /* Writes are protected by the BQL.  */
>>      atomic_rcu_set(&as->current_map, new_view);
>> -    call_rcu(old_view, flatview_unref, rcu);
>>  
>>      /* Note that all the old MemoryRegions are still alive up to this
>>       * point.  This relieves most MemoryListeners from the need to
>> @@ -763,7 +762,7 @@ static void address_space_update_topology(AddressSpace *as)
>>       * outside the iothread mutex, in which case precise reference
>>       * counting is necessary.
>>       */
>> -    flatview_unref(old_view);
>> +    call_rcu(old_view, flatview_unref, rcu);
>>  
>>      address_space_update_ioeventfds(as);
>>  }
>>
> 

Re: [Qemu-devel] [PATCH] memory: Fix double unref of flatview

[Paolo Bonzini]

On 12/02/2015 20:32, Matthew Rosato wrote:
> Could it be that the order in which flatview_unref (and therefore
> memory_region_unref) vs object_unparent(mr) matters (ie, object_unparent
> should always happen last)?  Prior to RCUification, seems like the
> old_view was always unreferenced prior to return from
> memory_region_del_subregion (so, memory_region_unref always occurred
> before the object_unparent(mr)).  Now, the two could happen in either
> order -- and looking at memory_region_unref, it is specifically looking
> at the existence of obj->parent.

I think you're right.

To test your theory, you could try using call_rcu to unparent the memory
region.  The complete fix would be to wrap the MemoryRegion within a
container device, and do object_unparent on the container device.  Then
the following happens:

1) the container object's unparent method calls memory_region_del_subregion

2) when the old flatview is destroyed, it calls memory_region_unref but
the parent is still in place

3) the container object is destroyed

4) the container object destroys the container device.

Note that these rules apply only to actual memory regions (I/O or RAM),
not aliases or containers.  I hope to post a doc patch next week, since
I'm completing the patches that fix the only other problematic case
(PCIe MMCONFIG).  I was not expecting this to trigger so early, which is
why I had left this for part 3.

Paolo

Re: [Qemu-devel] [PATCH] memory: Fix double unref of flatview

[Matthew Rosato]

On 02/12/2015 03:43 PM, Paolo Bonzini wrote:
> 
> 
> On 12/02/2015 20:32, Matthew Rosato wrote:
>> Could it be that the order in which flatview_unref (and therefore
>> memory_region_unref) vs object_unparent(mr) matters (ie, object_unparent
>> should always happen last)?  Prior to RCUification, seems like the
>> old_view was always unreferenced prior to return from
>> memory_region_del_subregion (so, memory_region_unref always occurred
>> before the object_unparent(mr)).  Now, the two could happen in either
>> order -- and looking at memory_region_unref, it is specifically looking
>> at the existence of obj->parent.
> 
> I think you're right.

FYI, then this probably also affects the places you hit in d8d9581460
"memory: convert memory_region_destroy to object_unparent", as that's
what I modeled this approach on -- but I haven't tested any of them.

> 
> To test your theory, you could try using call_rcu to unparent the memory
> region.  The complete fix would be to wrap the MemoryRegion within a

Yep, this test worked.

> container device, and do object_unparent on the container device.  Then
> the following happens:
> 
> 1) the container object's unparent method calls memory_region_del_subregion
> 
> 2) when the old flatview is destroyed, it calls memory_region_unref but
> the parent is still in place
> 
> 3) the container object is destroyed
> 
> 4) the container object destroys the container device.

OK great!  I noticed this problem while re-structuring this area of code
anyway, so I'll roll this in.

> 
> Note that these rules apply only to actual memory regions (I/O or RAM),
> not aliases or containers.  I hope to post a doc patch next week, since
> I'm completing the patches that fix the only other problematic case
> (PCIe MMCONFIG).  I was not expecting this to trigger so early, which is
> why I had left this for part 3.

Looking forward to it -- Thanks for the help Paolo.

Matt

> 
> Paolo
> 

Re: [Qemu-devel] [PATCH] memory: Fix double unref of flatview

[Paolo Bonzini]

On 13/02/2015 04:29, Matthew Rosato wrote:
> FYI, then this probably also affects the places you hit in d8d9581460
> "memory: convert memory_region_destroy to object_unparent", as that's
> what I modeled this approach on -- but I haven't tested any of them.

Luckily not, because only "real" regions (not aliases and containers)
end up in a FlatView.  So you can do object_unparent on aliases and
containers.  It's ugly and should be avoided, but not buggy.

There's only three cases (VFIO, for which patches have been posted and
reviewed already, plus other two in PCI) that have to be modified.

Paolo