在 2026/9/4 09:55, Ridong Chen 写道:
>
>
> 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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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.
>
Thanks, Ridong. I see what you mean about the name.
My intention with "child" was to describe the role of the partition in these
checks. Both callers use the helper while walking a parent's child partitions,
and the result depends on the parent's complete exclusive mask, whether the
parent is populated, and the active CPUs remaining at that point in the walk.
The helper also checks only the two conditions that can invalidate a child
during this process, rather than performing general validation of an arbitrary
cpuset partition.
For that reason, `cs_partition_error()` might suggest a broader scope than the
helper actually has. Would it make sense to keep `child_partition_error()` and
clarify the comment, for example:
/*
* Return the error that would invalidate @child under its parent
* partition's CPU configuration.
*/
I am also open to another name that preserves this parent-child context.
> 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
> }
>
> ```
>
That sounds like a good idea.
Since these validation paths have different inputs and side effects,
consolidating them may require a broader review and restructuring.
I will look into this separately after the current series is completed.
Thanks,
Guopeng
>
>>>> +{
>>>> + 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;
>>>> /*
>>>
>>
>
