From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755362AbcLTF7n (ORCPT ); Tue, 20 Dec 2016 00:59:43 -0500 Received: from mail-pg0-f68.google.com ([74.125.83.68]:36365 "EHLO mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751944AbcLTF7k (ORCPT ); Tue, 20 Dec 2016 00:59:40 -0500 Date: Tue, 20 Dec 2016 13:59:14 +0800 From: Boqun Feng To: "Paul E. McKenney" Cc: Colin Ian King , Mark Rutland , linux-kernel@vger.kernel.org, Josh Triplett , Steven Rostedt , Mathieu Desnoyers , Lai Jiangshan Subject: Re: [RFC v2 4/5] rcu: Use for_each_leaf_node_cpu() in force_qs_rnp() Message-ID: <20161220055914.GB1316@tardis.cn.ibm.com> References: <20161215024204.28620-1-boqun.feng@gmail.com> <20161215024204.28620-5-boqun.feng@gmail.com> <20161215120459.GE21758@leverpostej> <20161215144242.GN9728@tardis.cn.ibm.com> <05a9953b-aaa4-6117-b120-85c12ad56ace@canonical.com> <20161219151515.GP9728@tardis.cn.ibm.com> <20161220050913.GP3924@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="b5gNqxB1S1yM7hjW" Content-Disposition: inline In-Reply-To: <20161220050913.GP3924@linux.vnet.ibm.com> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --b5gNqxB1S1yM7hjW Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Dec 19, 2016 at 09:09:13PM -0800, Paul E. McKenney wrote: > On Mon, Dec 19, 2016 at 11:15:15PM +0800, Boqun Feng wrote: > > On Thu, Dec 15, 2016 at 02:51:36PM +0000, Colin Ian King wrote: > > > On 15/12/16 14:42, Boqun Feng wrote: > > > > On Thu, Dec 15, 2016 at 12:04:59PM +0000, Mark Rutland wrote: > > > >> On Thu, Dec 15, 2016 at 10:42:03AM +0800, Boqun Feng wrote: > > > >>> ->qsmask of an RCU leaf node is usually more sparse than the > > > >>> corresponding cpu_possible_mask. So replace the > > > >>> for_each_leaf_node_possible_cpu() in force_qs_rnp() with > > > >>> for_each_leaf_node_cpu() to save several checks. > > > >>> > > > >>> [Note we need to use "1UL << bit" instead of "1 << bit" to genera= te the > > > >>> corresponding mask for a bit because @mask is unsigned long, this= was > > > >>> spotted by Colin Ian King and Coverity= Scan in > > > >>> a previous version of this patch.] > > > >> > > > >> Nit: This note can go now that we use leaf_node_cpu_bit(). ;) > > > >> > > > >=20 > > > > I kinda keep this here for honoring the effort of finding out this = bug > > > > from Colin, but yes, it's no longer needed here for the current cod= e. > > >=20 > > > Yep, remove it. > > >=20 > >=20 > > Paul, here is a modified version of this patch, what I only did is > > removing this note. > >=20 > > Besides I rebased the whole series on the current rcu/dev branch of -rcu > > tree, on this very commit: > >=20 > > 8e9b2521b18a ("doc: Quick-Quiz answers are now inline") > >=20 > > And I put the latest version at > >=20 > > git://git.kernel.org/pub/scm/linux/kernel/git/boqun/linux.git leaf-node > >=20 > > If you thought it's better, I could send a v3 ;-) >=20 > I would feel better about this patchset if it reduced the number of lines > of code rather than increasing them. That said, part of the increase > is a commment. Still, I am not convinced that the extra level of macro > is carrying its weight. >=20 > dbf18a2422e2 ("rcu: Introduce for_each_leaf_node_cpu()") >=20 > The commit log needs a bit of wordsmithing. >=20 > The added WARN_ON_ONCE(!cpu_possible(cpu)) still seems strange. > What is its purpose, really? What does its triggering tell you? > What other checks did you consider as an alternative? >=20 The check is an over-case one, it's introduced because I'm worried about some code outside the RCU code mis-sets the ->qsmask* or ->expmask* on an "impossible" CPU. I will explanation later in more details. > And if you are going to add checks of this type, should you > also check for this being a leaf rcu_node structure? >=20 I don't think I want to check that, and I don't think check cpu_possible(cpu) in the macro is similar to that. > 3f0b4ba1fe94 ("rcu: Use for_each_leaf_node_cpu() in RCU stall checking") >=20 > This does look a bit nicer, but why the added blank lines? > Are they really helping? >=20 > The commit log seems a bit misplaced. This code is almost never > executed (once per 21 seconds at the most), so performance really > isn't a consideration. The simpler-looking code might be. >=20 > fd799f1ac7b7 ("rcu: Use for_each_leaf_node_cpu() in ->expmask iteration") >=20 > Ditto on blank lines. >=20 > Again, this code is executed per expedited grace period, so > performance really isn't a big deal. More of a big deal than > the stall-warning code, but we still are way off of any fastpath. >=20 > 69a1baedbf42 ("rcu: Use for_each_leaf_node_cpu() in force_qs_rnp()") >=20 > Ditto again on blank lines. >=20 > And on the commit log. This code is executed about once > per several jiffies, and on larger machines, per 20 jiffies > or so. Performance really isn't a consideration. >=20 > 7b00e50e3efb ("rcu: Use for_each_leaf_node_cpu() in online CPU iteration") >=20 > And another ditto on blank lines. >=20 > This code executes once per CPU-hotplug operation, so again isn't > at all performance critical. >=20 > In short, if you are trying to sell this to me as a significant performan= ce > boost, I am not buying. The added WARN_ON_ONCE() looks quite dubious, Yep, it won't help the performance a lot, but it=20 1) helps the performance in theory, because it iterates less CPUs 2) makes code cleaner. By "cleaner", I mean we can a) affort more blank lines to make loops separated from other code and b) descrease the indent levels for those loops. But, yes I should add those points in the commit log, because those are more visible effects. > though perhaps I am misunderstanding its purpose. My assumption is > that you want to detect missing UL suffixes on bitmask constants, in > which case I bet there is a better way. >=20 The WARN_ON_ONCE() is not for detecting missing UL suffixes on bitmask constatns, and we don't need to check that, because we use leaf_node_cpu_id() now. As I said, this is an over-case check, and we can drop if we guarante that CPUs masked in ->qsmask* and ->expmask* must be a "possible" CPU, IOW, ->qsmask* and ->expmask* are the subsets (with offset fixed by ->grplo) of cpu_possible_mask. Hmm.. and I just check the code, the initial values of ->qsmask* and ->expmask* are from ->qsmaskinitnext and ->expmaskinitnext, and the latter two are set in rcu_cpu_starting() since commit 7ec99de36f40 ("rcu: Provide exact CPU-online tracking for RCU") , and rcu_cpu_starting() only set the corresponding bit of _this_ cpu in a leaf node's ->qsmaskinitnext and ->expmaskinitnext. So looks like we are safe to remove the WARN_ON_ONCE() check, because a ever-running CPU must be a possible CPU, IIRC. But this brings a side question, is the callsite of rcu_cpu_starting() is correct? Given rcu_cpu_starting() ignores the @cpu parameter and only set _this_ cpu's bit in a leaf node? Regards, Boqun > Speaking of which, how do we know that this is free of bugs? >=20 > Thanx, Paul >=20 > > Regards, > > Boqun > >=20 > > ------------------------>8 > > From: Boqun Feng > > Date: Thu, 8 Dec 2016 23:21:11 +0800 > > Subject: [PATCH v2.1 4/5] rcu: Use for_each_leaf_node_cpu() in force_qs= _rnp() > >=20 > > ->qsmask of an RCU leaf node is usually more sparse than the > > corresponding cpu_possible_mask. So replace the > > for_each_leaf_node_possible_cpu() in force_qs_rnp() with > > for_each_leaf_node_cpu() to save several checks. > >=20 > > Signed-off-by: Boqun Feng > > --- > > kernel/rcu/tree.c | 12 +++++------- > > 1 file changed, 5 insertions(+), 7 deletions(-) > >=20 > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > index 4ea4496f4ecc..c2b753fb7f09 100644 > > --- a/kernel/rcu/tree.c > > +++ b/kernel/rcu/tree.c > > @@ -3046,13 +3046,11 @@ static void force_qs_rnp(struct rcu_state *rsp, > > continue; > > } > > } > > - for_each_leaf_node_possible_cpu(rnp, cpu) { > > - unsigned long bit =3D leaf_node_cpu_bit(rnp, cpu); > > - if ((rnp->qsmask & bit) !=3D 0) { > > - if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > - mask |=3D bit; > > - } > > - } > > + > > + for_each_leaf_node_cpu(rnp, rnp->qsmask, cpu) > > + if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj)) > > + mask |=3D leaf_node_cpu_bit(rnp, cpu); > > + > > if (mask !=3D 0) { > > /* Idle/offline CPUs, report (releases rnp->lock. */ > > rcu_report_qs_rnp(mask, rsp, rnp, rnp->gpnum, flags); > > --=20 > > 2.10.2 > >=20 >=20 --b5gNqxB1S1yM7hjW Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAlhYyK4ACgkQSXnow7UH +rhvpwf/V3B/jb/umTd3y9D0qv6fxcZ4IQoym/PIP6eUk8LlpzLBJDSxMTrrCmCy nMsjDVekFJgth7/7PrLq6oViARZp+7oYoAPMtihf8KiA/1cyu7FH17gXjl1afAIA SX0X9++n0nObxDM+Je3nFGCClxBs5ZYegTWd8IPYIcAyzlErxyLAEzswrNFE0r0O Xiw8MOuvQu0cPgDc6qODc81PI4XOUkvygW5KMNC4IIXid3OXHF+NI/WxWKyl8Rik v9GIX6SrbLfGUVNgO2LB3xBRtVvy/yDMO86Ocj6oOq9v+mIrHlh+AUJF3Oku81sC 26jALv2sgkErzcOenO/Vjqu9VNB1NQ== =uD7G -----END PGP SIGNATURE----- --b5gNqxB1S1yM7hjW--