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

[Valentin Schneider]

On 07/07/25 10:53, K Prateek Nayak wrote:
> Changelog v2..v3:
>
> o Added a check to skip uninitialized sd that can cause dereference of
>   sdd->sd beyond the percpu boundary (reported by Boris).
>
>   Tested on the trivial case using the QEMU cmdline:
>
>     sudo qemu-system-x86_64 -enable-kvm -cpu host -m 20G \
>     -smp cpus=10,socket=1,thread=10 -machine q35 \
>     -object memory-backend-ram,size=20G,id=m0 \
>     -numa node,cpus=0-9,memdev=m0,nodeid=0 \
>     ...
>

Urgh, of course directly using @sdd is not like walking up the sd hierarchy
where we end up getting a NULL sentinel... Sorry for suggesting that and
not thinking about that "small" detail, and thanks for being on top of it.

> ---
>  kernel/sched/topology.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index b958fe48e020..e682bf991ce6 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -2403,6 +2403,7 @@ static bool topology_span_sane(const struct cpumask *cpu_map)
>       id_seen = sched_domains_tmpmask2;
>
>       for_each_sd_topology(tl) {
> +		struct sd_data *sdd = &tl->data;
>
>               /* NUMA levels are allowed to overlap */
>               if (tl->flags & SDTL_OVERLAP)
> @@ -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.