Re: [PATCH v2 20/21] sched/deadline: Cancel the replenishment timer with the parameters

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] sched/deadline: stale deadline and runtime left by __dl_clear_params()
- [Medium] sched/deadline: uncancelled dl_timer in switched_from_dl() for sleepers
--

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

sched/deadline: Cancel the replenishment timer with the parameters

This commit cancels a deadline task's replenishment timer in
task_non_contending() and inactive_task_timer() when clearing its parameters
at the 0-lag time after leaving SCHED_DEADLINE. This prevents a task that
re-enters SCHED_DEADLINE before the old period expires from keeping the
armed replenishment timer of its previous reservation.

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index c0af06d0f3688..15d224846ebad 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -563,6 +563,7 @@ static void task_non_contending(struct sched_dl_entity *dl_se, bool dl_task)
>  				if (READ_ONCE(p->__state) == TASK_DEAD)
>  					sub_rq_bw(dl_se, &rq->dl);
>  				dl_task_release_bw(p);
> +				cancel_replenish_timer(dl_se);
>  				__dl_clear_params(dl_se);
>  			}
>  		}

[Severity: Medium]
When cancel_replenish_timer() and __dl_clear_params() run at the 0-lag time
in task_non_contending() or inactive_task_timer(), __dl_clear_params()
clears dl_throttled and dl_yielded along with the static reservation
parameters, but leaves the dynamic dl_se->deadline and dl_se->runtime from
the old reservation intact:

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;

	dl_se->dl_throttled		= 0;
	dl_se->dl_yielded		= 0;
	...
}

For the two cases described in the commit message where the 0-lag time is
reached before the old period ends, could leaving dl_se->deadline and
dl_se->runtime set cause problems if the task returns to SCHED_DEADLINE
before the old deadline or period expires?

In the first case, a task yields (or leaves with remaining runtime > 0), so
its 0-lag time is earlier than dl_se->deadline. After the 0-lag time passes
and releases the old bandwidth and clears dl_throttled and dl_yielded, if
the task returns to SCHED_DEADLINE before the old dl_se->deadline,
enqueue_dl_entity() checks whether the deadline has passed on the
ENQUEUE_MOVE path:

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

Because the stale dl_se->deadline is still in the future,
setup_new_dl_entity() is skipped and the task runs with its leftover
dl_se->runtime and old dl_se->deadline (regaining runtime it forfeited when
yielding, or exceeding a smaller new dl_runtime after its old bandwidth was
already released).

In the second case, a constrained-deadline task overruns slightly and
reaches its 0-lag time after dl_se->deadline, but before dl_next_period().
If it switches back to SCHED_DEADLINE as a constrained-deadline task before
the old period ends, enqueue_dl_entity() runs dl_check_constrained_dl()
before reaching the ENQUEUE_MOVE check:

kernel/sched/deadline.c:enqueue_dl_entity() {
	...
	if (!dl_se->dl_throttled && !dl_is_implicit(dl_se))
		dl_check_constrained_dl(dl_se);
	...
	if (!dl_se->dl_defer && dl_se->dl_throttled && !(flags & ENQUEUE_REPLENISH)) {
		if (flags & ENQUEUE_WAKEUP)
			task_contending(dl_se, flags);

		return;
	}
}

Seeing dl_se->deadline in the past and rq_clock(rq) before dl_next_period(),
dl_check_constrained_dl() sets dl_se->dl_throttled = 1 and re-arms dl_timer
for the old period, causing enqueue_dl_entity() to return early without
initializing the new reservation.

Should __dl_clear_params() also reset dl_se->deadline and dl_se->runtime?

> @@ -2326,6 +2327,7 @@ static enum hrtimer_restart inactive_task_timer(struct hrtimer *timer)
>  		}
>  
>  		dl_task_release_bw(p);
> +		cancel_replenish_timer(dl_se);
>  		__dl_clear_params(dl_se);
>  
>  		goto unlock;

[Severity: Medium]
What happens if a constrained-deadline task overruns slightly as it blocks,
reaches its 0-lag time while still sleeping in SCHED_DEADLINE, and only then
switches out of SCHED_DEADLINE before its replenishment timer fires?

When the task blocks in dequeue_task_dl(), update_curr_dl_se() sees the
runtime overrun, sets dl_se->dl_throttled = 1, and starts dl_se->dl_timer
for dl_next_period(). Then dequeue_dl_entity() calls
task_non_contending(&p->dl, true).

If the 0-lag time passes while the task is still asleep and still in
SCHED_DEADLINE, task_non_contending() or inactive_task_timer() drops
running_bw and clears dl_non_contending, while dl_throttled remains 1 and
dl_timer remains armed.

If the sleeping task is then switched to another scheduling class before
dl_timer fires, switched_from_dl() takes the !task_on_rq_queued(p) &&
!p->dl.dl_non_contending branch:

kernel/sched/deadline.c:switched_from_dl() {
	...
	if (!task_on_rq_queued(p)) {
		bool released = false;

		if (p->dl.dl_non_contending) {
			sub_running_bw(&p->dl, &rq->dl);
		} else if (p->dl.dl_runtime && !dl_entity_is_special(&p->dl)) {
			dl_task_release_bw(p);
			released = true;
		}
		sub_rq_bw(&p->dl, &rq->dl);
		if (released)
			p->dl.dl_bw = 0;
	}
}

Because neither task_non_contending() nor inactive_task_timer() runs on this
path, dl_task_release_bw(p) releases the bandwidth while leaving
p->dl.dl_timer armed and p->dl.dl_throttled set to 1. If the task returns to
SCHED_DEADLINE before the old dl_timer expires, enqueue_dl_entity() sees
dl_se->dl_throttled == 1 and returns early without enqueuing the task until
the old period ends.

Should switched_from_dl() also cancel the replenishment timer and clear the
throttled state when releasing a sleeping task's bandwidth past its 0-lag
time?

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