[PATCH v2] cpu/hotplug: Set st->cpu earlier

[PATCH v2] cpu/hotplug: Set st->cpu earlier

[Steven Price]

Setting the 'cpu' member of struct cpuhp_cpu_state in cpuhp_create() is
too late as other callbacks can be made before that point. In particular
if one of the earlier callbacks fails and triggers a rollback that
rollback will be done with st->cpu==0 causing CPU0 to be erroneously set
to be dying, causing the scheduler to get mightily confused and throw
its toys out of the pram.

Move the assignment earlier before any callbacks have a chance to run.

Fixes: 2ea46c6fc945 ("cpumask/hotplug: Fix cpu_dying() state tracking")
Signed-off-by: Steven Price <steven.price@arm.com>
CC: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
Changes since v1[1]:

 * Added a Fixes: tag.
 * Moved the assignment to just before cpuhp_set_state() which is the
   first place it is needed.

[1]: https://lore.kernel.org/r/20220225134918.105796-1-steven.price%40arm.com

 kernel/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 407a2568f35e..c1324c8677cf 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -720,7 +720,6 @@ static void cpuhp_create(unsigned int cpu)
 
 	init_completion(&st->done_up);
 	init_completion(&st->done_down);
-	st->cpu = cpu;
 }
 
 static int cpuhp_should_run(unsigned int cpu)
@@ -1351,6 +1350,7 @@ static int _cpu_up(unsigned int cpu, int tasks_frozen, enum cpuhp_state target)
 
 	cpuhp_tasks_frozen = tasks_frozen;
 
+	st->cpu = cpu;
 	cpuhp_set_state(st, target);
 	/*
 	 * If the current CPU state is in the range of the AP hotplug thread,
-- 
2.25.1

Re: [PATCH v2] cpu/hotplug: Set st->cpu earlier

[Vincent Donnefort]

On 16/03/2022 15:36, Steven Price wrote:
> Setting the 'cpu' member of struct cpuhp_cpu_state in cpuhp_create() is
> too late as other callbacks can be made before that point. In particular
> if one of the earlier callbacks fails and triggers a rollback that
> rollback will be done with st->cpu==0 causing CPU0 to be erroneously set
> to be dying, causing the scheduler to get mightily confused and throw
> its toys out of the pram.
> 
> Move the assignment earlier before any callbacks have a chance to run.
> 
> Fixes: 2ea46c6fc945 ("cpumask/hotplug: Fix cpu_dying() state tracking")
> Signed-off-by: Steven Price <steven.price@arm.com>
> CC: Dietmar Eggemann <dietmar.eggemann@arm.com>
> ---
> Changes since v1[1]:
> 
>   * Added a Fixes: tag.
>   * Moved the assignment to just before cpuhp_set_state() which is the
>     first place it is needed.
> 
> [1]: https://lore.kernel.org/r/20220225134918.105796-1-steven.price%40arm.com
> 
>   kernel/cpu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 407a2568f35e..c1324c8677cf 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -720,7 +720,6 @@ static void cpuhp_create(unsigned int cpu)
>   
>   	init_completion(&st->done_up);
>   	init_completion(&st->done_down);
> -	st->cpu = cpu;
>   }
>   
>   static int cpuhp_should_run(unsigned int cpu)
> @@ -1351,6 +1350,7 @@ static int _cpu_up(unsigned int cpu, int tasks_frozen, enum cpuhp_state target)
>   
>   	cpuhp_tasks_frozen = tasks_frozen;
>   
> +	st->cpu = cpu;
>   	cpuhp_set_state(st, target);
>   	/*
>   	 * If the current CPU state is in the range of the AP hotplug thread,

Reviewed-by: Vincent Donnefort <vincent.donnefort@arm.com>

I also gave a try with LISA's HotplugRollback test.

Re: [PATCH v2] cpu/hotplug: Set st->cpu earlier

[Thomas Gleixner]

On Wed, Mar 16 2022 at 15:36, Steven Price wrote:
> Setting the 'cpu' member of struct cpuhp_cpu_state in cpuhp_create() is
> too late as other callbacks can be made before that point.

What?

        CPUHP_OFFLINE = 0,
        CPUHP_CREATE_THREADS,

The create threads callback is the very first callback which is invoked
for a to be plugged CPU on the control CPU. So which earlier callback
can be invoked and fail?

Thanks,

        tglx

Re: [PATCH v2] cpu/hotplug: Set st->cpu earlier

[Vincent Donnefort]

On 22/03/2022 15:31, Thomas Gleixner wrote:
> On Wed, Mar 16 2022 at 15:36, Steven Price wrote:
>> Setting the 'cpu' member of struct cpuhp_cpu_state in cpuhp_create() is
>> too late as other callbacks can be made before that point.
> 
> What?
> 
>          CPUHP_OFFLINE = 0,
>          CPUHP_CREATE_THREADS,
> 
> The create threads callback is the very first callback which is invoked
> for a to be plugged CPU on the control CPU. So which earlier callback
> can be invoked and fail?
> 
> Thanks,
> 
>          tglx


CPUHP_CREATE_THREADS itself can fail, before st->cpu is set. Also, that 
value is used outside of the callbacks (cpuhp_set_state() in _cpu_up()).

But indeed this description could be refined a bit.

Re: [PATCH v2] cpu/hotplug: Set st->cpu earlier

[Thomas Gleixner]

On Tue, Mar 22 2022 at 15:59, Vincent Donnefort wrote:
> On 22/03/2022 15:31, Thomas Gleixner wrote:
>> On Wed, Mar 16 2022 at 15:36, Steven Price wrote:
>>> Setting the 'cpu' member of struct cpuhp_cpu_state in cpuhp_create() is
>>> too late as other callbacks can be made before that point.
>> 
>> What?
>> 
>>          CPUHP_OFFLINE = 0,
>>          CPUHP_CREATE_THREADS,
>> 
>> The create threads callback is the very first callback which is invoked
>> for a to be plugged CPU on the control CPU. So which earlier callback
>> can be invoked and fail?
>> 
>> Thanks,
>> 
>>          tglx
>
>
> CPUHP_CREATE_THREADS itself can fail, before st->cpu is set.

Sure. But that does not explain the problem.

> Also, that value is used outside of the callbacks (cpuhp_set_state()
> in _cpu_up()).

And why on earth is this not spelled out in the changelog?

> But indeed this description could be refined a bit.

Indeed. But the description is not the only problem here:

It's completely uncomprehensible from the code in _cpu_up() _WHY_ this

     st->cpu = cpu;
     
assignment has to be there.

It's non-sensical if you really think about it, right?

That said, I'm pretty sure you can come up with:

 - a proper one time initialization of @st which solves your problem

 - a proper changelog which explains it

Thanks,

        tglx

Re: [PATCH v2] cpu/hotplug: Set st->cpu earlier

[Steven Price]

Thanks for taking a look at this.

On 22/03/2022 22:58, Thomas Gleixner wrote:
> On Tue, Mar 22 2022 at 15:59, Vincent Donnefort wrote:
>> On 22/03/2022 15:31, Thomas Gleixner wrote:
>>> On Wed, Mar 16 2022 at 15:36, Steven Price wrote:
>>>> Setting the 'cpu' member of struct cpuhp_cpu_state in cpuhp_create() is
>>>> too late as other callbacks can be made before that point.
>>>
>>> What?
>>>
>>>          CPUHP_OFFLINE = 0,
>>>          CPUHP_CREATE_THREADS,
>>>
>>> The create threads callback is the very first callback which is invoked
>>> for a to be plugged CPU on the control CPU. So which earlier callback
>>> can be invoked and fail?
>>>
>>> Thanks,
>>>
>>>          tglx
>>
>>
>> CPUHP_CREATE_THREADS itself can fail, before st->cpu is set.
> 
> Sure. But that does not explain the problem.
> 
>> Also, that value is used outside of the callbacks (cpuhp_set_state()
>> in _cpu_up()).
> 
> And why on earth is this not spelled out in the changelog?

I apologies for that, I'm not very familiar with the code and I have to
admit I have been struggling to identify exactly what is going on here.
The actual issue I saw was if the callback fails then the rollback code
leaves things in a messed up state. By the looks of things that callback
that fails is indeed the first (CPUHP_CREATE_THREADS).

>> But indeed this description could be refined a bit.
> 
> Indeed. But the description is not the only problem here:
> 
> It's completely uncomprehensible from the code in _cpu_up() _WHY_ this
> 
>      st->cpu = cpu;
>      
> assignment has to be there.
> 
> It's non-sensical if you really think about it, right?

I entirely agree, and I did ask in my v1 posting[1] if anyone could
point me to a better place to do the assignment. Vincent suggested
moving it earlier in _cpu_up() which is this v2.

But it still seems out-of-place to me. I've just had a go at simply
removing the 'cpu' member and it doesn't look too bad. I'll post that
patch as a follow up. I'm open to other suggestions for the best way to
fix this.

Thanks,

Steve

[1]
https://lore.kernel.org/all/20220225134918.105796-1-steven.price@arm.com/

> That said, I'm pretty sure you can come up with:
> 
>  - a proper one time initialization of @st which solves your problem
> 
>  - a proper changelog which explains it
> 
> Thanks,
> 
>         tglx

[PATCH] cpu/hotplug: Remove the 'cpu' member of cpuhp_cpu_state

[Steven Price]

Currently the setting setting the 'cpu' member of struct cpuhp_cpu_state
in cpuhp_create() is too late as it is used earlier in _cpu_up(). If the
kzalloc_node() in __smpboot_create_thread() fails then the rollback will
be done with st->cpu==0 causing CPU0 to be erroneously set to be dying,
causing the scheduler to get mightily confused and throw its toys out of
the pram.

However the cpu number is actually available directly, so simply remove
the 'cpu' member and avoid the problem in the first place.

Fixes: 2ea46c6fc945 ("cpumask/hotplug: Fix cpu_dying() state tracking")
Signed-off-by: Steven Price <steven.price@arm.com>
---
 kernel/cpu.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 407a2568f35e..5601216eb51b 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -70,7 +70,6 @@ struct cpuhp_cpu_state {
 	bool			rollback;
 	bool			single;
 	bool			bringup;
-	int			cpu;
 	struct hlist_node	*node;
 	struct hlist_node	*last;
 	enum cpuhp_state	cb_state;
@@ -474,7 +473,7 @@ static inline bool cpu_smt_allowed(unsigned int cpu) { return true; }
 #endif
 
 static inline enum cpuhp_state
-cpuhp_set_state(struct cpuhp_cpu_state *st, enum cpuhp_state target)
+cpuhp_set_state(int cpu, struct cpuhp_cpu_state *st, enum cpuhp_state target)
 {
 	enum cpuhp_state prev_state = st->state;
 	bool bringup = st->state < target;
@@ -485,14 +484,15 @@ cpuhp_set_state(struct cpuhp_cpu_state *st, enum cpuhp_state target)
 	st->target = target;
 	st->single = false;
 	st->bringup = bringup;
-	if (cpu_dying(st->cpu) != !bringup)
-		set_cpu_dying(st->cpu, !bringup);
+	if (cpu_dying(cpu) != !bringup)
+		set_cpu_dying(cpu, !bringup);
 
 	return prev_state;
 }
 
 static inline void
-cpuhp_reset_state(struct cpuhp_cpu_state *st, enum cpuhp_state prev_state)
+cpuhp_reset_state(int cpu, struct cpuhp_cpu_state *st,
+		  enum cpuhp_state prev_state)
 {
 	bool bringup = !st->bringup;
 
@@ -519,8 +519,8 @@ cpuhp_reset_state(struct cpuhp_cpu_state *st, enum cpuhp_state prev_state)
 	}
 
 	st->bringup = bringup;
-	if (cpu_dying(st->cpu) != !bringup)
-		set_cpu_dying(st->cpu, !bringup);
+	if (cpu_dying(cpu) != !bringup)
+		set_cpu_dying(cpu, !bringup);
 }
 
 /* Regular hotplug invocation of the AP hotplug thread */
@@ -540,15 +540,16 @@ static void __cpuhp_kick_ap(struct cpuhp_cpu_state *st)
 	wait_for_ap_thread(st, st->bringup);
 }
 
-static int cpuhp_kick_ap(struct cpuhp_cpu_state *st, enum cpuhp_state target)
+static int cpuhp_kick_ap(int cpu, struct cpuhp_cpu_state *st,
+			 enum cpuhp_state target)
 {
 	enum cpuhp_state prev_state;
 	int ret;
 
-	prev_state = cpuhp_set_state(st, target);
+	prev_state = cpuhp_set_state(cpu, st, target);
 	__cpuhp_kick_ap(st);
 	if ((ret = st->result)) {
-		cpuhp_reset_state(st, prev_state);
+		cpuhp_reset_state(cpu, st, prev_state);
 		__cpuhp_kick_ap(st);
 	}
 
@@ -580,7 +581,7 @@ static int bringup_wait_for_ap(unsigned int cpu)
 	if (st->target <= CPUHP_AP_ONLINE_IDLE)
 		return 0;
 
-	return cpuhp_kick_ap(st, st->target);
+	return cpuhp_kick_ap(cpu, st, st->target);
 }
 
 static int bringup_cpu(unsigned int cpu)
@@ -703,7 +704,7 @@ static int cpuhp_up_callbacks(unsigned int cpu, struct cpuhp_cpu_state *st,
 			 ret, cpu, cpuhp_get_step(st->state)->name,
 			 st->state);
 
-		cpuhp_reset_state(st, prev_state);
+		cpuhp_reset_state(cpu, st, prev_state);
 		if (can_rollback_cpu(st))
 			WARN_ON(cpuhp_invoke_callback_range(false, cpu, st,
 							    prev_state));
@@ -720,7 +721,6 @@ static void cpuhp_create(unsigned int cpu)
 
 	init_completion(&st->done_up);
 	init_completion(&st->done_down);
-	st->cpu = cpu;
 }
 
 static int cpuhp_should_run(unsigned int cpu)
@@ -874,7 +874,7 @@ static int cpuhp_kick_ap_work(unsigned int cpu)
 	cpuhp_lock_release(true);
 
 	trace_cpuhp_enter(cpu, st->target, prev_state, cpuhp_kick_ap_work);
-	ret = cpuhp_kick_ap(st, st->target);
+	ret = cpuhp_kick_ap(cpu, st, st->target);
 	trace_cpuhp_exit(cpu, st->state, prev_state, ret);
 
 	return ret;
@@ -1106,7 +1106,7 @@ static int cpuhp_down_callbacks(unsigned int cpu, struct cpuhp_cpu_state *st,
 			 ret, cpu, cpuhp_get_step(st->state)->name,
 			 st->state);
 
-		cpuhp_reset_state(st, prev_state);
+		cpuhp_reset_state(cpu, st, prev_state);
 
 		if (st->state < prev_state)
 			WARN_ON(cpuhp_invoke_callback_range(true, cpu, st,
@@ -1133,7 +1133,7 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen,
 
 	cpuhp_tasks_frozen = tasks_frozen;
 
-	prev_state = cpuhp_set_state(st, target);
+	prev_state = cpuhp_set_state(cpu, st, target);
 	/*
 	 * If the current CPU state is in the range of the AP hotplug thread,
 	 * then we need to kick the thread.
@@ -1164,7 +1164,7 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen,
 	ret = cpuhp_down_callbacks(cpu, st, target);
 	if (ret && st->state < prev_state) {
 		if (st->state == CPUHP_TEARDOWN_CPU) {
-			cpuhp_reset_state(st, prev_state);
+			cpuhp_reset_state(cpu, st, prev_state);
 			__cpuhp_kick_ap(st);
 		} else {
 			WARN(1, "DEAD callback error for CPU%d", cpu);
@@ -1351,7 +1351,7 @@ static int _cpu_up(unsigned int cpu, int tasks_frozen, enum cpuhp_state target)
 
 	cpuhp_tasks_frozen = tasks_frozen;
 
-	cpuhp_set_state(st, target);
+	cpuhp_set_state(cpu, st, target);
 	/*
 	 * If the current CPU state is in the range of the AP hotplug thread,
 	 * then we need to kick the thread once more.
-- 
2.25.1

Re: [PATCH v2] cpu/hotplug: Set st->cpu earlier

[Thomas Gleixner]

On Wed, Mar 23 2022 at 10:10, Steven Price wrote:
> On 22/03/2022 22:58, Thomas Gleixner wrote:
>> Indeed. But the description is not the only problem here:
>> 
>> It's completely uncomprehensible from the code in _cpu_up() _WHY_ this
>> 
>>      st->cpu = cpu;
>>      
>> assignment has to be there.
>> 
>> It's non-sensical if you really think about it, right?
>
> I entirely agree, and I did ask in my v1 posting[1] if anyone could
> point me to a better place to do the assignment. Vincent suggested
> moving it earlier in _cpu_up() which is this v2.
>
> But it still seems out-of-place to me. I've just had a go at simply
> removing the 'cpu' member and it doesn't look too bad. I'll post that
> patch as a follow up. I'm open to other suggestions for the best way to
> fix this.

Yes, we can do that. The alternative solution is to initialize the
states once upfront. Something like the uncompiled below.

Thanks,

        tglx
---
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -714,15 +714,6 @@ static int cpuhp_up_callbacks(unsigned i
 /*
  * The cpu hotplug threads manage the bringup and teardown of the cpus
  */
-static void cpuhp_create(unsigned int cpu)
-{
-	struct cpuhp_cpu_state *st = per_cpu_ptr(&cpuhp_state, cpu);
-
-	init_completion(&st->done_up);
-	init_completion(&st->done_down);
-	st->cpu = cpu;
-}
-
 static int cpuhp_should_run(unsigned int cpu)
 {
 	struct cpuhp_cpu_state *st = this_cpu_ptr(&cpuhp_state);
@@ -882,15 +873,28 @@ static int cpuhp_kick_ap_work(unsigned i
 
 static struct smp_hotplug_thread cpuhp_threads = {
 	.store			= &cpuhp_state.thread,
-	.create			= &cpuhp_create,
 	.thread_should_run	= cpuhp_should_run,
 	.thread_fn		= cpuhp_thread_fun,
 	.thread_comm		= "cpuhp/%u",
 	.selfparking		= true,
 };
 
+static __init void cpuhp_init_state(void)
+{
+	struct cpuhp_cpu_state *st;
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		st = per_cpu_ptr(&cpuhp_state, cpu);
+		init_completion(&st->done_up);
+		init_completion(&st->done_down);
+		st->cpu = cpu;
+	}
+}
+
 void __init cpuhp_threads_init(void)
 {
+	cpuhp_init_state();
 	BUG_ON(smpboot_register_percpu_thread(&cpuhp_threads));
 	kthread_unpark(this_cpu_read(cpuhp_state.thread));
 }

Re: [PATCH v2] cpu/hotplug: Set st->cpu earlier

[Steven Price]

On 23/03/2022 11:21, Thomas Gleixner wrote:
> On Wed, Mar 23 2022 at 10:10, Steven Price wrote:
>> On 22/03/2022 22:58, Thomas Gleixner wrote:
>>> Indeed. But the description is not the only problem here:
>>>
>>> It's completely uncomprehensible from the code in _cpu_up() _WHY_ this
>>>
>>>      st->cpu = cpu;
>>>      
>>> assignment has to be there.
>>>
>>> It's non-sensical if you really think about it, right?
>>
>> I entirely agree, and I did ask in my v1 posting[1] if anyone could
>> point me to a better place to do the assignment. Vincent suggested
>> moving it earlier in _cpu_up() which is this v2.
>>
>> But it still seems out-of-place to me. I've just had a go at simply
>> removing the 'cpu' member and it doesn't look too bad. I'll post that
>> patch as a follow up. I'm open to other suggestions for the best way to
>> fix this.
> 
> Yes, we can do that. The alternative solution is to initialize the
> states once upfront. Something like the uncompiled below.

The below works as well. Do you prefer to go with that or my removal of
the 'cpu' member?

If the below then would you be able to send a proper patch? Feel free to
add my...

Tested-by: Steven Price <steven.price@arm.com>

Thanks,

Steve

> Thanks,
> 
>         tglx
> ---
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -714,15 +714,6 @@ static int cpuhp_up_callbacks(unsigned i
>  /*
>   * The cpu hotplug threads manage the bringup and teardown of the cpus
>   */
> -static void cpuhp_create(unsigned int cpu)
> -{
> -	struct cpuhp_cpu_state *st = per_cpu_ptr(&cpuhp_state, cpu);
> -
> -	init_completion(&st->done_up);
> -	init_completion(&st->done_down);
> -	st->cpu = cpu;
> -}
> -
>  static int cpuhp_should_run(unsigned int cpu)
>  {
>  	struct cpuhp_cpu_state *st = this_cpu_ptr(&cpuhp_state);
> @@ -882,15 +873,28 @@ static int cpuhp_kick_ap_work(unsigned i
>  
>  static struct smp_hotplug_thread cpuhp_threads = {
>  	.store			= &cpuhp_state.thread,
> -	.create			= &cpuhp_create,
>  	.thread_should_run	= cpuhp_should_run,
>  	.thread_fn		= cpuhp_thread_fun,
>  	.thread_comm		= "cpuhp/%u",
>  	.selfparking		= true,
>  };
>  
> +static __init void cpuhp_init_state(void)
> +{
> +	struct cpuhp_cpu_state *st;
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		st = per_cpu_ptr(&cpuhp_state, cpu);
> +		init_completion(&st->done_up);
> +		init_completion(&st->done_down);
> +		st->cpu = cpu;
> +	}
> +}
> +
>  void __init cpuhp_threads_init(void)
>  {
> +	cpuhp_init_state();
>  	BUG_ON(smpboot_register_percpu_thread(&cpuhp_threads));
>  	kthread_unpark(this_cpu_read(cpuhp_state.thread));
>  }