[patch 2/5] setup_per_zone_lowmem_reserve() oops fix

[patch 2/5] setup_per_zone_lowmem_reserve() oops fix

[akpm]

If you do 'echo 0 0 > /proc/sys/vm/lowmem_reserve_ratio' the kernel gets a
divide-by-zero.

Prevent that, and fiddle with some whitespace too.

Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 25-akpm/mm/page_alloc.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff -puN mm/page_alloc.c~setup_per_zone_lowmem_reserve-oops-fix mm/page_alloc.c
--- 25/mm/page_alloc.c~setup_per_zone_lowmem_reserve-oops-fix	2005-03-04 13:16:10.000000000 -0800
+++ 25-akpm/mm/page_alloc.c	2005-03-04 13:16:10.000000000 -0800
@@ -37,13 +37,17 @@
 #include <asm/tlbflush.h>
 #include "internal.h"
 
-/* MCD - HACK: Find somewhere to initialize this EARLY, or make this initializer cleaner */
+/*
+ * MCD - HACK: Find somewhere to initialize this EARLY, or make this
+ * initializer cleaner
+ */
 nodemask_t node_online_map = { { [0] = 1UL } };
 nodemask_t node_possible_map = NODE_MASK_ALL;
 struct pglist_data *pgdat_list;
 unsigned long totalram_pages;
 unsigned long totalhigh_pages;
 long nr_swap_pages;
+
 /*
  * results with 256, 32 in the lowmem_reserve sysctl:
  *	1G machine -> (16M dma, 800M-16M normal, 1G-800M high)
@@ -1924,15 +1928,20 @@ static void setup_per_zone_lowmem_reserv
 
 	for_each_pgdat(pgdat) {
 		for (j = 0; j < MAX_NR_ZONES; j++) {
-			struct zone * zone = pgdat->node_zones + j;
+			struct zone *zone = pgdat->node_zones + j;
 			unsigned long present_pages = zone->present_pages;
 
 			zone->lowmem_reserve[j] = 0;
 
 			for (idx = j-1; idx >= 0; idx--) {
-				struct zone * lower_zone = pgdat->node_zones + idx;
+				struct zone *lower_zone;
+
+				if (sysctl_lowmem_reserve_ratio[idx] < 1)
+					sysctl_lowmem_reserve_ratio[idx] = 1;
 
-				lower_zone->lowmem_reserve[j] = present_pages / sysctl_lowmem_reserve_ratio[idx];
+				lower_zone = pgdat->node_zones + idx;
+				lower_zone->lowmem_reserve[j] = present_pages /
+					sysctl_lowmem_reserve_ratio[idx];
 				present_pages += lower_zone->present_pages;
 			}
 		}
@@ -2039,7 +2048,7 @@ module_init(init_per_zone_pages_min)
  *	changes.
  */
 int min_free_kbytes_sysctl_handler(ctl_table *table, int write, 
-		struct file *file, void __user *buffer, size_t *length, loff_t *ppos)
+	struct file *file, void __user *buffer, size_t *length, loff_t *ppos)
 {
 	proc_dointvec(table, write, file, buffer, length, ppos);
 	setup_per_zone_pages_min();
@@ -2056,7 +2065,7 @@ int min_free_kbytes_sysctl_handler(ctl_t
  * if in function of the boot time zone sizes.
  */
 int lowmem_reserve_ratio_sysctl_handler(ctl_table *table, int write,
-		 struct file *file, void __user *buffer, size_t *length, loff_t *ppos)
+	struct file *file, void __user *buffer, size_t *length, loff_t *ppos)
 {
 	proc_dointvec_minmax(table, write, file, buffer, length, ppos);
 	setup_per_zone_lowmem_reserve();
_

Re: [patch 2/5] setup_per_zone_lowmem_reserve() oops fix

[Greg KH]

On Fri, Mar 04, 2005 at 01:16:55PM -0800, akpm@osdl.org wrote:
> 
> 
> If you do 'echo 0 0 > /proc/sys/vm/lowmem_reserve_ratio' the kernel gets a
> divide-by-zero.
> 
> Prevent that, and fiddle with some whitespace too.

Due to the whitespace fiddling, I'd say no to this patch, based on the
"criteria".

thanks,

greg k-h

Re: [patch 2/5] setup_per_zone_lowmem_reserve() oops fix

[Nick Piggin]

akpm@osdl.org wrote:
> If you do 'echo 0 0 > /proc/sys/vm/lowmem_reserve_ratio' the kernel gets a
> divide-by-zero.
> 
> Prevent that, and fiddle with some whitespace too.
> 
> Signed-off-by: Andrew Morton <akpm@osdl.org>

Can we instead have a patch that makes the value zero turn off the
lowmem reserve entirely if it is set to zero?

Just now I was just testing, and found no easy way to do this other
than to make the value large enough that the reserve is insignificant.

So the loop would be something like:

  			for (idx = j-1; idx >= 0; idx--) {
				struct zone *lower_zone;
				lower_zone = pgdat->node_zones + idx;

				lower_zone->lowmem_reserve[j] = 0;
				if (sysctl_lowmem_reserve_ratio[idx] > 0)
					lower_zone->lowmem_reserve[j] = present_pages /
						sysctl_lowmem_reserve_ratio[idx];

				present_pages += lower_zone->present_pages;
			}

Re: [patch 2/5] setup_per_zone_lowmem_reserve() oops fix

[Andrew Morton]

Nick Piggin <nickpiggin@yahoo.com.au> wrote:
>
> akpm@osdl.org wrote:
> > If you do 'echo 0 0 > /proc/sys/vm/lowmem_reserve_ratio' the kernel gets a
> > divide-by-zero.
> > 
> > Prevent that, and fiddle with some whitespace too.
> > 
> > Signed-off-by: Andrew Morton <akpm@osdl.org>
> 
> Can we instead have a patch that makes the value zero turn off the
> lowmem reserve entirely if it is set to zero?

That would make sense, I guess.

> Just now I was just testing, and found no easy way to do this other
> than to make the value large enough that the reserve is insignificant.

Me too.  I use 1000 to get my 50MB of pagecache back.

I haven't thought about it yet, but there must be some way to avoid leaving
huge amounts of lowmem free.  It should be OK to allow lowmem to be fully
used, as long as there's sufficent reclaimable stuff in there - slab,
blockdev pagecache, etc.  (Assuming nothing sane mmaps blockdevs.  INND
does).  Dunno....

Re: [patch 2/5] setup_per_zone_lowmem_reserve() oops fix

[Andrea Arcangeli]

On Mon, Mar 07, 2005 at 12:20:48AM -0800, Andrew Morton wrote:
> I haven't thought about it yet, but there must be some way to avoid leaving
> huge amounts of lowmem free.  It should be OK to allow lowmem to be fully
> used, as long as there's sufficent reclaimable stuff in there - slab,
> blockdev pagecache, etc.  (Assuming nothing sane mmaps blockdevs.  INND
> does).  Dunno....

Then mlock will have to unmap and migrate the cache, it's just much more
complicated, but it's certainly doable.

Re: [patch 2/5] setup_per_zone_lowmem_reserve() oops fix

[Andrea Arcangeli]

On Mon, Mar 07, 2005 at 07:10:05PM +1100, Nick Piggin wrote:
> akpm@osdl.org wrote:
> >If you do 'echo 0 0 > /proc/sys/vm/lowmem_reserve_ratio' the kernel gets a
> >divide-by-zero.
> >
> >Prevent that, and fiddle with some whitespace too.
> >
> >Signed-off-by: Andrew Morton <akpm@osdl.org>
> 
> Can we instead have a patch that makes the value zero turn off the
> lowmem reserve entirely if it is set to zero?
> 
> Just now I was just testing, and found no easy way to do this other
> than to make the value large enough that the reserve is insignificant.
> 
> So the loop would be something like:
> 
>  			for (idx = j-1; idx >= 0; idx--) {
> 				struct zone *lower_zone;
> 				lower_zone = pgdat->node_zones + idx;
> 
> 				lower_zone->lowmem_reserve[j] = 0;
> 				if (sysctl_lowmem_reserve_ratio[idx] > 0)
> 					lower_zone->lowmem_reserve[j] = 
> 					present_pages /
> 						sysctl_lowmem_reserve_ratio[idx];
> 
> 				present_pages += lower_zone->present_pages;
> 			}

Looks good to me. I noticed the divide by zero myself once, and I
also considered changing it so that zero disables it. Could you send a
full patch to Andrew? Thanks.