Re: [PATCH-next 2/6] cgroup/cpuset: Properly compute effective xcpus in update_parent_effective_cpumask()

[Waiman Long <longman@redhat.com>]

On 10/22/26 9:45 AM, Guopeng Zhang wrote:
> Hi Longman,
>
> 在 2026/10/22 11:16, Waiman Long 写道:
>> Since the introduction of cpuset.cpus.exclusive in commit
>> e2ffe502ba45 ("cgroup/cpuset: Add cpuset.cpus.exclusive for v2"), a
>> compute_effective_exclusive_cpumask() helper which is later renamed to
>> compute_excpus() is added to compute the effective xcpus of a partition
>> root. However update_parent_effective_cpumask() is not updated in all
>> proper places to use the new helper to check if a given cpuset should
>> remain a valid partition root. This can cause a cpuset to incorrectly
>> remain a valid partition root even if no effective xcpus is left.
>>
>> Fix that problem by using compute_excpus() to compute the effective xcpus
>> into delmask in all cases except partcmd_disable and partcmd_invalidate
>> and use it for tasks_nocpu_error() check as well as checking its
>> emptiness. This enables us to catch invalid partition root more
>> accurately in update_parent_effective_cpumask().
>>
> I re-tested the repro from the link below on top of this series
> (applied on cgroup/for-7.4-fixes), and it still fails:
>
>    https://lore.kernel.org/all/20261010075947.333549-3-guopeng.zhang@linux.dev/
>
>    cd /sys/fs/cgroup
>    echo +cpuset > cgroup.subtree_control
>    mkdir A
>    echo 0-7 > A/cpuset.cpus
>    echo 0-7 > A/cpuset.cpus.exclusive
>    echo root > A/cpuset.cpus.partition
>    cat A/cpuset.cpus.partition
>    echo 1 > A/cpuset.cpus.exclusive
>    cat A/cpuset.cpus.partition
>
> The last command still shows "root invalid (Parent unable to distribute
> cpu downstream)".
>
> In this case compute_excpus() returns the stale user mask unchanged.
> user_xcpus() is gone from update_parent_effective_cpumask(), but the
> dependency just moved one level down: the new code calls
> compute_excpus(cs, tmp->delmask), and its first line is
>
>    cpumask_and(excpus, user_xcpus(cs), parent->effective_xcpus)
>
> Here user_xcpus(cs) is cs->exclusive_cpus, which still holds the old
> 0-7 when compute_excpus() runs: update_exclusive_cpumask() publishes
> cs->exclusive_cpus only after partition_cpus_change(). Nothing corrects
> it either - the sibling trim is skipped as exclusive_cpus is non-empty,
> and for a direct child of the root cpuset the intersection with
> parent->effective_xcpus is a no-op. So the stale 0-7 mask still reaches
> tasks_nocpu_error() and PERR_NOCPUS is set again. Please point it out
> if I got the analysis wrong.
>
> I have rebased my fix on top of this series and the partition recovers
> correctly with it applied. Should the fix be folded into this patch,
> or is there a better way to fix it? Happy to hear your thoughts.

You are right. My patch series won't fix this particular issue. However, 
converting an invalid partition back to a valid one is done on a best 
effort basis. There is no guarantee that it will always happen. Just 
echo "root" to "cpuset.cpus.partition" again will make it valid again. 
So it is not a real bug from my point of view. It is just a nice to have 
feature.

Cheers,
Longman

> Also, is the test case below worth adding here as well? Note that it
> fails until the fix is in:
>
>      # An invalid local partition that took all the CPUs from its
>      # populated parent should recover after its cpuset.cpus.exclusive
>      # list is changed to a valid one.
>      "C0-7:X0-7:P1  .      .      .     X1      .      .      .     0 A1:1|XA1:1 A1:P1 ."
>
> Thanks,
> Guopeng
>
>> Some test case are added to test_cpuset_prs.sh for invalidating a
>> partition root due to its effective xcpus becoming empty as well as
>> making sure that an invalid partition can only be revived when exclusive
>> CPUs are available agin.
>>
>> Fixes: e2ffe502ba45 ("cgroup/cpuset: Add cpuset.cpus.exclusive for v2")
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>   kernel/cgroup/cpuset.c                        | 36 +++++++++----------
>>   .../selftests/cgroup/test_cpuset_prs.sh       |  6 ++++
>>   2 files changed, 22 insertions(+), 20 deletions(-)
>>
>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>> index 7ead8c3b7974..db6e4663f4be 100644
>> --- a/kernel/cgroup/cpuset.c
>> +++ b/kernel/cgroup/cpuset.c
>> @@ -1798,7 +1798,6 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>   	int deleting;	/* Deleting cpus from parent's effective_cpus	*/
>>   	int old_prs, new_prs;
>>   	int part_error = PERR_NONE;	/* Partition error? */
>> -	struct cpumask *xcpus = user_xcpus(cs);
>>   	int parent_prs = parent->partition_root_state;
>>   	bool nocpu;
>>   
>> @@ -1841,30 +1840,29 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>   	if (!newmask && xcpus_empty(cs))
>>   		return PERR_CPUSEMPTY;
>>   
>> -	nocpu = tasks_nocpu_error(parent, cs, xcpus);
>> +	/*
>> +	 * Except for partcmd_disable, use delmask to compute the current
>> +	 * effective xcpus value and use it for tasks_nocpu_error check.
>> +	 */
>> +	if (cmd != partcmd_disable) {
>> +		compute_excpus(cs, tmp->delmask);
>> +		nocpu = tasks_nocpu_error(parent, cs, tmp->delmask);
>> +	}
>>   
>>   	if ((cmd == partcmd_enable) || (cmd == partcmd_enablei)) {
>> -		/*
>> -		 * Need to call compute_excpus() in case
>> -		 * exclusive_cpus not set. Sibling conflict should only happen
>> -		 * if exclusive_cpus isn't set.
>> -		 */
>> -		xcpus = tmp->delmask;
>> -		if (compute_excpus(cs, xcpus))
>> -			WARN_ON_ONCE(!cpumask_empty(cs->exclusive_cpus));
>>   		new_prs = (cmd == partcmd_enable) ? PRS_ROOT : PRS_ISOLATED;
>>   
>>   		/*
>>   		 * Enabling partition root is not allowed if its
>>   		 * effective_xcpus is empty.
>>   		 */
>> -		if (cpumask_empty(xcpus))
>> +		if (cpumask_empty(tmp->delmask))
>>   			return PERR_INVCPUS;
>>   
>> -		if (prstate_housekeeping_conflict(new_prs, parent_prs, xcpus, NULL))
>> +		if (prstate_housekeeping_conflict(new_prs, parent_prs, tmp->delmask, NULL))
>>   			return PERR_HKEEPING;
>>   
>> -		if (tasks_nocpu_error(parent, cs, xcpus))
>> +		if (nocpu)
>>   			return PERR_NOCPUS;
>>   
>>   		/*
>> @@ -1876,9 +1874,8 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>   		 *
>>   		 * Warn if it is not the case.
>>   		 */
>> -		cpumask_and(tmp->new_cpus, xcpus, cpu_active_mask);
>> +		cpumask_and(tmp->new_cpus, tmp->delmask, cpu_active_mask);
>>   		WARN_ON_ONCE(!cpumask_subset(tmp->new_cpus, parent->effective_cpus));
>> -
>>   		deleting = true;
>>   	} else if (cmd == partcmd_disable) {
>>   		/*
>> @@ -1973,7 +1970,7 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>   		 * A partition error happens when parent has tasks and all
>>   		 * its effective CPUs will have to be distributed out.
>>   		 */
>> -		if (nocpu) {
>> +		if (nocpu || cpumask_empty(tmp->delmask)) {
>>   			part_error = PERR_NOCPUS;
>>   			if (is_partition_valid(cs))
>>   				adding = cpumask_and(tmp->addmask,
>> @@ -1986,8 +1983,7 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>   				cpumask_copy(tmp->addmask, cs->effective_xcpus);
>>   				adding = true;
>>   			}
>> -		} else if (is_partition_invalid(cs) && !cpumask_empty(xcpus) &&
>> -			   cpumask_subset(xcpus, parent->effective_xcpus)) {
>> +		} else if (is_partition_invalid(cs)) {
>>   			struct cgroup_subsys_state *css;
>>   			struct cpuset *child;
>>   			bool exclusive = true;
>> @@ -1998,7 +1994,7 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>   			 * check.
>>   			 */
>>   			if (prstate_housekeeping_conflict(cs->partition_root_state,
>> -							  parent_prs, xcpus, NULL)) {
>> +							  parent_prs, tmp->delmask, NULL)) {
>>   				part_error = PERR_HKEEPING;
>>   				goto write_error;
>>   			}
>> @@ -2014,7 +2010,7 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>   			rcu_read_unlock();
>>   			if (exclusive)
>>   				deleting = cpumask_and(tmp->delmask,
>> -						xcpus, parent->effective_cpus);
>> +						tmp->delmask, parent->effective_cpus);
>>   			else
>>   				part_error = PERR_NOTEXCL;
>>   		}
>> diff --git a/tools/testing/selftests/cgroup/test_cpuset_prs.sh b/tools/testing/selftests/cgroup/test_cpuset_prs.sh
>> index d04aad697b96..47791591002f 100755
>> --- a/tools/testing/selftests/cgroup/test_cpuset_prs.sh
>> +++ b/tools/testing/selftests/cgroup/test_cpuset_prs.sh
>> @@ -359,6 +359,8 @@ TEST_MATRIX=(
>>   								       A1:P0|A2:P1|A3:P2|B1:P1 2-4|3"
>>   	"  C1-6:P1 C3-6:P1 C5-6:P1 .      .      P2      .      .    0 A1:1-2|A2:3-6|A3:5-6 \
>>   								       A1:P1|A2:P2|A3:P-1 3-6"
>> +	"  C1-6:P2 C3-6:P2 C3-4:P2 .      .     C5-6     .      .    0 A1:1-4|A2:5-6|A3:5-6 \
>> +								       A1:P2|A2:P2|A3:P-2 1-6"
>>   	" C1-4:X2-4 C2-4:X3-4:P1 C3-4:X4:P2 \
>>   				   .      .      X5      .      .    0 A1:1-4|A2:2-4|A3:3-4 \
>>   								       A1:P0|A2:P-1|A3:P-2 ."
>> @@ -427,6 +429,10 @@ TEST_MATRIX=(
>>   	# cpuset.cpus is allowed, but those extra CPUs are ignored.
>>   	"  C2-3:P1   C3:P1  .      .      .     C2-4    .      .     0 A1:|A2:2-3 A1:P1|A2:P1"
>>   
>> +	# A child cannot recover until its exclusive CPUs are available again.
>> +	" CX1-4:P1 CX3-4:P1 .     .     X1-2     .      .      .     0 A1:1-2|A2:1-2|XA2: A1:P1|A2:P-1"
>> +	" CX1-4:P1 CX3-4:P1 .     .  X1-2:X1-4   .      .      .     0 A1:1-2|A2:3-4|XA2:3-4 A1:P1|A2:P1"
>> +
>>   	# Taking away all CPUs from parent or itself if there are tasks
>>   	# will make the partition invalid.
>>   	"  C2-3:P1   C3:P1  .      .      T     C2-3    .      .     0 A1:2-3|A2:2-3 A1:P1|A2:P-1"