Re: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation

[Guopeng Zhang]

在 2026/9/8 09:40, Ridong Chen 写道:
> 
> 
> On 9/7/2026 5:49 PM, Guopeng Zhang wrote:
>>
>>
>> 在 2026/9/4 09:25, Ridong Chen 写道:
>>>
>>>
>>> On 9/4/2026 2:33 AM, Waiman Long wrote:
>>>> On 9/2/26 6:26 AM, Guopeng Zhang wrote:
>>>>> From: Guopeng Zhang <zhangguopeng@xxxxxxxxxx>
>>>>>
>>>>> compute_partition_effective_cpumask() checks whether each valid child
>>>>> partition remains covered by the parent exclusive CPU mask and whether it
>>>>> would consume all remaining CPUs of a populated parent.
>>>>>
>>>>> Factor these two checks into child_partition_error() so the same rules can
>>>>> be reused when evaluating a proposed parent configuration. This is a
>>>>> preparatory refactoring with no intended functional change.
>>>>>
>>>>> Signed-off-by: Guopeng Zhang <zhangguopeng@xxxxxxxxxx>
>>>>> ---
>>>>>    kernel/cgroup/cpuset.c | 38 +++++++++++++++++++++++++++++---------
>>>>>    1 file changed, 29 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>>>>> index 8f24171b6055..6994dc75d940 100644
>>>>> --- a/kernel/cgroup/cpuset.c
>>>>> +++ b/kernel/cgroup/cpuset.c
>>>>> @@ -2085,6 +2085,26 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,
>>>>>        return 0;
>>>>>    }
>>>>> +/*
>>>>> + * Return the error that will invalidate a child partition under a proposed
>>>>> + * parent partition configuration.
>>>>> + */
>>>>> +static enum prs_errcode
>>>>> +child_partition_error(struct cpuset *child,
>>>>> +              const struct cpumask *partition_cpus,
>>>>> +              const struct cpumask *remaining_cpus,
>>>>> +              bool parent_populated)
>>>> I think you should add some functional comments on what "partition_cpus" and "remaining_cpus" are supposed to be so that caller knows what to pass into this helper.
>>>
>>> Would it help to rename them to excpus and local_excpus?
>>> local already implies "excluding children" (just like cgroup.stat.local), so I think that makes the intent clearer.
>>>
>>
>> Ridong, thanks for your review.
>>
>> Renaming the masks would help, but I think `local_excpus` may be slightly misleading here. When the helper is called, the mask may still contain active CPUs assigned to the current child and to children that have not yet been visited. It excludes only the CPUs of previously visited children that remain valid under the configuration being evaluated, and the mask itself is restricted to active CPUs.
>>
>> In addition, "local" already has the local-versus-remote partition meaning in cpuset.
>>
>> Would `parent_xcpus` for the complete effective exclusive mask, which may include offline CPUs, and `remaining_ecpus` for the remaining active CPUs before evaluating the current child be clearer? I will also document both parameters explicitly.
>>
> 
> Passing struct cpuset *parent into the function simplifies naming, as the needed values become local variables derived from it.
> 
Thanks, Ridong.

I agree that passing `parent` would make the interface simpler, but I don't think `parent` alone provides all the state needed by this helper.

In `compute_partition_effective_cpumask()`, the remaining active CPU mask is temporary state maintained during the child walk:

compute_excpus(cs, new_ecpus);
cpumask_and(new_ecpus, new_ecpus, cpu_active_mask);

cpuset_for_each_child(child, css, cs) {
	...

	if (child_err)
		continue;

	cpumask_andnot(new_ecpus, new_ecpus,
		       child->effective_xcpus);
}

So `new_ecpus` changes as valid children are processed and cannot be replaced by `parent->effective_cpus`. In this update path, `parent->effective_cpus` still contains the previously published effective mask, while `new_ecpus` already represents the active CPUs remaining before the current child is evaluated.

For example, assume CPUs 1-3 are active, the parent is populated, and child A is visited before child B. Initially:

parent:
        effective_xcpus = 1-3
        effective_cpus  = 3

child A:
        effective_xcpus = 1

child B:
        effective_xcpus = 2

If the parent's effective exclusive mask is changed to `1-2`, this update path publishes the new `effective_xcpus` before recomputing `effective_cpus`. Therefore, when entering `compute_partition_effective_cpumask()`:

parent->effective_xcpus = 1-2
parent->effective_cpus  = 3
new_ecpus               = 1-2

After child A remains valid, its CPU is removed from `new_ecpus`:

new_ecpus = 2

So when child B is evaluated, the `PERR_NOCPUS` check uses:

cpumask_subset(new_ecpus, child_B->effective_xcpus)
= cpumask_subset(2, 2)
= true

Since the parent is populated, the `PERR_NOCPUS` condition is met for child B.

At the same point:

remaining mask needed by the check = 2
parent->effective_cpus             = 3

Using `parent->effective_cpus` instead would give:

cpumask_subset(parent->effective_cpus,
               child_B->effective_xcpus)
= cpumask_subset(3, 2)
= false

and would miss the condition.

So unless `cs_partition_error()` also takes over or reconstructs the relevant child-walk state, passing only `parent` cannot provide the remaining active CPU mask needed to evaluate the current child.

The next patch also reuses this helper for a trial parent configuration. In that case, the complete mask being evaluated comes from `trialcs->effective_xcpus`, rather than from the currently published state in the real parent. The follow-up child-invalidation fix similarly needs to distinguish the complete effective exclusive mask from the remaining active CPU mask.

So for now, I will use the more general `cs_partition_error()` name, but keep the two masks explicit in the interface.

In the next version, I plan to include the remaining patches related to this helper and the associated invalidation paths so that all the call sites and use cases can be reviewed together.

I may still be thinking within the constraints of the current code structure, so if you see a cleaner interface once the full picture is visible, I would very much appreciate your suggestion.

Thanks,
Guopeng