Skip to contentSkip to conversation

sched/topology: Enable topology_span_sane check only for debug builds

PATCH In review Naman Jain 2 participants 6 messages Mar 10, 2025 – Apr 3, 2025

Subsystems (2)
Stable branch Scheduler
lore
Share & tools
lore

Naman Jain

author
#0 lore raw

PATCH v5 1/1 sched/topology: Enable topology_span_sane check only for debug builds

From: Saurabh Sengar <[email protected]>

On a x86 system under test with 1780 CPUs, topology_span_sane() takes around 8 seconds cumulatively for all the iterations. It is an expensive operation which does the sanity of non-NUMA topology masks.

CPU topology is not something which changes very frequently hence make this check optional for the systems where the topology is trusted and need faster bootup.

Restrict this to sched_verbose kernel cmdline option so that this penalty can be avoided for the systems who want to avoid it.

Cc [email protected]
Fixes ccf74128d66c ("sched/topology: Assert non-NUMA topology masks don't (partially) overlap")
Signed-off-by Saurabh Sengar <[email protected]>
Co-developed-by Naman Jain <[email protected]>
Signed-off-by Naman Jain <[email protected]>
Tested-by K Prateek Nayak <[email protected]>

---
Changes since v4:
https://lore.kernel.org/all/[email protected]/
- Rephrased print statement and moved it to sched_domain_debug. (addressing Valentin's comments)
Changes since v3:
https://lore.kernel.org/all/[email protected]/
- Minor typo correction in comment
- Added Tested-by tag from Prateek for x86
Changes since v2:
https://lore.kernel.org/all/[email protected]/
- Use sched_debug() instead of using sched_debug_verbose variable directly (addressing Prateek's comment)

Changes since v1:
https://lore.kernel.org/all/[email protected]/
- Use kernel cmdline param instead of compile time flag.

Adding a link to the other patch which is under review.
https://lore.kernel.org/lkml/[email protected]/
Above patch tries to optimize the topology sanity check, whereas this patch makes it optional. We believe both patches can coexist, as even with optimization, there will still be some performance overhead for this check.

---
 kernel/sched/topology.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
kernel/sched/topology.c +8 -1
index c49aea8c1025..d7254c47af45 100644
@@ -132,8 +132,11 @@ static void sched_domain_debug(struct sched_domain *sd, int cpu)
 {
 	int level = 0;
 
-	if (!sched_debug_verbose)
+	if (!sched_debug_verbose) {
+		pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
+			     __func__);
 		return;
+	}
 
 	if (!sd) {
 		printk(KERN_DEBUG "CPU%d attaching NULL sched-domain.\n", cpu);

@@ -2359,6 +2362,10 @@ static bool topology_span_sane(struct sched_domain_topology_level *tl,
 {
 	int i = cpu + 1;
 
+	/* Skip the topology sanity check for non-debug, as it is a time-consuming operation */
+	if (!sched_debug())
+		return true;
+
 	/* NUMA levels are allowed to overlap */
 	if (tl->flags & SDTL_OVERLAP)
 		return true;

base-commit: 7ec162622e66a4ff886f8f28712ea1b13069e1aa

Signature · 1 line
2.34.1
reviewer Reviewed-by on v5 1/1
#1 lore raw
Quoted 53 lines · Naman Jain

From: Saurabh Sengar <[email protected]>

On a x86 system under test with 1780 CPUs, topology_span_sane() takes around 8 seconds cumulatively for all the iterations. It is an expensive operation which does the sanity of non-NUMA topology masks.

CPU topology is not something which changes very frequently hence make this check optional for the systems where the topology is trusted and need faster bootup.

Restrict this to sched_verbose kernel cmdline option so that this penalty can be avoided for the systems who want to avoid it.

Cc [email protected]
Fixes ccf74128d66c ("sched/topology: Assert non-NUMA topology masks don't (partially) overlap")
Signed-off-by Saurabh Sengar <[email protected]>
Co-developed-by Naman Jain <[email protected]>
Signed-off-by Naman Jain <[email protected]>
Tested-by K Prateek Nayak <[email protected]>

---
Changes since v4:
https://lore.kernel.org/all/[email protected]/
- Rephrased print statement and moved it to sched_domain_debug. (addressing Valentin's comments)
Changes since v3:
https://lore.kernel.org/all/[email protected]/
- Minor typo correction in comment
- Added Tested-by tag from Prateek for x86
Changes since v2:
https://lore.kernel.org/all/[email protected]/
- Use sched_debug() instead of using sched_debug_verbose variable directly (addressing Prateek's comment)

Changes since v1:
https://lore.kernel.org/all/[email protected]/
- Use kernel cmdline param instead of compile time flag.

Adding a link to the other patch which is under review.
https://lore.kernel.org/lkml/[email protected]/
Above patch tries to optimize the topology sanity check, whereas this patch makes it optional. We believe both patches can coexist, as even with optimization, there will still be some performance overhead for this check.

---
 kernel/sched/topology.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
kernel/sched/topology.c +0 -0
index c49aea8c1025..d7254c47af45 100644

kernel/sched/topology.c static void sched_domain_debug(struct sched_domain *sd, int cpu) · line 140

Quoted 10 lines
kernel/sched/topology.c +4 -1
@@ -132,8 +132,11 @@ static void sched_domain_debug(struct sched_domain *sd, int cpu)
 {
      int level = 0;
 
-	if (!sched_debug_verbose)
+	if (!sched_debug_verbose) {
+		pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
+			     __func__);
              return;
+	}

Nit: I've been told not to break warnings over multiple lines so they can be grep'ed, but given this has the "sched_domain_debug:" prefix I think we could get away with the below.

Regardless:

Reviewed-by Valentin Schneider <[email protected]>

---

kernel/sched/topology.c +2 -1
index d7254c47af455..b4dc7c7d2c41c 100644
@@ -133,7 +133,8 @@ static void sched_domain_debug(struct sched_domain *sd, int cpu)
 	int level = 0;
 
 	if (!sched_debug_verbose) {
-		pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
+		pr_info_once("%s: Scheduler topology debugging disabled, "
+			     "add 'sched_verbose' to the cmdline to enable it\n",
 			     __func__);
 		return;
 	}
author
#2 lore raw
Quoted 78 lines · Valentin Schneider
Quoted 53 lines · Naman Jain

From: Saurabh Sengar <[email protected]>

On a x86 system under test with 1780 CPUs, topology_span_sane() takes around 8 seconds cumulatively for all the iterations. It is an expensive operation which does the sanity of non-NUMA topology masks.

CPU topology is not something which changes very frequently hence make this check optional for the systems where the topology is trusted and need faster bootup.

Restrict this to sched_verbose kernel cmdline option so that this penalty can be avoided for the systems who want to avoid it.

Cc [email protected]
Fixes ccf74128d66c ("sched/topology: Assert non-NUMA topology masks don't (partially) overlap")
Signed-off-by Saurabh Sengar <[email protected]>
Co-developed-by Naman Jain <[email protected]>
Signed-off-by Naman Jain <[email protected]>
Tested-by K Prateek Nayak <[email protected]>

---
Changes since v4:
https://lore.kernel.org/all/[email protected]/
- Rephrased print statement and moved it to sched_domain_debug. (addressing Valentin's comments)
Changes since v3:
https://lore.kernel.org/all/[email protected]/
- Minor typo correction in comment
- Added Tested-by tag from Prateek for x86
Changes since v2:
https://lore.kernel.org/all/[email protected]/
- Use sched_debug() instead of using sched_debug_verbose variable directly (addressing Prateek's comment)

Changes since v1:
https://lore.kernel.org/all/[email protected]/
- Use kernel cmdline param instead of compile time flag.

Adding a link to the other patch which is under review.
https://lore.kernel.org/lkml/[email protected]/
Above patch tries to optimize the topology sanity check, whereas this patch makes it optional. We believe both patches can coexist, as even with optimization, there will still be some performance overhead for this check.

---
  kernel/sched/topology.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)
kernel/sched/topology.c +0 -0
index c49aea8c1025..d7254c47af45 100644

kernel/sched/topology.c static void sched_domain_debug(struct sched_domain *sd, int cpu) · line 140

Quoted 10 lines
kernel/sched/topology.c +4 -1
@@ -132,8 +132,11 @@ static void sched_domain_debug(struct sched_domain *sd, int cpu)
  {
       int level = 0;
 
-	if (!sched_debug_verbose)
+	if (!sched_debug_verbose) {
+		pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
+			     __func__);
               return;
+	}

Nit: I've been told not to break warnings over multiple lines so they can be grep'ed, but given this has the "sched_domain_debug:" prefix I think we could get away with the below.

Regardless:

Reviewed-by Valentin Schneider <[email protected]>

---

kernel/sched/topology.c +0 -0
index d7254c47af455..b4dc7c7d2c41c 100644

kernel/sched/topology.c static void sched_domain_debug(struct sched_domain *sd, int cpu) · line 140

Quoted 9 lines
kernel/sched/topology.c +2 -1
@@ -133,7 +133,8 @@ static void sched_domain_debug(struct sched_domain *sd, int cpu)
  	int level = 0;
 
  	if (!sched_debug_verbose) {
-		pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
+		pr_info_once("%s: Scheduler topology debugging disabled, "
+			     "add 'sched_verbose' to the cmdline to enable it\n",
  			     __func__);
  		return;
  	}

Thanks for reviewing. I'll wait a couple more days to push the next version.

Thanks,
Naman

author
#3 lore raw
In reply to Valentin Schneider
Quoted 78 lines · Valentin Schneider
Quoted 53 lines · Naman Jain

From: Saurabh Sengar <[email protected]>

On a x86 system under test with 1780 CPUs, topology_span_sane() takes around 8 seconds cumulatively for all the iterations. It is an expensive operation which does the sanity of non-NUMA topology masks.

CPU topology is not something which changes very frequently hence make this check optional for the systems where the topology is trusted and need faster bootup.

Restrict this to sched_verbose kernel cmdline option so that this penalty can be avoided for the systems who want to avoid it.

Cc [email protected]
Fixes ccf74128d66c ("sched/topology: Assert non-NUMA topology masks don't (partially) overlap")
Signed-off-by Saurabh Sengar <[email protected]>
Co-developed-by Naman Jain <[email protected]>
Signed-off-by Naman Jain <[email protected]>
Tested-by K Prateek Nayak <[email protected]>

---
Changes since v4:
https://lore.kernel.org/all/[email protected]/
- Rephrased print statement and moved it to sched_domain_debug. (addressing Valentin's comments)
Changes since v3:
https://lore.kernel.org/all/[email protected]/
- Minor typo correction in comment
- Added Tested-by tag from Prateek for x86
Changes since v2:
https://lore.kernel.org/all/[email protected]/
- Use sched_debug() instead of using sched_debug_verbose variable directly (addressing Prateek's comment)

Changes since v1:
https://lore.kernel.org/all/[email protected]/
- Use kernel cmdline param instead of compile time flag.

Adding a link to the other patch which is under review.
https://lore.kernel.org/lkml/[email protected]/
Above patch tries to optimize the topology sanity check, whereas this patch makes it optional. We believe both patches can coexist, as even with optimization, there will still be some performance overhead for this check.

---
  kernel/sched/topology.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)
kernel/sched/topology.c +0 -0
index c49aea8c1025..d7254c47af45 100644

kernel/sched/topology.c static void sched_domain_debug(struct sched_domain *sd, int cpu) · line 140

Quoted 10 lines
kernel/sched/topology.c +4 -1
@@ -132,8 +132,11 @@ static void sched_domain_debug(struct sched_domain *sd, int cpu)
  {
       int level = 0;
 
-	if (!sched_debug_verbose)
+	if (!sched_debug_verbose) {
+		pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
+			     __func__);
               return;
+	}

Nit: I've been told not to break warnings over multiple lines so they can be grep'ed, but given this has the "sched_domain_debug:" prefix I think we could get away with the below.

Regardless:

Reviewed-by Valentin Schneider <[email protected]>

---

kernel/sched/topology.c +0 -0
index d7254c47af455..b4dc7c7d2c41c 100644

kernel/sched/topology.c static void sched_domain_debug(struct sched_domain *sd, int cpu) · line 140

Quoted 9 lines
kernel/sched/topology.c +2 -1
@@ -133,7 +133,8 @@ static void sched_domain_debug(struct sched_domain *sd, int cpu)
  	int level = 0;
 
  	if (!sched_debug_verbose) {
-		pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
+		pr_info_once("%s: Scheduler topology debugging disabled, "
+			     "add 'sched_verbose' to the cmdline to enable it\n",
  			     __func__);
  		return;
  	}

Hi Valentin,
Splitting the warning to different lines give checkpatch error with --strict option. --fix option suggests we keep it like we have it originally(single line). Please let me know if you feel we can change it to something else or if you are fine with picking the change for next iteration. Thanks again.

# ./scripts/checkpatch.pl --strict v6-0001-sched-topology-Enable-topology_span_sane-check-on.patch
WARNING: quoted string split across lines
#40: FILE: kernel/sched/topology.c:137:
+               pr_info_once("%s: Scheduler topology debugging disabled, "
+                            "add 'sched_verbose' to the cmdline to enable it\n",

total: 0 errors, 1 warnings, 0 checks, 23 lines checked

Regards,
Naman

reviewer
#4 lore raw
Quoted 81 lines · Naman Jain
Quoted 80 lines · Valentin Schneider
Quoted 53 lines · Naman Jain

From: Saurabh Sengar <[email protected]>

On a x86 system under test with 1780 CPUs, topology_span_sane() takes around 8 seconds cumulatively for all the iterations. It is an expensive operation which does the sanity of non-NUMA topology masks.

CPU topology is not something which changes very frequently hence make this check optional for the systems where the topology is trusted and need faster bootup.

Restrict this to sched_verbose kernel cmdline option so that this penalty can be avoided for the systems who want to avoid it.

Cc [email protected]
Fixes ccf74128d66c ("sched/topology: Assert non-NUMA topology masks don't (partially) overlap")
Signed-off-by Saurabh Sengar <[email protected]>
Co-developed-by Naman Jain <[email protected]>
Signed-off-by Naman Jain <[email protected]>
Tested-by K Prateek Nayak <[email protected]>

---
Changes since v4:
https://lore.kernel.org/all/[email protected]/
- Rephrased print statement and moved it to sched_domain_debug. (addressing Valentin's comments)
Changes since v3:
https://lore.kernel.org/all/[email protected]/
- Minor typo correction in comment
- Added Tested-by tag from Prateek for x86
Changes since v2:
https://lore.kernel.org/all/[email protected]/
- Use sched_debug() instead of using sched_debug_verbose variable directly (addressing Prateek's comment)

Changes since v1:
https://lore.kernel.org/all/[email protected]/
- Use kernel cmdline param instead of compile time flag.

Adding a link to the other patch which is under review.
https://lore.kernel.org/lkml/[email protected]/
Above patch tries to optimize the topology sanity check, whereas this patch makes it optional. We believe both patches can coexist, as even with optimization, there will still be some performance overhead for this check.

---
  kernel/sched/topology.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)
kernel/sched/topology.c +0 -0
index c49aea8c1025..d7254c47af45 100644

kernel/sched/topology.c static void sched_domain_debug(struct sched_domain *sd, int cpu) · line 140

Quoted 10 lines
kernel/sched/topology.c +4 -1
@@ -132,8 +132,11 @@ static void sched_domain_debug(struct sched_domain *sd, int cpu)
  {
       int level = 0;
 
-	if (!sched_debug_verbose)
+	if (!sched_debug_verbose) {
+		pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
+			     __func__);
               return;
+	}

Nit: I've been told not to break warnings over multiple lines so they can be grep'ed, but given this has the "sched_domain_debug:" prefix I think we could get away with the below.

Regardless:

Reviewed-by Valentin Schneider <[email protected]>

---

kernel/sched/topology.c +0 -0
index d7254c47af455..b4dc7c7d2c41c 100644
@@ -133,7 +133,8 @@ static void sched_domain_debug(struct sched_domain *sd, int cpu)
  	int level = 0;
Quoted 16 lines

kernel/sched/topology.c static void sched_domain_debug(struct sched_domain *sd, int cpu) · line 140

Quoted 7 lines
kernel/sched/topology.c +2 -1
  	if (!sched_debug_verbose) {
-		pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
+		pr_info_once("%s: Scheduler topology debugging disabled, "
+			     "add 'sched_verbose' to the cmdline to enable it\n",
  			     __func__);
  		return;
  	}

Hi Valentin,
Splitting the warning to different lines give checkpatch error with --strict option. --fix option suggests we keep it like we have it originally(single line). Please let me know if you feel we can change it to something else or if you are fine with picking the change for next iteration. Thanks again.

Hah, didn't know that was in checkpatch :-) As I said before that really was more of a nitpick, consider me OK with the current patch.

author
#5 lore raw
Quoted 99 lines · Valentin Schneider
Quoted 81 lines · Naman Jain
Quoted 80 lines · Valentin Schneider
Quoted 53 lines · Naman Jain

From: Saurabh Sengar <[email protected]>

On a x86 system under test with 1780 CPUs, topology_span_sane() takes around 8 seconds cumulatively for all the iterations. It is an expensive operation which does the sanity of non-NUMA topology masks.

CPU topology is not something which changes very frequently hence make this check optional for the systems where the topology is trusted and need faster bootup.

Restrict this to sched_verbose kernel cmdline option so that this penalty can be avoided for the systems who want to avoid it.

Cc [email protected]
Fixes ccf74128d66c ("sched/topology: Assert non-NUMA topology masks don't (partially) overlap")
Signed-off-by Saurabh Sengar <[email protected]>
Co-developed-by Naman Jain <[email protected]>
Signed-off-by Naman Jain <[email protected]>
Tested-by K Prateek Nayak <[email protected]>

---
Changes since v4:
https://lore.kernel.org/all/[email protected]/
- Rephrased print statement and moved it to sched_domain_debug. (addressing Valentin's comments)
Changes since v3:
https://lore.kernel.org/all/[email protected]/
- Minor typo correction in comment
- Added Tested-by tag from Prateek for x86
Changes since v2:
https://lore.kernel.org/all/[email protected]/
- Use sched_debug() instead of using sched_debug_verbose variable directly (addressing Prateek's comment)

Changes since v1:
https://lore.kernel.org/all/[email protected]/
- Use kernel cmdline param instead of compile time flag.

Adding a link to the other patch which is under review.
https://lore.kernel.org/lkml/[email protected]/
Above patch tries to optimize the topology sanity check, whereas this patch makes it optional. We believe both patches can coexist, as even with optimization, there will still be some performance overhead for this check.

---
   kernel/sched/topology.c | 9 ++++++++-
   1 file changed, 8 insertions(+), 1 deletion(-)
kernel/sched/topology.c +0 -0
index c49aea8c1025..d7254c47af45 100644

kernel/sched/topology.c static void sched_domain_debug(struct sched_domain *sd, int cpu) · line 140

Quoted 10 lines
kernel/sched/topology.c +4 -1
@@ -132,8 +132,11 @@ static void sched_domain_debug(struct sched_domain *sd, int cpu)
   {
        int level = 0;
 
-	if (!sched_debug_verbose)
+	if (!sched_debug_verbose) {
+		pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
+			     __func__);
                return;
+	}

Nit: I've been told not to break warnings over multiple lines so they can be grep'ed, but given this has the "sched_domain_debug:" prefix I think we could get away with the below.

Regardless:

Reviewed-by Valentin Schneider <[email protected]>

---

kernel/sched/topology.c +0 -0
index d7254c47af455..b4dc7c7d2c41c 100644
@@ -133,7 +133,8 @@ static void sched_domain_debug(struct sched_domain *sd, int cpu)
   	int level = 0;
Quoted 16 lines

kernel/sched/topology.c static void sched_domain_debug(struct sched_domain *sd, int cpu) · line 140

Quoted 7 lines
kernel/sched/topology.c +2 -1
   	if (!sched_debug_verbose) {
-		pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
+		pr_info_once("%s: Scheduler topology debugging disabled, "
+			     "add 'sched_verbose' to the cmdline to enable it\n",
   			     __func__);
   		return;
   	}

Hi Valentin,
Splitting the warning to different lines give checkpatch error with --strict option. --fix option suggests we keep it like we have it originally(single line). Please let me know if you feel we can change it to something else or if you are fine with picking the change for next iteration. Thanks again.

Quoted 2 lines

Hah, didn't know that was in checkpatch :-) As I said before that really was more of a nitpick, consider me OK with the current patch.

Hello maintainers,
Since merge window is currently open, I would like to request you to consider picking this change for the next release. Please ignore my email if you have already picked it or planning to do it.

Regards,
Naman