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

[Xu, Anthony]

> 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);