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, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. 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