Date:	Wed, 31 Dec 2008 12:11:31 +0000
From:	Mel Gorman <mel@....ul.ie>
To:	Nick Piggin <npiggin@...e.de>
Cc:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	LKML <linux-kernel@...r.kernel.org>,
	linux-mm <linux-mm@...ck.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	wassim dagash <wassim.dagash@...il.com>
Subject: Re: [PATCH] mm: stop kswapd's infinite loop at high order
	allocation

On Wed, Dec 31, 2008 at 12:16:47PM +0100, Nick Piggin wrote:
> On Wed, Dec 31, 2008 at 11:06:19AM +0000, Mel Gorman wrote:
> > On Wed, Dec 31, 2008 at 02:32:33AM +0100, Nick Piggin wrote:
> > > On Tue, Dec 30, 2008 at 06:59:19PM +0000, Mel Gorman wrote:
> > > > On Tue, Dec 30, 2008 at 07:55:47PM +0900, KOSAKI Motohiro wrote:
> > > > kswapd gets a sc.order when it is known there is a process trying to get
> > > > high-order pages so it can reclaim at that order in an attempt to prevent
> > > > future direct reclaim at a high-order. Your patch does not appear to depend on
> > > > GFP_KERNEL at all so I found the comment misleading. Furthermore, asking it to
> > > > loop again at order-0 means it may scan and reclaim more memory unnecessarily
> > > > seeing as all_zones_ok was calculated based on a high-order value, not order-0.
> > > 
> > > It shouldn't, because it should check all that.
> > > 
> > 
> > Ok, with KOSAKI's patch we
> > 
> > 1. Set order to 0 (and stop kswapd doing what it was asked to do)
> > 2. goto loop_again
> > 3. nr_reclaimed gets set to 0 (meaning we lose that value, but no biggie
> >    as it doesn't get used by the caller anyway)
> > 4. Reset all priorities
> > 5. Do something like the following
> > 
> > 	for (priority = DEF_PRIORITY; priority >= 0; priority--) {
> > 		...
> > 		all_zones_ok = 1;
> > 		for (i = pgdat->nr_zones - 1; i >= 0; i--) {
> > 			...
> > 			if (inactive_anon_is_low(zone)) {
> > 				shrink_active_list(SWAP_CLUSTER_MAX, zone,
> > 					&sc, priority, 0);
> > 			}
> > 
> > 			if (!zone_watermark_ok(zone, order, zone->pages_high,
> > 					0, 0)) {
> > 				end_zone = i;
> > 				break;
> > 			}
> > 		}
> > 	}
> > 
> >   So, by looping around, we could end up shrinking the active list again
> >   before we recheck the zone watermarks depending on the size of the
> >   inactive lists.
> 
> If this is a problem, it is a problem with that code, because kswapd
> can be woken up for any zone at any time anyway.
> 
> 
> > > >                 cond_resched();
> > > > 
> > > >                 try_to_freeze();
> > > > 
> > > >                 goto loop_again;
> > > >         }
> > > > 
> > > > I used PAGE_ALLOC_COSTLY_ORDER instead of sc.order == 0 because we are
> > > > expected to support allocations up to that order in a fairly reliable fashion.
> > > 
> > > I actually think it's better to do it for all orders, because that
> > > constant is more or less arbitrary.
> > 
> > i.e.
> > 
> > if (!all_zones_ok && sc.order == 0) {
> > 
> > ? or something else
> 
> Well, I jus tdon't see what's wrong with the original patch.
> 

I've more or less convinced myself it's ok as any anomolies I spotted have
either been described as intentional behaviour or is arguably correct. A
fixed up (or deleted - misleading comments suck) comment and I'm happy.

>  
> > What I did miss was that we have 
> > 
> >                 if (nr_reclaimed >= SWAP_CLUSTER_MAX)
> >                         break;
> > 
> > so with my patch, kswapd is bailing out early without trying to reclaim for
> > high-orders that hard. That was not what I intended as it means we only ever
> > really rebalance the full system for order-0 pages and for everything else we
> > do relatively light scanning. The impact is that high-order users will direct
> > reclaim rather than depending on kswapd scanning very heavily. Arguably,
> > this is a good thing.
> > 
> > However, it also means that KOSAKI's and my patches only differs in that mine
> > bails early and KOSAKI rechecks everything at order-0, possibly reclaiming
> > more. If the comment was not so misleading, I'd have been a lot happier.
> 
> Rechecking everything is fine by me; order-0 is going to be the most
> common and most important. If higher order allocations sometimes have
> to enter direct reclaim or kick off kswapd again, it isn't a big deal.
> 

Grand so. Initially it looked like accidental rather than intentional
behaviour but after thinking about it some more, it should be ok.

> 
> > > IOW, I don't see a big downside, and there is a real upside.
> > > 
> > > I think the patch is good.
> > > 
> > 
> > Which one, KOSAKI's or my one?
> > 
> > Here is my one again which bails out for any high-order allocation after
> > just light scanning.
> > 
> > ====
> > 
> > >From 0e09fe002d8e9956de227b880ef8458842b71ca9 Mon Sep 17 00:00:00 2001
> > From: Mel Gorman <mel@....ul.ie>
> > Date: Tue, 30 Dec 2008 18:53:23 +0000
> > Subject: [PATCH] mm: stop kswapd's infinite loop at high order allocation
> > 
> > Wassim Dagash reported the following (editted) kswapd infinite loop problem.
> > 
> >   kswapd runs in some infinite loop trying to swap until order 10 of zone
> >   highmem is OK.... kswapd will continue to try to balance order 10 of zone
> >   highmem forever (or until someone release a very large chunk of highmem).
> > 
> > For costly high-order allocations, the system may never be balanced due to
> > fragmentation but kswapd should not infinitely loop as a result. The
> > following patch lets kswapd stop reclaiming in the event it cannot
> > balance zones and the order is high-order.
> 
> This one bails out if it was a higher order reclaim, but there is still
> an order-0 shortage. I prefer to run the loop again at order==0 to avoid
> that condition. A higher kswapd reclaim order shouldn't weaken kswapd
> postcondition for order-0 memory.
> 
> > 
> > Reported-by: wassim dagash <wassim.dagash@...il.com>
> > Signed-off-by: Mel Gorman <mel@....ul.ie>
> > 
> > ---
> >  mm/vmscan.c |   11 ++++++++++-
> >  1 files changed, 10 insertions(+), 1 deletions(-)
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 62e7f62..7b0f412 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -1867,7 +1867,16 @@ out:
> >  
> >  		zone->prev_priority = temp_priority[i];
> >  	}
> > -	if (!all_zones_ok) {
> > +
> > +	/*
> > +	 * If zones are still not balanced, loop again and continue attempting
> > +	 * to rebalance the system. For high-order allocations, fragmentation
> > +	 * can prevent the zones being rebalanced no matter how hard kswapd
> > +	 * works, particularly on systems with little or no swap. For
> > +	 * high-orders, just give up and assume interested processes will
> > +	 * either direct reclaim or wake up kswapd again as necessary.
> > +	 */
> > +	if (!all_zones_ok && sc.order == 0) {
> >  		cond_resched();
> >  
> >  		try_to_freeze();
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@...ck.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists