Re: [PATCH REPOST RFC cgroup/for-3.7] cgroup: mark subsystems with broken hierarchy support and whine if cgroups are nested for them
From: Michal Hocko <hidden>
Date: 2012-09-11 10:04:42
Also in:
lkml
Subsystem:
control group - memory resource controller (memcg), memory management, the rest · Maintainers:
Johannes Weiner, Michal Hocko, Roman Gushchin, Shakeel Butt, Andrew Morton, Linus Torvalds
Possibly related (same subject, not in this thread)
- 2012-09-17 · Re: [PATCH REPOST RFC cgroup/for-3.7] cgroup: mark subsystems with broken hierarchy support and whine if cgroups are nested for them · Glauber Costa <hidden>
- 2012-09-14 · Re: [PATCH REPOST RFC cgroup/for-3.7] cgroup: mark subsystems with broken hierarchy support and whine if cgroups are nested for them · Tejun Heo <tj@kernel.org>
- 2012-09-14 · Re: [PATCH REPOST RFC cgroup/for-3.7] cgroup: mark subsystems with broken hierarchy support and whine if cgroups are nested for them · Glauber Costa <hidden>
- 2012-09-13 · Re: [PATCH REPOST RFC cgroup/for-3.7] cgroup: mark subsystems with broken hierarchy support and whine if cgroups are nested for them · Michal Hocko <hidden>
- 2012-09-13 · Re: [PATCH REPOST RFC cgroup/for-3.7] cgroup: mark subsystems with broken hierarchy support and whine if cgroups are nested for them · Tejun Heo <tj@kernel.org>
I like the approach in general but see the comments bellow: On Mon 10-09-12 15:33:55, Tejun Heo wrote: [...]
quoted hunk ↗ jump to hunk
--- a/mm/memcontrol.c +++ b/mm/memcontrol.c@@ -3855,12 +3855,17 @@ static int mem_cgroup_hierarchy_write(st */ if ((!parent_memcg || !parent_memcg->use_hierarchy) && (val == 1 || val == 0)) { - if (list_empty(&cont->children)) + if (list_empty(&cont->children)) { memcg->use_hierarchy = val; - else + /* we're fully hierarchical iff root uses hierarchy */ + if (mem_cgroup_is_root(memcg)) + mem_cgroup_subsys.broken_hierarchy = !val; + } else { retval = -EBUSY; - } else + } + } else { retval = -EINVAL; + } out: cgroup_unlock();@@ -4953,6 +4958,7 @@ mem_cgroup_create(struct cgroup *cont) &per_cpu(memcg_stock, cpu); INIT_WORK(&stock->work, drain_local_stock); } + mem_cgroup_subsys.broken_hierarchy = !memcg->use_hierarchy;
Hmmm, this will warn even if we have
root (default use_hierarchy=0)
\
A (use_hierarchy=1)
\
B <- here
which is unfortunate because it will add a noise to a reasonable
configuration.
I think this is fixable if you move the warning after
cgroup_subsys_state::create and broken_hierarchy would be set only if
parent is not root and use_hierarchy==0 in mem_cgroup_create. Something
like:
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 795e525..d5c93ab 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c@@ -4973,6 +4973,13 @@ mem_cgroup_create(struct cgroup *cont) } else { res_counter_init(&memcg->res, NULL); res_counter_init(&memcg->memsw, NULL); + /* + * Deeper hierachy with use_hierarchy == false doesn't make + * much sense so let cgroup subsystem know about this unfortunate + * state in our controller. + */ + if (parent && parent != root_mem_cgroup) + mem_cgroup_subsys.broken_hierarchy = true; } memcg->last_scanned_node = MAX_NUMNODES; INIT_LIST_HEAD(&memcg->oom_notify);
What do you think?
quoted hunk ↗ jump to hunk
hotcpu_notifier(memcg_cpu_hotplug_callback, 0); } else { parent = mem_cgroup_from_cont(cont->parent);--- a/net/core/netprio_cgroup.c +++ b/net/core/netprio_cgroup.c@@ -330,7 +330,17 @@ struct cgroup_subsys net_prio_subsys = { .subsys_id = net_prio_subsys_id, #endif .base_cftypes = ss_files, - .module = THIS_MODULE + .module = THIS_MODULE, + + /* + * net_prio has artificial limit on the number of cgroups and + * disallows nesting making it impossible to co-mount it with other + * hierarchical subsystems. Remove the artificially low PRIOIDX_SZ + * limit and properly nest configuration such that children follow + * their parents' configurations by default and are allowed to + * override and remove the following. + */ + .broken_hierarchy = trye,
typo -- Michal Hocko SUSE Labs