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

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

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.



> 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();
 }
==

nonsense ?

Thanks,
-Kame





--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>