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