| Message ID | 20230524155630.794584-8-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 24/05/2023 17:56:29, Michael Ellerman wrote: > Add support for HOTPLUG_SMT, which enables the generic sysfs SMT support > files in /sys/devices/system/cpu/smt, as well as the "nosmt" boot > parameter. Hi Michael, It seems that there is now a conflict between with the PPC 'smt-enabled' boot option. Booting the patched kernel with 'smt-enabled=4', later, change to the SMT level (for instance to 6) done through /sys/devices/system/cpu/smt/control are not applied. Nothing happens. Based on my early debug, I think the reasons is that cpu_smt_num_threads=8 when entering __store_smt_control(). But I need to dig further. BTW, should the 'smt-enabled' PPC specific option remain? Cheers, Laurent. > Implement the recently added hooks to allow partial SMT states, allow > any number of threads per core. > > Tie the config symbol to HOTPLUG_CPU, which enables it on the major > platforms that support SMT. If there are other platforms that want the > SMT support that can be tweaked in future. > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > --- > arch/powerpc/Kconfig | 1 + > arch/powerpc/include/asm/topology.h | 25 +++++++++++++++++++++++++ > arch/powerpc/kernel/smp.c | 3 +++ > 3 files changed, 29 insertions(+) > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 539d1f03ff42..5cf87ca10a9c 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -273,6 +273,7 @@ config PPC > select HAVE_SYSCALL_TRACEPOINTS > select HAVE_VIRT_CPU_ACCOUNTING > select HAVE_VIRT_CPU_ACCOUNTING_GEN > + select HOTPLUG_SMT if HOTPLUG_CPU > select HUGETLB_PAGE_SIZE_VARIABLE if PPC_BOOK3S_64 && HUGETLB_PAGE > select IOMMU_HELPER if PPC64 > select IRQ_DOMAIN > diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h > index 8a4d4f4d9749..1e9117a22d14 100644 > --- a/arch/powerpc/include/asm/topology.h > +++ b/arch/powerpc/include/asm/topology.h > @@ -143,5 +143,30 @@ static inline int cpu_to_coregroup_id(int cpu) > #endif > #endif > > +#ifdef CONFIG_HOTPLUG_SMT > +#include <linux/cpu_smt.h> > +#include <asm/cputhreads.h> > + > +static inline bool topology_smt_supported(void) > +{ > + return threads_per_core > 1; > +} > + > +static inline bool topology_smt_threads_supported(unsigned int num_threads) > +{ > + return num_threads <= threads_per_core; > +} > + > +static inline bool topology_is_primary_thread(unsigned int cpu) > +{ > + return cpu == cpu_first_thread_sibling(cpu); > +} > + > +static inline bool topology_smt_thread_allowed(unsigned int cpu) > +{ > + return cpu_thread_in_core(cpu) < cpu_smt_num_threads; > +} > +#endif > + > #endif /* __KERNEL__ */ > #endif /* _ASM_POWERPC_TOPOLOGY_H */ > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c > index 265801a3e94c..eed20b9253b7 100644 > --- a/arch/powerpc/kernel/smp.c > +++ b/arch/powerpc/kernel/smp.c > @@ -1154,6 +1154,9 @@ void __init smp_prepare_cpus(unsigned int max_cpus) > > if (smp_ops && smp_ops->probe) > smp_ops->probe(); > + > + // Initalise the generic SMT topology support > + cpu_smt_check_topology(threads_per_core); > } > > void smp_prepare_boot_cpu(void)
On 01/06/2023 15:27:30, Laurent Dufour wrote: > On 24/05/2023 17:56:29, Michael Ellerman wrote: >> Add support for HOTPLUG_SMT, which enables the generic sysfs SMT support >> files in /sys/devices/system/cpu/smt, as well as the "nosmt" boot >> parameter. > > Hi Michael, > > It seems that there is now a conflict between with the PPC 'smt-enabled' > boot option. > > Booting the patched kernel with 'smt-enabled=4', later, change to the SMT > level (for instance to 6) done through /sys/devices/system/cpu/smt/control > are not applied. Nothing happens. > Based on my early debug, I think the reasons is that cpu_smt_num_threads=8 > when entering __store_smt_control(). But I need to dig further. I dug deeper. FWIW, I think smt_enabled_at_boot should be passed to cpu_smt_check_topology() in smp_prepare_cpus(), instead of threads_per_core. But that's not enough to fix the issue because this value is also used to set cpu_smt_max_threads. To achieve that, cpu_smt_check_topology() should receive 2 parameters, the current SMT level define at boot time, and the maximum SMT level. The attached patch is fixing the issue on my ppc64 test LPAR. This patch is not addressing the x86 architecture (I didn't get the time to do it, but it should be doable) and should be spread among the patches 3 and 8 of your series. Hope this helps. Cheers, Laurent. > > BTW, should the 'smt-enabled' PPC specific option remain? > > Cheers, > Laurent. > >> Implement the recently added hooks to allow partial SMT states, allow >> any number of threads per core. >> >> Tie the config symbol to HOTPLUG_CPU, which enables it on the major >> platforms that support SMT. If there are other platforms that want the >> SMT support that can be tweaked in future. >> >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> >> --- >> arch/powerpc/Kconfig | 1 + >> arch/powerpc/include/asm/topology.h | 25 +++++++++++++++++++++++++ >> arch/powerpc/kernel/smp.c | 3 +++ >> 3 files changed, 29 insertions(+) >> >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig >> index 539d1f03ff42..5cf87ca10a9c 100644 >> --- a/arch/powerpc/Kconfig >> +++ b/arch/powerpc/Kconfig >> @@ -273,6 +273,7 @@ config PPC >> select HAVE_SYSCALL_TRACEPOINTS >> select HAVE_VIRT_CPU_ACCOUNTING >> select HAVE_VIRT_CPU_ACCOUNTING_GEN >> + select HOTPLUG_SMT if HOTPLUG_CPU >> select HUGETLB_PAGE_SIZE_VARIABLE if PPC_BOOK3S_64 && HUGETLB_PAGE >> select IOMMU_HELPER if PPC64 >> select IRQ_DOMAIN >> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h >> index 8a4d4f4d9749..1e9117a22d14 100644 >> --- a/arch/powerpc/include/asm/topology.h >> +++ b/arch/powerpc/include/asm/topology.h >> @@ -143,5 +143,30 @@ static inline int cpu_to_coregroup_id(int cpu) >> #endif >> #endif >> >> +#ifdef CONFIG_HOTPLUG_SMT >> +#include <linux/cpu_smt.h> >> +#include <asm/cputhreads.h> >> + >> +static inline bool topology_smt_supported(void) >> +{ >> + return threads_per_core > 1; >> +} >> + >> +static inline bool topology_smt_threads_supported(unsigned int num_threads) >> +{ >> + return num_threads <= threads_per_core; >> +} >> + >> +static inline bool topology_is_primary_thread(unsigned int cpu) >> +{ >> + return cpu == cpu_first_thread_sibling(cpu); >> +} >> + >> +static inline bool topology_smt_thread_allowed(unsigned int cpu) >> +{ >> + return cpu_thread_in_core(cpu) < cpu_smt_num_threads; >> +} >> +#endif >> + >> #endif /* __KERNEL__ */ >> #endif /* _ASM_POWERPC_TOPOLOGY_H */ >> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c >> index 265801a3e94c..eed20b9253b7 100644 >> --- a/arch/powerpc/kernel/smp.c >> +++ b/arch/powerpc/kernel/smp.c >> @@ -1154,6 +1154,9 @@ void __init smp_prepare_cpus(unsigned int max_cpus) >> >> if (smp_ops && smp_ops->probe) >> smp_ops->probe(); >> + >> + // Initalise the generic SMT topology support >> + cpu_smt_check_topology(threads_per_core); >> } >> >> void smp_prepare_boot_cpu(void) > From 682e7d78fb98d6298926e88e5093e2172488ea6f Mon Sep 17 00:00:00 2001 From: Laurent Dufour <ldufour@linux.ibm.com> Date: Thu, 1 Jun 2023 18:02:55 +0200 Subject: [PATCH] Consider the SMT level specify at boot time This allows PPC kernel to boot with a SMT level different from 1 or threads per core value. Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> --- arch/powerpc/kernel/smp.c | 2 +- include/linux/cpu_smt.h | 6 ++++-- kernel/cpu.c | 9 +++++++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c index eed20b9253b7..ec8ccf3d6294 100644 --- a/arch/powerpc/kernel/smp.c +++ b/arch/powerpc/kernel/smp.c @@ -1156,7 +1156,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus) smp_ops->probe(); // Initalise the generic SMT topology support - cpu_smt_check_topology(threads_per_core); + cpu_smt_check_topology(smt_enabled_at_boot, threads_per_core); } void smp_prepare_boot_cpu(void) diff --git a/include/linux/cpu_smt.h b/include/linux/cpu_smt.h index 8d4ae26047c9..b5f13acb323f 100644 --- a/include/linux/cpu_smt.h +++ b/include/linux/cpu_smt.h @@ -14,7 +14,8 @@ enum cpuhp_smt_control { 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(unsigned int num_threads); +extern void cpu_smt_check_topology(unsigned int num_threads, + unsigned int max_threads); extern bool cpu_smt_possible(void); extern int cpuhp_smt_enable(void); extern int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval); @@ -22,7 +23,8 @@ extern int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval); # 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(unsigned int num_threads) { } +static inline void cpu_smt_check_topology(unsigned int num_threads, + unsigned int max_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 aca23c7b4547..268d386bd4e7 100644 --- a/kernel/cpu.c +++ b/kernel/cpu.c @@ -435,12 +435,17 @@ 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(unsigned int num_threads) +void __init cpu_smt_check_topology(unsigned int num_threads, + unsigned int max_threads) { if (!topology_smt_supported()) cpu_smt_control = CPU_SMT_NOT_SUPPORTED; - cpu_smt_max_threads = num_threads; + cpu_smt_max_threads = max_threads; + + WARN_ON(num_threads > max_threads); + if (num_threads > max_threads) + num_threads = max_threads; // May already be disabled by nosmt command line parameter if (cpu_smt_control != CPU_SMT_ENABLED)
On Thu, Jun 01 2023 at 18:19, Laurent Dufour wrote: > @@ -435,12 +435,17 @@ 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(unsigned int num_threads) > +void __init cpu_smt_check_topology(unsigned int num_threads, > + unsigned int max_threads) > { > if (!topology_smt_supported()) > cpu_smt_control = CPU_SMT_NOT_SUPPORTED; > > - cpu_smt_max_threads = num_threads; > + cpu_smt_max_threads = max_threads; > + > + WARN_ON(num_threads > max_threads); > + if (num_threads > max_threads) > + num_threads = max_threads; This does not work. The call site does: > + cpu_smt_check_topology(smt_enabled_at_boot, threads_per_core); smt_enabled_at_boot is 0 when 'smt-enabled=off', which is not what the hotplug core expects. If SMT is disabled it brings up the primary thread, which means cpu_smt_num_threads = 1. This needs more thoughts to avoid a completely inconsistent duct tape mess. Btw, the command line parser and the variable smt_enabled_at_boot being type int allow negative number of threads too... Maybe not what you want. Thanks, tglx
On 10/06/2023 23:10:02, Thomas Gleixner wrote: > On Thu, Jun 01 2023 at 18:19, Laurent Dufour wrote: >> @@ -435,12 +435,17 @@ 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(unsigned int num_threads) >> +void __init cpu_smt_check_topology(unsigned int num_threads, >> + unsigned int max_threads) >> { >> if (!topology_smt_supported()) >> cpu_smt_control = CPU_SMT_NOT_SUPPORTED; >> >> - cpu_smt_max_threads = num_threads; >> + cpu_smt_max_threads = max_threads; >> + >> + WARN_ON(num_threads > max_threads); >> + if (num_threads > max_threads) >> + num_threads = max_threads; > > This does not work. The call site does: > >> + cpu_smt_check_topology(smt_enabled_at_boot, threads_per_core); > > smt_enabled_at_boot is 0 when 'smt-enabled=off', which is not what the > hotplug core expects. If SMT is disabled it brings up the primary > thread, which means cpu_smt_num_threads = 1. Thanks, Thomas, Definitively, a test against smt_enabled_at_boot==0 is required here. > This needs more thoughts to avoid a completely inconsistent duct tape > mess. Despite the test against smt_enabled_at_boot, mentioned above, I can't see anything else to rework. Am I missing something? > > Btw, the command line parser and the variable smt_enabled_at_boot being > type int allow negative number of threads too... Maybe not what you want. I do agree, it should an unsigned type. Thanks, Laurent. > Thanks, > > tglx > > > >
On Mon, Jun 12 2023 at 17:20, Laurent Dufour wrote: > On 10/06/2023 23:10:02, Thomas Gleixner wrote: >> This needs more thoughts to avoid a completely inconsistent duct tape >> mess. > > Despite the test against smt_enabled_at_boot, mentioned above, I can't see > anything else to rework. Am I missing something? See my comments on the core code changes. >> Btw, the command line parser and the variable smt_enabled_at_boot being >> type int allow negative number of threads too... Maybe not what you want. > > I do agree, it should an unsigned type. I assume you'll fix that yourself. :) Thanks, tglx
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 539d1f03ff42..5cf87ca10a9c 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -273,6 +273,7 @@ config PPC select HAVE_SYSCALL_TRACEPOINTS select HAVE_VIRT_CPU_ACCOUNTING select HAVE_VIRT_CPU_ACCOUNTING_GEN + select HOTPLUG_SMT if HOTPLUG_CPU select HUGETLB_PAGE_SIZE_VARIABLE if PPC_BOOK3S_64 && HUGETLB_PAGE select IOMMU_HELPER if PPC64 select IRQ_DOMAIN diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h index 8a4d4f4d9749..1e9117a22d14 100644 --- a/arch/powerpc/include/asm/topology.h +++ b/arch/powerpc/include/asm/topology.h @@ -143,5 +143,30 @@ static inline int cpu_to_coregroup_id(int cpu) #endif #endif +#ifdef CONFIG_HOTPLUG_SMT +#include <linux/cpu_smt.h> +#include <asm/cputhreads.h> + +static inline bool topology_smt_supported(void) +{ + return threads_per_core > 1; +} + +static inline bool topology_smt_threads_supported(unsigned int num_threads) +{ + return num_threads <= threads_per_core; +} + +static inline bool topology_is_primary_thread(unsigned int cpu) +{ + return cpu == cpu_first_thread_sibling(cpu); +} + +static inline bool topology_smt_thread_allowed(unsigned int cpu) +{ + return cpu_thread_in_core(cpu) < cpu_smt_num_threads; +} +#endif + #endif /* __KERNEL__ */ #endif /* _ASM_POWERPC_TOPOLOGY_H */ diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c index 265801a3e94c..eed20b9253b7 100644 --- a/arch/powerpc/kernel/smp.c +++ b/arch/powerpc/kernel/smp.c @@ -1154,6 +1154,9 @@ void __init smp_prepare_cpus(unsigned int max_cpus) if (smp_ops && smp_ops->probe) smp_ops->probe(); + + // Initalise the generic SMT topology support + cpu_smt_check_topology(threads_per_core); } void smp_prepare_boot_cpu(void)
Add support for HOTPLUG_SMT, which enables the generic sysfs SMT support files in /sys/devices/system/cpu/smt, as well as the "nosmt" boot parameter. Implement the recently added hooks to allow partial SMT states, allow any number of threads per core. Tie the config symbol to HOTPLUG_CPU, which enables it on the major platforms that support SMT. If there are other platforms that want the SMT support that can be tweaked in future. Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- arch/powerpc/Kconfig | 1 + arch/powerpc/include/asm/topology.h | 25 +++++++++++++++++++++++++ arch/powerpc/kernel/smp.c | 3 +++ 3 files changed, 29 insertions(+)