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

Daisuke Nishimura
2009-06-15T02:53:27Z

On Fri, 12 Jun 2009 15:19:24 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Fri, 12 Jun 2009 14:33:46 +0900
> Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:
>
> > Hi.
> >
> > I found a problem about rmdir: rmdir doesn't return(or take a very very long time).
> > Actually, I found this problem long ago, but I've not had enough time to
> > track it down until the stale swap cache problem has been fixed.
> >
> > The cause of this problem is the commit ec64f51545fffbc4cb968f0cea56341a4b07e85a
> > (cgroup: fix frequent -EBUSY at rmdir) and memcg's behavior about swap-in.
> >
> > The commit introduced cgroup_rmdir_waitq and make rmdir wait until someone
> > (who will decrement css->refcnt to 1) wake it up.
> > But even after we have succeeded pre_destroy, which means mem.usage has
> > become 0, a process which has moved to another cgroup from the cgroup being removed
> > can increment mem.usage(and css->refcnt as a result) by doing swap-in.
> > This css->refcnt won't be dropped, that is the rmdir process won't be woken up,
> > until the owner process frees the page.
> >
> > So, just "waking up after a while" by a patch below can fix this problem.
> >
> > ===
> > diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> > index 3737a68..2fe9645 100644
> > --- a/kernel/cgroup.c
> > +++ b/kernel/cgroup.c
> > @@ -2722,7 +2722,7 @@ again:
> >
> > if (!cgroup_clear_css_refs(cgrp)) {
> > mutex_unlock(&cgroup_mutex);
> > - schedule();
> > + schedule_timeout(HZ/10); /* don't wait forever */
> > finish_wait(&cgroup_rmdir_waitq, &wait);
> > clear_bit(CGRP_WAIT_ON_RMDIR, &cgrp->flags);
> > if (signal_pending(current))
> > ===
> >
> This is not a choice, maybe.
>
>
>
> > But, is there any reason why we should charge a NEW swap-in'ed page to
> > "the group to which the swap has been charged", not to "the group in which
> > the process is now" ?
> > I agree that we should uncharge "swap" at swap-in from "the group to which
> > the swap has been charged", but IIUC, memcg before/without mem+swap controller behaves
> > as the latter about the charge of a swap-in'ed page.
> >
> I have no objection to this direction. But this implies the resouce usage
> can be moved from a cgroup to other silently.
> But this bahavior is not different from behavior of page caches, I think
> this one is a choice.
>
> This happens only when swapped-out pages are swapped-in by a process in other
> cgroup. Maybe rare case.
>
It would be rare case if rmdir is executed after all the tasks have exited,
but I think it would not be so rare if we do "move tasks to another cgroup
and rmdir the old cgroup".

>
>
> > I've confirmed that a patch below can also fix this rmdir problem.
> >
> > ===
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 6ceb6f2..dbece65 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -1063,7 +1063,7 @@ static struct mem_cgroup *mem_cgroup_lookup(unsigned short id)
> >
> > static struct mem_cgroup *try_get_mem_cgroup_from_swapcache(struct page *page)
> > {
> > - struct mem_cgroup *mem;
> > + struct mem_cgroup *mem = NULL;
> > struct page_cgroup *pc;
> > unsigned short id;
> > swp_entry_t ent;
> > @@ -1079,14 +1079,6 @@ static struct mem_cgroup *try_get_mem_cgroup_from_swapcache(struct page *page)
> > mem = pc->mem_cgroup;
> > if (mem && !css_tryget(&mem->css))
> > mem = NULL;
> > - } else {
> > - ent.val = page_private(page);
> > - id = lookup_swap_cgroup(ent);
> > - rcu_read_lock();
> > - mem = mem_cgroup_lookup(id);
> > - if (mem && !css_tryget(&mem->css))
> > - mem = NULL;
> > - rcu_read_unlock();
> > }
> > unlock_page_cgroup(pc);
> > return mem;
> > ===
> >
> >
> > Any suggestions ?
> >
>
> After this, swap-cache behavior will be highly complecated ;(
>
> - If swap-cache is newly swapped-in, it's charged to current user and resource
> usage moves.
> - If swap-cache is used (or unmapped recently), it's charged to old user and
> resource usage don't move.
>
> Then, my suggestion is here.
> ==
> } else {
> ent.val = page_private(page);
> id = lookup_swap_cgroup(ent);
> rcu_read_lock();
> mem = mem_cgroup_lookup(id);
> if (mem) {
> if (css_tryget(mem->css)) {
> /*
> * If no processes in this cgroup, accounting back to
> * this cgroup seems silly and prevents RMDIR.
> */
> struct cgroup *cg = mem->css.cgroup;
> if (!atomic_read(&cg->count) && list_empty(&cg->children)) {
> css_put(&mem->css);
> mem = NULL;
> }
> }
> rcu_read_unlock();
> }
> ==
>
Thank you for your suggestion.
To be honest, I think swap cache behavior would be complicated anyway :(

I prefer my change because the behavior would become consistent with
the case we don't use mem+swap controller and with the behavior of page cache.

Thanks,
Daisuke Nishimura.