[PATCH] Fix crash with FLAT_MEMORY and ARCH_PFN_OFFSET != 0

[PATCH] Fix crash with FLAT_MEMORY and ARCH_PFN_OFFSET != 0

[Thomas Bogendoerfer]

When using FLAT_MEMORY and ARCH_PFN_OFFSET is not 0, the kernel
crashes in memmap_init_zone(). This bug got introduced by
commit c713216deebd95d2b0ab38fef8bb2361c0180c2d

Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---

 mm/page_alloc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b5a58d4..496f7f3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3427,7 +3427,7 @@ static void __init_refok alloc_node_mem_map(struct pglist_data *pgdat)
 		mem_map = NODE_DATA(0)->node_mem_map;
 #ifdef CONFIG_ARCH_POPULATES_NODE_MAP
 		if (page_to_pfn(mem_map) != pgdat->node_start_pfn)
-			mem_map -= pgdat->node_start_pfn;
+			mem_map -= (pgdat->node_start_pfn - ARCH_PFN_OFFSET);
 #endif /* CONFIG_ARCH_POPULATES_NODE_MAP */
 	}
 #endif

Re: [PATCH] Fix crash with FLAT_MEMORY and ARCH_PFN_OFFSET != 0

[Andrew Morton]

On Tue, 18 Dec 2007 13:03:00 +0100 (CET) Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote:

> When using FLAT_MEMORY and ARCH_PFN_OFFSET is not 0, the kernel
> crashes in memmap_init_zone(). This bug got introduced by
> commit c713216deebd95d2b0ab38fef8bb2361c0180c2d
> 
> Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> ---
> 
>  mm/page_alloc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index b5a58d4..496f7f3 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3427,7 +3427,7 @@ static void __init_refok alloc_node_mem_map(struct pglist_data *pgdat)
>  		mem_map = NODE_DATA(0)->node_mem_map;
>  #ifdef CONFIG_ARCH_POPULATES_NODE_MAP
>  		if (page_to_pfn(mem_map) != pgdat->node_start_pfn)
> -			mem_map -= pgdat->node_start_pfn;
> +			mem_map -= (pgdat->node_start_pfn - ARCH_PFN_OFFSET);
>  #endif /* CONFIG_ARCH_POPULATES_NODE_MAP */
>  	}
>  #endif

hm.  It's rather non-trivial to verify that this will compile OK on all
architectures.

Re: [PATCH] Fix crash with FLAT_MEMORY and ARCH_PFN_OFFSET != 0

[Thomas Bogendoerfer]

On Tue, Dec 18, 2007 at 04:24:04AM -0800, Andrew Morton wrote:
> On Tue, 18 Dec 2007 13:03:00 +0100 (CET) Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote:
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3427,7 +3427,7 @@ static void __init_refok alloc_node_mem_map(struct pglist_data *pgdat)
> >  		mem_map = NODE_DATA(0)->node_mem_map;
> >  #ifdef CONFIG_ARCH_POPULATES_NODE_MAP
> >  		if (page_to_pfn(mem_map) != pgdat->node_start_pfn)
> > -			mem_map -= pgdat->node_start_pfn;
> > +			mem_map -= (pgdat->node_start_pfn - ARCH_PFN_OFFSET);
> >  #endif /* CONFIG_ARCH_POPULATES_NODE_MAP */
> >  	}
> >  #endif
> 
> hm.  It's rather non-trivial to verify that this will compile OK on all
> architectures.

I grepped through include/asm-* and found:

#if defined(CONFIG_FLATMEM)

#ifndef ARCH_PFN_OFFSET
#define ARCH_PFN_OFFSET         (0UL)
#endif

in include/asm-generic/memory_model.h. So I'm still confident, that
this works on all architectures. It's tested on MIPS.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] Fix crash with FLAT_MEMORY and ARCH_PFN_OFFSET != 0

[Mel Gorman]

On (18/12/07 13:03), Thomas Bogendoerfer didst pronounce:
> When using FLAT_MEMORY and ARCH_PFN_OFFSET is not 0, the kernel
> crashes in memmap_init_zone(). This bug got introduced by
> commit c713216deebd95d2b0ab38fef8bb2361c0180c2d
> 

That commit is over a year old and it initially distressed me that it
would take this long to show up on a boot test. However, you said this was
to fix MIPS in a follow-on mail and I never made it use arch-independent
zone-sizing which is where CONFIG_ARCH_POPULATES_NODE_MAP comes from. Support
for arch-independent zone-sizing was added for MIPS on Nov 3rd 2007 by commit
cce335ae47e231398269fb05fa48e0e9cbf289e0 (relevant cc added) which is a much
more reasonable timeframe for catching this sort of problem. I believe the
real bug might be in there.

Can you post a full dmesg log with loglevel=8 so I can see what the layout
looks like? My aim is to find out why the start of your mem_map is not at
the start of node 0's mem_map which is what I generally expected with the
FLATMEM model.

Thanks.

> Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> ---
> 
>  mm/page_alloc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index b5a58d4..496f7f3 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3427,7 +3427,7 @@ static void __init_refok alloc_node_mem_map(struct pglist_data *pgdat)
>  		mem_map = NODE_DATA(0)->node_mem_map;
>  #ifdef CONFIG_ARCH_POPULATES_NODE_MAP
>  		if (page_to_pfn(mem_map) != pgdat->node_start_pfn)
> -			mem_map -= pgdat->node_start_pfn;
> +			mem_map -= (pgdat->node_start_pfn - ARCH_PFN_OFFSET);
>  #endif /* CONFIG_ARCH_POPULATES_NODE_MAP */
>  	}
>  #endif
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: [PATCH] Fix crash with FLAT_MEMORY and ARCH_PFN_OFFSET != 0

[Thomas Bogendoerfer]

On Tue, Dec 18, 2007 at 01:58:28PM +0000, Mel Gorman wrote:
> That commit is over a year old and it initially distressed me that it
> would take this long to show up on a boot test. However, you said this was
> to fix MIPS in a follow-on mail and I never made it use arch-independent
> zone-sizing which is where CONFIG_ARCH_POPULATES_NODE_MAP comes from. Support
> for arch-independent zone-sizing was added for MIPS on Nov 3rd 2007 by commit
> cce335ae47e231398269fb05fa48e0e9cbf289e0 (relevant cc added) which is a much
> more reasonable timeframe for catching this sort of problem. I believe the
> real bug might be in there.

either that or it got uncovered. But I'm not too deep into that part of
the kernel at the moment.

> 
> Can you post a full dmesg log with loglevel=8 so I can see what the layout
> looks like? My aim is to find out why the start of your mem_map is not at
> the start of node 0's mem_map which is what I generally expected with the
> FLATMEM model.


Linux version 2.6.24-rc5-g4b1e2593-dirty (tsbogend@solo.franken.de) (gcc version 4.2.1) #170 Tue Dec 18 17:03:53 CET 2007
ARCH: SGI-IP28
PROMLIB: ARC firmware Version 64 Revision 0
console [early0] enabled
CPU revision is: 00000925 (R10000)
FPU revision is: 00000900
MC: SGI memory controller Revision 5
MC: Probing memory configuration:
 bank0: 256M @ 20000000
 bank1: 128M @ 30000000
 bank2: 128M @ 38000000
Determined physical RAM map:
 memory: 0000000020000000 @ 0000000020000000 (usable)
Entering add_active_range(0, 131946, 262144) 0 entries of 256 used
Zone PFN ranges:
  Normal     131946 ->   262144
Movable zone start PFN for each node
early_node_map[1] active PFN ranges
    0:   131946 ->   262144
On node 0 totalpages: 130198
  Normal zone: 1780 pages used for memmap
  Normal zone: 0 pages reserved
  Normal zone: 128418 pages, LIFO batch:31
  Movable zone: 0 pages used for memmap
Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 128418
Kernel command line: root=scsi(0)disk(1)rdisk(0)partition(0) loglevel=8 ip=dhcp
Primary instruction cache 32kB, VIPT, 2-way, linesize 64 bytes.
Primary data cache 32kB, 2-way, VIPT, no aliases, linesize 32 bytes
Unified secondary cache 1024kB 2-way, linesize 128 bytes.
Synthesized clear page handler (44 instructions).
Synthesized copy page handler (82 instructions).
Synthesized TLB refill handler (37 instructions).
Synthesized TLB load handler fastpath (49 instructions).
Synthesized TLB store handler fastpath (49 instructions).
Synthesized TLB modify handler fastpath (48 instructions).
EISA: Probing bus...
EISA: Detected 0 card.
ISA support compiled in.
PID hash table entries: 2048 (order: 11, 16384 bytes)
Calibrating system timer... warning: timer counts differ, retrying... 87500 [175.0000 MHz CPU]
Console: colour dummy device 80x25
Dentry cache hash table entries: 65536 (order: 7, 524288 bytes)
Inode-cache hash table entries: 32768 (order: 6, 262144 bytes)
Memory: 512132k/524288k available (2659k kernel code, 8412k reserved, 817k data, 280k init, 0k highmem)


That's with my fix/workaround applied.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] Fix crash with FLAT_MEMORY and ARCH_PFN_OFFSET != 0

[Mel Gorman]

On (18/12/07 17:09), Thomas Bogendoerfer didst pronounce:
> On Tue, Dec 18, 2007 at 01:58:28PM +0000, Mel Gorman wrote:
> > That commit is over a year old and it initially distressed me that it
> > would take this long to show up on a boot test. However, you said this was
> > to fix MIPS in a follow-on mail and I never made it use arch-independent
> > zone-sizing which is where CONFIG_ARCH_POPULATES_NODE_MAP comes from. Support
> > for arch-independent zone-sizing was added for MIPS on Nov 3rd 2007 by commit
> > cce335ae47e231398269fb05fa48e0e9cbf289e0 (relevant cc added) which is a much
> > more reasonable timeframe for catching this sort of problem. I believe the
> > real bug might be in there.
> 
> either that or it got uncovered.

You're right, the arches using POPULATES_NODE_MAP would have had
ARCH_PFN_OFFSET at 0. Your patch corrects that and should not affect
other arches so;

Acked-by: Mel Gorman <mel@csn.ul.ie>

Thanks for fixing.

While looking at this, it occured to me that maybe you could avoid a
subtraction at runtime for MIPS using page_to_pfn() on FLATMEM by getting rid
of ARCH_PFN_OFFSET. It's probably only of miniscule benefit but something
like this (totally untested) patch may work if you want to try it out. It
should have fixed your problem too but would it would have been achieved by
hiding the problem. I believe your patch is still correct.

====

Get rid of ARCH_PFN_OFFSET calculation in MIPS with FLATMEM memory
model.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
--- 

 arch/mips/kernel/setup.c |   13 +++++++------
 include/asm-mips/page.h  |    9 +--------
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 7f6ddcb..9199314 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -273,6 +273,7 @@ static void __init bootmem_init(void)
 	unsigned long mapstart = ~0UL;
 	unsigned long bootmap_size;
 	int i;
+	unsigned long phys_pfn_offset = PFN_UP(PHYS_OFFSET);
 
 	/*
 	 * Init any data related to initrd. It's a nop if INITRD is
@@ -314,17 +315,17 @@ static void __init bootmem_init(void)
 
 	if (min_low_pfn >= max_low_pfn)
 		panic("Incorrect memory mapping !!!");
-	if (min_low_pfn > ARCH_PFN_OFFSET) {
+	if (min_low_pfn > phys_pfn_offset) {
 		printk(KERN_INFO
 		       "Wasting %lu bytes for tracking %lu unused pages\n",
-		       (min_low_pfn - ARCH_PFN_OFFSET) * sizeof(struct page),
-		       min_low_pfn - ARCH_PFN_OFFSET);
-	} else if (min_low_pfn < ARCH_PFN_OFFSET) {
+		       (min_low_pfn - phys_pfn_offset) * sizeof(struct page),
+		       min_low_pfn - phys_pfn_offset);
+	} else if (min_low_pfn < phys_pfn_offset) {
 		printk(KERN_INFO
 		       "%lu free pages won't be used\n",
-		       ARCH_PFN_OFFSET - min_low_pfn);
+		       phys_pfn_offset - min_low_pfn);
 	}
-	min_low_pfn = ARCH_PFN_OFFSET;
+	min_low_pfn = phys_pfn_offset;
 
 	/*
 	 * Determine low and high memory ranges
diff --git a/include/asm-mips/page.h b/include/asm-mips/page.h
index d2ea983..3e57755 100644
--- a/include/asm-mips/page.h
+++ b/include/asm-mips/page.h
@@ -37,13 +37,6 @@
 #include <linux/pfn.h>
 #include <asm/io.h>
 
-/*
- * It's normally defined only for FLATMEM config but it's
- * used in our early mem init code for all memory models.
- * So always define it.
- */
-#define ARCH_PFN_OFFSET		PFN_UP(PHYS_OFFSET)
-
 extern void clear_page(void * page);
 extern void copy_page(void * to, void * from);
 
@@ -159,7 +152,7 @@ typedef struct { unsigned long pgprot; } pgprot_t;
 
 #ifdef CONFIG_FLATMEM
 
-#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
+#define pfn_valid(pfn)		((pfn) >= PFN_UP(PHYS_OFFSET) && (pfn) < max_mapnr)
 
 #elif defined(CONFIG_SPARSEMEM)
 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: [PATCH] Fix crash with FLAT_MEMORY and ARCH_PFN_OFFSET != 0

[Thomas Bogendoerfer]

On Thu, Dec 20, 2007 at 11:44:06AM +0000, Mel Gorman wrote:
> --- a/include/asm-mips/page.h
> +++ b/include/asm-mips/page.h
> @@ -37,13 +37,6 @@
>  #include <linux/pfn.h>
>  #include <asm/io.h>
>  
> -/*
> - * It's normally defined only for FLATMEM config but it's
> - * used in our early mem init code for all memory models.
> - * So always define it.
> - */
> -#define ARCH_PFN_OFFSET		PFN_UP(PHYS_OFFSET)
> -

hmm, doesn't this break what I've fixed ? Without this #define
ARCH_PFN_OFFSET gets defined to 0 and the bug is back. Or did
I miss anything ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] Fix crash with FLAT_MEMORY and ARCH_PFN_OFFSET != 0

[Mel Gorman]

On (20/12/07 13:43), Thomas Bogendoerfer didst pronounce:
> On Thu, Dec 20, 2007 at 11:44:06AM +0000, Mel Gorman wrote:
> > --- a/include/asm-mips/page.h
> > +++ b/include/asm-mips/page.h
> > @@ -37,13 +37,6 @@
> >  #include <linux/pfn.h>
> >  #include <asm/io.h>
> >  
> > -/*
> > - * It's normally defined only for FLATMEM config but it's
> > - * used in our early mem init code for all memory models.
> > - * So always define it.
> > - */
> > -#define ARCH_PFN_OFFSET		PFN_UP(PHYS_OFFSET)
> > -
> 
> hmm, doesn't this break what I've fixed ? Without this #define
> ARCH_PFN_OFFSET gets defined to 0 and the bug is back. Or did
> I miss anything ?
> 

ARCH_PFN_OFFSET goes to 0, so page_to_pfn() is no longer adjusting by
PFN_UP(PHYS_OFFSET) like it was when your problem occured. I am guessing
that the nature of the crash was that page_to_pfn() was returning bogus
values early in boot and trying to initialise memmap that didn't exist.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: [PATCH] Fix crash with FLAT_MEMORY and ARCH_PFN_OFFSET != 0

[Thomas Bogendoerfer]

On Thu, Dec 20, 2007 at 01:27:20PM +0000, Mel Gorman wrote:
> On (20/12/07 13:43), Thomas Bogendoerfer didst pronounce:
> > On Thu, Dec 20, 2007 at 11:44:06AM +0000, Mel Gorman wrote:
> > > --- a/include/asm-mips/page.h
> > > +++ b/include/asm-mips/page.h
> > > @@ -37,13 +37,6 @@
> > >  #include <linux/pfn.h>
> > >  #include <asm/io.h>
> > >  
> > > -/*
> > > - * It's normally defined only for FLATMEM config but it's
> > > - * used in our early mem init code for all memory models.
> > > - * So always define it.
> > > - */
> > > -#define ARCH_PFN_OFFSET		PFN_UP(PHYS_OFFSET)
> > > -
> > 
> > hmm, doesn't this break what I've fixed ? Without this #define
> > ARCH_PFN_OFFSET gets defined to 0 and the bug is back. Or did
> > I miss anything ?
> > 
> 
> ARCH_PFN_OFFSET goes to 0, so page_to_pfn() is no longer adjusting by
> PFN_UP(PHYS_OFFSET) like it was when your problem occured. I am guessing
> that the nature of the crash was that page_to_pfn() was returning bogus
> values early in boot and trying to initialise memmap that didn't exist.

ic, your patch works for me.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] Fix crash with FLAT_MEMORY and ARCH_PFN_OFFSET != 0

[Mel Gorman]

On (30/12/07 12:37), Thomas Bogendoerfer didst pronounce:
> On Thu, Dec 20, 2007 at 01:27:20PM +0000, Mel Gorman wrote:
> > On (20/12/07 13:43), Thomas Bogendoerfer didst pronounce:
> > > On Thu, Dec 20, 2007 at 11:44:06AM +0000, Mel Gorman wrote:
> > > > --- a/include/asm-mips/page.h
> > > > +++ b/include/asm-mips/page.h
> > > > @@ -37,13 +37,6 @@
> > > >  #include <linux/pfn.h>
> > > >  #include <asm/io.h>
> > > >  
> > > > -/*
> > > > - * It's normally defined only for FLATMEM config but it's
> > > > - * used in our early mem init code for all memory models.
> > > > - * So always define it.
> > > > - */
> > > > -#define ARCH_PFN_OFFSET		PFN_UP(PHYS_OFFSET)
> > > > -
> > > 
> > > hmm, doesn't this break what I've fixed ? Without this #define
> > > ARCH_PFN_OFFSET gets defined to 0 and the bug is back. Or did
> > > I miss anything ?
> > > 
> > 
> > ARCH_PFN_OFFSET goes to 0, so page_to_pfn() is no longer adjusting by
> > PFN_UP(PHYS_OFFSET) like it was when your problem occured. I am guessing
> > that the nature of the crash was that page_to_pfn() was returning bogus
> > values early in boot and trying to initialise memmap that didn't exist.
> 
> ic, your patch works for me.
> 

Grand. For the moment, your patch is the correct fix. This is a
micro-optimisation that can be picked up again at a later date. Thanks
for the testing and the fix.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab