Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB

[Paolo Bonzini]

On 14/03/2017 22:23, Xu, Anthony wrote:
>> flatview_unref can call object_unref and thus reach:
> 
> Okay,  flatview_unref is the one you worried about,
> 
> Flatview_unref is registered as a RCU callback only in 
> address_space_update_topology,
> Strangely, it is registered as a RCU callback, and is called directly in this 
> function. 
> Basially flatview_unref is called twice for old view.

The first unref is done after as->current_map is overwritten.
as->current_map is accessed under RCU, so it needs call_rcu.  It
balances the initial reference that is present since flatview_init.

The second unref is done to balance the flatview_ref in
address_space_get_flatview.

> There are some comments, 
> but it is not clear to me, is this a bug or by design? Is flatview_destroy 
> called in current thread 
> or RCU thread?

You cannot know, because there are also other callers of
address_space_get_flatview.  Usually it's from the RCU thread.

> Let me split the patch, Do you think below patch is correct?
> 
> --- a/memory.c
> +++ b/memory.c
> @@ -1503,15 +1503,9 @@ static void memory_region_finalize(Object *obj)
>       * and cause an infinite loop.
>       */
>      mr->enabled = false;
> -    memory_region_transaction_begin();
> -    while (!QTAILQ_EMPTY(&mr->subregions)) {
> -        MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions);
> -        memory_region_del_subregion(mr, subregion);
> -    }
> -    memory_region_transaction_commit();
> -
> +    assert(QTAILQ_EMPTY(&mr->subregions));
>      mr->destructor(mr);
> -    memory_region_clear_coalescing(m
> +    assert(QTAILQ_EMPTY(&mr->coalesced));
>      g_free((char *)mr->name);
>      g_free(mr->ioeventfds);
>  }

No.  Callers can:

- let memory_region_finalize remove subregions (commit 2e2b8eb, "memory:
allow destroying a non-empty MemoryRegion", 2015-10-09)

- let memory_region_finalize disable coalesced I/O (in fact there are no
callers of memory_region_clear_coalescing outside memory.c)

> Then let us take a look at flatview_unref, memory_region_unref may call any 
> QOM finalize through 
> Object_unref(mr->owner), that's your concern, I got it. It is way too 
> complicated to look at each QOM 
> object release callbacks and each QOM property release callbacks.  I gave up 
> this path.
> How about fall back to synchronize_rcu?

I'm afraid it would be too slow, but you can test.

Something like the kernel's synchronize_rcu_expedited would work, but we
don't have anything like that in QEMU yet.

> As for address space, the RCU read lock is used to protect PhysPageMap, but 
> not the regular MemoryRegions,

Nope.  The RCU read lock protects all MemoryRegions through
flatview_unref: RCU keeps the FlatView alive, and the FlatView keeps
MemoryRegions alive.

Hence...

> The MemoryRegions returned from address_space_translate are regular 
> MemoryRegions, so 
> address_space_write_continue and address_space_read_continue don't need RCU 
> read lock.

... this is wrong too.

Paolo