Re: [PATCH 06/14] 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 rescue end in dispatch_one() via keep-last check
--

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

sched_ext: Fix a cpu idling with a runnable rescuee

This commit updates dispatch_one() to call scx_rescue_keep() when no other
task is runnable so that a rescuee with a zeroed slice continues running on
its unserved rescue remainder instead of letting the CPU go idle.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index e9224351e9dff..e587743b6136f 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -3389,6 +3389,26 @@ 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 declined @prev, and a
> +		 * reinsert from put_prev_task_scx() would come after the pick
> +		 * settled on idle. Keep @prev 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]
Should the scx_rescue_keep() check in kernel/sched/ext/ext.c:dispatch_one()
run before the SCX_EV_DISPATCH_KEEP_LAST check above it?

Right before this addition, dispatch_one() checks whether prev can stay on
the CPU:

	if (prev->scx.flags & SCX_TASK_QUEUED) {
		struct scx_sched *prev_sch = scx_task_sched(prev);

		if ((!(prev_sch->ops.flags & SCX_OPS_ENQ_LAST) ||
		     scx_bypassing(prev_sch, cpu)) && scx_task_can_stay_on_cpu(rq, prev)) {
			__scx_add_event(prev_sch, SCX_EV_DISPATCH_KEEP_LAST, 1);
			verdict = SCX_DSP_PREV;
			goto has_tasks;
		}

In kernel/sched/ext/sub.h:scx_task_can_stay_on_cpu(), this condition returns
true for an active rescuee if the task is in a migrate_disable() section, or
if prev_sch holds SCX_CAP_BASE on the CPU (for instance, when the task was
diverted to rescue because an enqueue required SCX_CAP_ENQ or SCX_CAP_PREEMPT,
or if SCX_CAP_BASE was granted after the rescue started):

	if (unlikely(is_migration_disabled(p)))
		return true;

	/* check caps against the sched running a bypassed task */
	return likely(!scx_missing_caps(scx_exec_sched(scx_task_sched(p), cpu), cpu,
					SCX_CAP_BASE));

When !(prev_sch->ops.flags & SCX_OPS_ENQ_LAST) and scx_task_can_stay_on_cpu()
is true, a rescuee whose slice was zeroed before serving its full rescue
quantum will take the SCX_EV_DISPATCH_KEEP_LAST branch and return
SCX_DSP_PREV with prev->scx.slice == 0 instead of calling scx_rescue_keep().

Then kernel/sched/ext/ext.c:do_pick_task_scx() sees !p->scx.slice:

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

Wouldn't that call scx_task_slice_ended() -> scx_rescue_end() to terminate
the rescue prematurely and refill p->scx.slice with an unmetered default
slice instead of restoring the unserved rescue remainder?

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

[ ... ]

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