[PATCH] rcu: Fix dynticks_nmi_nesting underflow check in rcu_is_cpu_rrupt_from_idle

[PATCH] rcu: Fix dynticks_nmi_nesting underflow check in rcu_is_cpu_rrupt_from_idle

[Neeraj Upadhyay]

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?

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?


Thanks
Neeraj

 kernel/rcu/tree.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index bc8b489..c3037cf 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -457,11 +457,10 @@ static int rcu_is_cpu_rrupt_from_idle(void)
 	/* Check for counter underflows */
 	RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nesting) < 0,
 			 "RCU dynticks_nesting counter underflow!");
-	RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
-			 "RCU dynticks_nmi_nesting counter underflow/zero!");
+	nesting = __this_cpu_read(rcu_data.dynticks_nmi_nesting);
+	RCU_LOCKDEP_WARN(nesting < 0, "RCU dynticks_nmi_nesting counter underflow!");
 
 	/* Are we at first interrupt nesting level? */
-	nesting = __this_cpu_read(rcu_data.dynticks_nmi_nesting);
 	if (nesting > 1)
 		return false;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] rcu: Fix dynticks_nmi_nesting underflow check in rcu_is_cpu_rrupt_from_idle

[Paul E. McKenney]

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.  But x86 has a number of idle
functions, and I might be missing it.  The "idle" kernel boot parameter
allows some ability to select them, and of course more profound (and maybe
more dangerous) changes can be made by changing select_idle_routine().

> 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.

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.

And another thing to try is to make the RCU_LOCKDEP_WARN() be WARN_ONCE()
or similar.  Can't hurt for testing purposes!

Of course, once Peter Zijlstra comes back from vacation, we should ask him.

And thank you for trying this out!

I am tempted to pull this in as is, given the current logical
inconsistency in the checks.  Thoughts?

							Thanx, Paul

> Thanks
> Neeraj
> 
>  kernel/rcu/tree.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index bc8b489..c3037cf 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -457,11 +457,10 @@ static int rcu_is_cpu_rrupt_from_idle(void)
>  	/* Check for counter underflows */
>  	RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nesting) < 0,
>  			 "RCU dynticks_nesting counter underflow!");
> -	RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> -			 "RCU dynticks_nmi_nesting counter underflow/zero!");
> +	nesting = __this_cpu_read(rcu_data.dynticks_nmi_nesting);
> +	RCU_LOCKDEP_WARN(nesting < 0, "RCU dynticks_nmi_nesting counter underflow!");
>  
>  	/* Are we at first interrupt nesting level? */
> -	nesting = __this_cpu_read(rcu_data.dynticks_nmi_nesting);
>  	if (nesting > 1)
>  		return false;
>  
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH] rcu: Fix dynticks_nmi_nesting underflow check in rcu_is_cpu_rrupt_from_idle

[Peter Zijlstra]

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)

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

Re: [PATCH] rcu: Fix dynticks_nmi_nesting underflow check in rcu_is_cpu_rrupt_from_idle

[Paul E. McKenney]

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