[PATCH] cpu: Make atomic callbacks run on UP with disabled interrupts

[PATCH] cpu: Make atomic callbacks run on UP with disabled interrupts

[Sebastian Andrzej Siewior]

On SMP callbacks in the "starting" range are invoked while the CPU is
brought up and interrupts are still disabled. Callbacks which are added
later ar invoked via the hotplug-thread on the target CPU and interrupts
are explicitly disabled.
In the UP case callbacks which are added later are invoked "directly"
without the thread. This is okay since there is just one CPU but with
enabled interrupts debug code, such as smp_processor_id(), will issue
warnings.

Disable interrupts before invoking the calback on UP if the state is
atomic and interrupts are expected to be disabled.
The "save" part is required because this is also invoked early in the
boot process while interrupts are disabled and must not be enabled.

Fixes: 06ddd17521bf1 ("sched/smp: Always define is_percpu_thread() and scheduler_ipi()")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/cpu.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index db9f6c539b28c..a6902646b4933 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -249,6 +249,14 @@ static int cpuhp_invoke_callback(unsigned int cpu, enum cpuhp_state state,
 	return ret;
 }
 
+/*
+ * The former STARTING/DYING states, ran with IRQs disabled and must not fail.
+ */
+static bool cpuhp_is_atomic_state(enum cpuhp_state state)
+{
+	return CPUHP_AP_IDLE_DEAD <= state && state < CPUHP_AP_ONLINE;
+}
+
 #ifdef CONFIG_SMP
 static bool cpuhp_is_ap_state(enum cpuhp_state state)
 {
@@ -271,14 +279,6 @@ static inline void complete_ap_thread(struct cpuhp_cpu_state *st, bool bringup)
 	complete(done);
 }
 
-/*
- * The former STARTING/DYING states, ran with IRQs disabled and must not fail.
- */
-static bool cpuhp_is_atomic_state(enum cpuhp_state state)
-{
-	return CPUHP_AP_IDLE_DEAD <= state && state < CPUHP_AP_ONLINE;
-}
-
 /* Synchronization state management */
 enum cpuhp_sync_state {
 	SYNC_STATE_DEAD,
@@ -2364,7 +2364,15 @@ static int cpuhp_issue_call(int cpu, enum cpuhp_state state, bool bringup,
 	else
 		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
 #else
-	ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
+	if (1) {
+		unsigned long flags = 0;
+
+		if (cpuhp_is_atomic_state(state))
+			local_irq_save(flags);
+		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
+		if (cpuhp_is_atomic_state(state))
+			local_irq_restore(flags);
+	}
 #endif
 	BUG_ON(ret && !bringup);
 	return ret;
-- 
2.51.0

Re: [PATCH] cpu: Make atomic callbacks run on UP with disabled interrupts

[Peter Zijlstra]

On Mon, Nov 03, 2025 at 01:03:54PM +0100, Sebastian Andrzej Siewior wrote:
> On SMP callbacks in the "starting" range are invoked while the CPU is
> brought up and interrupts are still disabled. Callbacks which are added
> later ar invoked via the hotplug-thread on the target CPU and interrupts
> are explicitly disabled.
> In the UP case callbacks which are added later are invoked "directly"
> without the thread. This is okay since there is just one CPU but with
> enabled interrupts debug code, such as smp_processor_id(), will issue
> warnings.
> 
> Disable interrupts before invoking the calback on UP if the state is
> atomic and interrupts are expected to be disabled.
> The "save" part is required because this is also invoked early in the
> boot process while interrupts are disabled and must not be enabled.
> 
> Fixes: 06ddd17521bf1 ("sched/smp: Always define is_percpu_thread() and scheduler_ipi()")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/cpu.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index db9f6c539b28c..a6902646b4933 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -249,6 +249,14 @@ static int cpuhp_invoke_callback(unsigned int cpu, enum cpuhp_state state,
>  	return ret;
>  }
>  
> +/*
> + * The former STARTING/DYING states, ran with IRQs disabled and must not fail.
> + */
> +static bool cpuhp_is_atomic_state(enum cpuhp_state state)
> +{
> +	return CPUHP_AP_IDLE_DEAD <= state && state < CPUHP_AP_ONLINE;
> +}
> +
>  #ifdef CONFIG_SMP
>  static bool cpuhp_is_ap_state(enum cpuhp_state state)
>  {
> @@ -271,14 +279,6 @@ static inline void complete_ap_thread(struct cpuhp_cpu_state *st, bool bringup)
>  	complete(done);
>  }
>  
> -/*
> - * The former STARTING/DYING states, ran with IRQs disabled and must not fail.
> - */
> -static bool cpuhp_is_atomic_state(enum cpuhp_state state)
> -{
> -	return CPUHP_AP_IDLE_DEAD <= state && state < CPUHP_AP_ONLINE;
> -}
> -
>  /* Synchronization state management */
>  enum cpuhp_sync_state {
>  	SYNC_STATE_DEAD,
> @@ -2364,7 +2364,15 @@ static int cpuhp_issue_call(int cpu, enum cpuhp_state state, bool bringup,
>  	else
>  		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
>  #else
> -	ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
> +	if (1) {
> +		unsigned long flags = 0;
> +
> +		if (cpuhp_is_atomic_state(state))
> +			local_irq_save(flags);
> +		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
> +		if (cpuhp_is_atomic_state(state))
> +			local_irq_restore(flags);
> +	}

How about:

	if (cpuhp_is_atomic_state(state)) {
		guard(irqsave)();
		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
		/*
		 * STARTING/DYING must not fail!
		 */
		WARN_ON_ONCE(ret);
	} else {
		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
	}

which is a little more like cpuhp_thread_fun()


>  #endif
>  	BUG_ON(ret && !bringup);
>  	return ret;
> -- 
> 2.51.0
> 

Re: [PATCH] cpu: Make atomic callbacks run on UP with disabled interrupts

[Sebastian Andrzej Siewior]

On 2025-11-03 13:42:54 [+0100], Peter Zijlstra wrote:
> How about:
> 
> 	if (cpuhp_is_atomic_state(state)) {
> 		guard(irqsave)();
> 		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
> 		/*
> 		 * STARTING/DYING must not fail!
> 		 */
> 		WARN_ON_ONCE(ret);
> 	} else {
> 		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
> 	}
> 
> which is a little more like cpuhp_thread_fun()

very nice indeed. What about WARN_ON_ONCE(ret && bringup) given the
BUG_ON(ret && !bringup) below?

> 

Sebastian

[PATCH v2] cpu: Make atomic callbacks run on UP with disabled interrupts

[Sebastian Andrzej Siewior]

On SMP callbacks in the "starting" range are invoked while the CPU is
brought up and interrupts are still disabled. Callbacks which are added
later ar invoked via the hotplug-thread on the target CPU and interrupts
are explicitly disabled.
In the UP case callbacks which are added later are invoked "directly"
without the thread. This is okay since there is just one CPU but with
enabled interrupts debug code, such as smp_processor_id(), will issue
warnings.

Disable interrupts before invoking the calback on UP if the state is
atomic and interrupts are expected to be disabled.
The "save" part is required because this is also invoked early in the
boot process while interrupts are disabled and must not be enabled. The
warnings aligns the function with cpuhp_thread_fun().

Fixes: 06ddd17521bf1 ("sched/smp: Always define is_percpu_thread() and scheduler_ipi()")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2:
 - guard() syntax as suggested by PeterZ plus the warning from
   cpuhp_thread_fun(). This will lead to a warning + bug in the bringup
   case which also happens with the thread on SMP.

 kernel/cpu.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index db9f6c539b28c..8ab69a891d7ae 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -249,6 +249,14 @@ static int cpuhp_invoke_callback(unsigned int cpu, enum cpuhp_state state,
 	return ret;
 }
 
+/*
+ * The former STARTING/DYING states, ran with IRQs disabled and must not fail.
+ */
+static bool cpuhp_is_atomic_state(enum cpuhp_state state)
+{
+	return CPUHP_AP_IDLE_DEAD <= state && state < CPUHP_AP_ONLINE;
+}
+
 #ifdef CONFIG_SMP
 static bool cpuhp_is_ap_state(enum cpuhp_state state)
 {
@@ -271,14 +279,6 @@ static inline void complete_ap_thread(struct cpuhp_cpu_state *st, bool bringup)
 	complete(done);
 }
 
-/*
- * The former STARTING/DYING states, ran with IRQs disabled and must not fail.
- */
-static bool cpuhp_is_atomic_state(enum cpuhp_state state)
-{
-	return CPUHP_AP_IDLE_DEAD <= state && state < CPUHP_AP_ONLINE;
-}
-
 /* Synchronization state management */
 enum cpuhp_sync_state {
 	SYNC_STATE_DEAD,
@@ -2364,7 +2364,16 @@ static int cpuhp_issue_call(int cpu, enum cpuhp_state state, bool bringup,
 	else
 		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
 #else
-	ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
+	if (cpuhp_is_atomic_state(state)) {
+		guard(irqsave)();
+		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
+		/*
+		 * STARTING/DYING must not fail!
+		 */
+		WARN_ON_ONCE(ret);
+	} else {
+		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
+	}
 #endif
 	BUG_ON(ret && !bringup);
 	return ret;
-- 
2.51.0

Re: [PATCH] cpu: Make atomic callbacks run on UP with disabled interrupts

[Peter Zijlstra]

On Mon, Nov 03, 2025 at 02:10:51PM +0100, Sebastian Andrzej Siewior wrote:
> On 2025-11-03 13:42:54 [+0100], Peter Zijlstra wrote:
> > How about:
> > 
> > 	if (cpuhp_is_atomic_state(state)) {
> > 		guard(irqsave)();
> > 		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
> > 		/*
> > 		 * STARTING/DYING must not fail!
> > 		 */
> > 		WARN_ON_ONCE(ret);
> > 	} else {
> > 		ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
> > 	}
> > 
> > which is a little more like cpuhp_thread_fun()
> 
> very nice indeed. What about WARN_ON_ONCE(ret && bringup) given the
> BUG_ON(ret && !bringup) below?

That would be confusing to read, the condition here is any failure. And
yes, this way you'll get a WARN and then a BUG, but meh :-)

Re: [PATCH v2] cpu: Make atomic callbacks run on UP with disabled interrupts

[Peter Zijlstra]

On Mon, Nov 03, 2025 at 02:28:20PM +0100, Sebastian Andrzej Siewior wrote:
> On SMP callbacks in the "starting" range are invoked while the CPU is
> brought up and interrupts are still disabled. Callbacks which are added
> later ar invoked via the hotplug-thread on the target CPU and interrupts
> are explicitly disabled.
> In the UP case callbacks which are added later are invoked "directly"
> without the thread. This is okay since there is just one CPU but with
> enabled interrupts debug code, such as smp_processor_id(), will issue
> warnings.
> 
> Disable interrupts before invoking the calback on UP if the state is
> atomic and interrupts are expected to be disabled.
> The "save" part is required because this is also invoked early in the
> boot process while interrupts are disabled and must not be enabled. The
> warnings aligns the function with cpuhp_thread_fun().
> 
> Fixes: 06ddd17521bf1 ("sched/smp: Always define is_percpu_thread() and scheduler_ipi()")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Did we want me to merge this into sched/urgent or what was the
hope/intention for this patch.

Re: [PATCH v2] cpu: Make atomic callbacks run on UP with disabled interrupts

[Sebastian Andrzej Siewior]

On 2025-11-03 15:10:58 [+0100], Peter Zijlstra wrote:
> On Mon, Nov 03, 2025 at 02:28:20PM +0100, Sebastian Andrzej Siewior wrote:
> > On SMP callbacks in the "starting" range are invoked while the CPU is
> > brought up and interrupts are still disabled. Callbacks which are added
> > later ar invoked via the hotplug-thread on the target CPU and interrupts
> > are explicitly disabled.
> > In the UP case callbacks which are added later are invoked "directly"
> > without the thread. This is okay since there is just one CPU but with
> > enabled interrupts debug code, such as smp_processor_id(), will issue
> > warnings.
> > 
> > Disable interrupts before invoking the calback on UP if the state is
> > atomic and interrupts are expected to be disabled.
> > The "save" part is required because this is also invoked early in the
> > boot process while interrupts are disabled and must not be enabled. The
> > warnings aligns the function with cpuhp_thread_fun().
> > 
> > Fixes: 06ddd17521bf1 ("sched/smp: Always define is_percpu_thread() and scheduler_ipi()")
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Did we want me to merge this into sched/urgent or what was the
> hope/intention for this patch.

I did testing on ARM/UP and run into this. Therefore I would suggest
sched/urgent or the matching hotplug/urgent should one exist.

But since it is just a smp_processor_id() debug warning on UP and
nothing serious I don't mind to delay it until the next merge window.

Sebastian