Re: [PATCH 03/15] 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:
- [High] sched_ext: bypassed task stranding in scx_do_enqueue_task() on SCX_ENQ_IMMED
--

commit 51c04312bb9dbd18b0f365aada46b01365632700
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 scheduler holding only
SCX_CAP_ENQ_IMMED can dispatch bypassed tasks onto the local DSQ.

> 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;
>  		}
> @@ -63,7 +71,8 @@ scx_dispatch_sched(struct scx_sched *sch, struct rq *rq,
>  		struct scx_sched_pcpu *pcpu = per_cpu_ptr(sch->pcpu, cpu);
>  
>  		if (!(pcpu->bypass_host_seq++ % SCX_BYPASS_HOST_NTH) &&
> -		    scx_consume_dispatch_q(sch, rq, scx_bypass_dsq(sch, cpu), 0)) {
> +		    scx_consume_dispatch_q(sch, rq, scx_bypass_dsq(sch, cpu),
> +					   SCX_ENQ_IMMED)) {
>  			__scx_add_event(sch, SCX_EV_SUB_BYPASS_DISPATCH, 1);
>  			return SCX_DSP_LOCAL;
>  		}
> @@ -135,7 +144,7 @@ scx_dispatch_sched(struct scx_sched *sch, struct rq *rq,
>  	 * scheduler's ops.dispatch() doesn't yield any tasks.
>  	 */
>  	if (scx_bypass_dsp_enabled(sch) &&
> -	    scx_consume_dispatch_q(sch, rq, scx_bypass_dsq(sch, cpu), 0)) {
> +	    scx_consume_dispatch_q(sch, rq, scx_bypass_dsq(sch, cpu), SCX_ENQ_IMMED)) {

[Severity: High]
Can passing SCX_ENQ_IMMED to scx_consume_dispatch_q() here cause bypassed
tasks to be permanently stranded off all dispatch queues in
scx_do_enqueue_task() when reenq_local() reenqueues them?

When a bypassed task p is consumed into rq->scx.local_dsq with
SCX_ENQ_IMMED, dsq_inc_nr() marks it with SCX_TASK_IMMED. If p cannot run
right away (for example, because a higher-priority scheduling class preempts
rq, a head insert occurs ahead of p, or a parent scheduler queues its own
task onto rq->scx.local_dsq before calling scx_bpf_sub_dispatch()),
reenq_local() dequeues p from rq->scx.local_dsq and calls
scx_do_enqueue_task(rq, p, SCX_ENQ_REENQ, -1).

In kernel/sched/ext/ext.c:scx_do_enqueue_task(), sch is set to
scx_task_sched(p) (the bypassing scheduler that owns p, rather than the host
scheduler scx_exec_sched(sch, cpu_of(rq))), and the reenqueue repeat check
runs before the scx_bypassing(sch, cpu_of(rq)) check:

kernel/sched/ext/ext.c:scx_do_enqueue_task() {
	...
	if (enq_flags & SCX_ENQ_REENQ) {
		if (++p->scx.reenq_cnt > 1)
			__scx_add_event(sch, SCX_EV_REENQ_REPEAT, 1);

		if (unlikely(p->scx.reenq_cnt > SCX_REENQ_MAX_REPEAT)) {
			__scx_exit(sch, SCX_EXIT_ERROR_REENQ, 0, cpu_of(rq),
				   "%s[%d] reenqueued %u times without running",
				   p->comm, p->pid, p->scx.reenq_cnt);
			goto out;
		}
	}

	if (!scx_rq_online(rq))
		goto local;

	if (scx_bypassing(sch, cpu_of(rq))) {
		__scx_add_event(sch, SCX_EV_BYPASS_DISPATCH, 1);
		goto bypass;
	}
	...
}

Jumping to "goto out" leaves action at its initial value and returns without
inserting p into any DSQ, leaving p->scx.dsq == NULL while p remains queued
in sched_ext.

This appears to break in two scenarios:

1. If p originally triggered SCX_EXIT_ERROR_REENQ, its p->scx.reenq_cnt is
257 (SCX_REENQ_MAX_REPEAT + 1). When scx_sub_disable() or scx_root_disable()
calls scx_bypass(sch, true), it cycles runnable tasks with
DEQUEUE_SAVE | DEQUEUE_MOVE, which calls:

dequeue_task_scx() -> ops_dequeue() -> clr_task_runnable(p, false)

kernel/sched/ext/ext.c:clr_task_runnable() {
	list_del_init(&p->scx.runnable_node);
	WRITE_ONCE(p->scx.runnable_cpu, -1);
	if (reset_runnable_at) {
		p->scx.flags |= SCX_TASK_RESET_RUNNABLE_AT;
		p->scx.reenq_cnt = 0;
	}
}

Because reset_runnable_at is false, p->scx.reenq_cnt is not reset and stays
at 257 on bypass_dsq. On the very first occasion p is consumed with
SCX_ENQ_IMMED and reenqueued by reenq_local(), ++p->scx.reenq_cnt becomes
258, __scx_exit() is a no-op because sch already exited, and "goto out"
strands p off all DSQs after scx_bypass(sch, true) has already finished.

2. Even when p->scx.reenq_cnt starts at 0, if p is reenqueued 257 times while
bypassing (for example, when a parent scheduler repeatedly queues its own
task before calling scx_bpf_sub_dispatch()), scx_do_enqueue_task() calls
__scx_exit() on the bypassing task owner sch instead of the active host or
parent scheduler, and jumps to "goto out". Because sch is already in bypass
mode (bypass_depth >= 1), any subsequent scx_bypass(sch, true) sees
!changed in kernel/sched/ext/ext.c:scx_bypass():

	if (!changed)
		goto unlock;

and returns without cycling rq->scx.runnable_list, leaving p stranded off all
DSQs and potentially deadlocking kernel/sched/ext/sub.c:scx_sub_disable() or
scx_root_disable() if p holds a lock or resource waited on during teardown.

Should scx_do_enqueue_task() check scx_bypassing(sch, cpu_of(rq)) before the
SCX_ENQ_REENQ handling (or still route the task to "goto bypass" when the
reenqueue limit is hit), and should scx_bypass() also reset p->scx.reenq_cnt?

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