Re: [PATCH] sched/fair: Let sync wakeups target the waker's core

[Madadi Vineeth Reddy]

On 05/08/26 09:00, K Prateek Nayak wrote:
> Hello Vineeth,
> 
> On 8/4/2026 5:43 PM, Madadi Vineeth Reddy wrote:
>> Hello Prateek,
>>
>> On 04/08/26 10:19, K Prateek Nayak wrote:
>>> Hello Vineeth,
>>>
>>> On 8/1/2026 9:25 AM, Madadi Vineeth Reddy wrote:
>>>> -static int select_idle_core(struct task_struct *p, int core, struct cpumask *cpus, int *idle_cpu)
>>>> +static int select_idle_core(struct task_struct *p, int core, struct cpumask *cpus,
>>>> +				int *idle_cpu, int sync_cpu)
>>>>  {
>>>>  	bool idle = true;
>>>>  	int cpu;
>>>>  
>>>>  	for_each_cpu(cpu, cpu_smt_mask(core)) {
>>>> -		if (!available_idle_cpu(cpu)) {
>>>> +		bool sync_waker = (cpu == sync_cpu);
>>>> +
>>>> +		/*
>>>> +		 * @sync_cpu, if set, is running a waker that is about to
>>>> +		 * block with nothing else runnable behind it. Treat it as
>>>> +		 * idle so this core stays an idle-core candidate: placing
>>>> +		 * the wakee on a sibling keeps the cache sharing that
>>>> +		 * stacking on the waker's rq would get, without serialising
>>>> +		 * the wakee behind the waker's remaining work.
>>>> +		 */
>>>> +		if (!available_idle_cpu(cpu) && !sync_waker) {
>>>
>>> If I'm not wrong, all you want to make is the sync_waker appear idle and
>>> then see if you can then consider that core as idle core or not right?
>>>
>>
>> Correct.
>>
>>> Why can't this be done in select_idle_sibling() extending that early
>>> check for (!has_idle_core && cpus_share_cache(prev, target)) condition
>>> and then initializing "idle_cpu" in select_idle_cpu() accordingly?
>>>
>>> Something along the lines of:
>>>
>>>   (Only build tested)
>>>
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index df8c9c2c7918..dd62bceb3838 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -1301,7 +1301,6 @@ static bool update_deadline(struct cfs_rq *cfs_rq, struct sched_entity *se)
>>>  
>>>  #include "pelt.h"
>>>  
>>> -static int select_idle_sibling(struct task_struct *p, int prev_cpu, int cpu);
>>>  static unsigned long task_h_load(struct task_struct *p);
>>>  static unsigned long capacity_of(int cpu);
>>>  
>>> @@ -8661,10 +8660,11 @@ static int select_idle_smt(struct task_struct *p, struct sched_domain *sd, int t
>>>   * comparing the average scan cost (tracked in sd->avg_scan_cost) against the
>>>   * average idle time for this rq (as found in rq->avg_idle).
>>>   */
>>> -static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool has_idle_core, int target)
>>> +static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool has_idle_core,
>>> +			   int target, int idle_cpu)
>>>  {
>>>  	struct cpumask *cpus = this_cpu_cpumask_var_ptr(select_rq_mask);
>>> -	int i, cpu, idle_cpu = -1, nr = INT_MAX;
>>> +	int i, cpu, nr = INT_MAX;
>>>  
>>>  	if (sched_feat(SIS_UTIL) && sd->shared) {
>>>  		/*
>>> @@ -8928,7 +8928,7 @@ static inline bool asym_fits_cpu(unsigned long util,
>>>  /*
>>>   * Try and locate an idle core/thread in the LLC cache domain.
>>>   */
>>> -static int select_idle_sibling(struct task_struct *p, int prev, int target)
>>> +static int select_idle_sibling(struct task_struct *p, int prev, int target, int sync_cpu)
>>>  {
>>>  	bool has_idle_core = false;
>>>  	struct sched_domain *sd;
>>> @@ -9028,16 +9028,19 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>>>  		return target;
>>>  
>>>  	if (sched_smt_active()) {
>>> +		int cpu = ((unsigned)sync_cpu < nr_cpumask_bits) ? sync_cpu : prev;
>>> +
>>>  		has_idle_core = test_idle_cores(target);
>>>  
>>> -		if (!has_idle_core && cpus_share_cache(prev, target)) {
>>> -			i = select_idle_smt(p, sd, prev);
>>> -			if ((unsigned int)i < nr_cpumask_bits)
>>> +		if (sync_cpu == target || (!has_idle_core && cpus_share_cache(prev, target))) {
>>> +			i = select_idle_smt(p, sd, cpu);
>>> +
>>> +			if (!has_idle_core && ((unsigned int)i < nr_cpumask_bits))
>>>  				return i;
>>>  		}
>>>  	}
>>>  
>>> -	i = select_idle_cpu(p, sd, has_idle_core, target);
>>> +	i = select_idle_cpu(p, sd, has_idle_core, target, i);
>>
>> `i` which is passed could be garbage value if we don't enter sched_smt_active block.
>>
>>>  	if ((unsigned)i < nr_cpumask_bits)
>>>  		return i;
>>>  
>>
>> This is different from what I wanted to achieve in a couple of ways.
>>
>> - Calling `select_idle_smt()` in sync case, would only give an idle CPU in that core but doesn't
>> test if that core is idle. That would be lost.
>> - You return `i` only when `!has_idle_core`, but I wanted to return waker core given that rest of the siblings
>> in that waker core are idle even though there are other idle cores present in the LLC.
>>
>> I agree that this could be done in `select_idle_sibling` but with a helper function.
>> Something like below (build tested)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index df8c9c2c7918..448af3c4b183 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -1301,7 +1301,7 @@ static bool update_deadline(struct cfs_rq *cfs_rq, struct sched_entity *se)
>>  
>>  #include "pelt.h"
>>  
>> -static int select_idle_sibling(struct task_struct *p, int prev_cpu, int cpu);
>> +static int select_idle_sibling(struct task_struct *p, int prev_cpu, int cpu, bool sync_core);
> 
> Is this forward declaration necessary? I think you can remove it
> entirely.

Right. This can be removed as definition is above the only caller.

> 
>>  static unsigned long task_h_load(struct task_struct *p);
>>  static unsigned long capacity_of(int cpu);
>>  
>> @@ -8656,6 +8656,27 @@ static int select_idle_smt(struct task_struct *p, struct sched_domain *sd, int t
>>  	return -1;
>>  }
>>  
>> +static int select_idle_sync_core(struct task_struct *p, struct sched_domain *sd,
>> +				 int target)
>> +{
>> +	int cpu, idle_sibling = -1;
>> +
>> +	for_each_cpu(cpu, cpu_smt_mask(target)) {
>> +		if (cpu == target)
>> +			continue;
>> +
>> +		if (!available_idle_cpu(cpu))
>> +			return -1;
> 
> Reads a lot like select_idle_smt(). Perhaps you can pass "has_idle_core"
> hint to the same and return idle CPU early if !has_idle_core or otherwise
> return the first idle sibling if core is idle.
> 
> That way even sync_core case is handled by the same function. You can
> gate the call with (sync_core || !has_idle_core)

Agreed. When has_idle_core is clear the LLC scan is not going to find a
fully idle core anyway, so an idle sibling on the waker's core is at least
as good.

> 
>> +
>> +		if (idle_sibling == -1 &&
>> +		    cpumask_test_cpu(cpu, sched_domain_span(sd)) &&
>> +		    cpumask_test_cpu(cpu, p->cpus_ptr))
>> +			idle_sibling = cpu;
>> +	}
>> +
>> +	return idle_sibling;
>> +}
>> +
>>  /*
>>   * Scan the LLC domain for idle CPUs; this is dynamically regulated by
>>   * comparing the average scan cost (tracked in sd->avg_scan_cost) against the
>> @@ -8928,7 +8949,7 @@ static inline bool asym_fits_cpu(unsigned long util,
>>  /*
>>   * Try and locate an idle core/thread in the LLC cache domain.
>>   */
>> -static int select_idle_sibling(struct task_struct *p, int prev, int target)
>> +static int select_idle_sibling(struct task_struct *p, int prev, int target, bool sync_core)
>>  {
>>  	bool has_idle_core = false;
>>  	struct sched_domain *sd;
>> @@ -9035,6 +9056,12 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>>  			if ((unsigned int)i < nr_cpumask_bits)
>>  				return i;
>>  		}
>> +
>> +		if (sync_core) {
>> +			i = select_idle_sync_core(p, sd, target);
>> +			if ((unsigned int)i < nr_cpumask_bits)
>> +				return i;
>> +		}
>>  	}
>>  
>>  	i = select_idle_cpu(p, sd, has_idle_core, target);
>> @@ -9733,8 +9760,16 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
>>  		return sched_balance_find_dst_cpu(sd, p, cpu, prev_cpu, sd_flag);
>>  
>>  	/* Fast path */
>> -	if (wake_flags & WF_TTWU)
>> -		return select_idle_sibling(p, prev_cpu, new_cpu);
>> +	if (wake_flags & WF_TTWU) {
>> +		bool sync_core = false;
>> +		if (want_affine && sync && new_cpu == cpu) {
>> +			struct rq *rq = cpu_rq(cpu);
>> +
>> +			sync_core = (rq->nr_running - cfs_h_nr_delayed(rq)) == 1;
> 
> Instead of computing this twice on wake_affine path, you can have a task
> flag like sched_task_hot that you just set in wake_affine_idle() before
> it returns early from the sync branch.
> 
> You can clear it up top in select_task_rq_fair() and check it
> select_idle_sibling() to conditionally call select_idle_sync_core().
> 

Fair point, will include this in the next version.

>> +		}
>> +
>> +		return select_idle_sibling(p, prev_cpu, new_cpu, sync_core);
>> +	}
>>  
>>  	return new_cpu;
>>  }
>>
>> This should also solve the issue raised by Zhan Xusheng and first target waker core given
>> that it is idle by giving exception to waker cpu.
>>
>> Thoughts?
> 
> Let me give it a spin. I'll report back if I see anything unexpected.

Thank you. Will post a new version with these set of changes and include numbers from Power10
box.

Thanks,
Vineeth

>