Re: Commit f50de2d38 seems to be breaking my oom killer

[Minchan Kim <minchan.kim@gmail.com>]

On Fri,  8 Jan 2010 13:08:46 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > > Hi, Mel 
> > > 
> > > On Thu, 7 Jan 2010 13:58:31 +0000
> > > Mel Gorman <mel@csn.ul.ie> wrote:
> > > 
> > > > vmscan: kswapd should notice that all zones are not ok if they are unreclaimble
> > > > 
> > > > In the event all zones are unreclaimble, it is possible for kswapd to
> > > > never go to sleep because "all zones are ok even though watermarks are
> > > > not reached". It gets into a situation where cond_reched() is not
> > > > called.
> > > > 
> > > > This patch notes that if all zones are unreclaimable then the zones are
> > > > not ok and cond_resched() should be called.
> > > > 
> > > > Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> > > > --- 
> > > >  mm/vmscan.c |    4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > > index 2ad8603..d3c0848 100644
> > > > --- a/mm/vmscan.c
> > > > +++ b/mm/vmscan.c
> > > > @@ -2022,8 +2022,10 @@ loop_again:
> > > >  				break;
> > > >  			}
> > > >  		}
> > > > -		if (i < 0)
> > > > +		if (i < 0) {
> > > > +			all_zones_ok = 0;
> > > >  			goto out;
> > > > +		}
> > > >  
> > > >  		for (i = 0; i <= end_zone; i++) {
> > > >  			struct zone *zone = pgdat->node_zones + i;
> > > > 
> > > > --
> > > 
> > > Nice catch!
> > > Don't we care following as although it is rare case?
> > > 
> > > ---
> > >                 for (i = 0; i <= end_zone; i++) {
> > >                         struct zone *zone = pgdat->node_zones + i; 
> > >                         int nr_slab;
> > >                         int nid, zid; 
> > > 
> > >                         if (!populated_zone(zone))
> > >                                 continue;
> > > 
> > >                         if (zone_is_all_unreclaimable(zone) &&
> > >                                         priority != DEF_PRIORITY)
> > >                                 continue;  <==== here
> > > 
> > > ---
> > > 
> > > And while I review all_zones_ok'usage in balance_pgdat, 
> > > I feel it's not consistent and rather confused. 
> > > How about this?
> > 
> > Can you please read my patch?
> 
> Grr. I'm sorry. such thread don't CCed LKML.
> cut-n-past here.
> 
> 
> ----------------------------------------
> Umm..
> This code looks a bit risky. Please imazine asymmetric numa. If the system has
> very small node, its nude have unreclaimable state at almost time.
> 
> Thus, if all zones in the node are unreclaimable, It should be slept. To retry balance_pgdat()
> is meaningless. this is original intention, I think.
> 
> So why can't we write following?
> 
> From c00d7bb29552d3aa4d934b5007f3d52ade5f2dfd Mon Sep 17 00:00:00 2001
> From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Date: Fri, 8 Jan 2010 08:36:05 +0900
> Subject: [PATCH] vmscan: kswapd don't retry balance_pgdat() if all zones are unreclaimable
> 
> Commit f50de2d3 (vmscan: have kswapd sleep for a short interval and
> double check it should be asleep) can cause kswapd to enter an infinite
> loop if running on a single-CPU system. If all zones are unreclaimble,
> sleeping_prematurely return 1 and kswapd will call balance_pgdat()
> again. but it's totally meaningless, balance_pgdat() doesn't anything
> against unreclaimable zone!
> 
> Cc: Mel Gorman <mel@csn.ul.ie>
> Reported-by: Will Newton <will.newton@gmail.com>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Looks good than mine. Thnaks, Kosaki. 
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>


-- 
Kind regards,
Minchan Kim

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