Re: [PATCH v1] sched/deadline: Fix this_bw on class change of a dying task

[Juri Lelli]

Hello,

On 25/10/26 08:46, Nickolai Zeldovich wrote:
> Moving a SCHED_DEADLINE task out of the class with sched_setscheduler(2)
> after it stored TASK_DEAD in do_task_dead(), but before __schedule()
> dequeued it, subtracts its bandwidth from the runqueue's this_bw twice.
> this_bw then stays short until reboot: __sub_rq_bw() warns about the
> underflow or about running_bw > this_bw, and GRUB reclaim for
> SCHED_FLAG_RECLAIM tasks on that CPU is miscomputed.
> 
> The class change dequeues the still queued task with DEQUEUE_SAVE, which
> removes its bandwidth from running_bw and this_bw. switched_from_dl()
> then calls task_non_contending(), which, if the 0-lag time has passed,
> removes it from this_bw again because the task is TASK_DEAD. That
> removal is meant for the dying task's own DEQUEUE_SLEEP dequeue, which
> has removed nothing before it.
> 
> Do the TASK_DEAD removal only on that dequeue path.
> 
> Reproduced under QEMU on x86_64 and riscv64 by moving exiting deadline
> tasks to SCHED_OTHER in a loop; with this patch there is no warning.
> 
> Fixes: 8fd27231c330 ("sched/deadline: Track the "total rq utilization" too")
> Signed-off-by: Nickolai Zeldovich <nickolai@xxxxxxxxxxxxx>
> ---

Thanks for the report, reproducer and fix!

I adapted the following to the set of other tests I maintain. You can
find my attempt here [1]. If you want to create a MR please go ahead,
otherwise I will merge mine (if you are OK with it :).

1 - https://github.com/jlelli/sched-deadline-tests/commit/ccea314c39957a935113f6a8c058690a3a48133b

Tested-by: Juri Lelli <juri.lelli@xxxxxxxxxx>

> Notes:
>     Reproducer (root or CAP_SYS_NICE, two or more CPUs; the warnings are
>     WARN_ON_ONCE, so boot afresh for each run):
>     
>       gcc -O2 -o dl-dead-short dl-dead-short.c && ./dl-dead-short
>       dmesg | grep 'WARNING:.*deadline'
>     

...

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index c0ebdcde5fe5d..dfae08625342e 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -444,7 +444,7 @@ static void task_non_contending(struct sched_dl_entity *dl_se, bool dl_task)
>  			if (!dl_task || READ_ONCE(p->__state) == TASK_DEAD) {
>  				struct dl_bw *dl_b = dl_bw_of(task_cpu(p));
>  
> -				if (READ_ONCE(p->__state) == TASK_DEAD)
> +				if (dl_task && READ_ONCE(p->__state) == TASK_DEAD)
>  					sub_rq_bw(dl_se, &rq->dl);
>  				raw_spin_lock(&dl_b->lock);
>  				__dl_sub(dl_b, dl_se->dl_bw, dl_bw_cpus(task_cpu(p)));
> 

And this makes sense to me.

Acked-by: Juri Lelli <juri.lelli@xxxxxxxxxx>

Best,
Juri