Introduce rb_replace_node_rcu()

PATCH In review David Howells 2 participants 4 messages Jul 1, 2016 Library code

lore

Share & tools

lore

David Howells

author

2016-07-01 07:16 UTC #0 lore raw

PATCH 1/1 Introduce rb_replace_node_rcu()

Hi Peter,

How about the attached patch? 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)?

David
---
commit 812667d2a82a6a8fe35a44e951e8b1515b04696a

Author: David Howells <[email protected]>
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 <[email protected]>

cc Peter Zijlstra <[email protected]>

include/linux/rbtree.h +2 -0

index b6900099ea81..e585018498d5 100644
@@ -76,6 +76,8 @@ extern struct rb_node *rb_next_postorder(const struct rb_node *);
 /* Fast replacement of a single node without remove/rebalance/add/rebalance */
 extern void rb_replace_node(struct rb_node *victim, struct rb_node *new,
 			    struct rb_root *root);
+extern void rb_replace_node_rcu(struct rb_node *victim, struct rb_node *new,
+				struct rb_root *root);
 
 static inline void rb_link_node(struct rb_node *node, struct rb_node *parent,
 				struct rb_node **rb_link)

include/linux/rbtree_augmented.h +13 -0

index 14d7b831b63a..d076183e49be 100644
@@ -130,6 +130,19 @@ __rb_change_child(struct rb_node *old, struct rb_node *new,
 		WRITE_ONCE(root->rb_node, new);
 }
 
+static inline void
+__rb_change_child_rcu(struct rb_node *old, struct rb_node *new,
+		      struct rb_node *parent, struct rb_root *root)
+{
+	if (parent) {
+		if (parent->rb_left == old)
+			rcu_assign_pointer(parent->rb_left, new);
+		else
+			rcu_assign_pointer(parent->rb_right, new);
+	} else
+		rcu_assign_pointer(root->rb_node, new);
+}
+
 extern void __rb_erase_color(struct rb_node *parent, struct rb_root *root,
 	void (*augment_rotate)(struct rb_node *old, struct rb_node *new));

lib/rbtree.c +19 -0

index 1356454e36de..59eb906c6c3b 100644
@@ -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 */
+	__rb_change_child_rcu(victim, new, parent, root);
+}
+EXPORT_SYMBOL(rb_replace_node_rcu);
+
 static struct rb_node *rb_left_deepest_node(const struct rb_node *node)
 {
 	for (;;) {

Peter Zijlstra

Acked-by on v1 1/1

2016-07-01 07:22 UTC #1 lore raw

Quoted 3 lines · David Howells

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.

Quoted 10 lines

commit 812667d2a82a6a8fe35a44e951e8b1515b04696a
Author: David Howells <[email protected]>
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 <[email protected]>
cc Peter Zijlstra <[email protected]>

One little niggle below, but:

Acked-by Peter Zijlstra (Intel) <[email protected]>

Quoted 13 lines

lib/rbtree.c +5 -0

index 1356454e36de..59eb906c6c3b 100644
@@ -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);
+

lib/rbtree.c void rb_replace_node(struct rb_node *victim, struct rb_node *new, · line 568

Quoted 10 lines

lib/rbtree.c +10 -0

+	/* 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.

Quoted 3 lines

lib/rbtree.c +3 -0

+	__rb_change_child_rcu(victim, new, parent, root);
+}
+EXPORT_SYMBOL(rb_replace_node_rcu);

David Howells

author Acked-by Peter Zijlstra (Intel)

2016-07-01 08:08 UTC #2 lore raw

Quoted 1 line · Peter Zijlstra

One little niggle below, but:

How about the attached? I've updated that comment and rearranged rb_replace_node_rcu().

David
---
commit 8bd38ef5a38728390348cc5b4a8797be16af493f

Author: David Howells <[email protected]>
Date:   Fri Jul 1 07:53:51 2016 +0100

Introduce rb_replace_node_rcu()

Implement an RCU-safe variant of rb_replace_node() and rearrange rb_replace_node() to do things in the same order.

Signed-off-by David Howells <[email protected]>

Acked-by Peter Zijlstra (Intel) <[email protected]>

include/linux/rbtree.h +2 -0

index b6900099ea81..e585018498d5 100644
@@ -76,6 +76,8 @@ extern struct rb_node *rb_next_postorder(const struct rb_node *);
 /* Fast replacement of a single node without remove/rebalance/add/rebalance */
 extern void rb_replace_node(struct rb_node *victim, struct rb_node *new,
 			    struct rb_root *root);
+extern void rb_replace_node_rcu(struct rb_node *victim, struct rb_node *new,
+				struct rb_root *root);
 
 static inline void rb_link_node(struct rb_node *node, struct rb_node *parent,
 				struct rb_node **rb_link)

include/linux/rbtree_augmented.h +13 -0

index 14d7b831b63a..d076183e49be 100644
@@ -130,6 +130,19 @@ __rb_change_child(struct rb_node *old, struct rb_node *new,
 		WRITE_ONCE(root->rb_node, new);
 }
 
+static inline void
+__rb_change_child_rcu(struct rb_node *old, struct rb_node *new,
+		      struct rb_node *parent, struct rb_root *root)
+{
+	if (parent) {
+		if (parent->rb_left == old)
+			rcu_assign_pointer(parent->rb_left, new);
+		else
+			rcu_assign_pointer(parent->rb_right, new);
+	} else
+		rcu_assign_pointer(root->rb_node, new);
+}
+
 extern void __rb_erase_color(struct rb_node *parent, struct rb_root *root,
 	void (*augment_rotate)(struct rb_node *old, struct rb_node *new));

lib/rbtree.c +24 -2

index 1356454e36de..eb8a19fee110 100644
@@ -539,17 +539,39 @@ void rb_replace_node(struct rb_node *victim, struct rb_node *new,
 {
 	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 */
-	__rb_change_child(victim, new, parent, root);
 	if (victim->rb_left)
 		rb_set_parent(victim->rb_left, new);
 	if (victim->rb_right)
 		rb_set_parent(victim->rb_right, new);
+	__rb_change_child(victim, new, parent, root);
+}
+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 parent's pointer to the new node last after an RCU barrier
+	 * so that the pointers onwards are seen to be set correctly when doing
+	 * an RCU walk over the tree.
+	 */
+	__rb_change_child_rcu(victim, new, parent, root);
 }
-EXPORT_SYMBOL(rb_replace_node);
+EXPORT_SYMBOL(rb_replace_node_rcu);
 
 static struct rb_node *rb_left_deepest_node(const struct rb_node *node)
 {

Peter Zijlstra

2016-07-01 08:25 UTC #3 lore raw

Quoted 6 lines · David Howells

Quoted 1 line · Peter Zijlstra
One little niggle below, but:
How about the attached? I've updated that comment and rearranged rb_replace_node_rcu().

Looks good, thanks!