On Thu 23-10-14 10:42:07, Johannes Weiner wrote: > Having these functions and their documentation split out and somewhere > makes it harder, not easier, to follow what's going on. > > Inline them directly where charge moving is prepared and finished, and > put an explanation right next to it. I do not see the open coded version much more readable or maintainable to be honest. mem_cgroup_{start,end}_move are a good markers of the transaction. But I do not have strong opinion about this. The preparation and move_lock parts are so far from each other that either way would be non-trivial to follow. That being said, I do not really care much. Your comment is better and more clear though. > Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx> > --- > mm/memcontrol.c | 40 ++++++++++++---------------------------- > 1 file changed, 12 insertions(+), 28 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 3cd4f1e0bfb3..5b5c784dc39d 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1447,32 +1447,6 @@ int mem_cgroup_swappiness(struct mem_cgroup *memcg) > } > > /* > - * memcg->moving_account is used for checking possibility that some thread is > - * calling move_account(). When a thread on CPU-A starts moving pages under > - * a memcg, other threads should check memcg->moving_account under > - * rcu_read_lock(), like this: > - * > - * CPU-A CPU-B > - * rcu_read_lock() > - * memcg->moving_account+1 if (memcg->mocing_account) > - * take heavy locks. > - * synchronize_rcu() update something. > - * rcu_read_unlock() > - * start move here. > - */ > - > -static void mem_cgroup_start_move(struct mem_cgroup *memcg) > -{ > - atomic_inc(&memcg->moving_account); > - synchronize_rcu(); > -} > - > -static void mem_cgroup_end_move(struct mem_cgroup *memcg) > -{ > - atomic_dec(&memcg->moving_account); > -} > - > -/* > * A routine for checking "mem" is under move_account() or not. > * > * Checking a cgroup is mc.from or mc.to or under hierarchy of > @@ -5431,7 +5405,8 @@ static void mem_cgroup_clear_mc(void) > mc.from = NULL; > mc.to = NULL; > spin_unlock(&mc.lock); > - mem_cgroup_end_move(from); > + > + atomic_dec(&memcg->moving_account); > } > > static int mem_cgroup_can_attach(struct cgroup_subsys_state *css, > @@ -5464,7 +5439,16 @@ static int mem_cgroup_can_attach(struct cgroup_subsys_state *css, > VM_BUG_ON(mc.precharge); > VM_BUG_ON(mc.moved_charge); > VM_BUG_ON(mc.moved_swap); > - mem_cgroup_start_move(from); > + > + /* > + * Signal mem_cgroup_begin_page_stat() to take > + * the memcg's move_lock while we're moving > + * its pages to another memcg. Then wait for > + * already started RCU-only updates to finish. > + */ > + atomic_inc(&memcg->moving_account); > + synchronize_rcu(); > + > spin_lock(&mc.lock); > mc.from = from; > mc.to = memcg; > -- > 2.1.2 > -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a>