linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/4] x86/smpboot: tidy sched-topology and drop useless SMT level
@ 2025-07-06  3:06 Li Chen
  2025-07-06  3:06 ` [PATCH v4 1/4] smpboot: introduce SDTL() helper to tidy sched topology setup Li Chen
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Li Chen @ 2025-07-06  3:06 UTC (permalink / raw)
  To: Li Chen,  Thomas Gleixner ,  Ingo Molnar ,  Borislav Petkov ,
	 Dave Hansen , x86,  H . Peter Anvin ,  Rafael J . Wysocki ,
	 Peter Zijlstra ,  Sohil Mehta ,  Brian Gerst ,  Patryk Wlazlyn ,
	linux-kernel

From: Li Chen <chenl311@chinatelecom.cn>

This series cleans up sched-domain topology handling and
eliminates hundreds of pointless attach/destroy cycles for large
machines when SMT is not available.

Patch 1, 2, and 3 do some cleanup and refactor.

Patch 4 is a follow-up that simply skip SMT domain when
cpu_smt_num_threads <= 1, so the SMT level never gets created.

Tested on Qemu.

changelog:
v2: fix wording issue as suggested by Thomas [1]
v3: remove pointless memset and adjust PKG index accordingly, as
    suggested by Thomas [2], and refine some other wording issues.
v4: v4: Split refactor patche into three parts (as suggested by Peter [3])
    and refined patch 4 logic (as done by K. [4]).

[1]: https://lore.kernel.org/all/87msa2r018.ffs@tglx/
[2]: https://lore.kernel.org/all/875xglntx1.ffs@tglx/
[3]: https://lkml.org/lkml/2025/6/25/584
[4]: https://lore.kernel.org/lkml/1b706790-2fec-4582-a425-55eeff36c32e@amd.com/

Li Chen (4):
  smpboot: introduce SDTL() helper to tidy sched topology setup
  x86/smpboot: remove redundant CONFIG_SCHED_SMT
  x86/smpboot: moves x86_topology to static initialize and truncate
  x86/smpboot: avoid SMT domain attach/destroy if SMT is not enabled

 arch/powerpc/kernel/smp.c      | 34 ++++++++-------------
 arch/s390/kernel/topology.c    | 10 +++----
 arch/x86/kernel/smpboot.c      | 55 ++++++++++++++++------------------
 include/linux/sched/topology.h |  6 ++--
 kernel/sched/topology.c        | 16 ++++------
 5 files changed, 53 insertions(+), 68 deletions(-)

-- 
2.49.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v4 1/4] smpboot: introduce SDTL() helper to tidy sched topology setup
  2025-07-06  3:06 [PATCH v4 0/4] x86/smpboot: tidy sched-topology and drop useless SMT level Li Chen
@ 2025-07-06  3:06 ` Li Chen
  2025-07-07  5:33   ` K Prateek Nayak
  2025-07-06  3:06 ` [PATCH v4 2/4] x86/smpboot: remove redundant CONFIG_SCHED_SMT Li Chen
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Li Chen @ 2025-07-06  3:06 UTC (permalink / raw)
  To: Li Chen,  Thomas Gleixner ,  Ingo Molnar ,  Borislav Petkov ,
	 Dave Hansen , x86,  H . Peter Anvin ,  Rafael J . Wysocki ,
	 Peter Zijlstra ,  Sohil Mehta ,  Brian Gerst ,  Patryk Wlazlyn ,
	linux-kernel, Madhavan Srinivasan, Michael Ellerman,
	Nicholas Piggin, Christophe Leroy, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Valentin Schneider, Thomas Weißschuh, Bibo Mao, Li Chen,
	Huacai Chen, Tobias Huschle, Mete Durlu, Joel Granados,
	Guo Weikang, Rafael J. Wysocki, Sohil Mehta, K Prateek Nayak,
	Brian Gerst, Patryk Wlazlyn, Swapnil Sapkal, linuxppc-dev,
	linux-s390

From: Li Chen <chenl311@chinatelecom.cn>

Define a small SDTL(maskfn, flagsfn, name) macro and use it to build the
sched_domain_topology_level. Purely a cleanup; behaviour is unchanged.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/powerpc/kernel/smp.c      | 34 +++++++++++++---------------------
 arch/s390/kernel/topology.c    | 10 +++++-----
 arch/x86/kernel/smpboot.c      | 21 ++++++---------------
 include/linux/sched/topology.h |  6 +++---
 kernel/sched/topology.c        | 16 ++++++----------
 5 files changed, 33 insertions(+), 54 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 5ac7084eebc0b..b05995e272134 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -1700,28 +1700,20 @@ static void __init build_sched_topology(void)
 #ifdef CONFIG_SCHED_SMT
 	if (has_big_cores) {
 		pr_info("Big cores detected but using small core scheduling\n");
-		powerpc_topology[i++] = (struct sched_domain_topology_level){
-			smallcore_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT)
-		};
-	} else {
-		powerpc_topology[i++] = (struct sched_domain_topology_level){
-			cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT)
-		};
-	}
+		powerpc_topology[i++] =
+			SDTL(smallcore_smt_mask, powerpc_smt_flags, SMT);
+	} else
+		powerpc_topology[i++] = SDTL(cpu_smt_mask, powerpc_smt_flags, SMT);
 #endif
-	if (shared_caches) {
-		powerpc_topology[i++] = (struct sched_domain_topology_level){
-			shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE)
-		};
-	}
-	if (has_coregroup_support()) {
-		powerpc_topology[i++] = (struct sched_domain_topology_level){
-			cpu_mc_mask, powerpc_shared_proc_flags, SD_INIT_NAME(MC)
-		};
-	}
-	powerpc_topology[i++] = (struct sched_domain_topology_level){
-		cpu_cpu_mask, powerpc_shared_proc_flags, SD_INIT_NAME(PKG)
-	};
+	if (shared_caches)
+		powerpc_topology[i++] =
+			SDTL(shared_cache_mask, powerpc_shared_cache_flags, CACHE);
+
+	if (has_coregroup_support())
+		powerpc_topology[i++] =
+			SDTL(cpu_mc_mask, powerpc_shared_proc_flags, MC);
+
+	powerpc_topology[i++] = SDTL(cpu_cpu_mask, powerpc_shared_proc_flags, PKG);
 
 	/* There must be one trailing NULL entry left.  */
 	BUG_ON(i >= ARRAY_SIZE(powerpc_topology) - 1);
diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
index 3df048e190b11..ccf5dcc56f86d 100644
--- a/arch/s390/kernel/topology.c
+++ b/arch/s390/kernel/topology.c
@@ -531,11 +531,11 @@ static const struct cpumask *cpu_drawer_mask(int cpu)
 }
 
 static struct sched_domain_topology_level s390_topology[] = {
-	{ cpu_thread_mask, cpu_smt_flags, SD_INIT_NAME(SMT) },
-	{ cpu_coregroup_mask, cpu_core_flags, SD_INIT_NAME(MC) },
-	{ cpu_book_mask, SD_INIT_NAME(BOOK) },
-	{ cpu_drawer_mask, SD_INIT_NAME(DRAWER) },
-	{ cpu_cpu_mask, SD_INIT_NAME(PKG) },
+	SDTL(cpu_thread_mask, cpu_smt_flags, SMT),
+	SDTL(cpu_coregroup_mask, cpu_core_flags, MC),
+	SDTL(cpu_book_mask, NULL, BOOK),
+	SDTL(cpu_drawer_mask, NULL, DRAWER),
+	SDTL(cpu_cpu_mask, NULL, PKG),
 	{ NULL, },
 };
 
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 58ede3fa6a75b..55f09946c791b 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -485,35 +485,26 @@ 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)
-	};
+	x86_topology[i++] = SDTL(cpu_smt_mask, cpu_smt_flags, SMT);
 #endif
 #ifdef CONFIG_SCHED_CLUSTER
-	x86_topology[i++] = (struct sched_domain_topology_level){
-		cpu_clustergroup_mask, x86_cluster_flags, SD_INIT_NAME(CLS)
-	};
+	x86_topology[i++] = SDTL(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)
-	};
+	x86_topology[i++] = SDTL(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)
-		};
-	}
+	if (!x86_has_numa_in_package)
+		x86_topology[i++] = SDTL(cpu_cpu_mask, x86_sched_itmt_flags, PKG);
 
 	/*
 	 * There must be one trailing NULL entry left.
 	 */
-	BUG_ON(i >= ARRAY_SIZE(x86_topology)-1);
+	BUG_ON(i >= ARRAY_SIZE(x86_topology) - 1);
 
 	set_sched_topology(x86_topology);
 }
diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
index 198bb5cc1774b..0b53e372c445c 100644
--- a/include/linux/sched/topology.h
+++ b/include/linux/sched/topology.h
@@ -197,9 +197,9 @@ struct sched_domain_topology_level {
 extern void __init set_sched_topology(struct sched_domain_topology_level *tl);
 extern void sched_update_asym_prefer_cpu(int cpu, int old_prio, int new_prio);
 
-
-# define SD_INIT_NAME(type)		.name = #type
-
+#define SDTL(maskfn, flagsfn, dname) \
+	((struct sched_domain_topology_level) \
+	    { .mask = maskfn, .sd_flags = flagsfn, .name = #dname, .numa_level = 0 })
 #else /* CONFIG_SMP */
 
 struct sched_domain_attr;
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index b958fe48e0205..e6ec65ae4b75d 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1739,17 +1739,17 @@ sd_init(struct sched_domain_topology_level *tl,
  */
 static struct sched_domain_topology_level default_topology[] = {
 #ifdef CONFIG_SCHED_SMT
-	{ cpu_smt_mask, cpu_smt_flags, SD_INIT_NAME(SMT) },
+	SDTL(cpu_smt_mask, cpu_smt_flags, SMT),
 #endif
 
 #ifdef CONFIG_SCHED_CLUSTER
-	{ cpu_clustergroup_mask, cpu_cluster_flags, SD_INIT_NAME(CLS) },
+	SDTL(cpu_clustergroup_mask, cpu_cluster_flags, CLS),
 #endif
 
 #ifdef CONFIG_SCHED_MC
-	{ cpu_coregroup_mask, cpu_core_flags, SD_INIT_NAME(MC) },
+	SDTL(cpu_coregroup_mask, cpu_core_flags, MC),
 #endif
-	{ cpu_cpu_mask, SD_INIT_NAME(PKG) },
+	SDTL(cpu_cpu_mask, NULL, PKG),
 	{ NULL, },
 };
 
@@ -2010,11 +2010,7 @@ void sched_init_numa(int offline_node)
 	/*
 	 * Add the NUMA identity distance, aka single NODE.
 	 */
-	tl[i++] = (struct sched_domain_topology_level){
-		.mask = sd_numa_mask,
-		.numa_level = 0,
-		SD_INIT_NAME(NODE)
-	};
+	tl[i++] = SDTL(sd_numa_mask, NULL, NODE);
 
 	/*
 	 * .. and append 'j' levels of NUMA goodness.
@@ -2025,7 +2021,7 @@ void sched_init_numa(int offline_node)
 			.sd_flags = cpu_numa_flags,
 			.flags = SDTL_OVERLAP,
 			.numa_level = j,
-			SD_INIT_NAME(NUMA)
+			.name = "NUMA",
 		};
 	}
 
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v4 2/4] x86/smpboot: remove redundant CONFIG_SCHED_SMT
  2025-07-06  3:06 [PATCH v4 0/4] x86/smpboot: tidy sched-topology and drop useless SMT level Li Chen
  2025-07-06  3:06 ` [PATCH v4 1/4] smpboot: introduce SDTL() helper to tidy sched topology setup Li Chen
@ 2025-07-06  3:06 ` Li Chen
  2025-07-06  3:06 ` [PATCH v4 3/4] x86/smpboot: moves x86_topology to static initialize and truncate Li Chen
  2025-07-06  3:06 ` [PATCH v4 4/4] x86/smpboot: avoid SMT domain attach/destroy if SMT is not enabled Li Chen
  3 siblings, 0 replies; 8+ messages in thread
From: Li Chen @ 2025-07-06  3:06 UTC (permalink / raw)
  To: Li Chen,  Thomas Gleixner ,  Ingo Molnar ,  Borislav Petkov ,
	 Dave Hansen , x86,  H . Peter Anvin ,  Rafael J . Wysocki ,
	 Peter Zijlstra ,  Sohil Mehta ,  Brian Gerst ,  Patryk Wlazlyn ,
	linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H. Peter Anvin, Rafael J. Wysocki, Li Chen,
	Brian Gerst, Patryk Wlazlyn, K Prateek Nayak

From: Li Chen <chenl311@chinatelecom.cn>

CONFIG_SCHED_SMT is default y if SMP is enabled,
so let's simply drop CONFIG_SCHED_SMT.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/smpboot.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 55f09946c791b..6e36306632792 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -484,9 +484,7 @@ static void __init build_sched_topology(void)
 {
 	int i = 0;
 
-#ifdef CONFIG_SCHED_SMT
 	x86_topology[i++] = SDTL(cpu_smt_mask, cpu_smt_flags, SMT);
-#endif
 #ifdef CONFIG_SCHED_CLUSTER
 	x86_topology[i++] = SDTL(cpu_clustergroup_mask, x86_cluster_flags, CLS);
 #endif
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v4 3/4] x86/smpboot: moves x86_topology to static initialize and truncate
  2025-07-06  3:06 [PATCH v4 0/4] x86/smpboot: tidy sched-topology and drop useless SMT level Li Chen
  2025-07-06  3:06 ` [PATCH v4 1/4] smpboot: introduce SDTL() helper to tidy sched topology setup Li Chen
  2025-07-06  3:06 ` [PATCH v4 2/4] x86/smpboot: remove redundant CONFIG_SCHED_SMT Li Chen
@ 2025-07-06  3:06 ` Li Chen
  2025-07-06  3:06 ` [PATCH v4 4/4] x86/smpboot: avoid SMT domain attach/destroy if SMT is not enabled Li Chen
  3 siblings, 0 replies; 8+ messages in thread
From: Li Chen @ 2025-07-06  3:06 UTC (permalink / raw)
  To: Li Chen,  Thomas Gleixner ,  Ingo Molnar ,  Borislav Petkov ,
	 Dave Hansen , x86,  H . Peter Anvin ,  Rafael J . Wysocki ,
	 Peter Zijlstra ,  Sohil Mehta ,  Brian Gerst ,  Patryk Wlazlyn ,
	linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H. Peter Anvin, Rafael J. Wysocki, Li Chen,
	K Prateek Nayak, Patryk Wlazlyn

From: Li Chen <chenl311@chinatelecom.cn>

The #ifdeffery and the initializers in build_sched_topology() are just
disgusting.

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.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 arch/x86/kernel/smpboot.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 6e36306632792..cd70e5322462a 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -478,32 +478,30 @@ static int x86_cluster_flags(void)
  */
 static bool x86_has_numa_in_package;
 
-static struct sched_domain_topology_level x86_topology[6];
-
-static void __init build_sched_topology(void)
-{
-	int i = 0;
-
-	x86_topology[i++] = SDTL(cpu_smt_mask, cpu_smt_flags, SMT);
+static struct sched_domain_topology_level x86_topology[] = {
+	SDTL(cpu_smt_mask, cpu_smt_flags, SMT),
 #ifdef CONFIG_SCHED_CLUSTER
-	x86_topology[i++] = SDTL(cpu_clustergroup_mask, x86_cluster_flags, CLS);
+	SDTL(cpu_clustergroup_mask, x86_cluster_flags, CLS),
 #endif
 #ifdef CONFIG_SCHED_MC
-	x86_topology[i++] = SDTL(cpu_coregroup_mask, x86_core_flags, MC);
+	SDTL(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++] = SDTL(cpu_cpu_mask, x86_sched_itmt_flags, PKG);
+	SDTL(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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v4 4/4] x86/smpboot: avoid SMT domain attach/destroy if SMT is not enabled
  2025-07-06  3:06 [PATCH v4 0/4] x86/smpboot: tidy sched-topology and drop useless SMT level Li Chen
                   ` (2 preceding siblings ...)
  2025-07-06  3:06 ` [PATCH v4 3/4] x86/smpboot: moves x86_topology to static initialize and truncate Li Chen
@ 2025-07-06  3:06 ` Li Chen
  2025-07-07  5:39   ` K Prateek Nayak
  3 siblings, 1 reply; 8+ messages in thread
From: Li Chen @ 2025-07-06  3:06 UTC (permalink / raw)
  To: Li Chen,  Thomas Gleixner ,  Ingo Molnar ,  Borislav Petkov ,
	 Dave Hansen , x86,  H . Peter Anvin ,  Rafael J . Wysocki ,
	 Peter Zijlstra ,  Sohil Mehta ,  Brian Gerst ,  Patryk Wlazlyn ,
	linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H. Peter Anvin, Rafael J. Wysocki, Li Chen,
	Sohil Mehta, Patryk Wlazlyn, K Prateek Nayak

From: Li Chen <chenl311@chinatelecom.cn>

Currently, the SMT domain is added into sched_domain_topology by default.

If cpu_attach_domain() finds that the CPU SMT domain’s cpumask_weight
is just 1, it will destroy it.

On a large machine, such as one with 512 cores, this results in
512 redundant domain attach/destroy operations.

Avoid these unnecessary operations by simply checking
cpu_smt_num_threads and skip SMT domain if the SMT domain is not
enabled.

Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>
Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 arch/x86/kernel/smpboot.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index cd70e5322462a..8c1960a455bfb 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -492,6 +492,8 @@ static struct sched_domain_topology_level x86_topology[] = {
 
 static void __init build_sched_topology(void)
 {
+	struct sched_domain_topology_level *topology = x86_topology;
+
 	/*
 	 * When there is NUMA topology inside the package invalidate the
 	 * PKG domain since the NUMA domains will auto-magically create the
@@ -502,7 +504,15 @@ static void __init build_sched_topology(void)
 
 		memset(&x86_topology[pkgdom], 0, sizeof(x86_topology[pkgdom]));
 	}
-	set_sched_topology(x86_topology);
+
+    /*
+     * Drop the SMT domains if there is only one thread per-core
+     * since it'll get degenerated by the scheduler anyways.
+     */
+	if (cpu_smt_num_threads <= 1)
+		++topology;
+
+	set_sched_topology(topology);
 }
 
 void set_cpu_sibling_map(int cpu)
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 1/4] smpboot: introduce SDTL() helper to tidy sched topology setup
  2025-07-06  3:06 ` [PATCH v4 1/4] smpboot: introduce SDTL() helper to tidy sched topology setup Li Chen
@ 2025-07-07  5:33   ` K Prateek Nayak
  2025-07-11  0:24     ` Li Chen
  0 siblings, 1 reply; 8+ messages in thread
From: K Prateek Nayak @ 2025-07-07  5:33 UTC (permalink / raw)
  To: Li Chen, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H . Peter Anvin, Rafael J . Wysocki,
	Peter Zijlstra, Sohil Mehta, Brian Gerst, Patryk Wlazlyn,
	linux-kernel, Madhavan Srinivasan, Michael Ellerman,
	Nicholas Piggin, Christophe Leroy, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt,
	Ben Segall, Mel Gorman, Valentin Schneider, Thomas Weißschuh,
	Bibo Mao, Li Chen, Huacai Chen, Tobias Huschle, Mete Durlu,
	Joel Granados, Guo Weikang, Swapnil Sapkal, linuxppc-dev,
	linux-s390

Hello Li,

Apart from few comments inline below, feel free to include:

Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>

for the entire series.

On 7/6/2025 8:36 AM, Li Chen wrote:
> diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
> index 198bb5cc1774b..0b53e372c445c 100644
> --- a/include/linux/sched/topology.h
> +++ b/include/linux/sched/topology.h
> @@ -197,9 +197,9 @@ struct sched_domain_topology_level {
>  extern void __init set_sched_topology(struct sched_domain_topology_level *tl);
>  extern void sched_update_asym_prefer_cpu(int cpu, int old_prio, int new_prio);
>  
> -
> -# define SD_INIT_NAME(type)		.name = #type
> -
> +#define SDTL(maskfn, flagsfn, dname) \
> +	((struct sched_domain_topology_level) \
> +	    { .mask = maskfn, .sd_flags = flagsfn, .name = #dname, .numa_level = 0 })

I prefer the following alignment:

#define SDTL(maskfn, flagsfn, dname) ((struct sched_domain_topology_level) \
	{ .mask = maskfn, .sd_flags = flagsfn, .name = #dname })

instead of having 3 lines. "numa_level" is 0 by default so I don't think
we need to explicitly specify it again.

Also perhaps the macro can be named "SDTL_INIT()" to keep consistent
with the naming convention.

>  #else /* CONFIG_SMP */

A bunch of the CONFIG_SMP related ifdeffry is being removed for the
next cycle. You can perhaps rebase the series on top of the tip tree
(git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git)

>  
>  struct sched_domain_attr;

[..snip..]

> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index b958fe48e0205..e6ec65ae4b75d 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -2025,7 +2021,7 @@ void sched_init_numa(int offline_node)
>  			.sd_flags = cpu_numa_flags,
>  			.flags = SDTL_OVERLAP,
>  			.numa_level = j,
> -			SD_INIT_NAME(NUMA)
> +			.name = "NUMA",

This can use SDTL() macro too. Just explicitly set "tl[i].numa_level" to
"j" after.

>  		};
>  	}
>  

-- 
Thanks and Regards,
Prateek


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 4/4] x86/smpboot: avoid SMT domain attach/destroy if SMT is not enabled
  2025-07-06  3:06 ` [PATCH v4 4/4] x86/smpboot: avoid SMT domain attach/destroy if SMT is not enabled Li Chen
@ 2025-07-07  5:39   ` K Prateek Nayak
  0 siblings, 0 replies; 8+ messages in thread
From: K Prateek Nayak @ 2025-07-07  5:39 UTC (permalink / raw)
  To: Li Chen, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86, H . Peter Anvin, Rafael J . Wysocki,
	Peter Zijlstra, Sohil Mehta, Brian Gerst, Patryk Wlazlyn,
	linux-kernel, Li Chen

Hello Li,

On 7/6/2025 8:36 AM, Li Chen wrote:
> From: Li Chen <chenl311@chinatelecom.cn>
> 
> Currently, the SMT domain is added into sched_domain_topology by default.
> 
> If cpu_attach_domain() finds that the CPU SMT domain’s cpumask_weight
> is just 1, it will destroy it.
> 
> On a large machine, such as one with 512 cores, this results in
> 512 redundant domain attach/destroy operations.
> 
> Avoid these unnecessary operations by simply checking
> cpu_smt_num_threads and skip SMT domain if the SMT domain is not
> enabled.
> 
> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>

"Suggested-by:" should be fine :)

> Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>
> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
> ---
>  arch/x86/kernel/smpboot.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index cd70e5322462a..8c1960a455bfb 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -492,6 +492,8 @@ static struct sched_domain_topology_level x86_topology[] = {
>  
>  static void __init build_sched_topology(void)
>  {
> +	struct sched_domain_topology_level *topology = x86_topology;
> +
>  	/*
>  	 * When there is NUMA topology inside the package invalidate the
>  	 * PKG domain since the NUMA domains will auto-magically create the
> @@ -502,7 +504,15 @@ static void __init build_sched_topology(void)
>  
>  		memset(&x86_topology[pkgdom], 0, sizeof(x86_topology[pkgdom]));
>  	}
> -	set_sched_topology(x86_topology);
> +
> +    /*
> +     * Drop the SMT domains if there is only one thread per-core
> +     * since it'll get degenerated by the scheduler anyways.
> +     */

The comment above has spaces instead of tab before it.

> +	if (cpu_smt_num_threads <= 1)
> +		++topology;> +
> +	set_sched_topology(topology);
>  }
>  
>  void set_cpu_sibling_map(int cpu)

-- 
Thanks and Regards,
Prateek


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 1/4] smpboot: introduce SDTL() helper to tidy sched topology setup
  2025-07-07  5:33   ` K Prateek Nayak
@ 2025-07-11  0:24     ` Li Chen
  0 siblings, 0 replies; 8+ messages in thread
From: Li Chen @ 2025-07-11  0:24 UTC (permalink / raw)
  To: K Prateek Nayak
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H . Peter Anvin, Rafael J . Wysocki, Peter Zijlstra, Sohil Mehta,
	Brian Gerst, Patryk Wlazlyn, linux-kernel, Madhavan Srinivasan,
	Michael Ellerman, Nicholas Piggin, Christophe Leroy,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
	Christian Borntraeger, Sven Schnelle, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Valentin Schneider, "Thomas Weißschuh", Bibo Mao,
	Li Chen, Huacai Chen, Tobias Huschle, Mete Durlu, Joel Granados,
	Guo Weikang, Swapnil Sapkal, linuxppc-dev, linux-s390

Hi K,

Thanks for your reviews and test! I have addressed all issues in v5:
https://www.spinics.net/lists/kernel/msg5761848.html

 ---- On Mon, 07 Jul 2025 13:33:53 +0800  K Prateek Nayak <kprateek.nayak@amd.com> wrote --- 
 > Hello Li,
 > 
 > Apart from few comments inline below, feel free to include:
 > 
 > Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>
 > 
 > for the entire series.
 > 
 > On 7/6/2025 8:36 AM, Li Chen wrote:
 > > diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
 > > index 198bb5cc1774b..0b53e372c445c 100644
 > > --- a/include/linux/sched/topology.h
 > > +++ b/include/linux/sched/topology.h
 > > @@ -197,9 +197,9 @@ struct sched_domain_topology_level {
 > >  extern void __init set_sched_topology(struct sched_domain_topology_level *tl);
 > >  extern void sched_update_asym_prefer_cpu(int cpu, int old_prio, int new_prio);
 > >  
 > > -
 > > -# define SD_INIT_NAME(type)        .name = #type
 > > -
 > > +#define SDTL(maskfn, flagsfn, dname) \
 > > +    ((struct sched_domain_topology_level) \
 > > +        { .mask = maskfn, .sd_flags = flagsfn, .name = #dname, .numa_level = 0 })
 > 
 > I prefer the following alignment:
 > 
 > #define SDTL(maskfn, flagsfn, dname) ((struct sched_domain_topology_level) \
 >     { .mask = maskfn, .sd_flags = flagsfn, .name = #dname })
 > 
 > instead of having 3 lines. "numa_level" is 0 by default so I don't think
 > we need to explicitly specify it again.
 > 
 > Also perhaps the macro can be named "SDTL_INIT()" to keep consistent
 > with the naming convention.
 > 
 > >  #else /* CONFIG_SMP */
 > 
 > A bunch of the CONFIG_SMP related ifdeffry is being removed for the
 > next cycle. You can perhaps rebase the series on top of the tip tree
 > (git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git)
 > 
 > >  
 > >  struct sched_domain_attr;
 > 
 > [..snip..]
 > 
 > > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
 > > index b958fe48e0205..e6ec65ae4b75d 100644
 > > --- a/kernel/sched/topology.c
 > > +++ b/kernel/sched/topology.c
 > > @@ -2025,7 +2021,7 @@ void sched_init_numa(int offline_node)
 > >              .sd_flags = cpu_numa_flags,
 > >              .flags = SDTL_OVERLAP,
 > >              .numa_level = j,
 > > -            SD_INIT_NAME(NUMA)
 > > +            .name = "NUMA",
 > 
 > This can use SDTL() macro too. Just explicitly set "tl[i].numa_level" to
 > "j" after.
 > 
 > >          };
 > >      }
 > >  
 > 
 > -- 
 > Thanks and Regards,
 > Prateek
 > 
 > 
Regards,

Li​


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-07-11  0:26 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-06  3:06 [PATCH v4 0/4] x86/smpboot: tidy sched-topology and drop useless SMT level Li Chen
2025-07-06  3:06 ` [PATCH v4 1/4] smpboot: introduce SDTL() helper to tidy sched topology setup Li Chen
2025-07-07  5:33   ` K Prateek Nayak
2025-07-11  0:24     ` Li Chen
2025-07-06  3:06 ` [PATCH v4 2/4] x86/smpboot: remove redundant CONFIG_SCHED_SMT Li Chen
2025-07-06  3:06 ` [PATCH v4 3/4] x86/smpboot: moves x86_topology to static initialize and truncate Li Chen
2025-07-06  3:06 ` [PATCH v4 4/4] x86/smpboot: avoid SMT domain attach/destroy if SMT is not enabled Li Chen
2025-07-07  5:39   ` K Prateek Nayak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).