Re: [Qemu-devel] [RFC PATCH qemu] exec: Destroy dispatch immediately

[Alexey Kardashevskiy]

On 25/08/17 18:53, Paolo Bonzini wrote:
> On 25/08/2017 10:31, Alexey Kardashevskiy wrote:
>> Otherwise old dispatch holds way too much memory before RCU gets
>> a chance to free old dispatches.
>>
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>> ---
>>
>> This is a follow-up to the "Memory use with >100 virtio devices"
>> thread.
>>
>> I assume this is a dirty hack (which fixes the problem though)
> 
> This doesn't work.  AddressSpaceDispatch can be accessed outside the big
> QEMU lock, which is why it is destroyed via call_rcu.
> 
> The solution is to: 1) share the FlatView structures if they refer to
> the same root memory region; 2) have one AddressSpaceDispatch per
> FlatView instead of one per AddressSpace, so that FlatView reference
> counting takes care of clearing the AddressSpaceDispatch too.  Neither
> is particularly hard.  The second requires getting rid of the
> as->dispatch_listener and "hard coding" the creation of the
> AddressSpaceDispatch from the FlatView in memory.c.


While I am trying this approach, here is a cheap workaround -

diff --git a/vl.c b/vl.c
index 8e247cc2a2..4d95bc2a6a 100644
--- a/vl.c
+++ b/vl.c
@@ -4655,12 +4655,16 @@ int main(int argc, char **argv, char **envp)
     igd_gfx_passthru();

     /* init generic devices */
+    memory_region_transaction_begin();
+
     rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
     if (qemu_opts_foreach(qemu_find_opts("device"),
                           device_init_func, NULL, NULL)) {
         exit(1);
     }

+    memory_region_transaction_commit();
+
     cpu_synchronize_all_post_init();

     rom_reset_order_override();



Just for self-education - how dirty is this hack?

This effectively postpones all dispatch trees allocation/disposal till MR
transation depth is 0 (which happens in this
memory_region_transaction_commit()), for 500 virtio devices it reduces the
amount of resident RAM from 45GB to 11GB with 2GB guest, good for speed too
- time to build a machine is reduced from 4:00 to 1:20.




> 
> Paolo
> 
>> and I wonder what the proper solution would be. Thanks.
>>
>>
>> What happens here is that every virtio block device creates 2 address
>> spaces - for modern config space (called "virtio-pci-cfg-as") and
>> for busmaster (common pci thing, called after the device name,
>> in my case "virtio-blk-pci").
>>
>> Each address_space_init() updates topology for _every_ address space.
>> Every topology update (address_space_update_topology()) creates a new
>> dispatch tree - AddressSpaceDispatch with nodes (1KB) and
>> sections (48KB) and destroys the old one.
>>
>> However the dispatch destructor is postponed via RCU which does not
>> get a chance to execute until the machine is initialized but before
>> we get there, memory is not returned to the pool, and this is a lot
>> of memory which grows n^2.
>>
>>
>> Interestingly, mem_add() from exec.c is called twice:
>> as as->dispatch_listener.region_add() and
>> as as->dispatch_listener.region_nop() - I did not understand
>> the trick but it does not work if I remove the .region_nop() hook.
>> How does it work? :)
>>
>> ---
>>  exec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 01ac21e3cd..ea5f3eb209 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2707,7 +2707,7 @@ static void mem_commit(MemoryListener *listener)
>>  
>>      atomic_rcu_set(&as->dispatch, next);
>>      if (cur) {
>> -        call_rcu(cur, address_space_dispatch_free, rcu);
>> +        address_space_dispatch_free(cur);
>>      }
>>  }
>>  
>>
> 


-- 
Alexey