Re: [PATCH v5 19/29] cgroup/cpuset: Wait for an attach before rebuilding the sched domains

[Waiman Long <longman@redhat.com>]

On 11/8/26 1:53 PM, Mathieu Desnoyers wrote:
> cpuset_can_attach() reserves the bandwidth of the deadline tasks it
> moves in the destination root domain. cpuset_attach() moves their
> bandwidth later, on the strength of that reservation.
>
> Commit 75f7a25ec6bb ("cgroup/cpuset: Prevent race between task attach
> and cpuset state change") lets cpuset_attach() rely on what
> cpuset_can_attach() set up. The writes of cpus, mems and cpus.partition
> wait for an attach in progress, in wait_attach_done_lock(). CPU hotplug
> cannot run in between either: it takes cpus_write_lock, and the attach
> holds cpus_read_lock throughout.
>
> But a rebuild of the sched domains does not wait. A write of
> sched_rt_runtime_us and the energy model rebuild them through
> rebuild_sched_domains(). The cgroup v1 writes of sched_load_balance and
> sched_relax_domain_level, and hk_sd_workfn(), use cpuset_full_lock().
>
> In this example, a rebuild drops the reservation. T is a deadline task
> asleep in root domain R. Thread A moves T to a partition root P, which
> has a root domain of its own. Thread B writes sched_rt_runtime_us:
>
>    A                                      B
>
>    cpuset_can_attach()
>      takes cpuset_mutex
>      cpuset_reserve_dl_bw()
>        P's total_bw += T's bandwidth
>      drops cpuset_mutex
>                                           rebuild_sched_domains()
>                                             takes cpuset_mutex
>                                             dl_rebuild_rd_accounting()
>                                               clears R's and P's total_bw
>                                               counts T in R (not moved yet)
>                                             drops cpuset_mutex
>    cpuset_attach()
>      takes cpuset_mutex
>      cpuset_attach_task(T)
>        set_cpus_allowed_dl(T)
>          R's total_bw -= T's bandwidth
>          records a CPU of P
>
> P now lacks the bandwidth of T, and admits deadline tasks beyond its
> capacity. When T leaves P, P's total_bw underflows. In testing, the
> race left a destination admitting 3 tasks of 10% too many, or P
> admitting no task at all.
>
> Fix this by waiting for an attach in progress in
> rebuild_sched_domains_cpuslocked() and in cpuset_full_lock(). The
> latter then takes its locks in the order of the writes of cpus and
> cpus.partition. A rebuild from CPU hotplug finds no attach in progress.
> The online, offline and kill of a cpuset never wait: they hold
> cgroup_mutex, which the attach holds throughout.
>
> The move_vs_rebuild case of deadline_bw_test, added later in this
> series, tests this race. Without this patch, it fails in 5 of 8 VMs with
> 8 CPUs. With it, the case passes in 8 of 8. The next patch needs this
> wait too: without it, it would give back a reservation that a rebuild
> dropped already.
>
> Found by the Sashiko AI reviews of v2 (the race) and v3 (the cgroup v1
> writes) of this series.
>
> 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>
> ---
>   kernel/cgroup/cpuset.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 194a438e25c3..51b6be460794 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -296,17 +296,20 @@ void lockdep_assert_cpuset_lock_held(void)
>   	lockdep_assert_held(&cpuset_mutex);
>   }
>   
> +static void wait_attach_done_lock(void) __acquires(&cpuset_mutex);
> +
>   /**
>    * cpuset_full_lock - Acquire full protection for cpuset modification
>    *
>    * Takes both CPU hotplug read lock (cpus_read_lock()) and cpuset mutex
> - * to safely modify cpuset data.
> + * to safely modify cpuset data. Waits for an attach in progress before
> + * it takes cpuset mutex.
>    */
>   void cpuset_full_lock(void)
>   {
>   	mutex_lock(&cpuset_top_mutex);
>   	cpus_read_lock();
> -	mutex_lock(&cpuset_mutex);
> +	wait_attach_done_lock();
>   }
>   
>   void cpuset_full_unlock(void)
> @@ -1005,7 +1008,14 @@ void rebuild_sched_domains_locked(void)
>   
>   static void rebuild_sched_domains_cpuslocked(void)
>   {
> -	mutex_lock(&cpuset_mutex);
> +	/*
> +	 * Wait for an attach in progress. cpuset_can_attach() has reserved
> +	 * the deadline bandwidth of the moving tasks in their new root
> +	 * domain, and cpuset_attach() moves it on the strength of that
> +	 * reservation. A rebuild of the accounting in between would drop
> +	 * the reservation.
> +	 */
> +	wait_attach_done_lock();
>   	rebuild_sched_domains_locked();
>   	mutex_unlock(&cpuset_mutex);
>   }

I agree that rebuild_sched_domains() should wait for an in-progress 
attach operation. The change to cpuset_full_lock(), however, is not 
really necessary. This cpuset_full_lock() is now mainly used for 
operations like cpuset_css_online() that are initiated from the cgroup 
side where cgroup_mutex should have been held. IOW, an attach operation 
which requires holding cgroup_mutex cannot happen in parallel. The only 
exception is hk_sd_workfn() which is a work item for updating the 
housekeeping cpumasks from hotplug which has no dependency on an attach 
operation.

Cheers,
Longman