linux-kernel - Re: [PATCH v9 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] [day] [month] [year] [list]
Message-ID: <427bd639-33c3-47e4-9e83-68c428eb1a7d@arm.com>
Date: Fri, 15 Nov 2024 10:42:31 +0100
From: Pierre Gondois <pierre.gondois@....com>
To: Yicong Yang <yangyicong@...wei.com>
Cc: linuxppc-dev@...ts.ozlabs.org, dietmar.eggemann@....com, x86@...nel.org,
 bp@...en8.de, mingo@...hat.com, linux-arm-kernel@...ts.infradead.org,
 mpe@...erman.id.au, peterz@...radead.org, tglx@...utronix.de,
 sudeep.holla@....com, catalin.marinas@....com, will@...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, yangyicong@...ilicon.com,
 xuwei5@...wei.com, guohanjun@...wei.com, dave.hansen@...ux.intel.com
Subject: Re: [PATCH v9 1/4] cpu/SMT: Provide a default
 topology_is_primary_thread()

Hello Yicong,


On 11/14/24 15:11, Yicong Yang wrote:
> From: Yicong Yang <yangyicong@...ilicon.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@...ilicon.com>
> ---
>   arch/powerpc/include/asm/topology.h |  1 +
>   arch/x86/include/asm/topology.h     |  2 +-
>   include/linux/topology.h            | 20 ++++++++++++++++++++
>   3 files changed, 22 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 92f3664dd933..d84d9b6d8678 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..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.

Personal note:
cpu_bootable() is called from an already online CPU:
cpu_bootable (kernel/cpu.c:678)
cpu_up (kernel/cpu.c:1722 kernel/cpu.c:1702)
bringup_nonboot_cpus (kernel/cpu.c:1793 kernel/cpu.c:1901)
smp_init (./include/linux/bitmap.h:445 ./include/linux/nodemask.h:241 ./include/linux/nodemask.h:438 kernel/smp.c:1011)
kernel_init_freeable (init/main.c:1573)
kernel_init (init/main.c:1473)
ret_from_fork (arch/arm64/kernel/entry.S:861)

store_cpu_topology() for arm64 is called from the booting CPU:
store_cpu_topology (drivers/base/arch_topology.c:921)
secondary_start_kernel (arch/arm64/kernel/smp.c:251)
__secondary_switched (arch/arm64/kernel/head.S:418)

> +	 */
> +	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));