Re: [PATCH 2/2] sched/fair: Honor asymmetric SMT priority in idle selection

[Andrea Righi <arighi@nvidia.com>]

Hi Prateek,

On Mon, Sep 07, 2026 at 09:27:22AM +0530, K Prateek Nayak wrote:
> Hello Andrea,
> 
> On 9/4/2026 2:48 PM, Andrea Righi wrote:
> > +/*
> > + * Return true when @cpu has a higher asymmetric-packing priority than
> > + * @other in their shared SMT scheduling domain.
> > + */
> > +static bool sched_smt_asym_prefer(int cpu, int other)
> > +{
> > +	struct sched_domain *sd = rcu_dereference_all(cpu_rq(cpu)->sd);
> > +
> > +	if (!sd)
> > +		return false;
> > +
> > +	if (!(sd->flags & SD_SHARE_CPUCAPACITY) ||
> > +	    !(sd->flags & SD_ASYM_PACKING))
> > +		return false;
> > +
> > +	if (!cpumask_test_cpu(other, sched_domain_span(sd)))
> > +		return false;
> > +
> > +	return sched_asym_prefer(cpu, other);
> > +}
> > +
> > +/*
> > + * Return the highest-priority available CPU in @cpu's SMT core that is also in @cpus.
> > + */
> > +static int __select_idle_smt_cpu(struct task_struct *p, int cpu, const struct cpumask *cpus)
> > +{
> > +	int best = cpu;
> > +	int sibling;
> > +
> > +	for_each_cpu_and(sibling, cpu_smt_mask(cpu), cpus) {
> > +		if (sibling == best || !choose_idle_cpu(sibling, p))
> > +			continue;
> > +
> > +		if (sched_smt_asym_prefer(sibling, best))
> > +			best = sibling;
> 
> nit. Since sched_smt_asym_prefer() is only used here, and we know rq->sd
> is the one that can have SD_SHARE_CPUCAPACITY | SD_ASYM_PACKING, perhaps
> you can inline the check here do a:
> 
>     sd = rcu_dereference_all(cpu_rq(cpu)->sd);
> 
>     if (!sd)
>          return cpu;
> 
>    if (!(sd->flags & SD_SHARE_CPUCAPACITY) || !(sd->flags & SD_ASYM_PACKING))
>        return cpu;
> 
>    for_each_cpu_and (sibling, sched_domain_span(sd), cpus) {
>        ...
>    }
> 
>   ...
> 
> 
> That way, you don't need to dereference cpu_rq(cpu)->sd every time in
> sched_smt_asym_prefer() and check cpumask_test_cpu(). Both, domain
> span and task affinity will be covered at once.
> 
> Thoughts?

Yes, agreed. I like this way more.

> 
> > +	}
> > +
> > +	return best;
> > +}
> > +
> > +static inline int
> > +select_idle_smt_cpu(struct task_struct *p, int cpu, const struct cpumask *cpus)
> > +{
> > +	if (!sched_smt_asym_active())
> > +		return cpu;
> > +
> > +	return __select_idle_smt_cpu(p, cpu, cpus);
> > +}
> > +
> > +/*
> > + * Redirect an available SMT CPU to a higher-priority available sibling allowed by task affinity.
> > + */
> > +static inline int select_idle_smt_priority(struct task_struct *p, int cpu)
> > +{
> > +	return select_idle_smt_cpu(p, cpu, p->cpus_ptr);
> > +}
> > +
> >  /*
> >   * Scans the local SMT mask to see if the entire core is idle, and records this
> >   * information in sd_balance_shared->has_idle_cores.
> > @@ -8645,7 +8702,7 @@ static int select_idle_core(struct task_struct *p, int core, struct cpumask *cpu
> >  	}
> >  
> >  	if (idle)
> > -		return core;
> > +		return select_idle_smt_cpu(p, core, cpus);
> >  
> >  	cpumask_andnot(cpus, cpus, cpu_smt_mask(core));
> >  	return -1;
> > @@ -8668,7 +8725,7 @@ static int select_idle_smt(struct task_struct *p, struct sched_domain *sd, int t
> >  		if (!cpumask_test_cpu(cpu, sched_domain_span(sd)))
> >  			continue;
> >  		if (choose_idle_cpu(cpu, p))
> > -			return cpu;
> > +			return select_idle_smt_priority(p, cpu);
> >  	}
> >  
> >  	return -1;
> > @@ -8720,7 +8777,7 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
> >  						return -1;
> >  					idle_cpu = __select_idle_cpu(cpu, p);
> >  					if ((unsigned int)idle_cpu < nr_cpumask_bits)
> > -						return idle_cpu;
> > +						return select_idle_smt_priority(p, idle_cpu);
> 
> Question for Shrikanth: On larger SMT (SMT-4, SMT-8), does the ranking
> make that big of a difference if the core is already busy?
> 
> Does the overehead of additional search get offset by the benefit of
> being placed on a better ranked thread? If not, maybe the paths for
> !has_idle_core can stay as is?

On Olympus it'd be fine either way, since it's an SMT2. For wider SMT systems I
also defer the question to Shrikanth, I don't have any of them to test. :)

> 
> >  				}
> >  			}
> >  			cpumask_andnot(cpus, cpus, sched_group_span(sg));
> > @@ -8745,7 +8802,8 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
> >  	if (has_idle_core)
> >  		set_idle_cores(target, false);
> >  
> > -	return idle_cpu;
> > +	return (unsigned int)idle_cpu < nr_cpumask_bits ?
> > +		select_idle_smt_priority(p, idle_cpu) : idle_cpu;
> 
> Since every path does a select_idle_smt_priority() - be it coming from
> select_idle_core(), the early-return from the cluster scan, or just an
> idle CPU from the LLc scan, can't we simply just do it once in
> select_idle_sibling()?
> 
> Something like:

Yes, consolidating it in select_idle_sibling() looks cleaner. One comment below.

> 
>   (Only build tested)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index f79fcba4afec..7c97585141dd 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8964,7 +8964,7 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>  
>  	if (choose_idle_cpu(target, p) &&
>  	    asym_fits_cpu(task_util, util_min, util_max, target))
> -		return target;
> +		goto out;
>  
>  	/*
>  	 * If the previous CPU is cache affine and idle, don't be stupid:
> @@ -8974,8 +8974,10 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>  	    asym_fits_cpu(task_util, util_min, util_max, prev)) {
>  
>  		if (!static_branch_unlikely(&sched_cluster_active) ||
> -		    cpus_share_resources(prev, target))
> -			return prev;
> +		    cpus_share_resources(prev, target)) {
> +			target = prev;
> +			goto out;
> +		}
>  
>  		prev_aff = prev;
>  	}
> @@ -8993,7 +8995,8 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>  	    prev == smp_processor_id() &&
>  	    this_rq()->nr_running <= 1 &&
>  	    asym_fits_cpu(task_util, util_min, util_max, prev)) {
> -		return prev;
> +		target = prev;
> +		goto out;
>  	}
>  
>  	/* Check a recently used CPU as a potential idle candidate: */
> @@ -9007,8 +9010,10 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>  	    asym_fits_cpu(task_util, util_min, util_max, recent_used_cpu)) {
>  
>  		if (!static_branch_unlikely(&sched_cluster_active) ||
> -		    cpus_share_resources(recent_used_cpu, target))
> -			return recent_used_cpu;
> +		    cpus_share_resources(recent_used_cpu, target)) {
> +			target = recent_used_cpu;
> +			goto out;
> +		}
>  
>  	} else {
>  		recent_used_cpu = -1;
> @@ -9030,7 +9035,8 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>  		 */
>  		if (sd) {
>  			i = select_idle_capacity(p, sd, target);
> -			return ((unsigned)i < nr_cpumask_bits) ? i : target;
> +			target = ((unsigned)i < nr_cpumask_bits) ? i : target;
> +			goto out;
>  		}
>  	}
>  
> @@ -9043,27 +9049,31 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>  
>  		if (!has_idle_core && cpus_share_cache(prev, target)) {
>  			i = select_idle_smt(p, sd, prev);
> -			if ((unsigned int)i < nr_cpumask_bits)
> -				return i;
> +			if ((unsigned int)i < nr_cpumask_bits) {
> +				target = i;
> +				goto out;
> +			}
>  		}
>  	}
>  
>  	i = select_idle_cpu(p, sd, has_idle_core, target);
>  	if ((unsigned)i < nr_cpumask_bits)
> -		return i;
> -
> +		target = i;

Not sure about this final fallback. Is it worth doing an additional
select_idle_smt_priority() after idle scan failed or stopped because the
SIS_UTIL scan budget was exhausted?

It seems better to jump to out only when one of these paths has actually
selected a candidate:

	i = select_idle_cpu(p, sd, has_idle_core, target);
	if ((unsigned int)i < nr_cpumask_bits) {
		target = i;
		goto out;
	}

The prev_aff and recent_used_cpu fallbacks can jump to "out" as well, since they
were already verified as suitable candidates. If none of those paths succeeds, I
think the existing final "return target" should remain unchanged.

Does that make sense?

Thanks for looking at this!
-Andrea

>  	/*
>  	 * For cluster machines which have lower sharing cache like L2 or
>  	 * LLC Tag, we tend to find an idle CPU in the target's cluster
>  	 * first. But prev_cpu or recent_used_cpu may also be a good candidate,
>  	 * use them if possible when no idle CPU found in select_idle_cpu().
>  	 */
> -	if ((unsigned int)prev_aff < nr_cpumask_bits)
> -		return prev_aff;
> -	if ((unsigned int)recent_used_cpu < nr_cpumask_bits)
> -		return recent_used_cpu;
> +	else if ((unsigned int)prev_aff < nr_cpumask_bits)
> +		target = prev_aff;
> +	else if ((unsigned int)recent_used_cpu < nr_cpumask_bits)
> +		target = recent_used_cpu;
> +out:
> +	if (!sched_smt_asym_active())
> +		return target;
>  
> -	return target;
> +	return select_idle_smt_priority(p, target);
>  }
>  
>  /**
> ---
> 
> That way, it lives in a single place, and we don't have to pepper
> select_idle_smt_priority() everywhere. Thoughts?
> 
> -- 
> Thanks and Regards,
> Prateek
>