Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1623404 > unrolled thread

Question regarding Linux implementation of rbtrees

Back to article view | Back to linux.kernel

Contents

  Question regarding Linux implementation of rbtrees Alexandru Moise <00moses.alexander00@gmail.com> - 2017-04-14 00:30 +0200
    Re: Question regarding Linux implementation of rbtrees Peter Zijlstra <peterz@infradead.org> - 2017-04-14 11:00 +0200
      Re: Question regarding Linux implementation of rbtrees Alexandru Moise <00moses.alexander00@gmail.com> - 2017-04-14 11:40 +0200

#1623404 — Question regarding Linux implementation of rbtrees

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

[toc] | [next] | [standalone]

#1623579

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.

[toc] | [prev] | [next] | [standalone]

#1623615

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web