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

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

On Mon, 19 Jan 2009 15:27:38 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-01-19 17:49:22]:
> 
> > On Thu, 15 Jan 2009 14:07:51 +0800
> > Li Zefan <lizf@cn.fujitsu.com> wrote:
> > 
> > > 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().
> > > 
> > 
> > Maybe a patch like this is necessary.  But details are not fixed yet. 
> > Any comments are welcome.
> > 
> > (This is base on my CSS ID patch set.)
> > 
> > -Kame
> > ==
> > 
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > 
> > As Li Zefan pointed out, shrinking memcg's limit should return -EBUSY
> > after reasonable retries. This patch tries to fix the current behavior
> > of shrink_usage.
> > 
> > Before looking into "shrink should return -EBUSY" problem, we should fix
> > hierarchical reclaim code. It compares current usage and current limit,
> > but it only makes sense when the kernel reclaims memory because hit limits.
> > This is also a problem.
> > 
> > What this patch does are.
> > 
> >   1. add new argument "shrink" to hierarchical reclaim. If "shrink==true",
> >      hierarchical reclaim returns immediately and the caller checks the kernel
> >      should shrink more or not.
> >      (At shrinking memory, usage is always smaller than limit. So check for
> >       usage < limit is useless.)
> > 
> >   2. For adjusting to above change, 2 changes in "shrink"'s retry path.
> >      2-a. retry_count depends on # of children because the kernel visits
> > 	  the children under hierarchy one by one.
> >      2-b. rather than checking return value of hierarchical_reclaim's progress,
> > 	  compares usage-before-shrink and usage-after-shrink.
> > 	  If usage-before-shrink > usage-after-shrink, retry_count is
> > 	  decremented.
> 
> The code seems to do the reverse, it checks for
>         if (currusage >= oldusage)
> 
Ah, the text is wrong ;(

> > -		oldusage = res_counter_read_u64(&memcg->memsw, RES_USAGE);
> > -		mem_cgroup_hierarchical_reclaim(memcg, GFP_KERNEL, true);
> > +		mem_cgroup_hierarchical_reclaim(memcg, GFP_KERNEL, true, true);
> >  		curusage = res_counter_read_u64(&memcg->memsw, RES_USAGE);
> > +		/* Usage is reduced ? */
> >  		if (curusage >= oldusage)
> >  			retry_count--;
> > +		else
> > +			oldusage = curusage;
> >  	}
> >  	return ret;
> >  }
> 
> Has this been tested? It seems OK to the naked eye :)
> 
Thank you, and yes, tested.
I'll try to make this patch simpler and queue on my stack.

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>