From: Yicong Yang <yangyicong@hisilicon.com>
Currently if architectures want to support HOTPLUG_SMT they need to
provide a topology_is_primary_thread() telling the framework which
thread in the SMT cannot offline. However arm64 doesn't have a
restriction on which thread in the SMT cannot offline, a simplest
choice is that just make 1st thread as the "primary" thread. So
just make this as the default implementation in the framework and
let architectures like x86 that have special primary thread to
override this function (which they've already done).
There's no need to provide a stub function if !CONFIG_SMP or
!CONFIG_HOTPLUG_SMP. In such case the testing CPU is already
the 1st CPU in the SMT so it's always the primary thread.
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
arch/powerpc/include/asm/topology.h | 1 +
arch/x86/include/asm/topology.h | 2 +-
include/linux/topology.h | 14 ++++++++++++++
3 files changed, 16 insertions(+), 1 deletion(-)
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));
--
2.24.0On 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 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.
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
© 2016 - 2024 Red Hat, Inc.