Re: [PATCH v5] sched/fair: Preserve wake-affine CPU for non-SMT reciprocal sync wakeups

[Shubhang]

Hi Peter,

Thanks for the review.

On Mon, 12 Oct 2026, Peter Zijlstra wrote:

> On Thu, Oct 08, 2026 at 03:22:37PM -0700, Shubhang Kaushik (Ampere) wrote:
>
> > +static bool prefer_sync_pair_cpu(struct task_struct *p, int cpu)
> > +{
> > +	struct rq *rq = cpu_rq(cpu);
> > +
> > +	if ((rq->nr_running - cfs_h_nr_delayed(rq)) != 1)
> > +		return false;
> > +
> > +	if (sched_asym_cpucap_active()) {
> > +		sync_entity_load_avg(&p->se);
> > +		if (!task_fits_cpu(p, cpu))
> > +			return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  /*
> >   * Try and locate an idle core/thread in the LLC cache domain.
> >   */
> > @@ -9955,6 +9975,12 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
> >  			if (cpu != prev_cpu)
> >  				new_cpu = wake_affine(tmp, p, cpu, prev_cpu, sync);
> >
> > +			if (sync && !sched_smt_active() &&
> > +			    new_cpu == cpu &&
> > +			    p->last_wakee == current &&
> > +			    prefer_sync_pair_cpu(p, cpu))
> > +				return cpu;
> > +
> >  			sd = NULL; /* Prefer wake_affine over balance flags */
> >  			break;
> >  		}
>
> Hurmph.. just looking at this patch makes me go WTF. I've not read this
> lengthy Changelog you put on top, because none of that survives unless
> one goes trawl through git.
>
> So this code is confusing as heck. Nor is is evident why it wants to be
> !SMT.
>
> The way I read this:
>
> if the new_cpu is the current cpu (default case) AND
>    p was last woken by us AND
>    random
>      stay here.
>
> The function babbles something about a sync-pair, but then doesn't
> actually do anything with a pair, or ensures there is a pair.
>
> I'm supposing what you're getting at is the earlier:
>
> 'p->last_wakee == current' combined with the fact that current is now
> waking p
>
> But then I have to note that last_wakee isn't all that nice, since
> record_wakee() isn't guarded by in_task() -- it really should be.
>
> Try and convince me with readable code?

Yes, I see. It seems implicit that v5 relies on the earlier 
record_wakee(p) update to establish current->last_wakee == p, while the 
placement decision only shows p->last_wakee == current.

I will respin it so the reciprocal state is recorded as a local boolean
immediately after record_wakee(p):

        reciprocal_wakeup = in_task() &&
                current->last_wakee == p &&
                p->last_wakee == current;

The later placement decision will use that named state rather than a
bare last_wakee comparison.

Yes, I will add the in_task() guard to record_wakee() itself. Otherwise,
an IRQ/softirq wakeup records the interrupted task as the waker and can
create a false last_wakee relationship.

Lastly, I will make the non-SMT restriction explicit at the decision
point. On a non-SMT CPU, stacking the reciprocal WF_SYNC wakee behind
the waker preserves the intended handoff when the waker is its only
runnable task. On SMT, an idle sibling or idle core may run the wakee
in parallel, so select_idle_sibling() remains responsible for that
placement.

So I'll send a v6 with these changes accordingly.

Thanks,
Shubhang Kaushik