Re: [PATCH] mm: vmscan: fix malused nr_reclaimed in shrinking zone

[Hillf Danton]

On Tue, Jan 24, 2012 at 9:03 AM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> 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 <dhillf@xxxxxxxxx>
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 <dhillf@xxxxxxxxx>
---

--- 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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/