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 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
