Re: [PATCH 03/14] 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 2 potential issue(s) to consider:
- [High] sched_ext: bypassed tasks stranded on IMMED reenqueue in scx_do_enqueue_task()
- [Medium] sched_ext: IMMED bypass task not reenqueued in dispatch_one() on SCX_DSP_PREV
--

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

sched_ext: Fix bypassed tasks looping on a host without SCX_CAP_ENQ

This commit updates bypass dispatch queue consumption in scx_dispatch_sched()
to pass SCX_ENQ_IMMED so that a host scheduler holding only SCX_CAP_ENQ_IMMED
can consume bypassed tasks onto the local DSQ without rejecting them for
lacking 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: High]
Can consuming bypassed tasks with SCX_ENQ_IMMED here cause a bypassed task to
be stranded off all dispatch queues in scx_do_enqueue_task() when it gets
reenqueued from the local DSQ?

When scx_consume_dispatch_q() moves a bypassed task to rq->scx.local_dsq with
SCX_ENQ_IMMED, dsq_inc_nr() marks the task with SCX_TASK_IMMED. If the task
cannot run right away (for example, due to a higher-priority wakeup, a head
insert, or a parent scheduler queueing its own task ahead of
scx_bpf_sub_dispatch()), reenq_local() dequeues it from rq->scx.local_dsq and
calls scx_do_enqueue_task() with SCX_ENQ_REENQ.

In kernel/sched/ext/ext.c:scx_do_enqueue_task(), sch is resolved as
scx_task_sched(p) (the bypassing scheduler that owns p, rather than the host
scheduler), and the reenqueue limit is checked before scx_bypassing():

	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;
	}

This appears to break in two ways:

1. If a task previously triggered SCX_EXIT_ERROR_REENQ, it enters bypass with
   p->scx.reenq_cnt already at SCX_REENQ_MAX_REPEAT + 1 because cycling the
   task in scx_bypass() via dequeue_task_scx() -> ops_dequeue() ->
   clr_task_runnable(p, false) does not reset p->scx.reenq_cnt. On the very
   first local DSQ reenqueue during bypass, p->scx.reenq_cnt increments again
   and hits the goto out path before reaching goto bypass. Because
   scx_bypass(sch, true) in scx_sub_disable() or scx_root_disable() has
   already run, nothing picks the task back up, leaving it stranded off all
   DSQs while teardown waits for forward progress.

2. Even if p->scx.reenq_cnt starts at 0, if a bypassed task is reenqueued
   more than SCX_REENQ_MAX_REPEAT times (for example, because a parent
   scheduler repeatedly queues a task before calling scx_bpf_sub_dispatch()),
   __scx_exit() is called on the already-bypassing descendant sch instead of
   the host scheduler, and goto out strands the task off all DSQs.

Should scx_do_enqueue_task() check scx_bypassing(sch, cpu_of(rq)) before the
SCX_ENQ_REENQ handling, or attribute and handle reenqueues of bypassed tasks
against the host scheduler without stranding the task?

> @@ -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)) {
>  		__scx_add_event(sch, SCX_EV_SUB_BYPASS_DISPATCH, 1);
>  		return SCX_DSP_LOCAL;
>  	}

[Severity: Medium]
What happens if a child host holding only SCX_CAP_ENQ_IMMED consumes a single
bypassed task with SCX_ENQ_IMMED during scx_bpf_sub_dispatch() while
rq->scx.local_dsq is empty, and the parent scheduler's ops.dispatch() also
extends prev->scx.slice?

During dispatch_one(), SCX_RQ_IN_DISPATCH is set on rq->scx.flags, so
rq_is_open() returns true:

	if (rq->scx.flags & SCX_RQ_IN_DISPATCH)
		return true;

When the child consumes one bypassed task onto the empty rq->scx.local_dsq,
dsq_inc_nr() sees dsq->nr == 1 and rq_is_open() == true, so it increments
rq->scx.nr_immed to 1 without calling scx_schedule_reenq_local().

When control returns to the parent's scx_dispatch_sched(), extending
prev->scx.slice causes it to return SCX_DSP_PREV before checking
rq->scx.local_dsq.nr:

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

Then at has_tasks in kernel/sched/ext/ext.c:dispatch_one(), the post-dispatch
check only schedules a local reenqueue when rq->scx.local_dsq.nr > 1:

has_tasks:
	if (unlikely(rq->scx.local_dsq.nr > 1 && rq->scx.nr_immed))
		scx_schedule_reenq_local(rq, 0);

Because prev is rq->donor and not on rq->scx.local_dsq, rq->scx.local_dsq.nr
is 1, so scx_schedule_reenq_local() is not called. Doesn't this leave the
single bypassed task waiting on rq->scx.local_dsq behind prev without holding
SCX_CAP_ENQ and without being reenqueued back to the bypass DSQ?

[ ... ]

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