Re: [PATCH 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 2 potential issue(s) to consider:
- [High] sched_ext: stale prev and donor use in dispatch_one() after rq lock drop
- [Medium] sched: stale rq->dl_server across RETRY_TASK in __pick_next_task()
--

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

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

When proxy execution is enabled, a concurrent wakeup while dispatch_one()
releases the runqueue lock can reset rq->donor to rq->curr and migrate the
previous donor to another CPU. This commit checks whether prev != rq->donor
in dispatch_pick() after dispatch_one() returns and restarts the pick with
SCX_DSP_RETRY.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 904627ef909cb..c3f2f631dfafb 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -3919,6 +3919,16 @@ static enum scx_dsp_verdict dispatch_pick(struct rq *rq, struct rq_flags *rf,
>  	verdict = dispatch_one(rq, prev);
>  	rq_repin_lock(rq, rf);
>  
> +	/*
> +	 * With proxy execution, @prev is the donor and need not be the running
> +	 * task. A wakeup while dispatch_one() has the rq lock released can
> +	 * reset the donor and move @prev to another CPU. The verdict was
> +	 * computed for @prev and doesn't apply once the donor changed. Restart
> +	 * the pick.
> +	 */
> +	if (unlikely(prev != rq->donor))
> +		return SCX_DSP_RETRY;

[Severity: High]
Because dispatch_pick() checks prev != rq->donor only after dispatch_one()
has completed, can a donor change during an internal rq->__lock drop inside
dispatch_one() cause dispatch_one() and scx_dispatch_sched() to operate on a
stale prev task and a mismatched rq->donor before returning?

In dispatch_one(), scx_process_sync_ecaps() -> __scx_process_sync_ecaps() ->
scx_flush_dispatch_buf() can drop and reacquire rq->__lock:

kernel/sched/ext/ext.c:dispatch_one() {
	scx_process_sync_ecaps(rq, prev);
	...
	if (prev->sched_class == &ext_sched_class) {
		update_curr_scx(rq);
		...
		if (scx_bypassing(scx_task_sched(prev), cpu))
			expire_task_slice(prev);
}

If another CPU wakes the blocked donor prev during that window,
proxy_needs_return() resets rq->donor to rq->curr and try_to_wake_up() can
migrate and enqueue prev on a remote CPU. When scx_process_sync_ecaps()
returns:

1. dispatch_one() tests prev->sched_class == &ext_sched_class on the old
   donor and calls update_curr_scx(rq), which reads donor = rq->donor (now
   rq->curr). If rq->curr is not in ext_sched_class, update_curr_scx() runs
   apply_task_slice_oob() and update_curr_common() on the non-scx rq->curr
   and charges its execution delta to rq->ext_server.

2. scx_task_sched(prev) is called without holding prev->pi_lock or
   task_rq(prev)->__lock, triggering the rcu_dereference_protected() check,
   and expire_task_slice(prev) -> set_task_slice_keep_oob(prev, 0) trips
   lockdep_assert_rq_held(task_rq(p)) while zeroing prev->scx.slice on a
   remote runqueue.

Similarly, if rq->__lock is dropped there or inside scx_dispatch_sched()
(via scx_consume_global_dsq(), scx_consume_dispatch_q(), or
scx_flush_dispatch_buf()):

kernel/sched/ext/inlines.h:scx_dispatch_sched() {
		SCX_CALL_OP(sch, dispatch, rq, scx_cpu_arg(cpu),
			    prev_on_sch ? prev : NULL);
}

kernel/sched/ext/ext.c:dispatch_one() {
	if (prev->scx.flags & SCX_TASK_QUEUED) {
		struct scx_sched *prev_sch = scx_task_sched(prev);
		...
	}
	...
has_tasks:
	if (unlikely((rq->scx.local_dsq.nr > 1 || verdict == SCX_DSP_PREV) &&
		     rq->scx.nr_immed))
		scx_schedule_reenq_local(rq, 0);
}

Wouldn't scx_dispatch_sched() pass the migrated prev pointer to ops.dispatch,
dispatch_one() call scx_task_sched(prev) unlocked when prev is queued on its
new CPU, and the has_tasks path schedule a spurious local DSQ re-enqueue for
a lone SCX_ENQ_IMMED task if a stale SCX_DSP_PREV verdict was reached before
dispatch_pick() turns it into SCX_DSP_RETRY?

[Severity: Medium]
Also, when dispatch_pick() returns SCX_DSP_RETRY during a deadline server
pick (do_pick_task_scx() called from ext_server_pick_task() with
sched_core_enabled(rq) false), could returning RETRY_TASK leave a stale
rq->dl_server pointer across the restart in __pick_next_task()?

In __pick_task_dl(), a RETRY_TASK return from server_pick_task() is non-NULL,
so rq->dl_server is set before returning RETRY_TASK:

kernel/sched/deadline.c:__pick_task_dl() {
	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 __pick_next_task(), jumping to restart bypasses rq->dl_server = NULL:

kernel/sched/core.c:__pick_next_task() {
	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 retry pass ends up picking a non-server task (for instance, if the
migrated donor was the last scx task so ext_server_pick_task() returns NULL
and stops ext_server, or a higher-priority task became runnable while the rq
lock was dropped), wouldn't put_prev_set_next_task() ->
__put_prev_set_next_dl_server() assign the stale &rq->ext_server pointer to
next->dl_server on that task?

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