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?
> Signed-off-by: Glauber Costa <glommer@xxxxxxxxxxxxx>
> ---
> 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 from this list: send the line "unsubscribe cgroups" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
