| Message ID | 20230524155630.794584-3-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/9] cpu/SMT: Move SMT prototypes into cpu_smt.h | expand |
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. > 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. 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) {
On Sat, Jun 10 2023 at 23:26, Thomas Gleixner wrote: > On Thu, May 25 2023 at 01:56, Michael Ellerman wrote: > /* > * 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) Which makes topology_smt_supported() redundant, i.e. it can be removed. Thanks, tglx
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) > {
On Tue, Jun 13 2023 at 19:16, Laurent Dufour wrote: > 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() Bah. I must have fatfingered the grep then. > 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. That's fine. Thanks, tglx
On 13/06/2023 20:53:56, Thomas Gleixner wrote: > On Tue, Jun 13 2023 at 19:16, Laurent Dufour wrote: >> 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. I spoke too quickly, cpu_smt_num_threads is used in the powerpc code. When a new CPU is added it used to decide whether a thread has to be onlined or not, and there is no way to pass it as argument at this time. In details, it is used in topology_smt_thread_allowed() called by dlpar_online_cpu() (see patch "powerpc/pseries: Honour current SMT state when DLPAR onlining CPUs" at the end of this series). I think the best option is to keep it global. >> >> 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() > > Bah. I must have fatfingered the grep then. > >> 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. > > That's fine. > > Thanks, > > tglx
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index 182af64387d0..3406799c1e9d 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -34,6 +34,7 @@ #include <asm/hypervisor.h> #include <asm/tlbflush.h> #include <asm/cpu.h> +#include <asm/smp.h> #include "cpu.h" @@ -133,7 +134,7 @@ void __init check_bugs(void) * identify_boot_cpu() initialized SMT support information, let the * core code know. */ - cpu_smt_check_topology(); + cpu_smt_check_topology(smp_num_siblings); if (!IS_ENABLED(CONFIG_SMP)) { pr_info("CPU: "); diff --git a/include/linux/cpu_smt.h b/include/linux/cpu_smt.h index 17e105b52d85..8d4ae26047c9 100644 --- a/include/linux/cpu_smt.h +++ b/include/linux/cpu_smt.h @@ -12,15 +12,17 @@ enum cpuhp_smt_control { #if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT) extern enum cpuhp_smt_control cpu_smt_control; +extern unsigned int cpu_smt_num_threads; extern void cpu_smt_disable(bool force); -extern void cpu_smt_check_topology(void); +extern void cpu_smt_check_topology(unsigned int num_threads); extern bool cpu_smt_possible(void); extern int cpuhp_smt_enable(void); extern int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval); #else # define cpu_smt_control (CPU_SMT_NOT_IMPLEMENTED) +# define cpu_smt_num_threads 1 static inline void cpu_smt_disable(bool force) { } -static inline void cpu_smt_check_topology(void) { } +static inline void cpu_smt_check_topology(unsigned int num_threads) { } static inline bool cpu_smt_possible(void) { return false; } static inline int cpuhp_smt_enable(void) { return 0; } static inline int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval) { return 0; } diff --git a/kernel/cpu.c b/kernel/cpu.c index 01398ce3e131..a08dd8f93675 100644 --- 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; +unsigned int cpu_smt_num_threads; 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; } static int __init smt_cmdline_disable(char *str)
A subsequent patch will enable partial SMT states, ie. when not all SMT threads are brought online. To support that the SMT code needs to know the maximum number of SMT threads, and also the currently configured number. The arch code knows the max number of threads, so have the arch code pass that value to cpu_smt_check_topology(). Note that although topology_max_smt_threads() exists, it is not configured early enough to be used here. Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- arch/x86/kernel/cpu/bugs.c | 3 ++- include/linux/cpu_smt.h | 6 ++++-- kernel/cpu.c | 12 +++++++++++- 3 files changed, 17 insertions(+), 4 deletions(-)