From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758145AbcLOOjW (ORCPT ); Thu, 15 Dec 2016 09:39:22 -0500 Received: from mail-pf0-f195.google.com ([209.85.192.195]:35936 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758083AbcLOOjU (ORCPT ); Thu, 15 Dec 2016 09:39:20 -0500 Date: Thu, 15 Dec 2016 22:38:58 +0800 From: Boqun Feng To: Mark Rutland Cc: linux-kernel@vger.kernel.org, "Paul E . McKenney " , Josh Triplett , Steven Rostedt , Mathieu Desnoyers , Lai Jiangshan , Colin King Subject: Re: [RFC v2 1/5] rcu: Introduce for_each_leaf_node_cpu() Message-ID: <20161215143858.GM9728@tardis.cn.ibm.com> References: <20161215024204.28620-1-boqun.feng@gmail.com> <20161215024204.28620-2-boqun.feng@gmail.com> <20161215114351.GA21758@leverpostej> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JvUS8mwutKMHKosv" Content-Disposition: inline In-Reply-To: <20161215114351.GA21758@leverpostej> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --JvUS8mwutKMHKosv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Dec 15, 2016 at 11:43:52AM +0000, Mark Rutland wrote: > On Thu, Dec 15, 2016 at 10:42:00AM +0800, Boqun Feng wrote: > > There are some places inside RCU core, where we need to iterate all mask > > (->qsmask, ->expmask, etc) bits in a leaf node, in order to iterate all > > corresponding CPUs. The current code iterates all possible CPUs in this > > leaf node and then checks with the mask to see whether the bit is set. > >=20 > > However, given the fact that most bits in cpu_possible_mask are set but > > rare bits in an RCU leaf node mask are set(in other words, ->qsmask and > > its friends are usually more sparse than cpu_possible_mask), it's better > > to iterate in the other way, that is iterating mask bits in a leaf node. > > By doing so, we can save several checks in the loop, moreover, that fast > > path checking(e.g. ->qsmask =3D=3D 0) could then be consolidated into t= he > > loop logic. > >=20 > > This patch introduce for_each_leaf_node_cpu() to iterate mask bits in a > > more efficient way. > >=20 > > By design, The CPUs whose bits are set in the leaf node masks should be > > a subset of possible CPUs, so we don't need extra check with > > cpu_possible(), however, a WARN_ON_ONCE() is put in the loop to check > > whether there are some nasty cases we miss. > >=20 > > Signed-off-by: Boqun Feng > > --- > > kernel/rcu/tree.h | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > >=20 > > diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h > > index c0a4bf8f1ed0..70ef44a082e0 100644 > > --- a/kernel/rcu/tree.h > > +++ b/kernel/rcu/tree.h > > @@ -295,6 +295,22 @@ struct rcu_node { > > cpu <=3D rnp->grphi; \ > > cpu =3D cpumask_next((cpu), cpu_possible_mask)) > > =20 > > + > > +#define MASK_BITS(mask) (BITS_PER_BYTE * sizeof(mask)) > > +/* > > + * Iterate over all CPUs a leaf RCU node which are still masked in > > + * @mask. > > + * > > + * Note @rnp has to be a leaf node and @mask has to belong to @rnp. >=20 > Not a big deal, but perhaps it's worth enforcing this? If we took just > the name of the mask here, (e.g. qsmask rather than rnp->qsmask), we > could have the macro always use (rnp)->(mask). That would also make the > invocations shorter. >=20 I thought about this approach, but there may be some cases it seems inappropriate, see patch #5, passing "qsmaskinitnext" directly to the for_each_leaf_node_cpu() might be OK, but it just break another abstraction layer which rcu_rnp_online_cpus() provides. > > And we > > + * assume that no CPU is masked in @mask but not set in cpu_possible_m= ask. IOW, > > + * masks of a leaf node never set a bit for an "impossible" CPU. > > + */ > > +#define for_each_leaf_node_cpu(rnp, mask, cpu) \ > > + for ((cpu) =3D (rnp)->grplo + find_first_bit(&(mask), MASK_BITS(mask)= ); \ > > + (cpu) <=3D (rnp)->grphi && !WARN_ON_ONCE(!cpu_possible(cpu)); \ >=20 > If this happens, we'll exit the loop. If there are any reamining > possible CPUs, we'll skip them, which would be less than ideal. >=20 > I guess this shouldn't happen anyway, but it might be worth continuing. >=20 I chose to break if we met impossible only because I wanted to avoid using that "if(...) else" trick in an iteration macro ;-) I don't know whether this is the first time something like this is brought into kernel, so I'm kinda hesitating to bring this in. But seems I got you as one supporter ;-) Certainly, skip is better than stop. > > + (cpu) =3D (rnp)->grplo + find_next_bit(&(mask), MASK_BITS(mask),= \ > > + (cpu) - (rnp)->grplo + 1)) > > + >=20 > I was going to ask if that + 1 was correct, but I see that it is! >=20 > So FWIW: >=20 > Acked-by: Mark Rutland >=20 Thanks ;-) >=20 > I had a go at handling my comments above, but I'm not sure it's any > better: >=20 > #define cpu_to_grp(rnp, cpu) ((cpu) - (rnp)->grplo) >=20 > #define grp_to_cpu(rnp, cpu) ((cpu) + (rnp)->grplo) >=20 > #define node_first_cpu(rnp, mask) \ > grp_to_cpu(find_first_bit(&(rnp)->mask, MASK_BITS((rnp)->mask))) >=20 > #define node_next_cpu(rnp, mask, cpu) > grp_to_cpu(rnp, find_next_bit(&(rnp)->mask, MASK_BITS((rnp)->mask), > cpu_to_grp(rnp, cpu) + 1)) >=20 I tried something similar, but it seems bringing too many abstraction layers just for one macro. I basically follow the rule: if the potential users are less than three, no need to do abstraction ;-) But thank you for looking into this ;-) Regards, Boqun > #define for_each_leaf_node_cpu(rnp, mask, cpu) \ > for ((cpu) =3D node_first_cpu(rnp, mask); \ > (cpu) <=3D (rnp)->grphi; \ > (cpu) =3D node_next_cpu(rnp, mask, cpu)) \ > if (WARN_ON_ONCE(!cpu_possible(cpu))) \ > continue; \ > else >=20 > Thanks, > Mark. --JvUS8mwutKMHKosv Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAABCAAGBQJYUqr/AAoJEEl56MO1B/q4+5UH/RUck8XJB3sd9MGNOTgPDY3M YcBSCQMKaoYZRbLEkBe49Sr36YNOJ3DOTgR4OzxkBdTargY6XSUVROHGrxezVkrt YM9EARrH7KFX0B6V56ljDBJYxVbnVumlIn1QH+XnbdRDS4qvx/bsV+OREzCkcXSA p4CpTBY8ohqrguAhdORA6kIEjQND5eZRCpNhyJBwNoh+YoqqPHKCV5zK3+4NPnW8 gUuwIrTVcesR1eSexwJcyqEEnKPcxohe5xmq5rkKHgFY3cBrs+ZZ/lRLv0BIYP1x x772W4Ml+WzMfwXN/KlVj9enpFQAh2BoUzGaK8f1T7/fbLkSAcDRmVjq3CowvWw= =mhSA -----END PGP SIGNATURE----- --JvUS8mwutKMHKosv--