| Message ID | 20210603212416.25934-3-longman@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cgroup/cpuset: Enable cpuset partition with no load balancing | expand |
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 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 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
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.
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.
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
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;
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(-)