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

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

Hello Andrea,

On 9/7/2026 2:41 PM, Andrea Righi wrote:
>>> @@ -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. :)

Same! Best I can do is a VM with -cpus ...,threads=8 but performance on
those are super flaky to make any meaningful deductions.

> 
>>
>>>  				}
>>>  			}
>>>  			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?

I see what you mean! We'll end up doing a:

  select_idle_smt_priority(p, target)

at the end which might indeed be wasteful.
> 
> 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?

Correct me if I'm wrong but you are suggesting to keep the current
return intact and put out label after it like:

    /* If no suitable target was found */
    return target;

out:
   if (!sched_smt_asym_active())
       return target;

   return select_idle_smt_priority(p, target);
---

That makes sense to me!

-- 
Thanks and Regards,
Prateek