Re: [PATCH v5 11/29] sched/deadline: Release the bandwidth of a task that leaves the class asleep

[Juri Lelli <juri.lelli@redhat.com>]

Hello,

On 08/11/26 13:53, Mathieu Desnoyers wrote:
> A task that leaves SCHED_DEADLINE while it is not queued can keep its
> bandwidth in total_bw until the next rebuild of the root domains.
> Repeated, this fills total_bw up to the admission limit. Then
> sched_setattr(SCHED_DEADLINE) and CPU offlining fail with EBUSY.
> 
> A task that leaves the class keeps its bandwidth until its 0-lag time.
> Releasing it earlier would allow temporary overloads. For a queued task,
> switched_from_dl() calls task_non_contending(). It releases the
> bandwidth if the 0-lag time is past. Otherwise it arms the inactive
> timer, which releases it then. For a task that is not queued,
> switched_from_dl() relies on the timer armed when the task blocked.
> 
> But that timer is not always armed. The task may have blocked after its
> 0-lag time. Or the timer fired while the task was still in the class,
> and only removed running_bw. Then nothing releases total_bw. The timer
> is also cancelled before the 0-lag time when the task wakes up on
> another CPU, or comes back to the class asleep.
> 
> Fix this in switched_from_dl(), for a task that is not queued and has no
> timer pending. If the 0-lag time is ahead, arm the timer. If it is past,
> release the bandwidth now. Then clear the parameters, as
> inactive_task_timer() does, so that no later path releases the bandwidth
> again. A timer handler that already started finds a dl_bw of 0. The
> deboost of a later priority inheritance boost finds no parameters. Since
> "sched/deadline: Take a deadline boost from the waiter's rt_mutex node",
> a boost that the task gives as a waiter does not read them.
> 
> Juri Lelli's first fix, since dropped from tip, released the bandwidth
> here without clearing dl_bw. The bandwidth of a boosted task was then
> released twice.
> 
> The Sashiko AI review of the third posting found the two cases with the
> 0-lag time ahead. deadline_bw_test, added later in this series, tests
> this in pi_former_owner_asleep and deboost_released_resets_period.
> 
> Link: https://lore.kernel.org/all/20261103-b4-sched-dl-bw-fixes-v1-2-dd761fe005b4@redhat.com/
> Fixes: 209a0cbda7a0 ("sched/deadline: Improve the tracking of active utilization")
> Reported-by: Tejun Heo <tj@kernel.org>
> Closes: https://lore.kernel.org/all/2b5fb3f22bca6aa383be8f4781923392@kernel.org/
> Cc: stable@vger.kernel.org
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
>  kernel/sched/deadline.c | 98 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 81 insertions(+), 17 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index e6c55afcfd01..d5d0f87d3a94 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -403,6 +403,23 @@ static void dl_change_utilization(struct task_struct *p, u64 new_bw)
>  
>  static void __dl_clear_params(struct sched_dl_entity *dl_se);
>  
> +/*
> + * Return the 0-lag time of @dl_se, relative to the clock of its runqueue.
> + * After @dl_se blocks or leaves the class, its utilization counts until
> + * that time.
> + */
> +static s64 dl_zero_lag(struct sched_dl_entity *dl_se)
> +{
> +	struct rq *rq = rq_of_dl_se(dl_se);
> +
> +	/*
> +	 * Using relative times instead of the absolute "0-lag time"
> +	 * allows to simplify the code
> +	 */
> +	return dl_se->deadline - rq_clock(rq) -
> +	       div64_long((dl_se->runtime * dl_se->dl_period), dl_se->dl_runtime);
> +}
> +
>  /*
>   * The utilization of a task cannot be immediately removed from
>   * the rq active utilization (running_bw) when the task blocks.
> @@ -476,15 +493,7 @@ static void task_non_contending(struct sched_dl_entity *dl_se, bool dl_task)
>  
>  	WARN_ON(dl_se->dl_non_contending);
>  
> -	zerolag_time = dl_se->deadline -
> -		 div64_long((dl_se->runtime * dl_se->dl_period),
> -			dl_se->dl_runtime);
> -
> -	/*
> -	 * Using relative times instead of the absolute "0-lag time"
> -	 * allows to simplify the code
> -	 */
> -	zerolag_time -= rq_clock(rq);
> +	zerolag_time = dl_zero_lag(dl_se);
>  
>  	/*
>  	 * If the "0-lag time" already passed, decrease the active
> @@ -3606,15 +3615,69 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
>  	dec_dl_tasks_cs(p);
>  
>  	if (!task_on_rq_queued(p)) {
> -		/*
> -		 * Inactive timer is armed. However, p is leaving DEADLINE and
> -		 * might migrate away from this rq while continuing to run on
> -		 * some other class. We need to remove its contribution from
> -		 * this rq running_bw now, or sub_rq_bw (below) will complain.
> -		 */
> -		if (p->dl.dl_non_contending)
> +		bool released = false;
> +
> +		if (p->dl.dl_non_contending) {
> +			/*
> +			 * Inactive timer is armed. However, p is leaving
> +			 * DEADLINE and might migrate away from this rq while
> +			 * continuing to run on some other class. We need to
> +			 * remove its contribution from this rq running_bw now,
> +			 * or sub_rq_bw (below) will complain.
> +			 *
> +			 * inactive_task_timer() will find !dl_task(p) and
> +			 * release total_bw via __dl_sub().
> +			 */
>  			sub_running_bw(&p->dl, &rq->dl);
> +		} else if (p->dl.dl_runtime && !dl_entity_is_special(&p->dl) &&
> +			   dl_zero_lag(&p->dl) >= 0 &&
> +			   !hrtimer_active(&p->dl.inactive_timer)) {
> +			/*
> +			 * No inactive timer is armed, but the 0-lag time is
> +			 * ahead. Either the task is waking up on another CPU,
> +			 * and migrate_task_rq_dl() cancelled the timer. Or it
> +			 * came back to the class asleep, and switched_to_dl()
> +			 * cancelled it. Arm the timer, as for a queued task.
> +			 * Its handler finds the task out of the class and
> +			 * releases total_bw. running_bw is already off this
> +			 * runqueue, or was never on it.
> +			 */
> +			task_non_contending(&p->dl, false);
> +		} else if (p->dl.dl_runtime && !dl_entity_is_special(&p->dl)) {
> +			/*
> +			 * No inactive timer is armed, and the 0-lag time is
> +			 * past. Either the task never ran in the class, or it
> +			 * blocked after its 0-lag time. Or the timer fired
> +			 * while the task was still in the class. Nothing else
> +			 * releases total_bw, so release it now.
> +			 *
> +			 * A timer whose handler already runs also leads here,
> +			 * since hrtimer_active() is true. The handler waits
> +			 * for the runqueue lock, then finds a dl_bw of 0,
> +			 * cleared below.
> +			 *
> +			 * A task deboosted from priority inheritance, with no
> +			 * parameters of its own, has no bandwidth in total_bw.
> +			 * Neither has a special task.
> +			 */
> +			struct dl_bw *dl_b = dl_bw_of(task_cpu(p));
> +
> +			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);
> +			released = true;
> +		}
>  		sub_rq_bw(&p->dl, &rq->dl);
> +		/*
> +		 * Clear the parameters too, as inactive_task_timer() does at
> +		 * the 0-lag time. Then no later path releases the bandwidth
> +		 * again. A later priority inheritance boost sees a task with no
> +		 * parameters of its own. task_contending() and
> +		 * task_non_contending() skip it, and its deboost releases
> +		 * nothing.
> +		 */
> +		if (released)
> +			__dl_clear_params(&p->dl);

The fix looks right, but I wondered if we can condense a bit the above
if-else tree. It's essantially doing what task_non_contending() does
already (special and non-DL tasks checks included)?

What about something like

     if (!task_on_rq_queued(p)) {
         if (p->dl.dl_non_contending)
           sub_running_bw(&p->dl, &rq->dl);
         sub_rq_bw(&p->dl, &rq->dl);

         if (!p->dl.dl_non_contending)
           task_non_contending(&p->dl, false);
    }

I ran the same collapse on top of the series and it passed the selftests
with LOCKDEP w/o warnings.

Thanks,
Juri