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

KAMEZAWA Hiroyuki
2009-06-15T08:18:18Z

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 ?).

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