* [PATCH] Introduce rb_replace_node_rcu()
@ 2016-07-01 7:08 David Howells
2016-07-01 7:22 ` Peter Zijlstra
2016-07-01 8:08 ` David Howells
0 siblings, 2 replies; 4+ messages in thread
From: David Howells @ 2016-07-01 7:08 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: dhowells, linux-afs, linux-kernel
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 <dhowells@redhat.com>
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 <dhowells@redhat.com>
cc: Peter Zijlstra <peterz@infradead.org>
diff --git a/include/linux/rbtree.h b/include/linux/rbtree.h
index b6900099ea81..e585018498d5 100644
--- a/include/linux/rbtree.h
+++ b/include/linux/rbtree.h
@@ -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)
diff --git a/include/linux/rbtree_augmented.h b/include/linux/rbtree_augmented.h
index 14d7b831b63a..d076183e49be 100644
--- a/include/linux/rbtree_augmented.h
+++ b/include/linux/rbtree_augmented.h
@@ -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));
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 */
+ __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 (;;) {
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Introduce rb_replace_node_rcu()
2016-07-01 7:08 [PATCH] Introduce rb_replace_node_rcu() David Howells
@ 2016-07-01 7:22 ` Peter Zijlstra
2016-07-01 8:08 ` David Howells
1 sibling, 0 replies; 4+ messages in thread
From: Peter Zijlstra @ 2016-07-01 7:22 UTC (permalink / raw)
To: David Howells; +Cc: linux-afs, linux-kernel
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 <dhowells@redhat.com>
> 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 <dhowells@redhat.com>
> cc: Peter Zijlstra <peterz@infradead.org>
One little niggle below, but:
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Introduce rb_replace_node_rcu()
2016-07-01 7:08 [PATCH] Introduce rb_replace_node_rcu() David Howells
2016-07-01 7:22 ` Peter Zijlstra
@ 2016-07-01 8:08 ` David Howells
2016-07-01 8:14 ` Peter Zijlstra
1 sibling, 1 reply; 4+ messages in thread
From: David Howells @ 2016-07-01 8:08 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: dhowells, linux-afs, linux-kernel
Peter Zijlstra <peterz@infradead.org> wrote:
> 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 <dhowells@redhat.com>
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 <dhowells@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
diff --git a/include/linux/rbtree.h b/include/linux/rbtree.h
index b6900099ea81..e585018498d5 100644
--- a/include/linux/rbtree.h
+++ b/include/linux/rbtree.h
@@ -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)
diff --git a/include/linux/rbtree_augmented.h b/include/linux/rbtree_augmented.h
index 14d7b831b63a..d076183e49be 100644
--- a/include/linux/rbtree_augmented.h
+++ b/include/linux/rbtree_augmented.h
@@ -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));
diff --git a/lib/rbtree.c b/lib/rbtree.c
index 1356454e36de..eb8a19fee110 100644
--- a/lib/rbtree.c
+++ b/lib/rbtree.c
@@ -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)
{
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Introduce rb_replace_node_rcu()
2016-07-01 8:08 ` David Howells
@ 2016-07-01 8:14 ` Peter Zijlstra
0 siblings, 0 replies; 4+ messages in thread
From: Peter Zijlstra @ 2016-07-01 8:14 UTC (permalink / raw)
To: David Howells; +Cc: linux-afs, linux-kernel
On Fri, Jul 01, 2016 at 09:08:05AM +0100, David Howells wrote:
> Peter Zijlstra <peterz@infradead.org> wrote:
>
> > One little niggle below, but:
>
> How about the attached? I've updated that comment and rearranged
> rb_replace_node_rcu().
Looks good, thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-07-01 8:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-01 7:08 [PATCH] Introduce rb_replace_node_rcu() David Howells
2016-07-01 7:22 ` Peter Zijlstra
2016-07-01 8:08 ` David Howells
2016-07-01 8:14 ` Peter Zijlstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).