Re: [PATCH 05/12] sched/deadline: Do not release a departing sleeper's bandwidth twice

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sched/deadline: infinite loop in replenish_dl_entity() on PI donor clear
--

commit 0ea45c3b24aa9d9591791893080fc323c30a9de8
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

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

This commit prevents switched_from_dl() from releasing root domain bandwidth
for tasks without their own deadline parameters or special sugov entities, and
clears the deadline parameters after releasing a departing sleeper's bandwidth
so it cannot be released a second time.

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index d474fbfd9be6d..3c57e5f5fc056 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -3567,6 +3567,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
> @@ -3579,12 +3581,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;
> @@ -3592,8 +3600,20 @@ 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: reset the parameters, as inactive_task_timer()
> +		 * does. Kept, they 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 that bandwidth again. 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).
> +		 */
> +		if (released)
> +			__dl_clear_params(&p->dl);

[Severity: High]
Can calling __dl_clear_params(&p->dl) in switched_from_dl() cause an infinite
loop in replenish_dl_entity() when p is blocked on an rt_mutex and acting as
the priority inheritance donor for the lock owner?

If a non-deadline owner holds an rt_mutex and a SCHED_DEADLINE task p blocks
on it as the top waiter, task_blocks_on_rt_mutex() -> rt_mutex_setprio() sets
owner->dl.pi_se = &p->dl and places the owner into dl_sched_class.

If p's 0-lag time has already elapsed while p sleeps on the lock (so
p->dl.dl_non_contending == 0) and p is switched to another policy via
__sched_setscheduler() in kernel/sched/syscalls.c, switched_from_dl() runs
inside the sched_change guard before rt_mutex_adjust_pi(p) is called:

    scoped_guard (sched_change, p, queue_flags) {
        /* sched_change_begin() -> switched_from_dl() */
        ...
    }

    preempt_disable();
    head = splice_balance_callbacks(rq);
    task_rq_unlock(rq, p, &rf);

    if (pi) {
        if (cpuset_locked)
            cpuset_unlock();
        rt_mutex_adjust_pi(p);
    }

At that point __dl_clear_params(&p->dl) zeroes p->dl.dl_runtime,
p->dl.dl_deadline, and p->dl.dl_period while the lock owner still has
owner->dl.pi_se pointing to &p->dl.

Even once rt_mutex_adjust_pi(p) calls rt_mutex_setprio() in
kernel/sched/core.c to deboost the owner, sched_change_begin() dequeues and
updates the owner before resetting owner->dl.pi_se:

    scoped_guard (sched_change, p, queue_flag) {
        /* sched_change_begin() -> dequeue_task_dl() -> update_curr_dl() */
        ...
        } else {
            if (dl_prio(oldprio))
                p->dl.pi_se = &p->dl;

If the boosted owner exhausts its runtime (or yields) either concurrently on
another CPU before rt_mutex_adjust_pi(p) runs, or during the dequeue in
rt_mutex_setprio(), update_curr_dl_se() sees is_dl_boosted(&owner->dl) and
calls:

    enqueue_task_dl(rq, owner, ENQUEUE_REPLENISH)
      -> enqueue_dl_entity()
        -> replenish_dl_entity(&owner->dl)

In kernel/sched/deadline.c:replenish_dl_entity(), pi_of(dl_se) still resolves
to &p->dl where dl_runtime and dl_period are now 0:

    WARN_ON_ONCE(pi_of(dl_se)->dl_runtime <= 0);
    ...
    while (dl_se->runtime <= 0) {
        dl_se->deadline += pi_of(dl_se)->dl_period;
        dl_se->runtime += pi_of(dl_se)->dl_runtime;
    }

Wouldn't this trigger the warning and spin forever with the runqueue lock
held and interrupts disabled?

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