Messages in this thread | | | Date | Wed, 16 Nov 2011 17:00:30 -0800 | From | David Daney <> | Subject | Re: [PATCH] hugetlb: Provide a default HPAGE_SHIFT if !CONFIG_HUGETLB_PAGE |
| |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |