+ Saravanna, Rafael, Lina

On Mon, 31 Aug 2020 at 21:44, Paul E. McKenney paulmck@kernel.org wrote:

On Mon, Aug 31, 2020 at 12:02:31PM +0530, Naresh Kamboju wrote:

...

While booting linux mainline kernel on arm64 db410c this kernel warning noticed.

metadata: git branch: master git repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git git commit: f75aef392f869018f78cfedf3c320a6b3fcfda6b git describe: v5.9-rc3 make_kernelversion: 5.9.0-rc3 kernel-config: http://snapshots.linaro.org/openembedded/lkft/lkft/sumo/dragonboard-410c/lkf...

Boot log,

[ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd030] [ 0.000000] Linux version 5.9.0-rc3 (oe-user@oe-host) (aarch64-linaro-linux-gcc (GCC) 7.3.0, GNU ld (GNU Binutils) 2.30.0.20180208) #1 SMP PREEMPT Mon Aug 31 00:23:15 UTC 2020 [ 0.000000] Machine model: Qualcomm Technologies, Inc. APQ 8016 SBC <> [ 5.299090] sdhci: Secure Digital Host Controller Interface driver [ 5.299140] sdhci: Copyright(c) Pierre Ossman [ 5.304313] [ 5.307771] Synopsys Designware Multimedia Card Interface Driver [ 5.308588] ============================= [ 5.308593] WARNING: suspicious RCU usage [ 5.316628] sdhci-pltfm: SDHCI platform and OF driver helper [ 5.320052] 5.9.0-rc3 #1 Not tainted [ 5.320057] ----------------------------- [ 5.320063] /usr/src/kernel/include/trace/events/lock.h:37 suspicious rcu_dereference_check() usage! [ 5.320068] [ 5.320068] other info that might help us debug this: [ 5.320068] [ 5.320074] [ 5.320074] rcu_scheduler_active = 2, debug_locks = 1 [ 5.320078] RCU used illegally from extended quiescent state! [ 5.320084] no locks held by swapper/0/0. [ 5.320089] [ 5.320089] stack backtrace: [ 5.320098] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc3 #1 [ 5.346354] sdhci_msm 7864900.sdhci: Got CD GPIO [ 5.346446] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) [ 5.346452] Call trace: [ 5.346463] dump_backtrace+0x0/0x1f8 [ 5.346471] show_stack+0x2c/0x38 [ 5.346480] dump_stack+0xec/0x15c [ 5.346490] lockdep_rcu_suspicious+0xd4/0xf8 [ 5.346499] lock_acquire+0x3d0/0x440 [ 5.346510] _raw_spin_lock_irqsave+0x80/0xb0 [ 5.413118] __pm_runtime_suspend+0x34/0x1d0 [ 5.417457] psci_enter_domain_idle_state+0x4c/0xb0 [ 5.421795] cpuidle_enter_state+0xc8/0x610 [ 5.426392] cpuidle_enter+0x3c/0x50 [ 5.430561] call_cpuidle+0x44/0x80 [ 5.434378] do_idle+0x240/0x2a0

RCU ignores CPUs in the idle loop, which means that you cannot use rcu_read_lock() from the idle loop without use of something like RCU_NONIDLE(). If this is due to event tracing, you should use the _rcuidle() variant of the event trace statement.

In the runtime suspend path, the runtime PM core calls device_links_read_lock() - if the device in question has any links to suppliers (to allow them to be suspended too).

device_links_read_lock() calls srcu_read_lock().

It turns out that the device in question (the CPU device that is attached to genpd) didn't have any links before - but that seems to have changed, due to the work done by Saravana (links become created on a per resource basis, parsed from DT during boot).

Note also that Peter Zijlstra (CCed) is working to shrink the portion of the idle loop that RCU ignores. Not sure that it covers your case, but it is worth checking.

Thanks for letting me know. Let's see what Peter thinks about this then.

Apologize for my ignorance, but from a cpuidle point of view, what does it mean using RCU_NONIDLE()? I guess we should avoid RCU_NONIDLE on bigger code paths?

I could add RCU_NONIDLE for the calls to pm_runtime_put_sync_suspend() and pm_runtime_get_sync() in psci_enter_domain_idle_state(). Perhaps that's the easiest approach, at least to start with.

Or do you have any other ideas?

[...]

Kind regards Uffe

On Tue, Sep 01, 2020 at 09:44:17AM -0600, Lina Iyer wrote:

...

...

...

...

...

I could add RCU_NONIDLE for the calls to pm_runtime_put_sync_suspend() and pm_runtime_get_sync() in psci_enter_domain_idle_state(). Perhaps that's the easiest approach, at least to start with.

I think this would be nice. This should also cover the case, where PM domain power off notification callbacks call trace function internally. Right?

That's just more crap for me to clean up later :-(

trace_*_rcuidle() and RCU_NONIDLE() need to die, not proliferate.

On Tue, Sep 01, 2020 at 09:13:40AM -0700, Paul E. McKenney wrote:

On Tue, Sep 01, 2020 at 05:50:14PM +0200, peterz@infradead.org wrote:

...

On Tue, Sep 01, 2020 at 09:44:17AM -0600, Lina Iyer wrote:

...

...

...

...

> > I could add RCU_NONIDLE for the calls to pm_runtime_put_sync_suspend() > > and pm_runtime_get_sync() in psci_enter_domain_idle_state(). Perhaps > > that's the easiest approach, at least to start with.

...

I think this would be nice. This should also cover the case, where PM domain power off notification callbacks call trace function internally. Right?

That's just more crap for me to clean up later :-(

trace_*_rcuidle() and RCU_NONIDLE() need to die, not proliferate.

Moving the idle-entry boundary further in is good in any number of ways. But experience indicates that no matter how far you move it, there will be something complex further in. Unless you are pushing it all the way into all the arch-specific code down as far as it can possibly go?

Not all; the simple cpuidle drivers should be good already. The more complicated ones need some help.

The patch provided earlier:

https://lkml.kernel.org/r/20200901104206.GU1362448@hirez.programming.kicks-a...

should allow the complicated drivers to take over and DTRT.

On Wed, Sep 02, 2020 at 09:03:37AM +0200, Ulf Hansson wrote:

Lots of cpuidle drivers are using CPU_PM notifiers (grep for cpu_pm_enter and you will see) from their idlestates ->enter() callbacks. And for those we are already calling rcu_irq_enter_irqson|off() in cpu_pm_notify() when firing them.

Yeah, that particular trainwreck is on my todo list already ... then again, that list is forever overflowing.

I'm thinking cpu_pm_unregister_notifier() is not a common thing? The few I looked at seem to suggest 'never' is a good approximation.

It would be fairly trivial to replace the atomic_notifier usage with a raw_notifier a lock and either stop-machine or IPIs. Better still would be if we can get rid of it entirely, but I can't tell in a hurry if that is possible.

On Thu, Sep 03, 2020 at 04:36:35PM +0200, Ulf Hansson wrote:

On Thu, 3 Sep 2020 at 15:53, peterz@infradead.org wrote:

...

static int cpu_pm_notify(enum cpu_pm_event event) { int ret;

•   lockdep_assert_irqs_disabled();
  

Nitpick, maybe the lockdep should be moved to a separate patch.

Well, the unregister relies on IRQs being disabled here, so I figured asserting this was a good thing ;-)

Starting the audit below, this might not in fact be true, which then invalidates the unregister implementation. In particular the notifier in arch/arm/kernel/hw_breakpoint.c seems to unconditionally enable IRQs.

...

•   ret = raw_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL);
  

Converting to raw_notifiers seems reasonable - if we need to avoid the RCU usage.

My point is, I wonder about if the notifier callbacks themselves are safe from RCU usage. For example, I would not be surprised if tracing is happening behind them.

A bunch of them seem to call into the clk domain stuff, and I think there's tracepoints in that.

Moreover, I am not sure that we really need to prevent and limit tracing from happening. Instead we could push rcu_idle_enter|exit() further down to the arch specific code in the cpuidle drivers, as you kind of all proposed earlier.

Well, at some point the CPU is in a really dodgy state, ISTR there being ARM platforms where you have to manually leave the cache coherency fabric and all sorts of insanity. There should be a definite cut-off on tracing before that.

Also, what is the point of all this clock and power domain callbacks, if not to put the CPU into an extremely low power state, surely you want to limit the amount of code that's ran when the CPU is in such a state.

In this way, we can step by step, move to a new "version" of cpu_pm_enter() that doesn't have to deal with rcu_irq_enter_irqson(), because RCU hasn't been pushed to idle yet.

That should be easy enough to audit. The thing is that mainline is now generating (debug) splats, and some people are upset with this.

If you're ok with ARM not being lockdep clean while this is being reworked I'm perfectly fine with that.

(There used to be a separate CONFIG for RCU-lockdep, but that seems to have been removed)

On Thu, 3 Sep 2020 at 17:08, peterz@infradead.org wrote:

On Thu, Sep 03, 2020 at 04:36:35PM +0200, Ulf Hansson wrote:

...

On Thu, 3 Sep 2020 at 15:53, peterz@infradead.org wrote:

...

static int cpu_pm_notify(enum cpu_pm_event event) { int ret;

•   lockdep_assert_irqs_disabled();
  

Nitpick, maybe the lockdep should be moved to a separate patch.

Well, the unregister relies on IRQs being disabled here, so I figured asserting this was a good thing ;-)

Okay, make sense then.

Starting the audit below, this might not in fact be true, which then invalidates the unregister implementation. In particular the notifier in arch/arm/kernel/hw_breakpoint.c seems to unconditionally enable IRQs.

I see.

...

...

•   ret = raw_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL);
  

Converting to raw_notifiers seems reasonable - if we need to avoid the RCU usage.

My point is, I wonder about if the notifier callbacks themselves are safe from RCU usage. For example, I would not be surprised if tracing is happening behind them.

A bunch of them seem to call into the clk domain stuff, and I think there's tracepoints in that.

...

Moreover, I am not sure that we really need to prevent and limit tracing from happening. Instead we could push rcu_idle_enter|exit() further down to the arch specific code in the cpuidle drivers, as you kind of all proposed earlier.

Well, at some point the CPU is in a really dodgy state, ISTR there being ARM platforms where you have to manually leave the cache coherency fabric and all sorts of insanity. There should be a definite cut-off on tracing before that.

That's probably the case for some platforms, but I don't see why we need to make everybody "suffer".

Also, what is the point of all this clock and power domain callbacks, if not to put the CPU into an extremely low power state, surely you want to limit the amount of code that's ran when the CPU is in such a state.

...

In this way, we can step by step, move to a new "version" of cpu_pm_enter() that doesn't have to deal with rcu_irq_enter_irqson(), because RCU hasn't been pushed to idle yet.

That should be easy enough to audit. The thing is that mainline is now generating (debug) splats, and some people are upset with this.

If you're ok with ARM not being lockdep clean while this is being reworked I'm perfectly fine with that.

I think the splats can easily be fixed. Short term.

Adding RCU_NONIDLE (or similar) around pm_runtime calls in psci_enter_domain_idle_state() does the trick. I have a patch for that, it's tested and ready. Let me send it out.

Perhaps we should just accept that this is needed, as to allow us to move step by step into a better situation, while also avoiding the current splats.

(There used to be a separate CONFIG for RCU-lockdep, but that seems to have been removed)

I don't think that would help. Infrastructure for testing will just enable that Kconfig anyway still complains to us.

Kind regards Uffe

On Tue, Sep 01, 2020 at 08:46:54AM +0200, Ulf Hansson wrote:

• Saravanna, Rafael, Lina

On Mon, 31 Aug 2020 at 21:44, Paul E. McKenney paulmck@kernel.org wrote:

...

On Mon, Aug 31, 2020 at 12:02:31PM +0530, Naresh Kamboju wrote:

...

While booting linux mainline kernel on arm64 db410c this kernel warning noticed.

metadata: git branch: master git repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git git commit: f75aef392f869018f78cfedf3c320a6b3fcfda6b git describe: v5.9-rc3 make_kernelversion: 5.9.0-rc3 kernel-config: http://snapshots.linaro.org/openembedded/lkft/lkft/sumo/dragonboard-410c/lkf...

Boot log,

[ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd030] [ 0.000000] Linux version 5.9.0-rc3 (oe-user@oe-host) (aarch64-linaro-linux-gcc (GCC) 7.3.0, GNU ld (GNU Binutils) 2.30.0.20180208) #1 SMP PREEMPT Mon Aug 31 00:23:15 UTC 2020 [ 0.000000] Machine model: Qualcomm Technologies, Inc. APQ 8016 SBC <> [ 5.299090] sdhci: Secure Digital Host Controller Interface driver [ 5.299140] sdhci: Copyright(c) Pierre Ossman [ 5.304313] [ 5.307771] Synopsys Designware Multimedia Card Interface Driver [ 5.308588] ============================= [ 5.308593] WARNING: suspicious RCU usage [ 5.316628] sdhci-pltfm: SDHCI platform and OF driver helper [ 5.320052] 5.9.0-rc3 #1 Not tainted [ 5.320057] ----------------------------- [ 5.320063] /usr/src/kernel/include/trace/events/lock.h:37 suspicious rcu_dereference_check() usage! [ 5.320068] [ 5.320068] other info that might help us debug this: [ 5.320068] [ 5.320074] [ 5.320074] rcu_scheduler_active = 2, debug_locks = 1 [ 5.320078] RCU used illegally from extended quiescent state! [ 5.320084] no locks held by swapper/0/0. [ 5.320089] [ 5.320089] stack backtrace: [ 5.320098] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc3 #1 [ 5.346354] sdhci_msm 7864900.sdhci: Got CD GPIO [ 5.346446] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) [ 5.346452] Call trace: [ 5.346463] dump_backtrace+0x0/0x1f8 [ 5.346471] show_stack+0x2c/0x38 [ 5.346480] dump_stack+0xec/0x15c [ 5.346490] lockdep_rcu_suspicious+0xd4/0xf8 [ 5.346499] lock_acquire+0x3d0/0x440 [ 5.346510] _raw_spin_lock_irqsave+0x80/0xb0 [ 5.413118] __pm_runtime_suspend+0x34/0x1d0 [ 5.417457] psci_enter_domain_idle_state+0x4c/0xb0 [ 5.421795] cpuidle_enter_state+0xc8/0x610 [ 5.426392] cpuidle_enter+0x3c/0x50 [ 5.430561] call_cpuidle+0x44/0x80 [ 5.434378] do_idle+0x240/0x2a0

RCU ignores CPUs in the idle loop, which means that you cannot use rcu_read_lock() from the idle loop without use of something like RCU_NONIDLE(). If this is due to event tracing, you should use the _rcuidle() variant of the event trace statement.

In the runtime suspend path, the runtime PM core calls device_links_read_lock() - if the device in question has any links to suppliers (to allow them to be suspended too).

device_links_read_lock() calls srcu_read_lock().

Except that it is perfectly legal to invoke srcu_read_lock() from the idle loop. The problem is instead rcu_read_lock() and similar.

It turns out that the device in question (the CPU device that is attached to genpd) didn't have any links before - but that seems to have changed, due to the work done by Saravana (links become created on a per resource basis, parsed from DT during boot).

...

Note also that Peter Zijlstra (CCed) is working to shrink the portion of the idle loop that RCU ignores. Not sure that it covers your case, but it is worth checking.

Thanks for letting me know. Let's see what Peter thinks about this then.

Apologize for my ignorance, but from a cpuidle point of view, what does it mean using RCU_NONIDLE()? I guess we should avoid RCU_NONIDLE on bigger code paths?

It means that as far as RCU (and only RCU) is concerned there is an exit from idle state for just long enough to execute RCU_NONIDLE()'s argument. This involves an atomic operation on both entry to and exit from RCU_NONIDLE(), which in most cases won't be noticeable. But in some cases you might (for example) want to enclose a loop in RCU_NONIDLE() rather than doing RCU_NONIDLE() on each pass through the loop.

I could add RCU_NONIDLE for the calls to pm_runtime_put_sync_suspend() and pm_runtime_get_sync() in psci_enter_domain_idle_state(). Perhaps that's the easiest approach, at least to start with.

Or do you have any other ideas?

Here is the list, though it is early in the morning here:

1. RCU_NONIDLE().

2. Peter's patch, if it turns out to hoist your code out of what RCU considers to be the idle loop.

3. If the problem is trace events, use the _rcuidle() variant of the trace event. Instead of trace_blah(), use trace_blah_rcuidle().

4. Switch from RCU (as in rcu_read_lock()) to SRCU (as in srcu_read_lock()).

5. Take Peter's patch a step further, moving the rcu_idle_enter() and rcu_idle_exit() calls as needed. But please keep in mind that these two functions require that irqs be disabled by their callers.

6. If RCU_NONIDLE() in inconvenient due to early exits and such, you could use the rcu_irq_enter_irqson() and rcu_irq_exit_irqson() functions that it calls.

Do any of those help?

Thanx, Paul

On Wed, Sep 02, 2020 at 08:49:11AM +0200, Ulf Hansson wrote:

On Tue, 1 Sep 2020 at 17:00, Paul E. McKenney paulmck@kernel.org wrote:

[ . . . ]

...

Here is the list, though it is early in the morning here:

1.  RCU_NONIDLE().
   
   

2.  Peter's patch, if it turns out to hoist your code out of what
    RCU considers to be the idle loop.
   
   

3.  If the problem is trace events, use the _rcuidle() variant of the
    trace event.  Instead of trace_blah(), use trace_blah_rcuidle().
   
   

4.  Switch from RCU (as in rcu_read_lock()) to SRCU (as in
    srcu_read_lock()).
   
   

5.  Take Peter's patch a step further, moving the rcu_idle_enter()
    and rcu_idle_exit() calls as needed.  But please keep in mind
    that these two functions require that irqs be disabled by their
    callers.
   
   

6.  If RCU_NONIDLE() in inconvenient due to early exits and such,
    you could use the rcu_irq_enter_irqson() and rcu_irq_exit_irqson()
    functions that it calls.
   
   

Do any of those help?

Yes, they will, in one way or the other. Thanks for providing me with all the available options.

BTW, I still don't get what good rcu_idle_enter|exit() does, but I am assuming those need to be called at some point before the CPU goes to sleep.

These functions allow RCU to leave idle CPUs undisturbed. If they were not invoked, RCU would periodically IPI idle CPUs to verify that there were no RCU readers running on them. This would be quite bad for battery lifetime, among other things. So the call to rcu_idle_enter() tells RCU that it may safely completely ignore this CPU until its next call to rcu_idle_exit().

Thanx, Paul

On Wed, Sep 02, 2020 at 06:07:05PM +0200, Ulf Hansson wrote:

On Wed, 2 Sep 2020 at 15:52, Paul E. McKenney paulmck@kernel.org wrote:

...

On Wed, Sep 02, 2020 at 08:49:11AM +0200, Ulf Hansson wrote:

...

On Tue, 1 Sep 2020 at 17:00, Paul E. McKenney paulmck@kernel.org wrote:

[ . . . ]

...

...

Here is the list, though it is early in the morning here:

1.  RCU_NONIDLE().
   
   

2.  Peter's patch, if it turns out to hoist your code out of what
    RCU considers to be the idle loop.
   
   

3.  If the problem is trace events, use the _rcuidle() variant of the
    trace event.  Instead of trace_blah(), use trace_blah_rcuidle().
   
   

4.  Switch from RCU (as in rcu_read_lock()) to SRCU (as in
    srcu_read_lock()).
   
   

5.  Take Peter's patch a step further, moving the rcu_idle_enter()
    and rcu_idle_exit() calls as needed.  But please keep in mind
    that these two functions require that irqs be disabled by their
    callers.
   
   

6.  If RCU_NONIDLE() in inconvenient due to early exits and such,
    you could use the rcu_irq_enter_irqson() and rcu_irq_exit_irqson()
    functions that it calls.
   
   

Do any of those help?

Yes, they will, in one way or the other. Thanks for providing me with all the available options.

BTW, I still don't get what good rcu_idle_enter|exit() does, but I am assuming those need to be called at some point before the CPU goes to sleep.

These functions allow RCU to leave idle CPUs undisturbed. If they were not invoked, RCU would periodically IPI idle CPUs to verify that there were no RCU readers running on them. This would be quite bad for battery lifetime, among other things. So the call to rcu_idle_enter() tells RCU that it may safely completely ignore this CPU until its next call to rcu_idle_exit().

Alright, thanks for explaining this, much appreciated.

So in one way, we would also like to call rcu_idle_enter(), as soon as we know there is no need for the RCU to be active. To prevent unnecessary IPIs I mean. :-)

Well, the IPIs don't happen until the better part of a second into the grace period. So delaying an rcu_idle_enter() a few microseconds, as Peter Zijlstra is proposing, is absolutely no problem whatsoever. And once the rcu_idle_enter() happens, the RCU grace-period kthread's next scan of the CPUs will see that this CPU needs to be ignored, so no more IPIs for it until it does the next rcu_idle_exit(), rcu_irq_enter(), or any of a number of other things that cause RCU to once again pay attention to that CPU.

Thanx, Paul