vmscan: cut down on struct scan_control

The RECLAIM_SWAP flag in zone_reclaim_mode controls whether zone_reclaim() is allowed to swap or not (obviously).

This is currently implemented by allowing or forbidding reclaim to unmap pages, which also controls reclaim of shared pages and is thus not appropriate.

We can do better by using the sc->may_swap parameter instead, which controls whether the anon lists are scanned.

Unmapping of pages is then allowed per default from zone_reclaim().

Signed-off-by Johannes Weiner <[email protected]>

---
 mm/vmscan.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

sorry for the long delayed review.

Quoted 26 lines

The RECLAIM_SWAP flag in zone_reclaim_mode controls whether zone_reclaim() is allowed to swap or not (obviously).

This is currently implemented by allowing or forbidding reclaim to unmap pages, which also controls reclaim of shared pages and is thus not appropriate.

We can do better by using the sc->may_swap parameter instead, which controls whether the anon lists are scanned.

Unmapping of pages is then allowed per default from zone_reclaim().

Signed-off-by Johannes Weiner <[email protected]>

---
 mm/vmscan.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

mm/vmscan.c +1 -2

@@ -2563,8 +2563,8 @@ static int __zone_reclaim(struct zone *z
 	int priority;
 	struct scan_control sc = {
 		.may_writepage = !!(zone_reclaim_mode & RECLAIM_WRITE),
-		.may_unmap = !!(zone_reclaim_mode & RECLAIM_SWAP),
-		.may_swap = 1,
+		.may_unmap = 1,

mm/vmscan.c static int __zone_reclaim(struct zone *z · line 2570

Quoted 4 lines

mm/vmscan.c +1 -0

+		.may_swap = !!(zone_reclaim_mode & RECLAIM_SWAP),
 		.nr_to_reclaim = max_t(unsigned long, nr_pages,
 				       SWAP_CLUSTER_MAX),
 		.gfp_mask = gfp_mask,

About half years ago, I did post exactly same patch. but at that time, it got Mel's objection. after some discution we agreed to merge documentation change instead code fix.

So, now the documentation describe clearly 4th bit meant no unmap. Please drop this, instead please make s/RECLAIM_SWAP/RECLAIM_MAPPED/ patch.

mm/vmscan.c static unsigned long do_try_to_free_page · line 1790

Quoted 6 lines

mm/vmscan.c +1 -1

@@ -1789,7 +1787,7 @@ static unsigned long do_try_to_free_page
 		sc->nr_scanned = 0;
 		if (!priority)
 			disable_swap_token();
-		shrink_zones(priority, zonelist, sc);
+		ret = shrink_zones(priority, zonelist, sc);

Please use more good name instead 'ret' ;)

otherwise looks good.

Reviewed-by KOSAKI Motohiro <[email protected]>

This scan control is abused to communicate a return value from shrink_zones(). Write this idiomatically and remove the knob.

Signed-off-by Johannes Weiner <[email protected]>

---
 mm/vmscan.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

mm/vmscan.c +5 -7

@@ -70,8 +70,6 @@ struct scan_control {
 
 	int swappiness;
 
-	int all_unreclaimable;
-
 	int order;
 
 	int lumpy_reclaim;

@@ -1701,14 +1699,14 @@ static void shrink_zone(int priority, st
  * If a zone is deemed to be full of pinned pages then just give it a light
  * scan then give up on it.
  */
-static void shrink_zones(int priority, struct zonelist *zonelist,
+static int 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;
 
-	sc->all_unreclaimable = 1;
 	for_each_zone_zonelist_nodemask(zone, z, zonelist, high_zoneidx,
 					sc->nodemask) {
 		if (!populated_zone(zone))

@@ -1724,19 +1722,19 @@ static void shrink_zones(int priority, s
 
 			if (zone->all_unreclaimable && priority != DEF_PRIORITY)
 				continue;	/* Let kswapd poll it */
-			sc->all_unreclaimable = 0;
 		} else {
 			/*
 			 * Ignore cpuset limitation here. We just want to reduce
 			 * # of used pages by us regardless of memory shortage.
 			 */
-			sc->all_unreclaimable = 0;
 			mem_cgroup_note_reclaim_priority(sc->mem_cgroup,
 							priority);
 		}
 
 		shrink_zone(priority, zone, sc);
+		progress = 1;
 	}
+	return progress;
 }
 
 /*

@@ -1789,7 +1787,7 @@ static unsigned long do_try_to_free_page
 		sc->nr_scanned = 0;
 		if (!priority)
 			disable_swap_token();
-		shrink_zones(priority, zonelist, sc);
+		ret = shrink_zones(priority, zonelist, sc);
 		/*
 		 * Don't shrink slabs when reclaiming memory from
 		 * over limit cgroups

	if (ret && scanning_global_lru(sc))

ret = sc->nr_reclaimed;

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.

Quoted 2 lines

	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.

Quoted 5 lines

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.

Quoted 7 lines

Quoted 2 lines

	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 <[email protected]>
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 <[email protected]>
	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 <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

CC to memcg folks.

Quoted 34 lines

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.

Quoted 7 lines

Quoted 2 lines

	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 <[email protected]>
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 <[email protected]>
	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.

Quoted 90 lines · KOSAKI Motohiro

CC to memcg folks.

Quoted 34 lines

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.

Quoted 7 lines

Quoted 2 lines

	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 <[email protected]>
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 <[email protected]>
	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.

Quoted 2 lines

This patch fixes it completely.

The patch seems reasonable to me, although I've not tested it

Acked-by Balbir Singh <[email protected]>

CC to memcg folks.

Quoted 34 lines

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.

Quoted 7 lines

Quoted 2 lines

	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 <[email protected]>
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 <[email protected]>
	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 <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

---
 mm/vmscan.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

mm/vmscan.c +16 -13

index 915dceb..a204209 100644
@@ -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,

Signature · 1 line

1.6.5.2

CC to memcg folks.

Quoted 34 lines

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.

Quoted 7 lines

Quoted 2 lines

	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 <[email protected]>
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 <[email protected]>
	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 <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

For now, we have global isolation vs. memory control group isolation, do not allow the reclaim entry function to set an arbitrary page isolation callback, we do not need that flexibility.

And since we already pass around the group descriptor for the memory control group isolation case, just use it to decide which one of the two isolator functions to use.

The decisions can be merged into nearby branches, so no extra cost there. In fact, we save the indirect calls.

Signed-off-by Johannes Weiner <[email protected]>

---
 include/linux/memcontrol.h |   13 ++++++-----
 mm/vmscan.c                |   52 ++++++++++++++++++++++++---------------------
 2 files changed, 35 insertions(+), 30 deletions(-)

mm/vmscan.c +5 -11

@@ -82,12 +82,6 @@ struct scan_control {
 	 * are scanned.
 	 */
 	nodemask_t	*nodemask;
-
-	/* Pluggable isolate pages callback */
-	unsigned long (*isolate_pages)(unsigned long nr, struct list_head *dst,
-			unsigned long *scanned, int order, int mode,
-			struct zone *z, struct mem_cgroup *mem_cont,
-			int active, int file);
 };
 
 #define lru_to_page(_head) (list_entry((_head)->prev, struct page, lru))

@@ -1000,7 +994,6 @@ static unsigned long isolate_pages_globa
 					struct list_head *dst,
 					unsigned long *scanned, int order,
 					int mode, struct zone *z,
-					struct mem_cgroup *mem_cont,
 					int active, int file)
 {
 	int lru = LRU_BASE;

@@ -1144,11 +1137,11 @@ static unsigned long shrink_inactive_lis
 		unsigned long nr_anon;
 		unsigned long nr_file;
 
-		nr_taken = sc->isolate_pages(SWAP_CLUSTER_MAX,
-			     &page_list, &nr_scan, sc->order, mode,
-				zone, sc->mem_cgroup, 0, file);
-
 		if (scanning_global_lru(sc)) {
+			nr_taken = isolate_pages_global(SWAP_CLUSTER_MAX,
+							&page_list, &nr_scan,
+							sc->order, mode,
+							zone, 0, file);
 			zone->pages_scanned += nr_scan;
 			if (current_is_kswapd())
 				__count_zone_vm_events(PGSCAN_KSWAPD, zone,

@@ -1156,6 +1149,16 @@ static unsigned long shrink_inactive_lis
 			else
 				__count_zone_vm_events(PGSCAN_DIRECT, zone,
 						       nr_scan);
+		} else {

Quoted 66 lines

For now, we have global isolation vs. memory control group isolation, do not allow the reclaim entry function to set an arbitrary page isolation callback, we do not need that flexibility.

And since we already pass around the group descriptor for the memory control group isolation case, just use it to decide which one of the two isolator functions to use.

The decisions can be merged into nearby branches, so no extra cost there. In fact, we save the indirect calls.

Signed-off-by Johannes Weiner <[email protected]>

---
 include/linux/memcontrol.h |   13 ++++++-----
 mm/vmscan.c                |   52 ++++++++++++++++++++++++---------------------
 2 files changed, 35 insertions(+), 30 deletions(-)

mm/vmscan.c +10 -11

@@ -82,12 +82,6 @@ struct scan_control {
 	 * are scanned.
 	 */
 	nodemask_t	*nodemask;
-
-	/* Pluggable isolate pages callback */
-	unsigned long (*isolate_pages)(unsigned long nr, struct list_head *dst,
-			unsigned long *scanned, int order, int mode,
-			struct zone *z, struct mem_cgroup *mem_cont,
-			int active, int file);
 };
 
 #define lru_to_page(_head) (list_entry((_head)->prev, struct page, lru))

@@ -1000,7 +994,6 @@ static unsigned long isolate_pages_globa
 					struct list_head *dst,
 					unsigned long *scanned, int order,
 					int mode, struct zone *z,
-					struct mem_cgroup *mem_cont,
 					int active, int file)
 {
 	int lru = LRU_BASE;

@@ -1144,11 +1137,11 @@ static unsigned long shrink_inactive_lis
 		unsigned long nr_anon;
 		unsigned long nr_file;
 
-		nr_taken = sc->isolate_pages(SWAP_CLUSTER_MAX,
-			     &page_list, &nr_scan, sc->order, mode,
-				zone, sc->mem_cgroup, 0, file);
-
 		if (scanning_global_lru(sc)) {
+			nr_taken = isolate_pages_global(SWAP_CLUSTER_MAX,
+							&page_list, &nr_scan,
+							sc->order, mode,
+							zone, 0, file);
 			zone->pages_scanned += nr_scan;
 			if (current_is_kswapd())
 				__count_zone_vm_events(PGSCAN_KSWAPD, zone,

@@ -1156,6 +1149,16 @@ static unsigned long shrink_inactive_lis
 			else
 				__count_zone_vm_events(PGSCAN_DIRECT, zone,
 						       nr_scan);
+		} else {
+			nr_taken = mem_cgroup_isolate_pages(SWAP_CLUSTER_MAX,
+							&page_list, &nr_scan,
+							sc->order, mode,
+							zone, sc->mem_cgroup,
+							0, file);

mm/vmscan.c static unsigned long shrink_inactive_lis · line 1162

Quoted 5 lines

mm/vmscan.c +4 -0

+			/*
+			 * mem_cgroup_isolate_pages() keeps track of
+			 * scanned pages on its own.
+			 */
 		}

There are the same logic in shrink_active/inactive_list. Can we make wrapper function? It probably improve code readability.

Quoted 2 lines

There are the same logic in shrink_active/inactive_list. Can we make wrapper function? It probably improve code readability.

They are not completely identical, PGSCAN_DIRECT/PGSCAN_KSWAPD accounting is only done in shrink_inactive_list(), so we would need an extra branch. Can we leave it like that for now?

The may_swap scan control flag can be naturally merged into the swappiness parameter: swap only if swappiness is non-zero.

Quoted 2 lines

The may_swap scan control flag can be naturally merged into the swappiness parameter: swap only if swappiness is non-zero.

Sorry, NAK.

AFAIK, swappiness==0 is very widely used in MySQL users community. They expect this parameter mean "very prefer to discard file cache rather than swap, but not completely disable swap".

We shouldn't ignore the real world use case. even if it is a bit strange.

Quoted 10 lines

Quoted 2 lines

The may_swap scan control flag can be naturally merged into the swappiness parameter: swap only if swappiness is non-zero.

Sorry, NAK.

AFAIK, swappiness==0 is very widely used in MySQL users community. They expect this parameter mean "very prefer to discard file cache rather than swap, but not completely disable swap".

We shouldn't ignore the real world use case. even if it is a bit strange.

Bummer. It's really ugly to have 'zero' mean 'almost nothing'.

But since swappiness is passed around as an int, I think we can instead use -1 for 'no swap'. Let me look into it and send a follow-up patch for this as well.

Thanks!