From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752553AbcGAHWr (ORCPT ); Fri, 1 Jul 2016 03:22:47 -0400 Received: from merlin.infradead.org ([205.233.59.134]:57270 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752160AbcGAHWq (ORCPT ); Fri, 1 Jul 2016 03:22:46 -0400 Date: Fri, 1 Jul 2016 09:22:38 +0200 From: Peter Zijlstra To: David Howells Cc: linux-afs@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] Introduce rb_replace_node_rcu() Message-ID: <20160701072238.GK30921@twins.programming.kicks-ass.net> References: <1412.1467356887@warthog.procyon.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1412.1467356887@warthog.procyon.org.uk> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jul 01, 2016 at 08:08:07AM +0100, David Howells wrote: > Should I also reorder rb_replace_node() whilst > I'm at it so that the new node is initialised first (it shouldn't make a > difference, I know)? Might as well, I can't imagine that making a performance difference and keeping the general structure of things similar helps avoid confusion. > commit 812667d2a82a6a8fe35a44e951e8b1515b04696a > Author: David Howells > Date: Fri Jul 1 07:53:51 2016 +0100 > > Introduce rb_replace_node_rcu() > > Implement an RCU-safe variant of rb_replace_node(). > > Signed-off-by: David Howells > cc: Peter Zijlstra One little niggle below, but: Acked-by: Peter Zijlstra (Intel) > diff --git a/lib/rbtree.c b/lib/rbtree.c > index 1356454e36de..59eb906c6c3b 100644 > --- a/lib/rbtree.c > +++ b/lib/rbtree.c > @@ -551,6 +551,25 @@ void rb_replace_node(struct rb_node *victim, struct rb_node *new, > } > EXPORT_SYMBOL(rb_replace_node); > > +void rb_replace_node_rcu(struct rb_node *victim, struct rb_node *new, > + struct rb_root *root) > +{ > + struct rb_node *parent = rb_parent(victim); > + > + /* Copy the pointers/colour from the victim to the replacement */ > + *new = *victim; > + > + /* Set the surrounding nodes to point to the replacement */ > + if (victim->rb_left) > + rb_set_parent(victim->rb_left, new); > + if (victim->rb_right) > + rb_set_parent(victim->rb_right, new); > + > + /* Set the onward pointer last with an RCU barrier */ Maybe also explain _why_ this needs to be last. Its obvious now and to us, but it might safe some head scratching later. > + __rb_change_child_rcu(victim, new, parent, root); > +} > +EXPORT_SYMBOL(rb_replace_node_rcu);