1. Patchew
  2. linux
  3. [PATCH v3 0/2] x86/smpboot: tidy sched-topology and drop useless SMT level
  4. View patch

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

Li Chen posted 2 patches 3 months, 2 weeks ago
There is a newer version of this series
[PATCH v3 1/2] x86/smpboot: Decrapify build_sched_topology()
Posted by Li Chen 3 months, 2 weeks ago 
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.

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);
 }
 
-- 
2.49.0
Re: [PATCH v3 1/2] x86/smpboot: Decrapify build_sched_topology()
Posted by Peter Zijlstra 3 months, 2 weeks ago 
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
Re: [PATCH v3 1/2] x86/smpboot: Decrapify build_sched_topology()
Posted by Li Chen 3 months, 1 week ago 
Hi Peter,


 ---- On Wed, 25 Jun 2025 16:28:19 +0800  Peter Zijlstra <peterz@infradead.org> wrote --- 
 > 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

Thanks for your review, I would split this Thomas's patch to 4 different patches
and preserve his signoff
Re: [PATCH v3 1/2] x86/smpboot: Decrapify build_sched_topology()
Posted by Thomas Gleixner 3 months, 1 week ago 
On Mon, Jun 30 2025 at 08:21, Li Chen wrote:
>  ---- On Wed, 25 Jun 2025 16:28:19 +0800  Peter Zijlstra <peterz@infradead.org> wrote --- 
>  > 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
>
> Thanks for your review, I would split this Thomas's patch to 4 different patches
> and preserve his signoff

Nah. Just split it up and add

     Suggested-by: Thomas ....

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.