Re: [PATCH v3 8/8] sched/fair: Simplify SIS_UTIL handling in select_idle_cpu()

[Shrikanth Hegde <sshegde@linux.ibm.com>]

On 1/20/26 5:02 PM, K Prateek Nayak wrote:
> Use the "sd_llc" passed to select_idle_cpu() to obtain the
> "sd_llc_shared" instead of dereferencing the per-CPU variable.
> 
> Since "sd->shared" is always reclaimed at the same time as "sd" via
> call_rcu() and update_top_cache_domain() always ensures a valid
> "sd->shared" assignment when "sd_llc" is present, "sd_llc->shared" can
> always be dereferenced without needing an additional check.
> 
> While at it move the cpumask_and() operation after the SIS_UTIL bailout
> check to avoid unnecessarily computing the cpumask.
> 
> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
> ---
> Changelog rfc v2..v3:
> 
> o No changes. to the diff. Added more details on directly dereferencing
>    "sd->shared" without a NULL check in the commit message.
> ---
>   kernel/sched/fair.c | 19 ++++++++-----------
>   1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index c308c0700a7f..b4ae9444d32f 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7629,21 +7629,18 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
>   {
>   	struct cpumask *cpus = this_cpu_cpumask_var_ptr(select_rq_mask);
>   	int i, cpu, idle_cpu = -1, nr = INT_MAX;
> -	struct sched_domain_shared *sd_share;
> -
> -	cpumask_and(cpus, sched_domain_span(sd), p->cpus_ptr);
>   
>   	if (sched_feat(SIS_UTIL)) {
> -		sd_share = rcu_dereference_all(per_cpu(sd_llc_shared, target));
> -		if (sd_share) {
> -			/* because !--nr is the condition to stop scan */
> -			nr = READ_ONCE(sd_share->nr_idle_scan) + 1;
> -			/* overloaded LLC is unlikely to have idle cpu/core */
> -			if (nr == 1)
> -				return -1;
> -		}
> +		/* because !--nr is the condition to stop scan */
> +		nr = READ_ONCE(sd->shared->nr_idle_scan) + 1;
> +		/* overloaded LLC is unlikely to have idle cpu/core */
> +		if (nr == 1)
> +			return -1;
>   	}


I stared at sd->shared->nr_idle_scan for a while to see why it is safe
even when lets say there is no LLC domain.

It is because it is sd_llc here. Not any other domains. and
there is sd_llc check before calling select_idle_cpu.

So maybe add a comment here, saying null check for sd_llc is already there
and that's why it is safe to call it directly.

>   
> +	if (!cpumask_and(cpus, sched_domain_span(sd), p->cpus_ptr))
> +		return -1;
> +
>   	if (static_branch_unlikely(&sched_cluster_active)) {
>   		struct sched_group *sg = sd->groups;
>   

While reading this series, this reminded me we had discussed about unifying
sd_llc->shared and sd_llc_shared thing into one (in v1 or v2).
is that dropped or you plan to fix it after this series?


Other than minor comments and nits series looks good to me.
So, for the series.

Reviewed-by: Shrikanth Hegde <sshegde@linux.ibm.com>