From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 04/15] memory: MemoryRegion topology must be stable when updating Date: Mon, 13 Aug 2012 15:28:26 -0300 Message-ID: <20120813182826.GC25268@amt.cnet> References: <1344407156-25562-1-git-send-email-qemulist@gmail.com> <1344407156-25562-5-git-send-email-qemulist@gmail.com> <50222DB6.8020505@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, Jan Kiszka , qemu-devel@nongnu.org, Blue Swirl , Avi Kivity , Anthony Liguori , Stefan Hajnoczi , Paolo Bonzini , Andreas =?iso-8859-1?Q?F=E4rber?= To: liu ping fan Return-path: Content-Disposition: inline In-Reply-To: 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 Thu, Aug 09, 2012 at 03:28:44PM +0800, liu ping fan wrote: > On Wed, Aug 8, 2012 at 5:13 PM, Avi Kivity wrote: > > 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. > > > Yes, what I want to do is "prepare unplug out of protection of global > lock". When io-dispatch and mmio-dispatch are all out of big lock, we > will run into the following scene: > In vcpu context A, qdev_unplug_complete()-> delete subregion; > In context B, write pci bar --> pci mapping update -> add subregion Per device lock should protect that. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T0zox-0003XG-Gw for qemu-devel@nongnu.org; Mon, 13 Aug 2012 14:57:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T0zov-0004rK-Fj for qemu-devel@nongnu.org; Mon, 13 Aug 2012 14:56:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12136) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T0zov-0004rB-7E for qemu-devel@nongnu.org; Mon, 13 Aug 2012 14:56:57 -0400 Date: Mon, 13 Aug 2012 15:28:26 -0300 From: Marcelo Tosatti Message-ID: <20120813182826.GC25268@amt.cnet> References: <1344407156-25562-1-git-send-email-qemulist@gmail.com> <1344407156-25562-5-git-send-email-qemulist@gmail.com> <50222DB6.8020505@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 , qemu-devel@nongnu.org, Blue Swirl , Avi Kivity , Anthony Liguori , Stefan Hajnoczi , Paolo Bonzini , Andreas =?iso-8859-1?Q?F=E4rber?= On Thu, Aug 09, 2012 at 03:28:44PM +0800, liu ping fan wrote: > On Wed, Aug 8, 2012 at 5:13 PM, Avi Kivity wrote: > > 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. > > > Yes, what I want to do is "prepare unplug out of protection of global > lock". When io-dispatch and mmio-dispatch are all out of big lock, we > will run into the following scene: > In vcpu context A, qdev_unplug_complete()-> delete subregion; > In context B, write pci bar --> pci mapping update -> add subregion Per device lock should protect that.