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

[Waiman Long <llong@redhat.com>]

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@redhat.com>
>> ---
>>   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