Re: [PATCH v5 16/29] sched/deadline: Record the root domain holding a task's bandwidth

[Juri Lelli]

Hello,

On 08/11/26 13:53, Mathieu Desnoyers wrote:
> Admission, moves and releases of a deadline task's bandwidth find the
> root domain that counts it from the CPU the task is on. That CPU can be
> in another root domain. Two cases show it.
> 
> Commit 7f51412a415d ("sched/deadline: Fix bandwidth check/update when
> migrating tasks between exclusive cpusets") moves a deadline task's
> bandwidth in two halves. cpuset_can_attach() reserves it in the
> destination. set_cpus_allowed_dl() frees it in the root domain of the
> task's runqueue. Both halves now move nothing when that root domain
> overlaps the destination. But a sleeping task stays on the CPU it last
> ran on. So a sleeping task moved to a partition and back leaves its
> bandwidth in the partition:
> 
>   admin                                       task T
> 
>   makes P a partition root of CPU 3
>     CPU 3 is now in root domain B
>     CPUs 0-2 are in root domain A
>                                               sched_setattr(SCHED_DEADLINE)
>                                                 A: total_bw += bw of T
>                                               runs on CPU 0, then sleeps
>   echo T > P/cgroup.procs
>     cpuset_can_attach()
>       dl_task_needs_bw_move(): true,
>       A does not overlap P
>       B: total_bw += bw of T
>     set_cpus_allowed_dl()
>       dl_task_needs_bw_move(): true
>       A: total_bw -= bw of T
>                                               still asleep on CPU 0
>   echo T > cgroup.procs of the root cgroup
>     cpuset_can_attach()
>       dl_task_needs_bw_move(): false,
>       A overlaps the root cpuset
>     set_cpus_allowed_dl()
>       dl_task_needs_bw_move(): false
>   kill -9 T
>                                               wakes up on a CPU of A
>                                               exits
>                                                 A: total_bw -= bw of T
> 
> B keeps the bandwidth of T. A does not count T while T is in A, so A
> admits beyond its capacity. When T exits, A loses the bandwidth of T a
> second time. Its total_bw can underflow.
> 
> A rebuild of the accounting counts a task in the root domain of its
> cpuset. With admission control off, a task can be pinned to one CPU.
> When that CPU goes offline, it joins the default root domain, and the
> release takes the bandwidth from there. The root domain of the cpuset
> keeps it until the next rebuild.
> 
> The first case was reproduced on v7.4-rc1. The second was reproduced
> with the earlier patches of this series. Without "sched/deadline:
> Release the bandwidth of a task that leaves the class asleep", nothing
> releases the bandwidth of that sleeping task.
> 
> Fix this by recording, in dl_bw_cpu, a CPU of the root domain that
> counts the bandwidth. Each site that counts it sets the record. Each
> release or move reads it, and the release clears it. A move skips a
> bandwidth of 0, so sched_dl_overflow() does not use its record. The
> move_zero_bw case of deadline_bw_test, added later in this series, tests
> this. The record replaces dl_departed_cpu. If dl_bw_gen shows that a
> rebuild dropped the bandwidth, nothing is taken back.
> 
> A move now leaves out a sugov kthread and a task boosted into the class
> with no bandwidth of its own. The rebuild leaves them out too: no root
> domain counts theirs. A move would take a sugov kthread's bandwidth
> from a root domain that never held it.
> 
> Weiheng Qiu reported the moved sleeper, and Juri Lelli analysed it. His
> fix, linked below, took the first active CPU of the task's mask. A mask
> can have none during CPU hotplug. Tip dropped the fix after a sugov
> kthread hit its BUG_ON() there. The Sashiko AI reviews of this series
> found the other cases.
> 
> Reported-by: Weiheng Qiu <weihengqiuu@xxxxxxxxx>
> Closes: https://lore.kernel.org/all/14508cb2-805f-41ac-9f6b-bfd7c3d7cb1c@xxxxxxxxx/
> Link: https://lore.kernel.org/all/20261103-b4-sched-dl-bw-fixes-v1-1-dd761fe005b4@xxxxxxxxxx/
> Fixes: 7f51412a415d ("sched/deadline: Fix bandwidth check/update when migrating tasks between exclusive cpusets")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> ---

...

> @@ -3541,17 +3570,29 @@ static void set_cpus_allowed_dl(struct task_struct *p,
>  	 * domain (see cpuset_can_attach()).
>  	 */
>  	if (dl_task_needs_bw_move(p, ctx->new_mask)) {
> -		struct dl_bw *src_dl_b;
> +		unsigned int src_cpu = dl_task_root_domain_cpu(p);
> +		struct dl_bw *src_dl_b = &cpu_rq(src_cpu)->rd->dl_bw;
> +		unsigned int cpu;
>  
> -		src_dl_b = dl_bw_of(cpu_of(rq));
>  		/*
>  		 * We now free resources of the root_domain we are migrating
>  		 * off. In the worst case, sched_setattr() may temporary fail
> -		 * until we complete the update.
> +		 * until we complete the update. A rebuild that dropped the
> +		 * bandwidth of @p leaves nothing to free.
> +		 */
> +		if (p->dl.dl_bw_gen == dl_bw_gen) {

So, this is not guarded by dl_departed_lock, but then this gets fixed in
later patches?

> +			raw_spin_lock(&src_dl_b->lock);
> +			__dl_sub(src_dl_b, p->dl.dl_bw, dl_bw_cpus(src_cpu));
> +			raw_spin_unlock(&src_dl_b->lock);
> +		}

...

> @@ -4189,6 +4245,23 @@ int sched_dl_overflow(struct task_struct *p, int policy,
>  	if (new_bw == p->dl.dl_bw && task_has_dl_policy(p))
>  		return 0;
>  
> +	/*
> +	 * Change the bandwidth of a deadline task where it is counted: in
> +	 * the root domain of the CPU recorded with it. A task moved asleep
> +	 * has not run there yet, so the CPU it is on can be elsewhere. No
> +	 * bandwidth is counted if a rebuild dropped it, or if the task
> +	 * enters the class. A runtime below about a millionth of the period
> +	 * gives a bandwidth of 0. It counts for nothing, and no move updates
                    ^^^
Ah right, quite cryptic corner case buried in this comment. :)

This is the 1024ns/2s of the test. Which indeed gets dl_bw 0. Don't
think it's a huge deal from a theory/practice pov as to actually make an
impact we probably need quite a lot of these admitted (fork bomb style).
But it's still weird :P. Not sure if we want to impose a min of 1 dl_bw
for these. If it actually complicates the accounting (for no good
returns) or not.

> +	 * its record. In these three cases, the root domain of the CPU the
> +	 * task is on takes the new bandwidth. The caller holds cpuset_mutex,
> +	 * or dl_accounting_mutex without cpusets, so no rebuild runs
> +	 * meanwhile.
> +	 */
> +	counted = task_has_dl_policy(p) && p->dl.dl_bw_cpu >= 0 &&
> +		  p->dl.dl_bw_gen == dl_bw_gen;
> +	cpu = counted && p->dl.dl_bw ? p->dl.dl_bw_cpu : task_cpu(p);
> +	dl_b = dl_bw_of(cpu);
> +

Thanks,
Juri