Re: [PATCH v3 1/2] x86/smpboot: Decrapify build_sched_topology()

[Peter Zijlstra <peterz@infradead.org>]

On Wed, Jun 25, 2025 at 11:45:49AM +0800, Li Chen wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The #ifdeffery and the initializers in build_sched_topology() are just
> disgusting. The SCHED_SMT #ifdef is also pointless because SCHED_SMT is
> unconditionally enabled when SMP is enabled.

On x86, but not across all archs. Yes this is x86 code, but how is one
supposed to keep all that nonsense straight in their head ;-)

> Statically initialize the domain levels in the topology array and let
> build_sched_topology() invalidate the package domain level when NUMA in
> package is available.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/kernel/smpboot.c | 45 +++++++++++++++------------------------
>  1 file changed, 17 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index fc78c2325fd29..7d202f9785362 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -478,43 +478,32 @@ static int x86_cluster_flags(void)
>   */
>  static bool x86_has_numa_in_package;
>  
> -static struct sched_domain_topology_level x86_topology[6];
> +#define DOMAIN(maskfn, flagsfn, dname) { .mask = maskfn, .sd_flags = flagsfn, .name = #dname }
>  
> -static void __init build_sched_topology(void)
> -{
> -	int i = 0;
> -
> -#ifdef CONFIG_SCHED_SMT
> -	x86_topology[i++] = (struct sched_domain_topology_level){
> -		cpu_smt_mask, cpu_smt_flags, SD_INIT_NAME(SMT)
> -	};
> -#endif
> +static struct sched_domain_topology_level x86_topology[] = {
> +	DOMAIN(cpu_smt_mask, cpu_smt_flags, SMT),
>  #ifdef CONFIG_SCHED_CLUSTER
> -	x86_topology[i++] = (struct sched_domain_topology_level){
> -		cpu_clustergroup_mask, x86_cluster_flags, SD_INIT_NAME(CLS)
> -	};
> +	DOMAIN(cpu_clustergroup_mask, x86_cluster_flags, CLS),
>  #endif
>  #ifdef CONFIG_SCHED_MC
> -	x86_topology[i++] = (struct sched_domain_topology_level){
> -		cpu_coregroup_mask, x86_core_flags, SD_INIT_NAME(MC)
> -	};
> +	DOMAIN(cpu_coregroup_mask, x86_core_flags, MC),
>  #endif
> -	/*
> -	 * When there is NUMA topology inside the package skip the PKG domain
> -	 * since the NUMA domains will auto-magically create the right spanning
> -	 * domains based on the SLIT.
> -	 */
> -	if (!x86_has_numa_in_package) {
> -		x86_topology[i++] = (struct sched_domain_topology_level){
> -			cpu_cpu_mask, x86_sched_itmt_flags, SD_INIT_NAME(PKG)
> -		};
> -	}
> +	DOMAIN(cpu_cpu_mask, x86_sched_itmt_flags, PKG),
> +	{ NULL },
> +};
>  
> +static void __init build_sched_topology(void)
> +{
>  	/*
> -	 * There must be one trailing NULL entry left.
> +	 * When there is NUMA topology inside the package invalidate the
> +	 * PKG domain since the NUMA domains will auto-magically create the
> +	 * right spanning domains based on the SLIT.
>  	 */
> -	BUG_ON(i >= ARRAY_SIZE(x86_topology)-1);
> +	if (x86_has_numa_in_package) {
> +		unsigned int pkgdom = ARRAY_SIZE(x86_topology) - 2;
>  
> +		memset(&x86_topology[pkgdom], 0, sizeof(x86_topology[pkgdom]));
> +	}
>  	set_sched_topology(x86_topology);
>  }

Urgh, this patch is doing at least 4 things and nigh on unreadable
because of that.

 - introduces DOMAIN() helper
 - drops (the now pointless) SD_INIT_NAME() helper
 - drops CONFIG_SCHED_SMT (x86 special)
 - moves to static initialize and truncate