From: "Paul E. McKenney" <paulmck@kernel.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Neeraj Upadhyay <neeraju@codeaurora.org>,
josh@joshtriplett.org, rostedt@goodmis.org,
mathieu.desnoyers@efficios.com, jiangshanlai@gmail.com,
joel@joelfernandes.org, rcu@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rcu: Fix dynticks_nmi_nesting underflow check in rcu_is_cpu_rrupt_from_idle
Date: Tue, 5 Jan 2021 09:14:50 -0800 [thread overview]
Message-ID: <20210105171450.GF17086@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <20210105134232.GI3040@hirez.programming.kicks-ass.net>
On Tue, Jan 05, 2021 at 02:42:32PM +0100, Peter Zijlstra wrote:
> On Wed, Dec 23, 2020 at 07:12:31AM -0800, Paul E. McKenney wrote:
> > On Wed, Dec 23, 2020 at 02:09:37PM +0530, Neeraj Upadhyay wrote:
> > > For the smp_call_function() optimization, where callbacks can run from
> > > idle context, in commit 806f04e9fd2c ("rcu: Allow for smp_call_function()
> > > running callbacks from idle"), an additional check is added in
> > > rcu_is_cpu_rrupt_from_idle(), for dynticks_nmi_nesting value being 0,
> > > for these smp_call_function() callbacks running from idle loop.
> > > However, this commit missed updating a preexisting underflow check
> > > of dynticks_nmi_nesting, which checks for a non zero positive value.
> > > Fix this warning and while at it, read the counter only once.
> > >
> > > Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
> > > ---
> > >
> > > Hi,
> > >
> > > I was not able to get this warning, with scftorture.
> > >
> > > RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> > > "RCU dynticks_nmi_nesting counter underflow/zero!");
> > >
> > > Not sure if idle loop smp_call_function() optimization is already present
> > > in mainline?
> >
> > Now that you mention it, I don't see it.
>
> kernel/sched/idle.c:do_idle() calls flush_smp_call_function_from_idle().
>
> (nothing x86 specific about it)
Got it, thank you!
The reason Neeraj was unable to trigger the problematic warning from
scftorture is that its smp_call_function() handlers do not invoke
rcu_is_cpu_rrupt_from_idle(). Without adding this to those handlers
(which would be a good change to make), the only way to trigger this is
for an expedited RCU grace period to IPI a CPU that goes idle while the
IPI is in flight, which is not the easiest thing to make happen.
> > > Another thing, which I am not sure of is, maybe lockdep gets disabled
> > > in the idle loop contexts, where rcu_is_cpu_rrupt_from_idle() is called?
> > > Was this the original intention, to keep the lockdep based
> > > RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0
> > > check separate from idle task context nesting value
> > > WARN_ON_ONCE(!nesting && !is_idle_task(current)) check?
> >
> > An easy way to test lockdep is to create a pair of locks, acquire them
> > in one order then release them both, and finally acquire them in the
> > other order and then release them both. If lockdep is configured and
> > enabled, it will complain.
>
> IIRC (and this is after not staring at the computer for 2 weeks) lockdep
> should work just fine in idle, except of course that RCU will be stopped
> so actually taking locks will scream bloody murder due to tracing etc..
Fair enough...
And I hope that the time off went well for you and yours!
> > The only reason I used RCU_LOCKDEP_WARN() was that people were complaining
> > to me about idle-entry overhead back at that time. So without lockdep,
> > there is zero overhead. Maybe people have become more tolerant of idle
> > delays, or perhaps they are not so worried about an extra check of a
> > cache-hot quantity.
>
> Not having checks also saves on $I and branches, in general I think
> having checks depend on DEBUG features, esp. those we don't really
> expect to trigger is still sane.
OK, so should we convert the WARN_ON_ONCE() to RCU_LOCKDEP_WARN() while
we are in the area?
> > I am tempted to pull this in as is, given the current logical
> > inconsistency in the checks. Thoughts?
>
> Patch looks ok, although I've seen compilers do CSE on
> __this_cpu_read() (on x86).
True, but the compilers might might have a harder time of this on other
architectures.
Thanx, Paul
prev parent reply other threads:[~2021-01-05 17:15 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-23 8:39 [PATCH] rcu: Fix dynticks_nmi_nesting underflow check in rcu_is_cpu_rrupt_from_idle Neeraj Upadhyay
2020-12-23 15:12 ` Paul E. McKenney
2021-01-05 13:42 ` Peter Zijlstra
2021-01-05 17:14 ` Paul E. McKenney [this message]
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=20210105171450.GF17086@paulmck-ThinkPad-P72 \
--to=paulmck@kernel.org \
--cc=jiangshanlai@gmail.com \
--cc=joel@joelfernandes.org \
--cc=josh@joshtriplett.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=neeraju@codeaurora.org \
--cc=peterz@infradead.org \
--cc=rcu@vger.kernel.org \
--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).