| Message ID | 20241015021841.35713-3-yangyicong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support SMT control on arm64 | expand |
Hi Yicong, kernel test robot noticed the following build warnings: [auto build test WARNING on arm64/for-next/core] [also build test WARNING on powerpc/next powerpc/fixes driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master tip/x86/core v6.12-rc3 next-20241015] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Yicong-Yang/cpu-SMT-Provide-a-default-topology_is_primary_thread/20241015-102147 base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core patch link: https://lore.kernel.org/r/20241015021841.35713-3-yangyicong%40huawei.com patch subject: [PATCH v6 2/4] arch_topology: Support SMT control for OF based system config: arm-randconfig-001-20241016 (https://download.01.org/0day-ci/archive/20241016/202410161038.WVTopMNt-lkp@intel.com/config) compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241016/202410161038.WVTopMNt-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202410161038.WVTopMNt-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/base/arch_topology.c:32:21: warning: 'max_smt_thread_num' defined but not used [-Wunused-variable] 32 | static unsigned int max_smt_thread_num; | ^~~~~~~~~~~~~~~~~~ vim +/max_smt_thread_num +32 drivers/base/arch_topology.c 28 29 static DEFINE_PER_CPU(struct scale_freq_data __rcu *, sft_data); 30 static struct cpumask scale_freq_counters_mask; 31 static bool scale_freq_invariant; > 32 static unsigned int max_smt_thread_num; 33 DEFINE_PER_CPU(unsigned long, capacity_freq_ref) = 1; 34 EXPORT_PER_CPU_SYMBOL_GPL(capacity_freq_ref); 35
Hi Yicong, kernel test robot noticed the following build warnings: [auto build test WARNING on arm64/for-next/core] [also build test WARNING on powerpc/next powerpc/fixes driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master tip/x86/core v6.12-rc3 next-20241015] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Yicong-Yang/cpu-SMT-Provide-a-default-topology_is_primary_thread/20241015-102147 base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core patch link: https://lore.kernel.org/r/20241015021841.35713-3-yangyicong%40huawei.com patch subject: [PATCH v6 2/4] arch_topology: Support SMT control for OF based system config: arm-defconfig (https://download.01.org/0day-ci/archive/20241016/202410161302.HjmSau6j-lkp@intel.com/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241016/202410161302.HjmSau6j-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202410161302.HjmSau6j-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/base/arch_topology.c:32:21: warning: unused variable 'max_smt_thread_num' [-Wunused-variable] static unsigned int max_smt_thread_num; ^ 1 warning generated. vim +/max_smt_thread_num +32 drivers/base/arch_topology.c 28 29 static DEFINE_PER_CPU(struct scale_freq_data __rcu *, sft_data); 30 static struct cpumask scale_freq_counters_mask; 31 static bool scale_freq_invariant; > 32 static unsigned int max_smt_thread_num; 33 DEFINE_PER_CPU(unsigned long, capacity_freq_ref) = 1; 34 EXPORT_PER_CPU_SYMBOL_GPL(capacity_freq_ref); 35
Hello Yicong, On 10/15/24 04:18, Yicong Yang wrote: > From: Yicong Yang <yangyicong@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 <yangyicong@hisilicon.com> > --- > drivers/base/arch_topology.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > index 75fcb75d5515..5eed864df5e6 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> > @@ -28,6 +29,7 @@ > static DEFINE_PER_CPU(struct scale_freq_data __rcu *, sft_data); > static struct cpumask scale_freq_counters_mask; > static bool scale_freq_invariant; > +static unsigned int max_smt_thread_num; > DEFINE_PER_CPU(unsigned long, capacity_freq_ref) = 1; > EXPORT_PER_CPU_SYMBOL_GPL(capacity_freq_ref); > > @@ -561,6 +563,17 @@ static int __init parse_core(struct device_node *core, int package_id, > i++; > } while (1); > > + if (max_smt_thread_num < i) > + max_smt_thread_num = i; Shouldn't the conditions above/below be inverted ? I.e. (max_smt_thread_num != i) should never be true if there is max_smt_thread_num = i; just before > + > + /* > + * 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"); > + > cpu = get_cpu_for_node(core); > if (cpu >= 0) { > if (!leaf) { > @@ -673,6 +686,14 @@ 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. 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) > + cpu_smt_set_num_threads(max_smt_thread_num, max_smt_thread_num); > + > return ret; > } >
On 2024/10/23 23:43, Pierre Gondois wrote: > Hello Yicong, > > On 10/15/24 04:18, Yicong Yang wrote: >> From: Yicong Yang <yangyicong@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 <yangyicong@hisilicon.com> >> --- >> drivers/base/arch_topology.c | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c >> index 75fcb75d5515..5eed864df5e6 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> >> @@ -28,6 +29,7 @@ >> static DEFINE_PER_CPU(struct scale_freq_data __rcu *, sft_data); >> static struct cpumask scale_freq_counters_mask; >> static bool scale_freq_invariant; >> +static unsigned int max_smt_thread_num; >> DEFINE_PER_CPU(unsigned long, capacity_freq_ref) = 1; >> EXPORT_PER_CPU_SYMBOL_GPL(capacity_freq_ref); >> @@ -561,6 +563,17 @@ static int __init parse_core(struct device_node *core, int package_id, >> i++; >> } while (1); >> + if (max_smt_thread_num < i) >> + max_smt_thread_num = i; > > Shouldn't the conditions above/below be inverted ? > I.e. (max_smt_thread_num != i) should never be true if there is > max_smt_thread_num = i; > just before > you're right. will get this fixed. thanks for catching this. Thanks. >> + >> + /* >> + * 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"); >> + >> cpu = get_cpu_for_node(core); >> if (cpu >= 0) { >> if (!leaf) { >> @@ -673,6 +686,14 @@ 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. 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) >> + cpu_smt_set_num_threads(max_smt_thread_num, max_smt_thread_num); >> + >> return ret; >> } >> > > .
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index 75fcb75d5515..5eed864df5e6 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> @@ -28,6 +29,7 @@ static DEFINE_PER_CPU(struct scale_freq_data __rcu *, sft_data); static struct cpumask scale_freq_counters_mask; static bool scale_freq_invariant; +static unsigned int max_smt_thread_num; DEFINE_PER_CPU(unsigned long, capacity_freq_ref) = 1; EXPORT_PER_CPU_SYMBOL_GPL(capacity_freq_ref); @@ -561,6 +563,17 @@ static int __init parse_core(struct device_node *core, int package_id, i++; } while (1); + if (max_smt_thread_num < i) + max_smt_thread_num = i; + + /* + * 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"); + cpu = get_cpu_for_node(core); if (cpu >= 0) { if (!leaf) { @@ -673,6 +686,14 @@ 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. 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) + cpu_smt_set_num_threads(max_smt_thread_num, max_smt_thread_num); + return ret; }