Re: [PATCH 3/4] memcg: split part of memcg creation to css_online
From: Michal Hocko <hidden>
Date: 2012-12-03 17:32:07
Also in:
cgroups
On Fri 30-11-12 17:31:25, Glauber Costa wrote:
Although there is arguably some value in doing this per se, the main goal of this patch is to make room for the locking changes to come. With all the value assignment from parent happening in a context where our iterators can already be used, we can safely lock against value change in some key values like use_hierarchy, without resorting to the cgroup core at all.
I am sorry but I really do not get why online_css callback is more appropriate. Quite contrary. With this change iterators can see a group which is not fully initialized which calls for a problem (even though it is not one yet). Could you be more specific why we cannot keep the initialization in mem_cgroup_css_alloc? We can lock there as well, no?
quoted hunk ↗ jump to hunk
Signed-off-by: Glauber Costa <redacted> --- mm/memcontrol.c | 52 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 17 deletions(-)diff --git a/mm/memcontrol.c b/mm/memcontrol.c index d80b6b5..b6d352f 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c@@ -5023,12 +5023,40 @@ mem_cgroup_css_alloc(struct cgroup *cont) INIT_WORK(&stock->work, drain_local_stock); } hotcpu_notifier(memcg_cpu_hotplug_callback, 0); - } else { - parent = mem_cgroup_from_cont(cont->parent); - memcg->use_hierarchy = parent->use_hierarchy; - memcg->oom_kill_disable = parent->oom_kill_disable; + + res_counter_init(&memcg->res, NULL); + res_counter_init(&memcg->memsw, NULL); } + memcg->last_scanned_node = MAX_NUMNODES; + INIT_LIST_HEAD(&memcg->oom_notify); + atomic_set(&memcg->refcnt, 1); + memcg->move_charge_at_immigrate = 0; + mutex_init(&memcg->thresholds_lock); + spin_lock_init(&memcg->move_lock); + + return &memcg->css; + +free_out: + __mem_cgroup_free(memcg); + return ERR_PTR(error); +} + +static int +mem_cgroup_css_online(struct cgroup *cont) +{ + struct mem_cgroup *memcg, *parent; + int error = 0; + + if (!cont->parent) + return 0; + + memcg = mem_cgroup_from_cont(cont); + parent = mem_cgroup_from_cont(cont->parent); + + memcg->use_hierarchy = parent->use_hierarchy; + memcg->oom_kill_disable = parent->oom_kill_disable; + if (parent && parent->use_hierarchy) { res_counter_init(&memcg->res, &parent->res); res_counter_init(&memcg->memsw, &parent->memsw);@@ -5050,15 +5078,8 @@ mem_cgroup_css_alloc(struct cgroup *cont) if (parent && parent != root_mem_cgroup) mem_cgroup_subsys.broken_hierarchy = true; } - memcg->last_scanned_node = MAX_NUMNODES; - INIT_LIST_HEAD(&memcg->oom_notify); - if (parent) - memcg->swappiness = mem_cgroup_swappiness(parent); - atomic_set(&memcg->refcnt, 1); - memcg->move_charge_at_immigrate = 0; - mutex_init(&memcg->thresholds_lock); - spin_lock_init(&memcg->move_lock); + memcg->swappiness = mem_cgroup_swappiness(parent); error = memcg_init_kmem(memcg, &mem_cgroup_subsys); if (error) {@@ -5068,12 +5089,8 @@ mem_cgroup_css_alloc(struct cgroup *cont) * call __mem_cgroup_free, so return directly */ mem_cgroup_put(memcg); - return ERR_PTR(error); } - return &memcg->css; -free_out: - __mem_cgroup_free(memcg); - return ERR_PTR(error); + return error; } static void mem_cgroup_css_offline(struct cgroup *cont)@@ -5702,6 +5719,7 @@ struct cgroup_subsys mem_cgroup_subsys = { .name = "memory", .subsys_id = mem_cgroup_subsys_id, .css_alloc = mem_cgroup_css_alloc, + .css_online = mem_cgroup_css_online, .css_offline = mem_cgroup_css_offline, .css_free = mem_cgroup_css_free, .can_attach = mem_cgroup_can_attach,-- 1.7.11.7
-- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>