Re: [PATCH v2 17/21] cgroup/cpuset: Give back the bandwidth an attach did not move

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] cgroup/cpuset: clear_attach_data() frees rebuilt bandwidth on concurrent rebuild
--

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

cgroup/cpuset: Give back the bandwidth an attach did not move

This commit tracks the deadline bandwidth actually moved during
cpuset_attach_task() in sum_moved_dl_bw via set_cpus_allowed_bw_reserved()
and frees any unused reservation (sum_migrate_dl_bw - sum_moved_dl_bw) in
clear_attach_data().

> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index ae0bbe23a6332..1ae86338f3c2a 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -3197,10 +3197,24 @@ static void clear_attach_data(struct llist_head *head, bool cancel)
>  		if (cs->nr_migrate_dl_tasks) {
>  			if (!cancel)
>  				atomic_add(cs->nr_migrate_dl_tasks, &cs->nr_deadline_tasks);
> -			else if (cs->dl_bw_cpu >= 0) /* && cancel */
> -				dl_bw_free(cs->dl_bw_cpu, cs->sum_migrate_dl_bw);
> +			/*
> +			 * Give back the bandwidth the attach did not move:
> +			 * all of the reservation when it is cancelled, and
> +			 * what was set aside for the tasks that are no
> +			 * longer deadline tasks otherwise. Theirs is
> +			 * released by the class they left, from the root
> +			 * domain that counts it.
> +			 */
> +			if (cs->dl_bw_cpu >= 0) {
> +				u64 bw = cancel ? cs->sum_migrate_dl_bw :
> +					 cs->sum_migrate_dl_bw - cs->sum_moved_dl_bw;
> +
> +				if (bw)
> +					dl_bw_free(cs->dl_bw_cpu, bw);
> +			}

[Severity: High]
Can clear_attach_data() free bandwidth that was already wiped or attributed
to the migrated tasks if rebuild_sched_domains() runs between
cpuset_can_attach() and cpuset_attach()?

During cgroup migration, cgroup_attach_lock() holds cpus_read_lock(), and
cpuset_can_attach() reserves cs->sum_migrate_dl_bw in cs->dl_bw_cpu's root
domain via cpuset_reserve_dl_bw(), increments attach_ctx.in_progress, and
drops cpuset_mutex.

Before cpuset_attach() runs, another thread calling rebuild_sched_domains()
(for instance via sched_rt_handler() or rebuild_sched_domains_energy()) only
takes cpus_read_lock() and mutex_lock(&cpuset_mutex) directly rather than
waiting on attach_ctx.in_progress via wait_attach_done_lock():

kernel/cgroup/cpuset.c:rebuild_sched_domains_cpuslocked() {
	mutex_lock(&cpuset_mutex);
	rebuild_sched_domains_locked();
	mutex_unlock(&cpuset_mutex);
}

rebuild_sched_domains_locked() -> partition_sched_domains() ->
dl_rebuild_rd_accounting() calls dl_clear_root_domains(), resetting
rd->dl_bw.total_bw to 0 and wiping the reservation made by
cpuset_reserve_dl_bw().

If dl_rebuild_rd_accounting() runs after css_set_move_task() moves the task
into cs, dl_add_task_root_domain() accounts the task's bandwidth into cs's
root domain and sets task->dl.dl_bw_cpu to a CPU in cs. When
cpuset_attach_task() then runs, dl_task_needs_bw_move(task,
cs->effective_cpus) returns false because task->dl.dl_bw_cpu is already in
cs's root domain, leaving cs->sum_moved_dl_bw at 0. Even if the rebuild runs
before css_set_move_task() and the task leaves SCHED_DEADLINE before
cpuset_attach_task(), the reservation in cs's root domain has already been
cleared by dl_clear_root_domains().

When clear_attach_data() subsequently calls dl_bw_free(cs->dl_bw_cpu,
cs->sum_migrate_dl_bw - cs->sum_moved_dl_bw), wouldn't it subtract
cs->sum_migrate_dl_bw from the rebuilt total_bw and leave the destination
root domain's total_bw short of what its tasks hold?

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