From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 04/15] memory: MemoryRegion topology must be stable when updating Date: Wed, 08 Aug 2012 12:13:26 +0300 Message-ID: <50222DB6.8020505@redhat.com> References: <1344407156-25562-1-git-send-email-qemulist@gmail.com> <1344407156-25562-5-git-send-email-qemulist@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Jan Kiszka , Marcelo Tosatti , qemu-devel@nongnu.org, Blue Swirl , Anthony Liguori , Stefan Hajnoczi , Paolo Bonzini , =?ISO-8859-1?Q?Andreas_F=E4rber?= To: Liu Ping Fan Return-path: In-Reply-To: <1344407156-25562-5-git-send-email-qemulist@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org List-Id: kvm.vger.kernel.org On 08/08/2012 09:25 AM, Liu Ping Fan wrote: > From: Liu Ping Fan > > Using mem_map_lock to protect among updaters. So we can get the intact > snapshot of mem topology -- FlatView & radix-tree. > > Signed-off-by: Liu Ping Fan > --- > exec.c | 3 +++ > memory.c | 22 ++++++++++++++++++++++ > memory.h | 2 ++ > 3 files changed, 27 insertions(+), 0 deletions(-) > > diff --git a/exec.c b/exec.c > index 8244d54..0e29ef9 100644 > --- a/exec.c > +++ b/exec.c > @@ -210,6 +210,8 @@ static unsigned phys_map_nodes_nb, phys_map_nodes_nb_alloc; > The bottom level has pointers to MemoryRegionSections. */ > static PhysPageEntry phys_map = { .ptr = PHYS_MAP_NODE_NIL, .is_leaf = 0 }; > > +QemuMutex mem_map_lock; > + > static void io_mem_init(void); > static void memory_map_init(void); > > @@ -637,6 +639,7 @@ void cpu_exec_init_all(void) > #if !defined(CONFIG_USER_ONLY) > memory_map_init(); > io_mem_init(); > + qemu_mutex_init(&mem_map_lock); > #endif > } > > diff --git a/memory.c b/memory.c > index aab4a31..5986532 100644 > --- a/memory.c > +++ b/memory.c > @@ -761,7 +761,9 @@ void memory_region_transaction_commit(void) > assert(memory_region_transaction_depth); > --memory_region_transaction_depth; > if (!memory_region_transaction_depth && memory_region_update_pending) { > + qemu_mutex_lock(&mem_map_lock); > memory_region_update_topology(NULL); > + qemu_mutex_unlock(&mem_map_lock); > } > } Seems to me that nothing in memory.c can susceptible to races. It must already be called under the big qemu lock, and with the exception of mutators (memory_region_set_*), changes aren't directly visible. I think it's sufficient to take the mem_map_lock at the beginning of core_begin() and drop it at the end of core_commit(). That means all updates of volatile state, phys_map, are protected. -- error compiling committee.c: too many arguments to function From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50162) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz2Ke-00084k-Ow for qemu-devel@nongnu.org; Wed, 08 Aug 2012 05:13:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sz2Kb-0001c9-2u for qemu-devel@nongnu.org; Wed, 08 Aug 2012 05:13:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8058) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz2Ka-0001c5-Q2 for qemu-devel@nongnu.org; Wed, 08 Aug 2012 05:13:33 -0400 Message-ID: <50222DB6.8020505@redhat.com> Date: Wed, 08 Aug 2012 12:13:26 +0300 From: Avi Kivity MIME-Version: 1.0 References: <1344407156-25562-1-git-send-email-qemulist@gmail.com> <1344407156-25562-5-git-send-email-qemulist@gmail.com> In-Reply-To: <1344407156-25562-5-git-send-email-qemulist@gmail.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 04/15] memory: MemoryRegion topology must be stable when updating List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Liu Ping Fan Cc: kvm@vger.kernel.org, Jan Kiszka , Marcelo Tosatti , qemu-devel@nongnu.org, Blue Swirl , Anthony Liguori , Stefan Hajnoczi , Paolo Bonzini , =?ISO-8859-1?Q?Andreas_F=E4rber?= On 08/08/2012 09:25 AM, Liu Ping Fan wrote: > From: Liu Ping Fan > > Using mem_map_lock to protect among updaters. So we can get the intact > snapshot of mem topology -- FlatView & radix-tree. > > Signed-off-by: Liu Ping Fan > --- > exec.c | 3 +++ > memory.c | 22 ++++++++++++++++++++++ > memory.h | 2 ++ > 3 files changed, 27 insertions(+), 0 deletions(-) > > diff --git a/exec.c b/exec.c > index 8244d54..0e29ef9 100644 > --- a/exec.c > +++ b/exec.c > @@ -210,6 +210,8 @@ static unsigned phys_map_nodes_nb, phys_map_nodes_nb_alloc; > The bottom level has pointers to MemoryRegionSections. */ > static PhysPageEntry phys_map = { .ptr = PHYS_MAP_NODE_NIL, .is_leaf = 0 }; > > +QemuMutex mem_map_lock; > + > static void io_mem_init(void); > static void memory_map_init(void); > > @@ -637,6 +639,7 @@ void cpu_exec_init_all(void) > #if !defined(CONFIG_USER_ONLY) > memory_map_init(); > io_mem_init(); > + qemu_mutex_init(&mem_map_lock); > #endif > } > > diff --git a/memory.c b/memory.c > index aab4a31..5986532 100644 > --- a/memory.c > +++ b/memory.c > @@ -761,7 +761,9 @@ void memory_region_transaction_commit(void) > assert(memory_region_transaction_depth); > --memory_region_transaction_depth; > if (!memory_region_transaction_depth && memory_region_update_pending) { > + qemu_mutex_lock(&mem_map_lock); > memory_region_update_topology(NULL); > + qemu_mutex_unlock(&mem_map_lock); > } > } Seems to me that nothing in memory.c can susceptible to races. It must already be called under the big qemu lock, and with the exception of mutators (memory_region_set_*), changes aren't directly visible. I think it's sufficient to take the mem_map_lock at the beginning of core_begin() and drop it at the end of core_commit(). That means all updates of volatile state, phys_map, are protected. -- error compiling committee.c: too many arguments to function