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

Andrew Morton
2012-01-24T01:03:57Z

On Sat, 21 Jan 2012 22:41:59 +0800
Hillf Danton <dhillf@gmail.com> wrote:

> The value of nr_reclaimed is the amount of pages reclaimed in the current round,
> whereas nr_to_reclaim shoud be compared with the amount of pages
> reclaimed in all
> rounds, so we have to buffer the pages reclaimed in the past rounds for correct
> comparison.
>
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
>
> --- a/mm/vmscan.c Sat Jan 14 14:02:20 2012
> +++ b/mm/vmscan.c Sat Jan 21 22:23:48 2012
> @@ -2081,13 +2081,15 @@ static void shrink_mem_cgroup_zone(int p
> struct scan_control *sc)
> {
> unsigned long nr[NR_LRU_LISTS];
> + unsigned long reclaimed = 0;
> unsigned long nr_to_scan;
> enum lru_list lru;
> - unsigned long nr_reclaimed, nr_scanned;
> + unsigned long nr_reclaimed = 0, nr_scanned;
> unsigned long nr_to_reclaim = sc->nr_to_reclaim;
> struct blk_plug plug;
>
> restart:
> + reclaimed += nr_reclaimed;
> nr_reclaimed = 0;
> nr_scanned = sc->nr_scanned;
> get_scan_count(mz, sc, nr, priority);
> @@ -2113,7 +2115,8 @@ 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 + reclaimed) >= nr_to_reclaim &&
> + priority < DEF_PRIORITY)
> break;
> }
> blk_finish_plug(&plug);

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.