Re: [PATCH 1/3] hugetlbfs: architecture header cleanup

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 02 Apr 2008 16:26:55 +0200
Gerald Schaefer <gerald.schaefer@de.ibm.com> wrote:

> Subject: [PATCH 1/3] hugetlbfs: architecture header cleanup
> 
> From: Gerald Schaefer <gerald.schaefer@de.ibm.com>
> 
> This patch moves all architecture functions for hugetlb to architecture
> header files (include/asm-foo/hugetlb.h). It also removes (!)
> ARCH_HAS_HUGEPAGE_ONLY_RANGE, ARCH_HAS_HUGETLB_FREE_PGD_RANGE,
> ARCH_HAS_PREPARE_HUGEPAGE_RANGE, ARCH_HAS_SETCLEAR_HUGE_PTE and
> ARCH_HAS_HUGETLB_PREFAULT_HOOK.
> 
> Cross-Compile tests on the affected architectures (and one unaffected)
> worked fine, but I had no cross-compiler for sh.
> 

This text explains what the patch does but not why it does it.  Always provide
both, please.  That way people will stop asking why this:

> 
>  include/asm-ia64/hugetlb.h    |   21 +++++++++++++++++++
>  include/asm-ia64/page.h       |    6 -----
>  include/asm-powerpc/hugetlb.h |   35 +++++++++++++++++++++++++++++++
>  include/asm-powerpc/page_64.h |    7 ------
>  include/asm-sh/hugetlb.h      |   28 +++++++++++++++++++++++++
>  include/asm-sparc64/hugetlb.h |   30 +++++++++++++++++++++++++++
>  include/asm-sparc64/page.h    |    2 -
>  include/asm-x86/hugetlb.h     |   28 +++++++++++++++++++++++++
>  include/linux/hugetlb.h       |   46 ------------------------------------------
>  9 files changed, 143 insertions(+), 60 deletions(-)

is the way it is.

> Index: linux-2.6.25-rc7/include/asm-ia64/hugetlb.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.25-rc7/include/asm-ia64/hugetlb.h
> @@ -0,0 +1,21 @@
> +#ifndef _ASM_IA64_HUGETLB_H
> +#define _ASM_IA64_HUGETLB_H
> +
> +#include <asm/page.h>
> +
> +
> +#define is_hugepage_only_range(mm, addr, len)		\
> +	(REGION_NUMBER(addr) == RGN_HPAGE ||	\
> +	 REGION_NUMBER((addr)+(len)-1) == RGN_HPAGE)
> +
> +void hugetlb_free_pgd_range(struct mmu_gather **tlb, unsigned long addr,
> +			    unsigned long end, unsigned long floor,
> +			    unsigned long ceiling);
> +int prepare_hugepage_range(unsigned long addr, unsigned long len);
> +
> +#define set_huge_pte_at(mm, addr, ptep, pte)	set_pte_at(mm, addr, ptep, pte)
> +#define huge_ptep_get_and_clear(mm, addr, ptep) ptep_get_and_clear(mm, addr, ptep)
> +
> +#define hugetlb_prefault_arch_hook(mm)		do { } while (0)
> +
> +#endif /* _ASM_IA64_HUGETLB_H */
> Index: linux-2.6.25-rc7/include/asm-sh/hugetlb.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.25-rc7/include/asm-sh/hugetlb.h
> @@ -0,0 +1,28 @@
> +#ifndef _ASM_SH_HUGETLB_H
> +#define _ASM_SH_HUGETLB_H
> +
> +#include <asm/page.h>
> +
> +
> +#define is_hugepage_only_range(mm, addr, len)	0
> +#define hugetlb_free_pgd_range			free_pgd_range
> +
> +/*
> + * If the arch doesn't supply something else, assume that hugepage
> + * size aligned regions are ok without further preparation.
> + */
> +static inline int prepare_hugepage_range(unsigned long addr, unsigned long len)
> +{
> +	if (len & ~HPAGE_MASK)
> +		return -EINVAL;
> +	if (addr & ~HPAGE_MASK)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +#define set_huge_pte_at(mm, addr, ptep, pte)	set_pte_at(mm, addr, ptep, pte)
> +#define huge_ptep_get_and_clear(mm, addr, ptep) ptep_get_and_clear(mm, addr, ptep)
> +
> +#define hugetlb_prefault_arch_hook(mm)		do { } while (0)

urgh.  I wouldn't say the result of this "cleanup" is very clean. 
Basically all the macros there should be zapped and turned into inlines. 
That will happen to fix the bug in ia64's is_hugepage_only_range(), which
presently references its arg once or twice, and will do bad things if
called with an expression-with-side-effects.

> +
> +#endif /* _ASM_SH_HUGETLB_H */
> Index: linux-2.6.25-rc7/include/asm-sparc64/hugetlb.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.25-rc7/include/asm-sparc64/hugetlb.h
> @@ -0,0 +1,30 @@
> +#ifndef _ASM_SPARC64_HUGETLB_H
> +#define _ASM_SPARC64_HUGETLB_H
> +
> +#include <asm/page.h>
> +
> +
> +#define is_hugepage_only_range(mm, addr, len)	0

This is bad too, because it can lead to unused-var warnings if compiled
with suitable config combinations.

So basically the existing hugetlb arch support code needs a big de-macro
crapectomy.  But your patch has gone and massively duplicated the existing
crap, making that decrapectomy larger and harder.