Re: [PATCH 4/4] memcg: replace cgroup_lock with memcg specific memcg_lock
From: Michal Hocko <hidden>
Date: 2012-12-03 17:15:36
Also in:
linux-mm
On Fri 30-11-12 17:31:26, Glauber Costa wrote: [...]
+/*
+ * must be called with memcg_lock held, unless the cgroup is guaranteed to be
+ * already dead (like in mem_cgroup_force_empty, for instance).
+ */
+static inline bool memcg_has_children(struct mem_cgroup *memcg)
+{
+ return mem_cgroup_count_children(memcg) != 1;
+}Why not just keep list_empty(&cgrp->children) which is much simpler much more effective and correct here as well because cgroup cannot vanish while we are at the call because all callers come from cgroup fs? [...]
quoted hunk ↗ jump to hunk
@@ -3900,7 +3911,7 @@ static int mem_cgroup_hierarchy_write(struct cgroup *cont, struct cftype *cft, if (parent) parent_memcg = mem_cgroup_from_cont(parent); - cgroup_lock(); + mutex_lock(&memcg_lock); if (memcg->use_hierarchy == val) goto out;@@ -3915,7 +3926,7 @@ static int mem_cgroup_hierarchy_write(struct cgroup *cont, struct cftype *cft, */ if ((!parent_memcg || !parent_memcg->use_hierarchy) && (val == 1 || val == 0)) { - if (list_empty(&cont->children)) + if (!memcg_has_children(memcg)) memcg->use_hierarchy = val; else retval = -EBUSY;
Nothing prevents from a race when a task is on the way to be attached to the group. This means that we might miss some charges up the way to the parent. mem_cgroup_hierarchy_write cgroup_attach_task ss->can_attach() = mem_cgroup_can_attach mutex_lock(&memcg_lock) memcg->attach_in_progress++ mutex_unlock(&memcg_lock) __mem_cgroup_can_attach mem_cgroup_precharge_mc (*) mutex_lock(memcg_lock) memcg_has_children(memcg)==false cgroup_task_migrate memcg->use_hierarchy = val; ss->attach() (*) All the charches here are not propagated upwards. Fixable simply by testing attach_in_progress as well. The same applies to all other cases so it would be much better to prepare a common helper which does the whole magic. [...] Thanks -- Michal Hocko SUSE Labs