lkml.org 
[lkml]   [2022]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2] cpu/hotplug: Set st->cpu earlier
From
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));
> }

\
 
 \ /
  Last update: 2022-03-28 17:21    [W:0.050 / U:0.604 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site