Re: [PATCH v2 07/21] sched/deadline: Do not release a departing sleeper's bandwidth twice

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sched/deadline: switched_from_dl() leaves stale params after clearing dl_bw
--

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

sched/deadline: Do not release a departing sleeper's bandwidth twice

This commit updates switched_from_dl() to only release total_bw when a
departing sleeping task has its own deadline parameters and is not a
special entity, and clears p->dl.dl_bw afterward so that subsequent paths
do not subtract its bandwidth a second time.

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index b646063a7b0d5..624cabdec9652 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -3573,6 +3573,8 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
>  	dec_dl_tasks_cs(p);
>  
>  	if (!task_on_rq_queued(p)) {
> +		bool released = false;
> +
>  		if (p->dl.dl_non_contending) {
>  			/*
>  			 * Inactive timer is armed. However, p is leaving
> @@ -3585,12 +3587,18 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
>  			 * release total_bw via __dl_sub().
>  			 */
>  			sub_running_bw(&p->dl, &rq->dl);
> -		} else {
> +		} else if (p->dl.dl_runtime && !dl_entity_is_special(&p->dl)) {
>  			/*
>  			 * No pending inactive timer: the task never ran as
>  			 * DEADLINE or the timer already fired while still
>  			 * DEADLINE (only dropping running_bw at that time).
>  			 * Nobody else will release total_bw, do it now.
> +			 *
> +			 * Not for a task without parameters of its own, one
> +			 * deboosted from priority inheritance, whose mask may
> +			 * have no active CPU for dl_task_root_domain_cpu(),
> +			 * nor for a special one: neither has bandwidth in
> +			 * total_bw.
>  			 */
>  			unsigned int cpu = dl_task_root_domain_cpu(p);
>  			struct dl_bw *dl_b = &cpu_rq(cpu)->rd->dl_bw;
> @@ -3598,8 +3606,29 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
>  			raw_spin_lock(&dl_b->lock);
>  			__dl_sub(dl_b, p->dl.dl_bw, dl_bw_cpus(cpu));
>  			raw_spin_unlock(&dl_b->lock);
> +			released = true;
>  		}
>  		sub_rq_bw(&p->dl, &rq->dl);
> +		/*
> +		 * Released: its bandwidth is no one's to release again, and
> +		 * a dl_bw of 0 is what every path that would takes. Kept, it
> +		 * would make the task one with bandwidth to task_contending()
> +		 * and task_non_contending() once priority inheritance boosts
> +		 * it, and a deboost while it sleeps would release it a second
> +		 * time. So would the inactive timer's handler when it runs
> +		 * late, dl_non_contending cleared under it by
> +		 * migrate_task_rq_dl(): it finds !dl_task(p).
> +		 *
> +		 * The parameters themselves stay, where
> +		 * inactive_task_timer() resets them with the bandwidth: the
> +		 * task may still be the donor of a priority inheritance
> +		 * boost until rt_mutex_adjust_pi() walks the chain, outside
> +		 * this runqueue lock, and replenish_dl_entity() of the
> +		 * boosted owner would never leave its loop over a dl_period
> +		 * and a dl_runtime of 0.
> +		 */
> +		if (released)
> +			p->dl.dl_bw = 0;

[Severity: High]
When switched_from_dl() takes the !p->dl.dl_non_contending branch, there is
no pending inactive timer to run inactive_task_timer() and call
__dl_clear_params(&p->dl) later. Does clearing only p->dl.dl_bw here leave
the departed task permanently carrying stale deadline parameters
(dl_runtime, dl_deadline, dl_period, dl_density, flags, runtime, and
deadline) after it switches to another policy such as SCHED_OTHER?

Several paths in kernel/sched/deadline.c check dl_deadline, dl_runtime, or
flags rather than dl_bw to distinguish a PI-boosted non-deadline task from
a task with its own deadline reservation:

1. In replenish_dl_entity(), initial replenishment for a PI-boosted task
checks dl_se->dl_deadline == 0:

kernel/sched/deadline.c:replenish_dl_entity() {
	if (dl_se->dl_deadline == 0 ||
	    (dl_se->dl_defer_armed && dl_entity_overflow(dl_se, rq_clock(rq)))) {
		dl_se->deadline = rq_clock(rq) + pi_of(dl_se)->dl_deadline;
		dl_se->runtime = pi_of(dl_se)->dl_runtime;
	}
}

If the departed task is later PI-boosted by a SCHED_DEADLINE waiter while
holding an rt_mutex, dl_se->dl_deadline is still non-zero. If its stale
p->dl.runtime is still positive and p->dl.deadline is still in the future,
replenish_dl_entity() skips both the while (dl_se->runtime <= 0) loop and
the dl_time_before(dl_se->deadline, rq_clock(rq)) check, so the boosted
task runs with its own stale runtime and deadline instead of inheriting the
donor's parameters. Conversely, if the task never ran as SCHED_DEADLINE
(p->dl.deadline == 0, p->dl.runtime == 0) or its stale deadline is in the
past, skipping the dl_se->dl_deadline == 0 initialization causes the loop
to advance from the stale deadline and trigger a false replenishment lag
warning.

2. If the task previously had SCHED_FLAG_RECLAIM set in p->dl.flags,
retaining p->dl.flags while p->dl.dl_bw == 0 causes dl_scaled_delta_exec()
to call grub_reclaim() when the task is later PI-boosted:

kernel/sched/deadline.c:grub_reclaim() {
	if (u_inact + rq->dl.extra_bw > rq->dl.max_bw - dl_se->dl_bw)
		u_act = dl_se->dl_bw;
	else
		u_act = rq->dl.max_bw - u_inact - rq->dl.extra_bw;
}

Whenever u_inact + rq->dl.extra_bw >= rq->dl.max_bw, u_act becomes
dl_se->dl_bw (0), scaling execution time to 0 so the boosted task's runtime
never depletes. Stale SCHED_FLAG_DL_OVERRUN in p->dl.flags can likewise
deliver SIGXCPU to the boosted non-deadline task.

3. Both task_non_contending() and task_contending() guard against
non-deadline boosted tasks by checking dl_se->dl_runtime == 0:

kernel/sched/deadline.c:task_non_contending() {
	if (dl_se->dl_runtime == 0)
		return;
}

Because p->dl.dl_runtime remains non-zero, blocking while PI-boosted (or
deboosting while runnable) runs task_non_contending() using the stale
parameters and sets p->dl.dl_non_contending = 1 with inactive_timer armed
Even though p->dl.dl_bw is 0 at that moment, if the task then re-enters
SCHED_DEADLINE via sched_setattr() while sleeping and PI-boosted
(prev_class == next_class == &dl_sched_class), __setparam_dl() assigns a
non-zero p->dl.dl_bw while dl_non_contending is still 1 without having
added the new bandwidth to rq->dl.running_bw, causing inactive_task_timer()
or migrate_task_rq_dl() to underflow rq->dl.running_bw.

4. If the departed task wakes up in SCHED_OTHER and later switches back to
SCHED_DEADLINE while runnable before its old p->dl.deadline passes,
enqueue_dl_entity() checks dl_time_before(dl_se->deadline, rq_clock(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);
	}
}

Because the stale p->dl.deadline is still in the future,
setup_new_dl_entity() is skipped and the task reuses its old unspent
runtime.

Should switched_from_dl() clear the remaining deadline parameters (or reset
them once any PI donor relationship is adjusted) instead of leaving them
populated when no inactive timer is pending?

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