Re: [PATCH v9 1/4] cpu/SMT: Provide a default topology_is_primary_thread()

[Dietmar Eggemann <dietmar.eggemann@arm.com>]

On 18/11/2024 11:50, Yicong Yang wrote:
> On 2024/11/15 17:42, Pierre Gondois wrote:
>> Hello Yicong,
>>
>>
>> On 11/14/24 15:11, Yicong Yang wrote:
>>> From: Yicong Yang <yangyicong@hisilicon.com>

[...]

>>>   diff --git a/include/linux/topology.h b/include/linux/topology.h
>>> index 52f5850730b3..b8e860276518 100644
>>> --- a/include/linux/topology.h
>>> +++ b/include/linux/topology.h
>>> @@ -240,6 +240,26 @@ 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.
>>> +     *
>>> +     * The sibling cpumask of an offline CPU contains always the CPU
>>> +     * itself.
>>
>> As Thomas suggested, would it be possible to check it for other
>> architectures ?
>> For instance for loongarch at arch/loongarch/kernel/smp.c,
>> clear_cpu_sibling_map() seems to completely clear &cpu_sibling_map[cpu]
>> when a CPU is put offline. This would make topology_sibling_cpumask(cpu)
>> to be empty and cpu_bootable() return false if the CPU never booted before.
>>
> 
> cpu_bootable() only affects architectures select HOTPLUG_SMT, otherwise it'll always
> return true. Since x86 and powerpc have their own illustration of primary thread and
> have an override version of this funciton, arm64 is the only user now by this patchset.
> We have this guarantee for arm64 and also for other architectures using arch_topology.c
> (see clear_cpu_topology()). So if loogarch has a different implementation, they
> should implement a topology_is_primary_thread() variant to support HOTPLUG_SMT.

I also stumbled over this sentence.

drivers/base/arch_topology.c:

void clear_cpu_topology(int cpu)                   (2)

  ...
  cpumask_set_cpu(cpu, &cpu_topo->thread_sibling)  (4)

void __init reset_cpu_topology(void)               (1)

  for_each_possible_cpu(cpu)

    ...
    clear_cpu_topology(cpu)                        (2)

#if defined(CONFIG_ARM64) || defined(CONFIG_RISCV) (3)
void __init init_cpu_topology(void)

  reset_cpu_topology()                             (1)
  ...

Does this mean the default implementation relies on (4), i.e. is only
valid for arm64 and riscv? (3)
Do all the other archs then have to overwrite the default implementation
 (like x86 and powerpc) if they want to implement CONFIG_HOTPLUG_SMT?

[...]