cgroup/cpuset: Fix partition type transitions
Subsystems (3)
v3 cover cgroup/cpuset: Fix partition type transitions
Several partition type-change and validation paths use effective_xcpus, which also includes CPUs assigned to valid child partitions. As a result, changing a parent between root and isolated can alter a child's isolation accounting or fail housekeeping validation because of CPUs the parent does not own.
A proposed parent CPU mask needs different treatment for children which would become invalid under that mask. Their CPUs return to the parent and must participate in housekeeping validation. Otherwise a parent can pass validation with a boot-isolated CPU hidden behind a child which is about to become invalid.
There are two related failure paths. Returning the last housekeeping CPU from a root child to an isolated parent must invalidate the appropriate isolated ancestor. Also, a root/isolated type change which fails housekeeping validation must release the partition's CPUs instead of leaving them unavailable to the owner of the invalidated subtree.
Factor out the child partition validity checks and reuse them when computing the CPUs owned directly by a partition under a proposed configuration. Use the directly owned CPUs for root/isolated type-change accounting and housekeeping validation, handle the last-housekeeping-CPU return case, and run the normal partition-disable path after a failed type-change validation. Add focused selftests for each case.
This is based on cgroup/for-7.3-fixes (3f4b7d1a49c5 "selftests/cgroup:
test clone3() into a previously killed cgroup").
Testing:
- Ran tools/testing/selftests/cgroup/test_cpuset_prs.sh with isolcpus=domain,15; all applicable tests passed.
- Ran the housekeeping CPU-return tests with nohz_full=1-14 isolcpus=domain,15; both cases passed.
Changes in v3:
- factor the child partition validity rules into a preparatory patch and reuse them for trial ownership calculation;
- during trial validation, subtract only children which remain valid under the proposed parent CPU mask, covering both PERR_INVCPUS and PERR_NOCPUS;
- keep cs->effective_xcpus as the default isolation-accounting mask and use the directly owned mask only for a successful root/isolated type change;
- use an isolated child for the boot-isolated trial-validation tests, as a root child containing that CPU fails immediately with PERR_HKEEPING;
- make the isolated-ancestor walk safe when it reaches the top cpuset and clarify the member-transition handling;
- clean up nested housekeeping-return test cgroups on failure.
Changes in v2:
- split the type-transition fixes from the original series;
- merge the child-owned accounting and validation changes;
- validate trial CPU masks against CPUs owned directly by the partition;
- handle a root child returning the last housekeeping CPU, including nested isolated ancestors;
- use the common partition-disable path after type-change validation failure;
- initialize boot-isolated CPU data during selftest setup;
- split the accounting, child-owned validation and housekeeping-return tests.
Guopeng Zhang (7):
cgroup/cpuset: Factor out child partition validation cgroup/cpuset: Account for child CPU ownership in partition changes selftests/cgroup: Add tests for type-change isolation accounting selftests/cgroup: Test child CPU ownership in partition changes selftests/cgroup: Add tests for housekeeping CPU return to isolated
parents
cgroup/cpuset: Release CPUs when type-change validation fails selftests/cgroup: Add CPU release tests for type-change validation
failures
kernel/cgroup/cpuset.c | 182 +++++++- .../selftests/cgroup/test_cpuset_prs.sh | 430 +++++++++++++++++- 2 files changed, 588 insertions(+), 24 deletions(-)
base-commit: 3f4b7d1a49c5c826f3be9b684313eea5b83ac232
Signature · 1 line
2.43.0
PATCH v3 1/7 cgroup/cpuset: Factor out child partition validation
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.
--- kernel/cgroup/cpuset.c | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-)
kernel/cgroup/cpuset.c +29 -9
@@ -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) +{ +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); + +if (child_err) {int old_prs = child->partition_root_state;/*
Signature · 1 line
2.43.0
Waiman Long
maintainerRe: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
Quoted 22 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +0
-0@@ -2085,6 +2085,26 @@ static int update_parent_effective_cpumask(struct cpuset *cs, int cmd,return 0 ; }
kernel/cgroup/cpuset.c static int update_parent_effective_cpumask(struct cpuset *cs, int cmd, · line 2096
Quoted 10 lines
kernel/cgroup/cpuset.c +9
-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.
Quoted 32 lines
kernel/cgroup/cpuset.c +13
-5+{ +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 &&
kernel/cgroup/cpuset.c static void compute_partition_effective_cpumask(struct cpuset *cs, · line 2159
Quoted 10 lines
kernel/cgroup/cpuset.c +6
-4- 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
Quoted 4 lines
kernel/cgroup/cpuset.c +1
-0+if (child_err) {int old_prs = child->partition_root_state;/*
Ridong Chen
reviewerRe: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
Quoted 26 lines · Waiman Long
Quoted 25 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +2
-0@@ -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
Quoted 8 lines
kernel/cgroup/cpuset.c static int update_parent_effective_cpumask(struct cpuset *cs, int cmd, · line 2096
Quoted 7 lines
kernel/cgroup/cpuset.c +7
-0+* 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.
Quoted 54 lines
Quoted 32 lines
kernel/cgroup/cpuset.c +13
-5+{ + 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(structcpuset *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(structcpuset *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 &&kernel/cgroup/cpuset.c static void compute_partition_effective_cpumask(struct · line 2160
Quoted 10 lines
kernel/cgroup/cpuset.c +6
-4- 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_NONEwrite and always write thechild_errvalue intochild->prs_err.Not big issue, just some nits.
Cheers,
LongmanQuoted 3 lines
kernel/cgroup/cpuset.c +1
-0+ if (child_err) { int old_prs = child->partition_root_state; /*
Signature · 2 lines
Best regards Ridong
Ridong Chen
reviewerRe: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
Quoted 38 lines · Ridong Chen
Quoted 26 lines · Waiman Long
Quoted 25 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +2
-0@@ -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 Quoted 8 lines
kernel/cgroup/cpuset.c static int update_parent_effective_cpumask(struct cpuset *cs, int cmd, · line 2096
Quoted 7 lines
kernel/cgroup/cpuset.c +7
-0+* 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.
Quoted 3 lines
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
}
```Quoted 55 lines
Quoted 54 lines
Quoted 32 lines
kernel/cgroup/cpuset.c +13
-5+{ + 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(structcpuset *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 voidcompute_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 &&kernel/cgroup/cpuset.c static void · line 2160
Quoted 10 lines
kernel/cgroup/cpuset.c +6
-4- 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_NONEwrite and always write thechild_errvalue intochild->prs_err.Not big issue, just some nits.
Cheers,
LongmanQuoted 3 lines
kernel/cgroup/cpuset.c +1
-0+ if (child_err) { int old_prs = child->partition_root_state; /*
Signature · 2 lines
Best regards Ridong
Re: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
Quoted 46 lines · Ridong Chen
Quoted 37 lines · Ridong Chen
Quoted 25 lines · Waiman Long
Quoted 24 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +2
-0@@ -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 Quoted 8 lines
kernel/cgroup/cpuset.c static int update_parent_effective_cpumask(struct cpuset *cs, int cmd, · line 2096
Quoted 7 lines
kernel/cgroup/cpuset.c +7
-0+* 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.Quoted 3 lines
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_erroris 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.
Quoted 9 lines
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.
Quoted 16 lines
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 }
Quoted 8 lines
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
Quoted 55 lines
Quoted 54 lines
Quoted 53 lines
Quoted 31 lines
kernel/cgroup/cpuset.c +13
-5+{ + 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 &&kernel/cgroup/cpuset.c static void compute_partition_effective_cpumask(struct cpuset *cs, · line 2159
Quoted 10 lines
kernel/cgroup/cpuset.c +6
-4- 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_NONEwrite and always write thechild_errvalue intochild->prs_err.Not big issue, just some nits.
Cheers,
LongmanQuoted 3 lines
kernel/cgroup/cpuset.c +1
-0+ if (child_err) { int old_prs = child->partition_root_state; /*
Ridong Chen
reviewerRe: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
Quoted 56 lines · Guopeng Zhang
Quoted 53 lines · Ridong Chen
Quoted 37 lines · Ridong Chen
Quoted 25 lines · Waiman Long
Quoted 24 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +2
-0@@ -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 Quoted 8 lines
kernel/cgroup/cpuset.c static int update_parent_effective_cpumask(struct cpuset *cs, int cmd, · line 2096
Quoted 7 lines
kernel/cgroup/cpuset.c +7
-0+* 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.Quoted 3 lines
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_erroris 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, ...) ```
Quoted 9 lines
Quoted 2 lines
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 keepchild_partition_error()and clarify the comment, for example:
As you can see, we are continuing to add partition checks, so I'd still suggest avoiding child_partition_error(), it would become confusing when we later refactor the partition validation logic. All of these functions are ultimately used to validate a partition, and the validation may depend on either the parent or a trialcs, or something else.
So I'd propose keeping it as:
static enum prs_errcode cs_partition_error(struct cpuset *cs, struct cpuset *parent);
This is more general and better accommodates future extensions.
Quoted 100 lines
/*
* 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.
Quoted 16 lines
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 }Quoted 8 lines
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,
GuopengQuoted 55 lines
Quoted 54 lines
Quoted 53 lines
Quoted 31 lines
kernel/cgroup/cpuset.c +13
-5+{ + 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 &&kernel/cgroup/cpuset.c static void compute_partition_effective_cpumask(struct cpuset *cs, · line 2159
Quoted 10 lines
kernel/cgroup/cpuset.c +6
-4- 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_NONEwrite and always write thechild_errvalue intochild->prs_err.Not big issue, just some nits.
Cheers,
LongmanQuoted 3 lines
kernel/cgroup/cpuset.c +1
-0+ if (child_err) { int old_prs = child->partition_root_state; /*
Signature · 2 lines
Best regards Ridong
Re: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
Quoted 69 lines · Ridong Chen
Quoted 56 lines · Guopeng Zhang
Quoted 53 lines · Ridong Chen
Quoted 37 lines · Ridong Chen
Quoted 25 lines · Waiman Long
Quoted 24 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +2
-0@@ -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 Quoted 8 lines
kernel/cgroup/cpuset.c static int update_parent_effective_cpumask(struct cpuset *cs, int cmd, · line 2096
Quoted 7 lines
kernel/cgroup/cpuset.c +7
-0+* 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.Quoted 3 lines
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_erroris 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, ...) ```Quoted 9 lines
Quoted 2 lines
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 keepchild_partition_error()and clarify the comment, for example:
Quoted 8 lines
As you can see, we are continuing to add partition checks, so I'd still suggest avoiding
child_partition_error(), it would become confusing when we later refactor the partition validation logic. All of these functions are ultimately used to validate a partition, and the validation may depend on either the parent or a trialcs, or something else.So I'd propose keeping it as:
static enum
prs_errcodecs_partition_error(struct cpuset *cs, struct cpuset *parent);This is more general and better accommodates future extensions.
I see your point now. You are looking at this from a longer-term design perspective. I will update it in the next version.
Thanks,
Guopeng
Ridong Chen
reviewerRe: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
In reply to Ridong ChenQuoted 38 lines · Ridong Chen
Quoted 26 lines · Waiman Long
Quoted 25 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +2
-0@@ -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 Quoted 8 lines
kernel/cgroup/cpuset.c static int update_parent_effective_cpumask(struct cpuset *cs, int cmd, · line 2096
Quoted 7 lines
kernel/cgroup/cpuset.c +7
-0+* 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.
Quoted 3 lines
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
}
```Quoted 55 lines
Quoted 54 lines
Quoted 32 lines
kernel/cgroup/cpuset.c +13
-5+{ + 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(structcpuset *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 voidcompute_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 &&kernel/cgroup/cpuset.c static void · line 2160
Quoted 10 lines
kernel/cgroup/cpuset.c +6
-4- 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_NONEwrite and always write thechild_errvalue intochild->prs_err.Not big issue, just some nits.
Cheers,
LongmanQuoted 3 lines
kernel/cgroup/cpuset.c +1
-0+ if (child_err) { int old_prs = child->partition_root_state; /*
Signature · 2 lines
Best regards Ridong
Ridong Chen
reviewerRe: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
In reply to Ridong ChenQuoted 38 lines · Ridong Chen
Quoted 26 lines · Waiman Long
Quoted 25 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +2
-0@@ -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 Quoted 8 lines
kernel/cgroup/cpuset.c static int update_parent_effective_cpumask(struct cpuset *cs, int cmd, · line 2096
Quoted 7 lines
kernel/cgroup/cpuset.c +7
-0+* 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.
Quoted 3 lines
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
}
```Quoted 55 lines
Quoted 54 lines
Quoted 32 lines
kernel/cgroup/cpuset.c +13
-5+{ + 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(structcpuset *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 voidcompute_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 &&kernel/cgroup/cpuset.c static void · line 2160
Quoted 10 lines
kernel/cgroup/cpuset.c +6
-4- 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_NONEwrite and always write thechild_errvalue intochild->prs_err.Not big issue, just some nits.
Cheers,
LongmanQuoted 3 lines
kernel/cgroup/cpuset.c +1
-0+ if (child_err) { int old_prs = child->partition_root_state; /*
Signature · 2 lines
Best regards Ridong
Re: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
In reply to Ridong ChenQuoted 37 lines · Ridong Chen
Quoted 25 lines · Waiman Long
Quoted 24 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +2
-0@@ -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 Quoted 8 lines
kernel/cgroup/cpuset.c static int update_parent_effective_cpumask(struct cpuset *cs, int cmd, · line 2096
Quoted 7 lines
kernel/cgroup/cpuset.c +7
-0+* 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.
Quoted 3 lines
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.
Thanks,
Guopeng
Quoted 53 lines
Quoted 52 lines
Quoted 30 lines
kernel/cgroup/cpuset.c +13
-5+{ + 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 &&kernel/cgroup/cpuset.c static void compute_partition_effective_cpumask(struct cpuset *cs, · line 2159
Quoted 10 lines
kernel/cgroup/cpuset.c +6
-4- 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_NONEwrite and always write thechild_errvalue intochild->prs_err.Not big issue, just some nits.
Cheers,
LongmanQuoted 3 lines
kernel/cgroup/cpuset.c +1
-0+ if (child_err) { int old_prs = child->partition_root_state; /*
Ridong Chen
reviewerRe: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
Quoted 44 lines · Guopeng Zhang
Quoted 37 lines · Ridong Chen
Quoted 25 lines · Waiman Long
Quoted 24 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +2
-0@@ -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 Quoted 8 lines
kernel/cgroup/cpuset.c static int update_parent_effective_cpumask(struct cpuset *cs, int cmd, · line 2096
Quoted 7 lines
kernel/cgroup/cpuset.c +7
-0+* 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.Quoted 3 lines
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.
Quoted 8 lines
Ridong, thanks for your review.
Renaming the masks would help, but I think
local_excpusmay 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_xcpusfor the complete effective exclusive mask, which may include offline CPUs, andremaining_ecpusfor 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.
Quoted 55 lines
Quoted 54 lines
Quoted 53 lines
Quoted 31 lines
kernel/cgroup/cpuset.c +13
-5+{ + 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 &&kernel/cgroup/cpuset.c static void compute_partition_effective_cpumask(struct cpuset *cs, · line 2159
Quoted 10 lines
kernel/cgroup/cpuset.c +6
-4- 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_NONEwrite and always write thechild_errvalue intochild->prs_err.Not big issue, just some nits.
Cheers,
LongmanQuoted 3 lines
kernel/cgroup/cpuset.c +1
-0+ if (child_err) { int old_prs = child->partition_root_state; /*
Signature · 2 lines
Best regards Ridong
Re: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
Quoted 49 lines · Ridong Chen
Quoted 46 lines · Guopeng Zhang
Quoted 37 lines · Ridong Chen
Quoted 25 lines · Waiman Long
Quoted 24 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +2
-0@@ -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 Quoted 8 lines
kernel/cgroup/cpuset.c static int update_parent_effective_cpumask(struct cpuset *cs, int cmd, · line 2096
Quoted 7 lines
kernel/cgroup/cpuset.c +7
-0+* 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.Quoted 3 lines
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.
Quoted 9 lines
Quoted 6 lines
Renaming the masks would help, but I think
local_excpusmay 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_xcpusfor the complete effective exclusive mask, which may include offline CPUs, andremaining_ecpusfor 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-2parent->effective_cpus = 3new_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 = 2parent->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
Re: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
In reply to Waiman LongQuoted 25 lines · Waiman Long
Quoted 24 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +0
-0@@ -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
Quoted 8 lines
kernel/cgroup/cpuset.c static int update_parent_effective_cpumask(struct cpuset *cs, int cmd, · line 2096
Quoted 7 lines
+ * parent partition configuration.
+ */kernel/cgroup/cpuset.c +5
-0+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.
Thanks for the suggestion. I will add comments in the next version.
Quoted 43 lines
Quoted 32 lines
kernel/cgroup/cpuset.c +11
-0+{ + 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- computeeffective_cpusfor partition
* @cs: partition root cpusetkernel/cgroup/cpuset.c +2
-0@@ -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);kernel/cgroup/cpuset.c static void compute_partition_effective_cpumask(struct cpuset *cs, · line 2159
Quoted 8 lines
kernel/cgroup/cpuset.c +6
-2- - 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_NONEwrite and always write thechild_errvalue intochild->prs_err.
Quoted 2 lines
Not big issue, just some nits.
Makes sense. I will update it.
Thanks,
Guopeng
Waiman Long
maintainerRe: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
In reply to Guopeng ZhangQuoted 56 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +22
-0@@ -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) +{ +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 ;
kernel/cgroup/cpuset.c static void compute_partition_effective_cpumask(struct cpuset *cs, · line 2134
Quoted 6 lines
kernel/cgroup/cpuset.c +0
-3@@ -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))
Looking at the patch again, I now see that the current code is incorrect. It should be "if (!cpumask_subset(child->effective_xcpus, new_ecpus))" So you only need to pass the "parent_excpus" (for instance) to the new helper instead of passing two.
Cheers,
Longman
Re: [PATCH v3 1/7] cgroup/cpuset: Factor out child partition validation
Quoted 54 lines · Waiman Long
Quoted 53 lines · Guopeng Zhang
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(-)kernel/cgroup/cpuset.c +0
-0@@ -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.
+ */kernel/cgroup/cpuset.c +16
-0+static enum prs_errcode +child_partition_error (struct cpuset *child, + const struct cpumask *partition_cpus, + const struct cpumask *remaining_cpus, + bool parent_populated) +{ + 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- computeeffective_cpusfor partition
* @cs: partition root cpusetkernel/cgroup/cpuset.c +2
-0@@ -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,
Quoted 9 lines
Quoted 6 lines
kernel/cgroup/cpuset.c +0
-0* partition root.*/ WARN_ON_ONCE(is_remote_partition(child)); -
WRITE_ONCE(child->prs_err, 0);
- if (!cpumask_subset(child->effective_xcpus, -cs->effective_xcpus))Looking at the patch again, I now see that the current code is incorrect. It should be "if (!cpumask_subset(child->effective_xcpus,
new_ecpus))" So you only need to pass the "parent_excpus" (for instance) to the new helper instead of passing two.
Longman, thanks for your review.
Yes, you are right that using cs->effective_xcpus here is incorrect.
In v1, I had a change for this issue, and based on your comment [1], I moved it to another fix. I also followed Ridong's suggestion to split the original 17-patch series into smaller series. I split it into three parts locally, and the current series is the second part, so this fix is not included here.
I plan to address this in the third series, together with the CPU ownership and isolation accounting issues following child partition invalidation.
In that series, the two masks have different meanings: the newly computed unfiltered exclusive mask, including offline CPUs, is used for the child containment check, while the active CPU mask remaining after child ownership is accounted for is used for the PERR_NOCPUS check. So I would prefer to keep the two-mask interface here and clarify their meanings with comments.
Does this approach make sense to you?
[1] https://lore.kernel.org/all/[email protected]/
Thanks,
Guopeng
PATCH v3 2/7 cgroup/cpuset: Account for child CPU ownership in partition changes
In reply to Guopeng ZhangFrom: Guopeng Zhang <[email protected]>
effective_xcpus includes CPUs granted to valid child partitions. A change to the parent must not apply its isolation state or housekeeping checks to CPUs which remain owned by those children.
For example, on a cgroup v2 system with CPUs 0-3 online:
cd /sys/fs/cgroup
echo +cpuset > cgroup.subtree_control
mkdir type-repro
echo 1-3 > type-repro/cpuset.cpus
echo isolated > type-repro/cpuset.cpus.partition echo +cpuset > type-repro/cgroup.subtree_control mkdir type-repro/child
echo 2-3 > type-repro/child/cpuset.cpus
echo isolated > type-repro/child/cpuset.cpus.partition echo root > type-repro/cpuset.cpus.partition cat cpuset.cpus.isolated
The isolated mask should still contain CPUs 2-3 after the parent becomes a root partition. Without this change, those CPUs are removed even though the child remains isolated.
Compute the CPUs owned directly by a partition by excluding CPUs granted to valid children. When validating a trial parent mask, exclude only CPUs granted to children that will remain valid under that mask. Reuse the child validation rules so PERR_INVCPUS and PERR_NOCPUS are handled consistently.
Use the directly owned mask for root/isolated type changes and housekeeping validation. If a root child returns the last housekeeping CPU to an isolated parent, invalidate the outermost isolated ancestor so the child can still become a member without violating the housekeeping constraint.
--- kernel/cgroup/cpuset.c | 138 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 128 insertions(+), 10 deletions(-)
kernel/cgroup/cpuset.c +128 -10
@@ -2175,6 +2175,46 @@ static void compute_partition_effective_cpumask(struct cpuset *cs,rcu_read_unlock (); } +/* +* Compute CPUs owned directly by a partition. +* +* effective_xcpus includes CPUs granted to valid child partitions. Exclude +* those CPUs when checking or changing this partition's type. +*/ +static void compute_partition_owned_cpumask (struct cpuset *cs, +const struct cpumask *partition_cpus, +struct cpumask *owned_cpus, +struct cpumask *remaining_cpus) +{ +struct cgroup_subsys_state *css; +struct cpuset *child; +bool populated =partition_is_populated (cs,NULL ); + +lockdep_assert_held (&cpuset_mutex); +cpumask_copy (owned_cpus, partition_cpus); +cpumask_and (remaining_cpus, partition_cpus, cpu_active_mask); + +rcu_read_lock (); +cpuset_for_each_child (child, css, cs) { +if (!is_partition_valid (child)) +continue ; + +/* +* A child that will become invalid under the proposed +* configuration cannot retain ownership of its CPUs. +*/ +if (child_partition_error (child, partition_cpus, + remaining_cpus, populated)) +continue ; + +cpumask_andnot (owned_cpus, owned_cpus, + child->effective_xcpus); +cpumask_andnot (remaining_cpus, remaining_cpus, + child->effective_xcpus); + } +rcu_read_unlock (); +} +/* * update_cpumasks_hier - Update effective cpumasks and tasks in the subtree * @cs: the cpuset to consider
@@ -2415,13 +2455,18 @@ static int parse_cpuset_cpulist(const char *buf, struct cpumask *out_mask)* validate_partition - Validate a cpuset partition configuration * @cs: The cpuset to validate * @trialcs: The trial cpuset containing proposed configuration changes +* @owned_cpus: Scratch mask for CPUs owned directly by the trial partition +* @remaining_cpus: Scratch mask used to predict valid child partitions * * If any validation check fails, the appropriate error code is set in the * cpuset's prs_err field. * * Return: PRS error code (0 if valid, non-zero error code if invalid) */ -static enum prs_errcode validate_partition (struct cpuset *cs,struct cpuset *trialcs) +static enum prs_errcode validate_partition (struct cpuset *cs, +struct cpuset *trialcs, +struct cpumask *owned_cpus, +struct cpumask *remaining_cpus) {struct cpuset *parent =parent_cs (cs);
@@ -2431,8 +2476,10 @@ static enum prs_errcode validate_partition(struct cpuset *cs, struct cpuset *triif (cpumask_empty (trialcs->effective_xcpus))return PERR_INVCPUS ; +compute_partition_owned_cpumask (cs, trialcs->effective_xcpus, + owned_cpus, remaining_cpus);if (prstate_housekeeping_conflict (trialcs->partition_root_state, - trialcs->effective_xcpus)) + owned_cpus))return PERR_HKEEPING ;if (tasks_nocpu_error (parent, cs, trialcs->effective_xcpus))
@@ -2458,7 +2505,8 @@ static void partition_cpus_change(struct cpuset *cs, struct cpuset *trialcs,if (cs_is_member (cs))return ; - prs_err =validate_partition (cs, trialcs); + prs_err =validate_partition (cs, trialcs, tmp->new_cpus, + tmp->addmask);if (prs_err) {WRITE_ONCE (cs->prs_err, prs_err); trialcs->prs_err = prs_err;
@@ -2937,6 +2985,48 @@ int cpuset_update_flag(cpuset_flagbits_t bit, struct cpuset *cs,return err; } +/* +* Invalidate the highest isolated partition that contains @cs. +* +* A root partition returning CPUs to an isolated parent can consume the last +* housekeeping CPU. Invalidating the whole chain returns the CPUs to a root +* partition instead. +*/ +static struct cpuset *invalidate_isolated_ancestor (struct cpuset *cs, +struct tmpmasks *tmp) +{ +struct cpuset *ancestor =parent_cs (cs); +struct cpuset *parent; +int err; + +lockdep_assert_held (&cpuset_mutex); +if (WARN_ON_ONCE (!ancestor)) +return NULL ; + +while ((ancestor != &top_cpuset) && + !is_remote_partition (ancestor)) { + parent =parent_cs (ancestor); +if (!parent || + parent->partition_root_state !=PRS_ISOLATED ) +break ; + ancestor = parent; + } + +if (WARN_ON_ONCE (ancestor == &top_cpuset)) +return NULL ; + +WRITE_ONCE (ancestor->prs_err,PERR_HKEEPING ); +if (is_remote_partition (ancestor)) { +remote_partition_disable (ancestor, tmp); + }else { + err =update_parent_effective_cpumask (ancestor, + partcmd_invalidate,NULL , tmp); +WARN_ON_ONCE (err); + } + +return ancestor; +} +/** * update_prstate - update partition_root_state * @cs: the cpuset to update
@@ -2949,6 +3039,8 @@ static int update_prstate(struct cpuset *cs, int new_prs) {int err =PERR_NONE , old_prs = cs->partition_root_state;struct cpuset *parent =parent_cs (cs); +struct cpuset *invalidated =NULL ; +struct cpumask *isolcpus_update_cpus = cs->effective_xcpus;struct tmpmasks tmpmask;bool isolcpus_updated = false;
@@ -3005,19 +3097,38 @@ static int update_prstate(struct cpuset *cs, int new_prs) }else if (old_prs && new_prs) {/* * A change in load balance state only, no change in cpumasks. -* Need to update isolated_cpus. +* Need to update isolated_cpus for CPUs owned by this partition, +* excluding CPUs distributed to valid child partitions. */ +compute_partition_owned_cpumask (cs, cs->effective_xcpus, + tmpmask.new_cpus, + tmpmask.addmask);if (((new_prs ==PRS_ISOLATED ) && - !isolated_cpus_can_update (cs->effective_xcpus,NULL )) || -prstate_housekeeping_conflict (new_prs, cs->effective_xcpus)) + !isolated_cpus_can_update (tmpmask.new_cpus,NULL )) || +prstate_housekeeping_conflict (new_prs, tmpmask.new_cpus)) { err =PERR_HKEEPING ; -else + }else { +/* +* Only directly owned CPUs change isolation state for a +* successful root <-> isolated type change. +*/ + isolcpus_update_cpus = tmpmask.new_cpus; isolcpus_updated = true; + } }else {/* * Switching back to member is always allowed even if it -* disables child partitions. +* disables child partitions. If returning CPUs to an isolated +* parent would consume the last housekeeping CPU, invalidate +* the outermost isolated ancestor and return its CPUs instead. */ +if (old_prs ==PRS_ROOT && + parent->partition_root_state ==PRS_ISOLATED && + !isolated_cpus_can_update (cs->effective_xcpus,NULL )) + invalidated =invalidate_isolated_ancestor (cs, &tmpmask); +if (invalidated) + goto out; +if (is_remote_partition (cs))remote_partition_disable (cs, &tmpmask);else
@@ -3045,11 +3156,18 @@ static int update_prstate(struct cpuset *cs, int new_prs)if (!is_partition_valid (cs))reset_partition_data (cs);else if (isolcpus_updated) -isolated_cpus_update (old_prs, new_prs, cs->effective_xcpus); +isolated_cpus_update (old_prs, new_prs, + isolcpus_update_cpus);spin_unlock_irq (&callback_lock);/* Force update if switching back to member & update effective_xcpus */ -update_cpumasks_hier (cs, &tmpmask, !new_prs); +if (invalidated) { +update_cpumasks_hier (invalidated, &tmpmask, false); +update_partition_sd_lb (invalidated,PRS_ISOLATED ); +notify_partition_change (invalidated,PRS_ISOLATED ); + }else { +update_cpumasks_hier (cs, &tmpmask, !new_prs); + }/* A newly created partition must have effective_xcpus set */ WARN_ON_ONCE(!old_prs && (new_prs > 0)
Signature · 1 line
2.43.0
PATCH v3 3/7 selftests/cgroup: Add tests for type-change isolation accounting
In reply to Guopeng ZhangFrom: Guopeng Zhang <[email protected]>
Add two matrix cases for parent type changes with valid child partitions. Cover root-to-isolated while the child remains root, and isolated-to-root while the child remains isolated. Check that only CPUs owned directly by the parent change isolation state.
--- tools/testing/selftests/cgroup/test_cpuset_prs.sh | 2 ++ 1 file changed, 2 insertions(+)
tools/testing/selftests/cgroup/test_cpuset_prs.sh +2 -0
@@ -224,6 +224,8 @@ TEST_MATRIX=( " C0-1 . . C2-3:P1 . . . C2 0 " " C0-1 . . C2-3:P1 . . . C4-5 0 B1:4-5" " C0-3:P1 C2-3:P1 . . . . . . 0 A1:0-1|A2:2-3|XA2:2-3" + " C0-3:P1 C2-3:P1 . . P2 . . . 0 A1:0-1|A2:2-3 A1:P2|A2:P1 0-1" + " C0-3:P2 C2-3:P2 . . P1 . . . 0 A1:0-1|A2:2-3 A1:P1|A2:P2 2-3" " C0-3:P1 C2-3:P1 . . C1-3 . . . 0 A1:1|A2:2-3|XA2:2-3" " C2-3:P1 C3:P1 . . C3 . . . 0 A1:|A2:3|XA2:3 A1:P1|A2:P1" " C2-3:P1 C3:P1 . . C3 P0 . . 0 A1:3|A2:3 A1:P1|A2:P0"
Signature · 1 line
2.43.0
PATCH v3 4/7 selftests/cgroup: Test child CPU ownership in partition changes
In reply to Guopeng ZhangFrom: Guopeng Zhang <[email protected]>
Create an isolated child which owns a boot-isolated CPU, then change its parent's partition type and CPU mask. Verify that a valid child keeps its CPUs and isolation state.
Also test trial parent masks which would invalidate the child because its CPU mask is no longer covered or because it would consume every remaining CPU of a populated parent. Verify that the returning boot-isolated CPU is included in the parent's housekeeping check.
Cache the boot-isolated CPU during setup so the tests do not depend on the order in which earlier test cases run.
--- .../selftests/cgroup/test_cpuset_prs.sh | 186 +++++++++++++++++- 1 file changed, 182 insertions(+), 4 deletions(-)
tools/testing/selftests/cgroup/test_cpuset_prs.sh +182 -4
@@ -107,6 +107,17 @@ then echo "Pre-isolated CPUs: $BOOT_ISOLCPUS" fi +# Cache the CPUs isolated from scheduler domains at boot. +BOOT_ISOLATED_FILE=/sys/devices/system/cpu/isolated +BOOT_CPUS= +BOOT_CPU= +if [[ -r $BOOT_ISOLATED_FILE ]] +then + BOOT_CPUS=$(cat $BOOT_ISOLATED_FILE) + [[ -n "$BOOT_CPUS" ]] && + BOOT_CPU=$(echo "$BOOT_CPUS" | sed -e 's/[,-].*//') +fi + cleanup() { online_cpus
@@ -1158,25 +1169,22 @@ test_isolated() } # -# Select an online CPU isolated from scheduler domains at boot. +# Check that a boot-isolated CPU selected during initialization is online. # $1: test name used in the skip message # get_boot_isolated_cpu() { TEST_NAME=$1 - BOOT_ISOLATED_FILE=/sys/devices/system/cpu/isolated [[ -r $BOOT_ISOLATED_FILE ]] || { echo "$TEST_NAME test SKIPPED: boot isolation state unavailable" return 1 } - BOOT_CPUS=$(cat $BOOT_ISOLATED_FILE) [[ -n "$BOOT_CPUS" ]] || { echo "$TEST_NAME test SKIPPED: no boot-isolated CPU" return 1 } - BOOT_CPU=$(echo "$BOOT_CPUS" | sed -e 's/[,-].*//') CPU_ONLINE=/sys/devices/system/cpu/cpu${BOOT_CPU}/online [[ ! -e $CPU_ONLINE || $(cat $CPU_ONLINE) -eq 1 ]] || { echo "$TEST_NAME test SKIPPED: CPU $BOOT_CPU is offline"
@@ -1214,6 +1222,174 @@ test_boot_isolated() echo "$TEST_NAME test PASSED." } +# +# A parent's type and CPU-mask changes must check only CPUs owned directly by +# the parent, not a boot-isolated CPU owned by a valid child partition. +# +test_child_owned_cpus() +{ + TEST_NAME="Child-owned CPU type change" + get_boot_isolated_cpu "$TEST_NAME" || return 0 + echo "Running $TEST_NAME test ..." + + cd $CGROUP2/test + echo member > cpuset.cpus.partition + echo +cpuset > cgroup.subtree_control + echo 2,$BOOT_CPU > cpuset.cpus + [[ $(cat cpuset.cpus.effective) = "2,$BOOT_CPU" ]] || { + echo "$TEST_NAME test SKIPPED: CPUs 2,$BOOT_CPU are unavailable" + echo "" > cpuset.cpus + cd $CGROUP2 + return 0 + } + test_partition isolated + mkdir A1 + cd A1 + echo $BOOT_CPU > cpuset.cpus + test_partition isolated + cd .. + test_effective_cpus 2 + test_partition root + echo 2-3,$BOOT_CPU > cpuset.cpus + test_effective_cpus 2-3 + [[ $(cat cpuset.cpus.partition) = root ]] || { + echo "Parent partition became invalid during CPU update" + exit 1 + } + [[ $(cat A1/cpuset.cpus.partition) = isolated ]] || { + echo "Child partition changed during parent update" + exit 1 + } + check_isolcpus "." || { + echo "Parent update corrupted child isolation" + exit 1 + } + cd A1 + test_partition member + cd .. + rmdir A1 + test_partition member + echo "" > cpuset.cpus + cd $CGROUP2 + echo "$TEST_NAME test PASSED." +} + +# +# A child that will become invalid under a trial parent CPU mask no longer +# owns its CPUs for the purpose of the parent's housekeeping validation. +# +test_trial_child_invalidation() +{ + TEST_NAME="Trial child partition invalidation" + get_boot_isolated_cpu "$TEST_NAME" || return 0 + echo "Running $TEST_NAME test ..." + + cd $CGROUP2/test + echo member > cpuset.cpus.partition + echo +cpuset > cgroup.subtree_control + echo 2-3,$BOOT_CPU > cpuset.cpus + [[ $(cat cpuset.cpus.effective) = "2-3,$BOOT_CPU" ]] || { + echo "$TEST_NAME test SKIPPED: CPUs 2-3,$BOOT_CPU are unavailable" + echo "" > cpuset.cpus + cd $CGROUP2 + return 0 + } + + # + # The child would fail validation with PERR_INVCPUS because CPU 2 is + # absent from the trial parent mask. CPU $BOOT_CPU must therefore be + # checked as a CPU that will return to the parent, not subtracted as + # child-owned. + # + test_partition isolated + mkdir A1 + cd A1 + echo 2,$BOOT_CPU > cpuset.cpus + test_partition isolated + cd .. + test_partition root + test_effective_cpus 3 + echo 3,$BOOT_CPU > cpuset.cpus + grep -q '^root invalid (partition config conflicts with housekeeping setup)$' \ + cpuset.cpus.partition || { + echo "PERR_INVCPUS trial allowed the parent to bypass housekeeping validation" + exit 1 + } + [[ $(cat A1/cpuset.cpus.partition) = "isolated invalid"* ]] || { + echo "Child did not become invalid after the parent CPU update" + exit 1 + } + [[ $(cat $CGROUP2/cpuset.cpus.effective) = "$CPULIST" ]] || { + echo "PERR_INVCPUS trial did not release the parent partition CPUs" + exit 1 + } + # + # An invalid isolated child retains its isolation state after its + # effective_xcpus is cleared. Explicitly transition CPU 2 through an + # isolated partition before starting the next case. + # + echo member > A1/cpuset.cpus.partition + rmdir A1 + echo member > cpuset.cpus.partition + echo "" > cpuset.cpus + echo 2 > cpuset.cpus + test_partition isolated + test_partition member + echo "" > cpuset.cpus + check_isolcpus "." || { + echo "PERR_INVCPUS trial cleanup left CPU 2 isolated" + exit 1 + } + + # + # With a task in the parent, the child would fail validation with + # PERR_NOCPUS because it consumes all CPUs in the trial mask. Its + # boot-isolated CPU must therefore participate in the parent's + # housekeeping validation. + # + echo 2-3,$BOOT_CPU > cpuset.cpus + test_partition isolated + mkdir A1 + cd A1 + echo 2,$BOOT_CPU > cpuset.cpus + test_partition isolated + cd .. + test_partition root + test_effective_cpus 3 + echo 0 > cgroup.procs + echo 2,$BOOT_CPU > cpuset.cpus + echo 0 > $CGROUP2/cgroup.procs + grep -q '^root invalid (partition config conflicts with housekeeping setup)$' \ + cpuset.cpus.partition || { + echo "PERR_NOCPUS trial allowed the parent to bypass housekeeping validation" + exit 1 + } + [[ $(cat A1/cpuset.cpus.partition) = "isolated invalid"* ]] || { + echo "Child did not become invalid after exhausting the parent CPUs" + exit 1 + } + [[ $(cat $CGROUP2/cpuset.cpus.effective) = "$CPULIST" ]] || { + echo "PERR_NOCPUS trial did not release the parent partition CPUs" + exit 1 + } + # Clean up the retained isolation state as in the PERR_INVCPUS case. + echo member > A1/cpuset.cpus.partition + rmdir A1 + echo member > cpuset.cpus.partition + echo "" > cpuset.cpus + echo 2 > cpuset.cpus + test_partition isolated + test_partition member + echo "" > cpuset.cpus + check_isolcpus "." || { + echo "PERR_NOCPUS trial cleanup left CPU 2 isolated" + exit 1 + } + + cd $CGROUP2 + echo "$TEST_NAME test PASSED." +} + # # Wait for inotify event for the given file and read it # $1: cgroup file to wait for
@@ -1286,5 +1462,7 @@ run_state_test TEST_MATRIX run_remote_state_test REMOTE_TEST_MATRIX test_isolated test_boot_isolated +test_child_owned_cpus +test_trial_child_invalidation test_inotify echo "All tests PASSED."
Signature · 1 line
2.43.0
PATCH v3 5/7 selftests/cgroup: Add tests for housekeeping CPU return to isolated parents
In reply to Guopeng ZhangFrom: Guopeng Zhang <[email protected]>
Return the last housekeeping CPU from a root child to an isolated parent. Check that the parent becomes invalid and releases its CPUs instead of adding the returned CPU to the isolated mask.
Repeat the test with two isolated ancestors and check that the outermost isolated partition is invalidated.
Add the nested test cgroup to the global cleanup list so a failed assertion does not leave the test hierarchy behind.
--- .../selftests/cgroup/test_cpuset_prs.sh | 149 +++++++++++++++++- 1 file changed, 148 insertions(+), 1 deletion(-)
tools/testing/selftests/cgroup/test_cpuset_prs.sh +148 -1
@@ -122,7 +122,8 @@ cleanup() { online_cpus cd $CGROUP2 - rmdir A1/A2/A3 A1/A2 A1 B1 test/A1 test/B1 test > /dev/null 2>&1 + rmdir A1/A2/A3 A1/A2 A1 B1 \ + test/A1/A2 test/A1 test/B1 test > /dev/null 2>&1 rmdir rtest/p1/c11 rtest/p1/c12 rtest/p2/c21 \ rtest/p2/c22 rtest/p1 rtest/p2 rtest > /dev/null 2>&1 [[ -n "$SCHED_DEBUG" ]] &&
@@ -1222,6 +1223,30 @@ test_boot_isolated() echo "$TEST_NAME test PASSED." } +# Return success if CPU $2 is present in cpulist $1. +cpulist_contains() +{ + local CPULIST_ARG=$1 + local CPU_ARG=$2 + local ITEM FIRST LAST + local OLD_IFS=$IFS + + IFS=, + for ITEM in $CPULIST_ARG + do + FIRST=${ITEM%-*} + LAST=${ITEM#*-} + [[ $ITEM != *-* ]] && LAST=$FIRST + if [[ $CPU_ARG -ge $FIRST && $CPU_ARG -le $LAST ]] + then + IFS=$OLD_IFS + return 0 + fi + done + IFS=$OLD_IFS + return 1 +} + # # A parent's type and CPU-mask changes must check only CPUs owned directly by # the parent, not a boot-isolated CPU owned by a valid child partition.
@@ -1390,6 +1415,127 @@ test_trial_child_invalidation() echo "$TEST_NAME test PASSED." } +# +# Returning the last housekeeping CPU to an isolated parent must invalidate +# the isolated partition instead of adding that CPU to the isolated mask. +# +test_housekeeping_cpu_return() +{ + TEST_NAME="Housekeeping CPU return" + NOHZ_FILE=/sys/devices/system/cpu/nohz_full + [[ -r $NOHZ_FILE ]] || { + echo "$TEST_NAME test SKIPPED: no nohz_full state" + return 0 + } + NOHZ_CPUS=$(cat $NOHZ_FILE) + [[ -n "$NOHZ_CPUS" && "$NOHZ_CPUS" != "(null)" ]] || { + echo "$TEST_NAME test SKIPPED: no nohz_full CPUs" + return 0 + } + + HK_CPU= + HK_COUNT=0 + TYPE_CPUS=() + for ((CPU=0; CPU < NR_CPUS; CPU++)) + do + CPU_ONLINE=/sys/devices/system/cpu/cpu${CPU}/online + [[ ! -e $CPU_ONLINE || $(cat $CPU_ONLINE) -eq 1 ]] || continue + cpulist_contains "$BOOT_CPUS" $CPU && continue + if cpulist_contains "$NOHZ_CPUS" $CPU + then + [[ ${#TYPE_CPUS[@]} -lt 3 ]] && TYPE_CPUS+=("$CPU") + else + HK_CPU=$CPU + ((HK_COUNT++)) + fi + done + + [[ $HK_COUNT -eq 1 && ${#TYPE_CPUS[@]} -ge 2 ]] || { + echo "$TEST_NAME test SKIPPED: requires one full housekeeping CPU" + return 0 + } + echo "Running $TEST_NAME test ..." + + cd $CGROUP2/test + echo member > cpuset.cpus.partition + echo +cpuset > cgroup.subtree_control + echo $HK_CPU,${TYPE_CPUS[0]},${TYPE_CPUS[1]} > cpuset.cpus + test_partition root + mkdir A1 + cd A1 + echo $HK_CPU > cpuset.cpus + test_partition root + cd .. + test_partition isolated + cd A1 + test_partition member + cd .. + grep -q '^isolated invalid (partition config conflicts with housekeeping setup)$' \ + cpuset.cpus.partition || { + echo "Isolated parent remained valid after housekeeping CPU return" + exit 1 + } + [[ $(cat $CGROUP2/cpuset.cpus.effective) = "$CPULIST" ]] || { + echo "Housekeeping CPU return did not release the partition CPUs" + exit 1 + } + check_isolcpus "." || { + echo "Housekeeping CPU was added to the isolated mask" + exit 1 + } + rmdir A1 + echo member > cpuset.cpus.partition + echo "" > cpuset.cpus + + if [[ ${#TYPE_CPUS[@]} -lt 3 ]] + then + echo "Nested $TEST_NAME test SKIPPED: requires three nohz_full CPUs" + cd $CGROUP2 + echo "$TEST_NAME test PASSED." + return 0 + fi + + # Repeat the check with two isolated ancestors. + echo $HK_CPU,${TYPE_CPUS[0]},${TYPE_CPUS[1]},${TYPE_CPUS[2]} > cpuset.cpus + test_partition root + mkdir A1 + cd A1 + echo $HK_CPU,${TYPE_CPUS[1]},${TYPE_CPUS[2]} > cpuset.cpus + test_partition root + echo +cpuset > cgroup.subtree_control + mkdir A2 + cd A2 + echo $HK_CPU > cpuset.cpus + test_partition root + cd .. + test_partition isolated + cd .. + test_partition isolated + cd A1/A2 + test_partition member + cd ../.. + grep -q '^isolated invalid (partition config conflicts with housekeeping setup)$' \ + cpuset.cpus.partition || { + echo "Outermost isolated partition remained valid after housekeeping CPU return" + exit 1 + } + [[ $(cat $CGROUP2/cpuset.cpus.effective) = "$CPULIST" ]] || { + echo "Nested housekeeping CPU return did not release the partition CPUs" + exit 1 + } + check_isolcpus "." || { + echo "Nested housekeeping CPU return added the CPU to the isolated mask" + exit 1 + } + rmdir A1/A2 + echo member > A1/cpuset.cpus.partition + rmdir A1 + echo member > cpuset.cpus.partition + echo "" > cpuset.cpus + cd $CGROUP2 + echo "$TEST_NAME test PASSED." +} + # # Wait for inotify event for the given file and read it # $1: cgroup file to wait for
@@ -1464,5 +1610,6 @@ test_isolated test_boot_isolated test_child_owned_cpus test_trial_child_invalidation +test_housekeeping_cpu_return test_inotify echo "All tests PASSED."
Signature · 1 line
2.43.0
PATCH v3 6/7 cgroup/cpuset: Release CPUs when type-change validation fails
In reply to Guopeng ZhangFrom: Guopeng Zhang <[email protected]>
When housekeeping validation fails during a root/isolated type change, update_prstate() marks the requested state invalid without running the partition-disable path. The failed partition's effective_xcpus may be cleared, but its CPUs remain unavailable to the partition which owns the invalidated subtree.
This can be reproduced on a cgroup v2 system booted with isolcpus=domain,15:
cd /sys/fs/cgroup
echo +cpuset > cgroup.subtree_control
mkdir type-fail-repro
echo 15 > type-fail-repro/cpuset.cpus
echo isolated > type-fail-repro/cpuset.cpus.partition echo root > type-fail-repro/cpuset.cpus.partition cat type-fail-repro/cpuset.cpus.partition cat cpuset.cpus.effective
The requested root state is recorded as invalid, but CPU 15 remains unavailable to the top cpuset.
Run the common partition-disable path when housekeeping validation fails. Disable remote partitions with remote_partition_disable() and return local partition CPUs to their parent. If that would consume the last housekeeping CPU, invalidate the outermost isolated ancestor instead.
--- kernel/cgroup/cpuset.c | 6 ++++++ 1 file changed, 6 insertions(+)
kernel/cgroup/cpuset.c +6 -0
@@ -3042,6 +3042,7 @@ static int update_prstate(struct cpuset *cs, int new_prs)struct cpuset *invalidated =NULL ;struct cpumask *isolcpus_update_cpus = cs->effective_xcpus;struct tmpmasks tmpmask; +bool disable_partition = false;bool isolcpus_updated = false;if (old_prs == new_prs)
@@ -3107,6 +3108,7 @@ static int update_prstate(struct cpuset *cs, int new_prs) !isolated_cpus_can_update(tmpmask.new_cpus, NULL)) || prstate_housekeeping_conflict(new_prs, tmpmask.new_cpus)) { err = PERR_HKEEPING; + disable_partition = true; } else {/* * Only directly owned CPUs change isolation state for a
@@ -3122,6 +3124,10 @@ static int update_prstate(struct cpuset *cs, int new_prs)* parent would consume the last housekeeping CPU, invalidate * the outermost isolated ancestor and return its CPUs instead. */ + disable_partition = true; + } + +if (disable_partition) {if (old_prs ==PRS_ROOT && parent->partition_root_state ==PRS_ISOLATED && !isolated_cpus_can_update (cs->effective_xcpus,NULL ))
Signature · 1 line
2.43.0
PATCH v3 7/7 selftests/cgroup: Add CPU release tests for type-change validation failures
In reply to Guopeng ZhangFrom: Guopeng Zhang <[email protected]>
Exercise housekeeping validation failures for local and remote partition type changes. Check that the requested partition type becomes invalid, its CPUs are returned to the top cpuset and their boot-time isolation state is preserved.
--- .../selftests/cgroup/test_cpuset_prs.sh | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+)
tools/testing/selftests/cgroup/test_cpuset_prs.sh +93 -0
@@ -1536,6 +1536,97 @@ test_housekeeping_cpu_return() echo "$TEST_NAME test PASSED." } +# +# A local type change that fails the boot housekeeping constraint must release +# the partition CPUs and preserve their boot-isolated state. +# +test_partition_type_failure() +{ + TEST_NAME="Local partition type-change validation failure" + get_boot_isolated_cpu "$TEST_NAME" || return 0 + echo "Running $TEST_NAME test ..." + + cd $CGROUP2/test + echo member > cpuset.cpus.partition + echo $BOOT_CPU > cpuset.cpus + [[ $(cat cpuset.cpus.effective) = "$BOOT_CPU" ]] || { + echo "$TEST_NAME test SKIPPED: CPU $BOOT_CPU is unavailable" + echo "" > cpuset.cpus + cd $CGROUP2 + return 0 + } + test_partition isolated + echo root > cpuset.cpus.partition + grep -q '^root invalid (partition config conflicts with housekeeping setup)$' \ + cpuset.cpus.partition || { + echo "Partition type failure did not produce the expected state" + exit 1 + } + [[ $(cat $CGROUP2/cpuset.cpus.effective) = "$CPULIST" ]] || { + echo "Partition type failure did not release CPU $BOOT_CPU" + exit 1 + } + check_isolcpus "." || { + echo "Partition type failure lost boot-isolated CPU $BOOT_CPU" + exit 1 + } + echo member > cpuset.cpus.partition + echo "" > cpuset.cpus + cd $CGROUP2 + echo "$TEST_NAME test PASSED." +} + +# +# Exercise the same type-change validation failure for a remote partition. +# +test_remote_partition_type_failure() +{ + TEST_NAME="Remote partition type-change validation failure" + get_boot_isolated_cpu "$TEST_NAME" || return 0 + echo "Running $TEST_NAME test ..." + + cd $CGROUP2/test + echo member > cpuset.cpus.partition + echo +cpuset > cgroup.subtree_control + echo $BOOT_CPU > cpuset.cpus + echo $BOOT_CPU > cpuset.cpus.exclusive + [[ $(cat cpuset.cpus.effective) = "$BOOT_CPU" ]] || { + echo "$TEST_NAME test SKIPPED: CPU $BOOT_CPU is unavailable" + echo "" > cpuset.cpus.exclusive + echo "" > cpuset.cpus + cd $CGROUP2 + return 0 + } + mkdir A1 + cd A1 + echo $BOOT_CPU > cpuset.cpus + echo $BOOT_CPU > cpuset.cpus.exclusive + test_partition isolated + echo root > cpuset.cpus.partition + grep -q '^root invalid (partition config conflicts with housekeeping setup)$' \ + cpuset.cpus.partition || { + echo "Remote type failure did not produce the expected state" + exit 1 + } + [[ $(cat $CGROUP2/cpuset.cpus.effective) = "$CPULIST" ]] || { + echo "Remote type failure did not release CPU $BOOT_CPU" + exit 1 + } + check_isolcpus "." || { + echo "Remote type failure lost boot-isolated CPU $BOOT_CPU" + exit 1 + } + echo member > cpuset.cpus.partition + echo "" > cpuset.cpus.exclusive + echo "" > cpuset.cpus + cd .. + rmdir A1 + echo "" > cpuset.cpus.exclusive + echo "" > cpuset.cpus + cd $CGROUP2 + echo "$TEST_NAME test PASSED." +} + # # Wait for inotify event for the given file and read it # $1: cgroup file to wait for
@@ -1611,5 +1702,7 @@ test_boot_isolated test_child_owned_cpus test_trial_child_invalidation test_housekeeping_cpu_return +test_partition_type_failure +test_remote_partition_type_failure test_inotify echo "All tests PASSED."
Signature · 1 line
2.43.0