Re: [PATCH 10/14] sched_ext: Fix bypassed tasks stalling when the host lacks SCX_CAP_BASE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sched_ext: dispatch_one() ignores kernel slice on bypassing rescuee
--

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

sched_ext: Fix bypassed tasks stalling when the host lacks SCX_CAP_BASE

This commit prevents bypassed tasks from stalling on a CPU where their
non-bypassing host scheduler lacks SCX_CAP_BASE by routing them to rescue
on that CPU at enqueue time. It also updates put_prev_task_scx() and
scx_rescue_keep() so that a rescuee with a kernel slice stays on the local
DSQ under bypass.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index bc2ce76c8578a..f19161a0b4d7f 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c

[ ... ]

> @@ -3704,8 +3723,13 @@ static void put_prev_task_scx(struct rq *rq, struct task_struct *p,
>  		 * The exception is an IMMED donor put by proxy_resched_idle():
>  		 * the CPU isn't busy, proxy resolution is only retrying through
>  		 * idle, so keep the donor where it can be picked again.
> +		 *
> +		 * A task with a kernel slice stays on the local DSQ even while
> +		 * @sch bypasses. Under bypass, rescue runs @p when the host
> +		 * lacks %SCX_CAP_BASE, see bypass_enq_target_dsq().
>  		 */
> -		if (p->scx.slice && !scx_bypassing(sch, cpu_of(rq))) {
> +		if (p->scx.slice && (!scx_bypassing(sch, cpu_of(rq)) ||
> +				     scx_task_has_kernel_slice(p))) {
>  			if ((p->scx.flags & SCX_TASK_IMMED) && !proxy_put) {
>  				p->scx.flags |= SCX_TASK_REENQ_PREEMPTED;
>  				scx_do_enqueue_task(rq, p, SCX_ENQ_REENQ, -1);
> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index f2718a761fe7e..dc9374d42f796 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c

[ ... ]

> @@ -509,12 +509,14 @@ void scx_rescue_end(struct rq *rq)
>   * this when nothing else is runnable and put_prev_task_scx() when another task
>   * won the pick. scx_rescue_charge() ends the rescue the moment the admitted
>   * slice is fully served, so arriving here with the rescue still open means @p
> - * was preempted. Restore the unserved remainder and return %true. @p stays the
> - * rescuee. dispatch_one() keeps it running and put_prev_task_scx() reinserts it
> - * at the tail of the local DSQ, behind whatever preempted the rescuee.
> + * was preempted or, under bypass, that dispatch_one() declined to keep it
> + * running although it had slice left. Restore the unserved remainder and return
> + * %true. @p stays the rescuee. dispatch_one() keeps it running and
> + * put_prev_task_scx() reinserts it at the tail of the local DSQ, behind
> + * whatever preempted the rescuee.
>   *
> - * Return %false to end the rescue instead - the slice is already fully served,
> - * @p is leaving the rq or bypass is dismantling rescues.
> + * Return %false to end the rescue instead - the slice is already fully served
> + * or @p is leaving the rq.
>   */
>  bool scx_rescue_keep(struct rq *rq, struct task_struct *p)
>  {
> @@ -522,8 +524,7 @@ bool scx_rescue_keep(struct rq *rq, struct task_struct *p)
>  
>  	lockdep_assert_rq_held(rq);
>  
> -	if (!remaining || !(p->scx.flags & SCX_TASK_QUEUED) ||
> -	    scx_bypassing(scx_task_sched(p), cpu_of(rq)))
> +	if (!remaining || !(p->scx.flags & SCX_TASK_QUEUED))
>  		return false;
>  
>  	scx_set_task_slice(p, remaining);

[Severity: High]
Should the initial slice check in dispatch_one() also allow a task with
scx_task_has_kernel_slice(prev) to keep running while its scheduler is
bypassing, matching put_prev_task_scx()?

In kernel/sched/ext/ext.c, dispatch_one() still checks:

	if (prev->sched_class == &ext_sched_class) {
		update_curr_scx(rq);

		if ((prev->scx.flags & SCX_TASK_QUEUED) && prev->scx.slice &&
		    !scx_bypassing(scx_task_sched(prev), cpu)) {
			verdict = SCX_DSP_PREV;
			goto has_tasks;
		}
	}

When a sub-scheduler is bypassing on a CPU where its host lacks
SCX_CAP_BASE, bypass_enq_target_dsq() routes the task to rescue and
scx_rescue_admit() grants it SCX_TASK_KERNEL_SLICE (and scx_rescue_timerfn()
sets SCX_TASK_PROTECTED when escalating a starved rescuee). If a reschedule
occurs that does not zero prev->scx.slice (such as a non-preempting kick,
a denied SCX_KICK_PREEMPT in kick_one_cpu(), queue_sync_ecaps(), or a
deferred reenq_local IPI), declining SCX_DSP_PREV at the top of
dispatch_one() appears to cause a couple of regressions:

1. If rq->scx.local_dsq.nr is non-zero (for example, after
scx_rescue_timerfn() escalated prev with SCX_TASK_PROTECTED ahead of a
queued task on local_dsq, or after an ancestor enqueued a task whose
SCX_ENQ_PREEMPT was refused due to SCX_TASK_PROTECTED), dispatch_one()
immediately returns SCX_DSP_LOCAL:

	if (rq->scx.local_dsq.nr) {
		verdict = SCX_DSP_LOCAL;
		goto has_tasks;
	}

do_pick_task_scx() then picks the head of local_dsq instead of prev. In
put_prev_task_scx(), because p->scx.slice is still non-zero, the
scx_rescue_keep() block is skipped and prev is reinserted at the head of
local_dsq without rescheduling, evicting the SCX_TASK_PROTECTED rescuee.

2. If rq->scx.local_dsq.nr is zero, dispatch_one() calls
scx_dispatch_sched(root_sch, rq, prev, false) while prev->scx.slice is still
non-zero. In kernel/sched/ext/inlines.h, scx_dispatch_sched() checks
prev->scx.slice before checking rq->scx.local_dsq.nr:

		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 ops.dispatch() queues a task onto local_dsq without zeroing
prev->scx.slice, scx_dispatch_sched() still returns SCX_DSP_PREV. If that
newly dispatched task had SCX_ENQ_IMMED set, dsq_inc_nr() skipped
scx_schedule_reenq_local() because SCX_RQ_IN_DISPATCH was set and dsq->nr
was 1, while the has_tasks label in dispatch_one() only checks
rq->scx.local_dsq.nr > 1, leaving the single IMMED task stranded on
local_dsq while prev continues running. Any nested scx_bpf_sub_dispatch()
call will also return true because SCX_DSP_PREV != SCX_DSP_NONE, even if the
sub-scheduler dispatched no tasks.

Would updating the top of dispatch_one() to check
(!scx_bypassing(scx_task_sched(prev), cpu) || scx_task_has_kernel_slice(prev))
avoid falling through with a non-zero slice on a bypassing rescuee?

[ ... ]

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