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@xxxxxxxxxxxxxx>
> > ---
> >
> > 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)
> > 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..
> 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.
> 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).
