sched/topology: Enable topology_span_sane check only for debug builds

PATCH Superseded Naman Jain 3 participants 5 messages Mar 6, 2025 – Mar 10, 2025

Subsystems (2)

Stable branch Scheduler

lore

Share & tools

lore

Naman Jain

author

2025-03-06 05:54 UTC #0 lore raw

PATCH v4 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 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 | 7 +++++++
 1 file changed, 7 insertions(+)

kernel/sched/topology.c +7 -0

index c49aea8c1025..666f0a18cc6c 100644
@@ -2359,6 +2359,13 @@ 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()) {
+		pr_info_once("%s: Skipping topology span sanity check. Use `sched_verbose` boot parameter to enable it.\n",
+			     __func__);
+		return true;
+	}
+
 	/* NUMA levels are allowed to overlap */
 	if (tl->flags & SDTL_OVERLAP)
 		return true;

Signature · 1 line

2.34.1

Valentin Schneider

reviewer

2025-03-06 16:48 UTC #1 lore raw

kernel/sched/topology.c static bool topology_span_sane(struct sched_domain_topology_level *tl, · line 2365

Quoted 12 lines · Naman Jain

kernel/sched/topology.c +4 -0

index c49aea8c1025..666f0a18cc6c 100644
@@ -2359,6 +2359,13 @@ 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()) {
+		pr_info_once("%s: Skipping topology span sanity check. Use `sched_verbose` boot parameter to enable it.\n",
+			     __func__);

FWIW I'm not against this change, however if you want to add messaging about sched_verbose I'd put that in e.g. sched_domain_debug() (as a print once like you've done here) with something along the lines of:

"Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it"

Quoted 8 lines

kernel/sched/topology.c +3 -1

+		return true;
+	}
+
      /* NUMA levels are allowed to overlap */
      if (tl->flags & SDTL_OVERLAP)
              return true;
--

2.34.1

Naman Jain

author

2025-03-07 15:05 UTC #2 lore raw

Quoted 18 lines · Valentin Schneider

kernel/sched/topology.c static bool topology_span_sane(struct sched_domain_topology_level *tl, · line 2365
Quoted 12 lines · Naman Jain
kernel/sched/topology.c +4 -0
index c49aea8c1025..666f0a18cc6c 100644
@@ -2359,6 +2359,13 @@ 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()) {
+		pr_info_once("%s: Skipping topology span sanity check. Use `sched_verbose` boot parameter to enable it.\n",
+			     __func__);
FWIW I'm not against this change, however if you want to add messaging about sched_verbose I'd put that in e.g. sched_domain_debug() (as a print once like you've done here) with something along the lines of:

Quoted 1 line

"Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it"

Thank you so much for reviewing.
Please correct me if I misunderstood. Are you proposing below change?

kernel/sched/topology.c +0 -0

@@ -2361,7 +2361,7 @@ static bool topology_span_sane(struct sched_domain_topology_level *tl,
 
        /* Skip the topology sanity check for non-debug, as it is a

time-consuming operation */

        if (!sched_debug()) {
-               pr_info_once("%s: Skipping topology span sanity check. Use `sched_verbose` boot parameter to enable it.\n",
+               pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
                             __func__);
                return true;
        }

Regards,
Naman

Quoted 9 lines

kernel/sched/topology.c +3 -1

+		return true;
+	}
+
       /* NUMA levels are allowed to overlap */
       if (tl->flags & SDTL_OVERLAP)
               return true;
--

2.34.1

K Prateek Nayak

reviewer

2025-03-07 15:42 UTC #3 lore raw

Hello Naman,

Quoted 31 lines · Naman Jain

Quoted 18 lines · Valentin Schneider
kernel/sched/topology.c static bool topology_span_sane(struct sched_domain_topology_level *tl, · line 2365
Quoted 12 lines · Naman Jain
kernel/sched/topology.c +4 -0
index c49aea8c1025..666f0a18cc6c 100644
@@ -2359,6 +2359,13 @@ 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()) {
+        pr_info_once("%s: Skipping topology span sanity check. Use `sched_verbose` boot parameter to enable it.\n",
+                 __func__);
FWIW I'm not against this change, however if you want to add messaging about sched_verbose I'd put that in e.g. sched_domain_debug() (as a print once like you've done here) with something along the lines of:
Quoted 1 line
"Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it"
Thank you so much for reviewing.
Please correct me if I misunderstood. Are you proposing below change?

kernel/sched/topology.c static bool topology_span_sane(struct sched_domain_topology_level *tl, · line 2367

Quoted 8 lines

kernel/sched/topology.c +1 -1

@@ -2361,7 +2361,7 @@ static bool topology_span_sane(struct sched_domain_topology_level *tl,
 
         /* Skip the topology sanity check for non-debug, as it is a time-consuming operation */
         if (!sched_debug()) {
-               pr_info_once("%s: Skipping topology span sanity check. Use `sched_verbose` boot parameter to enable it.\n",
+               pr_info_once("%s: Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it\n",
                              __func__);
                 return true;
         }

I think Valentin meant moving the same pr_info_once() to the early exit case in sched_domain_debug() for "!sched_debug_verbose" to notify the user that sched_debug() is disabled and they can turn it on using "sched_verbose" as opposed to announcing it from topology_span_sane().

Signature · 2 lines

Thanks and Regards,
Prateek

Quoted 14 lines

Regards,
Naman

Quoted 9 lines

kernel/sched/topology.c +3 -0

+        return true;
+    }
+
        /* NUMA levels are allowed to overlap */
        if (tl->flags & SDTL_OVERLAP)
                return true;

Signature · 1 line

2.34.1

Naman Jain

author

2025-03-10 05:26 UTC #4 lore raw

Quoted 38 lines · K Prateek Nayak

Hello Naman,
Quoted 35 lines · Naman Jain
Quoted 21 lines · Valentin Schneider
Quoted 10 lines · Naman Jain
kernel/sched/topology.c +1 -0
index c49aea8c1025..666f0a18cc6c 100644
@@ -2359,6 +2359,13 @@ 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 
Quoted 5 lines
time-consuming operation */
+    if (!sched_debug()) {
+        pr_info_once("%s: Skipping topology span sanity check. Use sched_verbose boot parameter to enable it.\n",
+                 __func__);
FWIW I'm not against this change, however if you want to add messaging about sched_verbose I'd put that in e.g. sched_domain_debug() (as a print once like you've done here) with something along the lines of:
Quoted 1 line
"Scheduler topology debugging disabled, add 'sched_verbose' to the cmdline to enable it"
Thank you so much for reviewing.
Please correct me if I misunderstood. Are you proposing below change?
kernel/sched/topology.c +0 -0
@@ -2361,7 +2361,7 @@ static bool topology_span_sane(struct 
 sched_domain_topology_level *tl,
 

Quoted 15 lines

Quoted 10 lines
kernel/sched/topology.c +1 -1
          /* Skip the topology sanity check for non-debug, as it is a time-consuming operation */
          if (!sched_debug()) {
-               pr_info_once("%s: Skipping topology span sanity check. 
 Use `sched_verbose` boot parameter to enable it.\n",
+               pr_info_once("%s: Scheduler topology debugging 
 disabled, add 'sched_verbose' to the cmdline to enable it\n",
                              __func__);
                 return true;
         }
I think Valentin meant moving the same pr_info_once() to the early exit case in sched_domain_debug() for "!sched_debug_verbose" to notify the user that sched_debug() is disabled and they can turn it on using "sched_verbose" as opposed to announcing it from topology_span_sane().

Sure, thanks. Sent v5 with this change.

Regards,
Naman

Quoted 1 line