Re: [RFC] [PATCH] memcg: fix infinite loop

[Balbir Singh <balbir@linux.vnet.ibm.com>]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-01-15 16:21:26]:

> On Thu, 15 Jan 2009 15:14:38 +0800
> Li Zefan <lizf@cn.fujitsu.com> wrote:
> 
> > KAMEZAWA Hiroyuki wrote:
> > > On Thu, 15 Jan 2009 11:45:57 +0530
> > > Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> > > 
> > >> * Li Zefan <lizf@cn.fujitsu.com> [2009-01-15 14:07:51]:
> > >>
> > >>> 1. task p1 is in /memcg/0
> > >>> 2. p1 does mmap(4096*2, MAP_LOCKED)
> > >>> 3. echo 4096 > /memcg/0/memory.limit_in_bytes
> > >>>
> > >>> The above 'echo' will never return, unless p1 exited or freed the memory.
> > >>> The cause is we can't reclaim memory from p1, so the while loop in
> > >>> mem_cgroup_resize_limit() won't break.
> > >>>
> > >>> This patch fixes it by decrementing retry_count regardless the return value
> > >>> of mem_cgroup_hierarchical_reclaim().
> > >>>
> > >> The problem definitely seems to exist, shouldn't we fix reclaim to
> > >> return 0, so that we know progress is not made and retry count
> > >> decrements? 
> > >>
> > > 
> > > The behavior is correct. And we already check signal_pending() in the loop.
> > > Ctrl-C or SIGALARM will works better than checking retry count.
> > 
> > But this behavior seems like a regression. Please try it in 2.6.28, you'll see
> > it returns EBUSY immediately.
> > 
> > Looks like the return value of mem_cgroup_hierarchical_reclaim() is buggy ?
> > 
> 
> This is intentional behavior change by
> ==
>  memcg-make-oom-less-frequently.patch
> ==
> 
> try_to_free_page() returns positive value if try_to_free_page() reclaims at
> least 1 pages. It itself doesn't seem to be buggy.
> 
> What buggy is resize_limit's retry-out check code, I think.
> 
> How about following ?
> ==
> 	while (1) {
> 		if (signal_pending())
> 			break;
> 		try to set limit ....
> 		...
> 		ret = mem_cgroup_hierarchical_reclaim(memcg,  GFP_KERNEL, false);
> 		total_progress += ret;	
> 
> 		if (total_progress > (memcg->res.usage - val) * 2) {
> 			/*
> 			 * It seems we reclaimed twice of necessary
> 			 * pages...this memcg is busy
> 			 */
> 			ret = -EBUSY;
> 			break;

I think we need the nr_retries here as well, we should refuse to
resize_limit beyond a certain number of retries. In the case that Li
mentioned total_progress will be 0, since we cannot reclaim anything.
I prefer a nr_retries based approach for failure.

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

-- 
	Balbir

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