On Tue, Jan 24, 2012 at 9:03 AM, Andrew Morton
[off-list ref] wrote:

Well, let's step back and look at it.

- The multiple-definitions-of-a-local-per-line thing is generally a
 bad idea, partly because it prevents people from adding comments to
 the definition.  It would be better like this:

       unsigned long reclaimed = 0;    /* total for this function */
       unsigned long nr_reclaimed = 0; /* on each pass through the loop */

- The names of these things are terrible!  Why not
 reclaimed_this_pass and reclaimed_total or similar?

- It would be cleaner to do the "reclaimed += nr_reclaimed" at the
 end of the loop, if we've decided to goto restart.  (But better
 to do it within the loop!)

- Only need to update sc->nr_reclaimed at the end of the function
 (assumes that callees of this function aren't interested in
 sc->nr_reclaimed, which seems a future-safe assumption to me).

- Should be able to avoid the temporary addition of nr_reclaimed to
 reclaimed inside the loop by updating `reclaimed' at an appropriate
 place.


Or whatever.  That code's handling of `reclaimed' and `nr_reclaimed' is
a twisty mess.  Please clean it up!  If it is done correctly,
`nr_reclaimed' can (and should) be local to the internal loop.

Hi Andrew

The mess is cleaned up, please review again.

Thanks
Hillf


===cut here===
From: Hillf Danton <redacted>
Subject: [PATCH] mm: vmscan: fix malused nr_reclaimed in shrinking zone

The value of nr_reclaimed is the amount of pages reclaimed in the current
round of loop, whereas nr_to_reclaim should be compared with pages reclaimed
in all rounds.

In each round of loop, reclaimed pages are cut off from the reclaim goal,
and loop stops once goal achieved.

Signed-off-by: Hillf Danton <redacted>
---

--- a/mm/vmscan.c	Mon Jan 23 00:23:10 2012
+++ b/mm/vmscan.c	Tue Jan 24 17:10:34 2012

@@ -2113,7 +2113,12 @@ restart:
 		 * with multiple processes reclaiming pages, the total
 		 * freeing target can get unreasonably large.
 		 */
-		if (nr_reclaimed >= nr_to_reclaim && priority < DEF_PRIORITY)
+		if (nr_reclaimed >= nr_to_reclaim)
+			nr_to_reclaim = 0;
+		else
+			nr_to_reclaim -= nr_reclaimed;
+
+		if (!nr_to_reclaim && priority < DEF_PRIORITY)
 			break;
 	}
 	blk_finish_plug(&plug);

--

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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>