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
--
2.18.1
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 <[email protected]>
---
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;
--
2.18.1
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 <[email protected]>
---
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);
--
2.18.1
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 <[email protected]>
---
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
============== ==============================
--
2.18.1
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 <[email protected]>
---
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)
--
2.18.1
Add a test script for exercising the cpuset v2 partition root state code.
Signed-off-by: Waiman Long <[email protected]>
---
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"
--
2.18.1
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
>
--
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 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,
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.
--
tejun
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;
> 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;
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.
--
tejun
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.
--
tejun
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
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
> ^ing
>
> and "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;
>> 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;
> 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
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