* [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages
@ 2006-01-20 0:05 Christoph Lameter
2006-01-20 0:43 ` Andrew Morton
0 siblings, 1 reply; 8+ messages in thread
From: Christoph Lameter @ 2006-01-20 0:05 UTC (permalink / raw)
To: akpm; +Cc: linux-kernel
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 <clameter@sgi.com>
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 */
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages
2006-01-20 0:05 [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages Christoph Lameter
@ 2006-01-20 0:43 ` Andrew Morton
2006-01-20 0:49 ` Christoph Lameter
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2006-01-20 0:43 UTC (permalink / raw)
To: Christoph Lameter; +Cc: linux-kernel
Christoph Lameter <clameter@engr.sgi.com> 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 <clameter@sgi.com>
>
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages
2006-01-20 0:43 ` Andrew Morton
@ 2006-01-20 0:49 ` Christoph Lameter
2006-01-20 1:20 ` Andrew Morton
0 siblings, 1 reply; 8+ messages in thread
From: Christoph Lameter @ 2006-01-20 0:49 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages
2006-01-20 0:49 ` Christoph Lameter
@ 2006-01-20 1:20 ` Andrew Morton
2006-01-20 1:30 ` Christoph Lameter
2006-01-20 1:46 ` Christoph Lameter
0 siblings, 2 replies; 8+ messages in thread
From: Andrew Morton @ 2006-01-20 1:20 UTC (permalink / raw)
To: Christoph Lameter; +Cc: linux-kernel
Christoph Lameter <clameter@engr.sgi.com> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages
2006-01-20 1:20 ` Andrew Morton
@ 2006-01-20 1:30 ` Christoph Lameter
2006-01-20 1:46 ` Christoph Lameter
1 sibling, 0 replies; 8+ messages in thread
From: Christoph Lameter @ 2006-01-20 1:30 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
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 <clameter@sgi.com>
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 */
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages
2006-01-20 1:20 ` Andrew Morton
2006-01-20 1:30 ` Christoph Lameter
@ 2006-01-20 1:46 ` Christoph Lameter
2006-01-20 2:27 ` Andrew Morton
1 sibling, 1 reply; 8+ messages in thread
From: Christoph Lameter @ 2006-01-20 1:46 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
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 <clameter@sgi.com>
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);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages
2006-01-20 1:46 ` Christoph Lameter
@ 2006-01-20 2:27 ` Andrew Morton
2006-01-20 2:38 ` Christoph Lameter
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2006-01-20 2:27 UTC (permalink / raw)
To: Christoph Lameter; +Cc: linux-kernel
Christoph Lameter <clameter@engr.sgi.com> 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 <clameter@sgi.com>
>
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages
2006-01-20 2:27 ` Andrew Morton
@ 2006-01-20 2:38 ` Christoph Lameter
0 siblings, 0 replies; 8+ messages in thread
From: Christoph Lameter @ 2006-01-20 2:38 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2006-01-20 2:38 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-01-20 0:05 [PATCH] shrink_list: Use of && instead || leads to unintended writing of pages Christoph Lameter
2006-01-20 0:43 ` Andrew Morton
2006-01-20 0:49 ` Christoph Lameter
2006-01-20 1:20 ` Andrew Morton
2006-01-20 1:30 ` Christoph Lameter
2006-01-20 1:46 ` Christoph Lameter
2006-01-20 2:27 ` Andrew Morton
2006-01-20 2:38 ` Christoph Lameter
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).