Re: [PATCH 08/18] sched_ext: Fix a cpu idling with a runnable rescuee

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] sched_ext: premature slice restore in dispatch_one() via scx_rescue_keep()
--

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

sched_ext: Fix a cpu idling with a runnable rescuee

This commit fixes a bug where a CPU idles with a runnable rescuee when a
scheduler zeroes the rescuee's slice without dispatching another task, by
calling scx_rescue_keep() in dispatch_one() to keep the rescuee running on
its unserved remainder when nothing else is runnable.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 56742d1ce5131..2504a6a9911b3 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -3396,6 +3396,34 @@ static enum scx_dsp_verdict dispatch_one(struct rq *rq, struct task_struct *prev
>  			verdict = SCX_DSP_PREV;
>  			goto has_tasks;
>  		}
> +
> +		/*
> +		 * The scheduler preempts a rescuee like any other task by
> +		 * zeroing its slice. The grant survives the preemption. The
> +		 * rescuee gets the unserved remainder back and runs again
> +		 * behind whatever the scheduler wanted to run first.
> +		 *
> +		 * Two sites restore the remainder. When another task won the
> +		 * pick, put_prev_task_scx() restores it and queues the rescuee
> +		 * at the tail of the local DSQ.
> +		 *
> +		 * When nothing else is runnable, the cpu must not go idle with
> +		 * the rescuee queued. The keep above takes a rescuee whose
> +		 * scheduler holds %SCX_CAP_BASE here and doesn't take the last
> +		 * task through %SCX_OPS_ENQ_LAST.
> +		 *
> +		 * The pick then ends its zeroed slice, and the rescue with it,
> +		 * and refills a default slice. The task continues as an
> +		 * ordinary one, which is fine as its scheduler holds BASE here.
> +		 *
> +		 * For any other rescuee, a reinsert from put_prev_task_scx()
> +		 * would come after the pick settled on idle. Keep it running on
> +		 * the remainder here.
> +		 */
> +		if (unlikely(prev == scx_rescuee(rq)) && scx_rescue_keep(rq, prev)) {
> +			verdict = SCX_DSP_PREV;
> +			goto has_tasks;
> +		}

[Severity: Medium]
Can calling scx_rescue_keep() inside dispatch_one() restore prev->scx.slice
prematurely when the pick is subsequently retried or aborted?

On the existing keep-last path right above this block, dispatch_one()
returns SCX_DSP_PREV while leaving prev->scx.slice at 0, and
do_pick_task_scx() only commits to ending and refilling the slice after
checking for retries and higher-priority class modifications:

kernel/sched/ext/ext.c:do_pick_task_scx() {
	...
	if (verdict == SCX_DSP_RETRY)
		return RETRY_TASK;
	...
	if (!force_scx && rq_modified_above(rq, &ext_sched_class)) {
		p = RETRY_TASK;
		goto out;
	}

	if (verdict == SCX_DSP_PREV) {
		p = prev;
		if (!p->scx.slice) {
			/* the slice is consumed, protection ends */
			scx_task_slice_ended(rq, p);
			refill_task_slice_dfl(scx_task_sched(p), p);
		}
	...
}

By contrast, calling scx_rescue_keep(rq, prev) in dispatch_one() writes
prev->scx.slice = remaining (> 0) before the pick is finalized. If
scx_dispatch_sched() temporarily dropped the rq lock before returning
SCX_DSP_NONE, two issues can occur:

1. Under core scheduling, dispatch_core_pick() sees that the rq lock was
   dropped and discards the verdict:

   kernel/sched/ext/ext.c:dispatch_core_pick() {
       ...
       verdict = dispatch_one(rq, prev);

       /* if dispatch_one() released the rq lock, restart the selection */
       if (rq->scx.lock_drop_seq != seq)
           return SCX_DSP_RETRY;
   }

   When the pick restarts and calls dispatch_one(rq, prev) again,
   prev->scx.slice is now non-zero, so dispatch_one() short-circuits at the
   top:

   kernel/sched/ext/ext.c:dispatch_one() {
       ...
       if ((prev->scx.flags & SCX_TASK_QUEUED) && prev->scx.slice &&
           !scx_bypassing(scx_task_sched(prev), cpu)) {
           verdict = SCX_DSP_PREV;
           goto has_tasks;
       }
   }

   This returns SCX_DSP_PREV before checking rq->scx.local_dsq.nr or calling
   scx_dispatch_sched(), ignoring any tasks enqueued while the rq lock was
   dropped.

2. If a higher-priority task (such as RT, DL, or fair) woke up while the rq
   lock was dropped, do_pick_task_scx() sees rq_modified_above() and returns
   RETRY_TASK (or core-sched cookie matching selects another task or
   rq->idle). When put_prev_task_scx() then runs for prev, p->scx.slice is
   already non-zero:

   kernel/sched/ext/ext.c:put_prev_task_scx() {
       ...
       if (!p->scx.slice) {
           if (unlikely(p == scx_rescuee(rq)))
               rescue_keep = scx_rescue_keep(rq, p);
           if (!rescue_keep)
               scx_task_slice_ended(rq, p);
       }
       ...
       if (p->scx.slice && !scx_bypassing(sch, cpu_of(rq))) {
           ...
           if (unlikely(p == scx_rescuee(rq))) {
               enq_flags |= SCX_ENQ_IGNORE_CAPS;
               if (!rescue_keep)
                   enq_flags |= SCX_ENQ_HEAD;
           }
   }

   Because !p->scx.slice is skipped, rescue_keep remains false and the
   preempted rescuee is queued with SCX_ENQ_HEAD instead of at the tail of
   the local DSQ.

Should the slice restoration via scx_rescue_keep() be deferred to the
verdict == SCX_DSP_PREV && !p->scx.slice block in do_pick_task_scx(), with
dispatch_one() only checking whether the rescuee can be kept (for example,
scx_rescue_slice_remaining(rq) > 0 && !scx_bypassing(prev_sch, cpu))?

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