| Message ID | 20241030125415.18994-2-yangyicong@huawei.com (mailing list archive) |
|---|---|
| State | New |
| 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
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));