Re: [PATCH v2 21/21] sched/deadline: Count the bandwidth a boosted task is given

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] sched/deadline: dl_change_utilization() checks old flags before update
--

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

sched/deadline: Count the bandwidth a boosted task is given

This commit moves the runqueue bandwidth accounting update from
sched_dl_overflow() to __setparam_dl() so that tasks already in the deadline
class due to priority inheritance have their new bandwidth added to the
runqueue's this_bw when sched_setattr() sets SCHED_DEADLINE parameters.

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 15d224846ebad..a3b45bb4f35e1 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -442,22 +442,32 @@ void cancel_inactive_timer(struct sched_dl_entity *dl_se)
>  	cancel_dl_timer(dl_se, &dl_se->inactive_timer);
>  }
>  
> +/*
> + * The runqueue a sleeping task is on counts the bandwidth the task has:
> + * give it @new_bw in place of what it counts of the task's.
> + */
>  static void dl_change_utilization(struct task_struct *p, u64 new_bw)
>  {
> -	WARN_ON_ONCE(p->dl.flags & SCHED_FLAG_SUGOV);
> +	struct sched_dl_entity *dl_se = &p->dl;
> +	struct rq *rq = task_rq(p);
>  
> -	if (task_on_rq_queued(p))
> +	if (task_on_rq_queued(p) || dl_entity_is_special(dl_se))
>  		return;

[ ... ]

> @@ -4266,6 +4275,18 @@ int sched_dl_overflow(struct task_struct *p, int policy,
>  void __setparam_dl(struct task_struct *p, const struct sched_attr *attr)
>  {
>  	struct sched_dl_entity *dl_se = &p->dl;
> +	u64 period = attr->sched_period ?: attr->sched_deadline;
> +
> +	/*
> +	 * The bandwidth of a special entity is counted on no runqueue.
> +	 * The rest have theirs changed here, where their parameters are:
> +	 * switched_to_dl() hands it to the runqueue of a task that enters
> +	 * the class, and a task that priority inheritance boosted into the
> +	 * class is in it already, so nothing dequeues it and enqueues it
> +	 * back for that.
> +	 */
> +	if (!(attr->sched_flags & SCHED_FLAG_SUGOV))
> +		dl_change_utilization(p, to_ratio(period, attr->sched_runtime));
>  
>  	dl_se->dl_runtime = attr->sched_runtime;
>  	dl_se->dl_deadline = attr->sched_deadline;

[Severity: Medium]
Does dl_change_utilization() check the old dl_se->flags before __setparam_dl()
updates them?

In __setparam_dl(), dl_change_utilization() is called when attr->sched_flags
does not have SCHED_FLAG_SUGOV set, before dl_se->flags is updated from
attr->sched_flags:

kernel/sched/deadline.c:__setparam_dl() {
	if (!(attr->sched_flags & SCHED_FLAG_SUGOV))
		dl_change_utilization(p, to_ratio(period, attr->sched_runtime));

	dl_se->dl_runtime = attr->sched_runtime;
	dl_se->dl_deadline = attr->sched_deadline;
	dl_se->dl_period = attr->sched_period ?: dl_se->dl_deadline;
	dl_se->flags = attr->sched_flags & SCHED_DL_FLAGS;
}

If a sleeping task that previously had SCHED_FLAG_SUGOV set in dl_se->flags is
reconfigured via __sched_setscheduler() -> __setscheduler_params() ->
__setparam_dl() without SCHED_FLAG_SUGOV, dl_entity_is_special(dl_se) in
dl_change_utilization() still observes the old SCHED_FLAG_SUGOV flag and
returns early without adding new_bw to rq->dl.this_bw or setting
dl_se->dl_bw_oncpu = 1.

Immediately afterward, __setparam_dl() clears SCHED_FLAG_SUGOV in dl_se->flags,
and switched_to_dl() does not run because the task is already in
dl_sched_class. When the task later wakes up on the same CPU, task_contending()
sees !dl_entity_is_special(dl_se) and adds dl_se->dl_bw to rq->dl.running_bw
while rq->dl.this_bw does not include it, triggering:

	WARN_ON_ONCE(dl_rq->running_bw > dl_rq->this_bw);

Since __setparam_dl() is the only caller of dl_change_utilization() and already
checks !(attr->sched_flags & SCHED_FLAG_SUGOV), should the
dl_entity_is_special(dl_se) check in dl_change_utilization() be dropped?

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