| Message ID | 20241030125415.18994-2-yangyicong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support SMT control on arm64 | expand |
On Wed, Oct 30 2024 at 20:54, Yicong Yang wrote: > > +#ifndef topology_is_primary_thread > +#define topology_is_primary_thread topology_is_primary_thread Please do not glue defines and functions together w/o a newline in between. > +static inline bool topology_is_primary_thread(unsigned int cpu) > +{ > + /* > + * On SMT hotplug the primary thread of the SMT won't be disabled. > + * Architectures do have a special primary thread (e.g. x86) need > + * to override this function. Otherwise just make the first thread > + * in the SMT as the primary thread. > + */ > + return cpu == cpumask_first(topology_sibling_cpumask(cpu)); How is that supposed to work? Assume both siblings are offline, then the sibling mask is empty and you can't boot the CPU anymore. Thanks, tglx
On 2024/10/30 22:55, Thomas Gleixner wrote: > On Wed, Oct 30 2024 at 20:54, Yicong Yang wrote: >> >> +#ifndef topology_is_primary_thread >> +#define topology_is_primary_thread topology_is_primary_thread > > Please do not glue defines and functions together w/o a newline in between. > sure, will add a newline here. >> +static inline bool topology_is_primary_thread(unsigned int cpu) >> +{ >> + /* >> + * On SMT hotplug the primary thread of the SMT won't be disabled. >> + * Architectures do have a special primary thread (e.g. x86) need >> + * to override this function. Otherwise just make the first thread >> + * in the SMT as the primary thread. >> + */ >> + return cpu == cpumask_first(topology_sibling_cpumask(cpu)); > > How is that supposed to work? Assume both siblings are offline, then the > sibling mask is empty and you can't boot the CPU anymore. > For architectures' using arch_topology, topology_sibling_cpumask() will at least contain the tested CPU itself. This is initialized in drivers/base/arch_topology.c:reset_cpu_topology(). So it won't be empty here. Besides we don't need to check topology_is_primary_thread() at boot time: -> cpu_up(cpu) cpu_bootable() if (cpu_smt_control == CPU_SMT_ENABLED && cpu_smt_thread_allowed(cpu)) // will always return true if !CONFIG_SMT_NUM_THREADS_DYNAMIC return true; // we'll always return here and @cpu is always bootable Also tested fine in practice. Thanks.
On Thu, Oct 31 2024 at 20:17, Yicong Yang wrote: > On 2024/10/30 22:55, Thomas Gleixner wrote: >>> +static inline bool topology_is_primary_thread(unsigned int cpu) >>> +{ >>> + /* >>> + * On SMT hotplug the primary thread of the SMT won't be disabled. >>> + * Architectures do have a special primary thread (e.g. x86) need >>> + * to override this function. Otherwise just make the first thread >>> + * in the SMT as the primary thread. >>> + */ >>> + return cpu == cpumask_first(topology_sibling_cpumask(cpu)); >> >> How is that supposed to work? Assume both siblings are offline, then the >> sibling mask is empty and you can't boot the CPU anymore. >> > > For architectures' using arch_topology, topology_sibling_cpumask() will at least > contain the tested CPU itself. This is initialized in > drivers/base/arch_topology.c:reset_cpu_topology(). So it won't be > empty here. Fair enough. Can you please expand the comment and say: The sibling cpumask of a offline CPU contains always the CPU itself. > Besides we don't need to check topology_is_primary_thread() at boot time: > -> cpu_up(cpu) > cpu_bootable() > if (cpu_smt_control == CPU_SMT_ENABLED && > cpu_smt_thread_allowed(cpu)) // will always return true if !CONFIG_SMT_NUM_THREADS_DYNAMIC > return true; // we'll always return here and @cpu is always bootable cpu_smt_control is not guaranteed to have CPU_SMT_ENABLED state, so this argument is bogus. > Also tested fine in practice. I've heard that song before. What matters is not what you tested. What matters is whether the code is correct _and_ understandable. Thanks, tglx
On 2024/10/31 21:33, Thomas Gleixner wrote: > On Thu, Oct 31 2024 at 20:17, Yicong Yang wrote: >> On 2024/10/30 22:55, Thomas Gleixner wrote: >>>> +static inline bool topology_is_primary_thread(unsigned int cpu) >>>> +{ >>>> + /* >>>> + * On SMT hotplug the primary thread of the SMT won't be disabled. >>>> + * Architectures do have a special primary thread (e.g. x86) need >>>> + * to override this function. Otherwise just make the first thread >>>> + * in the SMT as the primary thread. >>>> + */ >>>> + return cpu == cpumask_first(topology_sibling_cpumask(cpu)); >>> >>> How is that supposed to work? Assume both siblings are offline, then the >>> sibling mask is empty and you can't boot the CPU anymore. >>> >> >> For architectures' using arch_topology, topology_sibling_cpumask() will at least >> contain the tested CPU itself. This is initialized in >> drivers/base/arch_topology.c:reset_cpu_topology(). So it won't be >> empty here. > > Fair enough. Can you please expand the comment and say: > > The sibling cpumask of a offline CPU contains always the CPU > itself. > Sure, will make it clear. >> Besides we don't need to check topology_is_primary_thread() at boot time: >> -> cpu_up(cpu) >> cpu_bootable() >> if (cpu_smt_control == CPU_SMT_ENABLED && >> cpu_smt_thread_allowed(cpu)) // will always return true if !CONFIG_SMT_NUM_THREADS_DYNAMIC >> return true; // we'll always return here and @cpu is always bootable > > cpu_smt_control is not guaranteed to have CPU_SMT_ENABLED state, so this > argument is bogus. > sorry for didn't explain all the cases here. For cpu_sm_control == {CPU_SMT_ENABLED, CPU_SMT_NOT_SUPPORTED, CPU_SMT_NOT_IMPLEMENTED}, all the cpu's bootable and we won't check topology_is_primary_thread(). static inline bool cpu_bootable(unsigned int cpu) { if (cpu_smt_control == CPU_SMT_ENABLED && cpu_smt_thread_allowed(cpu)) return true; /* All CPUs are bootable if controls are not configured */ if (cpu_smt_control == CPU_SMT_NOT_IMPLEMENTED) return true; /* All CPUs are bootable if CPU is not SMT capable */ if (cpu_smt_control == CPU_SMT_NOT_SUPPORTED) return true; if (topology_is_primary_thread(cpu)) // Will be true for all the CPUs when thread sibling's not built // Only true for primary thread if thread sibling's updated // thread sibling will be updated once the CPU's bootup, for arm64 // in secondary_start_kernel() return true; return !cpumask_test_cpu(cpu, &cpus_booted_once_mask); // Also be updated once the CPU's bootup, in // secondary_start_kernel() for arm64 // Will return false in the second check of // cpu_bootable() in the call chain below } For cpu_smt_control == {CPUS_SMT_DISABLED, CPU_SMT_FORCE_DISABLED} if user specified the boot option "nosmt" or "nosmt=force", it'll be a bit more complex. For a non-primary thread CPU, cpu_bootable() will return true and it'll be boot. Then after thread sibling's built cpu_bootable() will be checked secondly it the cpuhp callbacks, since it'll return false then and we'll roll back and offline it. // for a non-primary thread CPU, system boot with "nosmt" or "nosmt=force" -> cpu_up() cpu_bootable() -> true, since the thread sibling mask only coutains CPU itself [...] cpuhp_bringup_ap() bringup_wait_for_ap_online() if (!cpu_bootable(cpu)) // target CPU has been bringup, thread sibling mask's updated // then this non-primay thread won't be bootable in this case return -ECANCELED // roll back and offline this CPU Thanks.
On Fri, Nov 01 2024 at 11:18, Yicong Yang wrote: > On 2024/10/31 21:33, Thomas Gleixner wrote: >> cpu_smt_control is not guaranteed to have CPU_SMT_ENABLED state, so this >> argument is bogus. >> > sorry for didn't explain all the cases here. > > For cpu_sm_control == {CPU_SMT_ENABLED, CPU_SMT_NOT_SUPPORTED, CPU_SMT_NOT_IMPLEMENTED}, > all the cpu's bootable and we won't check topology_is_primary_thread(). You don't have to copy the code to me. I'm familiar with it. All I need is a proper explanation why your topology_is_primary_thread() implementation is correct under all circumstances. Thanks, tglx
On 10/31/24 13:17, Yicong Yang wrote: > On 2024/10/30 22:55, Thomas Gleixner wrote: >> On Wed, Oct 30 2024 at 20:54, Yicong Yang wrote: >>> >>> +#ifndef topology_is_primary_thread >>> +#define topology_is_primary_thread topology_is_primary_thread >> >> Please do not glue defines and functions together w/o a newline in between. >> > > sure, will add a newline here. > >>> +static inline bool topology_is_primary_thread(unsigned int cpu) >>> +{ >>> + /* >>> + * On SMT hotplug the primary thread of the SMT won't be disabled. >>> + * Architectures do have a special primary thread (e.g. x86) need >>> + * to override this function. Otherwise just make the first thread >>> + * in the SMT as the primary thread. >>> + */ >>> + return cpu == cpumask_first(topology_sibling_cpumask(cpu)); >> >> How is that supposed to work? Assume both siblings are offline, then the >> sibling mask is empty and you can't boot the CPU anymore. >> > > For architectures' using arch_topology, topology_sibling_cpumask() will at least > contain the tested CPU itself. This is initialized in > drivers/base/arch_topology.c:reset_cpu_topology(). So it won't be empty here. > > Besides we don't need to check topology_is_primary_thread() at boot time: > -> cpu_up(cpu) > cpu_bootable() > if (cpu_smt_control == CPU_SMT_ENABLED && > cpu_smt_thread_allowed(cpu)) // will always return true if !CONFIG_SMT_NUM_THREADS_DYNAMIC > return true; // we'll always return here and @cpu is always bootable > > Also tested fine in practice. > > Thanks. > > FWIW, I also tested the case where: - setting maxcpus=1 in the kernel cmdline to have CPUs that never booted - setting smt to off: 'echo off > /sys/devices/system/cpu/smt/control' and effectively the primary CPUs can boot and secondary CPUs can't, so it works as expected.
diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h index 16bacfe8c7a2..da15b5efe807 100644 --- a/arch/powerpc/include/asm/topology.h +++ b/arch/powerpc/include/asm/topology.h @@ -152,6 +152,7 @@ static inline bool topology_is_primary_thread(unsigned int cpu) { return cpu == cpu_first_thread_sibling(cpu); } +#define topology_is_primary_thread topology_is_primary_thread static inline bool topology_smt_thread_allowed(unsigned int cpu) { diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h index aef70336d624..3a7c18851016 100644 --- a/arch/x86/include/asm/topology.h +++ b/arch/x86/include/asm/topology.h @@ -219,11 +219,11 @@ static inline bool topology_is_primary_thread(unsigned int cpu) { return cpumask_test_cpu(cpu, cpu_primary_thread_mask); } +#define topology_is_primary_thread topology_is_primary_thread #else /* CONFIG_SMP */ static inline int topology_phys_to_logical_pkg(unsigned int pkg) { return 0; } static inline int topology_max_smt_threads(void) { return 1; } -static inline bool topology_is_primary_thread(unsigned int cpu) { return true; } static inline unsigned int topology_amd_nodes_per_pkg(void) { return 1; } #endif /* !CONFIG_SMP */ diff --git a/include/linux/topology.h b/include/linux/topology.h index 52f5850730b3..9c9f6b9f3462 100644 --- a/include/linux/topology.h +++ b/include/linux/topology.h @@ -240,6 +240,20 @@ static inline const struct cpumask *cpu_smt_mask(int cpu) } #endif +#ifndef topology_is_primary_thread +#define topology_is_primary_thread topology_is_primary_thread +static inline bool topology_is_primary_thread(unsigned int cpu) +{ + /* + * On SMT hotplug the primary thread of the SMT won't be disabled. + * Architectures do have a special primary thread (e.g. x86) need + * to override this function. Otherwise just make the first thread + * in the SMT as the primary thread. + */ + return cpu == cpumask_first(topology_sibling_cpumask(cpu)); +} +#endif + static inline const struct cpumask *cpu_cpu_mask(int cpu) { return cpumask_of_node(cpu_to_node(cpu));