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