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

[Ridong Chen <ridong.chen@linux.dev>]

On 9/4/2026 9:25 AM, Ridong Chen wrote:
> 
> 
> On 9/4/2026 2:33 AM, Waiman Long wrote:
>> On 9/2/26 6:26 AM, Guopeng Zhang wrote:
>>> From: Guopeng Zhang <zhangguopeng@kylinos.cn>
>>>
>>> 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@kylinos.cn>
>>> ---
>>>   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.
> 

Regarding the naming: child_partition_error is a bit odd — it sounds like it's 
validating a hierarchical child partition, but it's actually validating the 
partition itself (the child argument). The parent is only needed as context for 
the validation logic, not because we're checking a subordinate partition.

I'd suggest renaming it to something like:

```
static enum prs_errcode cs_partition_error(struct cpuset *cs, ...)

```

That would better reflect what the function actually does.

On a related note, the current partition validation logic is scattered across 
the code. I think we should consider consolidating it into a common set of 
helpers, perhaps like:


```
static enum prs_errcode validate_partition(struct cpuset *cs, struct cpuset 
*trialcs, struct cpuset *parent)
{
...
}

static enum prs_errcode validate_local_partition(struct cpuset *cs, struct 
cpuset *trialcs, struct cpuset *parent)
{
	// local partion specific check
	// call validate_partition
	
}

static enum prs_errcode validate_remote_partition(struct cpuset *cs, struct 
cpuset *trialcs, struct cpuset *parent)
{
	// remote partion specific check
	// call validate_partition
}

```


>>> +{
>>> +    if (!cpumask_subset(child->effective_xcpus, partition_cpus))
>>> +        return PERR_INVCPUS;
>>> +
>>> +    if (parent_populated &&
>>> +        cpumask_subset(remaining_cpus, child->effective_xcpus))
>>> +        return PERR_NOCPUS;
>>> +
>>> +    return PERR_NONE;
>>> +}
>>> +
>>>   /**
>>>    * compute_partition_effective_cpumask - compute effective_cpus for partition
>>>    * @cs: partition root cpuset
>>> @@ -2121,6 +2141,8 @@ static void compute_partition_effective_cpumask(struct 
>>> cpuset *cs,
>>>       rcu_read_lock();
>>>       cpuset_for_each_child(child, css, cs) {
>>> +        enum prs_errcode child_err;
>>> +
>>>           if (!is_partition_valid(child))
>>>               continue;
>>> @@ -2129,15 +2151,13 @@ static void 
>>> compute_partition_effective_cpumask(struct cpuset *cs,
>>>            * partition root.
>>>            */
>>>           WARN_ON_ONCE(is_remote_partition(child));
>>> -        WRITE_ONCE(child->prs_err, 0);
>>> -        if (!cpumask_subset(child->effective_xcpus,
>>> -                    cs->effective_xcpus))
>>> -            WRITE_ONCE(child->prs_err, PERR_INVCPUS);
>>> -        else if (populated &&
>>> -             cpumask_subset(new_ecpus, child->effective_xcpus))
>>> -            WRITE_ONCE(child->prs_err, PERR_NOCPUS);
>>> -
>>> -        if (child->prs_err) {
>>> +        WRITE_ONCE(child->prs_err, PERR_NONE);
>>> +        child_err = child_partition_error(child, cs->effective_xcpus,
>>> +                          new_ecpus, populated);
>>> +        if (child_err)
>>> +            WRITE_ONCE(child->prs_err, child_err);
>>> +
>>
>> You can ignore the inital PERR_NONE write and always write the child_err value 
>> into child->prs_err.
>>
>> Not big issue, just some nits.
>>
>> Cheers,
>> Longman
>>
>>> +        if (child_err) {
>>>               int old_prs = child->partition_root_state;
>>>               /*
>>
> 

-- 
Best regards
Ridong