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

[Madadi Vineeth Reddy <vineethr@linux.ibm.com>]

Hi Chen Yu,

On 06/08/26 19:52, Chen Yu wrote:
> Hi Madadi,
> 
> On Thu, Aug 06, 2026 at 10:20:38AM +0530, Madadi Vineeth Reddy wrote:
> 
> [ ... ]
> 
>>>> -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;
> 
> If I understand correctly, the goal is to choose an idle SMT sibling as the waker
> CPU, if:
> 
> 1. the wakeup has WF_SYNC, and
> 2. the waker's SMT sibling CPUs are all idle, and
> 3. the waker is about to release the CPU.
> In this way, we can "stack" the wakee on a core that is about to become idle to
> get better cache locality.

Correct.

> 
> Condition 3 above might not always hold true, because WF_SYNC is not restricted to
> task context. softirq may also call wake_up_interruptible_sync_poll() with WF_SYNC,
> and in that case, current is whatever task the softirq happened to interrupt.
> 
> Given that, would it be reasonable to add in_task() check to gate the softirq case?
> 

Thanks for pointing this out. In the softirq case current is an arbitrary interrupted task
that resumes as soon as the softirq returns. I will add in_task() in v2.

> 
> =======================================================================================
> BTW, in your git log:
> "WF_SYNC tells the scheduler the waker is about to block ... when the waker's runqueue
> holds a single runnable task it returns the waker's CPU, select_idle_sibling() then
> discards that decision, because available_idle_cpu() is false for a CPU that is still
> running the waker"
> 
> Thanks for this description. I realized that WF_SYNC is not what I previously thought:
> stacking the wakee on the same CPU as the waker - that's not exactly right.
> Now my understanding is that, WF_SYNC is actually asking the wakee to find an idle CPU
> in the waker's LLC domain within select_idle_sibling(), humm, not sure if I missed anything:
> 
> sd = rcu_dereference_all(per_cpu(sd_llc, target));

Both wake_affine() branches bias towards waker's cpu on sync. target becomes waker's CPU
and select_idle_sibling() then searches the waker's sd_llc. So the hint directs the search
domain rather than a specific CPU is my understanding.

Thanks,
Vineeth

> ======================================================================================
> 
> thanks,
> Chenyu