Re: [PATCH 06/23] rcu/nocbs: Allow runtime changes in RCU NOCBS cpumask
Waiman Long <[email protected]>
| Newsgroups | dev.linux.lists.sashiko,org.kernel.vger.linux-hwmon |
|---|---|
| Message-ID | <[email protected]> |
On 4/21/26 11:08 PM, [email protected] wrote: > Sashiko AI review found 1 potential issue(s): > - [High] Dynamic RCU NOCB update attempts to de-offload online `rcu_nocbs=` CPUs, triggering a WARN_ON_ONCE and leaving isolation state inconsistent. > -- > > commit a58a92fdc96e43de871fd44a78e4e6da373179d5 > Author: Waiman Long <[email protected]> > > rcu/nocbs: Allow runtime changes in RCU NOCBS cpumask > > This commit enables dynamic runtime updates to the RCU NO-CB CPU offloading > state when isolated CPUs are added or removed. It introduces a new helper, > rcu_nocb_update_cpus(), which is called directly from housekeeping_update() > to synchronize the offloaded CPUs with the HK_TYPE_KERNEL_NOISE cpumask. > >> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h >> index 2d06dcb61f379..b2daba1e5cb98 100644 >> --- a/kernel/rcu/tree_nocb.h >> +++ b/kernel/rcu/tree_nocb.h > [ ... ] >> +void rcu_nocb_update_cpus(struct cpumask *cpumask) >> +{ >> + int cpu, ret; >> + >> + if (!rcu_state.nocb_is_setup) { >> + pr_warn_once("Dynamic RCU NOCB cannot be enabled without nohz_full/rcu_nocbs kernel boot parameter!\n"); >> + return; >> + } >> + >> + for_each_cpu_andnot(cpu, cpumask, rcu_nocb_mask) { >> + ret = rcu_nocb_cpu_offload(cpu); >> + if (WARN_ON_ONCE(ret)) >> + return; >> + } >> + >> + for_each_cpu_andnot(cpu, rcu_nocb_mask, cpumask) { >> + ret = rcu_nocb_cpu_deoffload(cpu); > What happens here if a system booted with explicit rcu_nocbs= CPUs, and > later a user dynamically updates an isolated cpuset? > > It looks like this loop assumes the active rcu_nocb_mask should identically > match the new isol_mask. If the boot-time rcu_nocbs= CPUs are not included > in the new isolation mask, this loop will attempt to de-offload them. > > Because the cpuset subsystem is unaware of the independent rcu_nocbs= mask, > wouldn't those original CPUs likely remain online? > Yes, the code should properly handle a pre-existing "rcu_nocbs" boot option with a different CPU list. I will update the patch to add a boot version of rcu_nocb_mask and make sure that CPUs that are in rcu_nocb_mask_boot will not be de-offloaded. >> + if (WARN_ON_ONCE(ret)) >> + return; > If rcu_nocb_cpu_deoffload() is called on an online CPU, it returns -EINVAL. > Could this unconditionally trigger this WARN_ON_ONCE and abort the rest of > the update loop? It should only be called for an offline CPU. The WARN_ON_ONCE() is there to just catch unexpected bug in the code. > > If the loop aborts early, since housekeeping_update() already committed the > new mask to housekeeping.cpumasks[], does this leave the system in a > desynchronized state where the scheduler assumes CPUs are isolated but > RCU callback offloading is not properly updated? Any triggered WARN_ON_ONCE() is a bug that needs to be investigated and fixed. Cheers, Longman > >> + } >> +}