assert/crash in __rmqueue() when enabling CONFIG_NUMA

assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]

[mail resent with a compressed crash.log]

the 2.6.17-rc2 kernel built with the attached config crashes quite 
easily under minimal load. Disabling CONFIG_NUMA makes it robust again.  
Crashlog attached.

	Ingo

------------[ cut here ]------------
kernel BUG at mm/page_alloc.c:507!
invalid opcode: 0000 [#1]
PREEMPT SMP DEBUG_PAGEALLOC
Modules linked in:
CPU:    0
EIP:    0060:[<c104e2a2>]    Not tainted VLI
EFLAGS: 00010002   (2.6.17-rc2-lockdep #18) 
EIP is at __rmqueue+0x112/0x120
eax: 00000001   ebx: c2d7d800   ecx: 00037e00   edx: 00037d00
esi: c1dc26d0   edi: 00000008   ebp: f3d29e00   esp: f3d29de4
ds: 007b   es: 007b   ss: 0068
Process cc1 (pid: 3314, threadinfo=f3d28000 task=f3d21660)
Stack: <0>00000000 c1dc2600 c2d79000 00000100 c4e97c98 00000006 c4e97ca4 f3d29e5c 
       c104e58a 00000003 00000044 00000001 00000001 c1dc2640 c1dc2600 00000000 
       00000003 c1dc2908 00000000 000280d2 c1dc2908 00000007 00000000 f3d29e60 
Call Trace:
 [<c10054dd>] show_stack_log_lvl+0xbd/0x100
 [<c100574b>] show_registers+0x1bb/0x250
 [<c1005a51>] die+0x131/0x2f0
 [<c1776e71>] do_trap+0x81/0xc0
 [<c1005fd7>] do_invalid_op+0x97/0xb0
 [<c1004f4b>] error_code+0x4f/0x54
 [<c104e58a>] get_page_from_freelist+0x2da/0x520
 [<c104e825>] __alloc_pages+0x55/0x2f0
 [<c1063a7e>] alloc_page_vma+0x3e/0xc0
 [<c105841c>] __handle_mm_fault+0xa6c/0xc90
 [<c1777fb9>] do_page_fault+0x259/0x7c0
 [<c1004f4b>] error_code+0x4f/0x54
Code: 8b 0e 39 f1 0f 85 46 ff ff ff 43 83 fb 0b 75 ea 89 f6 8d bc 27 00 00 00 00 c7 45 ec 00 00 00 00 8b 45 ec 83 c4 10 5b 5e 5f 5d c3 <0f> 0b fb 01 25 4b 7e c1 eb 96 8d 74 26 00 55 89 e5 57 56 53 83 


[-- Attachment #2: crash.log.bz2 --]
[-- Type: application/x-bzip2, Size: 28359 bytes --]

[-- Attachment #3: config.bz2 --]
[-- Type: application/x-bzip2, Size: 10264 bytes --]

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Nick Piggin]

Andy Whitcroft wrote:

> I agree that there is no need for these checks to leak out of
> page_is_buddy().  If its not there or in another zone, its not my buddy.
>  The allocator loop is nasty enough as it is.

OK, glad you agree.

> 
> I think we need to do a couple of things:
> 
> 1) check the alignment of the zones matches the implied alignment
> constraints and correct it as we go.

Yes. And preferably have checks in the generic page allocator setup
code, so we can do something sane if the arch code gets it wrong.

> 2) optionally allow an architecture to say its not aligning and doesn't
> want to have to align its zone -- providing a config option to add the
> zone index checks
> 
> I think the later is valuable for these test builds and potentially for
> the embedded side where megabytes mean something.

Yes. Depends whether we fold it under the HOLES_IN_ZONE config. I guess
HOLES_IN_ZONE is potentially quite a bit more expensive than the plain
zone check, so having 2 config options may not be unreasonable.

Also, if the architecture doesn't align the ends of zones, *and* they are
not adjacent to another zone, they need either CONFIG_HOLES_IN_ZONE or
they need to provide dummy 'struct page's that never have PageBuddy set.


> 
> I'm testing a patch for this at the moment and will drop it out when I'm
> done.

Great!

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Andy Whitcroft]

Nick Piggin wrote:
> Dave Hansen wrote:
> 
>> Ahhh.  I hadn't made the ia64 connection.  I wonder if it is worth
>> making CONFIG_HOLES_IN_ZONE say ia64 or something about vmem_map in it
>> somewhere.  Might be worth at least a comment like this:
>>
>> +               if (page_in_zone_hole(buddy)) /* noop on all but ia64 */
>> +                       break;
>> +               else if (page_zonenum(buddy) != page_zonenum(page))
>> +                       break;
>> +               else if (!page_is_buddy(buddy, order))
>>                         break;          /* Move the buddy up one
>> level. */
>>
>> BTW, wasn't the whole idea of discontig to have holes in zones (before
>> NUMA) without tricks like this? ;)
> 
> 
> Yes.
> 
> I don't like the patch much, because all that logic should be moved
> into page_is_buddy where I put it (surely it is more readable not to
> have the checks spilling out -- a page which is not in the correct
> zone or is a "hole" is by definition not a buddy, right?)
> 
> So, I agree with adding the zone check if any architecture needs it.
> But it would be something under CONFIG_HOLES_IN_ZONE, and the arch
> needs to *either* align zones correctly (as they've always had to),
> or turn this option on.

I agree that there is no need for these checks to leak out of
page_is_buddy().  If its not there or in another zone, its not my buddy.
 The allocator loop is nasty enough as it is.

I think we need to do a couple of things:

1) check the alignment of the zones matches the implied alignment
constraints and correct it as we go.
2) optionally allow an architecture to say its not aligning and doesn't
want to have to align its zone -- providing a config option to add the
zone index checks

I think the later is valuable for these test builds and potentially for
the embedded side where megabytes mean something.

I'm testing a patch for this at the moment and will drop it out when I'm
done.

-apw

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Nick Piggin]

Dave Hansen wrote:

> Ahhh.  I hadn't made the ia64 connection.  I wonder if it is worth
> making CONFIG_HOLES_IN_ZONE say ia64 or something about vmem_map in it
> somewhere.  Might be worth at least a comment like this:
> 
> +               if (page_in_zone_hole(buddy)) /* noop on all but ia64 */
> +                       break;
> +               else if (page_zonenum(buddy) != page_zonenum(page))
> +                       break;
> +               else if (!page_is_buddy(buddy, order))
>                         break;          /* Move the buddy up one level. */
> 
> BTW, wasn't the whole idea of discontig to have holes in zones (before
> NUMA) without tricks like this? ;)

Yes.

I don't like the patch much, because all that logic should be moved
into page_is_buddy where I put it (surely it is more readable not to
have the checks spilling out -- a page which is not in the correct
zone or is a "hole" is by definition not a buddy, right?)

So, I agree with adding the zone check if any architecture needs it.
But it would be something under CONFIG_HOLES_IN_ZONE, and the arch
needs to *either* align zones correctly (as they've always had to),
or turn this option on.

Thanks for working at this, everyone.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Bob Picco]

Martin J. Bligh wrote:	[Fri May 05 2006, 11:03:02AM EDT]
> >Ahhh.  I hadn't made the ia64 connection.  I wonder if it is worth
> >making CONFIG_HOLES_IN_ZONE say ia64 or something about vmem_map in it
> >somewhere.  Might be worth at least a comment like this:
> >
> >+               if (page_in_zone_hole(buddy)) /* noop on all but ia64 */
> >+                       break;
> >+               else if (page_zonenum(buddy) != page_zonenum(page))
> >+                       break;
> >+               else if (!page_is_buddy(buddy, order))
> >                        break;          /* Move the buddy up one level. */
> >
> >BTW, wasn't the whole idea of discontig to have holes in zones (before
> >NUMA) without tricks like this? ;)
> 
> Sparsemem should fix this - that was one of the things Andy designed it
> for. Then we can remove the virtual memmap stuff (and discontig).
> Indeed, I'd hope we're ready to do that real soon now ... has anyone
> got an ia64 box that needed virtual memmap that they could test this
> on?
> 
> M.
I totally agree about SPARSEMEM. I believe most ia64 boxes use
VIRTUAL_MEM_MAP. I only know of Fujitsu and myself that use SPARSEMEM
for ia64 (perhaps Andy too in his testing). Dave and I have advocated its use
more than once.

bob

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Bob Picco]

Dave Hansen wrote:	[Fri May 05 2006, 10:57:44AM EDT]
> On Fri, 2006-05-05 at 10:50 -0400, Bob Picco wrote:
> > Dave Hansen wrote:	[Fri May 05 2006, 10:33:10AM EDT]
> > > The page_zonenum() checks look good, but I'm not sure I understand the
> > > page_in_zone_hole() part.  If a page is in a hole in a zone, it will
> > > still have a valid mem_map entry, right?  It should also never have been
> > > put into the allocator, so it also won't ever be coalesced.  
> > This has always been subtle and not too revealing.  It probably should
> > have a comment. The page_in_zone_hole check is for ia64 
> > VIRTUAL_MEM_MAP. You might compute a page structure which is in a hole not 
> > backed by memory; an unallocated page which covers pages structures. 
> > VIRTUAL_MEM_MAP uses a contiguous virtual region with virtual space holes
> > not backed by memory. Take a look at ia64_pfn_valid.
> 
> Ahhh.  I hadn't made the ia64 connection.  I wonder if it is worth
> making CONFIG_HOLES_IN_ZONE say ia64 or something about vmem_map in it
> somewhere.  Might be worth at least a comment like this:
> 
> +               if (page_in_zone_hole(buddy)) /* noop on all but ia64 */
> +                       break;
> +               else if (page_zonenum(buddy) != page_zonenum(page))
> +                       break;
> +               else if (!page_is_buddy(buddy, order))
>                         break;          /* Move the buddy up one level. */
> 
> BTW, wasn't the whole idea of discontig to have holes in zones (before
> NUMA) without tricks like this? ;)
Sure you could boot ia64 with just DISCONTIGMEM and no VIRTUAL_MEM_MAP.
In fact that's exactly what I did to test code added in alloc_node_mem_map.
Unfortunately I was missing 1Gb from free memory after booting. The
missing 1Gb was consumed by reserved pages structures :)
> 
> -- Dave
> 
bob

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Martin J. Bligh]

> Ahhh.  I hadn't made the ia64 connection.  I wonder if it is worth
> making CONFIG_HOLES_IN_ZONE say ia64 or something about vmem_map in it
> somewhere.  Might be worth at least a comment like this:
> 
> +               if (page_in_zone_hole(buddy)) /* noop on all but ia64 */
> +                       break;
> +               else if (page_zonenum(buddy) != page_zonenum(page))
> +                       break;
> +               else if (!page_is_buddy(buddy, order))
>                         break;          /* Move the buddy up one level. */
> 
> BTW, wasn't the whole idea of discontig to have holes in zones (before
> NUMA) without tricks like this? ;)

Sparsemem should fix this - that was one of the things Andy designed it
for. Then we can remove the virtual memmap stuff (and discontig).
Indeed, I'd hope we're ready to do that real soon now ... has anyone
got an ia64 box that needed virtual memmap that they could test this
on?

M.

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Dave Hansen]

On Fri, 2006-05-05 at 10:50 -0400, Bob Picco wrote:
> Dave Hansen wrote:	[Fri May 05 2006, 10:33:10AM EDT]
> > The page_zonenum() checks look good, but I'm not sure I understand the
> > page_in_zone_hole() part.  If a page is in a hole in a zone, it will
> > still have a valid mem_map entry, right?  It should also never have been
> > put into the allocator, so it also won't ever be coalesced.  
> This has always been subtle and not too revealing.  It probably should
> have a comment. The page_in_zone_hole check is for ia64 
> VIRTUAL_MEM_MAP. You might compute a page structure which is in a hole not 
> backed by memory; an unallocated page which covers pages structures. 
> VIRTUAL_MEM_MAP uses a contiguous virtual region with virtual space holes
> not backed by memory. Take a look at ia64_pfn_valid.

Ahhh.  I hadn't made the ia64 connection.  I wonder if it is worth
making CONFIG_HOLES_IN_ZONE say ia64 or something about vmem_map in it
somewhere.  Might be worth at least a comment like this:

+               if (page_in_zone_hole(buddy)) /* noop on all but ia64 */
+                       break;
+               else if (page_zonenum(buddy) != page_zonenum(page))
+                       break;
+               else if (!page_is_buddy(buddy, order))
                        break;          /* Move the buddy up one level. */

BTW, wasn't the whole idea of discontig to have holes in zones (before
NUMA) without tricks like this? ;)

-- Dave

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Bob Picco]

Dave Hansen wrote:	[Fri May 05 2006, 10:33:10AM EDT]
> On Fri, 2006-05-05 at 09:55 -0400, Bob Picco wrote:
> > -               if (!page_is_buddy(buddy, order))
> > +               if (page_in_zone_hole(buddy))
> > +                       break;
> > +               else if (page_zonenum(buddy) != page_zonenum(page))
> > +                       break;
> > +               else if (!page_is_buddy(buddy, order))
> >                         break;          /* Move the buddy up one level. */ 
> 
> The page_zonenum() checks look good, but I'm not sure I understand the
> page_in_zone_hole() part.  If a page is in a hole in a zone, it will
> still have a valid mem_map entry, right?  It should also never have been
> put into the allocator, so it also won't ever be coalesced.  
This has always been subtle and not too revealing.  It probably should
have a comment. The page_in_zone_hole check is for ia64 
VIRTUAL_MEM_MAP. You might compute a page structure which is in a hole not 
backed by memory; an unallocated page which covers pages structures. 
VIRTUAL_MEM_MAP uses a contiguous virtual region with virtual space holes
not backed by memory. Take a look at ia64_pfn_valid.
> 
> I'm a bit confused. :(
> 
> BTW, I like the idea of just aligning HIGHMEM's start because it has no
> runtime cost.  Buuuuut, it is still just a shift and compare of the two
> page->flags, which should already be (or will soon anyway be) in the
> cache.
Yes. I'll defer to Andy whether he wants the zonenum check or to align
HIGHMEM corrrectly.
> 
> -- Dave
> 
bob

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Dave Hansen]

On Fri, 2006-05-05 at 09:55 -0400, Bob Picco wrote:
> -               if (!page_is_buddy(buddy, order))
> +               if (page_in_zone_hole(buddy))
> +                       break;
> +               else if (page_zonenum(buddy) != page_zonenum(page))
> +                       break;
> +               else if (!page_is_buddy(buddy, order))
>                         break;          /* Move the buddy up one level. */ 

The page_zonenum() checks look good, but I'm not sure I understand the
page_in_zone_hole() part.  If a page is in a hole in a zone, it will
still have a valid mem_map entry, right?  It should also never have been
put into the allocator, so it also won't ever be coalesced.  

I'm a bit confused. :(

BTW, I like the idea of just aligning HIGHMEM's start because it has no
runtime cost.  Buuuuut, it is still just a shift and compare of the two
page->flags, which should already be (or will soon anyway be) in the
cache.

-- Dave

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Bob Picco]

Andy Wihitcroft wrote:	[Thu May 04 2006, 05:50:29PM EDT]
> Bob Picco wrote:
> > Ingo Molnar wrote:	[Thu May 04 2006, 03:25:28PM EDT]
> > 
> >>* Bob Picco <bob.picco@hp.com> wrote:
> >>
> >>
> >>>Dave Hansen wrote:	[Thu May 04 2006, 11:21:06AM EDT]
> >>>
> >>>>I haven't thought through it completely, but these two lines worry me:
> >>>>
> >>>>
> >>>>>+ start = pgdat->node_start_pfn & ~((1 << (MAX_ORDER - 1)) - 1);
> >>>>>+ end = start + pgdat->node_spanned_pages;
> >>>>
> >>>>Should the "end" be based off of the original "start", or the aligned
> >>>>"start"?
> >>>
> >>>Yes. I failed to quilt refresh before sending. You mean end should be 
> >>>end = pgdat->node_start_pfn + pgdat->node_spanned_pages before 
> >>>rounding up.
> >>
> >>do you have an updated patch i should try?
> >>
> >>	Ingo
> > 
> > You can try this but don't believe it will change your outcome. I've
> > booted this on ia64 with slight modification to eliminate
> > VIRTUAL_MEM_MAP and have only DISCONTIGMEM. Your case is failing at the
> > front edge of of the zone and not the ending edge which had a flaw in my
> > first post of the patch. I would have expected the first patch to handle
> > the front edge correctly.
> > 
> > I don't remember seeing your .config in the thread (or blind and unable
> > to see it). Would you please send it my way.
> > 
> > I'm also hoping Andy has time to look into this.
> > 
> > bob
> 
> Yeah will have a look tommorrow my time.  Could you drop me the .config
> too.  There is definatly some unstated requirements on alignment, which
> I was testing today.  I presume its one of those thats being violated.
> 
> -apw
I think the problem was my not looking closely at the full email thread. 
I finally found time to read entire thread (found Ingo's config and boot logs). The patch below should fix Ingo's problem.  It's probably only required for 
ZONE_HIGHMEM.  To be safe, I think we should apply it generically. 

Not only must node_mem_map array be MAX_ORDER aligned but the the distance 
between interior zones covered by node_mem_map must satisfy this alignment. 
While in the buddy allocator before checking for a valid buddy the buddy page 
must reside in the parent's zone too. ZONE_HIGHMEM doesn't satisfy the zone 
alignment condition and requires this new check that the parent's buddy and 
parent are within by the same zone.

The other possible solution is aligning HIGHMEM zone to satisfy MAX_ORDER.
This I didn't pursue and possibly is what Andy refers to above.

Adding a printk for the line with the zonenum mismatch condition caught two
instances in boot up on my x86 which was configured similarly to Ingo's config.

bob

Index: linux-2.6.17-rc3/mm/page_alloc.c
===================================================================
--- linux-2.6.17-rc3.orig/mm/page_alloc.c	2006-04-27 09:44:02.000000000 -0400
+++ linux-2.6.17-rc3/mm/page_alloc.c	2006-05-05 07:42:40.000000000 -0400
@@ -280,6 +280,15 @@ __find_combined_index(unsigned long page
 	return (page_idx & ~(1 << order));
 }
 
+static inline int page_in_zone_hole(struct page *page)
+{
+#ifdef CONFIG_HOLES_IN_ZONE
+	if (!pfn_valid(page_to_pfn(page)))
+		return 1;
+#endif
+	return 0;
+}
+
 /*
  * This function checks whether a page is free && is the buddy
  * we can do coalesce a page and its buddy if
@@ -294,11 +303,6 @@ __find_combined_index(unsigned long page
  */
 static inline int page_is_buddy(struct page *page, int order)
 {
-#ifdef CONFIG_HOLES_IN_ZONE
-	if (!pfn_valid(page_to_pfn(page)))
-		return 0;
-#endif
-
 	if (PageBuddy(page) && page_order(page) == order) {
 		BUG_ON(page_count(page) != 0);
 		return 1;
@@ -351,7 +355,11 @@ static inline void __free_one_page(struc
 		struct page *buddy;
 
 		buddy = __page_find_buddy(page, page_idx, order);
-		if (!page_is_buddy(buddy, order))
+		if (page_in_zone_hole(buddy))
+			break;
+		else if (page_zonenum(buddy) != page_zonenum(page))
+			break;
+		else if (!page_is_buddy(buddy, order))
 			break;		/* Move the buddy up one level. */
 
 		list_del(&buddy->lru);
@@ -2123,14 +2131,22 @@ static void __init alloc_node_mem_map(st
 #ifdef CONFIG_FLAT_NODE_MEM_MAP
 	/* ia64 gets its own node_mem_map, before this, without bootmem */
 	if (!pgdat->node_mem_map) {
-		unsigned long size;
+		unsigned long size, start, end;
 		struct page *map;
 
-		size = (pgdat->node_spanned_pages + 1) * sizeof(struct page);
+		/*
+		 * The zone's endpoints aren't required to be MAX_ORDER
+		 * aligned but the node_mem_map endpoints must be in order
+		 * for the buddy allocator to function correctly.
+		 */
+		start = pgdat->node_start_pfn & ~(MAX_ORDER_NR_PAGES - 1);
+		end = pgdat->node_start_pfn + pgdat->node_spanned_pages;
+		end = ALIGN(end, MAX_ORDER_NR_PAGES);
+		size =  (end - start) * sizeof(struct page);
 		map = alloc_remap(pgdat->node_id, size);
 		if (!map)
 			map = alloc_bootmem_node(pgdat, size);
-		pgdat->node_mem_map = map;
+		pgdat->node_mem_map = map + (pgdat->node_start_pfn - start);
 	}
 #ifdef CONFIG_FLATMEM
 	/*
Index: linux-2.6.17-rc3/include/linux/mmzone.h
===================================================================
--- linux-2.6.17-rc3.orig/include/linux/mmzone.h	2006-04-27 09:44:02.000000000 -0400
+++ linux-2.6.17-rc3/include/linux/mmzone.h	2006-05-04 13:01:39.000000000 -0400
@@ -22,6 +22,7 @@
 #else
 #define MAX_ORDER CONFIG_FORCE_MAX_ZONEORDER
 #endif
+#define MAX_ORDER_NR_PAGES (1 << (MAX_ORDER - 1))
 
 struct free_area {
 	struct list_head	free_list;

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

* Andy Whitcroft <apw@shadowen.org> wrote:

> Yeah will have a look tommorrow my time.  Could you drop me the 
> .config too.  There is definatly some unstated requirements on 
> alignment, which I was testing today.  I presume its one of those 
> thats being violated.

the config is at:

 http://redhat.com/~mingo/misc/config

the bootlog is at:

 http://redhat.com/~mingo/misc/crash.log

	Ingo

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Andy Whitcroft]

Bob Picco wrote:
> Ingo Molnar wrote:	[Thu May 04 2006, 03:25:28PM EDT]
> 
>>* Bob Picco <bob.picco@hp.com> wrote:
>>
>>
>>>Dave Hansen wrote:	[Thu May 04 2006, 11:21:06AM EDT]
>>>
>>>>I haven't thought through it completely, but these two lines worry me:
>>>>
>>>>
>>>>>+ start = pgdat->node_start_pfn & ~((1 << (MAX_ORDER - 1)) - 1);
>>>>>+ end = start + pgdat->node_spanned_pages;
>>>>
>>>>Should the "end" be based off of the original "start", or the aligned
>>>>"start"?
>>>
>>>Yes. I failed to quilt refresh before sending. You mean end should be 
>>>end = pgdat->node_start_pfn + pgdat->node_spanned_pages before 
>>>rounding up.
>>
>>do you have an updated patch i should try?
>>
>>	Ingo
> 
> You can try this but don't believe it will change your outcome. I've
> booted this on ia64 with slight modification to eliminate
> VIRTUAL_MEM_MAP and have only DISCONTIGMEM. Your case is failing at the
> front edge of of the zone and not the ending edge which had a flaw in my
> first post of the patch. I would have expected the first patch to handle
> the front edge correctly.
> 
> I don't remember seeing your .config in the thread (or blind and unable
> to see it). Would you please send it my way.
> 
> I'm also hoping Andy has time to look into this.
> 
> bob

Yeah will have a look tommorrow my time.  Could you drop me the .config
too.  There is definatly some unstated requirements on alignment, which
I was testing today.  I presume its one of those thats being violated.

-apw

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Bob Picco]

Ingo Molnar wrote:	[Thu May 04 2006, 03:25:28PM EDT]
> 
> * Bob Picco <bob.picco@hp.com> wrote:
> 
> > Dave Hansen wrote:	[Thu May 04 2006, 11:21:06AM EDT]
> > > I haven't thought through it completely, but these two lines worry me:
> > > 
> > > > + start = pgdat->node_start_pfn & ~((1 << (MAX_ORDER - 1)) - 1);
> > > > + end = start + pgdat->node_spanned_pages;
> > > 
> > > Should the "end" be based off of the original "start", or the aligned
> > > "start"?
> >
> > Yes. I failed to quilt refresh before sending. You mean end should be 
> > end = pgdat->node_start_pfn + pgdat->node_spanned_pages before 
> > rounding up.
> 
> do you have an updated patch i should try?
> 
> 	Ingo
You can try this but don't believe it will change your outcome. I've
booted this on ia64 with slight modification to eliminate
VIRTUAL_MEM_MAP and have only DISCONTIGMEM. Your case is failing at the
front edge of of the zone and not the ending edge which had a flaw in my
first post of the patch. I would have expected the first patch to handle
the front edge correctly.

I don't remember seeing your .config in the thread (or blind and unable
to see it). Would you please send it my way.

I'm also hoping Andy has time to look into this.

bob


Index: linux-2.6.17-rc3/mm/page_alloc.c
===================================================================
--- linux-2.6.17-rc3.orig/mm/page_alloc.c	2006-04-27 09:44:02.000000000 -0400
+++ linux-2.6.17-rc3/mm/page_alloc.c	2006-05-04 13:01:25.000000000 -0400
@@ -2123,14 +2123,22 @@ static void __init alloc_node_mem_map(st
 #ifdef CONFIG_FLAT_NODE_MEM_MAP
 	/* ia64 gets its own node_mem_map, before this, without bootmem */
 	if (!pgdat->node_mem_map) {
-		unsigned long size;
+		unsigned long size, start, end;
 		struct page *map;
 
-		size = (pgdat->node_spanned_pages + 1) * sizeof(struct page);
+		/*
+		 * The zone's endpoints aren't required to be MAX_ORDER
+		 * aligned but the node_mem_map endpoints must be in order
+		 * for the buddy allocator to function correctly.
+		 */
+		start = pgdat->node_start_pfn & ~(MAX_ORDER_NR_PAGES - 1);
+		end = pgdat->node_start_pfn + pgdat->node_spanned_pages;
+		end = ALIGN(end, MAX_ORDER_NR_PAGES);
+		size =  (end - start) * sizeof(struct page);
 		map = alloc_remap(pgdat->node_id, size);
 		if (!map)
 			map = alloc_bootmem_node(pgdat, size);
-		pgdat->node_mem_map = map;
+		pgdat->node_mem_map = map + (pgdat->node_start_pfn - start);
 	}
 #ifdef CONFIG_FLATMEM
 	/*
Index: linux-2.6.17-rc3/include/linux/mmzone.h
===================================================================
--- linux-2.6.17-rc3.orig/include/linux/mmzone.h	2006-04-27 09:44:02.000000000 -0400
+++ linux-2.6.17-rc3/include/linux/mmzone.h	2006-05-04 13:01:39.000000000 -0400
@@ -22,6 +22,7 @@
 #else
 #define MAX_ORDER CONFIG_FORCE_MAX_ZONEORDER
 #endif
+#define MAX_ORDER_NR_PAGES (1 << (MAX_ORDER - 1))
 
 struct free_area {
 	struct list_head	free_list;

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

* Bob Picco <bob.picco@hp.com> wrote:

> Dave Hansen wrote:	[Thu May 04 2006, 11:21:06AM EDT]
> > I haven't thought through it completely, but these two lines worry me:
> > 
> > > + start = pgdat->node_start_pfn & ~((1 << (MAX_ORDER - 1)) - 1);
> > > + end = start + pgdat->node_spanned_pages;
> > 
> > Should the "end" be based off of the original "start", or the aligned
> > "start"?
>
> Yes. I failed to quilt refresh before sending. You mean end should be 
> end = pgdat->node_start_pfn + pgdat->node_spanned_pages before 
> rounding up.

do you have an updated patch i should try?

	Ingo

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Dave Hansen]

On Thu, 2006-05-04 at 11:46 -0400, Bob Picco wrote:
> Dave Hansen wrote:	[Thu May 04 2006, 11:21:06AM EDT]
> > I haven't thought through it completely, but these two lines worry me:
> > 
> > > + start = pgdat->node_start_pfn & ~((1 << (MAX_ORDER - 1)) - 1);
> > > + end = start + pgdat->node_spanned_pages;
> > 
> > Should the "end" be based off of the original "start", or the aligned
> > "start"?
> Yes. I failed to quilt refresh before sending. You mean end should be
> end = pgdat->node_start_pfn + pgdat->node_spanned_pages before rounding
> up.

Yep.  Looks good.

-- Dave

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Bob Picco]

Dave Hansen wrote:	[Thu May 04 2006, 11:21:06AM EDT]
> I haven't thought through it completely, but these two lines worry me:
> 
> > + start = pgdat->node_start_pfn & ~((1 << (MAX_ORDER - 1)) - 1);
> > + end = start + pgdat->node_spanned_pages;
> 
> Should the "end" be based off of the original "start", or the aligned
> "start"?
Yes. I failed to quilt refresh before sending. You mean end should be
end = pgdat->node_start_pfn + pgdat->node_spanned_pages before rounding
up.
> 
> (using decimal math to make it easy) ... 
> 
> Let's say that MAX_ORDER comes out to be 10 pages.  node_start_pfn is 9,
> and the node's end pfn is 21.  node_spanned_pages will be 12.  "start"
> will get rounded down to 0.  "end" will be "start" (0) +
> node_spanned_pages (12), so 12.  "end" then gets rounded up to 20.
> However, this is not sufficient space for the mem_map as the node
> *actually* ended at 21.
> 
> I think that "end" needs to be calculated without rounding down the
> start_pfn, or the node_spanned_pages number needs to be rounded up in
> the same way that "end" is.
> 
> Does that sound right? 
Yes.
> 
> Also, it might look nicer if there was an intermediate variable
> something like this:
> 
> 	#define MAX_ORDER_NR_PAGES (1 << (MAX_ORDER - 1))
Yes.
> 
> Take a look at the loop below, I've also used ALIGN() from kernel.h for
> the "end" alignment.  I think it is just a drop-in replacement.  
> 
>         /* ia64 gets its own node_mem_map, before this, without bootmem */
>         if (!pgdat->node_mem_map) {
>                unsigned long size, start, end;
>                struct page *map;
> 
>                /*
>                 * The zone's endpoints aren't required to be MAX_ORDER
>                 * aligned but the node_mem_map endpoints must be in order
>                 * for the buddy allocator to function correctly.
>                 */
>                start = pgdat->node_start_pfn & ~(MAX_ORDER_NR_PAGES - 1);
		 end = pgdat->node_start_pfn + pgdat->node_spanned_pages;
>                end = start + pgdat->node_spanned_pages;
>                end = ALIGN(end, MAX_ORDER_NR_PAGES);
>                size =  (end - start) * sizeof(struct page);
>                map = alloc_remap(pgdat->node_id, size);
>                if (!map)
>                        map = alloc_bootmem_node(pgdat, size);
>                pgdat->node_mem_map = map + (pgdat->node_start_pfn - start);
>        }
> 
> -- Dave
bob
> 

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Dave Hansen]

I haven't thought through it completely, but these two lines worry me:

> + start = pgdat->node_start_pfn & ~((1 << (MAX_ORDER - 1)) - 1);
> + end = start + pgdat->node_spanned_pages;

Should the "end" be based off of the original "start", or the aligned
"start"?

(using decimal math to make it easy) ... 

Let's say that MAX_ORDER comes out to be 10 pages.  node_start_pfn is 9,
and the node's end pfn is 21.  node_spanned_pages will be 12.  "start"
will get rounded down to 0.  "end" will be "start" (0) +
node_spanned_pages (12), so 12.  "end" then gets rounded up to 20.
However, this is not sufficient space for the mem_map as the node
*actually* ended at 21.

I think that "end" needs to be calculated without rounding down the
start_pfn, or the node_spanned_pages number needs to be rounded up in
the same way that "end" is.

Does that sound right? 

Also, it might look nicer if there was an intermediate variable
something like this:

	#define MAX_ORDER_NR_PAGES (1 << (MAX_ORDER - 1))

Take a look at the loop below, I've also used ALIGN() from kernel.h for
the "end" alignment.  I think it is just a drop-in replacement.  

        /* ia64 gets its own node_mem_map, before this, without bootmem */
        if (!pgdat->node_mem_map) {
               unsigned long size, start, end;
               struct page *map;

               /*
                * The zone's endpoints aren't required to be MAX_ORDER
                * aligned but the node_mem_map endpoints must be in order
                * for the buddy allocator to function correctly.
                */
               start = pgdat->node_start_pfn & ~(MAX_ORDER_NR_PAGES - 1);
               end = start + pgdat->node_spanned_pages;
               end = ALIGN(end, MAX_ORDER_NR_PAGES);
               size =  (end - start) * sizeof(struct page);
               map = alloc_remap(pgdat->node_id, size);
               if (!map)
                       map = alloc_bootmem_node(pgdat, size);
               pgdat->node_mem_map = map + (pgdat->node_start_pfn - start);
       }

-- Dave

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> the same easy crash still happens if i enable CONFIG_NUMA:

btw., with CONFIG_NUMA off i get this warning during bootup:

BUG: pfn: 0003fff0, page: c404d840, order: 4
 [<c0104e7f>] show_trace+0xd/0xf
 [<c0104e96>] dump_stack+0x15/0x17
 [<c0163312>] free_pages_bulk+0x207/0x370
 [<c01642f9>] free_hot_cold_page+0x127/0x17c
 [<c016438d>] free_hot_page+0xa/0xc
 [<c01643e5>] __free_pages+0x56/0x6f
 [<c0172e14>] __vunmap+0xc1/0xed
 [<c0172f02>] vfree+0x3b/0x3e
 [<c0128865>] build_sched_domains+0xaf2/0xcde
 [<c0128a6a>] arch_init_sched_domains+0x19/0x1b
 [<c1bd3a67>] sched_init_smp+0x18/0x349
 [<c01003c6>] init+0xb9/0x2cb
 [<c0102005>] kernel_thread_helper+0x5/0xb

but this is nonfatal and the system is robust afterwards. (this warning 
is not present if CONFIG_NUMA is on) [Btw., in the NUMA test i also had 
CONFIG_MIGRATION enabled.]

	Ingo

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > The patch below isn't compile tested or correct for those cases where 
> > alloc_remap is called or where arch code has allocated node_mem_map 
> > for CONFIG_FLAT_NODE_MEM_MAP. It's just conveying what I believe the 
> > issue is.
> 
> thx. One pair of parentheses were missing i think - see the delta fix 
> below. I'll try it.

the same easy crash still happens if i enable CONFIG_NUMA:

 zone c214e600 (HighMem):
 pfn: 00037d00
 zone->zone_start_pfn: 00037e00
 zone->spanned_pages: 00007e00
 zone->zone_start_pfn + zone->spanned_pages: 0003fc00

 [<c010574a>] do_invalid_op+0x63/0x93
 [<c0104a0b>] error_code+0x4f/0x54
 [<c0164d48>] get_page_from_freelist+0x13e/0x565
 [<c01651dd>] __alloc_pages+0x6e/0x325
 [<c017a6c9>] alloc_page_vma+0x80/0x86
 [<c016e2ae>] __handle_mm_fault+0x1e7/0xd00
 [<c10fe9af>] do_page_fault+0x339/0x7c5
 [<c0104a0b>] error_code+0x4f/0x54

see the debug patch below.

	Ingo

----
From: Ingo Molnar <mingo@elte.hu>

do buddy zone size checks unconditionally.

Signed-off-by: Ingo Molnar <mingo@elte.hu>

----

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

Index: linux/mm/page_alloc.c
===================================================================
--- linux.orig/mm/page_alloc.c
+++ linux/mm/page_alloc.c
@@ -101,17 +101,32 @@ static int page_outside_zone_boundaries(
 			ret = 1;
 	} while (zone_span_seqretry(zone, seq));
 
+#define P(x) printk("%s: %08lx\n", #x, x)
+
+	if (ret) {
+		printk("zone %p (%s):\n", zone, zone->name);
+		P(pfn);
+		P(zone->zone_start_pfn);
+		P(zone->spanned_pages);
+		P(zone->zone_start_pfn + zone->spanned_pages);
+	}
+
 	return ret;
 }
 
 static int page_is_consistent(struct zone *zone, struct page *page)
 {
-#ifdef CONFIG_HOLES_IN_ZONE
-	if (!pfn_valid(page_to_pfn(page)))
+	if (!pfn_valid(page_to_pfn(page))) {
+		printk("BUG: pfn: %08lx, page: %p\n",
+			page_to_pfn(page), page);
+		dump_stack();
 		return 0;
-#endif
-	if (zone != page_zone(page))
+	}
+	if (zone != page_zone(page)) {
+		printk("zone: %p != %p == page_zone(%p)\n",
+			zone, page_zone(page), page);
 		return 0;
+	}
 
 	return 1;
 }
@@ -309,10 +324,12 @@ __find_combined_index(unsigned long page
  */
 static inline int page_is_buddy(struct page *page, int order)
 {
-#ifdef CONFIG_HOLES_IN_ZONE
-	if (!pfn_valid(page_to_pfn(page)))
+	if (!pfn_valid(page_to_pfn(page))) {
+		printk("BUG: pfn: %08lx, page: %p, order: %d\n",
+			page_to_pfn(page), page, order);
+		dump_stack();
 		return 0;
-#endif
+	}
 
 	if (PageBuddy(page) && page_order(page) == order) {
 		BUG_ON(page_count(page) != 0);

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Andy Whitcroft]

Bob Picco wrote:
> Nick Piggin wrote:	[Tue May 02 2006, 10:25:57AM EDT]
> 
>>Martin J. Bligh wrote:
>>
>>>>Oh that's a 32bit kernel. I don't think the 32bit NUMA has ever worked
>>>>anywhere but some Summit systems (at least every time I tried it it 
>>>>blew up on me and nobody seems to use it regularly). Maybe it would be 
>>>>finally time to mark it CONFIG_BROKEN though or just remove it (even 
>>>>by design it doesn't work very well) 
>>>
>>>
>>>Bollocks. It works fine, and is tested every single day, on every git
>>>release, and every -mm tree.
>>
>>Whatever the case, there definitely does not appear to be sufficient
>>zone alignment enforced for the buddy allocator. I cannot see how it
>>could work if zones are not aligned on 4MB boundaries.
>>
>>Maybe some architectures / subarch code naturally does this for us,
>>but Ingo is definitely hitting this bug because his config does not
>>(align, that is).
>>
>>I've randomly added a couple more cc's.
>>
> 
> The patch below isn't compile tested or correct for those cases where
> alloc_remap is called or where arch code has allocated node_mem_map for
> CONFIG_FLAT_NODE_MEM_MAP. It's just conveying what I believe the issue is.
> 
> Andy added code to buddy allocator which doesn't require the zone's endpoints
> to be aligned to MAX_ORDER. I think the issue is that the buddy
> allocator requires the node_mem_map's endpoints to be MAX_ORDER aligned. 
> Otherwise __page_find_buddy could compute a buddy not in node_mem_map
> for partial MAX_ORDER regions at zone's endpoints. page_is_buddy will
> detect that these pages at endpoints aren't PG_buddy (they were zeroed
> out by bootmem allocator and not part of zone).  Of course the negative
> here is we could waste a little memory but the positive is eliminating
> all the old checks for zone boundary conditions.

Yes this is correct.  The buddy location algorithm uses the relative pfn
number to locate the buddy.  Both the old anew new free detect
algorithms require a struct page exist for that buddy regardless of
whether the page exists in memory.  Thus the node_mem_map needs to exist
out to a MAX_ORDER boundry in both directions.  With real machines we
would likely get this as memory is mostly in larger chunks than
MAX_ORDER and generally maximally aligned.  From what I can see we could
potentially not be allocating the end correctly but the rmap allocation
would likely be larger than the request so we'd get away with it.

> 
> SPARSEMEM won't encounter this issue because of MAX_ORDER size
> constraint when SPARSEMEM is configured. ia64 VIRTUAL_MEM_MAP doesn't
> need the logic either because the holes and endpoints are handled
> differently.  This leaves checking alloc_remap and other arches which
> privately allocate for node_mem_map.
> 
> Any how I could be totally wrong but like I said this requires more
> thought.

I'll have a go at testing this to see what difference it makes.  I think
there might be a problem with the buddy merging at zone boundries as we
are anyhow (or the code commentry is incomplete), so am going to have a
look at that at the same time.

-apw

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

* Bob Picco <bob.picco@hp.com> wrote:

> The patch below isn't compile tested or correct for those cases where 
> alloc_remap is called or where arch code has allocated node_mem_map 
> for CONFIG_FLAT_NODE_MEM_MAP. It's just conveying what I believe the 
> issue is.

thx. One pair of parentheses were missing i think - see the delta fix 
below. I'll try it.

	Ingo

Index: linux/mm/page_alloc.c
===================================================================
--- linux.orig/mm/page_alloc.c
+++ linux/mm/page_alloc.c
@@ -2296,7 +2296,7 @@ static void __init alloc_node_mem_map(st
 		 */
 		start = pgdat->node_start_pfn & ~((1 << (MAX_ORDER - 1)) - 1);
 		end = start + pgdat->node_spanned_pages;
-		end = (end + ((1 << (MAX_ORDER - 1)) - 1) &
+		end = (end + ((1 << (MAX_ORDER - 1)) - 1)) &
 			~((1 << (MAX_ORDER - 1)) - 1);
 		size =  (end - start) * sizeof(struct page);
 		map = alloc_remap(pgdat->node_id, size);

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Bob Picco]

Nick Piggin wrote:	[Tue May 02 2006, 10:25:57AM EDT]
> Martin J. Bligh wrote:
> >>Oh that's a 32bit kernel. I don't think the 32bit NUMA has ever worked
> >>anywhere but some Summit systems (at least every time I tried it it 
> >>blew up on me and nobody seems to use it regularly). Maybe it would be 
> >>finally time to mark it CONFIG_BROKEN though or just remove it (even 
> >>by design it doesn't work very well) 
> >
> >
> >Bollocks. It works fine, and is tested every single day, on every git
> >release, and every -mm tree.
> 
> Whatever the case, there definitely does not appear to be sufficient
> zone alignment enforced for the buddy allocator. I cannot see how it
> could work if zones are not aligned on 4MB boundaries.
> 
> Maybe some architectures / subarch code naturally does this for us,
> but Ingo is definitely hitting this bug because his config does not
> (align, that is).
> 
> I've randomly added a couple more cc's.
> 
The patch below isn't compile tested or correct for those cases where
alloc_remap is called or where arch code has allocated node_mem_map for
CONFIG_FLAT_NODE_MEM_MAP. It's just conveying what I believe the issue is.

Andy added code to buddy allocator which doesn't require the zone's endpoints
to be aligned to MAX_ORDER. I think the issue is that the buddy
allocator requires the node_mem_map's endpoints to be MAX_ORDER aligned. 
Otherwise __page_find_buddy could compute a buddy not in node_mem_map
for partial MAX_ORDER regions at zone's endpoints. page_is_buddy will
detect that these pages at endpoints aren't PG_buddy (they were zeroed
out by bootmem allocator and not part of zone).  Of course the negative
here is we could waste a little memory but the positive is eliminating
all the old checks for zone boundary conditions.

SPARSEMEM won't encounter this issue because of MAX_ORDER size
constraint when SPARSEMEM is configured. ia64 VIRTUAL_MEM_MAP doesn't
need the logic either because the holes and endpoints are handled
differently.  This leaves checking alloc_remap and other arches which
privately allocate for node_mem_map.

Any how I could be totally wrong but like I said this requires more
thought.

bob


Index: linux-2.6.17-rc3/mm/page_alloc.c
===================================================================
--- linux-2.6.17-rc3.orig/mm/page_alloc.c	2006-04-27 09:44:02.000000000 -0400
+++ linux-2.6.17-rc3/mm/page_alloc.c	2006-05-03 14:50:13.000000000 -0400
@@ -2123,14 +2123,23 @@ static void __init alloc_node_mem_map(st
 #ifdef CONFIG_FLAT_NODE_MEM_MAP
 	/* ia64 gets its own node_mem_map, before this, without bootmem */
 	if (!pgdat->node_mem_map) {
-		unsigned long size;
+		unsigned long size, start, end;
 		struct page *map;
 
-		size = (pgdat->node_spanned_pages + 1) * sizeof(struct page);
+		/*
+		 * The zone's endpoints aren't required to be MAX_ORDER
+		 * aligned but the node_mem_map endpoints must be in order
+		 * for the buddy allocator to function correctly.
+		 */
+		start = pgdat->node_start_pfn & ~((1 << (MAX_ORDER - 1)) - 1);
+		end = start + pgdat->node_spanned_pages;
+		end = (end + ((1 << (MAX_ORDER - 1)) - 1) &
+			~((1 << (MAX_ORDER - 1)) - 1);
+		size =  (end - start) * sizeof(struct page);
 		map = alloc_remap(pgdat->node_id, size);
 		if (!map)
 			map = alloc_bootmem_node(pgdat, size);
-		pgdat->node_mem_map = map;
+		pgdat->node_mem_map = map + ( pgdat->node_start_pfn - start);
 	}
 #ifdef CONFIG_FLATMEM
 	/*

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> > > The problem is that nobody regression tests it. So even if you fix it
> > > now it will be likely broken again in a few months.
> > 
> > We can add a box to the test.kernel.org harness easily enough, and
> > it will show up with an eerie red glow.
> 
> Single box is not enough - there are many possible combinations (e.g. 
> Opteron NUMA, IBM NUMA, no NUMA small box, big box with weird mappings 
> etc.). Basically you would need a real tester base.

nah. And the fact that i could boot this on a non-NUMA box already 
unearthed a weakness in the buddy allocator. (it should have much 
clearer asserts about mis-sized zones - it's not the first time we had 
them and they are hard to debug) So consider this a debugging feature. 
It also found other bugs, so even if nobody but me uses it, it's useful.

	ingo

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Andi Kleen]

On Tuesday 02 May 2006 22:13, Ingo Molnar wrote:

> nah. And the fact that i could boot this on a non-NUMA box already 
> unearthed a weakness in the buddy allocator. (it should have much 
> clearer asserts about mis-sized zones - it's not the first time we had 
> them and they are hard to debug) 

GIGO.

> So consider this a debugging feature.  
> It also found other bugs, so even if nobody but me uses it, it's useful.

It's an awful lot of ugly code for a debugging feature.

Also I never considered i386 NUMA to be particularly interesting 
because it doesn't work for the kernel lowmem which is always on node 0.
So no matter what you try you have a nasty hotspot on node 0's memory.

-Andi

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Andi Kleen]

On Tuesday 02 May 2006 21:56, Martin Bligh wrote:
> Andi Kleen wrote:
> > On Tuesday 02 May 2006 21:48, Ingo Molnar wrote:
> > 
> >>* Andi Kleen <ak@suse.de> wrote:
> >>
> >>
> >>>i386: Panic the system early when a NUMA kernel doesn't run on IBM 
> >>>NUMA
> >>
> >>nah! Lets just fix the zone sizing bug ...
> > 
> > 
> > The problem is that nobody regression tests it. So even if you fix it
> > now it will be likely broken again in a few months.
> 
> We can add a box to the test.kernel.org harness easily enough, and
> it will show up with an eerie red glow.

Single box is not enough - there are many possible combinations
(e.g. Opteron NUMA, IBM NUMA, no NUMA small box, big box with weird
mappings etc.). Basically you would need a real tester base.

-Andi

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Martin Bligh]

Andi Kleen wrote:
> On Tuesday 02 May 2006 21:48, Ingo Molnar wrote:
> 
>>* Andi Kleen <ak@suse.de> wrote:
>>
>>
>>>i386: Panic the system early when a NUMA kernel doesn't run on IBM 
>>>NUMA
>>
>>nah! Lets just fix the zone sizing bug ...
> 
> 
> The problem is that nobody regression tests it. So even if you fix it
> now it will be likely broken again in a few months.

We can add a box to the test.kernel.org harness easily enough, and
it will show up with an eerie red glow.

M.

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> i386: Panic the system early when a NUMA kernel doesn't run on IBM 
> NUMA

nah! Lets just fix the zone sizing bug ...

	Ingo

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

* Martin J. Bligh <mbligh@mbligh.org> wrote:

> > i only booted it on a non-NUMA PC. Most likely the instability is
> > caused by some sort of zone mis-sizing. (See more details in this
> > same thread.)
> 
> Ooooh, on ordinary PCs. that makes more sense.

btw., NUMA emulation on 64-bit had problems too on and off (no problems 
currently), so i'm not surprised that 32-bit NUMA has problems on 
non-NUMA boxes. It would still be useful to have it, because that way 
the NUMA codepaths can be excercised.

	Ingo

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Andi Kleen]

On Tuesday 02 May 2006 21:48, Ingo Molnar wrote:
> 
> * Andi Kleen <ak@suse.de> wrote:
> 
> > i386: Panic the system early when a NUMA kernel doesn't run on IBM 
> > NUMA
> 
> nah! Lets just fix the zone sizing bug ...

The problem is that nobody regression tests it. So even if you fix it
now it will be likely broken again in a few months.

-Andi

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Andi Kleen]

On Tuesday 02 May 2006 18:02, Martin J. Bligh wrote:

>  >
>  > i only booted it on a non-NUMA PC. Most likely the instability is
>  > caused by some sort of zone mis-sizing. (See more details in this
>  > same thread.)
> 
> Ooooh, on ordinary PCs. that makes more sense.

It tends to crash on Opteron NUMA systems too.

-Andi

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Martin J. Bligh]

 >>>>Bollocks. It works fine,
 >>>
 >>>On what kind of box? Some summit system, right?
 >>
 >>Summit and NUMA-Q, ie everything we originally created it for.
 >
 > Andi:
 >
> That's my point - it usually crashes everywhere else.

Ingo Molnar wrote:
 >
 > i only booted it on a non-NUMA PC. Most likely the instability is
 > caused by some sort of zone mis-sizing. (See more details in this
 > same thread.)

Ooooh, on ordinary PCs. that makes more sense.

> The problem is that it's not regression tested and quite complex and 
 > tends to break often and stay broken.

OK, well the regression testing issue is easily fixed, but whether it's
worth it or not is a different issue. It was originally done for the
distros really, so they could have a single kernel that supported
everything.

> If you don't want to mark it CONFIG_BROKEN then i would suggest a panic
> early when the system isn't SUMMIT (I think NUMAQ does this already)
> 
> Something like the appended patch

apw: this was your baby ... what do you want to do with it? Add it
to the automated regression testing, or kill it?

> -Andi
> 
> i386: Panic the system early when a NUMA kernel doesn't run on IBM NUMA
> 
> It has been broken forever anywhere else and is not too useful
> anyways so best to disable it.
> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> 
> Index: linux/arch/i386/kernel/srat.c
> ===================================================================
> --- linux.orig/arch/i386/kernel/srat.c
> +++ linux/arch/i386/kernel/srat.c
> @@ -327,6 +327,14 @@ int __init get_memcfg_from_srat(void)
>  	int tables = 0;
>  	int i = 0;
>  
> +	extern int use_cyclone;
> +	if (use_cyclone == 0) {
> +		/* Make sure user sees something */
> +		static const char s[] __initdata = "Not an IBM x440/NUMAQ. Don't use i386 CONFIG_NUMA anywhere else."
> +		early_printk(s);
> +		panic(s); 
> +	}
> +
>  	if (ACPI_FAILURE(acpi_find_root_pointer(ACPI_PHYSICAL_ADDRESSING,
>  						rsdp_address))) {
>  		printk("%s: System description tables not found\n",
> Index: linux/arch/i386/Kconfig
> ===================================================================
> --- linux.orig/arch/i386/Kconfig
> +++ linux/arch/i386/Kconfig
> @@ -517,6 +517,9 @@ config NUMA
>  	depends on SMP && HIGHMEM64G && (X86_NUMAQ || X86_GENERICARCH || (X86_SUMMIT && ACPI))
>  	default n if X86_PC
>  	default y if (X86_NUMAQ || X86_SUMMIT)
> +	help
> +		NUMA support. Note this only works on IBM x440 or IBM NUMAQ.
> +		Don't try to use it anywhere else.
>  
>  comment "NUMA (Summit) requires SMP, 64GB highmem support, ACPI"
>  	depends on X86_SUMMIT && (!HIGHMEM64G || !ACPI)

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

* Martin J. Bligh <mbligh@mbligh.org> wrote:

> >Well, it doesn't work for Ingo clearly. My own experiences every time
> >I tried it were similar.
> 
> What platform?

i only booted it on a non-NUMA PC. Most likely the instability is caused 
by some sort of zone mis-sizing. (See more details in this same thread.)

	Ingo

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Andi Kleen]

On Tuesday 02 May 2006 17:17, Martin J. Bligh wrote:
> Andi Kleen wrote:
> > On Tuesday 02 May 2006 16:02, Martin J. Bligh wrote:
> > 
> >>>Oh that's a 32bit kernel. I don't think the 32bit NUMA has ever worked
> >>>anywhere but some Summit systems (at least every time I tried it it blew up 
> >>>on me and nobody seems to use it regularly). Maybe it would be finally time to mark it 
> >>>CONFIG_BROKEN though or just remove it (even by design it doesn't work very well) 
> >>
> >>Bollocks. It works fine, 
> > 
> > On what kind of box? Some summit system, right?
> 
> Summit and NUMA-Q, ie everything we originally created it for.

That's my point - it usually crashes everywhere else.

> 
> > I think I stand by my original statement.
> 
> If it works fine on some platforms and not on others, I would venture to
> suggest it's a platform-specific issue, and marking the whole thing as
> CONFIG_BROKEN would be an entirely inappropriate overreaction to what
> is probably a simple bug.

The problem is that it's not regression tested and quite complex and tends
to break often and stay broken.

If you don't want to mark it CONFIG_BROKEN then i would suggest a panic
early when the system isn't SUMMIT (I think NUMAQ does this already)

Something like the appended patch

-Andi

i386: Panic the system early when a NUMA kernel doesn't run on IBM NUMA

It has been broken forever anywhere else and is not too useful
anyways so best to disable it.

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux/arch/i386/kernel/srat.c
===================================================================
--- linux.orig/arch/i386/kernel/srat.c
+++ linux/arch/i386/kernel/srat.c
@@ -327,6 +327,14 @@ int __init get_memcfg_from_srat(void)
 	int tables = 0;
 	int i = 0;
 
+	extern int use_cyclone;
+	if (use_cyclone == 0) {
+		/* Make sure user sees something */
+		static const char s[] __initdata = "Not an IBM x440/NUMAQ. Don't use i386 CONFIG_NUMA anywhere else."
+		early_printk(s);
+		panic(s); 
+	}
+
 	if (ACPI_FAILURE(acpi_find_root_pointer(ACPI_PHYSICAL_ADDRESSING,
 						rsdp_address))) {
 		printk("%s: System description tables not found\n",
Index: linux/arch/i386/Kconfig
===================================================================
--- linux.orig/arch/i386/Kconfig
+++ linux/arch/i386/Kconfig
@@ -517,6 +517,9 @@ config NUMA
 	depends on SMP && HIGHMEM64G && (X86_NUMAQ || X86_GENERICARCH || (X86_SUMMIT && ACPI))
 	default n if X86_PC
 	default y if (X86_NUMAQ || X86_SUMMIT)
+	help
+		NUMA support. Note this only works on IBM x440 or IBM NUMAQ.
+		Don't try to use it anywhere else.
 
 comment "NUMA (Summit) requires SMP, 64GB highmem support, ACPI"
 	depends on X86_SUMMIT && (!HIGHMEM64G || !ACPI)

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Martin J. Bligh]

Andi Kleen wrote:
> On Tuesday 02 May 2006 16:02, Martin J. Bligh wrote:
> 
>>>Oh that's a 32bit kernel. I don't think the 32bit NUMA has ever worked
>>>anywhere but some Summit systems (at least every time I tried it it blew up 
>>>on me and nobody seems to use it regularly). Maybe it would be finally time to mark it 
>>>CONFIG_BROKEN though or just remove it (even by design it doesn't work very well) 
>>
>>Bollocks. It works fine, 
> 
> On what kind of box? Some summit system, right?

Summit and NUMA-Q, ie everything we originally created it for.

> Well, it doesn't work for Ingo clearly. My own experiences every time
> I tried it were similar.

What platform?

> I think I stand by my original statement.

If it works fine on some platforms and not on others, I would venture to
suggest it's a platform-specific issue, and marking the whole thing as
CONFIG_BROKEN would be an entirely inappropriate overreaction to what
is probably a simple bug.

M.

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Andi Kleen]

On Tuesday 02 May 2006 16:02, Martin J. Bligh wrote:
> > Oh that's a 32bit kernel. I don't think the 32bit NUMA has ever worked
> > anywhere but some Summit systems (at least every time I tried it it blew up 
> > on me and nobody seems to use it regularly). Maybe it would be finally time to mark it 
> > CONFIG_BROKEN though or just remove it (even by design it doesn't work very well) 
> 
> Bollocks. It works fine, 

On what kind of box? Some summit system, right?

> and is tested every single day, on every git 
> release, and every -mm tree.

Well, it doesn't work for Ingo clearly. My own experiences every time
I tried it were similar.

I think I stand by my original statement.

-Andi

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Nick Piggin]

Martin J. Bligh wrote:
>> Oh that's a 32bit kernel. I don't think the 32bit NUMA has ever worked
>> anywhere but some Summit systems (at least every time I tried it it 
>> blew up on me and nobody seems to use it regularly). Maybe it would be 
>> finally time to mark it CONFIG_BROKEN though or just remove it (even 
>> by design it doesn't work very well) 
> 
> 
> Bollocks. It works fine, and is tested every single day, on every git
> release, and every -mm tree.

Whatever the case, there definitely does not appear to be sufficient
zone alignment enforced for the buddy allocator. I cannot see how it
could work if zones are not aligned on 4MB boundaries.

Maybe some architectures / subarch code naturally does this for us,
but Ingo is definitely hitting this bug because his config does not
(align, that is).

I've randomly added a couple more cc's.

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Martin J. Bligh]

> Oh that's a 32bit kernel. I don't think the 32bit NUMA has ever worked
> anywhere but some Summit systems (at least every time I tried it it blew up 
> on me and nobody seems to use it regularly). Maybe it would be finally time to mark it 
> CONFIG_BROKEN though or just remove it (even by design it doesn't work very well) 

Bollocks. It works fine, and is tested every single day, on every git
release, and every -mm tree.

M.

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> On Tuesday 02 May 2006 09:06, Ingo Molnar wrote:
> > 
> > * Andi Kleen <ak@suse.de> wrote:
> > 
> > > Ingo Molnar <mingo@elte.hu> writes:
> > > 
> > > > FYI, even on 2.6.17-rc3 i get the one below. v2.6.17 showstopper i 
> > > > guess?
> > > 
> > > Did you send a full boot log?
> > 
> > yes, in the previous mail, in the same thread. (maybe lkml ate it - it's 
> > an allyesconfig bootup so a large bootlog and a large config) I've also 
> > uploaded them to:
> > 
> > 	http://redhat.com/~mingo/misc/
> > 
> > debug-pagealloc.patch is the debug patch i made based on Nick's earlier 
> > suggestions.
> > 
> > > If it's using ACPI NUMA try numa=noacpi - it might be some problem 
> > > with the node discovery on your machine.
> > 
> > this is a non-NUMA box (Athlon64 X2 desktop machine).
> 
> Oh that's a 32bit kernel. I don't think the 32bit NUMA has ever worked 
> anywhere but some Summit systems (at least every time I tried it it 
> blew up on me and nobody seems to use it regularly). Maybe it would be 
> finally time to mark it CONFIG_BROKEN though or just remove it (even 
> by design it doesn't work very well)

what you saw before could easily be this particular bug - the zones are 
apparently mis-sized (aligned to 1MB while they need to be aligned to 
4MB), which causes quick and nasty crashes under light user load.

	Ingo

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> Ingo Molnar <mingo@elte.hu> writes:
> 
> > FYI, even on 2.6.17-rc3 i get the one below. v2.6.17 showstopper i 
> > guess?
> 
> Did you send a full boot log?

yes, in the previous mail, in the same thread. (maybe lkml ate it - it's 
an allyesconfig bootup so a large bootlog and a large config) I've also 
uploaded them to:

	http://redhat.com/~mingo/misc/

debug-pagealloc.patch is the debug patch i made based on Nick's earlier 
suggestions.

> If it's using ACPI NUMA try numa=noacpi - it might be some problem 
> with the node discovery on your machine.

this is a non-NUMA box (Athlon64 X2 desktop machine).

	Ingo

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Andi Kleen]

On Tuesday 02 May 2006 09:06, Ingo Molnar wrote:
> 
> * Andi Kleen <ak@suse.de> wrote:
> 
> > Ingo Molnar <mingo@elte.hu> writes:
> > 
> > > FYI, even on 2.6.17-rc3 i get the one below. v2.6.17 showstopper i 
> > > guess?
> > 
> > Did you send a full boot log?
> 
> yes, in the previous mail, in the same thread. (maybe lkml ate it - it's 
> an allyesconfig bootup so a large bootlog and a large config) I've also 
> uploaded them to:
> 
> 	http://redhat.com/~mingo/misc/
> 
> debug-pagealloc.patch is the debug patch i made based on Nick's earlier 
> suggestions.
> 
> > If it's using ACPI NUMA try numa=noacpi - it might be some problem 
> > with the node discovery on your machine.
> 
> this is a non-NUMA box (Athlon64 X2 desktop machine).

Oh that's a 32bit kernel. I don't think the 32bit NUMA has ever worked
anywhere but some Summit systems (at least every time I tried it it blew up 
on me and nobody seems to use it regularly). Maybe it would be finally time to mark it 
CONFIG_BROKEN though or just remove it (even by design it doesn't work very well) 

If you want NUMA use 64bit.

-Andi

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Andi Kleen]

Ingo Molnar <mingo@elte.hu> writes:

> FYI, even on 2.6.17-rc3 i get the one below. v2.6.17 showstopper i 
> guess?

Did you send a full boot log?

If it's using ACPI NUMA try numa=noacpi - it might be some problem
with the node discovery on your machine.

-Andi

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

FYI, even on 2.6.17-rc3 i get the one below. v2.6.17 showstopper i 
guess?

	Ingo

zone c1e04600 (HighMem):
pfn: 00037d00
zone->zone_start_pfn: 00037e00
zone->spanned_pages: 00007e00
zone->zone_start_pfn + zone->spanned_pages: 0003fc00
------------[ cut here ]------------
kernel BUG at mm/page_alloc.c:526!
invalid opcode: 0000 [#1]
PREEMPT SMP 
Modules linked in:
CPU:    1
EIP:    0060:[<c0159183>]    Not tainted VLI
EFLAGS: 00010002   (2.6.17-rc3-lockdep #143) 
EIP is at __rmqueue+0x98/0xdd
eax: 00000001   ebx: c305d400   ecx: c012ac22   edx: 00000001
esi: c1e046dc   edi: 00000008   ebp: f4ea6bd0   esp: f4ea6bb4
ds: 007b   es: 007b   ss: 0068
Process chroot02 (pid: 9970, threadinfo=f4ea6000 task=f448e030)
Stack: <0>00000000 c1e04600 c3058000 00000100 c6d42e50 c6d42e5c 00000296 f4ea6c20 
       c01592ec 00000000 f448e608 00000003 c1e04908 00000000 000201d2 c1e04908 
       00000014 00000000 00000003 00000001 00000001 c1e04600 00000377 00000000 
Call Trace:
 [<c0104e5c>] show_stack_log_lvl+0x8b/0x95
 [<c0104ffc>] show_registers+0x147/0x1ad
 [<c010531b>] die+0x179/0x24e
 [<c0f48f9f>] do_trap+0x7c/0x96
 [<c0105768>] do_invalid_op+0x89/0x93
 [<c0104a03>] error_code+0x4f/0x54
 [<c01592ec>] get_page_from_freelist+0x124/0x436
 [<c015965e>] __alloc_pages+0x60/0x290
 [<c016ba71>] alloc_pages_current+0x77/0x7c
 [<c0154cec>] page_cache_alloc_cold+0x7f/0x83
 [<c015aa4a>] __do_page_cache_readahead+0x9e/0x1b2
 [<c015ac6d>] do_page_cache_readahead+0x40/0x4d
 [<c0155fa8>] filemap_nopage+0x149/0x30a
 [<c016141b>] __handle_mm_fault+0x3e9/0xb18
 [<c0f49e4d>] do_page_fault+0x317/0x725
 [<c0104a03>] error_code+0x4f/0x54
 [<c019f63d>] padzero+0x19/0x28
 [<c01a06fc>] load_elf_binary+0x904/0x15de
 [<c0181896>] search_binary_handler+0xcf/0x29e
 [<c0181bc0>] do_execve+0x15b/0x1fe
 [<c01029df>] sys_execve+0x2a/0x6d
 [<c0103ddb>] syscall_call+0x7/0xb
Code: 4d e8 29 01 88 d9 d3 e2 89 55 f0 eb 40 d1 6d f0 83 ee 0c 8b 5d ec 4f 6b 45 f0 54 01 c3 8b 45 e8 89 da e8 24 f8 ff ff 85 c0 74 08 <0f> 0b 0e 02 43 8f 04 c1 8b 16 8d 43 4c 89 42 04 89 53 4c 89 70 

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Nick Piggin]

On Fri, Apr 21, 2006 at 01:20:50PM +0200, Ingo Molnar wrote:
> 
> * Nick Piggin <npiggin@suse.de> wrote:
> 
> > It would be interesting to know which assertion failed. I guess it 
> > might be a zone alignment problem -- it would be interesting to turn 
> > the 2 HOLES_IN_ZONE tests into BUG_ONs, and enable them (ie. move them 
> > out of HOLES_IN_ZONE).
> 
> ok, i added a couple of printks (see the patch below), and got this:
> 
>  zone c1f0a600 (HighMem):
>  pfn: 00037d00
>  zone->zone_start_pfn: 00037e00
>  zone->spanned_pages: 00007e00
>  zone->zone_start_pfn + zone->spanned_pages: 0003fc00
>  ------------[ cut here ]------------
>  kernel BUG at mm/page_alloc.c:524!
> 
> so the pfn is 1MB below the zone's start address - not good. You can 
> find the full bootup log at:

The zones are 2MB aligned, discontig.c seems to do this. They should
be 4MB aligned so the page allocator's assumption that zones are 
contiguous to MAX_ORDER is satisfied.

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

disabling CONFIG_NUMA i get this one during bootup:

 Brought up 2 CPUs
 BUG: pfn: 0003fff0, page: c3308b00, order: 4
  [<c0104cd0>] show_trace+0xd/0xf
  [<c0104ce7>] dump_stack+0x15/0x17
  [<c0154a1c>] free_pages_bulk+0x155/0x246
  [<c0155673>] free_hot_cold_page+0x104/0x12c
  [<c01556da>] free_hot_page+0xa/0xc
  [<c0155706>] __free_pages+0x2a/0x35
  [<c01620ba>] __vunmap+0x95/0xc1
  [<c0162183>] vfree+0x29/0x2b
  [<c0125e46>] build_sched_domains+0xa70/0xc34
  [<c0126023>] arch_init_sched_domains+0x19/0x1b
  [<c1a1f799>] sched_init_smp+0x12/0x23
  [<c01003b2>] init+0xb9/0x2cb
  [<c0102005>] kernel_thread_helper+0x5/0xb

but otherwise no problems.

	Ingo

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Ingo Molnar]

* Nick Piggin <npiggin@suse.de> wrote:

> On Wed, Apr 19, 2006 at 01:21:30PM +0200, Ingo Molnar wrote:
> > the 2.6.17-rc2 kernel built with the attached config crashes quite 
> > easily under minimal load. Disabling CONFIG_NUMA makes it robust again.  
> > Crashlog attached.
> 
> It would be interesting to know which assertion failed. I guess it 
> might be a zone alignment problem -- it would be interesting to turn 
> the 2 HOLES_IN_ZONE tests into BUG_ONs, and enable them (ie. move them 
> out of HOLES_IN_ZONE).

ok, i added a couple of printks (see the patch below), and got this:

 zone c1f0a600 (HighMem):
 pfn: 00037d00
 zone->zone_start_pfn: 00037e00
 zone->spanned_pages: 00007e00
 zone->zone_start_pfn + zone->spanned_pages: 0003fc00
 ------------[ cut here ]------------
 kernel BUG at mm/page_alloc.c:524!

so the pfn is 1MB below the zone's start address - not good. You can 
find the full bootup log at:

	http://redhat.com/~mingo/misc/crash.log

the log should also give an idea about how the zones are layed out, etc.  
Note that this is an ordinary desktop dual-core box with 1GB RAM booting 
an allyesconfig NUMA kernel. I suspect this could be some bootup-time 
zone sizing problem? The config is at:

	http://redhat.com/~mingo/misc/config

(the crash is easy to reproduce, so let me know if i can do anything 
else to debug this.)

	Ingo

Index: linux/mm/page_alloc.c
===================================================================
--- linux.orig/mm/page_alloc.c
+++ linux/mm/page_alloc.c
@@ -100,17 +100,32 @@ static int page_outside_zone_boundaries(
 			ret = 1;
 	} while (zone_span_seqretry(zone, seq));
 
+#define P(x) printk("%s: %08lx\n", #x, x)
+
+	if (ret) {
+		printk("zone %p (%s):\n", zone, zone->name);
+		P(pfn);
+		P(zone->zone_start_pfn);
+		P(zone->spanned_pages);
+		P(zone->zone_start_pfn + zone->spanned_pages);
+	}
+
 	return ret;
 }
 
 static int page_is_consistent(struct zone *zone, struct page *page)
 {
-#ifdef CONFIG_HOLES_IN_ZONE
-	if (!pfn_valid(page_to_pfn(page)))
+	if (!pfn_valid(page_to_pfn(page))) {
+		printk("BUG: pfn: %08lx, page: %p\n",
+			page_to_pfn(page), page);
+		dump_stack();
 		return 0;
-#endif
-	if (zone != page_zone(page))
+	}
+	if (zone != page_zone(page)) {
+		printk("zone: %p != %p == page_zone(%p)\n",
+			zone, page_zone(page), page);
 		return 0;
+	}
 
 	return 1;
 }
@@ -292,10 +307,12 @@ __find_combined_index(unsigned long page
  */
 static inline int page_is_buddy(struct page *page, int order)
 {
-#ifdef CONFIG_HOLES_IN_ZONE
-	if (!pfn_valid(page_to_pfn(page)))
+	if (!pfn_valid(page_to_pfn(page))) {
+		printk("BUG: pfn: %08lx, page: %p, order: %d\n",
+			page_to_pfn(page), page, order);
+		dump_stack();
 		return 0;
-#endif
+	}
 
 	if (PageBuddy(page) && page_order(page) == order) {
 		BUG_ON(page_count(page) != 0);

Re: assert/crash in __rmqueue() when enabling CONFIG_NUMA

[Nick Piggin]

On Wed, Apr 19, 2006 at 01:21:30PM +0200, Ingo Molnar wrote:
> the 2.6.17-rc2 kernel built with the attached config crashes quite 
> easily under minimal load. Disabling CONFIG_NUMA makes it robust again.  
> Crashlog attached.

It would be interesting to know which assertion failed. I
guess it might be a zone alignment problem -- it would be
interesting to turn the 2 HOLES_IN_ZONE tests into BUG_ONs,
and enable them (ie. move them out of HOLES_IN_ZONE).

It might be an idea to unconditionally enable DEBUG_VM in
prereleases, too.

Thanks,
Nick