Messages in this thread | | | Date | Tue, 3 May 2022 14:39:23 -0400 | Subject | Re: [PATCH v10 3/8] cgroup/cpuset: Allow no-task partition to have empty cpuset.cpus.effective | From | Waiman Long <> |
| |
On 5/3/22 13:54, Phil Auld wrote: > Hi Waiman > > On Tue, May 03, 2022 at 12:21:44PM -0400 Waiman Long wrote: >> Currently, a partition root cannot have empty "cpuset.cpus.effective". >> As a result, a parent partition root cannot distribute out all its CPUs >> to child partitions with no CPUs left. However in most cases, there >> shouldn't be any tasks associated with intermediate nodes of the default >> hierarchy. So the current rule is too restrictive and can waste valuable >> CPU resource. >> >> To address this issue, we are now allowing a partition to have empty >> "cpuset.cpus.effective" as long as it has no task. Therefore, a parent >> partition with no task can now have all its CPUs distributed out to its >> child partitions. The top cpuset always have some house-keeping tasks >> running and so its list of effective cpu can't never be empty. > s/never/ever/ It is a double negative. I think I will just remove "never". >> Once a partition with empty "cpuset.cpus.effective" is formed, no >> new task can be moved into it until "cpuset.cpus.effective" becomes >> non-empty. >> >> Signed-off-by: Waiman Long <longman@redhat.com> >> --- >> kernel/cgroup/cpuset.c | 111 +++++++++++++++++++++++++++++++---------- >> 1 file changed, 84 insertions(+), 27 deletions(-) >> >> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c >> index d156a39d7a08..7d9abd50a1b9 100644 >> --- a/kernel/cgroup/cpuset.c >> +++ b/kernel/cgroup/cpuset.c >> @@ -412,6 +412,41 @@ static inline bool is_in_v2_mode(void) >> (cpuset_cgrp_subsys.root->flags & CGRP_ROOT_CPUSET_V2_MODE); >> } >> >> +/** >> + * partition_is_populated - check if partition has tasks >> + * @cs: partition root to be checked >> + * @excluded_child: a child cpuset to be excluded in task checking >> + * Return: true if there are tasks, false otherwise >> + * >> + * It is assumed that @cs is a valid partition root. @excluded_child should >> + * be non-NULL when this cpuset is going to become a partition itself. >> + */ >> +static inline bool partition_is_populated(struct cpuset *cs, >> + struct cpuset *excluded_child) >> +{ >> + struct cgroup_subsys_state *css; >> + struct cpuset *child; >> + >> + if (cs->css.cgroup->nr_populated_csets) >> + return true; >> + if (!excluded_child && !cs->nr_subparts_cpus) >> + return cgroup_is_populated(cs->css.cgroup); >> + >> + rcu_read_lock(); >> + cpuset_for_each_child(child, css, cs) { >> + if (child == excluded_child) >> + continue; >> + if (is_partition_valid(child)) >> + continue; >> + if (cgroup_is_populated(child->css.cgroup)) { >> + rcu_read_unlock(); >> + return true; >> + } >> + } >> + rcu_read_unlock(); >> + return false; >> +} >> + >> /* >> * Return in pmask the portion of a task's cpusets's cpus_allowed that >> * are online and are capable of running the task. If none are found, >> @@ -1252,22 +1287,25 @@ static int update_parent_subparts_cpumask(struct cpuset *cs, int cmd, >> if ((cmd != partcmd_update) && css_has_online_children(&cs->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(cs->cpus_allowed, parent->effective_cpus) || >> - cpumask_equal(cs->cpus_allowed, parent->effective_cpus))) >> - return -EINVAL; >> - >> - /* >> - * A cpumask update cannot make parent's effective_cpus become empty. >> - */ >> adding = deleting = false; >> old_prs = new_prs = cs->partition_root_state; >> if (cmd == partcmd_enable) { >> + /* >> + * Enabling partition root is not allowed if not all the CPUs >> + * can be granted from parent's effective_cpus. >> + */ >> + if (!cpumask_subset(cs->cpus_allowed, parent->effective_cpus)) >> + return -EINVAL; >> + >> + /* >> + * A parent can be left with no CPU as long as there is no >> + * task directly associated with the parent partition. For >> + * such a parent, no new task can be moved into it. >> + */ >> + if (partition_is_populated(parent, cs) && >> + cpumask_equal(cs->cpus_allowed, parent->effective_cpus)) >> + return -EINVAL; >> + > You might consider switching these around to check the cpumasks first. Good point, partition_is_populated() is more expensive. > > >> cpumask_copy(tmp->addmask, cs->cpus_allowed); >> adding = true; >> } else if (cmd == partcmd_disable) { >> @@ -1289,9 +1327,10 @@ static int update_parent_subparts_cpumask(struct cpuset *cs, 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 && partition_is_populated(parent, cs) && >> cpumask_equal(parent->effective_cpus, tmp->addmask)) { > Same. Thanks, Longman
| |