Re: [PATCH 1/4] hugetlbfs: add arch_hugetlb_valid_size

From: Dave Hansen
Date: Wed Mar 18 2020 - 18:15:48 EST


Hi Mike,

The series looks like a great idea to me. One nit on the x86 bits,
though...

> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> index 5bfd5aef5378..51e6208fdeec 100644
> --- a/arch/x86/mm/hugetlbpage.c
> +++ b/arch/x86/mm/hugetlbpage.c
> @@ -181,16 +181,25 @@ hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
> #endif /* CONFIG_HUGETLB_PAGE */
>
> #ifdef CONFIG_X86_64
> +bool __init arch_hugetlb_valid_size(unsigned long long size)
> +{
> + if (size == PMD_SIZE)
> + return true;
> + else if (size == PUD_SIZE && boot_cpu_has(X86_FEATURE_GBPAGES))
> + return true;
> + else
> + return false;
> +}

I'm pretty sure it's possible to have a system without 2M/PMD page
support. We even have a handy-dandy comment about it in
arch/x86/include/asm/required-features.h:

#ifdef CONFIG_X86_64
#ifdef CONFIG_PARAVIRT
/* Paravirtualized systems may not have PSE or PGE available */
#define NEED_PSE 0
...

I *think* you need an X86_FEATURE_PSE check here to be totally correct.

if (size == PMD_SIZE && cpu_feature_enabled(X86_FEATURE_PSE))
return true;

BTW, I prefer cpu_feature_enabled() to boot_cpu_has() because it
includes disabled-features checking. I don't think any of it matters
for these specific features, but I generally prefer it on principle.