Re: [PATCH v2] cpu_pm: Make notifier chain use a raw spinlock

[Sebastian Andrzej Siewior]

On 2021-08-11 14:14:05 [+0100], Valentin Schneider wrote:
> Booting a recent PREEMPT_RT kernel (v5.14-rc5-rt8 with the previous version
> of this fix reverted) on my arm4 Juno leads to the idle task blocking on a
> sleeping spinlock down some notifier path:
> 
> [    5.163034] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:35
> [    5.163042] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 0, name: swapper/1
> [    5.163049] 1 lock held by swapper/1/0:
> [    5.163053] #0: ffff8000120950e8 (cpu_pm_notifier_chain.lock){+.+.}-{2:2}, at: atomic_notifier_call_chain_robust (kernel/notifier.c:186)
> [    5.163133] Preemption disabled at:
> [    5.163136] rt_mutex_slowunlock (kernel/locking/rtmutex.c:1242)
> [    5.163148] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.14.0-rc5-rt8-00001-ga7cd9160688d #202
> [    5.163158] Hardware name: ARM Juno development board (r0) (DT)
> [    5.163162] Call trace:
> [    5.163165] dump_backtrace (arch/arm64/kernel/stacktrace.c:161)
> [    5.163177] show_stack (arch/arm64/kernel/stacktrace.c:217)
> [    5.163187] dump_stack_lvl (lib/dump_stack.c:106)
> [    5.163195] dump_stack (lib/dump_stack.c:113)
> [    5.163202] ___might_sleep (kernel/sched/core.c:9286)
> [    5.163210] rt_spin_lock (kernel/locking/rtmutex.c:1668 (discriminator 4) kernel/locking/spinlock_rt.c:30 (discriminator 4) kernel/locking/spinlock_rt.c:36 (discriminator 4) kernel/locking/spinlock_rt.c:44 (discriminator 4))
> [    5.163216] atomic_notifier_call_chain_robust (kernel/notifier.c:186)
> [    5.163225] cpu_pm_notify_robust (kernel/cpu_pm.c:39)
> [    5.163233] cpu_pm_enter (kernel/cpu_pm.c:94)
> [    5.163239] psci_enter_idle_state (drivers/cpuidle/cpuidle-psci.c:53 drivers/cpuidle/cpuidle-psci.c:154)
> [    5.163250] cpuidle_enter_state (drivers/cpuidle/cpuidle.c:238)
> [    5.163258] cpuidle_enter (drivers/cpuidle/cpuidle.c:353)
> [    5.163266] call_cpuidle (kernel/sched/idle.c:159)
> [    5.163272] do_idle (kernel/sched/idle.c:243 kernel/sched/idle.c:306)
> [    5.163277] cpu_startup_entry (kernel/sched/idle.c:402 (discriminator 1))
> [    5.163285] secondary_start_kernel (arch/arm64/kernel/smp.c:266)
> [    5.163294] __secondary_switched (arch/arm64/kernel/head.S:661)

I would shrink that part above. The important part is that the CPU-idle
code runs with disabled interrupts. Then cpu_pm_notify_robust() invokes
the notifier which requires to acquire the spinlock_t. On PREEMPT_RT the
spinlock_t becomes a sleeping spinlock and must not be acquired with
disabled interrupts.

> Making *all* atomic_notifiers use a raw_spinlock is too big of a hammer, as
> only notifications issued by the idle task are problematic.
> 
> Special-case cpu_pm_notifier_chain by kludging a raw_notifier and
> raw_spinlock together, matching the atomic_notifier behavior with a
> raw_spinlock.
> 
> Fixes: 70d932985757 ("notifier: Fix broken error handling pattern")
> Signed-off-by: Valentin Schneider <valentin.schneider@xxxxxxx>
…
>  kernel/cpu_pm.c | 49 +++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
> index f7e1d0eccdbc..707b8ace9fc7 100644
> --- a/kernel/cpu_pm.c
> +++ b/kernel/cpu_pm.c
…
>  #include <linux/spinlock.h>
>  #include <linux/syscore_ops.h>
>  
> -static ATOMIC_NOTIFIER_HEAD(cpu_pm_notifier_chain);
> +/*
> + * atomic_notifiers use a regular spinlock, but notifications for this chain
> + * will be issued by the idle task which cannot block.

Maybe + a few details and make it more explicit

 * atomic_notifiers use a spinlock_t, but notifications for this chain
 * will be issued by the idle task with disabled interrupts which cannot
 * block on PREEMPT_RT.

?

…
> @@ -33,10 +45,13 @@ static int cpu_pm_notify(enum cpu_pm_event event)
>  
>  static int cpu_pm_notify_robust(enum cpu_pm_event event_up, enum cpu_pm_event event_down)
>  {
> +	unsigned long flags;
>  	int ret;
>  
>  	rcu_irq_enter_irqson();
> -	ret = atomic_notifier_call_chain_robust(&cpu_pm_notifier_chain, event_up, event_down, NULL);

could we get rid of atomic_notifier_call_chain_robust() now that we have
zero users?

> +	raw_spin_lock_irqsave(&cpu_pm_notifier.lock, flags);
> +	ret = raw_notifier_call_chain_robust(&cpu_pm_notifier.chain, event_up, event_down, NULL);
> +	raw_spin_unlock_irqrestore(&cpu_pm_notifier.lock, flags);
>  	rcu_irq_exit_irqson();
>  
>  	return notifier_to_errno(ret);

Sebastian