From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40793) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPcmA-00011u-4w for qemu-devel@nongnu.org; Wed, 12 Aug 2015 16:37:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZPcm5-0005JJ-Nw for qemu-devel@nongnu.org; Wed, 12 Aug 2015 16:37:29 -0400 Received: from out3-smtp.messagingengine.com ([66.111.4.27]:54749) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPcm5-0005HR-IZ for qemu-devel@nongnu.org; Wed, 12 Aug 2015 16:37:25 -0400 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id E977A22434 for ; Wed, 12 Aug 2015 16:37:23 -0400 (EDT) Date: Wed, 12 Aug 2015 16:37:34 -0400 From: "Emilio G. Cota" Message-ID: <20150812203734.GA17706@flamenco> References: <1439397664-70734-1-git-send-email-pbonzini@redhat.com> <1439397664-70734-9-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1439397664-70734-9-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH 08/10] tcg: add memory barriers in page_find_alloc accesses List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: mttcg@greensocs.com, qemu-devel@nongnu.org, fred.konrad@greensocs.com On Wed, Aug 12, 2015 at 18:41:00 +0200, Paolo Bonzini wrote: > page_find is reading the radix tree outside all locks, so it has to > use the RCU primitives. It does not need RCU critical sections > because the PageDescs are never removed, so there is never a need > to wait for the end of code sections that use a PageDesc. Note that rcu_find_alloc might end up writing to the tree, see below. BTW the fact that there are no removals makes the use of RCU unnecessary. > Signed-off-by: Paolo Bonzini > --- > translate-all.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/translate-all.c b/translate-all.c > index 7727091..78a787d 100644 > --- a/translate-all.c > +++ b/translate-all.c > @@ -437,14 +437,14 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc) > > /* Level 2..N-1. */ > for (i = V_L1_SHIFT / V_L2_BITS - 1; i > 0; i--) { > - void **p = *lp; > + void **p = atomic_rcu_read(lp); > > if (p == NULL) { > if (!alloc) { > return NULL; > } > p = g_new0(void *, V_L2_SIZE); > - *lp = p; > + atomic_rcu_set(lp, p); > } > > lp = p + ((index >> (i * V_L2_BITS)) & (V_L2_SIZE - 1)); > @@ -456,7 +456,7 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc) > return NULL; > } > pd = g_new0(PageDesc, V_L2_SIZE); > - *lp = pd; > + atomic_rcu_set(lp, pd); rcu_set is not enough; a cmpxchg with a fail path would be needed here, since if the find_alloc is called without any locks held (as described in the commit message) several threads could concurrently write to the same node, corrupting the tree. I argue however that it is better to call page_find/_alloc with a mutex held, since otherwise we'd have to add per-PageDesc locks (it's very common to call page_find and then update the PageDesc). I have an RFC patchset for multithreaded tcg that deals with this, will submit once I bring it up to date with upstream. Emilio