Re: [PATCH] cgroup/cpuset: Use effective_xcpus in partcmd_update add/del mask calculation

[Waiman Long <longman@redhat.com>]

On 5/22/26 4:41 AM, Guopeng Zhang wrote:
>
> 在 2026/5/22 15:53, Sun Shaojie 写道:
>> When sibling CPU exclusion occurs, a partition's user_xcpus may contain
>> CPUs that were never actually granted to it. These CPUs are present in
>> user_xcpus(cs) but not in cs->effective_xcpus.
>>
>> The partcmd_update path in update_parent_effective_cpumask() uses
>> user_xcpus(cs) (via the local variable xcpus) to compute the addmask
>> (CPUs to return to parent) and delmask (CPUs to request from parent).
>> This is incorrect:
>>
>>   1) When newmask removes a CPU that was previously excluded by a
>>      sibling, addmask incorrectly includes that CPU and tries to return
>>      it to the parent even though the partition never actually owned it,
>>      causing CPU overlap with sibling partitions and triggering warnings
>>      in generate_sched_domains().
>>
>>   2) When newmask adds a previously excluded CPU that is now available,
>>      delmask fails to request it from the parent because user_xcpus(cs)
>>      already includes it.
>>
>> Fix this by using cs->effective_xcpus instead of user_xcpus(cs) in all
>> partcmd_update paths that calculate addmask or delmask, including the
>> PERR_NOCPUS error handling paths.
>>
>> Reproducers:
>>
>>    Example 1 - Removing a sibling-excluded CPU incorrectly returns it:
>>
>>      # cd /sys/fs/cgroup
>>      # echo "0-1" > a1/cpuset.cpus
>>      # echo "root" > a1/cpuset.cpus.partition
>>      # echo "0-2" > b1/cpuset.cpus
>>      # echo "root" > b1/cpuset.cpus.partition
>>      # echo "2" > b1/cpuset.cpus
>>      # cat cpuset.cpus.effective
>>      # Actual: 0-1,3    Expected: 3
>>
>>    Example 2 - Expanding to a previously excluded CPU fails to request it:
>>
>>      # cd /sys/fs/cgroup
>>      # echo "0-1" > a1/cpuset.cpus
>>      # echo "root" > a1/cpuset.cpus.partition
>>      # echo "0-2" > b1/cpuset.cpus
>>      # echo "root" > b1/cpuset.cpus.partition
>>      # echo "member" > a1/cpuset.cpus.partition
>>      # echo "1-2" > b1/cpuset.cpus
>>      # cat cpuset.cpus.effective
>>      # Actual: 0-1,3    Expected: 0,3
>>
>> Fixes: 2a3602030d80 ("cgroup/cpuset: Don't invalidate sibling partitions on cpuset.cpus conflict")
>> Signed-off-by: Sun Shaojie <sunshaojie@kylinos.cn>
>> ---
>>   kernel/cgroup/cpuset.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>> index 1335e437098e..5a5fa2481467 100644
>> --- a/kernel/cgroup/cpuset.c
>> +++ b/kernel/cgroup/cpuset.c
>> @@ -1821,11 +1821,11 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>   			deleting = cpumask_and(tmp->delmask,
>>   					newmask, parent->effective_xcpus);
>>   		} else {
>> -			cpumask_andnot(tmp->addmask, xcpus, newmask);
>> +			cpumask_andnot(tmp->addmask, cs->effective_xcpus, newmask);
>>   			adding = cpumask_and(tmp->addmask, tmp->addmask,
>>   					     parent->effective_xcpus);
>>   
>> -			cpumask_andnot(tmp->delmask, newmask, xcpus);
>> +			cpumask_andnot(tmp->delmask, newmask, cs->effective_xcpus);
>>   			deleting = cpumask_and(tmp->delmask, tmp->delmask,
>>   					       parent->effective_xcpus);
>>   		}
>> @@ -1864,7 +1864,7 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>   			part_error = PERR_NOCPUS;
>>   			deleting = false;
>>   			adding = cpumask_and(tmp->addmask,
>> -					     xcpus, parent->effective_xcpus);
>> +					     cs->effective_xcpus, parent->effective_xcpus);
>>   		}
>>   	} else {
>>   		/*
>> @@ -1886,7 +1886,8 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>   			part_error = PERR_NOCPUS;
>>   			if (is_partition_valid(cs))
>>   				adding = cpumask_and(tmp->addmask,
>> -						xcpus, parent->effective_xcpus);
>> +						     cs->effective_xcpus,
>> +						     parent->effective_xcpus);
>>   		} else if (is_partition_invalid(cs) && !cpumask_empty(xcpus) &&
>>   			   cpumask_subset(xcpus, parent->effective_xcpus)) {
>>   			struct cgroup_subsys_state *css;
> Hi, Shaojie
>
> The code change looks reasonable to me, but I think the comment above
> the partcmd_update calculation should be updated as well.
>
> Maybe it can be updated like this:
>
> 		 * Compute add/delete mask to/from effective_cpus
> 		 *
> 		 * For valid partition:
> -		 *   addmask = exclusive_cpus & ~newmask
> +		 *   addmask = cs->effective_xcpus & ~newmask
> 		 *			      & parent->effective_xcpus
> -		 *   delmask = newmask & ~exclusive_cpus
> +		 *   delmask = newmask & ~cs->effective_xcpus
> 		 *		       & parent->effective_xcpus
> 		 *
> 		 * For invalid partition:
>
> Does this look reasonable to you?
>
> Tested-by: Guopeng Zhang <zhangguopeng@kylinos.cn>

Shaojie, thanks for the fix. The changes look good to me. I also like 
the suggested changes in the comment as suggested by Guopeng. Would you 
mind sending a v2 with this additional change as well?

Thanks,
Longman