From mboxrd@z Thu Jan 1 00:00:00 1970 From: liu ping fan Subject: Re: [PATCH 09/15] memory: prepare flatview and radix-tree for rcu style access Date: Sat, 11 Aug 2012 09:58:44 +0800 Message-ID: References: <1344407156-25562-1-git-send-email-qemulist@gmail.com> <1344407156-25562-10-git-send-email-qemulist@gmail.com> <5022342F.2070609@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 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: Avi Kivity Return-path: In-Reply-To: <5022342F.2070609@redhat.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 Wed, Aug 8, 2012 at 5:41 PM, Avi Kivity wrote: > On 08/08/2012 09:25 AM, Liu Ping Fan wrote: >> From: Liu Ping Fan >> >> Flatview and radix view are all under the protection of pointer. >> And this make sure the change of them seem to be atomic! >> >> The mr accessed by radix-tree leaf or flatview will be reclaimed >> after the prev PhysMap not in use any longer >> > > IMO this cleverness should come much later. Let's first take care of > dropping the big qemu lock, then make swithcing memory maps more efficient. > > The initial paths could look like: > > lookup: > take mem_map_lock > lookup > take ref > drop mem_map_lock > > update: > take mem_map_lock (in core_begin) > do updates > drop memo_map_lock > > Later we can replace mem_map_lock with either a rwlock or (real) rcu. > > >> >> #if !defined(CONFIG_USER_ONLY) >> >> -static void phys_map_node_reserve(unsigned nodes) >> +static void phys_map_node_reserve(PhysMap *map, unsigned nodes) >> { >> - if (phys_map_nodes_nb + nodes > phys_map_nodes_nb_alloc) { >> + if (map->phys_map_nodes_nb + nodes > map->phys_map_nodes_nb_alloc) { >> typedef PhysPageEntry Node[L2_SIZE]; >> - phys_map_nodes_nb_alloc = MAX(phys_map_nodes_nb_alloc * 2, 16); >> - phys_map_nodes_nb_alloc = MAX(phys_map_nodes_nb_alloc, >> - phys_map_nodes_nb + nodes); >> - phys_map_nodes = g_renew(Node, phys_map_nodes, >> - phys_map_nodes_nb_alloc); >> + map->phys_map_nodes_nb_alloc = MAX(map->phys_map_nodes_nb_alloc * 2, >> + 16); >> + map->phys_map_nodes_nb_alloc = MAX(map->phys_map_nodes_nb_alloc, >> + map->phys_map_nodes_nb + nodes); >> + map->phys_map_nodes = g_renew(Node, map->phys_map_nodes, >> + map->phys_map_nodes_nb_alloc); >> } >> } > > Please have a patch that just adds the map parameter to all these > functions. This makes the later patch, that adds the copy, easier to read. > >> + >> +void cur_map_update(PhysMap *next) >> +{ >> + qemu_mutex_lock(&cur_map_lock); >> + physmap_put(cur_map); >> + cur_map = next; >> + smp_mb(); >> + qemu_mutex_unlock(&cur_map_lock); >> +} > > IMO this can be mem_map_lock. > > If we take my previous suggestion: > > lookup: > take mem_map_lock > lookup > take ref > drop mem_map_lock > > update: > take mem_map_lock (in core_begin) > do updates > drop memo_map_lock > > And update it to > > > update: > prepare next_map (in core_begin) > do updates > take mem_map_lock (in core_commit) > switch maps > drop mem_map_lock > free old map > > > Note the lookup path copies the MemoryRegionSection instead of > referencing it. Thus we can destroy the old map without worrying; the > only pointers will point to MemoryRegions, which will be protected by > the refcounts on their Objects. > Just find there may be a leak here. If mrs points to subpage, then the subpage_t could be crashed by destroy. To avoid such situation, we can walk down the chain to pin us on the Object based mr, but then we must expose the address convert in subpage_read() right here. Right? Regards, pingfan > This can be easily switched to rcu: > > update: > prepare next_map (in core_begin) > do updates > switch maps - rcu_assign_pointer > call_rcu(free old map) (or synchronize_rcu; free old maps) > > Again, this should be done after the simplictic patch that enables > parallel lookup but keeps just one map. > > > > -- > 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]:54278) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T00yp-000398-E0 for qemu-devel@nongnu.org; Fri, 10 Aug 2012 21:59:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T00yo-00020a-9D for qemu-devel@nongnu.org; Fri, 10 Aug 2012 21:59:07 -0400 Received: from mail-wi0-f175.google.com ([209.85.212.175]:44230) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T00yo-00020E-1e for qemu-devel@nongnu.org; Fri, 10 Aug 2012 21:59:06 -0400 Received: by wibhm2 with SMTP id hm2so1384782wib.10 for ; Fri, 10 Aug 2012 18:59:04 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <5022342F.2070609@redhat.com> References: <1344407156-25562-1-git-send-email-qemulist@gmail.com> <1344407156-25562-10-git-send-email-qemulist@gmail.com> <5022342F.2070609@redhat.com> From: liu ping fan Date: Sat, 11 Aug 2012 09:58:44 +0800 Message-ID: Content-Type: text/plain; charset=ISO-8859-1 Subject: Re: [Qemu-devel] [PATCH 09/15] memory: prepare flatview and radix-tree for rcu style access List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity 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 Wed, Aug 8, 2012 at 5:41 PM, Avi Kivity wrote: > On 08/08/2012 09:25 AM, Liu Ping Fan wrote: >> From: Liu Ping Fan >> >> Flatview and radix view are all under the protection of pointer. >> And this make sure the change of them seem to be atomic! >> >> The mr accessed by radix-tree leaf or flatview will be reclaimed >> after the prev PhysMap not in use any longer >> > > IMO this cleverness should come much later. Let's first take care of > dropping the big qemu lock, then make swithcing memory maps more efficient. > > The initial paths could look like: > > lookup: > take mem_map_lock > lookup > take ref > drop mem_map_lock > > update: > take mem_map_lock (in core_begin) > do updates > drop memo_map_lock > > Later we can replace mem_map_lock with either a rwlock or (real) rcu. > > >> >> #if !defined(CONFIG_USER_ONLY) >> >> -static void phys_map_node_reserve(unsigned nodes) >> +static void phys_map_node_reserve(PhysMap *map, unsigned nodes) >> { >> - if (phys_map_nodes_nb + nodes > phys_map_nodes_nb_alloc) { >> + if (map->phys_map_nodes_nb + nodes > map->phys_map_nodes_nb_alloc) { >> typedef PhysPageEntry Node[L2_SIZE]; >> - phys_map_nodes_nb_alloc = MAX(phys_map_nodes_nb_alloc * 2, 16); >> - phys_map_nodes_nb_alloc = MAX(phys_map_nodes_nb_alloc, >> - phys_map_nodes_nb + nodes); >> - phys_map_nodes = g_renew(Node, phys_map_nodes, >> - phys_map_nodes_nb_alloc); >> + map->phys_map_nodes_nb_alloc = MAX(map->phys_map_nodes_nb_alloc * 2, >> + 16); >> + map->phys_map_nodes_nb_alloc = MAX(map->phys_map_nodes_nb_alloc, >> + map->phys_map_nodes_nb + nodes); >> + map->phys_map_nodes = g_renew(Node, map->phys_map_nodes, >> + map->phys_map_nodes_nb_alloc); >> } >> } > > Please have a patch that just adds the map parameter to all these > functions. This makes the later patch, that adds the copy, easier to read. > >> + >> +void cur_map_update(PhysMap *next) >> +{ >> + qemu_mutex_lock(&cur_map_lock); >> + physmap_put(cur_map); >> + cur_map = next; >> + smp_mb(); >> + qemu_mutex_unlock(&cur_map_lock); >> +} > > IMO this can be mem_map_lock. > > If we take my previous suggestion: > > lookup: > take mem_map_lock > lookup > take ref > drop mem_map_lock > > update: > take mem_map_lock (in core_begin) > do updates > drop memo_map_lock > > And update it to > > > update: > prepare next_map (in core_begin) > do updates > take mem_map_lock (in core_commit) > switch maps > drop mem_map_lock > free old map > > > Note the lookup path copies the MemoryRegionSection instead of > referencing it. Thus we can destroy the old map without worrying; the > only pointers will point to MemoryRegions, which will be protected by > the refcounts on their Objects. > Just find there may be a leak here. If mrs points to subpage, then the subpage_t could be crashed by destroy. To avoid such situation, we can walk down the chain to pin us on the Object based mr, but then we must expose the address convert in subpage_read() right here. Right? Regards, pingfan > This can be easily switched to rcu: > > update: > prepare next_map (in core_begin) > do updates > switch maps - rcu_assign_pointer > call_rcu(free old map) (or synchronize_rcu; free old maps) > > Again, this should be done after the simplictic patch that enables > parallel lookup but keeps just one map. > > > > -- > error compiling committee.c: too many arguments to function