[patch 0/5] vmscan: cut down on struct scan_control

[patch 0/5] vmscan: cut down on struct scan_control

[Johannes Weiner]

Hi,

here are 5 patches that remove 4 members from struct scan_control.

sc->may_unmap is no longer used after patch #1, sc->may_swap is folded
into sc->swappiness, sc->all_unreclaimable is made a return value, and
sc->isolate_pages is replaced with a branch on sc->mem_cgroup (reusing
a nearby branch, so no additional one) and direct function calling.

So nothing too spectecular.  It saves a bit of code and 2 to 4 stack
words depending on the wordsize and call path.

	Hannes

 include/linux/memcontrol.h |   13 +++--
 include/linux/swap.h       |    4 -
 mm/memcontrol.c            |   13 +++--
 mm/vmscan.c                |  105 ++++++++++++++++++---------------------------
 4 files changed, 61 insertions(+), 74 deletions(-)

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

[patch 1/5] vmscan: fix unmapping behaviour for RECLAIM_SWAP

[Johannes Weiner]

[-- Attachment #1: vmscan-fix-unmapping-behaviour-for-RECLAIM_SWAP.patch --]
[-- Type: text/plain, Size: 1264 bytes --]

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 <hannes@cmpxchg.org>
---
 mm/vmscan.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -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,
+		.may_swap = !!(zone_reclaim_mode & RECLAIM_SWAP),
 		.nr_to_reclaim = max_t(unsigned long, nr_pages,
 				       SWAP_CLUSTER_MAX),
 		.gfp_mask = gfp_mask,


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

[patch 2/5] vmscan: remove may_unmap scan control

[Johannes Weiner]

[-- Attachment #1: vmscan-remove-may_unmap-scan-control.patch --]
[-- Type: text/plain, Size: 2611 bytes --]

Every reclaim entry function sets this to the same value.

Make it default behaviour and remove the knob.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/vmscan.c |   12 ------------
 1 file changed, 12 deletions(-)

--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -65,9 +65,6 @@ struct scan_control {
 
 	int may_writepage;
 
-	/* Can mapped pages be reclaimed? */
-	int may_unmap;
-
 	/* Can pages be swapped as part of reclaim? */
 	int may_swap;
 
@@ -655,9 +652,6 @@ static unsigned long shrink_page_list(st
 		if (unlikely(!page_evictable(page, NULL)))
 			goto cull_mlocked;
 
-		if (!sc->may_unmap && page_mapped(page))
-			goto keep_locked;
-
 		/* Double the slab pressure for mapped and swapcache pages */
 		if (page_mapped(page) || PageSwapCache(page))
 			sc->nr_scanned++;
@@ -1868,7 +1862,6 @@ unsigned long try_to_free_pages(struct z
 		.gfp_mask = gfp_mask,
 		.may_writepage = !laptop_mode,
 		.nr_to_reclaim = SWAP_CLUSTER_MAX,
-		.may_unmap = 1,
 		.may_swap = 1,
 		.swappiness = vm_swappiness,
 		.order = order,
@@ -1889,7 +1882,6 @@ unsigned long mem_cgroup_shrink_node_zon
 {
 	struct scan_control sc = {
 		.may_writepage = !laptop_mode,
-		.may_unmap = 1,
 		.may_swap = !noswap,
 		.swappiness = swappiness,
 		.order = 0,
@@ -1922,7 +1914,6 @@ unsigned long try_to_free_mem_cgroup_pag
 	struct zonelist *zonelist;
 	struct scan_control sc = {
 		.may_writepage = !laptop_mode,
-		.may_unmap = 1,
 		.may_swap = !noswap,
 		.nr_to_reclaim = SWAP_CLUSTER_MAX,
 		.swappiness = swappiness,
@@ -1996,7 +1987,6 @@ static unsigned long balance_pgdat(pg_da
 	struct reclaim_state *reclaim_state = current->reclaim_state;
 	struct scan_control sc = {
 		.gfp_mask = GFP_KERNEL,
-		.may_unmap = 1,
 		.may_swap = 1,
 		/*
 		 * kswapd doesn't want to be bailed out while reclaim. because
@@ -2379,7 +2369,6 @@ unsigned long shrink_all_memory(unsigned
 	struct scan_control sc = {
 		.gfp_mask = GFP_HIGHUSER_MOVABLE,
 		.may_swap = 1,
-		.may_unmap = 1,
 		.may_writepage = 1,
 		.nr_to_reclaim = nr_to_reclaim,
 		.hibernation_mode = 1,
@@ -2563,7 +2552,6 @@ static int __zone_reclaim(struct zone *z
 	int priority;
 	struct scan_control sc = {
 		.may_writepage = !!(zone_reclaim_mode & RECLAIM_WRITE),
-		.may_unmap = 1,
 		.may_swap = !!(zone_reclaim_mode & RECLAIM_SWAP),
 		.nr_to_reclaim = max_t(unsigned long, nr_pages,
 				       SWAP_CLUSTER_MAX),


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

[patch 3/5] vmscan: remove all_unreclaimable scan control

[Johannes Weiner]

[-- Attachment #1: vmscan-remove-all_unreclaimable-scan-control.patch --]
[-- Type: text/plain, Size: 2425 bytes --]

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 <hannes@cmpxchg.org>
---
 mm/vmscan.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -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
@@ -1826,7 +1824,7 @@ static unsigned long do_try_to_free_page
 			congestion_wait(BLK_RW_ASYNC, HZ/10);
 	}
 	/* top priority shrink_zones still had more to do? don't OOM, then */
-	if (!sc->all_unreclaimable && scanning_global_lru(sc))
+	if (ret && scanning_global_lru(sc))
 		ret = sc->nr_reclaimed;
 out:
 	/*


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

[patch 4/5] vmscan: remove isolate_pages callback scan control

[Johannes Weiner]

[-- Attachment #1: vmscan-remove-isolate_pages-callback-scan-control.patch --]
[-- Type: text/plain, Size: 6053 bytes --]

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 <hannes@cmpxchg.org>
---
 include/linux/memcontrol.h |   13 ++++++-----
 mm/vmscan.c                |   52 ++++++++++++++++++++++++---------------------
 2 files changed, 35 insertions(+), 30 deletions(-)

--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -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);
+			/*
+			 * mem_cgroup_isolate_pages() keeps track of
+			 * scanned pages on its own.
+			 */
 		}
 
 		if (nr_taken == 0)
@@ -1333,16 +1336,23 @@ static void shrink_active_list(unsigned 
 
 	lru_add_drain();
 	spin_lock_irq(&zone->lru_lock);
-	nr_taken = sc->isolate_pages(nr_pages, &l_hold, &pgscanned, sc->order,
-					ISOLATE_ACTIVE, zone,
-					sc->mem_cgroup, 1, file);
-	/*
-	 * zone->pages_scanned is used for detect zone's oom
-	 * mem_cgroup remembers nr_scan by itself.
-	 */
 	if (scanning_global_lru(sc)) {
+		nr_taken = isolate_pages_global(nr_pages, &l_hold,
+						&pgscanned, sc->order,
+						ISOLATE_ACTIVE, zone,
+						1, file);
 		zone->pages_scanned += pgscanned;
+	} else {
+		nr_taken = mem_cgroup_isolate_pages(nr_pages, &l_hold,
+						&pgscanned, sc->order,
+						ISOLATE_ACTIVE, zone,
+						sc->mem_cgroup, 1, file);
+		/*
+		 * mem_cgroup_isolate_pages() keeps track of
+		 * scanned pages on its own.
+		 */
 	}
+
 	reclaim_stat->recent_scanned[file] += nr_taken;
 
 	__count_zone_vm_events(PGREFILL, zone, pgscanned);
@@ -1864,7 +1874,6 @@ unsigned long try_to_free_pages(struct z
 		.swappiness = vm_swappiness,
 		.order = order,
 		.mem_cgroup = NULL,
-		.isolate_pages = isolate_pages_global,
 		.nodemask = nodemask,
 	};
 
@@ -1884,7 +1893,6 @@ unsigned long mem_cgroup_shrink_node_zon
 		.swappiness = swappiness,
 		.order = 0,
 		.mem_cgroup = mem,
-		.isolate_pages = mem_cgroup_isolate_pages,
 	};
 	nodemask_t nm  = nodemask_of_node(nid);
 
@@ -1917,7 +1925,6 @@ unsigned long try_to_free_mem_cgroup_pag
 		.swappiness = swappiness,
 		.order = 0,
 		.mem_cgroup = mem_cont,
-		.isolate_pages = mem_cgroup_isolate_pages,
 		.nodemask = NULL, /* we don't care the placement */
 	};
 
@@ -1994,7 +2001,6 @@ static unsigned long balance_pgdat(pg_da
 		.swappiness = vm_swappiness,
 		.order = order,
 		.mem_cgroup = NULL,
-		.isolate_pages = isolate_pages_global,
 	};
 	/*
 	 * temp_priority is used to remember the scanning priority at which
@@ -2372,7 +2378,6 @@ unsigned long shrink_all_memory(unsigned
 		.hibernation_mode = 1,
 		.swappiness = vm_swappiness,
 		.order = 0,
-		.isolate_pages = isolate_pages_global,
 	};
 	struct zonelist * zonelist = node_zonelist(numa_node_id(), sc.gfp_mask);
 	struct task_struct *p = current;
@@ -2556,7 +2561,6 @@ static int __zone_reclaim(struct zone *z
 		.gfp_mask = gfp_mask,
 		.swappiness = vm_swappiness,
 		.order = order,
-		.isolate_pages = isolate_pages_global,
 	};
 	unsigned long slab_reclaimable;
 
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -25,6 +25,13 @@ struct page_cgroup;
 struct page;
 struct mm_struct;
 
+extern unsigned long mem_cgroup_isolate_pages(unsigned long nr_to_scan,
+					struct list_head *dst,
+					unsigned long *scanned, int order,
+					int mode, struct zone *z,
+					struct mem_cgroup *mem_cont,
+					int active, int file);
+
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR
 /*
  * All "charge" functions with gfp_mask should use GFP_KERNEL or
@@ -64,12 +71,6 @@ extern void mem_cgroup_uncharge_cache_pa
 extern int mem_cgroup_shmem_charge_fallback(struct page *page,
 			struct mm_struct *mm, gfp_t gfp_mask);
 
-extern unsigned long mem_cgroup_isolate_pages(unsigned long nr_to_scan,
-					struct list_head *dst,
-					unsigned long *scanned, int order,
-					int mode, struct zone *z,
-					struct mem_cgroup *mem_cont,
-					int active, int file);
 extern void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask);
 int task_in_mem_cgroup(struct task_struct *task, const struct mem_cgroup *mem);
 


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

[patch 5/5] vmscan: remove may_swap scan control

[Johannes Weiner]

[-- Attachment #1: vmscan-remove-may_swap-scan-control.patch --]
[-- Type: text/plain, Size: 5354 bytes --]

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

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 include/linux/swap.h |    4 ++--
 mm/memcontrol.c      |   13 +++++++++----
 mm/vmscan.c          |   27 +++++++++------------------
 3 files changed, 20 insertions(+), 24 deletions(-)

--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -248,10 +248,10 @@ static inline void lru_cache_add_active_
 extern unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 					gfp_t gfp_mask, nodemask_t *mask);
 extern unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *mem,
-						  gfp_t gfp_mask, bool noswap,
+						  gfp_t gfp_mask,
 						  unsigned int swappiness);
 extern unsigned long mem_cgroup_shrink_node_zone(struct mem_cgroup *mem,
-						gfp_t gfp_mask, bool noswap,
+						gfp_t gfp_mask,
 						unsigned int swappiness,
 						struct zone *zone,
 						int nid);
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1234,6 +1234,8 @@ static int mem_cgroup_hierarchical_recla
 		noswap = true;
 
 	while (1) {
+		unsigned int swappiness;
+
 		victim = mem_cgroup_select_victim(root_mem);
 		if (victim == root_mem) {
 			loop++;
@@ -1268,13 +1270,16 @@ static int mem_cgroup_hierarchical_recla
 			continue;
 		}
 		/* we use swappiness of local cgroup */
+		if (noswap)
+			swappiness = 0;
+		else
+			swappiness = get_swappiness(victim);
 		if (check_soft)
 			ret = mem_cgroup_shrink_node_zone(victim, gfp_mask,
-				noswap, get_swappiness(victim), zone,
-				zone->zone_pgdat->node_id);
+				swappiness, zone, zone->zone_pgdat->node_id);
 		else
 			ret = try_to_free_mem_cgroup_pages(victim, gfp_mask,
-						noswap, get_swappiness(victim));
+							swappiness);
 		css_put(&victim->css);
 		/*
 		 * At shrinking usage, we can't check we should stop here or
@@ -2966,7 +2971,7 @@ try_to_free:
 			goto out;
 		}
 		progress = try_to_free_mem_cgroup_pages(mem, GFP_KERNEL,
-						false, get_swappiness(mem));
+							get_swappiness(mem));
 		if (!progress) {
 			nr_retries--;
 			/* maybe some writeback is necessary */
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -65,9 +65,6 @@ struct scan_control {
 
 	int may_writepage;
 
-	/* Can pages be swapped as part of reclaim? */
-	int may_swap;
-
 	int swappiness;
 
 	int order;
@@ -1545,7 +1542,7 @@ static void get_scan_count(struct zone *
 	int noswap = 0;
 
 	/* If we have no swap space, do not bother scanning anon pages. */
-	if (!sc->may_swap || (nr_swap_pages <= 0)) {
+	if (!sc->swappiness || (nr_swap_pages <= 0)) {
 		noswap = 1;
 		fraction[0] = 0;
 		fraction[1] = 1;
@@ -1870,7 +1867,6 @@ unsigned long try_to_free_pages(struct z
 		.gfp_mask = gfp_mask,
 		.may_writepage = !laptop_mode,
 		.nr_to_reclaim = SWAP_CLUSTER_MAX,
-		.may_swap = 1,
 		.swappiness = vm_swappiness,
 		.order = order,
 		.mem_cgroup = NULL,
@@ -1883,13 +1879,11 @@ unsigned long try_to_free_pages(struct z
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR
 
 unsigned long mem_cgroup_shrink_node_zone(struct mem_cgroup *mem,
-						gfp_t gfp_mask, bool noswap,
-						unsigned int swappiness,
-						struct zone *zone, int nid)
+					gfp_t gfp_mask, unsigned int swappiness,
+					struct zone *zone, int nid)
 {
 	struct scan_control sc = {
 		.may_writepage = !laptop_mode,
-		.may_swap = !noswap,
 		.swappiness = swappiness,
 		.order = 0,
 		.mem_cgroup = mem,
@@ -1913,14 +1907,11 @@ unsigned long mem_cgroup_shrink_node_zon
 }
 
 unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *mem_cont,
-					   gfp_t gfp_mask,
-					   bool noswap,
-					   unsigned int swappiness)
+					gfp_t gfp_mask, unsigned int swappiness)
 {
 	struct zonelist *zonelist;
 	struct scan_control sc = {
 		.may_writepage = !laptop_mode,
-		.may_swap = !noswap,
 		.nr_to_reclaim = SWAP_CLUSTER_MAX,
 		.swappiness = swappiness,
 		.order = 0,
@@ -1992,7 +1983,6 @@ static unsigned long balance_pgdat(pg_da
 	struct reclaim_state *reclaim_state = current->reclaim_state;
 	struct scan_control sc = {
 		.gfp_mask = GFP_KERNEL,
-		.may_swap = 1,
 		/*
 		 * kswapd doesn't want to be bailed out while reclaim. because
 		 * we want to put equal scanning pressure on each zone.
@@ -2372,7 +2362,6 @@ unsigned long shrink_all_memory(unsigned
 	struct reclaim_state reclaim_state;
 	struct scan_control sc = {
 		.gfp_mask = GFP_HIGHUSER_MOVABLE,
-		.may_swap = 1,
 		.may_writepage = 1,
 		.nr_to_reclaim = nr_to_reclaim,
 		.hibernation_mode = 1,
@@ -2554,16 +2543,18 @@ static int __zone_reclaim(struct zone *z
 	struct reclaim_state reclaim_state;
 	int priority;
 	struct scan_control sc = {
-		.may_writepage = !!(zone_reclaim_mode & RECLAIM_WRITE),
-		.may_swap = !!(zone_reclaim_mode & RECLAIM_SWAP),
 		.nr_to_reclaim = max_t(unsigned long, nr_pages,
 				       SWAP_CLUSTER_MAX),
 		.gfp_mask = gfp_mask,
-		.swappiness = vm_swappiness,
 		.order = order,
 	};
 	unsigned long slab_reclaimable;
 
+	if (zone_reclaim_mode & RECLAIM_WRITE)
+		sc.may_writepage = 1;
+	if (zone_reclaim_mode & RECLAIM_SWAP)
+		sc.swappiness = vm_swappiness;
+
 	disable_swap_token();
 	cond_resched();
 	/*


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

Re: [patch 1/5] vmscan: fix unmapping behaviour for RECLAIM_SWAP

[KOSAKI Motohiro]

sorry for the long delayed review.

> 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 <hannes@cmpxchg.org>
> ---
>  mm/vmscan.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -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,
> +		.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.

But if mel ack this, I have no objection.


----------------------------------------------------------
commit 90afa5de6f3fa89a733861e843377302479fcf7e
Author: Mel Gorman <mel@csn.ul.ie>
Date:   Tue Jun 16 15:33:20 2009 -0700

diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
index 0ea5adb..c4de635 100644
--- a/Documentation/sysctl/vm.txt
+++ b/Documentation/sysctl/vm.txt
@@ -315,10 +315,14 @@ min_unmapped_ratio:

 This is available only on NUMA kernels.

-A percentage of the total pages in each zone.  Zone reclaim will only
-occur if more than this percentage of pages are file backed and unmapped.
-This is to insure that a minimal amount of local pages is still available for
-file I/O even if the node is overallocated.
+This is a percentage of the total pages in each zone. Zone reclaim will
+only occur if more than this percentage of pages are in a state that
+zone_reclaim_mode allows to be reclaimed.
+
+If zone_reclaim_mode has the value 4 OR'd, then the percentage is compared
+against all file-backed unmapped pages including swapcache pages and tmpfs
+files. Otherwise, only unmapped pages backed by normal files but not tmpfs
+files and similar are considered.


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

Re: [patch 3/5] vmscan: remove all_unreclaimable scan control

[KOSAKI Motohiro]

> @@ -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 <kosaki.motohiro@jp.fujitsu.com>




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

Re: [patch 4/5] vmscan: remove isolate_pages callback scan control

[KOSAKI Motohiro]

> 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 <hannes@cmpxchg.org>
> ---
>  include/linux/memcontrol.h |   13 ++++++-----
>  mm/vmscan.c                |   52 ++++++++++++++++++++++++---------------------
>  2 files changed, 35 insertions(+), 30 deletions(-)
> 
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -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);
> +			/*
> +			 * 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.




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

Re: [patch 5/5] vmscan: remove may_swap scan control

[KOSAKI Motohiro]

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



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

Re: [patch 1/5] vmscan: fix unmapping behaviour for RECLAIM_SWAP

[Johannes Weiner]

Hi,

On Thu, May 13, 2010 at 12:02:53PM +0900, KOSAKI Motohiro wrote:
> sorry for the long delayed review.

Yeah, I'm a bit on the slow side as well these days.  No problem.

> > 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 <hannes@cmpxchg.org>
> > ---
> >  mm/vmscan.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -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,
> > +		.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.

Interesting, let me dig through the archives.

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

Yep.

Thanks,
	Hannes

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

Re: [patch 3/5] vmscan: remove all_unreclaimable scan control

[Johannes Weiner]

On Thu, May 13, 2010 at 12:25:36PM +0900, KOSAKI Motohiro wrote:
> > @@ -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' ;)

Guess you are right, let me send a follow-up.

> otherwise looks good.
> 	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Thank you.

	Hannes

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

Re: [patch 4/5] vmscan: remove isolate_pages callback scan control

[Johannes Weiner]

On Thu, May 13, 2010 at 12:29:05PM +0900, KOSAKI Motohiro wrote:
> > 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 <hannes@cmpxchg.org>
> > ---
> >  include/linux/memcontrol.h |   13 ++++++-----
> >  mm/vmscan.c                |   52 ++++++++++++++++++++++++---------------------
> >  2 files changed, 35 insertions(+), 30 deletions(-)
> > 
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -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);
> > +			/*
> > +			 * 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.

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?

	Hannes

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

Re: [patch 5/5] vmscan: remove may_swap scan control

[Johannes Weiner]

On Thu, May 13, 2010 at 12:36:12PM +0900, KOSAKI Motohiro wrote:
> > 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!

	Hannes

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

Re: [patch 4/5] vmscan: remove isolate_pages callback scan control

[KOSAKI Motohiro]

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

Ah. ok, I see.



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

Re: [patch 5/5] vmscan: remove may_swap scan control

[KOSAKI Motohiro]

> On Thu, May 13, 2010 at 12:36:12PM +0900, KOSAKI Motohiro wrote:
> > > 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!

Yup, -1 is perfectly acceptable.

Moreover, I hope such strange habbit will disappear int the future.
I think our recent activity help to change their mind.
At that time, we can change the code more radically.


Thanks.




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

Re: [patch 3/5] vmscan: remove all_unreclaimable scan control

[Greg Thelen]

Johannes Weiner <hannes@cmpxchg.org> writes:
> 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 <hannes@cmpxchg.org>
> ---
>  mm/vmscan.c |   14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -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
> @@ -1826,7 +1824,7 @@ static unsigned long do_try_to_free_page
>  			congestion_wait(BLK_RW_ASYNC, HZ/10);
>  	}
>  	/* top priority shrink_zones still had more to do? don't OOM, then */
> -	if (!sc->all_unreclaimable && scanning_global_lru(sc))
> +	if (ret && scanning_global_lru(sc))
>  		ret = sc->nr_reclaimed;
>  out:
>  	/*

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.

If I apply the following fix, then your patch does not hang and the
system appears to operate correctly.
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 915dceb..772913c 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1850,7 +1850,7 @@ static unsigned long do_try_to_free_pages(struct
zonelist *zonelist,
                        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))
+       if (ret)
                ret = sc->nr_reclaimed;
 out:
        /*

I have not done thorough testing, so this may introduce other problems.
Is there a reason not return nr_reclaimed when operating on a cgroup?
This may affect mem_cgroup_hierarchical_reclaim().

--
Greg

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

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

[KOSAKI Motohiro]

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>

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

[KAMEZAWA Hiroyuki]

On Tue,  1 Jun 2010 12:29:41 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

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

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Thank you very much!!


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

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

[Balbir Singh]

* KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> [2010-06-01 12:29:41]:

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

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

Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>
 
 
-- 
	Three Cheers,
	Balbir

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

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

[Greg Thelen]

KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> writes:
> 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?

Your fixing patch looks good and passes the test which detected the
issue.  Thank you.

Tested-by: Greg Thelen <gthelen@google.com>

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

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

[KAMEZAWA Hiroyuki]

On Tue, 1 Jun 2010 13:40:59 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> [2010-06-01 12:29:41]:
> 
> > 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.
> >
> 
> The patch seems reasonable to me, although I've not tested it
> 
> Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>
>  
Don't worry, I tested.

Thanks,
-Kame

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

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

[Johannes Weiner]

On Tue, Jun 01, 2010 at 12:29:41PM +0900, KOSAKI Motohiro wrote:
> 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>

Greg, thanks for this great bug report.  KOSAKI-san, thanks a lot
for fixing it.  And I am sorry I did not reply earlier, there is
just too much going on right now :/

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

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