> 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.
Got it, thanks for explanation.
> > 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.
If it is from current thread, do we need to check if the global lock is
acquired?
As you mentioned flatview_unref may call memory_region_unref.
> - 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)
Okay, It may have sub regions,
In the comments of memory_region_finalize
" /* We know the region is not visible in any address space (it
* does not have a container and cannot be a root either because
* it has no references,
"
We know the memory region is not a root region, and the memory region has
already been removed from address space even it has sub regions.
memory_region_transaction_commit should be called when the
memory region is removed from address space.
memory_region_transaction_commit seems not be needed in memory_region_finalize.
Let me know if you think otherwise.
> > How about fall back to synchronize_rcu?
>
> I'm afraid it would be too slow, but you can test.
It is not slow, the contention is not high. Yes we can test.
> Nope. The RCU read lock protects all MemoryRegions through
> flatview_unref: RCU keeps the FlatView alive, and the FlatView keeps
> MemoryRegions alive.
I see, RCU needs to protect MemoryRegions as well.
Please review below patch, MemoryRegion is protected by RCU.
Thanks,
Anthony
diff --git a/exec.c b/exec.c
index a22f5a0..6631668 100644
--- a/exec.c
+++ b/exec.c
@@ -2521,7 +2521,8 @@ static void mem_commit(MemoryListener *listener)
atomic_rcu_set(&as->dispatch, next);
if (cur) {
- call_rcu(cur, address_space_dispatch_free, rcu);
+ synchronize_rcu();
+ address_space_dispatch_free(cur);
}
}
@@ -2567,7 +2568,8 @@ void address_space_destroy_dispatch(AddressSpace *as)
atomic_rcu_set(&as->dispatch, NULL);
if (d) {
- call_rcu(d, address_space_dispatch_free, rcu);
+ synchronize_rcu();
+ address_space_dispatch_free(d);
}
}
@@ -2712,19 +2714,22 @@ static bool prepare_mmio_access(MemoryRegion *mr)
return release_lock;
}
-/* Called within RCU critical section. */
-static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
- MemTxAttrs attrs,
- const uint8_t *buf,
- int len, hwaddr addr1,
- hwaddr l, MemoryRegion *mr)
+MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
+ MemTxAttrs attrs, const uint8_t *buf, int len)
{
uint8_t *ptr;
uint64_t val;
+ hwaddr l=len;
+ hwaddr addr1;
+ MemoryRegion *mr;
MemTxResult result = MEMTX_OK;
bool release_lock = false;
for (;;) {
+ rcu_read_lock();
+ mr = address_space_translate(as, addr, &addr1, &l, true);
+ memory_region_ref(mr);
+ rcu_read_unlock();
if (!memory_access_is_direct(mr, true)) {
release_lock |= prepare_mmio_access(mr);
l = memory_access_size(mr, l, addr1);
@@ -2764,7 +2769,7 @@ static MemTxResult
address_space_write_continue(AddressSpace *as, hwaddr addr,
memcpy(ptr, buf, l);
invalidate_and_set_dirty(mr, addr1, l);
}
-
+ memory_region_unref(mr);
if (release_lock) {
qemu_mutex_unlock_iothread();
release_lock = false;
@@ -2779,27 +2784,6 @@ static MemTxResult
address_space_write_continue(AddressSpace *as, hwaddr addr,
}
l = len;
- mr = address_space_translate(as, addr, &addr1, &l, true);
- }
-
- return result;
-}
-
-MemTxResult address_space_write(AddressSpace *as, hwaddr addr, MemTxAttrs
attrs,
- const uint8_t *buf, int len)
-{
- hwaddr l;
- hwaddr addr1;
- MemoryRegion *mr;
- MemTxResult result = MEMTX_OK;
-
- if (len > 0) {
- rcu_read_lock();
- l = len;
- mr = address_space_translate(as, addr, &addr1, &l, true);
- result = address_space_write_continue(as, addr, attrs, buf, len,
- addr1, l, mr);
- rcu_read_unlock();
}
return result;
diff --git a/memory.c b/memory.c
index 64b0a60..d12437c 100644
--- a/memory.c
+++ b/memory.c
@@ -1505,13 +1505,10 @@ 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();
-
mr->destructor(mr);
memory_region_clear_coalescing(mr);
g_free((char *)mr->name);
