On 2025/2/28 19:11, Dietmar Eggemann wrote: > On 18/02/2025 15:10, Yicong Yang wrote: >> From: Yicong Yang <yangyic...@hisilicon.com> >> >> On building the topology from the devicetree, we've already >> gotten the SMT thread number of each core. Update the largest >> SMT thread number and enable the SMT control by the end of >> topology parsing. >> >> The core's SMT control provides two interface to the users [1]: >> 1) enable/disable SMT by writing on/off >> 2) enable/disable SMT by writing thread number 1/max_thread_number > > 1/max_thread_number stands for '1 or max_thread_number', right ? > > Aren't the two interfaces: > > (a) /sys/devices/system/cpu/smt/active > (b) /sys/devices/system/cpu/smt/control > > and you write 1) or 2) (or 'forceoff') into (b)?
yes you're correct. "active" is a RO file for status only so not for this interface. Let me explicitly mention the /sys/devices/system/cpu/smt/control here in the commit. > >> If a system have more than one SMT thread number the 2) may > > s/have/has > >> not handle it well, since there're multiple thread numbers in the > > multiple thread numbers other than 1, right? according to the pr_warn_once() we implemented below it also includes the case where the system have one type of SMT cores and non-SMT cores (the thread number is 1): - 1 thread - X (!= 1) threads Discussion made in [1] and I thought we have agreement (hope I understood correctly) that all the asymmetric cases need to notify. Do you and Sudeep think we should not warn in such case? [1] https://lore.kernel.org/linux-arm-kernel/10082e64-b00a-a30b-b9c5-1401a54f6...@huawei.com/ > >> system and 2) only accept 1/max_thread_number. So issue a warning >> to notify the users if such system detected. > > This paragraph seems to be about heterogeneous systems. Maybe mention this? > > Heterogeneous system with SMT only on a subset of cores (like Intel > Hybrid): This one works (N threads per core with N=1 and N=2) just fine. > > But on Arm64 (default) we would still see: > > [0.075782] Heterogeneous SMT topology is partly supported by SMT control > more clearer, will add it. Thanks. >> [1] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-devices-system-cpu#n542 >> >> Signed-off-by: Yicong Yang <yangyic...@hisilicon.com> >> --- >> drivers/base/arch_topology.c | 27 +++++++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c >> index 3ebe77566788..23f425a9d77a 100644 >> --- a/drivers/base/arch_topology.c >> +++ b/drivers/base/arch_topology.c >> @@ -11,6 +11,7 @@ >> #include <linux/cleanup.h> >> #include <linux/cpu.h> >> #include <linux/cpufreq.h> >> +#include <linux/cpu_smt.h> >> #include <linux/device.h> >> #include <linux/of.h> >> #include <linux/slab.h> >> @@ -506,6 +507,10 @@ core_initcall(free_raw_capacity); >> #endif >> >> #if defined(CONFIG_ARM64) || defined(CONFIG_RISCV) >> + >> +/* Maximum SMT thread number detected used to enable the SMT control */ > > maybe shorter ? > > /* used to enable SMT control */ > sure. >> +static unsigned int max_smt_thread_num; >> + >> /* >> * This function returns the logic cpu number of the node. >> * There are basically three kinds of return values: >> @@ -565,6 +570,16 @@ static int __init parse_core(struct device_node *core, >> int package_id, >> i++; >> } while (1); >> >> + /* >> + * If max_smt_thread_num has been initialized and doesn't match >> + * the thread number of this entry, then the system has >> + * heterogeneous SMT topology. >> + */ >> + if (max_smt_thread_num && max_smt_thread_num != i) >> + pr_warn_once("Heterogeneous SMT topology is partly supported by >> SMT control\n"); >> + >> + max_smt_thread_num = max_t(unsigned int, max_smt_thread_num, i); >> + >> cpu = get_cpu_for_node(core); >> if (cpu >= 0) { >> if (!leaf) { >> @@ -677,6 +692,18 @@ static int __init parse_socket(struct device_node >> *socket) >> if (!has_socket) >> ret = parse_cluster(socket, 0, -1, 0); >> >> + /* >> + * Notify the CPU framework of the SMT support. Initialize the >> + * max_smt_thread_num to 1 if no SMT support detected or failed >> + * to parse the topology. A thread number of 1 can be handled by >> + * the framework so we don't need to check max_smt_thread_num to >> + * see we support SMT or not. > > Not sure whether the last sentence is needed here? > We always need to call cpu_smt_set_num_threads() to notify the framework of the thread number even if SMT is not supported. In which case the thread number is 1 but the framework can handle this well. I worry readers may get confused for notifying a thread number of 1 so add this comment this. Will get rid of this if thought redundant. Thanks.
