From: Boqun Feng <boqun.feng@gmail.com>
To: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Colin Ian King <colin.king@canonical.com>,
Mark Rutland <mark.rutland@arm.com>,
linux-kernel@vger.kernel.org,
Josh Triplett <josh@joshtriplett.org>,
Steven Rostedt <rostedt@goodmis.org>,
Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
Lai Jiangshan <jiangshanlai@gmail.com>
Subject: Re: [RFC v2 4/5] rcu: Use for_each_leaf_node_cpu() in force_qs_rnp()
Date: Tue, 20 Dec 2016 13:59:14 +0800 [thread overview]
Message-ID: <20161220055914.GB1316@tardis.cn.ibm.com> (raw)
In-Reply-To: <20161220050913.GP3924@linux.vnet.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 7467 bytes --]
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 generate the
> > > >>> corresponding mask for a bit because @mask is unsigned long, this was
> > > >>> spotted by Colin Ian King <colin.king@canonical.com> and CoverityScan in
> > > >>> a previous version of this patch.]
> > > >>
> > > >> Nit: This note can go now that we use leaf_node_cpu_bit(). ;)
> > > >>
> > > >
> > > > 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 code.
> > >
> > > Yep, remove it.
> > >
> >
> > Paul, here is a modified version of this patch, what I only did is
> > removing this note.
> >
> > Besides I rebased the whole series on the current rcu/dev branch of -rcu
> > tree, on this very commit:
> >
> > 8e9b2521b18a ("doc: Quick-Quiz answers are now inline")
> >
> > And I put the latest version at
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/boqun/linux.git leaf-node
> >
> > If you thought it's better, I could send a v3 ;-)
>
> 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.
>
> dbf18a2422e2 ("rcu: Introduce for_each_leaf_node_cpu()")
>
> The commit log needs a bit of wordsmithing.
>
> 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?
>
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?
>
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")
>
> This does look a bit nicer, but why the added blank lines?
> Are they really helping?
>
> 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.
>
> fd799f1ac7b7 ("rcu: Use for_each_leaf_node_cpu() in ->expmask iteration")
>
> Ditto on blank lines.
>
> 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.
>
> 69a1baedbf42 ("rcu: Use for_each_leaf_node_cpu() in force_qs_rnp()")
>
> Ditto again on blank lines.
>
> 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.
>
> 7b00e50e3efb ("rcu: Use for_each_leaf_node_cpu() in online CPU iteration")
>
> And another ditto on blank lines.
>
> This code executes once per CPU-hotplug operation, so again isn't
> at all performance critical.
>
> In short, if you are trying to sell this to me as a significant performance
> boost, I am not buying. The added WARN_ON_ONCE() looks quite dubious,
Yep, it won't help the performance a lot, but it
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.
>
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?
>
> Thanx, Paul
>
> > Regards,
> > Boqun
> >
> > ------------------------>8
> > From: Boqun Feng <boqun.feng@gmail.com>
> > 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()
> >
> > ->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.
> >
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> > kernel/rcu/tree.c | 12 +++++-------
> > 1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > 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 = leaf_node_cpu_bit(rnp, cpu);
> > - if ((rnp->qsmask & bit) != 0) {
> > - if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj))
> > - mask |= bit;
> > - }
> > - }
> > +
> > + for_each_leaf_node_cpu(rnp, rnp->qsmask, cpu)
> > + if (f(per_cpu_ptr(rsp->rda, cpu), isidle, maxj))
> > + mask |= leaf_node_cpu_bit(rnp, cpu);
> > +
> > if (mask != 0) {
> > /* Idle/offline CPUs, report (releases rnp->lock. */
> > rcu_report_qs_rnp(mask, rsp, rnp, rnp->gpnum, flags);
> > --
> > 2.10.2
> >
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2016-12-20 5:59 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-15 2:41 [RFC v2 0/5] rcu: Introduce for_each_leaf_node_cpu() Boqun Feng
2016-12-15 2:42 ` [RFC v2 1/5] " Boqun Feng
2016-12-15 11:43 ` Mark Rutland
2016-12-15 14:38 ` Boqun Feng
2016-12-15 15:10 ` Mark Rutland
2016-12-15 15:14 ` Boqun Feng
2016-12-15 15:21 ` [RFC v2.1 " Boqun Feng
2016-12-15 15:29 ` Mark Rutland
2016-12-15 2:42 ` [RFC v2 2/5] rcu: Use for_each_leaf_node_cpu() in RCU stall checking Boqun Feng
2016-12-15 2:42 ` [RFC v2 3/5] rcu: Use for_each_leaf_node_cpu() in ->expmask iteration Boqun Feng
2016-12-15 2:42 ` [RFC v2 4/5] rcu: Use for_each_leaf_node_cpu() in force_qs_rnp() Boqun Feng
2016-12-15 12:04 ` Mark Rutland
2016-12-15 14:42 ` Boqun Feng
2016-12-15 14:51 ` Colin Ian King
2016-12-19 15:15 ` Boqun Feng
2016-12-20 5:09 ` Paul E. McKenney
2016-12-20 5:59 ` Boqun Feng [this message]
2016-12-20 8:11 ` Boqun Feng
2016-12-20 15:32 ` Paul E. McKenney
2016-12-20 15:23 ` Paul E. McKenney
2016-12-21 2:34 ` Boqun Feng
2016-12-21 3:40 ` Paul E. McKenney
2016-12-21 4:18 ` Boqun Feng
2016-12-21 16:48 ` Paul E. McKenney
2016-12-22 1:08 ` Boqun Feng
2016-12-15 2:42 ` [RFC v2 5/5] rcu: Use for_each_leaf_node_cpu() in online CPU iteration Boqun Feng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161220055914.GB1316@tardis.cn.ibm.com \
--to=boqun.feng@gmail.com \
--cc=colin.king@canonical.com \
--cc=jiangshanlai@gmail.com \
--cc=josh@joshtriplett.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mathieu.desnoyers@efficios.com \
--cc=paulmck@linux.vnet.ibm.com \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).