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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <32ac2c66-6286-5d35-81e0-a3adcf8c35d4@arm.com>
Date:   Mon, 28 Mar 2022 16:19:46 +0100
From:   Steven Price <steven.price@....com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Vincent Donnefort <vincent.donnefort@....com>,
        Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org, Baokun Li <libaokun1@...wei.com>,
        Dongli Zhang <dongli.zhang@...cle.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Valentin Schneider <valentin.schneider@....com>,
        Yuan ZhaoXiong <yuanzhaoxiong@...du.com>,
        YueHaibing <yuehaibing@...wei.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>
Subject: Re: [PATCH v2] cpu/hotplug: Set st->cpu earlier

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@....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));
>  }