[PATCH] vmscan: Fix do_try_to_free_pages() return value when priority==0 reclaim failure

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

CC to memcg folks.

> I agree with the direction of this patch, but I am seeing a hang when
> testing with mmotm-2010-05-21-16-05.  The following test hangs, unless I
> remove this patch from mmotm:
>   mount -t cgroup none /cgroups -o memory
>   mkdir /cgroups/cg1
>   echo $$ > /cgroups/cg1/tasks
>   dd bs=1024 count=1024 if=/dev/null of=/data/foo
>   echo $$ > /cgroups/tasks
>   echo 1 > /cgroups/cg1/memory.force_empty
> 
> I think the hang is caused by the following portion of
> mem_cgroup_force_empty():
> 	while (nr_retries && mem->res.usage > 0) {
> 		int progress;
> 
> 		if (signal_pending(current)) {
> 			ret = -EINTR;
> 			goto out;
> 		}
> 		progress = try_to_free_mem_cgroup_pages(mem, GFP_KERNEL,
> 						false, get_swappiness(mem));
> 		if (!progress) {
> 			nr_retries--;
> 			/* maybe some writeback is necessary */
> 			congestion_wait(BLK_RW_ASYNC, HZ/10);
> 		}
> 
> 	}
> 
> With this patch applied, it is possible that when do_try_to_free_pages()
> calls shrink_zones() for priority 0 that shrink_zones() may return 1
> indicating progress, even though no pages may have been reclaimed.
> Because this is a cgroup operation, scanning_global_lru() is false and
> the following portion of do_try_to_free_pages() fails to set ret=0.
> > 	if (ret && scanning_global_lru(sc))
> >  		ret = sc->nr_reclaimed;
> This leaves ret=1 indicating that do_try_to_free_pages() reclaimed 1
> page even though it did not reclaim any pages.  Therefore
> mem_cgroup_force_empty() erroneously believes that
> try_to_free_mem_cgroup_pages() is making progress (one page at a time),
> so there is an endless loop.

Good catch!

Yeah, your analysis is fine. thank you for both your testing and
making analysis.

Unfortunatelly, this logic need more fix. because It have already been
corrupted by another regression. my point is, if priority==0 reclaim 
failure occur, "ret = sc->nr_reclaimed" makes no sense at all.

The fixing patch is here. What do you think?



From 49a395b21fe1b2f864112e71d027ffcafbdc9fc0 Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date: Tue, 1 Jun 2010 11:29:50 +0900
Subject: [PATCH] vmscan: Fix do_try_to_free_pages() return value when priority==0 reclaim failure

Greg Thelen reported recent Johannes's stack diet patch makes kernel
hang. His test is following.

  mount -t cgroup none /cgroups -o memory
  mkdir /cgroups/cg1
  echo $$ > /cgroups/cg1/tasks
  dd bs=1024 count=1024 if=/dev/null of=/data/foo
  echo $$ > /cgroups/tasks
  echo 1 > /cgroups/cg1/memory.force_empty

Actually, This OOM hard to try logic have been corrupted
since following two years old patch.

	commit a41f24ea9fd6169b147c53c2392e2887cc1d9247
	Author: Nishanth Aravamudan <nacc@us.ibm.com>
	Date:   Tue Apr 29 00:58:25 2008 -0700

	    page allocator: smarter retry of costly-order allocations

Original intention was "return success if the system have shrinkable
zones though priority==0 reclaim was failure". But the above patch
changed to "return nr_reclaimed if .....". Oh, That forgot nr_reclaimed
may be 0 if priority==0 reclaim failure.

And Johannes's patch made more corrupt. Originally, priority==0 recliam
failure on memcg return 0, but this patch changed to return 1. It
totally confused memcg.

This patch fixes it completely.

Reported-by: Greg Thelen <gthelen@google.com>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/vmscan.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 915dceb..a204209 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1724,13 +1724,13 @@ static void shrink_zone(int priority, struct zone *zone,
  * If a zone is deemed to be full of pinned pages then just give it a light
  * scan then give up on it.
  */
-static int shrink_zones(int priority, struct zonelist *zonelist,
+static bool shrink_zones(int priority, struct zonelist *zonelist,
 					struct scan_control *sc)
 {
 	enum zone_type high_zoneidx = gfp_zone(sc->gfp_mask);
 	struct zoneref *z;
 	struct zone *zone;
-	int progress = 0;
+	bool all_unreclaimable = true;
 
 	for_each_zone_zonelist_nodemask(zone, z, zonelist, high_zoneidx,
 					sc->nodemask) {
@@ -1757,9 +1757,9 @@ static int shrink_zones(int priority, struct zonelist *zonelist,
 		}
 
 		shrink_zone(priority, zone, sc);
-		progress = 1;
+		all_unreclaimable = false;
 	}
-	return progress;
+	return all_unreclaimable;
 }
 
 /*
@@ -1782,7 +1782,7 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 					struct scan_control *sc)
 {
 	int priority;
-	unsigned long ret = 0;
+	bool all_unreclaimable; 
 	unsigned long total_scanned = 0;
 	struct reclaim_state *reclaim_state = current->reclaim_state;
 	unsigned long lru_pages = 0;
@@ -1813,7 +1813,7 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 		sc->nr_scanned = 0;
 		if (!priority)
 			disable_swap_token();
-		ret = shrink_zones(priority, zonelist, sc);
+		all_unreclaimable = shrink_zones(priority, zonelist, sc);
 		/*
 		 * Don't shrink slabs when reclaiming memory from
 		 * over limit cgroups
@@ -1826,10 +1826,8 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 			}
 		}
 		total_scanned += sc->nr_scanned;
-		if (sc->nr_reclaimed >= sc->nr_to_reclaim) {
-			ret = sc->nr_reclaimed;
+		if (sc->nr_reclaimed >= sc->nr_to_reclaim)
 			goto out;
-		}
 
 		/*
 		 * Try to write back as many pages as we just scanned.  This
@@ -1849,9 +1847,7 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
 		    priority < DEF_PRIORITY - 2)
 			congestion_wait(BLK_RW_ASYNC, HZ/10);
 	}
-	/* top priority shrink_zones still had more to do? don't OOM, then */
-	if (ret && scanning_global_lru(sc))
-		ret = sc->nr_reclaimed;
+
 out:
 	/*
 	 * Now that we've scanned all the zones at this priority level, note
@@ -1877,7 +1873,14 @@ out:
 	delayacct_freepages_end();
 	put_mems_allowed();
 
-	return ret;
+	if (sc->nr_reclaimed)
+		return sc->nr_reclaimed;
+
+	/* top priority shrink_zones still had more to do? don't OOM, then */
+	if (scanning_global_lru(sc) && !all_unreclaimable)
+		return 1;
+
+	return 0;
 }
 
 unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
-- 
1.6.5.2




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