| Message ID | 20170617093253.3990-6-kpraveen.lkml@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 2017-06-17 at 15:02 +0530, Praveen Kumar wrote: > Furthermore, notice that the initial checks: > > if (!node->rb_left) > child = node->rb_right; > else if (!node->rb_right) > child = node->rb_left; > else > { > ... > } > guarantee that old->rb_right is set in the final else branch, > therefore > we can omit checking that again. > > Signed-off-by: Wolfram Strepp <wstrepp@gmx.de> > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > [Linux commit 4b324126e0c6c3a5080ca3ec0981e8766ed6f1ee] > And yet, the actual patch is slightly different. As in... > --- a/xen/common/rbtree.c > +++ b/xen/common/rbtree.c > @@ -250,15 +250,16 @@ void rb_erase(struct rb_node *node, struct > rb_root *root) > if (child) > rb_set_parent(child, parent); > parent->rb_left = child; > + > + node->rb_right = old->rb_right; > + rb_set_parent(old->rb_right, node); > } > > node->rb_parent_color = old->rb_parent_color; > - node->rb_right = old->rb_right; > node->rb_left = old->rb_left; > ...In the Linux commit, this blank line is removed too. > rb_set_parent(old->rb_left, node); > - if (old->rb_right) > - rb_set_parent(old->rb_right, node); > + > goto color; > } > I don't think this is too big of a deal per se, I'd I'd leave to maintainers and committers to decide whether something like this is enough for asking a resend, or whether it can be fixed upon commit or even left as it is. For sure, we know that these patches really needs to be, as much as possible, 1:1 copies of Linux's ones, even in the smallest detail. And the reason is to make the life of someone wanting to do another round of import, in future, as easy as possible. Regards, Dario
>>> On 19.06.17 at 18:53, <dario.faggioli@citrix.com> wrote: > On Sat, 2017-06-17 at 15:02 +0530, Praveen Kumar wrote: >> --- a/xen/common/rbtree.c >> +++ b/xen/common/rbtree.c >> @@ -250,15 +250,16 @@ void rb_erase(struct rb_node *node, struct >> rb_root *root) >> if (child) >> rb_set_parent(child, parent); >> parent->rb_left = child; >> + >> + node->rb_right = old->rb_right; >> + rb_set_parent(old->rb_right, node); >> } >> >> node->rb_parent_color = old->rb_parent_color; >> - node->rb_right = old->rb_right; >> node->rb_left = old->rb_left; >> > ...In the Linux commit, this blank line is removed too. > >> rb_set_parent(old->rb_left, node); >> - if (old->rb_right) >> - rb_set_parent(old->rb_right, node); >> + >> goto color; >> } >> > I don't think this is too big of a deal per se, I'd I'd leave to > maintainers and committers to decide whether something like this is > enough for asking a resend, or whether it can be fixed upon commit or > even left as it is. > > For sure, we know that these patches really needs to be, as much as > possible, 1:1 copies of Linux's ones, even in the smallest detail. And > the reason is to make the life of someone wanting to do another round > of import, in future, as easy as possible. Yes, and it is for this very reason that I'd like to see a resend. Else there's the risk that fixing this up upon commit causes subsequent patches to also not apply. The committer, if doing any edits at all, should not be required to spend meaningful amounts of extra time on applying a patch (or series). Jan
diff --git a/xen/common/rbtree.c b/xen/common/rbtree.c index 90db00a5e8..9d3c5fab95 100644 --- a/xen/common/rbtree.c +++ b/xen/common/rbtree.c @@ -250,15 +250,16 @@ void rb_erase(struct rb_node *node, struct rb_root *root) if (child) rb_set_parent(child, parent); parent->rb_left = child; + + node->rb_right = old->rb_right; + rb_set_parent(old->rb_right, node); } node->rb_parent_color = old->rb_parent_color; - node->rb_right = old->rb_right; node->rb_left = old->rb_left; rb_set_parent(old->rb_left, node); - if (old->rb_right) - rb_set_parent(old->rb_right, node); + goto color; }
Furthermore, notice that the initial checks: if (!node->rb_left) child = node->rb_right; else if (!node->rb_right) child = node->rb_left; else { ... } guarantee that old->rb_right is set in the final else branch, therefore we can omit checking that again. Signed-off-by: Wolfram Strepp <wstrepp@gmx.de> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> [Linux commit 4b324126e0c6c3a5080ca3ec0981e8766ed6f1ee] Ported to Xen. Signed-off-by: Praveen Kumar <kpraveen.lkml@gmail.com> --- xen/common/rbtree.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)