Re: [PATCH v2 sched_ext/for-7.4-fixes] sched_ext: Fix stale pick when a wakeup resets the donor during dispatch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] sched_ext: unguarded keep-prev check in scx_dispatch_sched()
- [Medium] sched_ext: premature cpu_acquire() before stale donor check in dispatch_one()
- [Medium] sched: stale rq->dl_server across RETRY_TASK in __pick_next_task()
--

commit da4492c7a1c1f5575c5658737f248cb38f6f4291
Author: Tejun Heo <tj@kernel.org>

sched_ext: Fix stale pick when a wakeup resets the donor during dispatch

This commit restarts the sched_ext pick with SCX_DSP_RETRY when a wakeup or
sched_proxy_block_task() resets rq->donor while dispatch_one() or
scx_dispatch_sched() has the rq lock dropped. It also updates
scx_dispatch_sched() to re-evaluate whether prev is still the donor and on
the target scheduler before passing prev to ops.dispatch().

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index ffaab9fafca87..fe1be3d9444dd 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -3407,6 +3407,12 @@ static enum scx_dsp_verdict dispatch_one(struct rq *rq, struct task_struct *prev
>  		rq->scx.cpu_released = false;
>  	}
>  
> +	/* the ecaps sync may have dropped the rq lock */
> +	if (scx_dsp_prev_stale(rq, prev)) {
> +		verdict = SCX_DSP_RETRY;
> +		goto out;
> +	}

[Severity: Medium]
Should the scx_dsp_prev_stale() check happen immediately after
scx_process_sync_ecaps(), before checking rq->scx.cpu_released and invoking
ops.cpu_acquire()?

If scx_process_sync_ecaps() drops rq->__lock and another CPU wakes the SCX
proxy donor while rq->curr belongs to a higher scheduling class,
proxy_needs_return() calls proxy_reset_donor():

  proxy_needs_return()
    proxy_reset_donor()
      put_prev_set_next_task(rq, rq->donor, rq->curr)
        put_prev_task_scx(rq, prev, rq->curr)
          switch_class(rq, rq->curr)

That path invokes ops.cpu_release() and sets rq->scx.cpu_released = true, and
proxy_reset_donor() sets rq->donor = rq->curr.

When scx_process_sync_ecaps() returns to dispatch_one(), the
rq->scx.cpu_released block then sees rq->scx.cpu_released == true, calls
ops.cpu_acquire(), and clears rq->scx.cpu_released = false right before
scx_dsp_prev_stale() aborts the pick with SCX_DSP_RETRY.

On the restarted pick, rq->donor is already rq->curr in the higher class, so
put_prev_task_scx() will not run again. Wouldn't that leave
rq->scx.cpu_released false and the BPF scheduler thinking it owns the CPU
while a higher class runs?

> @@ -3449,8 +3455,19 @@ static enum scx_dsp_verdict dispatch_one(struct rq *rq, struct task_struct *prev
>  	}
>  
>  	verdict = scx_dispatch_sched(root_sch, rq, prev, false);
> -	if (verdict != SCX_DSP_NONE)
> +
> +	/* scx_dispatch_sched() may have dropped the rq lock */
> +	if (scx_dsp_prev_stale(rq, prev))
> +		verdict = SCX_DSP_RETRY;
> +
> +	switch (verdict) {
> +	case SCX_DSP_RETRY:
> +		goto out;

[Severity: Medium]
As reported by Andrea Righi on v1 [1], when dispatch_one() returns
SCX_DSP_RETRY during an ext_server pick, do_pick_task_scx() returns
RETRY_TASK to __pick_task_dl():

  __pick_next_task()
    pick_task_dl()
      __pick_task_dl()
        ext_server_pick_task()
          do_pick_task_scx()
            dispatch_pick()
              dispatch_one()

In kernel/sched/deadline.c:__pick_task_dl(), RETRY_TASK is non-NULL, so
rq->dl_server is set before returning:

	if (dl_server(dl_se)) {
		p = dl_se->server_pick_task(dl_se, rf);
		if (!p) {
			dl_server_stop(dl_se);
			goto again;
		}
		rq->dl_server = dl_se;
	}

In kernel/sched/core.c:__pick_next_task(), rq->dl_server = NULL is placed
above the restart label:

	rq->dl_server = NULL;

	if (scx_enabled())
		goto restart;
...
restart:
	prev_balance(rq, rf);

	for_each_active_class(class) {
		p = class->pick_task(rq, rf);
		if (unlikely(p == RETRY_TASK))
			goto restart;
		if (p) {
			put_prev_set_next_task(rq, rq->donor, p);
			return p;
		}
	}

If the retried pass picks a non-server task (for instance, if the woken donor
was the only sched_ext task and migrated away so ext_server_pick_task()
returns NULL and stops ext_server, or if a higher-priority task was woken
during the lock drop), won't put_prev_set_next_task() hand the stale
&rq->ext_server pointer in rq->dl_server to next->dl_server?

> diff --git a/kernel/sched/ext/inlines.h b/kernel/sched/ext/inlines.h
> index 1e39d12678c4d..fb0c58927d421 100644
> --- a/kernel/sched/ext/inlines.h
> +++ b/kernel/sched/ext/inlines.h
[ ... ]
> @@ -106,6 +124,11 @@ scx_dispatch_sched(struct scx_sched *sch, struct rq *rq,
>  			rq->scx.sub_dispatch_prev = prev;
>  #endif
>  
> +		/* the dispatch so far may have dropped the rq lock */
> +		prev_on_sch = !scx_dsp_prev_stale(rq, prev) &&
> +			      prev->sched_class == &ext_sched_class &&
> +			      scx_task_on_sched(sch, prev);
> +
>  		SCX_CALL_OP(sch, dispatch, rq, scx_cpu_arg(cpu),
>  			    prev_on_sch ? prev : NULL);

[Severity: High]
As reported by Sashiko on v1 [2], prev_on_sch guards passing prev to
ops.dispatch(), but right after scx_flush_dispatch_buf(sch, rq),
scx_dispatch_sched() still checks prev->scx.flags and prev->scx.slice
unconditionally:

		SCX_CALL_OP(sch, dispatch, rq, scx_cpu_arg(cpu),
			    prev_on_sch ? prev : NULL);

#ifdef CONFIG_EXT_SUB_SCHED
		if (!nested)
			rq->scx.sub_dispatch_prev = NULL;
#endif

		scx_flush_dispatch_buf(sch, rq);

		if ((prev->scx.flags & SCX_TASK_QUEUED) && prev->scx.slice)
			return SCX_DSP_PREV;
		if (rq->scx.local_dsq.nr)
			return SCX_DSP_LOCAL;
		if (scx_consume_global_dsq(sch, rq))
			return SCX_DSP_LOCAL;

If rq->__lock was dropped (either before ops.dispatch() or inside
scx_flush_dispatch_buf()) and another CPU woke the donor prev and enqueued it
remotely with SCX_TASK_QUEUED and a fresh slice, or if prev does not belong
to sch, won't this check read prev's state without holding its rq lock and
return SCX_DSP_PREV prematurely?

In that case:
- Tasks just dispatched to sch's global DSQ are left unconsumed because
  scx_dispatch_sched() returns before reaching scx_consume_global_dsq(sch, rq)
  or the dspc->nr_tasks retry loop.
- When called from scx_bpf_sub_dispatch(), returning SCX_DSP_PREV causes
  scx_bpf_sub_dispatch() (which tests verdict != SCX_DSP_NONE) to return true
  to the caller even though child did not dispatch a task, including inside
  one-shot ops.sub_ecaps_updated() / ops.sub_child_ecaps_updated() callbacks
  in __scx_process_sync_ecaps() that already drained rq->scx.ecaps_to_sync
  and will not re-run when dispatch_one() retries.
- If prev == rq->curr == rq->donor was in fair_sched_class when dispatch_one()
  started via ext_server_pick_task() and another CPU switched prev to
  SCHED_EXT during scx_flush_dispatch_buf(), scx_dsp_prev_stale(rq, prev) is
  still false while prev now has SCX_TASK_QUEUED and a default slice, so
  scx_dispatch_sched() returns SCX_DSP_PREV ahead of rq->scx.local_dsq.nr even
  though ops.dispatch() was passed NULL for prev.

Should the post-flush SCX_DSP_PREV check also require prev_on_sch and
!scx_dsp_prev_stale(rq, prev)?

[1] https://lore.kernel.org/r/auTNPtiaPyO9R5NG@gpd4
[2] https://lore.kernel.org/r/20261030142338.3453484-1-tj@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030235728.4098872-1-tj@kernel.org?part=1