2006-01-20 00:05:45

by Christoph Lameter

[permalink] [raw]
Subject: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages

The check for laptop mode and sc->may_writepage is intended to not write
pages if either laptop mode is set or we are not allowed to write.

The && there means that currently pages may be written in laptop mode and during
zone_reclaim. This patch also applies to 2.6.15 and 2.6.14!

Signed-off-by: Christoph Lameter <[email protected]>

Index: linux-2.6.16-rc1-mm1/mm/vmscan.c
===================================================================
--- linux-2.6.16-rc1-mm1.orig/mm/vmscan.c 2006-01-19 15:40:28.000000000 -0800
+++ linux-2.6.16-rc1-mm1/mm/vmscan.c 2006-01-19 15:40:30.000000000 -0800
@@ -491,7 +491,7 @@ static int shrink_list(struct list_head
goto keep_locked;
if (!may_enter_fs)
goto keep_locked;
- if (laptop_mode && !sc->may_writepage)
+ if (laptop_mode || !sc->may_writepage)
goto keep_locked;

/* Page is dirty, try to write it out here */


2006-01-20 00:42:27

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages

Christoph Lameter <[email protected]> wrote:
>
> The check for laptop mode and sc->may_writepage is intended to not write
> pages if either laptop mode is set or we are not allowed to write.
>
> The && there means that currently pages may be written in laptop mode and during
> zone_reclaim. This patch also applies to 2.6.15 and 2.6.14!
>
> Signed-off-by: Christoph Lameter <[email protected]>
>
> Index: linux-2.6.16-rc1-mm1/mm/vmscan.c
> ===================================================================
> --- linux-2.6.16-rc1-mm1.orig/mm/vmscan.c 2006-01-19 15:40:28.000000000 -0800
> +++ linux-2.6.16-rc1-mm1/mm/vmscan.c 2006-01-19 15:40:30.000000000 -0800
> @@ -491,7 +491,7 @@ static int shrink_list(struct list_head
> goto keep_locked;
> if (!may_enter_fs)
> goto keep_locked;
> - if (laptop_mode && !sc->may_writepage)
> + if (laptop_mode || !sc->may_writepage)
> goto keep_locked;
>
> /* Page is dirty, try to write it out here */

erk.

The effects of this fix will be a) slightly improved memory allocator
latency, b) somehat improved disk writeout patterns and c) somewhat
increased risk of ooms.

So we'll need to sit on it for quite some time to let it settle in, thanks.

2006-01-20 00:50:43

by Christoph Lameter

[permalink] [raw]
Subject: Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages

On Thu, 19 Jan 2006, Andrew Morton wrote:

> The effects of this fix will be a) slightly improved memory allocator
> latency, b) somehat improved disk writeout patterns and c) somewhat
> increased risk of ooms.

If we do not operate in laptop mode and are not using zone_reclaim
(!may_writepage) which is the common case then there will be no effect at
all.

2006-01-20 01:18:39

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages

Christoph Lameter <[email protected]> wrote:
>
> On Thu, 19 Jan 2006, Andrew Morton wrote:
>
> > The effects of this fix will be a) slightly improved memory allocator
> > latency, b) somehat improved disk writeout patterns and c) somewhat
> > increased risk of ooms.
>
> If we do not operate in laptop mode and are not using zone_reclaim
> (!may_writepage) which is the common case then there will be no effect at
> all.

Ah, I misremembered how the code works. Your patch will break laptop mode.

They way it works is:

laptop_mode=0: always write out dirty pages which come off the tail of the LRU.

laptop_mode=1: don't write out dirty pages if we're only performing light
scanning. But do write them out once page reclaim starts getting into
difficulty.

The idea is that in laptop mode we'll avoid spinning up the disk for
occasional random dirty pages which are interspersed amongst a majority of
clean, reclaimable pages. But once reclaim is getting into trouble, we
need to spin that disk up anyway to clean out some memory.

Your patch means that in laptop moe we'll just never write out these dirty
pages ever - we'll overscan and go oom.


I guess if zone reclaim wants to permanently disable writeback then it'll
be needing a new scan_control flag for that. Which means that we need to
remember to initialise that flag in lots of different places, which is why
I dislike scan_control. A (separate, preceding) patch which converts
scan_control initialisation to do memset+initialise-non-zero-members would
be appreciated.


2006-01-20 01:30:27

by Christoph Lameter

[permalink] [raw]
Subject: Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages

On Thu, 19 Jan 2006, Andrew Morton wrote:

> laptop_mode=1: don't write out dirty pages if we're only performing light
> scanning. But do write them out once page reclaim starts getting into
> difficulty.

Ahh. Now I see ..... Wrong field.

> I guess if zone reclaim wants to permanently disable writeback then it'll
> be needing a new scan_control flag for that. Which means that we need to

We have such a flag and its called may_swap (was introduced by Martin
Hicks). If we cannot swap then we should not write out pages.

Thus we need this fix instead:

Signed-off-by: Christoph Lameter <[email protected]>

Index: linux-2.6.16-rc1-mm1/mm/vmscan.c
===================================================================
--- linux-2.6.16-rc1-mm1.orig/mm/vmscan.c 2006-01-19 15:50:19.000000000 -0800
+++ linux-2.6.16-rc1-mm1/mm/vmscan.c 2006-01-19 17:26:50.000000000 -0800
@@ -491,7 +491,7 @@ static int shrink_list(struct list_head
goto keep_locked;
if (!may_enter_fs)
goto keep_locked;
- if (laptop_mode && !sc->may_writepage)
+ if ((laptop_mode && !sc->may_writepage) || !sc->may_swap)
goto keep_locked;

/* Page is dirty, try to write it out here */

2006-01-20 01:46:40

by Christoph Lameter

[permalink] [raw]
Subject: Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages

This is all crap. may_writepage needs to do as it says and control
write behavior .... We can set the proper writemode in try_to_free_pages
based on the laptop mode. Then everything falls into the proper place.



[PATCH] Implement sane function of sc->may_writepage

Make sc->may_writepage control the writeout behavior of shrink_list.

Remove the laptop_mode trick from shrink_list and instead set may_writepage in
try_to_free_pages properly.

Signed-off-by: Christoph Lameter <[email protected]>

Index: linux-2.6.16-rc1-mm1/mm/vmscan.c
===================================================================
--- linux-2.6.16-rc1-mm1.orig/mm/vmscan.c 2006-01-19 15:50:19.000000000 -0800
+++ linux-2.6.16-rc1-mm1/mm/vmscan.c 2006-01-19 17:42:07.000000000 -0800
@@ -491,7 +491,7 @@ static int shrink_list(struct list_head
goto keep_locked;
if (!may_enter_fs)
goto keep_locked;
- if (laptop_mode && !sc->may_writepage)
+ if (!sc->may_writepage)
goto keep_locked;

/* Page is dirty, try to write it out here */
@@ -1409,7 +1409,7 @@ int try_to_free_pages(struct zone **zone
int i;

sc.gfp_mask = gfp_mask;
- sc.may_writepage = 0;
+ sc.may_writepage = !laptop_mode;
sc.may_swap = 1;

inc_page_state(allocstall);
@@ -1512,7 +1512,7 @@ loop_again:
total_scanned = 0;
total_reclaimed = 0;
sc.gfp_mask = GFP_KERNEL;
- sc.may_writepage = 0;
+ sc.may_writepage = 1;
sc.may_swap = 1;
sc.nr_mapped = read_page_state(nr_mapped);

2006-01-20 02:27:40

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages

Christoph Lameter <[email protected]> wrote:
>
> [PATCH] Implement sane function of sc->may_writepage
>
> Make sc->may_writepage control the writeout behavior of shrink_list.
>
> Remove the laptop_mode trick from shrink_list and instead set may_writepage in
> try_to_free_pages properly.
>
> Signed-off-by: Christoph Lameter <[email protected]>
>
> Index: linux-2.6.16-rc1-mm1/mm/vmscan.c
> ===================================================================
> --- linux-2.6.16-rc1-mm1.orig/mm/vmscan.c 2006-01-19 15:50:19.000000000 -0800
> +++ linux-2.6.16-rc1-mm1/mm/vmscan.c 2006-01-19 17:42:07.000000000 -0800
> @@ -491,7 +491,7 @@ static int shrink_list(struct list_head
> goto keep_locked;
> if (!may_enter_fs)
> goto keep_locked;
> - if (laptop_mode && !sc->may_writepage)
> + if (!sc->may_writepage)
> goto keep_locked;
>
> /* Page is dirty, try to write it out here */
> @@ -1409,7 +1409,7 @@ int try_to_free_pages(struct zone **zone
> int i;
>
> sc.gfp_mask = gfp_mask;
> - sc.may_writepage = 0;
> + sc.may_writepage = !laptop_mode;
> sc.may_swap = 1;
>
> inc_page_state(allocstall);
> @@ -1512,7 +1512,7 @@ loop_again:
> total_scanned = 0;
> total_reclaimed = 0;
> sc.gfp_mask = GFP_KERNEL;
> - sc.may_writepage = 0;
> + sc.may_writepage = 1;
> sc.may_swap = 1;
> sc.nr_mapped = read_page_state(nr_mapped);

The balance_pgdat() change is wrong, surely? We want !laptop_mode.

2006-01-20 02:38:17

by Christoph Lameter

[permalink] [raw]
Subject: Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages

On Thu, 19 Jan 2006, Andrew Morton wrote:

> > @@ -1512,7 +1512,7 @@ loop_again:
> > total_scanned = 0;
> > total_reclaimed = 0;
> > sc.gfp_mask = GFP_KERNEL;
> > - sc.may_writepage = 0;
> > + sc.may_writepage = 1;
> > sc.may_swap = 1;
> > sc.nr_mapped = read_page_state(nr_mapped);
>
> The balance_pgdat() change is wrong, surely? We want !laptop_mode.

I did not see any special processing there like in the other place. But
you could be right.