* Re: Do not depend on MAX_ORDER when grouping pages by mobility
@ 2007-11-12 2:21 Stephen Rothwell
2007-11-12 15:54 ` Mel Gorman
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Rothwell @ 2007-11-12 2:21 UTC (permalink / raw)
To: Mel Gorman; +Cc: ppc-dev
[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]
Hi Mel,
I discovered recently that a kernel built with ppc64_defconfig no longer
boots on legacy iSeries. It did in 2.6.23. I bisected down the commit
d9c2340052278d8eb2ffb16b0484f8f794def4de ("Do not depend on MAX_ORDER
when grouping pages by mobility") which fails while its parent is ok.
Also, an iseries_defconfig kernel will boot. The reason it seem is
because on PowerPC 64 with CONFIG_HUGETLB_PAGE, HPAGE_SHIFT is not
constant and its value is determined at runtime early.
For legacy iSeries HPAGE_SHIFT remains 0 which means that
HUGETLB_PAGE_ORDER becomes -PAGE_SHIFT and things degenerate badly.
I can enable CONFIG_HUGETLB_PAGE_SIZE_VARIABLE for PowerPC 64, but I
still need to know a good value for HPAGE_SHIFT. Do you have a
suggestion? Is there a better way to fix this problem? There are places
in the PowerPC code that assume that HPAGE_SHIFT == 0 means that we have
no huge pages.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Do not depend on MAX_ORDER when grouping pages by mobility 2007-11-12 2:21 Do not depend on MAX_ORDER when grouping pages by mobility Stephen Rothwell @ 2007-11-12 15:54 ` Mel Gorman 2007-11-13 0:44 ` Stephen Rothwell 0 siblings, 1 reply; 6+ messages in thread From: Mel Gorman @ 2007-11-12 15:54 UTC (permalink / raw) To: Stephen Rothwell; +Cc: ppc-dev On (12/11/07 13:21), Stephen Rothwell didst pronounce: > I discovered recently that a kernel built with ppc64_defconfig no longer > boots on legacy iSeries. It did in 2.6.23. I bisected down the commit > d9c2340052278d8eb2ffb16b0484f8f794def4de ("Do not depend on MAX_ORDER > when grouping pages by mobility") which fails while its parent is ok. > Also, an iseries_defconfig kernel will boot. The reason it seem is > because on PowerPC 64 with CONFIG_HUGETLB_PAGE, HPAGE_SHIFT is not > constant and its value is determined at runtime early. > Ok, that in itself is ok. IA-64 does something similar. > For legacy iSeries HPAGE_SHIFT remains 0 which means that > HUGETLB_PAGE_ORDER becomes -PAGE_SHIFT and things degenerate badly. > D'oh. That would have problems for sure. > I can enable CONFIG_HUGETLB_PAGE_SIZE_VARIABLE for PowerPC 64, but I > still need to know a good value for HPAGE_SHIFT. Do you have a > suggestion? Is there a better way to fix this problem? There are places > in the PowerPC code that assume that HPAGE_SHIFT == 0 means that we have > no huge pages. > How about the following both in terms of taste and whether it works or not? === Ordinarily, the size of a pageblock is determined from the hugepage size. On PPC64, the hugepage size is determined at runtime based on the ability of the machine. If the machine does not support hugepages, HPAGE_SHIFT is 0. This results in pageblock_order being set to -PAGE_SHIFT and a crash results shortly afterwards. This patch checks that HPAGE_SHIFT is a sensible value before using the hugepage size. If it is 0, MAX_ORDER-1 is used instead as this is a sensible value of pageblock_order. Signed-off-by: Mel Gorman <mel@csn.ul.ie> --- diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 18f397c..232c298 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -187,6 +187,11 @@ config FORCE_MAX_ZONEORDER default "9" if PPC_64K_PAGES default "13" +config HUGETLB_PAGE_SIZE_VARIABLE + bool + depends on HUGETLB_PAGE + default y + config MATH_EMULATION bool "Math emulation" depends on 4xx || 8xx || E200 || PPC_MPC832x || E500 diff --git a/mm/page_alloc.c b/mm/page_alloc.c index da69d83..14e0ac3 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3386,7 +3386,16 @@ static void __meminit free_area_init_core(struct pglist_data *pgdat, if (!size) continue; - set_pageblock_order(HUGETLB_PAGE_ORDER); + /* + * If HPAGE_SHIFT is a sensible value, base the size of a + * pageblock on the hugepage size. Otherwise MAX_ORDER-1 + * is a sensible choice + */ + if (HPAGE_SHIFT > PAGE_SHIFT) + set_pageblock_order(HUGETLB_PAGE_ORDER); + else + set_pageblock_order(MAX_ORDER-1); + setup_usemap(pgdat, zone, size); ret = init_currently_empty_zone(zone, zone_start_pfn, size, MEMMAP_EARLY); -- Mel Gorman Part-time Phd Student Linux Technology Center University of Limerick IBM Dublin Software Lab ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Do not depend on MAX_ORDER when grouping pages by mobility 2007-11-12 15:54 ` Mel Gorman @ 2007-11-13 0:44 ` Stephen Rothwell 2007-11-14 18:10 ` Mel Gorman 0 siblings, 1 reply; 6+ messages in thread From: Stephen Rothwell @ 2007-11-13 0:44 UTC (permalink / raw) To: Mel Gorman; +Cc: ppc-dev [-- Attachment #1: Type: text/plain, Size: 1121 bytes --] On Mon, 12 Nov 2007 15:54:53 +0000 mel@skynet.ie (Mel Gorman) wrote: > > Ordinarily, the size of a pageblock is determined from the hugepage size. > On PPC64, the hugepage size is determined at runtime based on the ability > of the machine. If the machine does not support hugepages, HPAGE_SHIFT is > 0. This results in pageblock_order being set to -PAGE_SHIFT and a crash > results shortly afterwards. > > This patch checks that HPAGE_SHIFT is a sensible value before using the > hugepage size. If it is 0, MAX_ORDER-1 is used instead as this is a sensible > value of pageblock_order. > > Signed-off-by: Mel Gorman <mel@csn.ul.ie> Looks good. Legacy iSeries boots fine with this and David Gibson has run his libhugetlbfs test suite on a Power5+ machine also running the same kernel (ppc64_defconfig). I would be good if we could get this in for 2.6.24 (since, as far as legacy iSeries is concerned, this is a regression from 2.6.23). I am not sure what other testing needs to be done. -- Cheers, Stephen Rothwell sfr@canb.auug.org.au http://www.canb.auug.org.au/~sfr/ [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Do not depend on MAX_ORDER when grouping pages by mobility 2007-11-13 0:44 ` Stephen Rothwell @ 2007-11-14 18:10 ` Mel Gorman 2007-11-15 3:17 ` Stephen Rothwell 0 siblings, 1 reply; 6+ messages in thread From: Mel Gorman @ 2007-11-14 18:10 UTC (permalink / raw) To: Stephen Rothwell; +Cc: ppc-dev On (13/11/07 11:44), Stephen Rothwell didst pronounce: > On Mon, 12 Nov 2007 15:54:53 +0000 mel@skynet.ie (Mel Gorman) wrote: > > > > Ordinarily, the size of a pageblock is determined from the hugepage size. > > On PPC64, the hugepage size is determined at runtime based on the ability > > of the machine. If the machine does not support hugepages, HPAGE_SHIFT is > > 0. This results in pageblock_order being set to -PAGE_SHIFT and a crash > > results shortly afterwards. > > > > This patch checks that HPAGE_SHIFT is a sensible value before using the > > hugepage size. If it is 0, MAX_ORDER-1 is used instead as this is a sensible > > value of pageblock_order. > > > > Signed-off-by: Mel Gorman <mel@csn.ul.ie> > > Looks good. Legacy iSeries boots fine with this and David Gibson has run > his libhugetlbfs test suite on a Power5+ machine also running the same > kernel (ppc64_defconfig). > > I would be good if we could get this in for 2.6.24 (since, as far as > legacy iSeries is concerned, this is a regression from 2.6.23). I am not > sure what other testing needs to be done. > libhugetlbfs test suite and boot test on iSeries is sufficient in this case. However, the version I sent would break on IA-64 due to the lack of a definition for HPAGE_SHIFT when CONFIG_HUGETLB_PAGE is not set. Can you confirm this patch still fixes the problem please? If it does, I'll send it to Andrew as a fix for 2.6.24. Whether iSeries is legacy or not, this is breakage and should be fixed. Thanks ==== Ordinarily the size of a pageblock is determined at compile-time based on the hugepage size. On PPC64, the hugepage size is determined at runtime based on what is supported by the machine. With legacy machines such as iSeries that do not support hugepages, HPAGE_SHIFT is 0. This results in pageblock_order being set to -PAGE_SHIFT and a crash results shortly afterwards. This patch adds a function to select a sensible value for pageblock order by default when HUGETLB_PAGE_SIZE_VARIABLE is set. It checks that HPAGE_SHIFT is a sensible value before using the hugepage size; if it is not MAX_ORDER-1 is used. This is a fix for 2.6.24. Credit goes to Stephen Rothwell for identifying the bug and testing on iSeries. Additional credit goes to Andy Whitcroft for spotting a problem with respects to IA-64 before releasing. Additional credit goes to David Gibson for testing with the libhugetlbfs test suite. Signed-off-by: Mel Gorman <mel@csn.ul.ie> --- arch/powerpc/Kconfig | 5 +++++ mm/page_alloc.c | 14 ++++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.24-rc2-mm1-clean/arch/powerpc/Kconfig linux-2.6.24-rc2-005_iSeries_fix/arch/powerpc/Kconfig --- linux-2.6.24-rc2-mm1-clean/arch/powerpc/Kconfig 2007-11-14 11:38:05.000000000 +0000 +++ linux-2.6.24-rc2-005_iSeries_fix/arch/powerpc/Kconfig 2007-11-14 11:39:12.000000000 +0000 @@ -187,6 +187,11 @@ config FORCE_MAX_ZONEORDER default "9" if PPC_64K_PAGES default "13" +config HUGETLB_PAGE_SIZE_VARIABLE + bool + depends on HUGETLB_PAGE + default y + config MATH_EMULATION bool "Math emulation" depends on 4xx || 8xx || E200 || PPC_MPC832x || E500 diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.24-rc2-mm1-clean/mm/page_alloc.c linux-2.6.24-rc2-005_iSeries_fix/mm/page_alloc.c --- linux-2.6.24-rc2-mm1-clean/mm/page_alloc.c 2007-11-14 11:38:08.000000000 +0000 +++ linux-2.6.24-rc2-005_iSeries_fix/mm/page_alloc.c 2007-11-14 13:45:19.000000000 +0000 @@ -3342,6 +3342,16 @@ static void inline setup_usemap(struct p #endif /* CONFIG_SPARSEMEM */ #ifdef CONFIG_HUGETLB_PAGE_SIZE_VARIABLE + +/* Return a sensible default order for the pageblock size. */ +static inline int __init pageblock_default_order(void) +{ + if (HPAGE_SHIFT > PAGE_SHIFT) + return HUGETLB_PAGE_ORDER; + + return MAX_ORDER-1; +} + /* Initialise the number of pages represented by NR_PAGEBLOCK_BITS */ static inline void __init set_pageblock_order(unsigned int order) { @@ -3357,7 +3367,7 @@ static inline void __init set_pageblock_ } #else /* CONFIG_HUGETLB_PAGE_SIZE_VARIABLE */ -/* Defined this way to avoid accidently referencing HUGETLB_PAGE_ORDER */ +#define pageblock_default_order(x) (0) #define set_pageblock_order(x) do {} while (0) #endif /* CONFIG_HUGETLB_PAGE_SIZE_VARIABLE */ @@ -3442,7 +3452,7 @@ static void __meminit free_area_init_cor if (!size) continue; - set_pageblock_order(HUGETLB_PAGE_ORDER); + set_pageblock_order(pageblock_default_order()); setup_usemap(pgdat, zone, size); ret = init_currently_empty_zone(zone, zone_start_pfn, size, MEMMAP_EARLY); -- Mel Gorman Part-time Phd Student Linux Technology Center University of Limerick IBM Dublin Software Lab ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Do not depend on MAX_ORDER when grouping pages by mobility 2007-11-14 18:10 ` Mel Gorman @ 2007-11-15 3:17 ` Stephen Rothwell 2007-11-15 10:13 ` Mel Gorman 0 siblings, 1 reply; 6+ messages in thread From: Stephen Rothwell @ 2007-11-15 3:17 UTC (permalink / raw) To: Mel Gorman; +Cc: ppc-dev [-- Attachment #1: Type: text/plain, Size: 858 bytes --] Hi Mel, On Wed, 14 Nov 2007 18:10:45 +0000 mel@skynet.ie (Mel Gorman) wrote: > > libhugetlbfs test suite and boot test on iSeries is sufficient in this > case. However, the version I sent would break on IA-64 due to the lack of > a definition for HPAGE_SHIFT when CONFIG_HUGETLB_PAGE is not set. Can you > confirm this patch still fixes the problem please? If it does, I'll send > it to Andrew as a fix for 2.6.24. Whether iSeries is legacy or not, this is > breakage and should be fixed. The new patch works fine. I reran the libhugetlbfs tests on a Power5+ machine and the ppc64_defconfig boots on legacy iSeries. So Tested-by: Stephen Rothwell <sfr@canb.auug.org.au> iSeries boot test and hugetlb tests on PPC64 Thanks. -- Cheers, Stephen Rothwell sfr@canb.auug.org.au http://www.canb.auug.org.au/~sfr/ [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Do not depend on MAX_ORDER when grouping pages by mobility 2007-11-15 3:17 ` Stephen Rothwell @ 2007-11-15 10:13 ` Mel Gorman 0 siblings, 0 replies; 6+ messages in thread From: Mel Gorman @ 2007-11-15 10:13 UTC (permalink / raw) To: Stephen Rothwell; +Cc: ppc-dev On (15/11/07 14:17), Stephen Rothwell didst pronounce: > Hi Mel, > > On Wed, 14 Nov 2007 18:10:45 +0000 mel@skynet.ie (Mel Gorman) wrote: > > > > libhugetlbfs test suite and boot test on iSeries is sufficient in this > > case. However, the version I sent would break on IA-64 due to the lack of > > a definition for HPAGE_SHIFT when CONFIG_HUGETLB_PAGE is not set. Can you > > confirm this patch still fixes the problem please? If it does, I'll send > > it to Andrew as a fix for 2.6.24. Whether iSeries is legacy or not, this is > > breakage and should be fixed. > > The new patch works fine. I reran the libhugetlbfs tests on a Power5+ > machine and the ppc64_defconfig boots on legacy iSeries. > Thanks a million for reporting and testing. I've pushed the patch to Andrew. -- Mel Gorman Part-time Phd Student Linux Technology Center University of Limerick IBM Dublin Software Lab ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-11-15 10:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-11-12 2:21 Do not depend on MAX_ORDER when grouping pages by mobility Stephen Rothwell 2007-11-12 15:54 ` Mel Gorman 2007-11-13 0:44 ` Stephen Rothwell 2007-11-14 18:10 ` Mel Gorman 2007-11-15 3:17 ` Stephen Rothwell 2007-11-15 10:13 ` Mel Gorman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).