Re: [PATCH RFC v3] sched/proxy: Defer donor commit until after proxy resolution

[Xukai Wang <kingxukai@zohomail.com>]

Hi Prateek,

Thanks for the detailed reply.

On 2026/10/12 17:21, K Prateek Nayak wrote:
> I see. I guess, proxy_reshed_idle() would be a better spot to
> consolidate it rather than juggling it in __schedule().
>
>> The next pick clearing rq->dl_server does not undo that assignment,
>> since the pointer has already moved to rq->idle->dl_server. It is only
>> cleared later when idle becomes the prev task of another
>> put_prev_set_next_task() transition.
> Which should be the very next __schedule() since we retain NEED_RESCHED
> except if all the tasks migrate away and we genuinely select rq->idle,
> then the dl_server lingers around.
...
>> If proxy resolution returns NULL or idle, the saved value is not
>> restored into rq->dl_server for a donor commit that will not happen.
>>
>> So the intent is to keep the DL-server selection made for the candidate
>> out of rq->dl_server while proxy resolution is in progress, and restore
>> it only if that donor is actually going to be committed, while keeping
>> the state seen by find_proxy_task() consistent with the pre-patch path.
>>
>> Do you think rq->idle->dl_server being set in the second sequence above
>> is harmless, or is keeping the save/clear/restore the right way to avoid
>> that state?
> I assume it may take the dl_defer_idle path but I'm not super sure. I
> personally prefer resetting state in proxy_resched_idle() since it is
> all in one place to refer at any point.

I agree with your suggestion. So I removed the save/clear/restore around
find_proxy_task() and now clear rq->dl_server in proxy_resched_idle()
before committing idle.

This leaves the successful-resolution path untouched, while a NULL
retry lets the next pick_next_task() reset/recompute rq->dl_server.

> On current tip + sched-ext changes lined for v7.4 from
> https://lore.kernel.org/lkml/20260922165445.943315-9-arighi@nvidia.com/
> some sched-ext bits will need rq->donor to already reflect the next
> selection before they are called

I rebased this onto the latest tip, and also applied the patch on top of
Andrea's sched_ext proxy-exec changes through the curr/donor split to
check the resulting ordering.

The rebased scheduling path now looks roughly like this:

```

pick_again:
        donor = pick_next_task(rq, &rf);
        rq->next_class = donor->sched_class;
        next = donor;

        if (sched_proxy_exec()) {
                struct task_struct *prev_donor = rq->donor;

                donor->blocked_donor = NULL;

                if (unlikely(donor->is_blocked)) {
                        next = find_proxy_task(rq, donor, &rf);
                        if (!next)
                                goto pick_again;

                        if (next == rq->idle) {
                                scx_proxy_reenqueue_retry(rq, next);
                                goto keep_resched;
                        }
                }

                put_prev_set_next_task(rq, prev_donor, donor);

                if (donor == prev_donor && prev != next) {
                        donor->sched_class->put_prev_task(rq, donor, donor);
                        donor->sched_class->set_next_task(rq, donor,
SNT_PICK);
                }

                rq_set_donor(rq, donor);

                scx_proxy_donor_start(rq);
                scx_proxy_reenqueue_retry(rq, next);
        } else {
                put_prev_set_next_task(rq, rq->donor, donor);
                rq_set_donor(rq, donor);
        }
```

and proxy_resched_idle() now does:

```

proxy_resched_idle()

{

    rq->dl_server = NULL;

    put_prev_set_next_task(...);

    ...

}

```

So the proxy-specific handling stays in one place in __schedule(), and
the existing sched_ext hooks remain in their current order.

I also looked more closely at the rq->donor dependency
in do_pick_task_scx():

```

static struct task_struct *do_pick_task_scx(...)

{

    struct task_struct *prev = rq->donor;

    ...

}

```

Suppose A is the currently committed donor and the first SCX pick
selects blocked donor D.

With the current early-commit ordering, the sequence is roughly:

```

    next = pick_next_task(); /* rq->donor is A */

        |-> pick_next_task_scx(); /* SCX pick with rq->donor
is A, selects D */

        |-> set_next_task_scx(D); /* D is removed from the local DSQ */

    rq_set_donor(rq, D); /*rq->donor is D */;

    next = find_proxy_task(D); /* if next == NULL */

    if (!next) goto pick_again;


pick_again:    

    pick_next_task(); /* rq->donor is D */

        |-> pick_next_task_scx(); /* SCX pick with prev is D, and D may
be retained through SCX_DSP_PREV. */

```


With the deferred ordering:

```

    pick_next_task(); /* rq->donor is A */

        |-> pick_next_task_scx(prev=A); /* selects D */

    next = find_proxy_task(D); /* if next == NULL */

    if (!next) goto pick_again;


pick_again:    

    pick_next_task(); /* rq->donor is A */

        |-> pick_next_task_scx(prev=A); /* D is still SCX_TASK_QUEUED
and still on the local DSQ, because set_next_task_scx() has not run yet. */

```

If find_proxy_task() did not otherwise change the relevant runqueue/SCX
state, the retry therefore starts from the same committed prev=A, while
D remains the local candidate selected by the first pass. Since the
first pass did not retain A through SCX_DSP_PREV, the retry reaches the
local DSQ again and selects D through SCX_DSP_LOCAL.

So the internal retry mechanism changes from `provisional D ->
SCX_DSP_PREV` to `committed A + queued D -> SCX_DSP_LOCAL`  without
changing the candidate in the unchanged-state case.

If find_proxy_task() deactivates or migrates D, then the runqueue state
has genuinely changed and the retry should of course select against that
new state. In that case it seems preferable for rq->donor to continue
describing the last committed scheduling context rather than committing
D provisionally and repairing that state afterwards.

Similarly, if SCX dispatch drops the rq lock, the existing
lock_drop_seq / SCX_DSP_RETRY handling already restarts selection
against the new state, so I don't see a need for an early rq->donor
update there either.

For a successful proxy resolution, the existing sched_ext ordering still
works unchanged:

```

find_proxy_task(D);

put_prev_set_next_task(..., D);

rq_set_donor(D);

scx_proxy_donor_start(); /* ops.running(D) is still issued only after D
has become the committed rq->donor. */

```

Given this, I don't see an additional sched_ext complexity introduced by
deferring the commit; the main difference is that rq->donor continues to
describe the committed scheduling context while proxy resolution is
still provisional.

Also, I probably wouldn't characterize the main benefit of this change
as a performance optimization. The end-to-end performance impact I
measured is indeed small.

The main benefit I am after is the scheduling-state semantics. The
invariant I am trying to preserve is that rq->donor and the
scheduler-class current state describe a scheduling context which has
survived proxy resolution, rather than a provisional candidate which
may still be deactivated, migrated, or cause the proxy walk to retry.

In other words, the intended ordering is:

1. pick candidate

2. resolve proxy chain

3. commit donor / scheduler-class state

rather than:

1. pick candidate

    -> commit donor / scheduler-class state

2. resolve proxy chain

    -> possibly discard that candidate and retry 1.


The reduction in redundant put_prev_task()/set_next_task() callbacks is
a consequence of keeping that commit boundary after proxy resolution,
rather than the primary reason for the change.

The sched_ext integration also seems to illustrate why keeping the
commit boundary after proxy resolution is useful. In Andrea's
'sched_ext: Fix ops.running/stopping() pairing for proxy-exec donors'
commit (5f882051efabaf19f9b4dbe4b50cee1d250ac19e), set_next_task_scx()
has to treat a blocked donor specially and defer ops.running() to
scx_proxy_donor_start(), specifically because proxy resolution can still
fail or migrate the donor after set_next_task_scx() has run.

> Thank you but I'd wait for Peter and John to chime in regarding the added
> code complexity given the visible benefits of this optimization isn't
> too much.

Thanks. I'll wait for Peter/John's thoughts before sending v4.

-- 
Best regards,
Xukai