Re: [PATCH] cgroup/cpuset: Fix missing effective_xcpus accounting of child partitions

[Guopeng Zhang <guopeng.zhang@linux.dev>]

在 2026/10/27 21:44, Guopeng Zhang 写道:
> 
> 
> 在 2026/10/27 20:38, Guopeng Zhang 写道:
>> From: Guopeng Zhang <zhangguopeng@kylinos.cn>
>>
>> When a partition root expands its exclusive CPUs, a valid child
>> partition will be able to claim more CPUs from its parent. As
>> update_cpumasks_hier() always updates a cpuset before its descendants,
>> the parent has already been committed when the child partition is
>> reached. Its effective_cpus is computed with the child's old
>> effective_xcpus subtracted and no further adjustment is made
>> after the child commits its new effective_xcpus.
>>
>> The additional CPUs are thus granted to both the parent and the child
>> partitions. generate_sched_domains() then emits the warning below
>> about the overlapping sched domains:
>>
>>   WARNING: kernel/cgroup/cpuset.c:919 at rebuild_sched_domains_locked+0x1ec/0x2e0, CPU#21: bash/1225
>>
>> Fix that by taking the CPUs newly added to a valid child partition
>> away from its parent at the commit point of update_cpumasks_hier()
>> right before the new effective_xcpus is committed. The tasks in the
>> parent cpuset as well as its other children will be updated if its
>> effective_cpus is changed.
>>
>> A new test case is added to test_cpuset_prs.sh to test this patch.
>>
>> Fixes: e2ffe502ba45 ("cgroup/cpuset: Add cpuset.cpus.exclusive for v2")
>> Signed-off-by: Guopeng Zhang <zhangguopeng@kylinos.cn>
>> ---
>>  kernel/cgroup/cpuset.c                        | 25 +++++++++++++++++++
>>  .../selftests/cgroup/test_cpuset_prs.sh       |  4 +++
>>  2 files changed, 29 insertions(+)
>>
>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>> index 3219f927c35e..ec0ef3ad0726 100644
>> --- a/kernel/cgroup/cpuset.c
>> +++ b/kernel/cgroup/cpuset.c
>> @@ -2241,6 +2241,7 @@ static void update_cpumasks_hier(struct cpuset *cs, struct tmpmasks *tmp,
>>  		struct cpuset *parent = parent_cs(cp);
>>  		bool remote = is_remote_partition(cp);
>>  		bool update_parent = false;
>> +		bool xcpus_added = false;
>>  
>>  		old_prs = new_prs = cp->partition_root_state;
>>  
>> @@ -2356,6 +2357,19 @@ static void update_cpumasks_hier(struct cpuset *cs, struct tmpmasks *tmp,
>>  		}
>>  
>>  		spin_lock_irq(&callback_lock);
>> +		/*
>> +		 * A child partition may claim more CPUs when its parent expands
>> +		 * the exclusive CPUs. Take the newly granted CPUs away from the
>> +		 * parent which was committed earlier in this walk.
>> +		 */
>> +		if (!remote && new_prs > 0) {
>> +			compute_excpus(cp, tmp->addmask);
>> +			if (cpumask_andnot(tmp->delmask, tmp->addmask,
>> +					   cp->effective_xcpus)) {
>> +				partition_xcpus_add(new_prs, parent, tmp->delmask);
>> +				xcpus_added = true;
>> +			}
>> +		}
>>  		cpumask_copy(cp->effective_cpus, tmp->new_cpus);
>>  		cp->partition_root_state = new_prs;
>>  		/*
>> @@ -2375,6 +2389,17 @@ static void update_cpumasks_hier(struct cpuset *cs, struct tmpmasks *tmp,
>>  
>>  		cpuset_update_tasks_cpumask(cp, tmp->new_cpus);
>>  
>> +		/*
>> +		 * The parent's effective_cpus may have been changed by
>> +		 * partition_xcpus_add() above. Update the tasks in the parent
>> +		 * cpuset and its other children, if necessary.
>> +		 */
>> +		if (xcpus_added) {
>> +			cpumask_copy(tmp->addmask, parent->effective_cpus);
>> +			cpuset_update_tasks_cpumask(parent, tmp->addmask);
>> +			update_sibling_cpumasks(parent, cp, tmp);
>> +		}
>> +
>>  		/*
>>  		 * On default hierarchy, inherit the CS_SCHED_LOAD_BALANCE
>>  		 * from parent if current cpuset isn't a valid partition root
>> diff --git a/tools/testing/selftests/cgroup/test_cpuset_prs.sh b/tools/testing/selftests/cgroup/test_cpuset_prs.sh
>> index 83ef6c781af3..09e430570815 100755
>> --- a/tools/testing/selftests/cgroup/test_cpuset_prs.sh
>> +++ b/tools/testing/selftests/cgroup/test_cpuset_prs.sh
>> @@ -437,6 +437,10 @@ TEST_MATRIX=(
>>  	" 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"
>>  
>> +	# The extra CPUs gained by a child partition when its parent expands the
>> +	# exclusive CPUs have to be taken away from the parent.
>> +	" C2-6:X2-5:P1 C3-6:X3-6:P1 .      .      X2-6     .      .      .     0 A1:2|A2:3-6|XA2:3-6 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"
> 
> Hi Longman,
> 
> I thought of another way to fix this problem that attacks the stale
> value at its source. The comment in
> update_parent_effective_cpumask() says:
> 
>     When both cs and parent remain valid, parent's
>     compute_partition_effective_cpumask() call would have removed cs's
>     effective_xcpus from the parent's effective_cpus. So there is no
>     need to adjust parent's effective_cpus here.
> 
> That is the design intent, but compute_partition_effective_cpumask()
> subtracts the child's old effective_xcpus, which hasn't been updated
> yet when the parent is processed. What if the parent subtracts the
> CPUs the child can claim from the newly computed parent mask instead?
> The rework (on top of the same base as the posted patch) looks like:
> 
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 3219f927c35e..cd02b6b74ca2 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -1602,10 +1602,14 @@ static int compute_trialcs_excpus(struct cpuset *trialcs, struct cpuset *cs)
>  static void compute_partition_effective_cpumask(struct cpuset *cs,
>  						struct cpumask *new_ecpus)
>  {
> +	static cpumask_t child_xcpus;
>  	struct cgroup_subsys_state *css;
>  	struct cpuset *child;
>  	bool populated = partition_is_populated(cs, NULL);
>  
> +	lockdep_assert_cpuset_lock_held();
> +
>  	/*
>  	 * Check child partition roots to see if they should be invalidated
>  	 * when all the effective_cpus will be used up and cs is populated.
> @@ -1628,7 +1632,16 @@ static void compute_partition_effective_cpumask(struct cpuset *cs,
>  		 * partition root.
>  		 */
>  		WARN_ON_ONCE(is_remote_partition(child));
> -		if (populated && cpumask_subset(new_ecpus, child->effective_xcpus)) {
> +		/*
> +		 * The child has not been updated yet. Compute the CPUs it
> +		 * can take from this newly computed parent mask, without
> +		 * overwriting its old effective_xcpus.
> +		 */
> +		cpumask_and(&child_xcpus, new_ecpus, user_xcpus(child));
> +		if (cpumask_empty(child->exclusive_cpus))
> +			rm_siblings_excl_cpus(cs, child, &child_xcpus);
> +
> +		if (populated && cpumask_subset(new_ecpus, &child_xcpus)) {
>  			int old_prs = child->partition_root_state;
>  			int parent_prs = cs->partition_root_state;
>  
> @@ -1646,8 +1659,7 @@ static void compute_partition_effective_cpumask(struct cpuset *cs,
>  			notify_partition_change(child, old_prs);
>  			continue;
>  		}
> -		cpumask_andnot(new_ecpus, new_ecpus,
> -			       child->effective_xcpus);
> +		cpumask_andnot(new_ecpus, new_ecpus, &child_xcpus);
>  	}
>  	rcu_read_unlock();
>  }
> 
> compute_excpus() is not directly reused here as it reads
> parent->effective_xcpus which hasn't been updated yet at this point.
> 
> This fixes the double grant in one step and keeps the accounting
> inside compute_partition_effective_cpumask(). I boot tested it on a
> 32-CPU VM with the whole test_cpuset_prs.sh suite and it passes, same
> as the posted patch.
> 

Sorry, the rework I suggested has a problem. I kept testing it and
found that it fixes the effective_cpus overlap, but misses the
isolated_cpus accounting.

Test setup (CPUs 2-6, A is a root partition, B is an isolated child):

    A:  cpuset.cpus = 2-6, cpuset.cpus.exclusive = 2-5
    B:  cpuset.cpus = 3-6, cpuset.cpus.exclusive = 3-6 (clamped to 3-5)

    echo 2-6 > A/cpuset.cpus.exclusive

Expected after the expansion: B holds 3-6 and cpuset.cpus.isolated
becomes 3-6.

    current code:   B = 3-6, isolated = 3-5, A effective = 2,6
    rework:         B = 3-6, isolated = 3-5, A effective = 2
    posted patch:   B = 3-6, isolated = 3-6, A effective = 2

The rework subtracts the child's claim from the parent's effective_cpus
in one step, but the accounting for a growing child partition isn't
only about the parent's effective_cpus. partition_xcpus_add() also
updates subpartitions_cpus and isolated_cpus, e.g. a CPU newly gained
by an isolated child has to be added to isolated_cpus.

Note that the isolated_cpus miss above exists in the current code too:
an isolated child that gains CPUs from a parent expansion updates its
effective_xcpus, but isolated_cpus is never updated. The posted patch
fixes that as well.

Sorry again for the noise.

Thanks,
Guopeng