Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d42e9452-8210-a06a-4c91-6c2f1d038a61@linux.ibm.com>
Date:   Tue, 13 Jun 2023 19:16:54 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.11.2
Subject: Re: [PATCH 3/9] cpu/SMT: Store the current/max number of threads
Content-Language: en-US
To:     Thomas Gleixner <tglx@linutronix.de>,
        Michael Ellerman <mpe@ellerman.id.au>,
        linux-kernel@vger.kernel.org
Cc:     linux-arch@vger.kernel.org, x86@kernel.org,
        dave.hansen@linux.intel.com, mingo@redhat.com, bp@alien8.de,
        linuxppc-dev@lists.ozlabs.org
References: <20230524155630.794584-1-mpe@ellerman.id.au>
 <20230524155630.794584-3-mpe@ellerman.id.au> <87fs6z80w5.ffs@tglx>
From:   Laurent Dufour <ldufour@linux.ibm.com>
In-Reply-To: <87fs6z80w5.ffs@tglx>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/06/2023 23:26:18, Thomas Gleixner wrote:
> On Thu, May 25 2023 at 01:56, Michael Ellerman wrote:
>>  #ifdef CONFIG_HOTPLUG_SMT
>>  enum cpuhp_smt_control cpu_smt_control __read_mostly = CPU_SMT_ENABLED;
>> +static unsigned int cpu_smt_max_threads __ro_after_init;
>> +unsigned int cpu_smt_num_threads;
> 
> Why needs this to be global? cpu_smt_control is pointlessly global already.

I agree that cpu_smt_*_threads should be static.

Howwever, regarding cpu_smt_control, it is used in 2 places in the x86 code:
 - arch/x86/power/hibernate.c in arch_resume_nosmt()
 - arch/x86/kernel/cpu/bugs.c in spectre_v2_user_select_mitigation()

An accessor function may be introduced to read that value in these 2
functions, but I'm wondering if that's really the best option.

Unless there is a real need to change this through this series, I think
cpu_smt_control can remain global.

Thomas, are you ok with that?

> 
>>  void __init cpu_smt_disable(bool force)
>>  {
>> @@ -433,10 +435,18 @@ void __init cpu_smt_disable(bool force)
>>   * The decision whether SMT is supported can only be done after the full
>>   * CPU identification. Called from architecture code.
>>   */
>> -void __init cpu_smt_check_topology(void)
>> +void __init cpu_smt_check_topology(unsigned int num_threads)
>>  {
>>  	if (!topology_smt_supported())
>>  		cpu_smt_control = CPU_SMT_NOT_SUPPORTED;
>> +
>> +	cpu_smt_max_threads = num_threads;
>> +
>> +	// May already be disabled by nosmt command line parameter
>> +	if (cpu_smt_control != CPU_SMT_ENABLED)
>> +		cpu_smt_num_threads = 1;
>> +	else
>> +		cpu_smt_num_threads = num_threads;
> 
> Taking Laurents findings into account this should be something like
> the incomplete below.
> 
> x86 would simply invoke cpu_smt_set_num_threads() with both arguments as
> smp_num_siblings while PPC can funnel its command line parameter through
> the num_threads argument.

I do prefer cpu_smt_set_num_threads() also.

Thanks,
Laurent

> 
> Thanks,
> 
>         tglx
> ---
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -414,6 +414,8 @@ void __weak arch_smt_update(void) { }
>  
>  #ifdef CONFIG_HOTPLUG_SMT
>  enum cpuhp_smt_control cpu_smt_control __read_mostly = CPU_SMT_ENABLED;
> +static unsigned int cpu_smt_max_threads __ro_after_init;
> +static unsigned int cpu_smt_num_threads = UINT_MAX;
>  
>  void __init cpu_smt_disable(bool force)
>  {
> @@ -427,24 +429,31 @@ void __init cpu_smt_disable(bool force)
>  		pr_info("SMT: disabled\n");
>  		cpu_smt_control = CPU_SMT_DISABLED;
>  	}
> +	cpu_smt_num_threads = 1;
>  }
>  
>  /*
>   * The decision whether SMT is supported can only be done after the full
>   * CPU identification. Called from architecture code.
>   */
> -void __init cpu_smt_check_topology(void)
> +void __init cpu_smt_set_num_threads(unsigned int max_threads, unsigned int num_threads)
>  {
> -	if (!topology_smt_supported())
> +	if (max_threads == 1)
>  		cpu_smt_control = CPU_SMT_NOT_SUPPORTED;
> -}
>  
> -static int __init smt_cmdline_disable(char *str)
> -{
> -	cpu_smt_disable(str && !strcmp(str, "force"));
> -	return 0;
> +	cpu_smt_max_threads = max_threads;
> +
> +	/*
> +	 * If SMT has been disabled via the kernel command line or SMT is
> +	 * not supported, set cpu_smt_num_threads to 1 for consistency.
> +	 * If enabled, take the architecture requested number of threads
> +	 * to bring up into account.
> +	 */
> +	if (cpu_smt_control != CPU_SMT_ENABLED)
> +		cpu_smt_num_threads = 1;
> +	else if (num_threads < cpu_smt_num_threads)
> +		cpu_smt_num_threads = num_threads;
>  }
> -early_param("nosmt", smt_cmdline_disable);
>  
>  static inline bool cpu_smt_allowed(unsigned int cpu)
>  {
> @@ -463,6 +472,13 @@ static inline bool cpu_smt_allowed(unsig
>  	return !cpumask_test_cpu(cpu, &cpus_booted_once_mask);
>  }
>  
> +static int __init smt_cmdline_disable(char *str)
> +{
> +	cpu_smt_disable(str && !strcmp(str, "force"));
> +	return 0;
> +}
> +early_param("nosmt", smt_cmdline_disable);
> +
>  /* Returns true if SMT is not supported of forcefully (irreversibly) disabled */
>  bool cpu_smt_possible(void)
>  {