[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB
From: |
Xu, Anthony |
Subject: |
Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB |
Date: |
Thu, 16 Mar 2017 20:02:28 +0000 |
> > 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
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, (continued)
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Paolo Bonzini, 2017/03/10
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Xu, Anthony, 2017/03/10
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Paolo Bonzini, 2017/03/11
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Xu, Anthony, 2017/03/14
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Paolo Bonzini, 2017/03/14
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Xu, Anthony, 2017/03/14
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Paolo Bonzini, 2017/03/15
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Xu, Anthony, 2017/03/15
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Paolo Bonzini, 2017/03/15
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Zhong, Yang, 2017/03/15
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB,
Xu, Anthony <=
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Paolo Bonzini, 2017/03/22
- Re: [Qemu-devel] [PATCH v1 2/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Xu, Anthony, 2017/03/22
Re: [Qemu-devel] [PATCH v1 1/2] reduce qemu's heap Rss size from 12252kB to 2752KB, Paolo Bonzini, 2017/03/10