From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 06/20] rbtree: empty nodes have no color Date: Mon, 19 Jun 2017 19:00:48 +0200 Message-ID: <1497891648.7405.11.camel@citrix.com> References: <20170617093253.3990-1-kpraveen.lkml@gmail.com> <20170617093253.3990-7-kpraveen.lkml@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7862121168515570085==" Return-path: In-Reply-To: <20170617093253.3990-7-kpraveen.lkml@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Praveen Kumar , xen-devel@lists.xen.org Cc: sstabellini@kernel.org, wei.liu2@citrix.com, George.Dunlap@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, tim@xen.org, jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org --===============7862121168515570085== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-NPH061nLG/mouNq7orJ0" --=-NPH061nLG/mouNq7orJ0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sat, 2017-06-17 at 15:02 +0530, Praveen Kumar wrote: > Empty nodes have no color.=C2=A0=C2=A0We can make use of this property to > simplify > the code emitted by the RB_EMPTY_NODE and RB_CLEAR_NODE > macros.=C2=A0=C2=A0Also, > we can get rid of the rb_init_node function which had been introduced > by > commit 88d19cf37952 ("timers: Add rb_init_node() to allow for stack > allocated rb nodes") to avoid some issue with the empty node's color > not > being initialized. >=20 > I'm not sure what the RB_EMPTY_NODE checks in rb_prev() / rb_next() > are > doing there, though.=C2=A0=C2=A0axboe introduced them in commit 10fd48f23= 76d > ("rbtree: fixed reversed RB_EMPTY_NODE and rb_next/prev").=C2=A0=C2=A0The= way I > see it, the 'empty node' abstraction is only used by rbtree users to > flag nodes that they haven't inserted in any rbtree, so asking the > predecessor or successor of such nodes doesn't make any sense. >=20 > One final rb_init_node() caller was recently added in sysctl code to > implement faster sysctl name lookups.=C2=A0=C2=A0This code doesn't make u= se of > RB_EMPTY_NODE at all, and from what I could see it only called > rb_init_node() under the mistaken assumption that such initialization > was > required before node insertion. >=20 > [sfr@canb.auug.org.au: fix net/ceph/osd_client.c build] > Signed-off-by: Michel Lespinasse > Cc: Andrea Arcangeli > Acked-by: David Woodhouse > Cc: Rik van Riel > Cc: Peter Zijlstra > Cc: Daniel Santos > Cc: Jens Axboe > Cc: "Eric W. Biederman" > Cc: John Stultz > Signed-off-by: Stephen Rothwell > Signed-off-by: Andrew Morton > Signed-off-by: Linus Torvalds > [Linux commit 4c199a93a2d36b277a9fd209a0f2793f8460a215] >=20 > Ported rbtree.h and rbtree.c changes which are relevant to Xen. >=20 > Signed-off-by: Praveen Kumar > I was about to say: Reviewed-by: Dario Faggioli Although... > diff --git a/xen/include/xen/rbtree.h b/xen/include/xen/rbtree.h > index 3eb527eb37..f74b68ce62 100644 > --- a/xen/include/xen/rbtree.h > +++ b/xen/include/xen/rbtree.h > @@ -52,9 +52,12 @@ static inline void rb_set_color(struct rb_node > *rb, int color) > =C2=A0#define RB_ROOT (struct rb_root) { NULL, } > =C2=A0#define rb_entry(ptr, type, member) container_of(ptr, type, member) > =C2=A0 > -#define RB_EMPTY_ROOT(root) ((root)->rb_node =3D=3D NULL) > -#define RB_EMPTY_NODE(node) (rb_parent(node) =3D=3D node) > -#define RB_CLEAR_NODE(node) (rb_set_parent(node, node)) > +#define RB_EMPTY_ROOT(root)=C2=A0=C2=A0((root)->rb_node =3D=3D NULL) > + > +/* 'empty' nodes are nodes that are known not to be inserted in an > rbree */ > +#define RB_EMPTY_NODE(node)=C2=A0=C2=A0((node)->rb_parent_color =3D=3D (= unsigned > long)(node)) > +#define RB_CLEAR_NODE(node)=C2=A0=C2=A0((node)->rb_parent_color =3D (uns= igned > long)(node)) > + >=20 ...There's again divergence between this patch and Linux's commit (in this case, there is one blank line added, which is not in Linux patch). I'm again uncertain about the severity of this, and about the best course of action, but please, try to avoid things like this... When reviewing patches like this, it's very hard to spot the differences, and these only add noise, and make it even harder :-/ Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-NPH061nLG/mouNq7orJ0 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJZSANAAAoJEBZCeImluHPu0UgP/Rwc5OulI2NcCmGVgEv6outo ZP3dlrQDlHPS07KehG9nXwP1rktNLWai0ZhqoEJoBYuM61itPAgUunAo/HD/psDB GEiYsCVlzU0YkyOPHCmx/VrOQaNmMw0cjRlCNdbVQBYCQHG45B+Kzuuw3av9DpiP 1uY4+onca2MpHN2Jyc+tdEcugZKXy5WzBHShzM1Rgca7DD3XAE3p1Db82do2Ovmw DmwVL38Pq0JxDKxJ8O5gR20kcBjFCOw5+lxzNw9Ay4oBSGdShQdXAY9SGNXzYs7h v95w2j9EZSKufGBpWLcDb1Z8CudE3WhShZZa2S4i/A1QCux0FByp0kwzpfPaQiYW 1TkzCulR9EnUBoZZgFenkgmzLEQpSY7rpsuUM2jclWhvgvOOsqN3hHCLdeFMsj5o ZZrQJbH8ABo+vLNR/CaXqmajw51+im8Lp0qqVkThHt/LBrrpz8mLLEcbJjo3txSi uoUFutaLW5GDVPs9iy5iKGwUvinB24STPonJHZ/X8dj6TaVZjErx0N30oNgGScEL FQff1hY2NCSwQ5UktOsL4VXZgYu8gjPXsSpszVNO6uVqdiSD7xR18lhqZ/WFtifC qOdzaEZBDSpukP1P9kmFhYikTcIr67pxaTAhii+KRC09VOQrTBEY7I7OYhFr8Jeo 6510tcaUs6MplEuOZ95h =Mm/t -----END PGP SIGNATURE----- --=-NPH061nLG/mouNq7orJ0-- --===============7862121168515570085== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============7862121168515570085==--