Re: [PATCH 20/23] cgroup/cpuset: Enable runtime update of HK_TYPE_{KERNEL_NOISE,MANAGED_IRQ} cpumasks

sashiko-bot
2026-04-22T03:09:08Z

Sashiko AI review found 6 potential issue(s):
- [High] Stale mask used for DOMAIN isolation when `tick_nohz_full_enabled()` is false, permanently breaking CPU isolation updates.
- [High] Dynamic CPU de-isolation fails silently for `HK_TYPE_KERNEL_NOISE` and `HK_TYPE_MANAGED_IRQ` masks, causing isolated CPUs to grow monotonically.
- [High] Lack of rollback on `housekeeping_update()` or hotplug failures causes unrecoverable state desynchronization.
- [High] `cpuset_nohz_update_cbfunc()` silently swallows the return value of `housekeeping_update()`, hiding failures from the hotplug callback.
- [High] Housekeeping updates triggered by partition invalidations are permanently lost during CPU offline callbacks.
- [Medium] TOCTOU race when reading `cpu_online_mask` after dropping `cpus_read_lock()` leads to potential hotplug abortion and state inconsistency.
--

commit 35eda8af3da1589e50363cd927f6b3b902b398a2
Author: Waiman Long <longman@redhat.com>
Subject: cgroup/cpuset: Enable runtime update of HK_TYPE_{KERNEL_NOISE,MANAGED_IRQ} cpumasks

This commit enables runtime updates for KERNEL_NOISE and MANAGED_IRQ
housekeeping cpumasks. It uses CPU hotplug to facilitate transitioning CPUs
into and out of isolated states when a nohz_full boot parameter is provided.

> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 1b0c50b46a49c..a927b9cd4f718 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c

[ ... ]

> @@ -1328,29 +1336,67 @@ static bool prstate_housekeeping_conflict(int prstate, struct cpumask *new_cpus)
> return false;
> }
>
> +static int cpuset_nohz_update_cbfunc(void *arg)
> +{
> + struct cpumask *isol_cpus = (struct cpumask *)arg;
> +
> + if (isol_cpus)
> + housekeeping_update(isol_cpus, BIT(HK_TYPE_KERNEL_NOISE));
> + return 0;
> +}

Is it intentional to discard the return value of housekeeping_update() here?
If housekeeping_update() fails, returning 0 will cause cpuhp_offline_cb()
to assume success, potentially hiding failures from the hotplug callback.

[ ... ]

> + /*
> + * Update isolated_nohz_cpus/isolated_mirq_cpus if necessary
> + */
> + if (!boot_nohz_le_domain) {
> + cpumask_andnot(cpus, cpu_possible_mask,
> + housekeeping_cpumask(HK_TYPE_KERNEL_NOISE));
> + cpumask_or(cpus, cpus, isolated_cpus);

Could this prevent dynamic CPU de-isolation? Because housekeeping_cpumask()
returns the current runtime mask rather than the boot mask, the newly
isolated CPUs remain in the mask when cpumask_or() is called. This might
cause isolated CPUs to grow monotonically rather than allowing removal.

> + update_nohz = !cpumask_equal(isolated_nohz_cpus, cpus);
> + if (update_nohz)
> + cpumask_copy(isolated_nohz_cpus, cpus);

If a failure occurs later in this function, these tracking masks are updated
but the state is not. For example, if cpuhp_offline_cb() fails, the code
jumps to out_free. If userspace then retries the operation, won't it see
cpumask_equal(isolated_cpus, isolated_hk_cpus) as true and return early,
leaving the system permanently out of sync?

[ ... ]

> @@ -1360,10 +1406,103 @@ static void cpuset_update_sd_hk_unlock(void)
> */
> mutex_unlock(&cpuset_mutex);
> cpus_read_unlock();

[ ... ]

> + /*
> + * Mask out offline CPUs in cpus
> + * If there is no online CPUs, we can call
> + * housekeeping_update() directly if needed.
> + */
> + cpumask_and(cpus, cpus, cpu_online_mask);

Can this race with CPU hotplug operations? The cpus_read_lock was released
earlier in this function. If a CPU is offlined concurrently right after this
check, cpuhp_offline_cb() might be passed a CPU that is no longer online,
potentially causing the hotplug loop to abort.

[ ... ]

> +/*
> + * cpuset_update_sd_hk_unlock - Rebuild sched domains, update HK & unlock
> + *
> + * Update housekeeping cpumasks and rebuild sched domains if necessary and
> + * then do a cpuset_full_unlock().
> + * This should be called at the end of cpuset operation.
> + */
> +static void cpuset_update_sd_hk_unlock(void)
> + __releases(&cpuset_mutex)
> + __releases(&cpuset_top_mutex)
> +{
> + /* force_sd_rebuild will be cleared in rebuild_sched_domains_locked() */
> + if (force_sd_rebuild)
> + rebuild_sched_domains_locked();
> +
> + update_housekeeping = false;
> +
> + if (cpumask_equal(isolated_cpus, isolated_hk_cpus)) {
> + cpuset_full_unlock();
> + return;
> + }
> +
> + if (!tick_nohz_full_enabled()) {
> + /*
> + * housekeeping_update() is now called without holding
> + * cpus_read_lock and cpuset_mutex. Only cpuset_top_mutex
> + * is still being held for mutual exclusion.
> + */
> + mutex_unlock(&cpuset_mutex);
> + cpus_read_unlock();
> + WARN_ON_ONCE(housekeeping_update(isolated_hk_cpus,
> + BIT(HK_TYPE_DOMAIN)));

Are we passing a stale mask here? If tick_nohz_full_enabled() is false, we
skip calling cpuset_update_housekeeping_unlock(), which means
isolated_hk_cpus is never updated to match isolated_cpus. Subsequent checks
for cpumask_equal(isolated_cpus, isolated_hk_cpus) will continue to fail,
and the new mask is never applied to the DOMAIN isolation.

> + mutex_unlock(&cpuset_top_mutex);
> + } else {
> + cpuset_update_housekeeping_unlock();
> + }
> +}

[ ... ]

> @@ -3954,7 +4116,10 @@ static void cpuset_handle_hotplug(void)
> */
> if (force_sd_rebuild)
> rebuild_sched_domains_cpuslocked();
> - if (update_housekeeping)
> + /*
> + * Don't need to update housekeeping cpumasks in cpuhp_offline_cb mode.
> + */
> + if (update_housekeeping && !cpuhp_offline_cb_mode)
> queue_work(system_dfl_wq, &hk_sd_work);
>
> free_tmpmasks(ptmp);

Does this cause housekeeping updates to be lost? If a CPU is offlined and
it invalidates a cpuset partition, update_housekeeping is set to true. If we
skip queuing the work here because cpuhp_offline_cb_mode is true, there
doesn't appear to be a mechanism to retroactively process the pending flag
once the bounce cycle completes.

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260421030351.281436-1-longman@redhat.com?part=20