Re: [PATCH v2 7/8] sched/topology: Avoid redundant percpu allocations and bitmap walks in build_sched_domains()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] sched/topology: sched_update_llc_bytes() reuses llc_bytes outside cpus
--

commit d95533f0e2e71e5e08ee625a1ea2afb7ecbe1b86
Author: Pasha Tatashin <pasha.tatashin@soleen.com>

sched/topology: Avoid redundant percpu allocations and bitmap walks in build_sched_domains()

This commit optimizes build_sched_domains() during CPU hotplug by tracking
uncovered CPUs during sched_group construction, reusing temporary per-CPU
pointer arrays across rebuilds, initializing sd->span_weight in sd_init(),
and reusing the first LLC sibling's computed llc_bytes in alloc_sd_llc()
and sched_update_llc_bytes().

> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index b5dec7f486b5c..33210fd9dc2c6 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -917,9 +917,15 @@ static bool alloc_sd_llc(const struct cpumask *cpu_map,
>  			top_llc = parent;
>  
>  		if (top_llc->flags & SD_SHARE_LLC) {
> +			int first = cpumask_first(sched_domain_span(top_llc));
> +
>  			sd->llc_max = max_lid + 1;
>  			sd->llc_counts = p;
> -			sd->llc_bytes = get_effective_llc_bytes(i, top_llc);
> +			/* All CPUs in top_llc share the same LLC size and weight. */
> +			if (first < i)
> +				sd->llc_bytes = (*per_cpu_ptr(d->sd, first))->llc_bytes;
> +			else
> +				sd->llc_bytes = get_effective_llc_bytes(i, top_llc);
>  		} else {
>  			/* avoid memory leak */
>  			kfree(p);
> @@ -1018,12 +1024,20 @@ void sched_update_llc_bytes(const struct cpumask *cpus)
>  	 * own LLC domain to scale its share of the physical cache.
>  	 */
>  	for_each_cpu(i, cpus) {
> +		int first;
> +
>  		sdp = rcu_dereference_sched_domain(per_cpu(sd_llc, i));
>  		if (!sdp)
>  			continue;
>  
>  		sd = rcu_dereference_sched_domain(cpu_rq(i)->sd);
> -		if (sd)
> +		if (!sd)
> +			continue;
> +
> +		first = cpumask_first(sched_domain_span(sdp));
> +		if (first < i)
> +			sd->llc_bytes = rcu_dereference_sched_domain(cpu_rq(first)->sd)->llc_bytes;
> +		else
>  			sd->llc_bytes = get_effective_llc_bytes(i, sdp);
>  	}

[Severity: Medium]
Can sched_update_llc_bytes() and alloc_sd_llc() copy an un-updated or
mismatched llc_bytes value when the topmost SD_SHARE_LLC domain spans
multiple cacheinfo LLC leaves?

When cacheinfo_cpu_online() or cacheinfo_cpu_pre_down() calls
sched_update_llc_bytes(cpus), cpus is a single cacheinfo leaf's
&llc->shared_cpu_map, whereas sdp (and top_llc in alloc_sd_llc()) is the
highest sched_domain with SD_SHARE_LLC, whose span comes from
cpu_coregroup_mask().

On topologies where cpu_coregroup_mask() is wider than the cacheinfo LLC
leaf's shared_cpu_map, sched_domain_span(sdp) spans multiple physical
cacheinfo LLCs:

1. On ARM64/RISC-V systems with clusters and per-core L2 caches (no shared
   L3), cpu_coregroup_mask() in drivers/base/arch_topology.c extends the MC
   domain to cluster_sibling:

   if (IS_ENABLED(CONFIG_SCHED_CLUSTER) &&
       cpumask_subset(core_mask, &cpu_topology[cpu].cluster_sibling))
           core_mask = &cpu_topology[cpu].cluster_sibling;

2. On AMD/Hygon x86 systems without an L3 cache, legacy_set_llc() in
   arch/x86/kernel/cpu/topology_amd.c sets llc_id to the die ID while
   get_cpu_cacheinfo_llc() returns the per-core L2 cache leaf:

   /* If none of the parsers set LLC ID then use the die ID for it. */
   if (tscan->c->topo.llc_id == BAD_APICID)
           tscan->c->topo.llc_id = apicid >> tscan->dom_shifts[TOPO_CORE_DOMAIN];

When sched_update_llc_bytes(cpus) is invoked for any core other than the
first core in sdp, first = cpumask_first(sched_domain_span(sdp)) is not in
cpus, so cpu_rq(first)->sd->llc_bytes is not updated by the
for_each_cpu(i, cpus) loop. Because first < i holds for every CPU i in cpus,
get_effective_llc_bytes() is skipped and sd->llc_bytes copies the un-updated
value from cpu_rq(first)->sd.

Similarly, in alloc_sd_llc(), copying (*per_cpu_ptr(d->sd, first))->llc_bytes
across all CPUs in top_llc reuses the first CPU's L2 cache calculation even
when other cores in top_llc have different cacheinfo shared_cpu_map weights
(for example, partially online SMT siblings) or cache sizes, which can cause
exceed_llc_capacity() in kernel/sched/fair.c to use the wrong capacity.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019011446.834487-1-pasha.tatashin@soleen.com?part=7