HOLES_IN_ZONE...

HOLES_IN_ZONE...

[David Miller]

So I've been fighting mysterious crashes on my main sparc64 devel
machine.  What's happening is that the assertion in
mm/page_alloc.c:move_freepages() is triggering:

	BUG_ON(page_zone(start_page) != page_zone(end_page));

Once I knew this is what was happening, I added some annotations:

	if (unlikely(page_zone(start_page) != page_zone(end_page))) {
		printk(KERN_ERR "move_freepages: Bogus zones: "
		       "start_page[%p] end_page[%p] zone[%p]\n",
		       start_page, end_page, zone);
		printk(KERN_ERR "move_freepages: "
		       "start_zone[%p] end_zone[%p]\n",
		       page_zone(start_page), page_zone(end_page));
		printk(KERN_ERR "move_freepages: "
		       "start_pfn[0x%lx] end_pfn[0x%lx]\n",
		       page_to_pfn(start_page), page_to_pfn(end_page));
		printk(KERN_ERR "move_freepages: "
		       "start_nid[%d] end_nid[%d]\n",
		       page_to_nid(start_page), page_to_nid(end_page));
 ...

And here's what I got:

	move_freepages: Bogus zones: start_page[2207d0000] end_page[2207dffc0] zone[fffff8103effcb00]
	move_freepages: start_zone[fffff8103effcb00] end_zone[fffff8003fffeb00]
	move_freepages: start_pfn[0x81f600] end_pfn[0x81f7ff]
	move_freepages: start_nid[1] end_nid[0]

My memory layout on this box is:

[    0.000000] Zone PFN ranges:
[    0.000000]   Normal   0x00000000 -> 0x0081ff5d
[    0.000000] Movable zone start PFN for each node
[    0.000000] early_node_map[8] active PFN ranges
[    0.000000]     0: 0x00000000 -> 0x00020000
[    0.000000]     1: 0x00800000 -> 0x0081f7ff
[    0.000000]     1: 0x0081f800 -> 0x0081fe50
[    0.000000]     1: 0x0081fed1 -> 0x0081fed8
[    0.000000]     1: 0x0081feda -> 0x0081fedb
[    0.000000]     1: 0x0081fedd -> 0x0081fee5
[    0.000000]     1: 0x0081fee7 -> 0x0081ff51
[    0.000000]     1: 0x0081ff59 -> 0x0081ff5d

So it's a block move in that 0x81f600-->0x81f7ff region which triggers
the problem.

So I did a lot (and I do mean _A LOT_) of digging.  And it seems that
unless you set HOLES_IN_ZONE you have to make sure that all of the
memmap regions of free space in a zone begin and end on an HPAGE_SIZE
boundary (the requirement used to be that it had to be MAX_ORDER
sized).

Well, this assumption enterred the tree back in 2005 (!!!) from
the following commit in the history-2.6 tree:

commit 69fba2dd0335abec0b0de9ac53d5bbb67c31fc60
Author: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Date:   Fri Jan 7 22:01:35 2005 -0800

    [PATCH] no buddy bitmap patch revisit: for mm/page_alloc.c

At the time only IA64 had HOLES_IN_ZONE added, this happens in the
commit right after the above one.

So in theory Sparc64 has been broken since that commit and subject to
potential memory corruption and other unnice things.

I also noticed that when S390 got virtual memmap support, it acquired
the HOLES_IN_ZONE setting as well, in this commit:

commit f4eb07c17df2e6cf9bd58bfcd9cc9e05e9489d07
Author: Heiko Carstens <heiko.carstens@de.ibm.com>
Date:   Fri Dec 8 15:56:07 2006 +0100

    [S390] Virtual memmap for s390.

This is confusing.  Is HOLES_IN_ZONE only required when virtual mmap
is being used?  If so, why is that?  This is a very poorly documented
flag, and I'm saying this after pouring over every commit referencing
it.

Later this HOLES_IN_ZONE requirement was removed on s390 by commit:

commit 9f4b0ba81f158df459fa2cfc98ab1475c090f29c
Author: Heiko Carstens <heiko.carstens@de.ibm.com>
Date:   Sat Jan 26 14:11:02 2008 +0100

    [S390] Get rid of HOLES_IN_ZONE requirement.

Anyways...

The point of this email is, do I really need to set this thing on
sparc64?  I've never seen this check triggered before, it only seems
to have started triggering in 2.6.27 or so but I obviously can't find
anything that would influence something of this nature.

It takes a lot of stressing to get that specific chunk of pages to
attempt to be freed up in a group like that :-/

As a suggestion, it would have been a lot more pleasant if the code
validated this requirement (in the !HOLES_IN_ZONE case) at boot time
instead of after 2 hours of stress testing :-(

Re: HOLES_IN_ZONE...

[KAMEZAWA Hiroyuki]

On Wed, 04 Feb 2009 22:26:51 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> 
> So I've been fighting mysterious crashes on my main sparc64 devel
> machine.  What's happening is that the assertion in
> mm/page_alloc.c:move_freepages() is triggering:
> 
> 	BUG_ON(page_zone(start_page) != page_zone(end_page));
> 
> Once I knew this is what was happening, I added some annotations:
> 
> 	if (unlikely(page_zone(start_page) != page_zone(end_page))) {
> 		printk(KERN_ERR "move_freepages: Bogus zones: "
> 		       "start_page[%p] end_page[%p] zone[%p]\n",
> 		       start_page, end_page, zone);
> 		printk(KERN_ERR "move_freepages: "
> 		       "start_zone[%p] end_zone[%p]\n",
> 		       page_zone(start_page), page_zone(end_page));
> 		printk(KERN_ERR "move_freepages: "
> 		       "start_pfn[0x%lx] end_pfn[0x%lx]\n",
> 		       page_to_pfn(start_page), page_to_pfn(end_page));
> 		printk(KERN_ERR "move_freepages: "
> 		       "start_nid[%d] end_nid[%d]\n",
> 		       page_to_nid(start_page), page_to_nid(end_page));
>  ...
> 
> And here's what I got:
> 
> 	move_freepages: Bogus zones: start_page[2207d0000] end_page[2207dffc0] zone[fffff8103effcb00]
> 	move_freepages: start_zone[fffff8103effcb00] end_zone[fffff8003fffeb00]
> 	move_freepages: start_pfn[0x81f600] end_pfn[0x81f7ff]
> 	move_freepages: start_nid[1] end_nid[0]
> 
> My memory layout on this box is:
> 
> [    0.000000] Zone PFN ranges:
> [    0.000000]   Normal   0x00000000 -> 0x0081ff5d
> [    0.000000] Movable zone start PFN for each node
> [    0.000000] early_node_map[8] active PFN ranges
> [    0.000000]     0: 0x00000000 -> 0x00020000
> [    0.000000]     1: 0x00800000 -> 0x0081f7ff
> [    0.000000]     1: 0x0081f800 -> 0x0081fe50
> [    0.000000]     1: 0x0081fed1 -> 0x0081fed8
> [    0.000000]     1: 0x0081feda -> 0x0081fedb
> [    0.000000]     1: 0x0081fedd -> 0x0081fee5
> [    0.000000]     1: 0x0081fee7 -> 0x0081ff51
> [    0.000000]     1: 0x0081ff59 -> 0x0081ff5d
> 

Ah, end_pfn is not valid page. And, page->flags shows nid 0.
It seems memmap for end_pfn is not initialized correctly.

At first, there are some complicated around here..

1. pfn_valid() is just for "there is memmap." not for "the memory is valid"
2. If "memory is invalid" && it has memmap, it should be marked as PG_Reserved.
   And it will never be put into buddy allocator. 
3. memmap for not exisiting memory can be initialized but it's depends on
   zone->spanned_pages. (see free_area_init_core())
4. What CONFIG_HOLES_IN_ZONE means is 
   "there can be invalid memmap within coutinuous range of zone->mem_map"
   This comes from VIRTUAL_MEMMAP.
   In usual arch, mem_map is guaranteed to be coutinuous always.




> 	move_freepages: start_zone[fffff8103effcb00] end_zone[fffff8003fffeb00]
> 	move_freepages: start_pfn[0x81f600] end_pfn[0x81f7ff]
> 	move_freepages: start_nid[1] end_nid[0]
> [    0.000000]     0: 0x00000000 -> 0x00020000
> [    0.000000]     1: 0x00800000 -> 0x0081f7ff

> [    0.000000]     1: 0x00800000 -> 0x0081f7ff

I think it's strange that end_pfn's nid is 0.

>From this log, mem_map for end_pfn exists (means pfn_valid(end_pfn) == true)
So, it should be initialized correctly and should have nid 1 if initialized.

Maybe Node1's zone->start_pfn and zone->spanned_pages covers 0x81f7ff, and it's
range is 0x00800000 -> 0x0081ff5d

But,  this check in memmap_init_zone()
==
2619                 if (context == MEMMAP_EARLY) {
2620                         if (!early_pfn_valid(pfn))
2621                                 continue;
2622                         if (!early_pfn_in_nid(pfn, nid))
2623                                 continue;
2624                 }
==
will allow skip to init this mem_map of 0x8af7ff.
*AND*, SetPageResreved() is never called. This is a problem I think.

> It takes a lot of stressing to get that specific chunk of pages to
> attempt to be freed up in a group like that :-/
> 
> As a suggestion, it would have been a lot more pleasant if the code
> validated this requirement (in the !HOLES_IN_ZONE case) at boot time
> instead of after 2 hours of stress testing :-(
> 

Can this patch help you ? (maybe more careful study is necessary...)
---
 mm/page_alloc.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: mmotm-2.6.29-Feb03/mm/page_alloc.c
===================================================================
--- mmotm-2.6.29-Feb03.orig/mm/page_alloc.c
+++ mmotm-2.6.29-Feb03/mm/page_alloc.c
@@ -2618,6 +2618,7 @@ void __meminit memmap_init_zone(unsigned
 	unsigned long end_pfn = start_pfn + size;
 	unsigned long pfn;
 	struct zone *z;
+	int tmp;
 
 	if (highest_memmap_pfn < end_pfn - 1)
 		highest_memmap_pfn = end_pfn - 1;
@@ -2632,7 +2633,8 @@ void __meminit memmap_init_zone(unsigned
 		if (context == MEMMAP_EARLY) {
 			if (!early_pfn_valid(pfn))
 				continue;
-			if (!early_pfn_in_nid(pfn, nid))
+			tmp = early_pfn_in_nid(pfn, nid);
+			if (tmp > -1 && tmp != nid)
 				continue;
 		}
 		page = pfn_to_page(pfn);
@@ -2999,8 +3001,9 @@ int __meminit early_pfn_to_nid(unsigned 
 			return early_node_map[i].nid;
 	}
 
-	return 0;
+	return -1;
 }
+
 #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
 
 /* Basic iterator support to walk early_node_map[] */

Re: HOLES_IN_ZONE...

[Heiko Carstens]

On Wed, 04 Feb 2009 22:26:51 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> I also noticed that when S390 got virtual memmap support, it acquired
> the HOLES_IN_ZONE setting as well, in this commit:
> 
> commit f4eb07c17df2e6cf9bd58bfcd9cc9e05e9489d07
> Author: Heiko Carstens <heiko.carstens@de.ibm.com>
> Date:   Fri Dec 8 15:56:07 2006 +0100
> 
>     [S390] Virtual memmap for s390.
> 
> This is confusing.  Is HOLES_IN_ZONE only required when virtual mmap
> is being used?  If so, why is that?  This is a very poorly documented
> flag, and I'm saying this after pouring over every commit referencing
> it.

I should have split that into two commits back then. When writing the
vmemmap code for s390 I realized that we never guaranteed that zones
start on a MAX_ORDER boundary or have a size that is a multiple of
MAX_ORDER. So I just added HOLES_IN_ZONE.

> Later this HOLES_IN_ZONE requirement was removed on s390 by commit:
> 
> commit 9f4b0ba81f158df459fa2cfc98ab1475c090f29c
> Author: Heiko Carstens <heiko.carstens@de.ibm.com>
> Date:   Sat Jan 26 14:11:02 2008 +0100
> 
>     [S390] Get rid of HOLES_IN_ZONE requirement.

This just made sure that all zones start on a MAX_ORDER boundary and
just leaves memory that doesn't fit unused. So the requirement for
HOLES_IN_ZONE went away.

Later I reduced MAX_ORDER to 9 on s390, so we don't leave large
portions of memory unused.

Re: HOLES_IN_ZONE...

[David Miller]

From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Thu, 5 Feb 2009 09:00:16 +0100

> On Wed, 04 Feb 2009 22:26:51 -0800 (PST)
> David Miller <davem@davemloft.net> wrote:
> 
> > Later this HOLES_IN_ZONE requirement was removed on s390 by commit:
> > 
> > commit 9f4b0ba81f158df459fa2cfc98ab1475c090f29c
> > Author: Heiko Carstens <heiko.carstens@de.ibm.com>
> > Date:   Sat Jan 26 14:11:02 2008 +0100
> > 
> >     [S390] Get rid of HOLES_IN_ZONE requirement.
> 
> This just made sure that all zones start on a MAX_ORDER boundary and
> just leaves memory that doesn't fit unused. So the requirement for
> HOLES_IN_ZONE went away.
> 
> Later I reduced MAX_ORDER to 9 on s390, so we don't leave large
> portions of memory unused.

Isn't is easier to just make sure your vmemmap mappings extend to such
boundaries, whether they contain available memory or not?

That's the only requirement you have to satisfy to avoid having to
specify HOLES_IN_ZONE.  You don't have to have memory there, just
some vmmemmap page structs have to be mapped at those indices.

And then you won't need waste memory with these MAX_ORDER boundary
adjustments.

Really, I think IA64 should do this too (it's the only remaining
HOLES_IN_ZONE setting arch), then we can just delete this whole thing
completely.

Re: HOLES_IN_ZONE...

[Heiko Carstens]

On Thu, 05 Feb 2009 15:44:03 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> > Later I reduced MAX_ORDER to 9 on s390, so we don't leave large
> > portions of memory unused.
> 
> Isn't is easier to just make sure your vmemmap mappings extend to such
> boundaries, whether they contain available memory or not?
> 
> That's the only requirement you have to satisfy to avoid having to
> specify HOLES_IN_ZONE.  You don't have to have memory there, just
> some vmmemmap page structs have to be mapped at those indices.
> 
> And then you won't need waste memory with these MAX_ORDER boundary
> adjustments.

Sounds reasonable. Back then I was under the impression that it is not
sufficient to only have a struct page but also a page for each PFN.
I'll give it a try as soon as time permits.

Thanks!

Re: HOLES_IN_ZONE...

[KAMEZAWA Hiroyuki]

On Wed, 04 Feb 2009 22:26:51 -0800 (PST)
David Miller <davem@davemloft.net> wrote:
> The point of this email is, do I really need to set this thing on
> sparc64?  I've never seen this check triggered before, it only seems
> to have started triggering in 2.6.27 or so but I obviously can't find
> anything that would influence something of this nature.
> 

Clarifing my e-mail again..(sorry for bad text.)

1. This is not problem from HOLES_IN_ZONE.
2. Your end_pfn(0x81f7ff)'s memmap is not initialized correctly.
3. It's because 0x81f7ff has memmap but not in early_node_map[].

So, initialzing mem_map() for this end_pfn is necessary.

This is my patch. Mel, how do you think ?
==

A struct page for pfn which is not in early_node_map[] can exist and
it's not be initalized.

This patch changes early_pfn_to_nid() to return -1 and if -1 returns,
initialized mem_map with passed nid. After this patch, the page will be
marked as PG_reserved and never freed later.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 mm/page_alloc.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: mmotm-2.6.29-Feb03/mm/page_alloc.c
===================================================================
--- mmotm-2.6.29-Feb03.orig/mm/page_alloc.c
+++ mmotm-2.6.29-Feb03/mm/page_alloc.c
@@ -2618,6 +2618,7 @@ void __meminit memmap_init_zone(unsigned
 	unsigned long end_pfn = start_pfn + size;
 	unsigned long pfn;
 	struct zone *z;
+	int tmp;
 
 	if (highest_memmap_pfn < end_pfn - 1)
 		highest_memmap_pfn = end_pfn - 1;
@@ -2632,7 +2633,8 @@ void __meminit memmap_init_zone(unsigned
 		if (context == MEMMAP_EARLY) {
 			if (!early_pfn_valid(pfn))
 				continue;
-			if (!early_pfn_in_nid(pfn, nid))
+			tmp = early_pfn_in_nid(pfn, nid);
+			if (tmp > -1 && tmp != nid)
 				continue;
 		}
 		page = pfn_to_page(pfn);
@@ -2999,8 +3001,9 @@ int __meminit early_pfn_to_nid(unsigned 
 			return early_node_map[i].nid;
 	}
 
-	return 0;
+	return -1;
 }
+
 #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
 
 /* Basic iterator support to walk early_node_map[] */

Re: HOLES_IN_ZONE...

[David Miller]

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Date: Thu, 5 Feb 2009 18:06:17 +0900

> @@ -2632,7 +2633,8 @@ void __meminit memmap_init_zone(unsigned
>  		if (context == MEMMAP_EARLY) {
>  			if (!early_pfn_valid(pfn))
>  				continue;
> -			if (!early_pfn_in_nid(pfn, nid))
> +			tmp = early_pfn_in_nid(pfn, nid);
> +			if (tmp > -1 && tmp != nid)

early_pfn_in_nid() returns true or false, not the found nid

I think you meant to change this to call early_pfn_to_nid()

I'll make that correction and test your patch.

BTW, if you make that conversion there is no need for
early_pfn_in_nid() since there will be no other users.

Re: HOLES_IN_ZONE...

[KAMEZAWA Hiroyuki]

On Thu, 05 Feb 2009 01:21:23 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 5 Feb 2009 18:06:17 +0900
> 
> > @@ -2632,7 +2633,8 @@ void __meminit memmap_init_zone(unsigned
> >  		if (context == MEMMAP_EARLY) {
> >  			if (!early_pfn_valid(pfn))
> >  				continue;
> > -			if (!early_pfn_in_nid(pfn, nid))
> > +			tmp = early_pfn_in_nid(pfn, nid);
> > +			if (tmp > -1 && tmp != nid)
> 
> early_pfn_in_nid() returns true or false, not the found nid
> 
> I think you meant to change this to call early_pfn_to_nid()
> 
sorry..

> I'll make that correction and test your patch.
> 
> BTW, if you make that conversion there is no need for
> early_pfn_in_nid() since there will be no other users.
> 

Thanks, maybe the patch will be like this.
-Kame
==
If a pfn is not in early_node_map[], memmap for it is not initialized.
By this, PG_reserved is not set and the invalid memmap may sneak into buddy
allocator.

To avoid that, initialize it with give nid if no early_node_map[] for pfn
exists. PG_reserved will make this page unused.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 include/linux/mmzone.h |    6 ------
 mm/page_alloc.c        |    7 +++++--
 2 files changed, 5 insertions(+), 8 deletions(-)

Index: mmotm-2.6.29-Feb03/mm/page_alloc.c
===================================================================
--- mmotm-2.6.29-Feb03.orig/mm/page_alloc.c
+++ mmotm-2.6.29-Feb03/mm/page_alloc.c
@@ -2618,6 +2618,7 @@ void __meminit memmap_init_zone(unsigned
 	unsigned long end_pfn = start_pfn + size;
 	unsigned long pfn;
 	struct zone *z;
+	int tmp;
 
 	if (highest_memmap_pfn < end_pfn - 1)
 		highest_memmap_pfn = end_pfn - 1;
@@ -2632,7 +2633,8 @@ void __meminit memmap_init_zone(unsigned
 		if (context == MEMMAP_EARLY) {
 			if (!early_pfn_valid(pfn))
 				continue;
-			if (!early_pfn_in_nid(pfn, nid))
+			tmp = early_pfn_to_nid(pfn);
+			if (tmp > -1 && tmp != nid)
 				continue;
 		}
 		page = pfn_to_page(pfn);
@@ -2999,8 +3001,9 @@ int __meminit early_pfn_to_nid(unsigned 
 			return early_node_map[i].nid;
 	}
 
-	return 0;
+	return -1;
 }
+
 #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
 
 /* Basic iterator support to walk early_node_map[] */
Index: mmotm-2.6.29-Feb03/include/linux/mmzone.h
===================================================================
--- mmotm-2.6.29-Feb03.orig/include/linux/mmzone.h
+++ mmotm-2.6.29-Feb03/include/linux/mmzone.h
@@ -1070,12 +1070,6 @@ void sparse_init(void);
 #define sparse_index_init(_sec, _nid)  do {} while (0)
 #endif /* CONFIG_SPARSEMEM */
 
-#ifdef CONFIG_NODES_SPAN_OTHER_NODES
-#define early_pfn_in_nid(pfn, nid)	(early_pfn_to_nid(pfn) == (nid))
-#else
-#define early_pfn_in_nid(pfn, nid)	(1)
-#endif
-
 #ifndef early_pfn_valid
 #define early_pfn_valid(pfn)	(1)
 #endif

Re: HOLES_IN_ZONE...

[David Miller]

From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Date: Thu, 5 Feb 2009 18:34:09 +0900

> On Thu, 05 Feb 2009 01:21:23 -0800 (PST)
> David Miller <davem@davemloft.net> wrote:
> 
> > early_pfn_in_nid() returns true or false, not the found nid
> > 
> > I think you meant to change this to call early_pfn_to_nid()
> > 
> sorry..
> 
> > I'll make that correction and test your patch.
> > 
> > BTW, if you make that conversion there is no need for
> > early_pfn_in_nid() since there will be no other users.
> > 
> 
> Thanks, maybe the patch will be like this.

I tested this and I can no longer reproduce the problem.

Thanks!

Re: HOLES_IN_ZONE...

[Mel Gorman]

On Thu, Feb 05, 2009 at 06:34:09PM +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 05 Feb 2009 01:21:23 -0800 (PST)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Date: Thu, 5 Feb 2009 18:06:17 +0900
> > 
> > > @@ -2632,7 +2633,8 @@ void __meminit memmap_init_zone(unsigned
> > >  		if (context == MEMMAP_EARLY) {
> > >  			if (!early_pfn_valid(pfn))
> > >  				continue;
> > > -			if (!early_pfn_in_nid(pfn, nid))
> > > +			tmp = early_pfn_in_nid(pfn, nid);
> > > +			if (tmp > -1 && tmp != nid)
> > 
> > early_pfn_in_nid() returns true or false, not the found nid
> > 
> > I think you meant to change this to call early_pfn_to_nid()
> > 
> sorry..
> 
> > I'll make that correction and test your patch.
> > 
> > BTW, if you make that conversion there is no need for
> > early_pfn_in_nid() since there will be no other users.
> > 
> 
> Thanks, maybe the patch will be like this.
> -Kame
> ==
> If a pfn is not in early_node_map[], memmap for it is not initialized.
> By this, PG_reserved is not set and the invalid memmap may sneak into buddy
> allocator.
> 

Under ordinary circumstances, it should not sneak into the buddy allocator. If
it is not initialised, then the page zone and node linkages will also not
be setup and none of the flags, critically PageBuddy, will never get set
either so it cannot merge. Things like move_freepages(), lumpy reclaim
and /proc/pagetypinfo read them though which is bad.

====
If a PFN is not in early_node_map[] then the struct page for it is not
initialised. If there are holes within a MAX_ORDER_NR_PAGES range of
pages, then PG_reserved will not be set. Code that walks PFNs within
MAX_ORDER_NR_PAGES will then use uninitialised struct pages.

To avoid any problems, this patch initialises holes within a MAX_ORDER_NR_PAGES
that valid memmap exists but is otherwise unused.
====

On a different note, deleting that BUG_ON would also have been safe in
this context as PageBuddy() would not have been set.

> To avoid that, initialize it with give nid if no early_node_map[] for pfn
> exists. PG_reserved will make this page unused.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  include/linux/mmzone.h |    6 ------
>  mm/page_alloc.c        |    7 +++++--
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> Index: mmotm-2.6.29-Feb03/mm/page_alloc.c
> ===================================================================
> --- mmotm-2.6.29-Feb03.orig/mm/page_alloc.c
> +++ mmotm-2.6.29-Feb03/mm/page_alloc.c
> @@ -2618,6 +2618,7 @@ void __meminit memmap_init_zone(unsigned
>  	unsigned long end_pfn = start_pfn + size;
>  	unsigned long pfn;
>  	struct zone *z;
> +	int tmp;
>  

tmp is a horrible name and can be declared below

>  	if (highest_memmap_pfn < end_pfn - 1)
>  		highest_memmap_pfn = end_pfn - 1;
> @@ -2632,7 +2633,8 @@ void __meminit memmap_init_zone(unsigned
>  		if (context == MEMMAP_EARLY) {

int actual_nid;

or something similar to indicate it is the NID as identified by
early_node_map[].

>  			if (!early_pfn_valid(pfn))
>  				continue;
> -			if (!early_pfn_in_nid(pfn, nid))
> +			tmp = early_pfn_to_nid(pfn);
> +			if (tmp > -1 && tmp != nid)
>  				continue;
>  		}
>  		page = pfn_to_page(pfn);
> @@ -2999,8 +3001,9 @@ int __meminit early_pfn_to_nid(unsigned 
>  			return early_node_map[i].nid;
>  	}
>  
> -	return 0;
> +	return -1;
>  }

Ok, I think these changes are safe. I looked at the other callers of
early_pfn_in_nid() and to have any trouble, they would have to be
passing in PFNs that make no sense.

Because you check -1, I also see no way for memmap for nodes to be
accidentally initialised twice.

> +
>  #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
>  
>  /* Basic iterator support to walk early_node_map[] */
> Index: mmotm-2.6.29-Feb03/include/linux/mmzone.h
> ===================================================================
> --- mmotm-2.6.29-Feb03.orig/include/linux/mmzone.h
> +++ mmotm-2.6.29-Feb03/include/linux/mmzone.h
> @@ -1070,12 +1070,6 @@ void sparse_init(void);
>  #define sparse_index_init(_sec, _nid)  do {} while (0)
>  #endif /* CONFIG_SPARSEMEM */
>  
> -#ifdef CONFIG_NODES_SPAN_OTHER_NODES
> -#define early_pfn_in_nid(pfn, nid)	(early_pfn_to_nid(pfn) == (nid))
> -#else
> -#define early_pfn_in_nid(pfn, nid)	(1)
> -#endif
> -
>  #ifndef early_pfn_valid
>  #define early_pfn_valid(pfn)	(1)
>  #endif
> 

Other than the tmp thing which is pure cosmetic

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

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

[BUGFIX][PATCH] fix memmap init to initialize valid memmap for memory hole. (Was Re: HOLES_IN_ZONE...

[KAMEZAWA Hiroyuki]

On Thu, 5 Feb 2009 10:39:25 +0000
Mel Gorman <mel@csn.ul.ie> wrote:

> ====
> If a PFN is not in early_node_map[] then the struct page for it is not
> initialised. If there are holes within a MAX_ORDER_NR_PAGES range of
> pages, then PG_reserved will not be set. Code that walks PFNs within
> MAX_ORDER_NR_PAGES will then use uninitialised struct pages.
> 
> To avoid any problems, this patch initialises holes within a MAX_ORDER_NR_PAGES
> that valid memmap exists but is otherwise unused.
> ====
> 
Thank you, I use this text.
==
If PFN is not in early_node_map[] then struct page for it is not initialized.
If there are holes within a MAX_ORDER_NE_PAGES range of pages, then PG_reserved
will not be set. Code that walks PFNs within MAX_ORDER_NR_PAGES will the use
uninitialized struct pages.

To avoid any problems, this patch initializes holes within a MAX_ORDER_NR_PAGES
that valid memmap exists but is otherwise unused.

Changelog:
  - renamed variable from "tmp" to "actual_nid"

Reported-by: David Miller <davem@davemlloft.net>
Acked-by: Mel Gorman <mel@csn.ul.ie>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 include/linux/mmzone.h |    6 ------
 mm/page_alloc.c        |   13 +++++++++++--
 2 files changed, 11 insertions(+), 8 deletions(-)

Index: linux-2.6.28-rc3/mm/page_alloc.c
===================================================================
--- linux-2.6.28-rc3.orig/mm/page_alloc.c
+++ linux-2.6.28-rc3/mm/page_alloc.c
@@ -2632,9 +2632,18 @@ void __meminit memmap_init_zone(unsigned
 		 * exist on hotplugged memory.
 		 */
 		if (context == MEMMAP_EARLY) {
+			int nid_from_node_memory_map;
+
 			if (!early_pfn_valid(pfn))
 				continue;
-			if (!early_pfn_in_nid(pfn, nid))
+			/*
+			 * early_pfn_to_nid() returns -1 if the page doesn't
+			 * exist in early_node_map[]. Initialize it in force
+			 * and set PG_reserved at el.
+			 */
+			nid_from_node_memory_map = early_pfn_to_nid(pfn);
+			if (nid_from_node_memory_map > -1 &&
+				nid_from_node_memory_map != nid)
 				continue;
 		}
 		page = pfn_to_page(pfn);
@@ -3001,7 +3010,7 @@ int __meminit early_pfn_to_nid(unsigned 
 			return early_node_map[i].nid;
 	}
 
-	return 0;
+	return -1;
 }
 #endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
 
Index: linux-2.6.28-rc3/include/linux/mmzone.h
===================================================================
--- linux-2.6.28-rc3.orig/include/linux/mmzone.h
+++ linux-2.6.28-rc3/include/linux/mmzone.h
@@ -1070,12 +1070,6 @@ void sparse_init(void);
 #define sparse_index_init(_sec, _nid)  do {} while (0)
 #endif /* CONFIG_SPARSEMEM */
 
-#ifdef CONFIG_NODES_SPAN_OTHER_NODES
-#define early_pfn_in_nid(pfn, nid)	(early_pfn_to_nid(pfn) == (nid))
-#else
-#define early_pfn_in_nid(pfn, nid)	(1)
-#endif
-
 #ifndef early_pfn_valid
 #define early_pfn_valid(pfn)	(1)
 #endif

Re: HOLES_IN_ZONE...

[Mel Gorman]

On Wed, Feb 04, 2009 at 10:26:51PM -0800, David Miller wrote:
> 
> So I've been fighting mysterious crashes on my main sparc64 devel
> machine.  What's happening is that the assertion in
> mm/page_alloc.c:move_freepages() is triggering:
> 
> 	BUG_ON(page_zone(start_page) != page_zone(end_page));
> 
> Once I knew this is what was happening, I added some annotations:
> 
> 	if (unlikely(page_zone(start_page) != page_zone(end_page))) {
> 		printk(KERN_ERR "move_freepages: Bogus zones: "
> 		       "start_page[%p] end_page[%p] zone[%p]\n",
> 		       start_page, end_page, zone);
> 		printk(KERN_ERR "move_freepages: "
> 		       "start_zone[%p] end_zone[%p]\n",
> 		       page_zone(start_page), page_zone(end_page));
> 		printk(KERN_ERR "move_freepages: "
> 		       "start_pfn[0x%lx] end_pfn[0x%lx]\n",
> 		       page_to_pfn(start_page), page_to_pfn(end_page));
> 		printk(KERN_ERR "move_freepages: "
> 		       "start_nid[%d] end_nid[%d]\n",
> 		       page_to_nid(start_page), page_to_nid(end_page));
>  ...
> 
> And here's what I got:
> 
> 	move_freepages: Bogus zones: start_page[2207d0000] end_page[2207dffc0] zone[fffff8103effcb00]
> 	move_freepages: start_zone[fffff8103effcb00] end_zone[fffff8003fffeb00]
> 	move_freepages: start_pfn[0x81f600] end_pfn[0x81f7ff]
> 	move_freepages: start_nid[1] end_nid[0]
> 
> My memory layout on this box is:
> 
> [    0.000000] Zone PFN ranges:
> [    0.000000]   Normal   0x00000000 -> 0x0081ff5d
> [    0.000000] Movable zone start PFN for each node
> [    0.000000] early_node_map[8] active PFN ranges
> [    0.000000]     0: 0x00000000 -> 0x00020000
> [    0.000000]     1: 0x00800000 -> 0x0081f7ff
> [    0.000000]     1: 0x0081f800 -> 0x0081fe50
> [    0.000000]     1: 0x0081fed1 -> 0x0081fed8
> [    0.000000]     1: 0x0081feda -> 0x0081fedb
> [    0.000000]     1: 0x0081fedd -> 0x0081fee5
> [    0.000000]     1: 0x0081fee7 -> 0x0081ff51
> [    0.000000]     1: 0x0081ff59 -> 0x0081ff5d

Based on your memory layout, it would appear that end_pfn was not
initialised at startup. What is the full dmesg output when
CONFIG_MEMORY_INIT is set and mminit_loglevel=4? It'll tell me what
ranges of memory were actually initialised.

> 
> So it's a block move in that 0x81f600-->0x81f7ff region which triggers
> the problem.
> 
> So I did a lot (and I do mean _A LOT_) of digging.  And it seems that
> unless you set HOLES_IN_ZONE you have to make sure that all of the
> memmap regions of free space in a zone begin and end on an HPAGE_SIZE
> boundary (the requirement used to be that it had to be MAX_ORDER
> sized).
> 

hmm, it still should be a MAX_ORDER boundary. HPAGE_SIZE defines
pageblock_order and it should be smaller than MAX_ORDER. Has this
changed?

> Well, this assumption enterred the tree back in 2005 (!!!) from
> the following commit in the history-2.6 tree:
> 
> commit 69fba2dd0335abec0b0de9ac53d5bbb67c31fc60
> Author: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date:   Fri Jan 7 22:01:35 2005 -0800
> 
>     [PATCH] no buddy bitmap patch revisit: for mm/page_alloc.c
> 
> At the time only IA64 had HOLES_IN_ZONE added, this happens in the
> commit right after the above one.
> 
> So in theory Sparc64 has been broken since that commit and subject to
> potential memory corruption and other unnice things.
> 

Potentially, but unlikely. The number of areas that read a block of
pages are fairly small and relatively recent. move_freepages() is one
obviously and it had that BUG_ON() in place just in case this assumption
triggered. Conceivably, you might also be able to trigger badness with 
"cat /proc/pagetypeinfo".

The real badness happens when pages belonging to different zones or node are
considered to be buddies of each other and merged but that situation would
not have triggered on your machine at least.

> I also noticed that when S390 got virtual memmap support, it acquired
> the HOLES_IN_ZONE setting as well, in this commit:
> 
> commit f4eb07c17df2e6cf9bd58bfcd9cc9e05e9489d07
> Author: Heiko Carstens <heiko.carstens@de.ibm.com>
> Date:   Fri Dec 8 15:56:07 2006 +0100
> 
>     [S390] Virtual memmap for s390.
> 
> This is confusing.  Is HOLES_IN_ZONE only required when virtual mmap
> is being used?  If so, why is that?  This is a very poorly documented
> flag, and I'm saying this after pouring over every commit referencing
> it.
> 

!HOLES_IN_ZONE
	If any one PFN within a MAX_ORDER_NR_PAGES is valid, then you
	can assume all PFNs in there are valid.

HOLES_IN_ZONE
	Due to memory holes or unaligned zones, all PFNs within a
	MAX_ORDER_NR_PAGE may not be valid.

What this affects in practice is what pfn_valid_within() does. If you are
walking all the PFNs within a MAX_ORDER block, you need to call pfn_valid()
once per MAX_ORDER and pfn_valid_within() for each PFN. When
!HOLES_IN_ZONE, this becomes a no-op.

> Later this HOLES_IN_ZONE requirement was removed on s390 by commit:
> 
> commit 9f4b0ba81f158df459fa2cfc98ab1475c090f29c
> Author: Heiko Carstens <heiko.carstens@de.ibm.com>
> Date:   Sat Jan 26 14:11:02 2008 +0100
> 
>     [S390] Get rid of HOLES_IN_ZONE requirement.
> 
> Anyways...
> 
> The point of this email is, do I really need to set this thing on
> sparc64? I've never seen this check triggered before, it only seems
> to have started triggering in 2.6.27 or so but I obviously can't find
> anything that would influence something of this nature.
> 

Possibly not. If we ensure there is valid memmap for each PFN within a
MAX_ORDER block, then they would get marked reserved and never merged.
Other walkers should just ignore them in that case and you don't need
HOLES_IN_ZONE.

> It takes a lot of stressing to get that specific chunk of pages to
> attempt to be freed up in a group like that :-/
> 

It'd be down to luck.

> As a suggestion, it would have been a lot more pleasant if the code
> validated this requirement (in the !HOLES_IN_ZONE case) at boot time
> instead of after 2 hours of stress testing :-(
> 

Nice maybe, but we'd take a hit on pfn_valid_within() which goes from
being compiled-away on architectures that don't need it to being
a read of a shared cacheline and a branch.

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

Re: HOLES_IN_ZONE...

[David Miller]

From: Mel Gorman <mel@csn.ul.ie>
Date: Thu, 5 Feb 2009 10:10:23 +0000

> On Wed, Feb 04, 2009 at 10:26:51PM -0800, David Miller wrote:
> > As a suggestion, it would have been a lot more pleasant if the code
> > validated this requirement (in the !HOLES_IN_ZONE case) at boot time
> > instead of after 2 hours of stress testing :-(
> > 
> 
> Nice maybe, but we'd take a hit on pfn_valid_within() which goes from
> being compiled-away on architectures that don't need it to being
> a read of a shared cacheline and a branch.

I said at boot time Mel, not in the fast paths.  Meaning, we'd
check it after all the available ranges get registered and
tossed into the free page lists.

Anyways, it appears we have a fix already, read the rest of
the thread.

Re: HOLES_IN_ZONE...

[Mel Gorman]

On Thu, Feb 05, 2009 at 02:14:29AM -0800, David Miller wrote:
> From: Mel Gorman <mel@csn.ul.ie>
> Date: Thu, 5 Feb 2009 10:10:23 +0000
> 
> > On Wed, Feb 04, 2009 at 10:26:51PM -0800, David Miller wrote:
> > > As a suggestion, it would have been a lot more pleasant if the code
> > > validated this requirement (in the !HOLES_IN_ZONE case) at boot time
> > > instead of after 2 hours of stress testing :-(
> > > 
> > 
> > Nice maybe, but we'd take a hit on pfn_valid_within() which goes from
> > being compiled-away on architectures that don't need it to being
> > a read of a shared cacheline and a branch.
> 
> I said at boot time Mel, not in the fast paths. 

pfn_valid_within() is not used in any critical paths, but it's used in
the page free path so it would be a hit there.

> Meaning, we'd
> check it after all the available ranges get registered and
> tossed into the free page lists.
> 

It's not a free-list problem, it's walkers of PFN ranges. Anyway, it's
not important to the current problem.

> Anyways, it appears we have a fix already, read the rest of
> the thread.
> 

Yes. Thanks Kamezawa Hiroyu for the fix.

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