Re: [PATCH] Fix boot problem with iSeries lacking hugepage support

[Mel Gorman]

On (15/11/07 02:39), Andrew Morton didst pronounce:
> On Thu, 15 Nov 2007 10:13:22 +0000 mel@xxxxxxxxx (Mel Gorman) wrote:
> 
> > This patch is a fix for 2.6.24.
> > 
> > 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 becomes 0. This results in pageblock_order
> > being set to -PAGE_SHIFT and a crash results shortly afterwards.
> > 
> > This patch sets HUGETLB_PAGE_SIZE_VARIABLE for PPC64 and adds a function
> > to select a sensible value for pageblock order by default.  It checks that
> > HPAGE_SHIFT is a sensible value before using the hugepage size; if it is
> > not MAX_ORDER-1 is used.
> > 
> > Credit goes to Stephen Rothwell for identifying the bug and testing candidate
> > patches.  Additional credit goes to Andy Whitcroft for spotting a problem
> > with respects to IA-64 before releasing. Additional credit to David Gibson
> > for testing with the libhugetlbfs test suite.
> > 
> > Signed-off-by: Mel Gorman <mel@xxxxxxxxx>
> > Tested-by: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx>
> > 
> > --- 
> >  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)
> 
> Shouldn't this have been HUGETLB_PAGE_ORDER?
> 

As a #define, possibly but as a static inline - definitly not.

In this context, the define is not used because set_pageblock_order()
is a no-op when CONFIG_HUGETLB_PAGE_SIZE_VARIABLE is unset.
pageblock_default_order() is only defined for symmetry as set_pageblock_order()
is defined in both contexts. However, as a #define it might make more sense
to a casual reader to see HUGETLB_PAGE_ORDER even if it has no effect. I
can send a version of the patch that does this with a comment explaining
what is going on with set_pageblock_order() if you like.

However, in a follow-up fix, you make pageblock_default_order() a static
inline. If it tries to return HUGETLB_PAGE_ORDER, it will fail to compile
when CONFIG_HUGETLB_PAGE is not set.

Which would you prefer?

> >  #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
> 

-- 
-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/