Re: [PATCH v2 06/21] sched/deadline: Do not lose a throttled task switched out and back

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sched/deadline: replenish_dl_entity() in dl_task_timer() skips 0-lag reset
--

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

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

This commit calls replenish_dl_entity() from dl_task_timer() when a throttled
task has switched out of SCHED_DEADLINE before the replenishment timer fires,
so that if the task switches back before its 0-lag time, it does not remain
throttled with no pending timer.

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 30a52e42c26d8..b646063a7b0d5 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -1232,10 +1232,23 @@ 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: give it the replenishment this timer owes it, as
> +	 * the !queued case below does, and leave the enqueue to its return.
> +	 * A throttle left behind would never be lifted; an overrun left in
> +	 * its runtime would be forgiven by the new period its enqueue
> +	 * starts; and a yield left set would throttle it again at once.
>  	 */
> -	if (!dl_task(p))
> +	if (!dl_task(p)) {
> +		if (dl_se->dl_throttled && !is_dl_boosted(dl_se)) {
> +			sched_clock_tick();
> +			update_rq_clock(rq);
> +			replenish_dl_entity(dl_se);
> +		}
>  		goto unlock;
> +	}

[Severity: High]
What happens if the task stays out of SCHED_DEADLINE past its 0-lag time, and
then switches back to SCHED_DEADLINE while runnable before the replenished
dl_se->deadline expires?

When replenish_dl_entity() runs here in dl_task_timer() at the end of the
period, it advances dl_se->deadline by one or more periods (past the 0-lag
time) and sets a positive dl_se->runtime:

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

If the task does not return before the 0-lag time, inactive_task_timer()
fires, subtracts the old bandwidth from dl_b, and calls __dl_clear_params():

kernel/sched/deadline.c:inactive_task_timer() {
	...
	raw_spin_lock(&dl_b->lock);
	__dl_sub(dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
	raw_spin_unlock(&dl_b->lock);
	__dl_clear_params(dl_se);
	...
}

However, __dl_clear_params() only zeroes the static reservation parameters and
flags; it leaves the dynamic dl_se->deadline and dl_se->runtime untouched:

kernel/sched/deadline.c:__dl_clear_params() {
	dl_se->dl_runtime		= 0;
	dl_se->dl_deadline		= 0;
	dl_se->dl_period		= 0;
	dl_se->flags			= 0;
	dl_se->dl_bw			= 0;
	dl_se->dl_density		= 0;
	...
}

If the runnable task then switches back to SCHED_DEADLINE via sched_setattr()
after the 0-lag time has passed, but before the replenished dl_se->deadline,
__setparam_dl() writes the new static parameters and sched_change_end() calls
enqueue_task_dl() -> enqueue_dl_entity() with ENQUEUE_MOVE (and without
ENQUEUE_WAKEUP):

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

Previously, dl_se->deadline stayed at the old deadline (which is earlier than
the 0-lag time), so dl_time_before(dl_se->deadline, rq_clock(rq)) was always
true after the 0-lag time and setup_new_dl_entity() initialized a fresh period
from the new static parameters.

With dl_se->deadline pushed past the 0-lag time by replenish_dl_entity(),
won't dl_time_before(dl_se->deadline, rq_clock(rq)) evaluate to false during
the window between the 0-lag time and the replenished dl_se->deadline,
skipping setup_new_dl_entity()?

If setup_new_dl_entity() is skipped after the 0-lag time has already freed the
old bandwidth and reset the reservation, the task will run with stale dynamic
dl_se->runtime and dl_se->deadline from its previous reservation. For example,
a task could re-enter SCHED_DEADLINE with a tiny new runtime reservation after
its old bandwidth was released at 0-lag, yet still consume the large
dl_se->runtime replenished from its previous reservation, or conversely remain
penalized by a previous overrun after its 0-lag time has already elapsed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104211207.1392343-1-mathieu.desnoyers@xxxxxxxxxxxx?part=6