On Fri, 20 Dec 2024 15:53:11 +0800 Yicong Yang <yangyic...@huawei.com> 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 > > If a system have more than one SMT thread number the 2) may > not handle it well, since there're multiple thread numbers in the > system and 2) only accept 1/max_thread_number. So issue a warning > to notify the users if such system detected. > > [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> Hi Yicong, Apologies that I'm late to the game on this one. A few comments inline. Only important one is whether to bail out early on error from parse_cluster() Thanks, Jonathan > --- > drivers/base/arch_topology.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > index 3ebe77566788..9e81060144c7 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 */ > +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,17 @@ 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"); > + > + if (max_smt_thread_num < i) > + max_smt_thread_num = i; Maybe more self documenting if you use min()? I'm not sure... max_smt_thread_num = min(max_smt_thread_num, i); > + > cpu = get_cpu_for_node(core); > if (cpu >= 0) { > if (!leaf) { > @@ -677,6 +693,16 @@ static int __init parse_socket(struct device_node > *socket) > if (!has_socket) > ret = parse_cluster(socket, 0, -1, 0); Is it appropriate to check ret before setting num threads? if (!has_socket) { ret = parse_cluster(socket, 0, -1, 0); if (ret) return ret; } ... return 0; > > + /* > + * Notify the CPU framework of the SMT support. Initialize the > + * max_smt_thread_num to 1 if no SMT support detected. 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. > + */ > + if (!max_smt_thread_num) > + max_smt_thread_num = 1; > + > + cpu_smt_set_num_threads(max_smt_thread_num, max_smt_thread_num); Trivial but I'd put a blank line here. > return ret; > } >
