Re: [PATCH 04/17] sched_ext: Fix bypassed tasks looping on a host without SCX_CAP_ENQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] sched_ext: missing IMMED reenqueue in dispatch_one() on SCX_DSP_PREV
--

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

sched_ext: Fix bypassed tasks looping on a host without SCX_CAP_ENQ

This commit updates scx_dispatch_sched() to pass SCX_ENQ_IMMED when consuming
tasks from a scheduler's bypass DSQ so that a host holding only
SCX_CAP_ENQ_IMMED (SCX_CAP_BASE) can run bypassed tasks without needing
SCX_CAP_ENQ.

> diff --git a/kernel/sched/ext/inlines.h b/kernel/sched/ext/inlines.h
> index 8de9d8fb4b25f..1e39d12678c4d 100644
> --- a/kernel/sched/ext/inlines.h
> +++ b/kernel/sched/ext/inlines.h
> @@ -39,10 +39,18 @@ scx_dispatch_sched(struct scx_sched *sch, struct rq *rq,
>  	if (scx_consume_global_dsq(sch, rq))
>  		return SCX_DSP_LOCAL;
>  
> +	/*
> +	 * A bypass consume puts a task on the cpu being dispatched for, to run
> +	 * right away. That is an IMMED insert and needs only
> +	 * %SCX_CAP_ENQ_IMMED, the base cap. Dispatch descends into @sch only on
> +	 * a cpu where @sch holds the base cap, so @sch holds it here. The same
> +	 * holds for the consume at the end of this function.
> +	 */
>  	if (scx_bypass_dsp_enabled(sch)) {
>  		/* if @sch is bypassing, only the bypass DSQs are active */
>  		if (scx_bypassing(sch, cpu)) {
> -			if (scx_consume_dispatch_q(sch, rq, scx_bypass_dsq(sch, cpu), 0))
> +			if (scx_consume_dispatch_q(sch, rq, scx_bypass_dsq(sch, cpu),
> +						   SCX_ENQ_IMMED))
>  				return SCX_DSP_LOCAL;
>  			return SCX_DSP_NONE;
>  		}

[Severity: Medium]
Can a bypassed task consumed with SCX_ENQ_IMMED get left waiting on
rq->scx.local_dsq without being reenqueued when dispatch_one() finishes with
SCX_DSP_PREV?

During dispatch_one(), SCX_RQ_IN_DISPATCH is set on rq->scx.flags, which makes
rq_is_open() unconditionally return true so that resolution of whether prev
keeps running is deferred until the end of dispatch_one():

kernel/sched/ext/ext.c:rq_is_open() {
	...
	if (rq->scx.flags & SCX_RQ_IN_DISPATCH)
		return true;
	...
}

If a parent scheduler's ops.dispatch() calls scx_bpf_sub_dispatch() on a child
while rq->scx.local_dsq is empty and scx_dispatch_sched() consumes a single
task from scx_bypass_dsq(child, cpu) with SCX_ENQ_IMMED (or if
scx_process_sync_ecaps() queues a single SCX_ENQ_IMMED task during its
callbacks), dsq_inc_nr() sees dsq->nr == 1 and rq_is_open() == true, so it
does not call scx_schedule_reenq_local():

kernel/sched/ext/ext.c:dsq_inc_nr() {
	...
	if (p->scx.flags & SCX_TASK_IMMED) {
		...
		rq->scx.nr_immed++;

		if (unlikely(dsq->nr > 1 || !rq_is_open(rq, enq_flags)))
			scx_schedule_reenq_local(rq, 0);
	}
}

If the parent's ops.dispatch() also extends prev->scx.slice (or prev already
had slice left after scx_process_sync_ecaps()), scx_dispatch_sched() returns
SCX_DSP_PREV before checking rq->scx.local_dsq.nr:

kernel/sched/ext/inlines.h:scx_dispatch_sched() {
	...
		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;
	...
}

At the has_tasks label in dispatch_one(), the check only tests whether
rq->scx.local_dsq.nr > 1:

kernel/sched/ext/ext.c:dispatch_one() {
	...
has_tasks:
	if (unlikely(rq->scx.local_dsq.nr > 1 && rq->scx.nr_immed))
		scx_schedule_reenq_local(rq, 0);

	rq->scx.flags &= ~SCX_RQ_IN_DISPATCH;
	return verdict;
}

Because prev is not on local_dsq, rq->scx.local_dsq.nr is 1, so
scx_schedule_reenq_local() is not called, and do_pick_task_scx() keeps running
prev while the single SCX_TASK_IMMED task remains queued on local_dsq (even
when its scheduler lacks SCX_CAP_ENQ).

Should the check at has_tasks in dispatch_one() also trigger
scx_schedule_reenq_local() when verdict == SCX_DSP_PREV and rq->scx.nr_immed
is non-zero, such as checking (rq->scx.local_dsq.nr > 1 || verdict ==
SCX_DSP_PREV) && rq->scx.nr_immed?

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