Re: [PATCH v25 1/9] sched: Make class_schedulers avoid pushing current, and get rid of proxy_tag_curr()

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

Hello John,

On 3/17/2026 10:19 AM, John Stultz wrote:
> On Sun, Mar 15, 2026 at 9:27 AM K Prateek Nayak <kprateek.nayak@amd.com> wrote:
>>
>> Back to my concern with the queuing of the balance_callback, and the
>> deadline and RT folks can keep me honest here, consider the following:
>>
>>     CPU0
>>     ====
>>
>>   ======> Task A (prio: 80)
>>   ...
>>
>>   mutex_lock(Mutex0)
>>   ... /* Executing critical section. */
>>
>>     =====> Interrupt: Wakes up Task B (prio: 50); B->blocked_on = Mutex0;
>>       resched_curr()
>>     <===== Interrupt return
>>   preempt_schedule_irq()
>>     schedule()
>>       put_prev_set_next_Task(A, B)
>>       rq->donor = B
>>       if (task_is_blocked(B)
>>         next = find_proxy_task() /* Return Task A */
>>       rq->curr = A
>>       queue_balance_callback()
>>     do_balance_callbacks()
>>       /* Finds A as task_on_cpu(); Does nothing. */
>>
>>   ... /* returns from schedule */
>>   ... /* continues with critical section */
>>
>>   mutex_unlock(Mutex0)
>>     mutex_handoff(B /* Task B */)
>>     preempt_disable()
>>       try_to_wake_up()
>>         resched_curr()
>>     preempt_enable()
>>       preempt_schedule()
>>         proxy_force_return()
>>           /* Returns to same CPU */
>>
>>         /*
>>          * put_prev_set_next_task() is skipped since
>>          * rq->donor context is same. no balance
>>          * callbacks are queued. Task A still on the
>>          * push list.
>>          */
>>         rq->donor = B
>>         rq->curr = B
>>   =======> sched_out: Task A
>>
>>   !!! No balance callback; Task A still on push list. !!!
>>
>>   <======= sched_in: Task B
> 
> Hrm. I'm feeling like I'm a little lost here, specifically after
> proxy_force_return(), since it doesn't exist yet at this point in the
> patch series.

Yeah I had to look a little bit ahead to poke holes here. Sorry about
that!

> But assuming we're at the "Handle blocked-waiter
> migration" point in the series, I'd think it would be something like:
> 
> rq->donor= B
> rq->curr = A
> << task A >>
> mutex_unlock(Mutex0)
>   mutex_handoff(B /* Task B */)
>    preempt_disable()
>      try_to_wake_up()
>        resched_curr()
>     preempt_enable()
>       preempt_schedule()
>         __schedule()
>           find_proxy_task()
>              proxy_force_return()
>              return NULL
>         pick_again:
>           next = pick_next_task()
>                       __pick_next_task() /* Returns B */
>           rq->donor =B
>           rq->curr = B
>           context_switch()
> <<switch to B >>
>           finish_task_switch()
>             finish_lock_switch()
>               __balance_callbacks()
> 
> Your point "put_prev_set_next_task() is skipped since rq->donor
> context is same" wasn't initially obvious to me, as the fair scheduler
> does have a (p == prev) check, but it doesn't enqueue balance
> callbacks.  And for RT/DL/SCX we should be using the pick_task()
> method, which calls put_prev_set_next_task() in __pick_next_task().
> But indeed, *inside* of put_prev_set_next_task() we return early if
> (next == prev).
> 
> So I see your concern and agree.
> 
>> So what I'm getting to is, if we find that rq->donor has not changed
>> with sched_proxy_exec() but rq->curr has changed during schedule(), we
>> should forcefully do a:
>>
>>   prev->sched_class->put_prev_task(rq, rq->donor, rq->donor /* or rq->idle / NULL ? */);
>>   next->sched_class->set_next_task(rq, rq->donor, true /* to queue balance callback. */);
>>
>> That way, when we do set_nex_task(), we see if we potentially have
>> tasks in the push list and queue a balance callback since the
>> task_on_cpu() condition may no longer apply to the tasks left behind
>> on the list.
>>
>> Thoughts?
> 
> Yeah. I wonder if we can express this inside of
> put_prev_set_next_task(). Reworking the shortcut to maybe:
>     if (next == prev && next != rq->curr)
> 
> I probably need to think on this tomorrow, as I suspect the above has
> some holes, but it seems like it would catch the cases that would
> matter

Also this needs to be done after find_proxy_task() since
"donor->blocked_on" needs to be cleared to queue callbacks else we'll
bail out on task_is_blocked() check in set_next_task.*() with
PROXY_WAKING when it is done as a part of pick_next_task().

> (maybe the issue is it catches too much - we'd probably also
> trip it if we A boosted B, and then we hit schedule and again chose A
> to boost B, which we probably could have skipped).

Ack! Doing it when the execution context changes with donor context
remaining the same would be the most optimal.

> 
> I guess adding a new helper function to manually do the
> put_prev/set_next could be added to the top level __schedule() logic
> in the (prev != next) case, though we'll have to preserve the
> prev_donor on the stack probably.

That seems like the best option to me too.

Also, deadline, RT, fair, and idle don't really care about the "next"
argument of put_prev_task() and the only one that does care is
put_prev_task_scx() to call switch_class() callback so putting it as
either NULL or "rq->donor" should be safe.

-- 
Thanks and Regards,
Prateek