linux-kernel - Re: [PATCH] mm: vmscan: fix malused nr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJd=RBByNhLSiBtyaYOHeMRQpXmAO=hEKTOanPTzrb2gRZTOSg@mail.gmail.com>
Date:	Tue, 24 Jan 2012 19:00:19 +0800
From:	Hillf Danton <dhillf@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-mm@...ck.org,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Mel Gorman <mgorman@...e.de>, Rik van Riel <riel@...hat.com>,
	Hugh Dickins <hughd@...gle.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm: vmscan: fix malused nr_reclaimed in shrinking zone

On Tue, Jan 24, 2012 at 9:03 AM, Andrew Morton
<akpm@...ux-foundation.org> 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@...il.com>
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@...il.com>
---

--- 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/