Re: [PATCH v23 7/9] sched: Have try_to_wake_up() handle return-migration for PROXY_WAKING case

[John Stultz]

On Thu, Oct 30, 2025 at 9:28 PM K Prateek Nayak <kprateek.nayak@xxxxxxx> wrote:
> On 10/30/2025 5:48 AM, John Stultz wrote:
> > +/*
> > + * Checks to see if task p has been proxy-migrated to another rq
> > + * and needs to be returned. If so, we deactivate the task here
> > + * so that it can be properly woken up on the p->wake_cpu
> > + * (or whichever cpu select_task_rq() picks at the bottom of
> > + * try_to_wake_up()
> > + */
> > +static inline bool proxy_needs_return(struct rq *rq, struct task_struct *p)
> > +{
> > +     bool ret = false;
> > +
> > +     if (!sched_proxy_exec())
> > +             return false;
> > +
> > +     raw_spin_lock(&p->blocked_lock);
> > +     if (p->blocked_on == PROXY_WAKING) {
> > +             if (!task_current(rq, p) && p->wake_cpu != cpu_of(rq)) {
> > +                     if (task_current_donor(rq, p))
> > +                             proxy_resched_idle(rq);
> > +
> > +                     deactivate_task(rq, p, DEQUEUE_NOCLOCK);
> > +                     ret = true;
> > +             }
> > +             __clear_task_blocked_on(p, PROXY_WAKING);
> > +             resched_curr(rq);
>
> Do we need to resched_curr() if we've simply dequeued a waiting
> owner who is neither the current, nor the donor? I would have

Hrm. Yeah, I guess really we only need to resched_curr() when we are
skipping the deactivation because already we're waking on the wake_cpu
rq.

I'll fix that up.

> preferred if this function was similar to ttwu_queue_cond() with
> each step explaining the the intent - something like:
>
> static inline bool
> proxy_needs_return(struct rq *rq, struct task_struct *p, int wake_flags)
> {
>         if (!sched_proxy_exec())
>                 return false;
>
>         guard(raw_spinlock)(&p->blocked_lock);
>
>         /* Task has been woken up / is running. */
>         if (p->blocked_on != PROXY_WAKING)
>                 return false;
>
>         __clear_task_blocked_on(p, PROXY_WAKING);

Ah, this is nicer! I'll rework the function in this style! Thanks for
that suggestion!

> > +     /*
> > +      * If we're PROXY_WAKING, we have deactivated on this cpu, so we should
> > +      * activate it here as well, to avoid IPI'ing a cpu that is stuck in
> > +      * task_rq_lock() spinning on p->on_rq, deadlocking that cpu.
> > +      */
>
> Sounds like block_task() would be better than deactivate_task() above
> in that case. Anything that is waiting on the task's state change takes
> the pi_lock afaik and the wakeup is always done with pi_lock held so
> blocking the task shouldn't cause any problems based on my reading.

So earlier I did try using block_task() but it always seemed to run
into crashes, which I assumed was because other cpus were picking the
task up as it wasn't on_rq (any references to a task after
block_task() in other situations often runs into this trouble).

But your point about the pi_lock being held is a good one, so I will
tinker and think a bit more on this.

Thanks as always for the review!
-john