Re: [PATCH v3] sched/fair: Use sched_domain_span() for topology_span_sane()

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Valentin,

Thank you for taking a look!

On 7/8/2025 5:14 PM, Valentin Schneider wrote:
>> @@ -2418,22 +2419,34 @@ static bool topology_span_sane(const struct cpumask *cpu_map)
>>                * breaks the linking done for an earlier span.
>>                */
>>               for_each_cpu(cpu, cpu_map) {
>> -			const struct cpumask *tl_cpu_mask = tl->mask(cpu);
>> +			struct sched_domain *sd = *per_cpu_ptr(sdd->sd, cpu);
>> +			struct cpumask *sd_span = sched_domain_span(sd);
>>                       int id;
>>
>> +			/*
>> +			 * If a child level for a CPU has already covered
>> +			 * the entire cpumap, build_sched_domain() for the
>> +			 * domains above is skipped. Use sd->private to detect
>> +			 * levels that have not been initialized in the CPU's
>> +			 * hierarchy and skip them.
>> +			 */
>> +			if (!sd->private)
>> +				continue;
>> +
> 
> So this works, but how about using a cpumask_empty(sd_span) check instead?
> It's IMO a bit more future proof than relying on how sd->private is used.

How about the following instead of cpumask_empty() to avoid two cpumask
operation in the common case where sd is initialized:

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index e682bf991ce6..e889ae012c17 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -2423,19 +2423,13 @@ static bool topology_span_sane(const struct cpumask *cpu_map)
 			struct cpumask *sd_span = sched_domain_span(sd);
 			int id;
 
-			/*
-			 * If a child level for a CPU has already covered
-			 * the entire cpumap, build_sched_domain() for the
-			 * domains above is skipped. Use sd->private to detect
-			 * levels that have not been initialized in the CPU's
-			 * hierarchy and skip them.
-			 */
-			if (!sd->private)
-				continue;
-
 			/* lowest bit set in this mask is used as a unique id */
 			id = cpumask_first(sd_span);
 
+			/* Skip if span is empty */
+			if (id >= nr_cpu_ids)
+				continue;
+
 			if (cpumask_test_cpu(id, id_seen)) {
 				/* First CPU has already been seen, ensure identical sd spans */
 				sd = *per_cpu_ptr(sdd->sd, id);
---

-- 
Thanks and Regards,
Prateek