Re: [cgroup/for-6.19 PATCH 3/3] cgroup/cpuset: Globally track isolated_cpus update

[Waiman Long]

On 11/3/25 1:46 AM, Chen Ridong wrote:
> On 2025/11/3 9:34, Waiman Long wrote:
> > The current cpuset code passes a local isolcpus_updated flag around in a
> > number of functions to determine if external isolation related cpumasks
> > like wq_unbound_cpumask should be updated. It is a bit cumbersome and
> > makes the code more complex. Simplify the code by using a global boolean
> Agree.
>
> > flag "isolated_cpus_updating" to track this. This flag will be set in
> > isolated_cpus_update() and cleared in update_isolation_cpumasks().
> >
> > No functional change is expected.
> >
> > Signed-off-by: Waiman Long <longman@xxxxxxxxxx>
> > ---
> >   kernel/cgroup/cpuset.c | 74 ++++++++++++++++++++----------------------
> >   1 file changed, 35 insertions(+), 39 deletions(-)
> >
> > diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> > index d6d459c95d82..406a1c3789f5 100644
> > --- a/kernel/cgroup/cpuset.c
> > +++ b/kernel/cgroup/cpuset.c
> > @@ -81,6 +81,13 @@ static cpumask_var_t	subpartitions_cpus;
> >    */
> >   static cpumask_var_t	isolated_cpus;
> >   
> Is isolated_cpus protected by cpuset_mutex or callback_lock?
>
> If isolated_cpus is indeed protected by cpuset_mutex, perhaps we can move the update of
> isolated_cpus outside the critical section of callback_lock. This would allow us to call
> update_isolation_cpumasks in isolated_cpus_update, making the isolated_cpus_updating variable
> unnecessary. Reducing a global variable would be beneficial.

isolated_cpus is a user visible cpumask. So I would like to protect it 
with both cpuset_mutex and callback_lock like the other user visible 
cpumasks.


> > +/*
> > + * isolated_cpus updating flag (protected by cpuset_mutex)
> > + * Set if isolated_cpus is going to be updated in the current
> > + * cpuset_mutex crtical section.
> > + */
> > +static bool isolated_cpus_updating;
> > +
> >   /*
> >    * Housekeeping (HK_TYPE_DOMAIN) CPUs at boot
> >    */
> > @@ -1327,13 +1334,14 @@ static void isolated_cpus_update(int old_prs, int new_prs, struct cpumask *xcpus
> >   		cpumask_or(isolated_cpus, isolated_cpus, xcpus);
> >   	else
> >   		cpumask_andnot(isolated_cpus, isolated_cpus, xcpus);
> > +
> > +	isolated_cpus_updating = true;
> >   }
> >   
> >   /*
> >    * isolated_cpus_should_update - Returns if the isolated_cpus mask needs update
> >    * @prs: new or old partition_root_state
> >    * @parent: parent cpuset
> > - * Return: true if isolated_cpus needs modification, false otherwise
> >    */
> >   static bool isolated_cpus_should_update(int prs, struct cpuset *parent)
> >   {
> > @@ -1347,15 +1355,12 @@ static bool isolated_cpus_should_update(int prs, struct cpuset *parent)
> >    * @new_prs: new partition_root_state
> >    * @parent: parent cpuset
> >    * @xcpus: exclusive CPUs to be added
> > - * Return: true if isolated_cpus modified, false otherwise
> >    *
> >    * Remote partition if parent == NULL
> >    */
> > -static bool partition_xcpus_add(int new_prs, struct cpuset *parent,
> > +static void partition_xcpus_add(int new_prs, struct cpuset *parent,
> >   				struct cpumask *xcpus)
> >   {
> > -	bool isolcpus_updated;
> > -
> >   	WARN_ON_ONCE(new_prs < 0);
> >   	lockdep_assert_held(&callback_lock);
> >   	if (!parent)
> > @@ -1365,13 +1370,11 @@ static bool partition_xcpus_add(int new_prs, struct cpuset *parent,
> >   	if (parent == &top_cpuset)
> >   		cpumask_or(subpartitions_cpus, subpartitions_cpus, xcpus);
> >   
> > -	isolcpus_updated = (new_prs != parent->partition_root_state);
> > -	if (isolcpus_updated)
> > +	if (new_prs != parent->partition_root_state)
> Can this if statement be replaced with new helper isolated_cpus_should_update？

The isolated_cpus_should_update() helper is for validating the change. 
It is too late to call in partition_xcpus_add/del().

Cheers, Longman