Re: [PATCH 05/15] sched_ext: Restore a rescuee's slice after ops.stopping()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sched_ext: put_prev_task_scx() enqueues protected rescuee with zero slice
--

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

sched_ext: Restore a rescuee's slice after ops.stopping()

This commit moves the scx_rescue_keep() and scx_task_slice_ended() block in
put_prev_task_scx() after ops.stopping() and removes the scx_rescuee(rq)
special case from the local DSQ reinsertion condition.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 4eff83b9a841b..94a49e1ae95b2 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -3558,17 +3558,6 @@ static void put_prev_task_scx(struct rq *rq, struct task_struct *p,
>  
>  	update_curr_scx(rq);
>  
> -	/*
> -	 * If the slice is consumed, protection ends with it. A rescuee
> -	 * preempted beforehand keeps going, see scx_rescue_keep().
> -	 */
> -	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);
> -	}
> -
>  	/*
>  	 * Preserve the running session when proxy execution refreshes the same
>  	 * donor around an execution-context switch on this rq.
> @@ -3581,6 +3570,18 @@ static void put_prev_task_scx(struct rq *rq, struct task_struct *p,
>  		p->scx.flags &= ~SCX_TASK_RUN_TRACKED;
>  	}
>  
> +	/*
> +	 * If the slice is consumed, protection ends with it. A rescuee keeps
> +	 * going when a preemption or ops.stopping() zeroed its slice, see
> +	 * scx_rescue_keep().
> +	 */
> +	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.flags & SCX_TASK_QUEUED) {
>  		if (proxy_block)
>  			goto switch_class;
> @@ -3605,12 +3606,8 @@ 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.
> -		 *
> -		 * An open rescue must keep @p on the local DSQ even if the
> -		 * scheduler zeroed the slice in ops.stopping() above.
>  		 */
> -		if ((p->scx.slice || unlikely(p == scx_rescuee(rq))) &&
> -		    !scx_bypassing(sch, cpu_of(rq))) {
> +		if (p->scx.slice && !scx_bypassing(sch, cpu_of(rq))) {

[Severity: High]
Can removing the p == scx_rescuee(rq) check here allow a rescuee with
SCX_TASK_PROTECTED set to reach this branch with p->scx.slice == 0 and fall
through to scx_do_enqueue_task() while still set as rq->scx.rescue.curr?

When a rescuee p is admitted in scx_rescue_admit(), it snapshots
rq->scx.rescue.exec_snap = p->se.sum_exec_runtime, and scx_rescue_timerfn()
can escalate the rescue by setting SCX_TASK_PROTECTED on p.

If p is a mutex-blocked proxy donor (p->is_blocked == 1, rq->donor == p) and
a mutex owner runs on its behalf (rq->curr != rq->donor), update_curr_scx()
calls update_curr_common(rq) -> update_se(rq, &rq->donor->se) in
kernel/sched/fair.c, which credits delta_exec to rq->curr->se.sum_exec_runtime
rather than p->se.sum_exec_runtime:

kernel/sched/fair.c:update_se() {
	...
	if (entity_is_task(se)) {
		struct task_struct *running = rq->curr;
		/*
		 * If se is a task, we account the time against the running
		 * task, as w/ proxy-exec they may not be the same.
		 */
		running->se.exec_start = now;
		running->se.sum_exec_runtime += delta_exec;
	...
}

Meanwhile, update_curr_scx() decrements donor->scx.slice down to 0, while
scx_rescue_slice_remaining(rq) in kernel/sched/ext/sub.c still sees positive
remaining runtime because rq->scx.rescue.curr->se.sum_exec_runtime did not
advance:

kernel/sched/ext/sub.c:scx_rescue_slice_remaining() {
	s64 served = rq->scx.rescue.curr->se.sum_exec_runtime - rq->scx.rescue.exec_snap;

	return max(rq->scx.rescue.slice - served, 0);
}

When put_prev_task_scx() runs for p with p->scx.slice == 0, it calls
scx_rescue_keep(rq, p) in kernel/sched/ext/sub.c:

kernel/sched/ext/sub.c:scx_rescue_keep() {
	s64 remaining = scx_rescue_slice_remaining(rq);
	...
	if (!remaining || !(p->scx.flags & SCX_TASK_QUEUED) ||
	    scx_bypassing(scx_task_sched(p), cpu_of(rq)))
		return false;

	scx_set_task_slice(p, remaining);
	return true;
}

However, scx_set_task_slice(p, remaining) calls set_task_slice_keep_oob() in
kernel/sched/ext/ext.c, which refuses the slice update because
SCX_TASK_PROTECTED is still set on p:

kernel/sched/ext/ext.c:set_task_slice_keep_oob() {
	lockdep_assert_rq_held(task_rq(p));

	if (unlikely(p->scx.flags & SCX_TASK_PROTECTED))
		return false;

	p->scx.slice = slice;
	return true;
}

Because scx_rescue_keep() ignores that false return value and returns true,
put_prev_task_scx() skips scx_task_slice_ended(rq, p), leaving
rq->scx.rescue.curr == p with p->scx.slice == 0.

Without unlikely(p == scx_rescuee(rq)) in the local DSQ reinsertion check in
put_prev_task_scx(), p now skips local DSQ reinsertion and falls through to
scx_do_enqueue_task(), which can enqueue p onto another DSQ (such as
rq->scx.rescue.dsq, rq->scx.reject_dsq, or a global/user DSQ) while
rq->scx.rescue.curr is still p. If scx_rescue_timerfn() in
kernel/sched/ext/sub.c subsequently fires, it assumes p->scx.dsq is
&rq->scx.local_dsq and unlinks p from the wrong DSQ:

kernel/sched/ext/sub.c:scx_rescue_timerfn() {
	...
	} else if (p->scx.dsq && rq->scx.rescue.budget > 2 * scx_rescue_quantum_ns) {
		scx_set_task_slice(p, scx_rescue_slice_remaining(rq));
		p->scx.flags |= SCX_TASK_PROTECTED;
		scx_task_unlink_from_dsq(p, &rq->scx.local_dsq);
	...
}

Should scx_rescue_keep() either bypass SCX_TASK_PROTECTED when restoring the
rescue slice or propagate the failure from scx_set_task_slice()?

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