Re: [PATCH 04/12] sched/deadline: Do not lose a throttled task switched out and back

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] sched/deadline: incomplete replenishment when clearing dl_throttled
- [Medium] sched/deadline: stale dl_yielded left set by dl_task_timer()
--

commit b274aa4dbc0a2822908373f43a14a8b8436d20eb
Author: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>

sched/deadline: Do not lose a throttled task switched out and back

This commit clears dl_se->dl_throttled in dl_task_timer() when a throttled
SCHED_DEADLINE task has been switched to another scheduling policy before
its replenishment timer fires, so that switching back to SCHED_DEADLINE
before the 0-lag time does not leave the task permanently throttled.

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 30a52e42c26d8..d474fbfd9be6d 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -1232,10 +1232,17 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>  
>  	/*
>  	 * The task might have changed its scheduling policy to something
> -	 * different than SCHED_DEADLINE (through switched_from_dl()).
> +	 * different than SCHED_DEADLINE (through switched_from_dl()). It
> +	 * keeps its parameters until its 0-lag time and may come back to
> +	 * SCHED_DEADLINE before then, switched_to_dl() cancelling the
> +	 * inactive timer: it must not come back throttled, since this was
> +	 * its replenishment. Its deadline has passed: the enqueue starts a
> +	 * new period.
>  	 */
> -	if (!dl_task(p))
> +	if (!dl_task(p)) {
> +		dl_se->dl_throttled = 0;
>  		goto unlock;
> +	}

[Severity: Medium]
Could clearing only dl_se->dl_throttled here without calling
replenish_dl_entity(dl_se) forgive negative runtime overruns and leave
dl_se->deadline stale when the task switches back to SCHED_DEADLINE before
its 0-lag time?

The window between the replenishment timer and the inactive timer exists
specifically when a task has overrun its runtime (dl_se->runtime < 0),
making the 0-lag time later than dl_next_period(dl_se). Normal replenishment
in replenish_dl_entity() deducts that overrun from subsequent periods:

kernel/sched/deadline.c:replenish_dl_entity() {
	...
	while (dl_se->runtime <= 0) {
		dl_se->deadline += pi_of(dl_se)->dl_period;
		dl_se->runtime += pi_of(dl_se)->dl_runtime;
	}
	...
}

By only clearing dl_se->dl_throttled and leaving dl_se->deadline in the past
and dl_se->runtime negative, a task returning to SCHED_DEADLINE before its
0-lag time enters enqueue_dl_entity(), where setup_new_dl_entity() (on
ENQUEUE_MOVE) or update_dl_entity() (on ENQUEUE_WAKEUP) calls
replenish_dl_new_period(dl_se, rq):

kernel/sched/deadline.c:enqueue_dl_entity() {
	...
	} else if ((flags & ENQUEUE_MOVE) &&
		   !is_dl_boosted(dl_se) &&
		   dl_time_before(dl_se->deadline, rq_clock(rq_of_dl_se(dl_se)))) {
		setup_new_dl_entity(dl_se);
	}
	...
}

replenish_dl_new_period() unconditionally resets dl_se->runtime to the full
pi_of(dl_se)->dl_runtime rather than deducting the prior negative runtime
overrun, allowing the task to bypass bandwidth enforcement by switching
policies across its replenishment point.

In addition, in enqueue_dl_entity(), dl_check_constrained_dl() runs before
the ENQUEUE_MOVE branch:

kernel/sched/deadline.c:enqueue_dl_entity() {
	...
	if (!dl_se->dl_throttled && !dl_is_implicit(dl_se))
		dl_check_constrained_dl(dl_se);
	...
}

If the task returns with constrained-deadline parameters (for example, with
a larger dl_period passed to sched_setattr() so that dl_se->deadline <
rq_clock(rq) < dl_next_period(dl_se)), dl_check_constrained_dl() sees the
un-advanced dl_se->deadline and re-throttles the task before
setup_new_dl_entity() is reached. And if the task migrated to another CPU
while in SCHED_OTHER whose rq_clock(rq) has not yet strictly passed
dl_se->deadline when it switches back, dl_time_before(dl_se->deadline,
rq_clock(rq)) evaluates to false and the task is enqueued onto dl_rq with
dl_se->runtime <= 0.

[Severity: Medium]
What happens if the task had also called sched_yield() in the period where
it was throttled, leaving dl_se->dl_yielded set to 1 when dl_task_timer()
fires?

In yield_task_dl(), rq->donor->dl.dl_yielded = 1 is set before calling
update_curr_dl(rq) -> update_curr_dl_se():

kernel/sched/deadline.c:yield_task_dl() {
	...
	rq->donor->dl.dl_yielded = 1;

	update_rq_clock(rq);
	update_curr_dl(rq);
	...
}

If the elapsed execution time in that slice also pushes dl_se->runtime < 0,
update_curr_dl_se() throttles the task with both dl_se->dl_throttled == 1
and dl_se->dl_yielded == 1, and arms the inactive timer for a 0-lag time
later than the deadline when the task switches out of SCHED_DEADLINE.

Unlike replenish_dl_entity() and __dl_clear_params(), which clear both
dl_se->dl_yielded and dl_se->dl_throttled:

kernel/sched/deadline.c:replenish_dl_entity() {
	...
	if (dl_se->dl_yielded)
		dl_se->dl_yielded = 0;
	if (dl_se->dl_throttled)
		dl_se->dl_throttled = 0;
	...
}

neither the !dl_task(p) path in dl_task_timer() nor replenish_dl_new_period()
clears dl_se->dl_yielded. When the task switches back to SCHED_DEADLINE
before its 0-lag time, wouldn't it be enqueued with a stale
dl_se->dl_yielded == 1 and immediately get throttled again for an entire
new period on the next call to update_curr_dl_se()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793804635.git.mathieu.desnoyers@xxxxxxxxxxxx?part=4