Re: [PATCH v5 20/29] cgroup/cpuset: Give back the bandwidth an attach did not move

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

Hello,

On 08/11/26 13:53, Mathieu Desnoyers wrote:
> Commit 2ef269ef1ac0 ("cgroup/cpuset: Free DL BW in case can_attach()
> fails") reserves the bandwidth of an attach as one sum.
> cpuset_can_attach() adds up the bandwidth of the deadline tasks it
> moves. Once all tasks pass their checks, it reserves the sum in the
> destination root domain. If another controller refuses the attach,
> cpuset_cancel_attach() frees the sum. If the attach goes through, the
> move of each task uses its part of the sum.
> 
> But a task can change its policy, or die, before the attach:
> 
>  - A task that left SCHED_DEADLINE, or died, moves no bandwidth. Its
>    part of the reservation stays in the destination's total_bw, until
>    the next rebuild of the accounting.
>  - A task that became a deadline task has nothing reserved. The attach
>    still moves its bandwidth. The destination lacks it, and its
>    total_bw underflows when the task leaves.
> 
> In testing, a task switched between SCHED_DEADLINE and SCHED_OTHER
> while it was moved 200 times into a one-CPU partition root and back.
> On v7.4-rc1 with the patches before this one, the partition's total_bw
> ended wrong in 5 of 6 VMs with 8 CPUs. It kept up to two reservations,
> or wrapped below zero.
> 
> Fix this by keeping the tally of what the attach moves, in
> sum_moved_dl_bw. Flag the move of a task only if the rest of the
> reservation covers its bandwidth. Once the attach is through, give back
> what it did not move.
> 
> set_cpus_allowed_dl() decides the move under the runqueue lock.
> cpuset_attach_task() does not hold it. dl_task_offline_migration() can
> change the task's record meanwhile, for a task throttled on a CPU gone
> offline. So the tally counts what set_cpus_allowed_dl() reports in the
> affinity context.
> 
> With this patch, total_bw ended right in 6 VMs of 6. The random_mix case
> of deadline_bw_test, added later in this series, found the bug. It could
> no longer lower a fair server in a partition root with no room left.
> 
> Fixes: 2ef269ef1ac0 ("cgroup/cpuset: Free DL BW in case can_attach() fails")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---

...

> @@ -3285,17 +3299,28 @@ static void cpuset_attach_task(struct cpuset *cs, struct task_struct *task)
>  	 * cpuset_reserve_dl_bw() made room in @cs for the bandwidth of the
>  	 * deadline tasks that cpuset_can_attach() counted. Flag the move, so
>  	 * that the bandwidth of this task moves with it.
> +	 *
> +	 * Nothing was reserved for a task that became a deadline task since.
> +	 * Flag its move only if the rest of the reservation covers its
> +	 * bandwidth.
>  	 */
> -	if (dl_task(task) && dl_task_needs_bw_move(task, cs->effective_cpus))
> +	if (dl_task(task) && dl_task_needs_bw_move(task, cs->effective_cpus) &&
> +	    cs->sum_moved_dl_bw + task->dl.dl_bw <= cs->sum_migrate_dl_bw)
>  		reserved = true;

Humm, this is identity-blind: it cannot tell a task that can_attach()
counted (its bandwidth really is inside sum_migrate_dl_bw) from one that
became a deadline task in the can_attach() -> attach() gap, for which
nothing was reserved, can it?

Say A and B, 50 each, are reserved for a sum of 100, and C becomes a
deadline task with 70 in the gap (can_attach() <-> attach()). If C comes
first in the taskset it passes 0 + 70 <= 100 and moves; A and B then
fail 70 + 50 <= 100 and both properly reserved tasks are denied their
moves?

Not sure if we can consider tracking the counted tasks per task? The
taskset is pinned between can_attach() and attach(), so we could build a
list of (task, bw) pairs at can_attach() and let the gate check
membership?

Thanks,
Juri