Re: [PATCH] hugetlb: Provide a default HPAGE_SHIFT if!CONFIG_HUGETLB_PAGE

[David Rientjes]

On Wed, 16 Nov 2011, David Daney wrote:

> > > > > This is required now to get MIPS kernels to compile with
> > > > > !CONFIG_HUGETLB_PAGE.
> > > > > 
> > > > 
> > > > Why?
> > > 
> > > I should have been more specific.  The failure is in Ralf's
> > > mips-for-linux-next branch.
> > > 
> > 
> > I can't find that branch (it's not in Ralf's tree at git.kernel.org), so
> > I'm looking at next-20111116.  It doesn't compile for mips for other
> > reasons related to arch/mips/sgi-ip22/ip22-gio.c.
> 
> Ralf's various trees are accessible via linux-mips.org
> 

Wow, we've certainly gone a long way from a patch that appears to be 
fixing a problem in Linus' tree based on your commit message.  You're 
working from ralf/upstream-sfr.git#mips-for-linux-next from 
git.linux-mips.org.

Might want to have mentioned that for a patch labeled "hugetlb".

> > Which is wrong.  MIPS code should not be using HPAGE_SHIFT without
> > CONFIG_HUGETLB_PAGE and in fact defines it itself for such a configuration
> > in arch/mips/include/asm/page.h.  The only generic uses are in
> > page_alloc.c where we need CONFIG_HUGETLB_PAGE_SIZE_VARIABLE, which isn't
> > available on mips, and in mm/hugetlb.c which requires CONFIG_HUGETLB_PAGE
> > by way of CONFIG_HUGETLBFS.
> > 
> > So feel free to show the actual compile error this time and I'll suggest a
> > mips fix for it.
> 
> arch/mips/mm/tlb-r4k.c: In function âlocal_flush_tlb_rangeâ:
> arch/mips/mm/tlb-r4k.c:129:28: error: âHPAGE_SHIFTâ undeclared (first use in
> this function)
> 

Do you see where that file has #ifdef's for CONFIG_HUGETLB_PAGE?  You need 
them here too.  The problem is that is_vm_hugetlb_page() is not #define to 
0 for CONFIG_HUGETLB_PAGE=n so the clauses using HPAGE_* aren't compiled 
out like the author is expecting.

> However, I call B.S. on your reasoning.
> 
> It is a well established kernel idiom to supply dummy values for symbols that
> are required to be defined in order to form a syntactically correct C program,
> but that are known by the programmer to be used only on dead code paths.
> 
> This is exactly what we are doing here.
> 
> To do otherwise requires that code be cluttered with #ifdefery.
> 

You're wrong, nobody should be using HPAGE_SHIFT unless they are working 
with hugepages and, in fact, that code that placed those "dummy values" 
for HPAGE_MASK and HPAGE_SIZE there has since been removed since 2005.

Defining HPAGE_SHIFT to be PAGE_SHIFT is just stupid and doing so may 
allow the program to compile but will hide real bugs later on.  In 
fact if you merged your patch, it would be a bug since the vma would have 
VM_HUGETLB but you'd now be operating on PAGE_SIZE pages!

Like I said, we should be removing those existing definitions of 
HPAGE_MASK and HPAGE_SIZE rather than leaving them so someone like you can 
come along and pretend there's any legitimacy to them whatsoever and 
extend the insanity.

You may not realize it, but changes to include/linux/hugetlb.h go through 
Andrew; Ralf won't be merging anything into this generic header file 
because of a mips problem.