[PATCH v4] sched/topology: Enable topology_span_sane check only for debug builds

[PATCH v4] sched/topology: Enable topology_span_sane check only for debug builds

[Naman Jain]

From: Saurabh Sengar <ssengar@linux.microsoft.com>

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: stable@vger.kernel.org
Fixes: ccf74128d66c ("sched/topology: Assert non-NUMA topology masks don't (partially) overlap")
Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
Co-developed-by: Naman Jain <namjain@linux.microsoft.com>
Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>
---
Changes since v3:
https://lore.kernel.org/all/20250203114738.3109-1-namjain@linux.microsoft.com/
	- Minor typo correction in comment
	- Added Tested-by tag from Prateek for x86	
Changes since v2:
https://lore.kernel.org/all/1731922777-7121-1-git-send-email-ssengar@linux.microsoft.com/
	- Use sched_debug() instead of using sched_debug_verbose
	  variable directly (addressing Prateek's comment)

Changes since v1:
https://lore.kernel.org/all/1729619853-2597-1-git-send-email-ssengar@linux.microsoft.com/
	- 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/20241031200431.182443-1-steve.wahl@hpe.com/
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(+)

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index c49aea8c1025..666f0a18cc6c 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -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;
-- 
2.34.1

Re: [PATCH v4] sched/topology: Enable topology_span_sane check only for debug builds

[Valentin Schneider]

On 06/03/25 11:23, Naman Jain wrote:
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index c49aea8c1025..666f0a18cc6c 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -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"

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

Re: [PATCH v4] sched/topology: Enable topology_span_sane check only for debug builds

[Naman Jain]

On 3/6/2025 10:18 PM, Valentin Schneider wrote:
> On 06/03/25 11:23, Naman Jain wrote:
>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>> index c49aea8c1025..666f0a18cc6c 100644
>> --- a/kernel/sched/topology.c
>> +++ b/kernel/sched/topology.c
>> @@ -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"


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

--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -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

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

Re: [PATCH v4] sched/topology: Enable topology_span_sane check only for debug builds

[K Prateek Nayak]

Hello Naman,

On 3/7/2025 8:35 PM, Naman Jain wrote:
> 
> 
> On 3/6/2025 10:18 PM, Valentin Schneider wrote:
>> On 06/03/25 11:23, Naman Jain wrote:
>>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>>> index c49aea8c1025..666f0a18cc6c 100644
>>> --- a/kernel/sched/topology.c
>>> +++ b/kernel/sched/topology.c
>>> @@ -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"
> 
> 
> Thank you so much for reviewing.
> Please correct me if I misunderstood. Are you proposing below change?
> 
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -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().

-- 
Thanks and Regards,
Prateek

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

Re: [PATCH v4] sched/topology: Enable topology_span_sane check only for debug builds

[Naman Jain]

On 3/7/2025 9:11 PM, K Prateek Nayak wrote:
> Hello Naman,
> 
> On 3/7/2025 8:35 PM, Naman Jain wrote:
>>
>>
>> On 3/6/2025 10:18 PM, Valentin Schneider wrote:
>>> On 06/03/25 11:23, Naman Jain wrote:
>>>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>>>> index c49aea8c1025..666f0a18cc6c 100644
>>>> --- a/kernel/sched/topology.c
>>>> +++ b/kernel/sched/topology.c
>>>> @@ -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"
>>
>>
>> Thank you so much for reviewing.
>> Please correct me if I misunderstood. Are you proposing below change?
>>
>> --- a/kernel/sched/topology.c
>> +++ b/kernel/sched/topology.c
>> @@ -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().

Sure, thanks. Sent v5 with this change.

Regards,
Naman
>