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

[Yicong Yang <yangyicong@huawei.com>]

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.