Question regarding Linux implementation of rbtrees

Question regarding Linux implementation of rbtrees

[Alexandru Moise]

Seeing as RB_RED is defined to be 0 in include/linux/rbtree_augmented.h
A call of this form: rb_set_parent_color(node, parent, RB_RED);
as seen in __rb_insert would only end up reassigning the parent "color"
(which is the parent pointer value cast to unsigned long) OR'd with 0.
Which would mean that nothing would really change regarding the parent's
"color". So, that would lead one to think that the diagram at case 2 showing
the grandparent's color going from black to red could not be completely accurate
as the Linux implementation presently stands.

Could the maintainers provide an answer as to why the below patch is the
__wrong__ thing to do? Apart from the obvious "the values of the macros
might change in the future".

Thanks,
../Alex
---
 lib/rbtree.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/lib/rbtree.c b/lib/rbtree.c
index 4ba2828a67c0..6b540be4dda4 100644
--- a/lib/rbtree.c
+++ b/lib/rbtree.c
@@ -135,7 +135,6 @@ __rb_insert(struct rb_node *node, struct rb_root *root,
 				rb_set_parent_color(parent, gparent, RB_BLACK);
 				node = gparent;
 				parent = rb_parent(node);
-				rb_set_parent_color(node, parent, RB_RED);
 				continue;
 			}
 
@@ -159,7 +158,6 @@ __rb_insert(struct rb_node *node, struct rb_root *root,
 				if (tmp)
 					rb_set_parent_color(tmp, parent,
 							    RB_BLACK);
-				rb_set_parent_color(parent, node, RB_RED);
 				augment_rotate(parent, node);
 				parent = node;
 				tmp = node->rb_right;
@@ -189,7 +187,6 @@ __rb_insert(struct rb_node *node, struct rb_root *root,
 				rb_set_parent_color(parent, gparent, RB_BLACK);
 				node = gparent;
 				parent = rb_parent(node);
-				rb_set_parent_color(node, parent, RB_RED);
 				continue;
 			}
 
@@ -202,7 +199,6 @@ __rb_insert(struct rb_node *node, struct rb_root *root,
 				if (tmp)
 					rb_set_parent_color(tmp, parent,
 							    RB_BLACK);
-				rb_set_parent_color(parent, node, RB_RED);
 				augment_rotate(parent, node);
 				parent = node;
 				tmp = node->rb_left;
-- 
2.12.2

Re: Question regarding Linux implementation of rbtrees

[Peter Zijlstra]

On Fri, Apr 14, 2017 at 12:24:55AM +0200, Alexandru Moise wrote:
> Seeing as RB_RED is defined to be 0 in include/linux/rbtree_augmented.h
> A call of this form: rb_set_parent_color(node, parent, RB_RED);
> as seen in __rb_insert would only end up reassigning the parent "color"
> (which is the parent pointer value cast to unsigned long) OR'd with 0.
> Which would mean that nothing would really change regarding the parent's
> "color". So, that would lead one to think that the diagram at case 2 showing
> the grandparent's color going from black to red could not be completely accurate
> as the Linux implementation presently stands.
> 
> Could the maintainers provide an answer as to why the below patch is the
> __wrong__ thing to do? Apart from the obvious "the values of the macros
> might change in the future".
> 
> Thanks,
> ../Alex
> ---
>  lib/rbtree.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/lib/rbtree.c b/lib/rbtree.c
> index 4ba2828a67c0..6b540be4dda4 100644
> --- a/lib/rbtree.c
> +++ b/lib/rbtree.c
> @@ -135,7 +135,6 @@ __rb_insert(struct rb_node *node, struct rb_root *root,
>  				rb_set_parent_color(parent, gparent, RB_BLACK);
>  				node = gparent;
>  				parent = rb_parent(node);
> -				rb_set_parent_color(node, parent, RB_RED);
>  				continue;
>  			}
>  

So who would clear the bit then? The point here is (IIRC) that node is
black and needs to become red.

Re: Question regarding Linux implementation of rbtrees

[Alexandru Moise]

On Fri, Apr 14, 2017 at 10:51:32AM +0200, Peter Zijlstra wrote:
> On Fri, Apr 14, 2017 at 12:24:55AM +0200, Alexandru Moise wrote:
> > Seeing as RB_RED is defined to be 0 in include/linux/rbtree_augmented.h
> > A call of this form: rb_set_parent_color(node, parent, RB_RED);
> > as seen in __rb_insert would only end up reassigning the parent "color"
> > (which is the parent pointer value cast to unsigned long) OR'd with 0.
> > Which would mean that nothing would really change regarding the parent's
> > "color". So, that would lead one to think that the diagram at case 2 showing
> > the grandparent's color going from black to red could not be completely accurate
> > as the Linux implementation presently stands.
> > 
> > Could the maintainers provide an answer as to why the below patch is the
> > __wrong__ thing to do? Apart from the obvious "the values of the macros
> > might change in the future".
> > 
> > Thanks,
> > ../Alex
> > ---
> >  lib/rbtree.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/lib/rbtree.c b/lib/rbtree.c
> > index 4ba2828a67c0..6b540be4dda4 100644
> > --- a/lib/rbtree.c
> > +++ b/lib/rbtree.c
> > @@ -135,7 +135,6 @@ __rb_insert(struct rb_node *node, struct rb_root *root,
> >  				rb_set_parent_color(parent, gparent, RB_BLACK);
> >  				node = gparent;
> >  				parent = rb_parent(node);
> > -				rb_set_parent_color(node, parent, RB_RED);
> >  				continue;
> >  			}
> >  
> 
> So who would clear the bit then? The point here is (IIRC) that node is
> black and needs to become red.

Now I've read it again and realized that it's actually in rb_parent() that
the bit gets cleared and all rb_set_parent_color() does is assign the new
pointer cast to ulong to the node's color. I was expecting that the bit would be cleared
in rb_set_parent_color().

Sorry for the noise.

../Alex