Re: [PATCH 09/12] sched/deadline: Do not take a waking task's bandwidth off a runqueue it left

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] sched/deadline: double add_rq_bw() in task_contending() on wake_list switch
- [Medium] sched/deadline: migrate_task_rq_dl() leaves dl_bw_offrq set for dl_runtime == 0
--

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

sched/deadline: Do not take a waking task's bandwidth off a runqueue it left

This commit adds a dl_bw_offrq bit to struct sched_dl_entity to track when a
waking deadline task has had its bandwidth removed from its previous runqueue
by migrate_task_rq_dl() but has not yet been enqueued on its target CPU. While
dl_bw_offrq is set, dl_change_utilization() and switched_from_dl() avoid
subtracting the task's bandwidth from the target CPU's runqueue before the
wakeup enqueue charges it via add_rq_bw().

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 8ead9b050dfcf..a8906cf966401 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c

[ ... ]

> @@ -2785,6 +2794,12 @@ static void migrate_task_rq_dl(struct task_struct *p, int new_cpu __maybe_unused
>  		cancel_inactive_timer(&p->dl);
>  	}
>  	sub_rq_bw(&p->dl, &rq->dl);
> +	/*
> +	 * Until the enqueue on the CPU it wakes on charges it there, which
> +	 * add_rq_bw() tells of, this bandwidth is on no runqueue: the rq of
> +	 * task_cpu() is not the one it was taken from.
> +	 */
> +	p->dl.dl_bw_offrq = 1;
>  	rq_unlock(rq, &rf);
>  }

[Severity: Medium]
What happens if a non-deadline task boosted via priority inheritance (with
p->dl.dl_runtime == 0) blocks and migrates on wakeup through
try_to_wake_up() -> set_task_cpu() -> migrate_task_rq_dl()?

Here migrate_task_rq_dl() unconditionally sets p->dl.dl_bw_offrq = 1, but
when the wakeup enqueues the task via enqueue_task_dl() ->
enqueue_dl_entity() -> task_contending(), task_contending() returns early
before calling add_rq_bw():

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

	if (flags & ENQUEUE_MIGRATED)
		add_rq_bw(dl_se, dl_rq);

Because switched_from_dl() only clears dl_bw_offrq when !task_on_rq_queued(p),
won't p->dl.dl_bw_offrq remain stuck at 1 after the wakeup completes?

If the task later blocks while still PI-boosted in dl_sched_class and
sched_setattr() assigns it SCHED_DEADLINE parameters, __sched_setscheduler()
sees prev_class == next_class == &dl_sched_class and does not call
switched_to_dl(). Wouldn't the stale p->dl.dl_bw_offrq == 1 then cause
dl_change_utilization() or a subsequent switched_from_dl() while blocked to
skip updating rq->dl.this_bw?

> @@ -3736,7 +3751,9 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
>  			dl_task_release_bw(p, dl_task_root_domain_cpu(p));
>  			released = true;
>  		}
> -		sub_rq_bw(&p->dl, &rq->dl);
> +		if (!p->dl.dl_bw_offrq)
> +			sub_rq_bw(&p->dl, &rq->dl);
> +		p->dl.dl_bw_offrq = 0;

[Severity: High]
Can this leak p->dl.dl_bw into the target CPU's rq->dl.this_bw when a task
queued on the target CPU's wake_list switches out of SCHED_DEADLINE and back
into SCHED_DEADLINE before sched_ttwu_pending() runs?

When try_to_wake_up() migrates a waking deadline task and queues it via
ttwu_queue_wakelist(), p->sched_remote_wakeup is set to 1 while
!task_on_rq_queued(p) and migrate_task_rq_dl() sets p->dl.dl_bw_offrq = 1.

If the task then leaves SCHED_DEADLINE before sched_ttwu_pending() runs,
switched_from_dl() sees p->dl.dl_bw_offrq == 1, skips sub_rq_bw(), and clears
p->dl.dl_bw_offrq = 0. If the task is then switched back to SCHED_DEADLINE
(or if a non-deadline task on the wake_list with p->sched_remote_wakeup == 1
switches to SCHED_DEADLINE) while still waiting on the wake_list,
switched_to_dl() charges the target CPU's rq->dl.this_bw immediately:

kernel/sched/deadline.c:switched_to_dl() {
	...
	/* If p is not queued we will update its parameters at next wakeup. */
	if (!task_on_rq_queued(p)) {
		add_rq_bw(&p->dl, &rq->dl);

		return;
	}

When sched_ttwu_pending() subsequently runs on the target CPU, it sees
p->sched_remote_wakeup == 1 and calls ttwu_do_activate() with WF_MIGRATED,
which passes ENQUEUE_MIGRATED into enqueue_task_dl() -> enqueue_dl_entity() ->
task_contending():

kernel/sched/deadline.c:task_contending() {
	...
	if (flags & ENQUEUE_MIGRATED)
		add_rq_bw(dl_se, dl_rq);

Since task_contending() checks (flags & ENQUEUE_MIGRATED) rather than
dl_se->dl_bw_offrq, won't it call add_rq_bw() a second time on the target
CPU's dl_rq even though switched_to_dl() already added p->dl.dl_bw and cleared
dl_bw_offrq?

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