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

[Xu, Anthony]

> > memory_region_finalize.
> > Let me know if you think otherwise.
> 
> Yes, you can replace memory_region_del_subregion in
> memory_region_finalize
> with special code that does
> 
>     assert(!mr->enabled);
>     assert(subregion->container == mr);
>     subregion->container = NULL;
>     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
>     memory_region_unref(subregion);
> 
> The last four lines are shared with memory_region_del_subregion, so please
> factor them in a new function, for example
> memory_region_del_subregion_internal.


After adding synchronize_rcu, I saw an infinite recursive call,
  mem_commit-> memory_region_finalize-> mem_commit->
memory_region_finalize-> ......
it caused a segment fault, because 8M stack space is used up, and found when 
memory_region_finalize is called, memory_region_transaction_depth is 0 and 
memory_region_update_pending is true. That's not normal!

As you mentioned in your previous email, that should  not happen.
>" But if memory_region_transaction_depth is == 0, there should be no
>update pending because the loop has never run"

The root cause is,
MEMORY_LISTENER_CALL_GLOBAL(commit, Forward) is called before 
memory_region_clear_pending() in memory_region_transaction_commit.
so when MEMORY_LISTENER_CALL_GLOBAL(commit, Forward) is called,
memory_region_transaction_depth is 0 and memory_region_update_pending is true.
mem_commit may call memory_region_finalize, it causes infinite recursive call.

my previous fix for this is not complete.
We may clear memory_region_update_pending before calling 
MEMORY_LISTENER_CALL_GLOBAL(commit, Forward)

Please review below patch

diff --git a/memory.c b/memory.c
index 64b0a60..4c95aaf 100644
--- a/memory.c
+++ b/memory.c
@@ -906,12 +906,6 @@ void memory_region_transaction_begin(void)
     ++memory_region_transaction_depth;
 }

-static void memory_region_clear_pending(void)
-{
-    memory_region_update_pending = false;
-    ioeventfd_update_pending = false;
-}
-
 void memory_region_transaction_commit(void)
 {
     AddressSpace *as;
@@ -927,14 +921,14 @@ void memory_region_transaction_commit(void)
             QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
                 address_space_update_topology(as);
             }
-
+            memory_region_update_pending = false;
             MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
         } else if (ioeventfd_update_pending) {
             QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
                 address_space_update_ioeventfds(as);
             }
+            ioeventfd_update_pending = false;
         }
-        memory_region_clear_pending();
    }
 }




> > It is not slow, the contention is not high. Yes we can test.
> 
> It's not a matter of contention.  It's a matter of how long an RCU
> critical section can be---not just at startup, but at any point
> during QEMU execution.
The thing is, seems both address_space_translate and 
address_space_dispatch_free 
are called under the global lock. When synchronize_rcu is called, no other 
threads 
are in RCU critical section.

Seems RCU is not that useful for address space.


> 
> Have you tried tcmalloc or jemalloc?  They use the brk region
> less and sometimes are more aggressive in releasing mmap-ed memory
> areas.

They may be aggressive. But if memory are freed afterward, it may not help in 
some cases,
for example, starting a lot of VMs at the same time.


> 
> > Please review below patch, MemoryRegion is protected by RCU.
> 
> Removing transaction begin/commit in memory_region_finalize makes
> little sense because memory_region_del_subregion calls those two
> functions again.  See above for an alternative.  Apart from this,
> the patch is at least correct, though I'm not sure it's a good
> idea (synchronize_rcu needs testing). 
> I'm not sure I understand the
> address_space_write change).

After adding synchronize_rcu, we noticed a RCU dead loop.  synchronize_rcu is 
called 
inside RCU critical section. It happened when guest OS programmed the PCI bar.
The call trace is like,
address_space_write-> pci_host_config_write_common -> 
memory_region_transaction_commit ->mem_commit-> synchronize_rcu
pci_host_config_write_common is called inside RCU critical section.

The address_space_write change fixed this issue.


Thanks,
Anthony