| Message ID | 20170617093253.3990-10-kpraveen.lkml@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 2017-06-17 at 15:02 +0530, Praveen Kumar wrote: > --- a/xen/common/rbtree.c > +++ b/xen/common/rbtree.c > @@ -90,8 +90,23 @@ void rb_insert_color(struct rb_node *node, struct > rb_root *root) > { > struct rb_node *parent, *gparent; > > - while ((parent = rb_parent(node)) && rb_is_red(parent)) > + while (true) > { > And here we are again. (I.e., in the cited Linux's commit, this is being turned into 'while (true) {`. So, I think we should gather others' opinion about how to deal with these aspects of this series. So, I'll stop my review for now, and chase feedback. Regards, Dario
>>> On 19.06.17 at 19:13, <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 >> @@ -90,8 +90,23 @@ void rb_insert_color(struct rb_node *node, struct >> rb_root *root) >> { >> struct rb_node *parent, *gparent; >> >> - while ((parent = rb_parent(node)) && rb_is_red(parent)) >> + while (true) >> { >> > And here we are again. (I.e., in the cited Linux's commit, this is > being turned into 'while (true) {`. > > So, I think we should gather others' opinion about how to deal with > these aspects of this series. So, I'll stop my review for now, and > chase feedback. I fully second your opinion here. I even wonder whether we shouldn't convert the file back to be fully Linux style first thing, so that Linux changes can be applied (mostly) as is, specifically without having to convert tabs to spaces. Jan
On Tue, 2017-06-20 at 01:26 -0600, Jan Beulich wrote: > > > > On 19.06.17 at 19:13, <dario.faggioli@citrix.com> wrote: > > And here we are again. (I.e., in the cited Linux's commit, this is > > being turned into 'while (true) {`. > > > > So, I think we should gather others' opinion about how to deal with > > these aspects of this series. So, I'll stop my review for now, and > > chase feedback. > > I fully second your opinion here. I even wonder whether we > shouldn't convert the file back to be fully Linux style first thing, > so that Linux changes can be applied (mostly) as is, specifically > without having to convert tabs to spaces. > That indeed would be good! Praveen, this would mean having a patch, at the beginning of the series, which converts the coding style of the files to Linux one. Basically, that would mean using tabs for indentation, and undoing any style change that may have been done in our tree, to make the file adopt the Xen style. In practise, the idea is ending up with something that is basically identical to what was in Linux, before all the patches you are porting were committed (and without the additional parts and features that we don't need, of course). At this point, even generating and applying the patches that you are porting, in this very series, would be really easy, and less error prone (as it can be almost entirely automated). Are you up for this? Thanks and Regards, Dario
Hi Dario and Jan, Sorry, I missed this update. On Tue, Jun 20, 2017 at 7:24 PM, Dario Faggioli <dario.faggioli@citrix. com> wrote: > > On Tue, 2017-06-20 at 01:26 -0600, Jan Beulich wrote: > > > > > > > > > > > > > > > > > > > On 19.06.17 at 19:13, <dario.faggioli@citrix.com> wrote: > > > And here we are again. (I.e., in the cited Linux's commit, this > > > is > > > being turned into 'while (true) {`. > > > > > > So, I think we should gather others' opinion about how to deal > > > with > > > these aspects of this series. So, I'll stop my review for now, > > > and > > > chase feedback. > > > > I fully second your opinion here. I even wonder whether we > > shouldn't convert the file back to be fully Linux style first > > thing, > > so that Linux changes can be applied (mostly) as is, specifically > > without having to convert tabs to spaces. > > > That indeed would be good! > > Praveen, this would mean having a patch, at the beginning of the > series, which converts the coding style of the files to Linux one. > > Basically, that would mean using tabs for indentation, and undoing > any > style change that may have been done in our tree, to make the file > adopt the Xen style. > > In practise, the idea is ending up with something that is basically > identical to what was in Linux, before all the patches you are > porting > were committed (and without the additional parts and features that we > don't need, of course). > > At this point, even generating and applying the patches that you are > porting, in this very series, would be really easy, and less error > prone (as it can be almost entirely automated). > > Are you up for this? Sounds good. Let me work on the same. Will re-send the updated patch series having first indentation changes followed by series of changes in Linux code base ( as sent already ) > > > Thanks and Regards, > Dario > -- > <<This happens because I choose it to happen!>> (Raistlin Majere) > ----------------------------------------------------------------- > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
On Tue, 2017-06-27 at 21:05 +0530, Praveen Kumar wrote: > Hi Dario and Jan, > > Sorry, I missed this update. > No problem. > On Tue, Jun 20, 2017 at 7:24 PM, Dario Faggioli > <dario.faggioli@citrix.com> wrote: > > In practise, the idea is ending up with something that is basically > > identical to what was in Linux, before all the patches you are > > porting > > were committed (and without the additional parts and features that > > we > > don't need, of course). > > > > At this point, even generating and applying the patches that you > > are > > porting, in this very series, would be really easy, and less error > > prone (as it can be almost entirely automated). > > > > Are you up for this? > > Sounds good. Let me work on the same. Will re-send the updated patch > series having first indentation changes followed by series of changes > in Linux code base ( as sent already ) > Great. Make sure you update your git tree, as some of your patches have already been committed (which means, of course, you don't have to include them in the series any longer! :-)) Dario
diff --git a/xen/common/rbtree.c b/xen/common/rbtree.c index 25902c0659..fff8e22526 100644 --- a/xen/common/rbtree.c +++ b/xen/common/rbtree.c @@ -90,8 +90,23 @@ void rb_insert_color(struct rb_node *node, struct rb_root *root) { struct rb_node *parent, *gparent; - while ((parent = rb_parent(node)) && rb_is_red(parent)) + while (true) { + /* + * Loop invariant: node is red + * + * If there is a black parent, we are done. + * Otherwise, take some corrective action as we don't + * want a red root or two consecutive red nodes. + */ + parent = rb_parent(node); + if (!parent) + { + rb_set_black(node); + break; + } else if (rb_is_black(parent)) + break; + gparent = rb_parent(parent); if (parent == gparent->rb_left) @@ -143,8 +158,6 @@ void rb_insert_color(struct rb_node *node, struct rb_root *root) break; } } - - rb_set_black(root->rb_node); } EXPORT_SYMBOL(rb_insert_color);