linux-kernel - Re: [PATCH v7 1/4] cpu/SMT: Provide a default topology_is_primary

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5617934d-45ac-509a-510e-d96d52a2ebf9@huawei.com>
Date: Fri, 1 Nov 2024 11:18:33 +0800
From: Yicong Yang <yangyicong@...wei.com>
To: Thomas Gleixner <tglx@...utronix.de>, <catalin.marinas@....com>,
	<will@...nel.org>, <sudeep.holla@....com>, <peterz@...radead.org>,
	<mpe@...erman.id.au>, <linux-arm-kernel@...ts.infradead.org>,
	<mingo@...hat.com>, <bp@...en8.de>, <dave.hansen@...ux.intel.com>,
	<pierre.gondois@....com>, <dietmar.eggemann@....com>
CC: <yangyicong@...ilicon.com>, <linuxppc-dev@...ts.ozlabs.org>,
	<x86@...nel.org>, <linux-kernel@...r.kernel.org>, <morten.rasmussen@....com>,
	<msuchanek@...e.de>, <gregkh@...uxfoundation.org>, <rafael@...nel.org>,
	<jonathan.cameron@...wei.com>, <prime.zeng@...ilicon.com>,
	<linuxarm@...wei.com>, <xuwei5@...wei.com>, <guohanjun@...wei.com>
Subject: Re: [PATCH v7 1/4] cpu/SMT: Provide a default
 topology_is_primary_thread()

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.