Re: [RFC][BUGFIX] memcg: rmdir doesn't return

Daisuke Nishimura
2009-06-16T03:12:55Z

On Mon, 15 Jun 2009 17:17:15 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Mon, 15 Jun 2009 12:02:13 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > I don't like implict resource move. I'll try some today. plz see it.
> > _But_ this case just happens when swap is shared between cgroups and _very_ heavy
> > swap-in continues very long. I don't think this is a fatal and BUG.
> >
> > But ok, maybe wake-up path is not enough.
> >
> Here.
> Anyway, there is an unfortunate complexity in cgroup's rmdir() path.
> I think this will remove all concern in
> pre_destroy -> check -> start rmdir path
> if subsys is aware of what they does.
> Usual subsys just consider "tasks" and no extra references I hope.
> If your test result is good, I'll post again (after merge window ?).
>
Thank you for your patch.

At first, I thought this problem can be solved by this direction, but
there is a race window yet.

The root cause of this problem is that mem.usage can be incremented
by swap-in behavior of memcg even after it has become 0 once.
So, mem.usage can also be incremented between cgroup_need_restart_rmdir()
and schedule().
I can see rmdir being locked up actually in my test.

hmm, sleeping until being waken up might not be good if we don't change
swap-in behavior of memcg in some way.

Thanks,
Daisuke Nishimura.

> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
> Cgroup is designed for do some work against _tasks_. But when it comes to
> memcg, a cgroup can be obtained by something other...i.e. page and swap entry.
> Then, pre_destroy at el. are provided. Historically, there are some races
> around this...this is new one.
>
> Now, rmdir() path uses following logic.
>
> pre_destroy(); # drop all css->refcnt to be 0.
> lock cgroup mutex # no new task after this
> check cgroup has no tasks.
> check cgroup has no children.
> check css refcnt
> (*) if refcnt is not 0, sleep and wait for refcnt goes down to 0.
>
> The logic (*) assumes the refcnt will goes down soon, but in some case(memcg),
> it's better to call pre_destroy() again if pre_destroy() can handle it.
> (The most unfortunate in above logic is that we can't have some trustable
> lock in this path..but..we may never be able to do.)
>
> This patch adds ss->restart_rmdir() callback to subsys and allow immediate
> retry of pre_destroy() if necessary.
>
> Reported-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
> Index: linux-2.6.30.org/include/linux/cgroup.h
> ===================================================================
> --- linux-2.6.30.org.orig/include/linux/cgroup.h
> +++ linux-2.6.30.org/include/linux/cgroup.h
> @@ -374,6 +374,7 @@ struct cgroup_subsys {
> struct cgroup_subsys_state *(*create)(struct cgroup_subsys *ss,
> struct cgroup *cgrp);
> int (*pre_destroy)(struct cgroup_subsys *ss, struct cgroup *cgrp);
> + bool (*restart_rmdir)(struct cgroup_subsys *ss, struct cgroup *cgrp);
> void (*destroy)(struct cgroup_subsys *ss, struct cgroup *cgrp);
> int (*can_attach)(struct cgroup_subsys *ss,
> struct cgroup *cgrp, struct task_struct *tsk);
> Index: linux-2.6.30.org/kernel/cgroup.c
> ===================================================================
> --- linux-2.6.30.org.orig/kernel/cgroup.c
> +++ linux-2.6.30.org/kernel/cgroup.c
> @@ -635,6 +635,23 @@ static int cgroup_call_pre_destroy(struc
> }
> return ret;
> }
> +/*
> + * Check we have to restart rmdir immediately or not. Because we don't have any
> + * system which prevents "new reference comes after pre_destroy", we checks
> + * whether we have to call pre_destroy() again or not.
> + * i.e. if css_get()'s refcnt is not a temporal one, we can't expect css_put()
> + * is called and need to call pre_destroy().
> + */
> +static bool cgroup_need_restart_rmdir(struct cgroup *cgrp)
> +{
> + struct cgroup_subsys *ss;
> +
> + for_each_subsys(cgrp->root, ss)
> + if (ss->restart_rmdir)
> + if (ss->restart_rmdir(ss, cgrp))
> + return true;
> + return false;
> +}
>
> static void free_cgroup_rcu(struct rcu_head *obj)
> {
> @@ -2705,7 +2722,8 @@ again:
>
> if (!cgroup_clear_css_refs(cgrp)) {
> mutex_unlock(&cgroup_mutex);
> - schedule();
> + if (!cgroup_need_restart_rmdir(cgrp))
> + schedule();
> finish_wait(&cgroup_rmdir_waitq, &wait);
> clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
> if (signal_pending(current))
> Index: linux-2.6.30.org/mm/memcontrol.c
> ===================================================================
> --- linux-2.6.30.org.orig/mm/memcontrol.c
> +++ linux-2.6.30.org/mm/memcontrol.c
> @@ -2462,6 +2462,18 @@ static int mem_cgroup_pre_destroy(struct
> return mem_cgroup_force_empty(mem, false);
> }
>
> +static bool mem_cgroup_restart_rmdir(struct cgroup_subsys *ss,
> + struct cgroup *cont)
> +{
> + struct mem_cgroup *mem = mem_cgroup_from_cont(cont);
> + unsigned long long usage;
> +
> + usage = res_counter_read_u64(&mem->res, RES_USAGE);
> + if (usage)/* some charge after pre_destroy() (via swap)....*/
> + return true;
> + return false;
> +}
> +
> static void mem_cgroup_destroy(struct cgroup_subsys *ss,
> struct cgroup *cont)
> {
> @@ -2501,6 +2513,7 @@ struct cgroup_subsys mem_cgroup_subsys =
> .subsys_id = mem_cgroup_subsys_id,
> .create = mem_cgroup_create,
> .pre_destroy = mem_cgroup_pre_destroy,
> + .restart_rmdir = mem_cgroup_restart_rmdir,
> .destroy = mem_cgroup_destroy,
> .populate = mem_cgroup_populate,
> .attach = mem_cgroup_move_task,