Re: [PATCH 4/4] mm: vmscan: Only read new_classzone_idx from pgdat when reclaiming successfully

[Mel Gorman <mgorman@suse.de>]

On Thu, Jun 30, 2011 at 06:05:59PM +0900, KOSAKI Motohiro wrote:
> (2011/06/24 23:44), Mel Gorman wrote:
> > During allocator-intensive workloads, kswapd will be woken frequently
> > causing free memory to oscillate between the high and min watermark.
> > This is expected behaviour.  Unfortunately, if the highest zone is
> > small, a problem occurs.
> > 
> > When balance_pgdat() returns, it may be at a lower classzone_idx than
> > it started because the highest zone was unreclaimable. Before checking
> > if it should go to sleep though, it checks pgdat->classzone_idx which
> > when there is no other activity will be MAX_NR_ZONES-1. It interprets
> > this as it has been woken up while reclaiming, skips scheduling and
> > reclaims again. As there is no useful reclaim work to do, it enters
> > into a loop of shrinking slab consuming loads of CPU until the highest
> > zone becomes reclaimable for a long period of time.
> > 
> > There are two problems here. 1) If the returned classzone or order is
> > lower, it'll continue reclaiming without scheduling. 2) if the highest
> > zone was marked unreclaimable but balance_pgdat() returns immediately
> > at DEF_PRIORITY, the new lower classzone is not communicated back to
> > kswapd() for sleeping.
> > 
> > This patch does two things that are related. If the end_zone is
> > unreclaimable, this information is communicated back. Second, if
> > the classzone or order was reduced due to failing to reclaim, new
> > information is not read from pgdat and instead an attempt is made to go
> > to sleep. Due to this, it is also necessary that pgdat->classzone_idx
> > be initialised each time to pgdat->nr_zones - 1 to avoid re-reads
> > being interpreted as wakeups.
> > 
> > Reported-and-tested-by: Pádraig Brady <P@draigBrady.com>
> > Signed-off-by: Mel Gorman <mgorman@suse.de>
> > ---
> >  mm/vmscan.c |   34 +++++++++++++++++++++-------------
> >  1 files changed, 21 insertions(+), 13 deletions(-)
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index a76b6cc2..fe854d7 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -2448,7 +2448,6 @@ loop_again:
> >  			if (!zone_watermark_ok_safe(zone, order,
> >  					high_wmark_pages(zone), 0, 0)) {
> >  				end_zone = i;
> > -				*classzone_idx = i;
> >  				break;
> >  			}
> >  		}
> > @@ -2528,8 +2527,11 @@ loop_again:
> >  			    total_scanned > sc.nr_reclaimed + sc.nr_reclaimed / 2)
> >  				sc.may_writepage = 1;
> >  
> > -			if (zone->all_unreclaimable)
> > +			if (zone->all_unreclaimable) {
> > +				if (end_zone && end_zone == i)
> > +					end_zone--;
> >  				continue;
> > +			}
> >  
> >  			if (!zone_watermark_ok_safe(zone, order,
> >  					high_wmark_pages(zone), end_zone, 0)) {
> > @@ -2709,8 +2711,8 @@ static void kswapd_try_to_sleep(pg_data_t *pgdat, int order, int classzone_idx)
> >   */
> >  static int kswapd(void *p)
> >  {
> > -	unsigned long order;
> > -	int classzone_idx;
> > +	unsigned long order, new_order;
> > +	int classzone_idx, new_classzone_idx;
> >  	pg_data_t *pgdat = (pg_data_t*)p;
> >  	struct task_struct *tsk = current;
> >  
> > @@ -2740,17 +2742,23 @@ static int kswapd(void *p)
> >  	tsk->flags |= PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD;
> >  	set_freezable();
> >  
> > -	order = 0;
> > -	classzone_idx = MAX_NR_ZONES - 1;
> > +	order = new_order = 0;
> > +	classzone_idx = new_classzone_idx = pgdat->nr_zones - 1;
> >  	for ( ; ; ) {
> > -		unsigned long new_order;
> > -		int new_classzone_idx;
> >  		int ret;
> >  
> > -		new_order = pgdat->kswapd_max_order;
> > -		new_classzone_idx = pgdat->classzone_idx;
> > -		pgdat->kswapd_max_order = 0;
> > -		pgdat->classzone_idx = MAX_NR_ZONES - 1;
> > +		/*
> > +		 * If the last balance_pgdat was unsuccessful it's unlikely a
> > +		 * new request of a similar or harder type will succeed soon
> > +		 * so consider going to sleep on the basis we reclaimed at
> > +		 */
> > +		if (classzone_idx >= new_classzone_idx && order == new_order) {
> 
> I'm confusing this. If we take a following scenario, new_classzone_idx may be garbage.
> 
> 1. new_classzone_idx = pgdat->classzone_idx
> 2. kswapd_try_to_sleep()
> 3. classzone_idx = pgdat->classzone_idx
> 4. balance_pgdat()
> 
> Wouldn't we need to reinitialize new_classzone_idx nad new_order at kswapd_try_to_sleep()
> path too?
> 

I don't understand your question. new_classzone_idx is initialised
before the kswapd main loop and after this patch is only updated
only when balance_pgdat() successfully balanced but the following
situation can arise

1. Read for balance-request-A (order, classzone) pair
2. Fail balance_pgdat
3. Sleep based on (order, classzone) pair
4. Wake for balance-request-B (order, classzone) pair where
   balance-request-B != balance-request-A
5. Succeed balance_pgdat
6. Compare order,classzone with balance-request-A which will treat
   balance_pgdat() as fail and try go to sleep

This is not the same as new_classzone_idx being "garbage" but is it
what you mean? If so, is this your proposed fix?

diff --git a/mm/vmscan.c b/mm/vmscan.c
index fe854d7..1a518e6 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2770,6 +2770,8 @@ static int kswapd(void *p)
 			kswapd_try_to_sleep(pgdat, order, classzone_idx);
 			order = pgdat->kswapd_max_order;
 			classzone_idx = pgdat->classzone_idx;
+			new_order = order;
+			new_classzone_idx = classzone_idx;
 			pgdat->kswapd_max_order = 0;
 			pgdat->classzone_idx = pgdat->nr_zones - 1;
 		}