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

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

[Valentin Schneider]

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)

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@arm.com>
---
v1 -> v2:
  Dropped the treewide change of atomic_notifier and special-cased
  cpu_pm_notifier_chain. This is done in cpu_pm itself rather than as a new
  notifier interface since this is caused by the idle task being "special",
  and AFAIA we don't want to encourage more widespread use.
---
 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
@@ -13,19 +13,31 @@
 #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.
+ */
+static struct {
+	struct raw_notifier_head chain;
+	raw_spinlock_t lock;
+} cpu_pm_notifier = {
+	.chain = RAW_NOTIFIER_INIT(cpu_pm_notifier.chain),
+	.lock  = __RAW_SPIN_LOCK_UNLOCKED(cpu_pm_notifier.lock),
+};
 
 static int cpu_pm_notify(enum cpu_pm_event event)
 {
 	int ret;
 
 	/*
-	 * atomic_notifier_call_chain has a RCU read critical section, which
-	 * could be disfunctional in cpu idle. Copy RCU_NONIDLE code to let
-	 * RCU know this.
+	 * This introduces a RCU read critical section, which could be
+	 * disfunctional in cpu idle. Copy RCU_NONIDLE code to let RCU know
+	 * this.
 	 */
 	rcu_irq_enter_irqson();
-	ret = atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL);
+	rcu_read_lock();
+	ret = raw_notifier_call_chain(&cpu_pm_notifier.chain, event, NULL);
+	rcu_read_unlock();
 	rcu_irq_exit_irqson();
 
 	return notifier_to_errno(ret);
@@ -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);
+	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);
@@ -49,12 +64,17 @@ static int cpu_pm_notify_robust(enum cpu_pm_event event_up, enum cpu_pm_event ev
  * Add a driver to a list of drivers that are notified about
  * CPU and CPU cluster low power entry and exit.
  *
- * This function may sleep, and has the same return conditions as
- * raw_notifier_chain_register.
+ * This function has the same return conditions as raw_notifier_chain_register.
  */
 int cpu_pm_register_notifier(struct notifier_block *nb)
 {
-	return atomic_notifier_chain_register(&cpu_pm_notifier_chain, nb);
+	unsigned long flags;
+	int ret;
+
+	raw_spin_lock_irqsave(&cpu_pm_notifier.lock, flags);
+	ret = raw_notifier_chain_register(&cpu_pm_notifier.chain, nb);
+	raw_spin_unlock_irqrestore(&cpu_pm_notifier.lock, flags);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(cpu_pm_register_notifier);
 
@@ -64,12 +84,17 @@ EXPORT_SYMBOL_GPL(cpu_pm_register_notifier);
  *
  * Remove a driver from the CPU PM notifier list.
  *
- * This function may sleep, and has the same return conditions as
- * raw_notifier_chain_unregister.
+ * This function has the same return conditions as raw_notifier_chain_unregister.
  */
 int cpu_pm_unregister_notifier(struct notifier_block *nb)
 {
-	return atomic_notifier_chain_unregister(&cpu_pm_notifier_chain, nb);
+	unsigned long flags;
+	int ret;
+
+	raw_spin_lock_irqsave(&cpu_pm_notifier.lock, flags);
+	ret = raw_notifier_chain_unregister(&cpu_pm_notifier.chain, nb);
+	raw_spin_unlock_irqrestore(&cpu_pm_notifier.lock, flags);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(cpu_pm_unregister_notifier);
 
-- 
2.25.1

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@arm.com>
…
>  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

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

[Valentin Schneider]

On 11/08/21 15:52, Sebastian Andrzej Siewior wrote:
> 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.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.

Noted, I'll pluck the warning out.

>> +/*
>> + * 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.
>
> ?
>

More generally I'd say the idle task is never preemptible (as in
preempt_count > 0 at all times), so any notification issued by the idle
task itself cannot block. The fact those are also issued in an IRQ-off
region just further cements that.

> …
>> @@ -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?
>

No objections from my end, I'll add that in v3 and see if anyone complains.

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