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

[David Daney]

On 11/16/2011 04:08 PM, David Rientjes wrote:
> 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.

Yes, as I already said, I should have been more specific about this.

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

Yes I see that.  With git annotate we can even see who wrote it.

The code would be much cleaner if that #ifdef were removed.  The author 
was unaware that a dummy version of pmd_huge() existed in hugetlb.h, 
making the entire if(pmd_huge()) huge block dead code.

I will prepare a patch that gets rid of the #ifdef CONFIG_HUGETLB_PAGE 
in that file.

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

Ok, then please tell me why:

1) the dummy version of is_vm_hugetlb_page() exists in hugetlb_inline.h?
2) the dummy version of PageHuge() exists in hugetlb.h
3) the dummy version of reset_vma_resv_huge_pages()
4) the dummy version of hugetlb_total_pages() exists in hugetlb.h
5) the dummy version of follow_hugetlb_page() exists in hugetlb.h
6) the dummy version of follow_huge_addr() exists in hugetlb.h
7) the dummy version of copy_hugetlb_page_range() exists in hugetlb.h
8) the dummy version of hugetlb_prefault() exists in hugetlb.h
9) the dummy version of unmap_hugepage_range() exists in hugetlb.h
10) the dummy version of hugetlb_report_meminfo() exists in hugetlb.h
11) the dummy version of hugetlb_report_node_meminfo() exists in hugetlb.h
12) the dummy version of follow_huge_pmd() exists in hugetlb.h
13) the dummy version of follow_huge_pud() exists in hugetlb.h
14) the dummy version of prepare_hugepage_range() exists in hugetlb.h
15) the dummy version of pmd_huge() exists in hugetlb.h
16) the dummy version of pud_huge() exists in hugetlb.h
17) the dummy version of is_hugepage_only_range() exists in hugetlb.h
17) the dummy version of hugetlb_free_pgd_range() exists in hugetlb.h
18) the dummy version of hugetlb_fault() exists in hugetlb.h
19) the dummy version of huge_pte_offset() exists in hugetlb.h
20) the dummy version of dequeue_hwpoisoned_huge_page() exists in hugetlb.h
21) the dummy version of copy_huge_page() exists in hugetlb.h
22) the dummy version of hugetlb_change_protection() exists in hugetlb.h
.
.
.
[Other non HUGETLB_PAGE examples omitted for conciseness]

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

Indeed, you may not realize that akpm has been on the to or cc list for 
this entire thread because I explicitly put him there for this exact reason.

Thanks,
David Daney

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