From ee5e8472b8df54fc00031d3b1089d5be50fb2ce0 Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Fri, 22 Feb 2013 16:34:50 -0800 Subject: [PATCH] memcg: prevent changes to move_charge_at_immigrate during task attach In memcg, we use the cgroup_lock basically to synchronize against attaching new children to a cgroup. We do this because we rely on cgroup core to provide us with this information. We need to guarantee that upon child creation, our tunables are consistent. For those, the calls to cgroup_lock() all live in handlers like mem_cgroup_hierarchy_write(), where we change a tunable in the group that is hierarchy-related. For instance, the use_hierarchy flag cannot be changed if the cgroup already have children. Furthermore, those values are propagated from the parent to the child when a new child is created. So if we don't lock like this, we can end up with the following situation: A B memcg_css_alloc() mem_cgroup_hierarchy_write() copy use hierarchy from parent change use hierarchy in parent finish creation. This is mainly because during create, we are still not fully connected to the css tree. So all iterators and the such that we could use, will fail to show that the group has children. My observation is that all of creation can proceed in parallel with those tasks, except value assignment. So what this patch series does is to first move all value assignment that is dependent on parent values from css_alloc to css_online, where the iterators all work, and then we lock only the value assignment. This will guarantee that parent and children always have consistent values. Together with an online test, that can be derived from the observation that the refcount of an online memcg can be made to be always positive, we should be able to synchronize our side without the cgroup lock. This patch: Currently, we rely on the cgroup_lock() to prevent changes to move_charge_at_immigrate during task migration. However, this is only needed because the current strategy keeps checking this value throughout the whole process. Since all we need is serialization, one needs only to guarantee that whatever decision we made in the beginning of a specific migration is respected throughout the process. We can achieve this by just saving it in mc. By doing this, no kind of locking is needed. Signed-off-by: Glauber Costa Acked-by: Michal Hocko Cc: Tejun Heo Cc: Hiroyuki Kamezawa Cc: Johannes Weiner Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- mm/memcontrol.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index ae0433885b69..8e0988fee888 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -416,8 +416,8 @@ static bool memcg_kmem_test_and_clear_dead(struct mem_cgroup *memcg) /* Stuffs for move charges at task migration. */ /* - * Types of charges to be moved. "move_charge_at_immitgrate" is treated as a - * left-shifted bitmap of these types. + * Types of charges to be moved. "move_charge_at_immitgrate" and + * "immigrate_flags" are treated as a left-shifted bitmap of these types. */ enum move_type { MOVE_CHARGE_TYPE_ANON, /* private anonymous page and swap of it */ @@ -430,6 +430,7 @@ static struct move_charge_struct { spinlock_t lock; /* for from, to */ struct mem_cgroup *from; struct mem_cgroup *to; + unsigned long immigrate_flags; unsigned long precharge; unsigned long moved_charge; unsigned long moved_swap; @@ -442,14 +443,12 @@ static struct move_charge_struct { static bool move_anon(void) { - return test_bit(MOVE_CHARGE_TYPE_ANON, - &mc.to->move_charge_at_immigrate); + return test_bit(MOVE_CHARGE_TYPE_ANON, &mc.immigrate_flags); } static bool move_file(void) { - return test_bit(MOVE_CHARGE_TYPE_FILE, - &mc.to->move_charge_at_immigrate); + return test_bit(MOVE_CHARGE_TYPE_FILE, &mc.immigrate_flags); } /* @@ -5193,15 +5192,14 @@ static int mem_cgroup_move_charge_write(struct cgroup *cgrp, if (val >= (1 << NR_MOVE_TYPE)) return -EINVAL; + /* - * We check this value several times in both in can_attach() and - * attach(), so we need cgroup lock to prevent this value from being - * inconsistent. + * No kind of locking is needed in here, because ->can_attach() will + * check this value once in the beginning of the process, and then carry + * on with stale data. This means that changes to this value will only + * affect task migrations starting after the change. */ - cgroup_lock(); memcg->move_charge_at_immigrate = val; - cgroup_unlock(); - return 0; } #else @@ -6559,8 +6557,15 @@ static int mem_cgroup_can_attach(struct cgroup *cgroup, struct task_struct *p = cgroup_taskset_first(tset); int ret = 0; struct mem_cgroup *memcg = mem_cgroup_from_cont(cgroup); + unsigned long move_charge_at_immigrate; - if (memcg->move_charge_at_immigrate) { + /* + * We are now commited to this value whatever it is. Changes in this + * tunable will only affect upcoming migrations, not the current one. + * So we need to save it, and keep it going. + */ + move_charge_at_immigrate = memcg->move_charge_at_immigrate; + if (move_charge_at_immigrate) { struct mm_struct *mm; struct mem_cgroup *from = mem_cgroup_from_task(p); @@ -6580,6 +6585,7 @@ static int mem_cgroup_can_attach(struct cgroup *cgroup, spin_lock(&mc.lock); mc.from = from; mc.to = memcg; + mc.immigrate_flags = move_charge_at_immigrate; spin_unlock(&mc.lock); /* We set mc.moving_task later */ -- 2.20.1