vmscan: initialize sc->nr_reclaimed in do_try_to_free_pages()

From the patch description I couldn't really make sure what the

Commit a79311c14eae4bb946a97af25f3e1b17d625985d "vmscan: bail out of direct reclaim after swap_cluster_max pages" moved the nr_reclaimed counter into the scan control to accumulate the number of all reclaimed pages in one direct reclaim invocation.

The commit missed to actually adjust do_try_to_free_pages() which now does not initialize sc.nr_reclaimed and makes shrink_zone() make assumptions on whether to bail out of the reclaim cycle based on an uninitialized value.

Fix it up by initializing the counter to zero before entering the priority loop.

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

---
 mm/vmscan.c |    1 +
 1 file changed, 1 insertion(+)

The comment of the .nr_reclaimed field says it accumulates the reclaim counter over ONE shrink_zones() call. This means, we should break out if ONE shrink_zones() call alone does more than swap_cluster_max.

OTOH, the patch title suggests that we break out if ALL shrink_zones() calls in the priority loop have reclaimed that much. I.e. accumulating the reclaimed number over the prio loop, not just over one zones iteration.

From the patch description I couldn't really make sure what the intended behaviour was.

So, should the sc.nr_reclaimed be reset before the prio loop or in each iteration of the prio loop?

Either this patch is wrong or the comment above .nr_reclaimed is.

And why didn't this have any observable effects? Do I miss something

really obvious here?

---

mm/vmscan.c +1 -0

index 9a27c44..18406ee 100644
@@ -1699,6 +1699,7 @@ unsigned long try_to_free_pages(struct zonelist
 *zonelist, int order,
                .order = order,
                .mem_cgroup = NULL,
                .isolate_pages = isolate_pages_global,
+               .nr_reclaimed = 0,
        };
 

        return do_try_to_free_pages(zonelist, &sc);

Quoted 15 lines

---

mm/vmscan.c +1 -0

index 9a27c44..18406ee 100644
@@ -1699,6 +1699,7 @@ unsigned long try_to_free_pages(struct zonelist
 *zonelist, int order,
                .order = order,
                .mem_cgroup = NULL,
                .isolate_pages = isolate_pages_global,
+               .nr_reclaimed = 0,
        };
 

        return do_try_to_free_pages(zonelist, &sc);

mm/vmscan.c unsigned long · line 1726

Quoted 7 lines

mm/vmscan.c +1 -0

@@ -1719,6 +1720,7 @@ unsigned long
 try_to_free_mem_cgroup_pages(struct mem_cgroup *mem_cont,
                .order = 0,
                .mem_cgroup = mem_cont,
                .isolate_pages = mem_cgroup_isolate_pages,
+               .nr_reclaimed = 0;
        };
        struct zonelist *zonelist;

I think this code is better.

and, I think we also need to

static int __zone_reclaim(struct zone *zone, gfp_t gfp_mask, unsigned int order) {
/* Minimum pages needed in order to stay on node */
const unsigned long nr_pages = 1 << order;
struct task_struct *p = current;
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), .swap_cluster_max = max_t(unsigned long, nr_pages,
SWAP_CLUSTER_MAX),
.gfp_mask = gfp_mask,
.swappiness = vm_swappiness,
.isolate_pages = isolate_pages_global,

+               .nr_reclaimed = 0;
       };

Quoted 15 lines

static int __zone_reclaim(struct zone *zone, gfp_t gfp_mask, unsigned int order) {
/* Minimum pages needed in order to stay on node */
const unsigned long nr_pages = 1 << order;
struct task_struct *p = current;
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), .swap_cluster_max = max_t(unsigned long, nr_pages,
SWAP_CLUSTER_MAX),
.gfp_mask = gfp_mask,
.swappiness = vm_swappiness,
.isolate_pages = isolate_pages_global,

Quoted 2 lines

+               .nr_reclaimed = 0;
       };

Hmm.. I missed that. Thanks.
There is one in shrink_all_memory.

Quoted 18 lines

Quoted 17 lines

static int __zone_reclaim(struct zone *zone, gfp_t gfp_mask, unsigned int order) {
/* Minimum pages needed in order to stay on node */
const unsigned long nr_pages = 1 << order;
struct task_struct *p = current;
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), .swap_cluster_max = max_t(unsigned long, nr_pages,
SWAP_CLUSTER_MAX),
.gfp_mask = gfp_mask,
.swappiness = vm_swappiness,
.isolate_pages = isolate_pages_global,

+               .nr_reclaimed = 0;
       };

Quoted 2 lines

Hmm.. I missed that. Thanks.
There is one in shrink_all_memory.

No.
__zone_reclaim isn't a part of shrink_all_memory(). Currently, shrink_all_memory() don't use sc.nr_reclaimed member. (maybe, it's another wrong thing ;)

Hmm.. You're right.
As Johannes pointed out,
too many page shrinking can degrade resume performance.

We need to bale out in shrink_all_memory.
Other people, thought ?

Quoted 6 lines

Hmm.. You're right.
As Johannes pointed out,
too many page shrinking can degrade resume performance.

We need to bale out in shrink_all_memory.
Other people, thought ?

shrink_all_zones() already have bale-out code ;)

Impact: cleanup

Commit a79311c14eae4bb946a97af25f3e1b17d625985d "vmscan: bail out of direct reclaim after swap_cluster_max pages" moved the nr_reclaimed counter into the scan control to accumulate the number of all reclaimed pages in a reclaim invocation.

shrink_all_memory() can use the same mechanism. it increase code consistency.

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

Cc MinChan Kim <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Rik van Riel <[email protected]>

---
 mm/vmscan.c |   49 ++++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

Index: b/mm/vmscan.c
===================================================================

mm/vmscan.c +1 -2

@@ -2004,16 +2004,15 @@ unsigned long global_lru_pages(void)
 #ifdef CONFIG_PM
 /*
  * Helper function for shrink_all_memory().  Tries to reclaim 'nr_pages' pages
- * from LRU lists system-wide, for given pass and priority, and returns the
- * number of reclaimed pages
+ * from LRU lists system-wide, for given pass and priority.
  *

mm/vmscan.c +15 -17

 	for_each_zone(zone) {

@@ -2038,15 +2037,13 @@ static unsigned long shrink_all_zones(un
 				nr_to_scan = min(nr_pages,
 					zone_page_state(zone,
 							NR_LRU_BASE + l));
-				ret += shrink_list(l, nr_to_scan, zone,
-								sc, prio);
-				if (ret >= nr_pages)
-					return ret;
+				sc->nr_reclaimed += shrink_list(l, nr_to_scan,
+								zone, sc, prio);
+				if (sc->nr_reclaimed >= nr_pages)
+					return;
 			}
 		}
 	}
-
-	return ret;
 }
 
 /*

@@ -2060,10 +2057,10 @@ static unsigned long shrink_all_zones(un
 unsigned long shrink_all_memory(unsigned long nr_pages)
 {
 	unsigned long lru_pages, nr_slab;
-	unsigned long ret = 0;
 	int pass;
 	struct reclaim_state reclaim_state;
 	struct scan_control sc = {
+		.nr_reclaimed = 0,
 		.gfp_mask = GFP_KERNEL,
 		.may_swap = 0,
 		.swap_cluster_max = nr_pages,

@@ -2083,8 +2080,8 @@ unsigned long shrink_all_memory(unsigned
 		if (!reclaim_state.reclaimed_slab)
 			break;
 
-		ret += reclaim_state.reclaimed_slab;
-		if (ret >= nr_pages)
+		sc.nr_reclaimed += reclaim_state.reclaimed_slab;
+		if (sc.nr_reclaimed >= nr_pages)
 			goto out;
 
 		nr_slab -= reclaim_state.reclaimed_slab;

@@ -2108,18 +2105,18 @@ unsigned long shrink_all_memory(unsigned
 		}
 
 		for (prio = DEF_PRIORITY; prio >= 0; prio--) {
-			unsigned long nr_to_scan = nr_pages - ret;
+			unsigned long nr_to_scan = nr_pages - sc.nr_reclaimed;
 
 			sc.nr_scanned = 0;
-			ret += shrink_all_zones(nr_to_scan, prio, pass, &sc);
-			if (ret >= nr_pages)
+			shrink_all_zones(nr_to_scan, prio, pass, &sc);
+			if (sc.nr_reclaimed >= nr_pages)
 				goto out;
 
 			reclaim_state.reclaimed_slab = 0;
 			shrink_slab(sc.nr_scanned, sc.gfp_mask,
 					global_lru_pages());
-			ret += reclaim_state.reclaimed_slab;
-			if (ret >= nr_pages)
+			sc.nr_reclaimed += reclaim_state.reclaimed_slab;
+			if (sc.nr_reclaimed >= nr_pages)
 				goto out;
 
 			if (sc.nr_scanned && prio < DEF_PRIORITY - 2)

@@ -2128,21 +2125,23 @@ unsigned long shrink_all_memory(unsigned
 	}
 
 	/*
-	 * If ret = 0, we could not shrink LRUs, but there may be something
-	 * in slab caches
+	 * If sc.nr_reclaimed = 0, we could not shrink LRUs, but there may be
+	 * something in slab caches
 	 */
-	if (!ret) {
+	if (!sc.nr_reclaimed) {

mm/vmscan.c unsigned long global_lru_pages(void) · line 2016

Quoted 5 lines

 {
      struct zone *zone;

mm/vmscan.c +1 -1

-     unsigned long nr_to_scan, ret = 0;
+     unsigned long nr_to_scan;
      enum lru_list l;

Basing it on swsusp-clean-up-shrink_all_zones.patch probably makes it easier for Andrew to pick it up.

mm/vmscan.c unsigned long shrink_all_memory(unsigned · line 2138

Quoted 9 lines

                      reclaim_state.reclaimed_slab = 0;

mm/vmscan.c +5 -3

-                     shrink_slab(nr_pages, sc.gfp_mask, global_lru_pages());
-                     ret += reclaim_state.reclaimed_slab;
-             } while (ret < nr_pages && reclaim_state.reclaimed_slab > 0);
+                     shrink_slab(nr_pages, sc.gfp_mask,
+                                 global_lru_pages());
+                     sc.nr_reclaimed += reclaim_state.reclaimed_slab;
+             } while (sc.nr_reclaimed < nr_pages &&
+                      reclaim_state.reclaimed_slab > 0);

:(

Is this really an improvement? ret' is better to read than sc.nr_reclaimed'.

Quoted 8 lines

Review

Quoted 5 lines

 {
      struct zone *zone;

(quoted diff) +1 -1

-     unsigned long nr_to_scan, ret = 0;
+     unsigned long nr_to_scan;
      enum lru_list l;

Basing it on swsusp-clean-up-shrink_all_zones.patch probably makes it easier for Andrew to pick it up.

ok, thanks.

Quoted 10 lines

Quoted 9 lines

                      reclaim_state.reclaimed_slab = 0;

(quoted diff) +5 -3

-                     shrink_slab(nr_pages, sc.gfp_mask, global_lru_pages());
-                     ret += reclaim_state.reclaimed_slab;
-             } while (ret < nr_pages && reclaim_state.reclaimed_slab > 0);
+                     shrink_slab(nr_pages, sc.gfp_mask,
+                                 global_lru_pages());
+                     sc.nr_reclaimed += reclaim_state.reclaimed_slab;
+             } while (sc.nr_reclaimed < nr_pages &&
+                      reclaim_state.reclaimed_slab > 0);

Quoted 4 lines

:(

Is this really an improvement? ret' is better to read than sc.nr_reclaimed'.

I know it's debetable thing.
but I still think code consistency is important than variable name preference.

Quoted 21 lines

Quoted 8 lines

Review

Quoted 5 lines

 {
      struct zone *zone;

(quoted diff) +1 -1

-     unsigned long nr_to_scan, ret = 0;
+     unsigned long nr_to_scan;
      enum lru_list l;

Basing it on swsusp-clean-up-shrink_all_zones.patch probably makes it easier for Andrew to pick it up.

ok, thanks.

Quoted 10 lines

Quoted 9 lines

                      reclaim_state.reclaimed_slab = 0;

(quoted diff) +5 -3

-                     shrink_slab(nr_pages, sc.gfp_mask, global_lru_pages());
-                     ret += reclaim_state.reclaimed_slab;
-             } while (ret < nr_pages && reclaim_state.reclaimed_slab > 0);
+                     shrink_slab(nr_pages, sc.gfp_mask,
+                                 global_lru_pages());
+                     sc.nr_reclaimed += reclaim_state.reclaimed_slab;
+             } while (sc.nr_reclaimed < nr_pages &&
+                      reclaim_state.reclaimed_slab > 0);

Quoted 7 lines

Quoted 4 lines

:(

Is this really an improvement? ret' is better to read than sc.nr_reclaimed'.

I know it's debetable thing.
but I still think code consistency is important than variable name preference.

How about this ?

I followed do_try_to_free_pages coding style. It use both 'sc->nr_reclaimed' and 'ret'. It can support code consistency and readability.

So, I think it would be better.
If you don't mind, I will resend with your sign-off.

Quoted 30 lines · KOSAKI Motohiro

Quoted 21 lines

Quoted 8 lines

Review

Quoted 5 lines

 {
      struct zone *zone;

(quoted diff) +1 -1

-     unsigned long nr_to_scan, ret = 0;
+     unsigned long nr_to_scan;
      enum lru_list l;

Basing it on swsusp-clean-up-shrink_all_zones.patch probably makes it easier for Andrew to pick it up.

ok, thanks.

Quoted 10 lines

Quoted 9 lines

                      reclaim_state.reclaimed_slab = 0;

(quoted diff) +5 -3

-                     shrink_slab(nr_pages, sc.gfp_mask, global_lru_pages());
-                     ret += reclaim_state.reclaimed_slab;
-             } while (ret < nr_pages && reclaim_state.reclaimed_slab > 0);
+                     shrink_slab(nr_pages, sc.gfp_mask,
+                                 global_lru_pages());
+                     sc.nr_reclaimed += reclaim_state.reclaimed_slab;
+             } while (sc.nr_reclaimed < nr_pages &&
+                      reclaim_state.reclaimed_slab > 0);

Quoted 7 lines

Quoted 4 lines

:(

Is this really an improvement? ret' is better to read than sc.nr_reclaimed'.

I know it's debetable thing.
but I still think code consistency is important than variable name preference.

Quoted 8 lines

How about this ?

I followed do_try_to_free_pages coding style. It use both 'sc->nr_reclaimed' and 'ret'. It can support code consistency and readability.

So, I think it would be better.
If you don't mind, I will resend with your sign-off.

looks good. thanks.

mm/vmscan.c unsigned long global_lru_pages(void) · line 2061

Quoted 8 lines

mm/vmscan.c +2 -1

-static unsigned long shrink_all_zones(unsigned long nr_pages, int prio,
+static void shrink_all_zones(unsigned long nr_pages, int prio,
 				      int pass, struct scan_control *sc)
 {
 	struct zone *zone;
 	unsigned long nr_to_scan, ret = 0;
+	unsigned long nr_reclaimed = sc->nr_reclaimed;
 	enum lru_list l;

and, please changelog change.
this patch have behavior change.

old bale-out checking didn't checked properly.
it's because shrink_all_memory() has five pass. but shrink_all_zones() initialize ret = 0 every time.

then, at pass 1-4, if(ret >= nr_pages) don't judge reclaimed enough page or not.

Hmm, I think old bale-out code is right.
In shrink_all_memory, As more reclaiming with pass progressing,
the smaller nr_to_scan is. The nr_to_scan is the number of page shrinking which user want.
The shrink_all_zones have to reclaim nr_to_scan's page by doing best effort. So, If you use accumulation of reclaim, it can break bale-out in shrink_all_zones.

I mean here.

'

              NR_LRU_BASE + l)); 
        ret += shrink_list(l, nr_to_scan, zone,
                sc, prio);
        if (ret >= nr_pages)
          return ret; 
      }    

'

I have to make patch again so that it will keep on old bale-out behavior.

Quoted 11 lines

I mean here.

'

              NR_LRU_BASE + l)); 
        ret += shrink_list(l, nr_to_scan, zone,
                sc, prio);
        if (ret >= nr_pages)
          return ret; 
      }    

'

Quoted 1 line

I have to make patch again so that it will keep on old bale-out behavior.

Sure.
thanks.

How about this?

mm/vmscan.c +2 -0

@@ -1665,6 +1665,7 @@ unsigned long try_to_free_pages(struct z
 								gfp_t gfp_mask)
 {
 	struct scan_control sc = {
+		.nr_reclaimed = 0,
 		.gfp_mask = gfp_mask,
 		.may_writepage = !laptop_mode,
 		.swap_cluster_max = SWAP_CLUSTER_MAX,

@@ -1686,6 +1687,7 @@ unsigned long try_to_free_mem_cgroup_pag
 					   unsigned int swappiness)
 {
 	struct scan_control sc = {
+		.nr_reclaimed = 0,
 		.may_writepage = !laptop_mode,
 		.may_swap = 1,
 		.swap_cluster_max = SWAP_CLUSTER_MAX,

Quoted 21 lines · On Tue, 10 Feb 2009 21:58:04 +0900 (JST) KOSAKI Motohiro…

mm/vmscan.c +2 -0

@@ -1665,6 +1665,7 @@ unsigned long try_to_free_pages(struct z
 								gfp_t gfp_mask)
 {
 	struct scan_control sc = {
+		.nr_reclaimed = 0,
 		.gfp_mask = gfp_mask,
 		.may_writepage = !laptop_mode,
 		.swap_cluster_max = SWAP_CLUSTER_MAX,

@@ -1686,6 +1687,7 @@ unsigned long try_to_free_mem_cgroup_pag
 					   unsigned int swappiness)
 {
 	struct scan_control sc = {
+		.nr_reclaimed = 0,
 		.may_writepage = !laptop_mode,
 		.may_swap = 1,
 		.swap_cluster_max = SWAP_CLUSTER_MAX,

@@ -2245,6 +2247,7 @@ static int __zone_reclaim(struct zone *z
 	struct reclaim_state reclaim_state;
 	int priority;

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

Quoted 5 lines

mm/vmscan.c +1 -0

 	struct scan_control sc = {
+		.nr_reclaimed = 0,
 		.may_writepage = !!(zone_reclaim_mode & RECLAIM_WRITE),
 		.may_swap = !!(zone_reclaim_mode & RECLAIM_SWAP),
 		.swap_cluster_max = max_t(unsigned long, nr_pages,

Confused. The compiler already initialises any unmentioned fields to zero, so this patch has no effect.

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

Quoted 8 lines

mm/vmscan.c +1 -0

@@ -2245,6 +2247,7 @@ static int __zone_reclaim(struct zone *z
 	struct reclaim_state reclaim_state;
 	int priority;
 	struct scan_control sc = {
+		.nr_reclaimed = 0,
 		.may_writepage = !!(zone_reclaim_mode & RECLAIM_WRITE),
 		.may_swap = !!(zone_reclaim_mode & RECLAIM_SWAP),
 		.swap_cluster_max = max_t(unsigned long, nr_pages,

Confused. The compiler already initialises any unmentioned fields to zero, so this patch has no effect.

The commit missed to actually adjust do_try_to_free_pages() which now does not initialize sc.nr_reclaimed and makes shrink_zone() make assumptions on whether to bail out of the reclaim cycle based on an uninitialized value.