A subsequent patch would like to use the cpuhp_smt_control enum as part
of the interface between generic and arch code.
Currently that leads to circular header dependencies. So split the enum
and related declarations into a separate header.
Signed-off-by: Michael Ellerman <[email protected]>
---
arch/x86/include/asm/topology.h | 2 ++
include/linux/cpu.h | 25 +------------------------
include/linux/cpu_smt.h | 29 +++++++++++++++++++++++++++++
3 files changed, 32 insertions(+), 24 deletions(-)
create mode 100644 include/linux/cpu_smt.h
diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index 458c891a8273..66927a59e822 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -136,6 +136,8 @@ static inline int topology_max_smt_threads(void)
return __max_smt_threads;
}
+#include <linux/cpu_smt.h>
+
int topology_update_package_map(unsigned int apicid, unsigned int cpu);
int topology_update_die_map(unsigned int dieid, unsigned int cpu);
int topology_phys_to_logical_pkg(unsigned int pkg);
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 8582a7142623..40548f3c201c 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -18,6 +18,7 @@
#include <linux/compiler.h>
#include <linux/cpumask.h>
#include <linux/cpuhotplug.h>
+#include <linux/cpu_smt.h>
struct device;
struct device_node;
@@ -202,30 +203,6 @@ void cpuhp_report_idle_dead(void);
static inline void cpuhp_report_idle_dead(void) { }
#endif /* #ifdef CONFIG_HOTPLUG_CPU */
-enum cpuhp_smt_control {
- CPU_SMT_ENABLED,
- CPU_SMT_DISABLED,
- CPU_SMT_FORCE_DISABLED,
- CPU_SMT_NOT_SUPPORTED,
- CPU_SMT_NOT_IMPLEMENTED,
-};
-
-#if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT)
-extern enum cpuhp_smt_control cpu_smt_control;
-extern void cpu_smt_disable(bool force);
-extern void cpu_smt_check_topology(void);
-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)
-static inline void cpu_smt_disable(bool force) { }
-static inline void cpu_smt_check_topology(void) { }
-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; }
-#endif
-
extern bool cpu_mitigations_off(void);
extern bool cpu_mitigations_auto_nosmt(void);
diff --git a/include/linux/cpu_smt.h b/include/linux/cpu_smt.h
new file mode 100644
index 000000000000..17e105b52d85
--- /dev/null
+++ b/include/linux/cpu_smt.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_CPU_SMT_H_
+#define _LINUX_CPU_SMT_H_
+
+enum cpuhp_smt_control {
+ CPU_SMT_ENABLED,
+ CPU_SMT_DISABLED,
+ CPU_SMT_FORCE_DISABLED,
+ CPU_SMT_NOT_SUPPORTED,
+ CPU_SMT_NOT_IMPLEMENTED,
+};
+
+#if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT)
+extern enum cpuhp_smt_control cpu_smt_control;
+extern void cpu_smt_disable(bool force);
+extern void cpu_smt_check_topology(void);
+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)
+static inline void cpu_smt_disable(bool force) { }
+static inline void cpu_smt_check_topology(void) { }
+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; }
+#endif
+
+#endif /* _LINUX_CPU_SMT_H_ */
--
2.40.1
As part of the generic HOTPLUG_SMT code, there is support for disabling
secondary SMT threads at boot time, by passing "nosmt" on the kernel
command line.
The way that is implemented is the secondary threads are brought partly
online, and then taken back offline again. That is done to support x86
CPUs needing certain initialisation done on all threads. However powerpc
has similar needs, see commit d70a54e2d085 ("powerpc/powernv: Ignore
smt-enabled on Power8 and later").
For that to work the powerpc CPU hotplug callbacks need to be registered
before secondary CPUs are brought online, otherwise __cpu_disable()
fails due to smp_ops->cpu_disable being NULL.
So split the basic initialisation into pseries_cpu_hotplug_init() which
can be called early from setup_arch(). The DLPAR related initialisation
can still be done later, because it needs to do allocations.
Signed-off-by: Michael Ellerman <[email protected]>
---
arch/powerpc/platforms/pseries/hotplug-cpu.c | 22 ++++++++++++--------
arch/powerpc/platforms/pseries/pseries.h | 2 ++
arch/powerpc/platforms/pseries/setup.c | 2 ++
3 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 1a3cb313976a..61fb7cb00880 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -845,15 +845,9 @@ static struct notifier_block pseries_smp_nb = {
.notifier_call = pseries_smp_notifier,
};
-static int __init pseries_cpu_hotplug_init(void)
+void __init pseries_cpu_hotplug_init(void)
{
int qcss_tok;
- unsigned int node;
-
-#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
- ppc_md.cpu_probe = dlpar_cpu_probe;
- ppc_md.cpu_release = dlpar_cpu_release;
-#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */
rtas_stop_self_token = rtas_function_token(RTAS_FN_STOP_SELF);
qcss_tok = rtas_function_token(RTAS_FN_QUERY_CPU_STOPPED_STATE);
@@ -862,12 +856,22 @@ static int __init pseries_cpu_hotplug_init(void)
qcss_tok == RTAS_UNKNOWN_SERVICE) {
printk(KERN_INFO "CPU Hotplug not supported by firmware "
"- disabling.\n");
- return 0;
+ return;
}
smp_ops->cpu_offline_self = pseries_cpu_offline_self;
smp_ops->cpu_disable = pseries_cpu_disable;
smp_ops->cpu_die = pseries_cpu_die;
+}
+
+static int __init pseries_dlpar_init(void)
+{
+ unsigned int node;
+
+#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
+ ppc_md.cpu_probe = dlpar_cpu_probe;
+ ppc_md.cpu_release = dlpar_cpu_release;
+#endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */
/* Processors can be added/removed only on LPAR */
if (firmware_has_feature(FW_FEATURE_LPAR)) {
@@ -886,4 +890,4 @@ static int __init pseries_cpu_hotplug_init(void)
return 0;
}
-machine_arch_initcall(pseries, pseries_cpu_hotplug_init);
+machine_arch_initcall(pseries, pseries_dlpar_init);
diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
index f8bce40ebd0c..f8893ba46e83 100644
--- a/arch/powerpc/platforms/pseries/pseries.h
+++ b/arch/powerpc/platforms/pseries/pseries.h
@@ -75,11 +75,13 @@ static inline int dlpar_hp_pmem(struct pseries_hp_errorlog *hp_elog)
#ifdef CONFIG_HOTPLUG_CPU
int dlpar_cpu(struct pseries_hp_errorlog *hp_elog);
+void pseries_cpu_hotplug_init(void);
#else
static inline int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
{
return -EOPNOTSUPP;
}
+static inline void pseries_cpu_hotplug_init(void) { }
#endif
/* PCI root bridge prepare function override for pseries */
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index e2a57cfa6c83..41451b76c6e5 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -816,6 +816,8 @@ static void __init pSeries_setup_arch(void)
/* Discover PIC type and setup ppc_md accordingly */
smp_init_pseries();
+ // Setup CPU hotplug callbacks
+ pseries_cpu_hotplug_init();
if (radix_enabled() && !mmu_has_feature(MMU_FTR_GTSE))
if (!firmware_has_feature(FW_FEATURE_RPT_INVALIDATE))
--
2.40.1
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 <[email protected]>
---
arch/x86/kernel/cpu/bugs.c | 3 ++-
include/linux/cpu_smt.h | 6 ++++--
kernel/cpu.c | 12 +++++++++++-
3 files changed, 17 insertions(+), 4 deletions(-)
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)
--
2.40.1
Move the simple exit cases, ie. which don't depend on the value written,
earlier in the function. That makes it clearer that regardless of the
input those states can not be transitioned out of.
That does have a user-visible effect, in that the error returned will
now always be EPERM/ENODEV for those states, regardless of the value
written. Previously writing an invalid value would return EINVAL even
when in those states.
Signed-off-by: Michael Ellerman <[email protected]>
---
kernel/cpu.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/kernel/cpu.c b/kernel/cpu.c
index f4a2c5845bcb..01398ce3e131 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -2481,6 +2481,12 @@ __store_smt_control(struct device *dev, struct device_attribute *attr,
{
int ctrlval, ret;
+ if (cpu_smt_control == CPU_SMT_FORCE_DISABLED)
+ return -EPERM;
+
+ if (cpu_smt_control == CPU_SMT_NOT_SUPPORTED)
+ return -ENODEV;
+
if (sysfs_streq(buf, "on"))
ctrlval = CPU_SMT_ENABLED;
else if (sysfs_streq(buf, "off"))
@@ -2490,12 +2496,6 @@ __store_smt_control(struct device *dev, struct device_attribute *attr,
else
return -EINVAL;
- if (cpu_smt_control == CPU_SMT_FORCE_DISABLED)
- return -EPERM;
-
- if (cpu_smt_control == CPU_SMT_NOT_SUPPORTED)
- return -ENODEV;
-
ret = lock_device_hotplug_sysfs();
if (ret)
return ret;
--
2.40.1
A subsequent patch will enable partial SMT states, ie. when not all SMT
threads are brought online.
To support that, add an arch helper to check how many SMT threads are
supported.
To retain existing behaviour, the x86 implementation only allows a
single thread or all threads to be online.
Signed-off-by: Michael Ellerman <[email protected]>
---
arch/x86/include/asm/topology.h | 2 ++
arch/x86/kernel/smpboot.c | 12 ++++++++++++
2 files changed, 14 insertions(+)
diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index 66927a59e822..197ec2589f5d 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -144,6 +144,7 @@ int topology_phys_to_logical_pkg(unsigned int pkg);
int topology_phys_to_logical_die(unsigned int die, unsigned int cpu);
bool topology_is_primary_thread(unsigned int cpu);
bool topology_smt_supported(void);
+bool topology_smt_threads_supported(unsigned int threads);
#else
#define topology_max_packages() (1)
static inline int
@@ -157,6 +158,7 @@ static inline int topology_max_die_per_package(void) { return 1; }
static inline int topology_max_smt_threads(void) { return 1; }
static inline bool topology_is_primary_thread(unsigned int cpu) { return true; }
static inline bool topology_smt_supported(void) { return false; }
+static inline bool topology_smt_threads_supported(unsigned int threads) { return false; }
#endif
static inline void arch_fix_phys_package_id(int num, u32 slot)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 352f0ce1ece4..c7ba62beae3e 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -286,6 +286,18 @@ bool topology_smt_supported(void)
return smp_num_siblings > 1;
}
+/**
+ * topology_smt_threads_supported - Check if the given number of SMT threads
+ * is supported.
+ *
+ * @threads: The number of SMT threads.
+ */
+bool topology_smt_threads_supported(unsigned int threads)
+{
+ // Only support a single thread or all threads.
+ return threads == 1 || threads == smp_num_siblings;
+}
+
/**
* topology_phys_to_logical_pkg - Map a physical package id to a logical
*
--
2.40.1
A subsequent patch will enable partial SMT states, ie. when not all SMT
threads are brought online.
To support that, add an arch helper which checks whether a given CPU is
allowed to be brought online depending on how many SMT threads are
currently enabled.
Call the helper from cpu_smt_enable(), and cpu_smt_allowed() when SMT is
enabled, to check if the particular thread should be onlined. Notably,
also call it from cpu_smt_disable() if CPU_SMT_ENABLED, to allow
offlining some threads to move from a higher to lower number of threads
online.
Signed-off-by: Michael Ellerman <[email protected]>
---
arch/x86/include/asm/topology.h | 2 ++
arch/x86/kernel/smpboot.c | 15 +++++++++++++++
kernel/cpu.c | 10 +++++++++-
3 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index 197ec2589f5d..c6245ea6e589 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -145,6 +145,7 @@ int topology_phys_to_logical_die(unsigned int die, unsigned int cpu);
bool topology_is_primary_thread(unsigned int cpu);
bool topology_smt_supported(void);
bool topology_smt_threads_supported(unsigned int threads);
+bool topology_smt_thread_allowed(unsigned int cpu);
#else
#define topology_max_packages() (1)
static inline int
@@ -159,6 +160,7 @@ static inline int topology_max_smt_threads(void) { return 1; }
static inline bool topology_is_primary_thread(unsigned int cpu) { return true; }
static inline bool topology_smt_supported(void) { return false; }
static inline bool topology_smt_threads_supported(unsigned int threads) { return false; }
+static inline bool topology_smt_thread_allowed(unsigned int cpu) { return false; }
#endif
static inline void arch_fix_phys_package_id(int num, u32 slot)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index c7ba62beae3e..244b4df40600 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -298,6 +298,21 @@ bool topology_smt_threads_supported(unsigned int threads)
return threads == 1 || threads == smp_num_siblings;
}
+/**
+ * topology_smt_thread_allowed - When enabling SMT check whether this particular
+ * CPU thread is allowed to be brought online.
+ * @cpu: CPU to check
+ */
+bool topology_smt_thread_allowed(unsigned int cpu)
+{
+ /*
+ * No extra logic s required here to support different thread values
+ * because threads will always == 1 or smp_num_siblings because of
+ * topology_smt_threads_supported().
+ */
+ return true;
+}
+
/**
* topology_phys_to_logical_pkg - Map a physical package id to a logical
*
diff --git a/kernel/cpu.c b/kernel/cpu.c
index a08dd8f93675..72b9a03a4fef 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -458,7 +458,7 @@ early_param("nosmt", smt_cmdline_disable);
static inline bool cpu_smt_allowed(unsigned int cpu)
{
- if (cpu_smt_control == CPU_SMT_ENABLED)
+ if (cpu_smt_control == CPU_SMT_ENABLED && topology_smt_thread_allowed(cpu))
return true;
if (topology_is_primary_thread(cpu))
@@ -2273,6 +2273,12 @@ int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval)
for_each_online_cpu(cpu) {
if (topology_is_primary_thread(cpu))
continue;
+ /*
+ * Disable can be called with CPU_SMT_ENABLED when changing
+ * from a higher to lower number of SMT threads per core.
+ */
+ if (ctrlval == CPU_SMT_ENABLED && topology_smt_thread_allowed(cpu))
+ continue;
ret = cpu_down_maps_locked(cpu, CPUHP_OFFLINE);
if (ret)
break;
@@ -2307,6 +2313,8 @@ int cpuhp_smt_enable(void)
/* Skip online CPUs and CPUs on offline nodes */
if (cpu_online(cpu) || !node_online(cpu_to_node(cpu)))
continue;
+ if (!topology_smt_thread_allowed(cpu))
+ continue;
ret = _cpu_up(cpu, 0, CPUHP_ONLINE);
if (ret)
break;
--
2.40.1
Add support to the /sys/devices/system/cpu/smt/control interface for
enabling a specified number of SMT threads per core, including partial
SMT states where not all threads are brought online.
The current interface accepts "on" and "off", to enable either 1 or all
SMT threads per core.
This commit allows writing an integer, between 1 and the number of SMT
threads supported by the machine. Writing 1 is a synonym for "off", 2 or
more enables SMT with the specified number of threads.
When reading the file, if all threads are online "on" is returned, to
avoid changing behaviour for existing users. If some other number of
threads is online then the integer value is returned.
There is a hook which allows arch code to control how many threads per
core are supported. To retain the existing behaviour, the x86 hook only
supports 1 thread or all threads.
Signed-off-by: Michael Ellerman <[email protected]>
---
.../ABI/testing/sysfs-devices-system-cpu | 1 +
kernel/cpu.c | 39 ++++++++++++++++---
2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
index f54867cadb0f..3c4cfb59d495 100644
--- a/Documentation/ABI/testing/sysfs-devices-system-cpu
+++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
@@ -555,6 +555,7 @@ Description: Control Symmetric Multi Threading (SMT)
================ =========================================
"on" SMT is enabled
"off" SMT is disabled
+ "<N>" SMT is enabled with N threads per core.
"forceoff" SMT is force disabled. Cannot be changed.
"notsupported" SMT is not supported by the CPU
"notimplemented" SMT runtime toggling is not
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 72b9a03a4fef..aca23c7b4547 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -2497,7 +2497,7 @@ static ssize_t
__store_smt_control(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- int ctrlval, ret;
+ int ctrlval, ret, num_threads, orig_threads;
if (cpu_smt_control == CPU_SMT_FORCE_DISABLED)
return -EPERM;
@@ -2505,20 +2505,38 @@ __store_smt_control(struct device *dev, struct device_attribute *attr,
if (cpu_smt_control == CPU_SMT_NOT_SUPPORTED)
return -ENODEV;
- if (sysfs_streq(buf, "on"))
+ if (sysfs_streq(buf, "on")) {
ctrlval = CPU_SMT_ENABLED;
- else if (sysfs_streq(buf, "off"))
+ num_threads = cpu_smt_max_threads;
+ } else if (sysfs_streq(buf, "off")) {
ctrlval = CPU_SMT_DISABLED;
- else if (sysfs_streq(buf, "forceoff"))
+ num_threads = 1;
+ } else if (sysfs_streq(buf, "forceoff")) {
ctrlval = CPU_SMT_FORCE_DISABLED;
- else
+ num_threads = 1;
+ } else if (kstrtoint(buf, 10, &num_threads) == 0) {
+ if (num_threads == 1)
+ ctrlval = CPU_SMT_DISABLED;
+ else if (num_threads > 1 && topology_smt_threads_supported(num_threads))
+ ctrlval = CPU_SMT_ENABLED;
+ else
+ return -EINVAL;
+ } else {
return -EINVAL;
+ }
ret = lock_device_hotplug_sysfs();
if (ret)
return ret;
- if (ctrlval != cpu_smt_control) {
+ orig_threads = cpu_smt_num_threads;
+ cpu_smt_num_threads = num_threads;
+
+ if (num_threads > orig_threads) {
+ ret = cpuhp_smt_enable();
+ } else if (num_threads < orig_threads) {
+ ret = cpuhp_smt_disable(ctrlval);
+ } else if (ctrlval != cpu_smt_control) {
switch (ctrlval) {
case CPU_SMT_ENABLED:
ret = cpuhp_smt_enable();
@@ -2556,6 +2574,15 @@ static ssize_t control_show(struct device *dev,
{
const char *state = smt_states[cpu_smt_control];
+ /*
+ * If SMT is enabled but not all threads are enabled then show the
+ * number of threads. If all threads are enabled show "on". Otherwise
+ * show the state name.
+ */
+ if (cpu_smt_control == CPU_SMT_ENABLED &&
+ cpu_smt_num_threads != cpu_smt_max_threads)
+ return sysfs_emit(buf, "%d\n", cpu_smt_num_threads);
+
return snprintf(buf, PAGE_SIZE - 2, "%s\n", state);
}
--
2.40.1
Integrate with the generic SMT support, so that when a CPU is DLPAR
onlined it is brought up with the correct SMT mode.
Signed-off-by: Michael Ellerman <[email protected]>
---
arch/powerpc/platforms/pseries/hotplug-cpu.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 61fb7cb00880..e62835a12d73 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -398,6 +398,14 @@ static int dlpar_online_cpu(struct device_node *dn)
for_each_present_cpu(cpu) {
if (get_hard_smp_processor_id(cpu) != thread)
continue;
+
+ if (!topology_is_primary_thread(cpu)) {
+ if (cpu_smt_control != CPU_SMT_ENABLED)
+ break;
+ if (!topology_smt_thread_allowed(cpu))
+ break;
+ }
+
cpu_maps_update_done();
find_and_update_cpu_nid(cpu);
rc = device_online(get_cpu_device(cpu));
--
2.40.1
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 <[email protected]>
---
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)
--
2.40.1
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 Sat, Jun 10 2023 at 22:09, Thomas Gleixner wrote:
> On Thu, May 25 2023 at 01:56, Michael Ellerman wrote:
>> There is a hook which allows arch code to control how many threads per
>
> Can you please write out architecture in changelogs and comments?
>
> I know 'arch' is commonly used but while my brain parser tolerates
> 'arch_' prefixes it raises an exception on 'arch' in prose as 'arch' is
> a regular word with a completely different meaning. Changelogs and
> comments are not space constraint.
>
>> @@ -2505,20 +2505,38 @@ __store_smt_control(struct device *dev, struct device_attribute *attr,
>> if (cpu_smt_control == CPU_SMT_NOT_SUPPORTED)
>> return -ENODEV;
>>
>> - if (sysfs_streq(buf, "on"))
>> + if (sysfs_streq(buf, "on")) {
>> ctrlval = CPU_SMT_ENABLED;
>> - else if (sysfs_streq(buf, "off"))
>> + num_threads = cpu_smt_max_threads;
>> + } else if (sysfs_streq(buf, "off")) {
>> ctrlval = CPU_SMT_DISABLED;
>> - else if (sysfs_streq(buf, "forceoff"))
>> + num_threads = 1;
>> + } else if (sysfs_streq(buf, "forceoff")) {
>> ctrlval = CPU_SMT_FORCE_DISABLED;
>> - else
>> + num_threads = 1;
>> + } else if (kstrtoint(buf, 10, &num_threads) == 0) {
>> + if (num_threads == 1)
>> + ctrlval = CPU_SMT_DISABLED;
>> + else if (num_threads > 1 && topology_smt_threads_supported(num_threads))
Why does this not simply check cpu_smt_max_threads?
else if (num_threads > 1 && num_threads <= cpu_smt_max_threads)
cpu_smt_max_threads should have been established already, no?
Thanks,
tglx
On Thu, May 25 2023 at 01:56, Michael Ellerman wrote:
> There is a hook which allows arch code to control how many threads per
Can you please write out architecture in changelogs and comments?
I know 'arch' is commonly used but while my brain parser tolerates
'arch_' prefixes it raises an exception on 'arch' in prose as 'arch' is
a regular word with a completely different meaning. Changelogs and
comments are not space constraint.
> @@ -2505,20 +2505,38 @@ __store_smt_control(struct device *dev, struct device_attribute *attr,
> if (cpu_smt_control == CPU_SMT_NOT_SUPPORTED)
> return -ENODEV;
>
> - if (sysfs_streq(buf, "on"))
> + if (sysfs_streq(buf, "on")) {
> ctrlval = CPU_SMT_ENABLED;
> - else if (sysfs_streq(buf, "off"))
> + num_threads = cpu_smt_max_threads;
> + } else if (sysfs_streq(buf, "off")) {
> ctrlval = CPU_SMT_DISABLED;
> - else if (sysfs_streq(buf, "forceoff"))
> + num_threads = 1;
> + } else if (sysfs_streq(buf, "forceoff")) {
> ctrlval = CPU_SMT_FORCE_DISABLED;
> - else
> + num_threads = 1;
> + } else if (kstrtoint(buf, 10, &num_threads) == 0) {
> + if (num_threads == 1)
> + ctrlval = CPU_SMT_DISABLED;
> + else if (num_threads > 1 && topology_smt_threads_supported(num_threads))
> + ctrlval = CPU_SMT_ENABLED;
> + else
> + return -EINVAL;
> + } else {
> return -EINVAL;
> + }
>
> ret = lock_device_hotplug_sysfs();
> if (ret)
> return ret;
>
> - if (ctrlval != cpu_smt_control) {
> + orig_threads = cpu_smt_num_threads;
> + cpu_smt_num_threads = num_threads;
> +
> + if (num_threads > orig_threads) {
> + ret = cpuhp_smt_enable();
> + } else if (num_threads < orig_threads) {
> + ret = cpuhp_smt_disable(ctrlval);
> + } else if (ctrlval != cpu_smt_control) {
> switch (ctrlval) {
> case CPU_SMT_ENABLED:
> ret = cpuhp_smt_enable();
This switch case does not make sense anymore.
The only situation which reaches this is when the control value goes
from CPU_SMT_DISABLED to CPU_SMT_FORCE_DISABLED because that's not
changing the number of threads.
So something like this is completely sufficient:
if (num_threads > orig_threads)
ret = cpuhp_smt_enable();
else if (num_threads < orig_threads || ctrval == CPU_SMT_FORCE_DISABLED)
ret = cpuhp_smt_disable(ctrlval);
No?
Thanks,
tglx
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 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 Thu, May 25 2023 at 01:56, Michael Ellerman wrote:
> +/**
> + * topology_smt_threads_supported - Check if the given number of SMT threads
> + * is supported.
> + *
> + * @threads: The number of SMT threads.
> + */
> +bool topology_smt_threads_supported(unsigned int threads)
> +{
> + // Only support a single thread or all threads.
> + return threads == 1 || threads == smp_num_siblings;
> +}
You can make that a simple core function when cpu_smt_*_threads is
consistent along the lines of my previous reply.
static bool cpu_smt_num_threads_valid(unsigned int threads)
{
if (IS_ENABLED(CONFIG_SMT_NUM_THREADS_DYNAMIC))
return threads >= 1 && threads <= cpu_smt_max_threads;
return threads == 1 || threads == cpu_smt_max_threads;
}
Or something like that.
Thanks,
tglx
On Thu, May 25 2023 at 01:56, Michael Ellerman wrote:
> A subsequent patch will enable partial SMT states, ie. when not all SMT
> threads are brought online.
Nitpick. I stumbled over this 'subsequent patch' theme a couple of times
now because it's very similar to the 'This patch does' phrase.
Just explain what you want to achieve at the end.
> #else
> #define topology_max_packages() (1)
> static inline int
> @@ -159,6 +160,7 @@ static inline int topology_max_smt_threads(void) { return 1; }
> static inline bool topology_is_primary_thread(unsigned int cpu) { return true; }
> static inline bool topology_smt_supported(void) { return false; }
> static inline bool topology_smt_threads_supported(unsigned int threads) { return false; }
> +static inline bool topology_smt_thread_allowed(unsigned int cpu) { return false; }
Not all these functions need a !SMP stub. Think about the context in
which they are called. There is probably precedence for pointless ones,
but that does not make an argument to add more.
> +/**
> + * topology_smt_thread_allowed - When enabling SMT check whether this particular
> + * CPU thread is allowed to be brought online.
> + * @cpu: CPU to check
> + */
> +bool topology_smt_thread_allowed(unsigned int cpu)
> +{
> + /*
> + * No extra logic s required here to support different thread values
> + * because threads will always == 1 or smp_num_siblings because of
> + * topology_smt_threads_supported().
> + */
> + return true;
> +}
> +
As x86 only supoorts the on/off model there is no need for this function
if you pick up the CONFIG_SMT_NUM_THREADS_DYNAMIC idea.
You still need something like that for your PPC use case, but that
reduces the overall impact, right?
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
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