* [PATCH] mm: vmscan: fix malused nr_reclaimed in shrinking zone
@ 2012-01-21 14:41 Hillf Danton
2012-01-24 1:03 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: Hillf Danton @ 2012-01-21 14:41 UTC (permalink / raw)
To: linux-mm
Cc: KOSAKI Motohiro, Mel Gorman, Rik van Riel, Hugh Dickins,
Andrew Morton, LKML, Hillf Danton
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);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm: vmscan: fix malused nr_reclaimed in shrinking zone
2012-01-21 14:41 [PATCH] mm: vmscan: fix malused nr_reclaimed in shrinking zone Hillf Danton
@ 2012-01-24 1:03 ` Andrew Morton
2012-01-24 11:00 ` Hillf Danton
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2012-01-24 1:03 UTC (permalink / raw)
To: Hillf Danton
Cc: linux-mm, KOSAKI Motohiro, Mel Gorman, Rik van Riel, Hugh Dickins, LKML
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm: vmscan: fix malused nr_reclaimed in shrinking zone
2012-01-24 1:03 ` Andrew Morton
@ 2012-01-24 11:00 ` Hillf Danton
2012-01-26 1:00 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: Hillf Danton @ 2012-01-24 11:00 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, KOSAKI Motohiro, Mel Gorman, Rik van Riel, Hugh Dickins, LKML
On Tue, Jan 24, 2012 at 9:03 AM, Andrew Morton
<akpm@linux-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@gmail.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@gmail.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);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm: vmscan: fix malused nr_reclaimed in shrinking zone
2012-01-24 11:00 ` Hillf Danton
@ 2012-01-26 1:00 ` Andrew Morton
2012-01-26 3:19 ` Hillf Danton
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2012-01-26 1:00 UTC (permalink / raw)
To: Hillf Danton
Cc: linux-mm, KOSAKI Motohiro, Mel Gorman, Rik van Riel, Hugh Dickins, LKML
On Tue, 24 Jan 2012 19:00:19 +0800
Hillf Danton <dhillf@gmail.com> wrote:
> On Tue, Jan 24, 2012 at 9:03 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >
> >
> > - 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.
umph. It's still not exactly a thing of beautiful clarity :(
> 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@gmail.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);
So local variable nr_to_reclaim has had its meaning changed. It used
to be a function-wide constant (should have actually been marked
"const") telling us how many pages we are asked to reclaim.
But now it becomes "remaining number of pages to reclaim". And the
name happens to still be sufficiently appropriate, so fair enough.
I'm thinking we have a bit of code rot happening here. This comment:
/*
* On large memory systems, scan >> priority can become
* really large. This is fine for the starting priority;
* we want to put equal scanning pressure on each zone.
* However, if the VM has a harder time of freeing pages,
* with multiple processes reclaiming pages, the total
* freeing target can get unreasonably large.
*/
seems to have little to do with the code which it is trying to
describe. Or at least, I'm not sure this is the best we can possibly
do :(
Also, your email client is adding MIME goop to the emails which mine
(sylpheed) is unable to decrypt. It turns "=" into "=3D" everywhere.
This:
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
I blame sylpheed for this, but if you can make it stop, that would make
my life easier, and perhaps others.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm: vmscan: fix malused nr_reclaimed in shrinking zone
2012-01-26 1:00 ` Andrew Morton
@ 2012-01-26 3:19 ` Hillf Danton
0 siblings, 0 replies; 5+ messages in thread
From: Hillf Danton @ 2012-01-26 3:19 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, KOSAKI Motohiro, Mel Gorman, Rik van Riel, Hugh Dickins, LKML
Hi Andrew
On Thu, Jan 26, 2012 at 9:00 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> I'm thinking we have a bit of code rot happening here. This comment:
>
> /*
> * On large memory systems, scan >> priority can become
> * really large. This is fine for the starting priority;
> * we want to put equal scanning pressure on each zone.
> * However, if the VM has a harder time of freeing pages,
> * with multiple processes reclaiming pages, the total
> * freeing target can get unreasonably large.
> */
>
> seems to have little to do with the code which it is trying to
> describe. Or at least, I'm not sure this is the best we can possibly
> do :(
>
Try to do it soon 8-)
>
> Also, your email client is adding MIME goop to the emails which mine
> (sylpheed) is unable to decrypt. It turns "=" into "=3D" everywhere.
> This:
>
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: quoted-printable
>
> I blame sylpheed for this, but if you can make it stop, that would make
> my life easier, and perhaps others.
>
More care will take gmail later on.
Thanks
Hillf
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-01-26 3:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-21 14:41 [PATCH] mm: vmscan: fix malused nr_reclaimed in shrinking zone Hillf Danton
2012-01-24 1:03 ` Andrew Morton
2012-01-24 11:00 ` Hillf Danton
2012-01-26 1:00 ` Andrew Morton
2012-01-26 3:19 ` Hillf Danton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).