From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 09/15] memory: prepare flatview and radix-tree for rcu style access Date: Wed, 08 Aug 2012 12:41:03 +0300 Message-ID: <5022342F.2070609@redhat.com> References: <1344407156-25562-1-git-send-email-qemulist@gmail.com> <1344407156-25562-10-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-10-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 > > 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. 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]:55152) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz2lL-0003PJ-1E for qemu-devel@nongnu.org; Wed, 08 Aug 2012 05:41:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sz2lJ-0001hB-Vu for qemu-devel@nongnu.org; Wed, 08 Aug 2012 05:41:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:63731) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz2lJ-0001gi-Nv for qemu-devel@nongnu.org; Wed, 08 Aug 2012 05:41:09 -0400 Message-ID: <5022342F.2070609@redhat.com> Date: Wed, 08 Aug 2012 12:41:03 +0300 From: Avi Kivity MIME-Version: 1.0 References: <1344407156-25562-1-git-send-email-qemulist@gmail.com> <1344407156-25562-10-git-send-email-qemulist@gmail.com> In-Reply-To: <1344407156-25562-10-git-send-email-qemulist@gmail.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: 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 > > 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. 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