This patchset makes the following two major changes to the cpuset v2 code:
Patch 2: Add a new partition state "root-nolb" to create a partition root with load balancing disabled. This is for handling intermitten workloads that have a strict low latency requirement.
Patch 3: Allow partition roots that are not the top cpuset to distribute all its cpus to child partitions as long as there is no task associated with that partition root. This allows more flexibility for middleware to manage multiple partitions.
Patch 4 updates the cgroup-v2.rst file accordingly. Patch 5 adds a test to test the new cpuset partition code.
Waiman Long (5): cgroup/cpuset: Don't call validate_change() for some flag changes cgroup/cpuset: Add new cpus.partition type with no load balancing cgroup/cpuset: Allow non-top parent partition root to distribute out all CPUs cgroup/cpuset: Update description of cpuset.cpus.partition in cgroup-v2.rst kselftest/cgroup: Add cpuset v2 partition root state test
Documentation/admin-guide/cgroup-v2.rst | 19 ++- kernel/cgroup/cpuset.c | 124 +++++++++++---- tools/testing/selftests/cgroup/Makefile | 2 +- .../selftests/cgroup/test_cpuset_prs.sh | 141 ++++++++++++++++++ 4 files changed, 247 insertions(+), 39 deletions(-) create mode 100755 tools/testing/selftests/cgroup/test_cpuset_prs.sh
The update_flag() is called with one flag bit change and without change in the various cpumasks in the cpuset. Moreover, not all changes in the flag bits are validated in validate_change(). In particular, the load balance flag and the two spread flags are not checked there. So there is no point in calling validate_change() if those flag bits change.
Signed-off-by: Waiman Long longman@redhat.com --- kernel/cgroup/cpuset.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c index adb5190c4429..65ad6995ad77 100644 --- a/kernel/cgroup/cpuset.c +++ b/kernel/cgroup/cpuset.c @@ -1891,7 +1891,7 @@ static void update_tasks_flags(struct cpuset *cs) * cs: the cpuset to update * turning_on: whether the flag is being set or cleared * - * Call with cpuset_mutex held. + * Call with cpuset_mutex held & cpumasks remain unchanged. */
static int update_flag(cpuset_flagbits_t bit, struct cpuset *cs, @@ -1911,16 +1911,22 @@ static int update_flag(cpuset_flagbits_t bit, struct cpuset *cs, else clear_bit(bit, &trialcs->flags);
- err = validate_change(cs, trialcs); - if (err < 0) - goto out; - balance_flag_changed = (is_sched_load_balance(cs) != is_sched_load_balance(trialcs));
spread_flag_changed = ((is_spread_slab(cs) != is_spread_slab(trialcs)) || (is_spread_page(cs) != is_spread_page(trialcs)));
+ /* + * validate_change() doesn't validate changes in load balance + * and spread flags. + */ + if (!balance_flag_changed && !spread_flag_changed) { + err = validate_change(cs, trialcs); + if (err < 0) + goto out; + } + spin_lock_irq(&callback_lock); cs->flags = trialcs->flags; spin_unlock_irq(&callback_lock);
Hello,
On Thu, Jun 03, 2021 at 05:24:12PM -0400, Waiman Long wrote:
The update_flag() is called with one flag bit change and without change in the various cpumasks in the cpuset. Moreover, not all changes in the flag bits are validated in validate_change(). In particular, the load balance flag and the two spread flags are not checked there. So there is no point in calling validate_change() if those flag bits change.
The fact that it's escaping validation conditionally from caller side is bothersome given that the idea is to have self-contained verifier to ensure correctness. I'd prefer to make the validation more complete and optimized (ie. detect or keep track of what changed) if really necessary rather than escaping partially because certain conditions aren't checked.
Thanks.
On 6/16/21 4:39 PM, Tejun Heo wrote:
Hello,
On Thu, Jun 03, 2021 at 05:24:12PM -0400, Waiman Long wrote:
The update_flag() is called with one flag bit change and without change in the various cpumasks in the cpuset. Moreover, not all changes in the flag bits are validated in validate_change(). In particular, the load balance flag and the two spread flags are not checked there. So there is no point in calling validate_change() if those flag bits change.
The fact that it's escaping validation conditionally from caller side is bothersome given that the idea is to have self-contained verifier to ensure correctness. I'd prefer to make the validation more complete and optimized (ie. detect or keep track of what changed) if really necessary rather than escaping partially because certain conditions aren't checked.
Thanks for the comments.
You are right. I will leave out this patch. Anyway, the rests of the patchset don't have a strict dependency on it.
Cheers, Longman
Cpuset v1 uses the sched_load_balance control file to determine if load balancing should be enabled. Cpuset v2 gets rid of sched_load_balance as its use may require disabling load balancing at cgroup root.
For workloads that require very low latency like DPDK, the latency jitters caused by periodic load balancing may exceed the desired latency limit.
When cpuset v2 is in use, the only way to avoid this latency cost is to use the "isolcpus=" kernel boot option to isolate a set of CPUs. After the kernel boot, however, there is no way to add or remove CPUs from this isolated set. For workloads that are more dynamic in nature, that means users have to provision enough CPUs for the worst case situation resulting in excess idle CPUs.
To address this issue for cpuset v2, a new cpuset.cpus.partition type "root-nolb" is added which allows the creation of a cpuset partition with no load balancing. This will allow system administrators to dynamically adjust the size of the no load balancing partition to the current need of the workload without rebooting the system.
Signed-off-by: Waiman Long longman@redhat.com --- kernel/cgroup/cpuset.c | 64 ++++++++++++++++++++++++++++++++---------- 1 file changed, 49 insertions(+), 15 deletions(-)
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c index 65ad6995ad77..78dd6c91dcd6 100644 --- a/kernel/cgroup/cpuset.c +++ b/kernel/cgroup/cpuset.c @@ -169,6 +169,8 @@ struct cpuset { * * 1 - partition root * + * 2 - partition root (no load balance) + * * -1 - invalid partition root * None of the cpus in cpus_allowed can be put into the parent's * subparts_cpus. In this case, the cpuset is not a real partition @@ -178,6 +180,7 @@ struct cpuset { */ #define PRS_DISABLED 0 #define PRS_ENABLED 1 +#define PRS_ENABLED_NOLB 2 #define PRS_ERROR -1
/* @@ -1246,6 +1249,7 @@ static int update_parent_subparts_cpumask(struct cpuset *cpuset, int cmd, */ switch (cpuset->partition_root_state) { case PRS_ENABLED: + case PRS_ENABLED_NOLB: if (part_error) cpuset->partition_root_state = PRS_ERROR; break; @@ -1943,30 +1947,29 @@ static int update_flag(cpuset_flagbits_t bit, struct cpuset *cs,
/* * update_prstate - update partititon_root_state - * cs: the cpuset to update - * val: 0 - disabled, 1 - enabled + * cs: the cpuset to update + * new_prs: new partition root state * * Call with cpuset_mutex held. */ -static int update_prstate(struct cpuset *cs, int val) +static int update_prstate(struct cpuset *cs, int new_prs) { int err; struct cpuset *parent = parent_cs(cs); - struct tmpmasks tmp; + struct tmpmasks tmpmask; + bool sched_domain_rebuilt = false;
- if ((val != 0) && (val != 1)) - return -EINVAL; - if (val == cs->partition_root_state) + if (new_prs == cs->partition_root_state) return 0;
/* * Cannot force a partial or invalid partition root to a full * partition root. */ - if (val && cs->partition_root_state) + if (new_prs && (cs->partition_root_state < 0)) return -EINVAL;
- if (alloc_cpumasks(NULL, &tmp)) + if (alloc_cpumasks(NULL, &tmpmask)) return -ENOMEM;
err = -EINVAL; @@ -1984,12 +1987,31 @@ static int update_prstate(struct cpuset *cs, int val) goto out;
err = update_parent_subparts_cpumask(cs, partcmd_enable, - NULL, &tmp); + NULL, &tmpmask); + if (err) { update_flag(CS_CPU_EXCLUSIVE, cs, 0); goto out; + } else if (new_prs == PRS_ENABLED_NOLB) { + /* + * Disable the load balance flag should not return an + * error unless the system is running out of memory. + */ + update_flag(CS_SCHED_LOAD_BALANCE, cs, 0); + sched_domain_rebuilt = true; } - cs->partition_root_state = PRS_ENABLED; + + cs->partition_root_state = new_prs; + } else if (cs->partition_root_state && new_prs) { + /* + * A change in load balance state only, no change in cpumasks. + */ + update_flag(CS_SCHED_LOAD_BALANCE, cs, + (new_prs != PRS_ENABLED_NOLB)); + + cs->partition_root_state = new_prs; + err = 0; + goto out; /* Sched domain is rebuilt in update_flag() */ } else { /* * Turning off partition root will clear the @@ -2003,7 +2025,7 @@ static int update_prstate(struct cpuset *cs, int val) }
err = update_parent_subparts_cpumask(cs, partcmd_disable, - NULL, &tmp); + NULL, &tmpmask); if (err) goto out;
@@ -2011,6 +2033,12 @@ static int update_prstate(struct cpuset *cs, int val)
/* Turning off CS_CPU_EXCLUSIVE will not return error */ update_flag(CS_CPU_EXCLUSIVE, cs, 0); + + if (!is_sched_load_balance(cs)) { + /* Make sure load balance is on */ + update_flag(CS_SCHED_LOAD_BALANCE, cs, 1); + sched_domain_rebuilt = true; + } }
/* @@ -2021,11 +2049,12 @@ static int update_prstate(struct cpuset *cs, int val) update_tasks_cpumask(parent);
if (parent->child_ecpus_count) - update_sibling_cpumasks(parent, cs, &tmp); + update_sibling_cpumasks(parent, cs, &tmpmask);
- rebuild_sched_domains_locked(); + if (!sched_domain_rebuilt) + rebuild_sched_domains_locked(); out: - free_cpumasks(NULL, &tmp); + free_cpumasks(NULL, &tmpmask); return err; }
@@ -2518,6 +2547,9 @@ static int sched_partition_show(struct seq_file *seq, void *v) case PRS_ENABLED: seq_puts(seq, "root\n"); break; + case PRS_ENABLED_NOLB: + seq_puts(seq, "root-nolb\n"); + break; case PRS_DISABLED: seq_puts(seq, "member\n"); break; @@ -2544,6 +2576,8 @@ static ssize_t sched_partition_write(struct kernfs_open_file *of, char *buf, val = PRS_ENABLED; else if (!strcmp(buf, "member")) val = PRS_DISABLED; + else if (!strcmp(buf, "root-nolb")) + val = PRS_ENABLED_NOLB; else return -EINVAL;
On Thu, Jun 03, 2021 at 05:24:13PM -0400, Waiman Long wrote:
Cpuset v1 uses the sched_load_balance control file to determine if load balancing should be enabled. Cpuset v2 gets rid of sched_load_balance as its use may require disabling load balancing at cgroup root.
For workloads that require very low latency like DPDK, the latency jitters caused by periodic load balancing may exceed the desired latency limit.
When cpuset v2 is in use, the only way to avoid this latency cost is to use the "isolcpus=" kernel boot option to isolate a set of CPUs. After the kernel boot, however, there is no way to add or remove CPUs from this isolated set. For workloads that are more dynamic in nature, that means users have to provision enough CPUs for the worst case situation resulting in excess idle CPUs.
To address this issue for cpuset v2, a new cpuset.cpus.partition type "root-nolb" is added which allows the creation of a cpuset partition with no load balancing. This will allow system administrators to dynamically adjust the size of the no load balancing partition to the current need of the workload without rebooting the system.
I'm confused, why do you need this? Just create a parition for each cpu.
On 6/10/21 2:50 PM, Peter Zijlstra wrote:
On Thu, Jun 03, 2021 at 05:24:13PM -0400, Waiman Long wrote:
Cpuset v1 uses the sched_load_balance control file to determine if load balancing should be enabled. Cpuset v2 gets rid of sched_load_balance as its use may require disabling load balancing at cgroup root.
For workloads that require very low latency like DPDK, the latency jitters caused by periodic load balancing may exceed the desired latency limit.
When cpuset v2 is in use, the only way to avoid this latency cost is to use the "isolcpus=" kernel boot option to isolate a set of CPUs. After the kernel boot, however, there is no way to add or remove CPUs from this isolated set. For workloads that are more dynamic in nature, that means users have to provision enough CPUs for the worst case situation resulting in excess idle CPUs.
To address this issue for cpuset v2, a new cpuset.cpus.partition type "root-nolb" is added which allows the creation of a cpuset partition with no load balancing. This will allow system administrators to dynamically adjust the size of the no load balancing partition to the current need of the workload without rebooting the system.
I'm confused, why do you need this? Just create a parition for each cpu.
From a management point of view, it is more cumbersome to do one cpu per partition. I have suggested this idea of 1 cpu per partition to the container developers, but they don't seem to like it.
Cheers, Longman
On Thu, Jun 10, 2021 at 03:16:29PM -0400, Waiman Long wrote:
On 6/10/21 2:50 PM, Peter Zijlstra wrote:
On Thu, Jun 03, 2021 at 05:24:13PM -0400, Waiman Long wrote:
Cpuset v1 uses the sched_load_balance control file to determine if load balancing should be enabled. Cpuset v2 gets rid of sched_load_balance as its use may require disabling load balancing at cgroup root.
For workloads that require very low latency like DPDK, the latency jitters caused by periodic load balancing may exceed the desired latency limit.
When cpuset v2 is in use, the only way to avoid this latency cost is to use the "isolcpus=" kernel boot option to isolate a set of CPUs. After the kernel boot, however, there is no way to add or remove CPUs from this isolated set. For workloads that are more dynamic in nature, that means users have to provision enough CPUs for the worst case situation resulting in excess idle CPUs.
To address this issue for cpuset v2, a new cpuset.cpus.partition type "root-nolb" is added which allows the creation of a cpuset partition with no load balancing. This will allow system administrators to dynamically adjust the size of the no load balancing partition to the current need of the workload without rebooting the system.
I'm confused, why do you need this? Just create a parition for each cpu.
From a management point of view, it is more cumbersome to do one cpu per partition. I have suggested this idea of 1 cpu per partition to the container developers, but they don't seem to like it.
Oh, because it then creates a cgroup tree per CPU and you get to move tasks between cgroups?
OK I suppose.
On Thu, Jun 03, 2021 at 05:24:13PM -0400, Waiman Long wrote:
Cpuset v1 uses the sched_load_balance control file to determine if load balancing should be enabled. Cpuset v2 gets rid of sched_load_balance as its use may require disabling load balancing at cgroup root.
For workloads that require very low latency like DPDK, the latency jitters caused by periodic load balancing may exceed the desired latency limit.
When cpuset v2 is in use, the only way to avoid this latency cost is to use the "isolcpus=" kernel boot option to isolate a set of CPUs. After the kernel boot, however, there is no way to add or remove CPUs from this isolated set. For workloads that are more dynamic in nature, that means users have to provision enough CPUs for the worst case situation resulting in excess idle CPUs.
Also, can we change isolcpus to create a default cgroup hierarchy instead of being the fugly hack that it is? I really hate isolcpus with a passion, it needs to die.
On 6/10/21 3:00 PM, Peter Zijlstra wrote:
On Thu, Jun 03, 2021 at 05:24:13PM -0400, Waiman Long wrote:
Cpuset v1 uses the sched_load_balance control file to determine if load balancing should be enabled. Cpuset v2 gets rid of sched_load_balance as its use may require disabling load balancing at cgroup root.
For workloads that require very low latency like DPDK, the latency jitters caused by periodic load balancing may exceed the desired latency limit.
When cpuset v2 is in use, the only way to avoid this latency cost is to use the "isolcpus=" kernel boot option to isolate a set of CPUs. After the kernel boot, however, there is no way to add or remove CPUs from this isolated set. For workloads that are more dynamic in nature, that means users have to provision enough CPUs for the worst case situation resulting in excess idle CPUs.
Also, can we change isolcpus to create a default cgroup hierarchy instead of being the fugly hack that it is? I really hate isolcpus with a passion, it needs to die.
That is probably doable assuming that we can allow cpuset v2 to have a non-load balanced partition.
Depending on which cpuset version is set up, we can automatically set up a isolated subdirectory under / to contain cpus that are isolated. However, that will be a follow-on patch after this one.
Cheers, Longman
Hello,
Generally looks fine to me.
On Thu, Jun 03, 2021 at 05:24:13PM -0400, Waiman Long wrote:
@@ -1984,12 +1987,31 @@ static int update_prstate(struct cpuset *cs, int val) goto out; err = update_parent_subparts_cpumask(cs, partcmd_enable,
NULL, &tmp);
NULL, &tmpmask);- if (err) { update_flag(CS_CPU_EXCLUSIVE, cs, 0); goto out;
} else if (new_prs == PRS_ENABLED_NOLB) {/** Disable the load balance flag should not return an
^ing
and "else if" after "if (err) goto out" block is weird. The two conditions don't need to be tied together.
@@ -2518,6 +2547,9 @@ static int sched_partition_show(struct seq_file *seq, void *v) case PRS_ENABLED: seq_puts(seq, "root\n"); break;
- case PRS_ENABLED_NOLB:
seq_puts(seq, "root-nolb\n");break;@@ -2544,6 +2576,8 @@ static ssize_t sched_partition_write(struct kernfs_open_file *of, char *buf, val = PRS_ENABLED; else if (!strcmp(buf, "member")) val = PRS_DISABLED;
- else if (!strcmp(buf, "root-nolb"))
val = PRS_ENABLED_NOLB;
I wonder whether there's a better name than "root-nolb" because nolb isn't the most readable and we are using space as the delimiter for other names. Would something like "isolated" work?
Thanks.
On 6/16/21 4:47 PM, Tejun Heo wrote:
Hello,
Generally looks fine to me.
On Thu, Jun 03, 2021 at 05:24:13PM -0400, Waiman Long wrote:
@@ -1984,12 +1987,31 @@ static int update_prstate(struct cpuset *cs, int val) goto out; err = update_parent_subparts_cpumask(cs, partcmd_enable,
NULL, &tmp);
NULL, &tmpmask);- if (err) { update_flag(CS_CPU_EXCLUSIVE, cs, 0); goto out;
} else if (new_prs == PRS_ENABLED_NOLB) {/** Disable the load balance flag should not return an^ingand "else if" after "if (err) goto out" block is weird. The two conditions don't need to be tied together.
Yes, the else part is redundant in this case. Will remove it.
@@ -2518,6 +2547,9 @@ static int sched_partition_show(struct seq_file *seq, void *v) case PRS_ENABLED: seq_puts(seq, "root\n"); break;
- case PRS_ENABLED_NOLB:
seq_puts(seq, "root-nolb\n");break;@@ -2544,6 +2576,8 @@ static ssize_t sched_partition_write(struct kernfs_open_file *of, char *buf, val = PRS_ENABLED; else if (!strcmp(buf, "member")) val = PRS_DISABLED;
- else if (!strcmp(buf, "root-nolb"))
val = PRS_ENABLED_NOLB;I wonder whether there's a better name than "root-nolb" because nolb isn't the most readable and we are using space as the delimiter for other names. Would something like "isolated" work?
Right. "isolated" is a better name and it corresponds better with the isolcpus kernel command line option. Will change the name.
Thanks, Longman
Currently, a parent partition root cannot distribute all its CPUs to child partition roots with no CPUs left. However in some use cases, a management application may want to create a parent partition root as a management unit with no task associated with it and has all its CPUs distributed to various child partition roots dynamically according to their needs. Leaving a cpu in the parent partition root in such a case is now a waste.
To accommodate such use cases, a parent partition root can now have all its CPUs distributed to its child partition roots as long as: 1) it is not the top cpuset; and 2) there is no task directly associated with the parent.
Once an empty parent partition root is formed, no new task can be moved into it.
Signed-off-by: Waiman Long longman@redhat.com --- kernel/cgroup/cpuset.c | 44 +++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-)
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c index 78dd6c91dcd6..ef19eb317fef 100644 --- a/kernel/cgroup/cpuset.c +++ b/kernel/cgroup/cpuset.c @@ -1117,7 +1117,7 @@ enum subparts_cmd { * cpus_allowed can be granted or an error code will be returned. * * For partcmd_disable, the cpuset is being transofrmed from a partition - * root back to a non-partition root. any CPUs in cpus_allowed that are in + * root back to a non-partition root. Any CPUs in cpus_allowed that are in * parent's subparts_cpus will be taken away from that cpumask and put back * into parent's effective_cpus. 0 should always be returned. * @@ -1172,21 +1172,31 @@ static int update_parent_subparts_cpumask(struct cpuset *cpuset, int cmd, if ((cmd != partcmd_update) && css_has_online_children(&cpuset->css)) return -EBUSY;
- /* - * Enabling partition root is not allowed if not all the CPUs - * can be granted from parent's effective_cpus or at least one - * CPU will be left after that. - */ - if ((cmd == partcmd_enable) && - (!cpumask_subset(cpuset->cpus_allowed, parent->effective_cpus) || - cpumask_equal(cpuset->cpus_allowed, parent->effective_cpus))) - return -EINVAL; - /* * A cpumask update cannot make parent's effective_cpus become empty. */ adding = deleting = false; if (cmd == partcmd_enable) { + bool parent_is_top_cpuset = !parent_cs(parent); + bool no_cpu_in_parent = cpumask_equal(cpuset->cpus_allowed, + parent->effective_cpus); + /* + * Enabling partition root is not allowed if not all the CPUs + * can be granted from parent's effective_cpus. If the parent + * is the top cpuset, at least one CPU must be left after that. + */ + if (!cpumask_subset(cpuset->cpus_allowed, parent->effective_cpus) || + (parent_is_top_cpuset && no_cpu_in_parent)) + return -EINVAL; + + /* + * A non-top parent can be left with no CPU as long as there + * is no task directly associated with the parent. For such + * a parent, no new task can be moved into it. + */ + if (no_cpu_in_parent && parent->css.cgroup->nr_populated_csets) + return -EINVAL; + cpumask_copy(tmp->addmask, cpuset->cpus_allowed); adding = true; } else if (cmd == partcmd_disable) { @@ -1208,9 +1218,10 @@ static int update_parent_subparts_cpumask(struct cpuset *cpuset, int cmd, adding = cpumask_andnot(tmp->addmask, tmp->addmask, parent->subparts_cpus); /* - * Return error if the new effective_cpus could become empty. + * Return error if the new effective_cpus could become empty + * and there are tasks in the parent. */ - if (adding && + if (adding && parent->css.cgroup->nr_populated_csets && cpumask_equal(parent->effective_cpus, tmp->addmask)) { if (!deleting) return -EINVAL; @@ -2181,6 +2192,13 @@ static int cpuset_can_attach(struct cgroup_taskset *tset) (cpumask_empty(cs->cpus_allowed) || nodes_empty(cs->mems_allowed))) goto out_unlock;
+ /* + * On default hierarchy, task cannot be moved to a cpuset with empty + * effective cpus. + */ + if (is_in_v2_mode() && cpumask_empty(cs->effective_cpus)) + goto out_unlock; + cgroup_taskset_for_each(task, css, tset) { ret = task_can_attach(task, cs->cpus_allowed); if (ret)
Hello,
On Thu, Jun 03, 2021 at 05:24:14PM -0400, Waiman Long wrote:
@@ -2181,6 +2192,13 @@ static int cpuset_can_attach(struct cgroup_taskset *tset) (cpumask_empty(cs->cpus_allowed) || nodes_empty(cs->mems_allowed))) goto out_unlock;
- /*
* On default hierarchy, task cannot be moved to a cpuset with empty* effective cpus.*/- if (is_in_v2_mode() && cpumask_empty(cs->effective_cpus))
goto out_unlock;
This is inconsistent with how other events which leave a root partition empty is handled. Woudln't it be more consistent to switch the parent to PRS_ERROR and behave accordingly but allow it to have valid child roots?
Thanks.
On 6/16/21 4:57 PM, Tejun Heo wrote:
Hello,
On Thu, Jun 03, 2021 at 05:24:14PM -0400, Waiman Long wrote:
@@ -2181,6 +2192,13 @@ static int cpuset_can_attach(struct cgroup_taskset *tset) (cpumask_empty(cs->cpus_allowed) || nodes_empty(cs->mems_allowed))) goto out_unlock;
- /*
* On default hierarchy, task cannot be moved to a cpuset with empty* effective cpus.*/- if (is_in_v2_mode() && cpumask_empty(cs->effective_cpus))
goto out_unlock;This is inconsistent with how other events which leave a root partition empty is handled. Woudln't it be more consistent to switch the parent to PRS_ERROR and behave accordingly but allow it to have valid child roots?
From my point of view, PRS_ERROR is used when cpus are gone because of cpu hotplug (offline). It can be a temporary condition that will be corrected later on. I don't want to use PRS_ERROR for the particular case that the users have explicitly distributed out all the cpus to child partitions. I will clarify it in the next version and double check to make sure that this rule is consistently apply.
Thanks, Longman
Update Documentation/admin-guide/cgroup-v2.rst on the new "root-nolb" cpuset partition type as well as the ability to create non-top cpuset partition with no cpu allocated to it.
Signed-off-by: Waiman Long longman@redhat.com --- Documentation/admin-guide/cgroup-v2.rst | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst index b1e81aa8598a..36a923cabeb0 100644 --- a/Documentation/admin-guide/cgroup-v2.rst +++ b/Documentation/admin-guide/cgroup-v2.rst @@ -2010,8 +2010,9 @@ Cpuset Interface Files It accepts only the following input values when written to.
======== ================================ - "root" a partition root - "member" a non-root member of a partition + "member" Non-root member of a partition + "root" Partition root + "root-nolb" Partition root with no load balancing ======== ================================
When set to be a partition root, the current cgroup is the @@ -2020,6 +2021,10 @@ Cpuset Interface Files partition roots themselves and their descendants. The root cgroup is always a partition root.
+ With "root-nolb", the CPUs in that partition root will be in an + isolated state with no load balancing by the scheduler. Tasks in + such a partition must be explicitly bind to each individual CPU. + There are constraints on where a partition root can be set. It can only be set in a cgroup if all the following conditions are true. @@ -2038,9 +2043,12 @@ Cpuset Interface Files file cannot be reverted back to "member" if there are any child cgroups with cpuset enabled.
- A parent partition cannot distribute all its CPUs to its - child partitions. There must be at least one cpu left in the - parent partition. + A parent partition may distribute all its CPUs to its child + partitions as long as it is not the root cgroup and there is no + task directly associated with that parent partition. Otherwise, + there must be at least one cpu left in the parent partition. + A new task cannot be moved to a partition root with no effective + cpu.
Once becoming a partition root, changes to "cpuset.cpus" is generally allowed as long as the first condition above is true, @@ -2056,6 +2064,7 @@ Cpuset Interface Files ============== ============================== "member" Non-root member of a partition "root" Partition root + "root-nolb" Partition root with no load balancing "root invalid" Invalid partition root ============== ==============================
Add a test script for exercising the cpuset v2 partition root state code.
Signed-off-by: Waiman Long longman@redhat.com --- tools/testing/selftests/cgroup/Makefile | 2 +- .../selftests/cgroup/test_cpuset_prs.sh | 141 ++++++++++++++++++ 2 files changed, 142 insertions(+), 1 deletion(-) create mode 100755 tools/testing/selftests/cgroup/test_cpuset_prs.sh
diff --git a/tools/testing/selftests/cgroup/Makefile b/tools/testing/selftests/cgroup/Makefile index f027d933595b..e8ff3ffc3a43 100644 --- a/tools/testing/selftests/cgroup/Makefile +++ b/tools/testing/selftests/cgroup/Makefile @@ -4,7 +4,7 @@ CFLAGS += -Wall -pthread all:
TEST_FILES := with_stress.sh -TEST_PROGS := test_stress.sh +TEST_PROGS := test_stress.sh test_cpuset_prs.sh TEST_GEN_PROGS = test_memcontrol TEST_GEN_PROGS += test_kmem TEST_GEN_PROGS += test_core diff --git a/tools/testing/selftests/cgroup/test_cpuset_prs.sh b/tools/testing/selftests/cgroup/test_cpuset_prs.sh new file mode 100755 index 000000000000..6a9c02c301b5 --- /dev/null +++ b/tools/testing/selftests/cgroup/test_cpuset_prs.sh @@ -0,0 +1,141 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# Test for cpuset v2 partition root state (PRS) +# +# The sched verbose flag is set, if available, so that the console log +# can be examined for the correct setting of scheduling domain. +# + +skip_test() { + echo "$1" + echo "Test SKIPPED" + exit 0 +} + +[[ $(id -u) -eq 0 ]] || skip_test "Test must be run as root!" + +# Set sched verbose flag, if available +[[ -d /sys/kernel/debug/sched ]] && echo Y > /sys/kernel/debug/sched/verbose + +# Find cgroup v2 mount point +CGROUP2=$(mount | grep "^cgroup2" | awk -e '{print $3}') +[[ -n "$CGROUP2" ]] || skip_test "Cgroup v2 mount point not found!" + +CPUS=$(lscpu | grep "^CPU(s)" | sed -e "s/.*:[[:space:]]*//") +[[ $CPUS -lt 4 ]] && skip_test "Test needs at least 4 cpus available!" + +cd $CGROUP2 +echo +cpuset > cgroup.subtree_control +[[ -d test ]] || mkdir test +cd test +echo 2-3 > cpuset.cpus +TYPE=$(cat cpuset.cpus.partition) +[[ $TYPE = member ]] || echo member > cpuset.cpus.partition + +console_msg() +{ + MSG=$1 + echo "$MSG" + echo "" > /dev/console + echo "$MSG" > /dev/console + sleep 1 +} + +test_partition() +{ + EXPECTED_VAL=$1 + echo $EXPECTED_VAL > cpuset.cpus.partition + [[ $? -eq 0 ]] || exit 1 + ACTUAL_VAL=$(cat cpuset.cpus.partition) + [[ $ACTUAL_VAL != $EXPECTED_VAL ]] && { + echo "cpuset.cpus.partition: expect $EXPECTED_VAL, found $EXPECTED_VAL" + echo "Test FAILED" + exit 1 + } +} + +test_effective_cpus() +{ + EXPECTED_VAL=$1 + ACTUAL_VAL=$(cat cpuset.cpus.effective) + [[ "$ACTUAL_VAL" != "$EXPECTED_VAL" ]] && { + echo "cpuset.cpus.effective: expect '$EXPECTED_VAL', found '$EXPECTED_VAL'" + echo "Test FAILED" + exit 1 + } +} + +# Adding current process to cgroup.procs as a test +test_add_proc() +{ + OUTSTR="$1" + ERRMSG=$((echo $$ > cgroup.procs) |& cat) + echo $ERRMSG | grep -q "$OUTSTR" + [[ $? -ne 0 ]] && { + echo "cgroup.procs: expect '$OUTSTR', got '$ERRMSG'" + echo "Test FAILED" + exit 1 + } + echo $$ > $CGROUP2/cgroup.procs # Move out the task +} + +# +# Testing the new "root-nolb" partition root type +# +console_msg "Change from member to root" +test_partition root + +console_msg "Change from root to root-nolb" +test_partition root-nolb + +console_msg "Change from root-nolb to member" +test_partition member + +console_msg "Change from member to root-nolb" +test_partition root-nolb + +console_msg "Change from root-nolb to root" +test_partition root + +console_msg "Change from root to member" +test_partition member + +# +# Testing partition root with no cpu +# +console_msg "Distribute all cpus to child partition" +echo +cpuset > cgroup.subtree_control +test_partition root + +mkdir t1 +cd t1 +echo 2-3 > cpuset.cpus +test_partition root +test_effective_cpus 2-3 +cd .. +test_effective_cpus "" + +console_msg "Moving task to partition test" +test_add_proc "No space left" +cd t1 +test_add_proc "" +cd .. + +console_msg "Shrink and expand child partition" +cd t1 +echo 2 > cpuset.cpus +cd .. +test_effective_cpus 3 +cd t1 +echo 2-3 > cpuset.cpus +cd .. +test_effective_cpus "" + +# Cleaning up +console_msg "Cleaning up" +echo $$ > $CGROUP2/cgroup.procs +[[ -d t1 ]] && rmdir t1 +cd .. +rmdir test +echo "Test PASSED"
Hi Waiman,
On Thu, Jun 03, 2021 at 05:24:11PM -0400 Waiman Long wrote:
This patchset makes the following two major changes to the cpuset v2 code:
Patch 2: Add a new partition state "root-nolb" to create a partition root with load balancing disabled. This is for handling intermitten workloads that have a strict low latency requirement.
Patch 3: Allow partition roots that are not the top cpuset to distribute all its cpus to child partitions as long as there is no task associated with that partition root. This allows more flexibility for middleware to manage multiple partitions.
Thanks! This looks like it will be a usable replacement for the functionality lost when SD_LOAD_BALANCE went away.
Cheers, Phil
Patch 4 updates the cgroup-v2.rst file accordingly. Patch 5 adds a test to test the new cpuset partition code.
Waiman Long (5): cgroup/cpuset: Don't call validate_change() for some flag changes cgroup/cpuset: Add new cpus.partition type with no load balancing cgroup/cpuset: Allow non-top parent partition root to distribute out all CPUs cgroup/cpuset: Update description of cpuset.cpus.partition in cgroup-v2.rst kselftest/cgroup: Add cpuset v2 partition root state test
Documentation/admin-guide/cgroup-v2.rst | 19 ++- kernel/cgroup/cpuset.c | 124 +++++++++++---- tools/testing/selftests/cgroup/Makefile | 2 +- .../selftests/cgroup/test_cpuset_prs.sh | 141 ++++++++++++++++++ 4 files changed, 247 insertions(+), 39 deletions(-) create mode 100755 tools/testing/selftests/cgroup/test_cpuset_prs.sh
-- 2.18.1
linux-kselftest-mirror@lists.linaro.org
-
Peter Zijlstra -
Phil Auld -
Tejun Heo -
Waiman Long -
Waiman Long