Messages in this thread Patch in this message | | | Subject | Re: [PATCH 1/4] hugetlbfs: add arch_hugetlb_valid_size | From | Christophe Leroy <> | Date | Thu, 19 Mar 2020 08:00:59 +0100 |
| |
Le 18/03/2020 à 23:06, Mike Kravetz a écrit : > The architecture independent routine hugetlb_default_setup sets up > the default huge pages size. It has no way to verify if the passed > value is valid, so it accepts it and attempts to validate at a later > time. This requires undocumented cooperation between the arch specific > and arch independent code. > > For architectures that support more than one huge page size, provide > a routine arch_hugetlb_valid_size to validate a huge page size. > hugetlb_default_setup can use this to validate passed values. > > arch_hugetlb_valid_size will also be used in a subsequent patch to > move processing of the "hugepagesz=" in arch specific code to a common > routine in arch independent code. > > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> > --- > arch/arm64/include/asm/hugetlb.h | 2 ++ > arch/arm64/mm/hugetlbpage.c | 19 ++++++++++++++----- > arch/powerpc/include/asm/hugetlb.h | 3 +++ > arch/powerpc/mm/hugetlbpage.c | 20 +++++++++++++------- > arch/riscv/include/asm/hugetlb.h | 3 +++ > arch/riscv/mm/hugetlbpage.c | 28 ++++++++++++++++++---------- > arch/s390/include/asm/hugetlb.h | 3 +++ > arch/s390/mm/hugetlbpage.c | 18 +++++++++++++----- > arch/sparc/include/asm/hugetlb.h | 3 +++ > arch/sparc/mm/init_64.c | 23 ++++++++++++++++------- > arch/x86/include/asm/hugetlb.h | 3 +++ > arch/x86/mm/hugetlbpage.c | 21 +++++++++++++++------ > include/linux/hugetlb.h | 7 +++++++ > mm/hugetlb.c | 16 +++++++++++++--- > 14 files changed, 126 insertions(+), 43 deletions(-) >
[snip]
> diff --git a/arch/powerpc/include/asm/hugetlb.h b/arch/powerpc/include/asm/hugetlb.h > index bd6504c28c2f..3b5939016955 100644 > --- a/arch/powerpc/include/asm/hugetlb.h > +++ b/arch/powerpc/include/asm/hugetlb.h > @@ -64,6 +64,9 @@ static inline void arch_clear_hugepage_flags(struct page *page) > { > } > > +#define arch_hugetlb_valid_size arch_hugetlb_valid_size > +extern bool __init arch_hugetlb_valid_size(unsigned long long size);
Don't add 'extern' keyword, it is irrelevant for a function declaration.
checkpatch --strict doesn't like it either (https://openpower.xyz/job/snowpatch/job/snowpatch-linux-checkpatch/12318//artifact/linux/checkpatch.log)
> + > #include <asm-generic/hugetlb.h> > > #else /* ! CONFIG_HUGETLB_PAGE */ > diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c > index 33b3461d91e8..b78f660252f3 100644 > --- a/arch/powerpc/mm/hugetlbpage.c > +++ b/arch/powerpc/mm/hugetlbpage.c > @@ -558,7 +558,7 @@ unsigned long vma_mmu_pagesize(struct vm_area_struct *vma) > return vma_kernel_pagesize(vma); > } > > -static int __init add_huge_page_size(unsigned long long size) > +bool __init arch_hugetlb_valid_size(unsigned long long size) > { > int shift = __ffs(size); > int mmu_psize; > @@ -566,20 +566,26 @@ static int __init add_huge_page_size(unsigned long long size) > /* Check that it is a page size supported by the hardware and > * that it fits within pagetable and slice limits. */ > if (size <= PAGE_SIZE || !is_power_of_2(size)) > - return -EINVAL; > + return false; > > mmu_psize = check_and_get_huge_psize(shift); > if (mmu_psize < 0) > - return -EINVAL; > + return false; > > BUG_ON(mmu_psize_defs[mmu_psize].shift != shift); > > - /* Return if huge page size has already been setup */ > - if (size_to_hstate(size)) > - return 0; > + return true; > +} > > - hugetlb_add_hstate(shift - PAGE_SHIFT); > +static int __init add_huge_page_size(unsigned long long size) > +{ > + int shift = __ffs(size); > + > + if (!arch_hugetlb_valid_size(size)) > + return -EINVAL; > > + if (!size_to_hstate(size)) > + hugetlb_add_hstate(shift - PAGE_SHIFT); > return 0; > } >
[snip]
> 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; > +} > + > static __init int setup_hugepagesz(char *opt) > { > - unsigned long ps = memparse(opt, &opt); > - if (ps == PMD_SIZE) { > - hugetlb_add_hstate(PMD_SHIFT - PAGE_SHIFT); > - } else if (ps == PUD_SIZE && boot_cpu_has(X86_FEATURE_GBPAGES)) { > - hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT); > + unsigned long long ps = memparse(opt, &opt); > + > + if (arch_hugetlb_valid_size(ps)) { > + hugetlb_add_hstate(ilog2(ps) - PAGE_SHIFT); > } else { > hugetlb_bad_size(); > - printk(KERN_ERR "hugepagesz: Unsupported page size %lu M\n", > + printk(KERN_ERR "hugepagesz: Unsupported page size %llu M\n", > ps >> 20);
Nowadays we use pr_err() instead of printk.
It would also likely allow you to have everything fit on a single line.
> return 0; > } > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h > index b831e9fa1a26..33343eb980d0 100644 > --- a/include/linux/hugetlb.h > +++ b/include/linux/hugetlb.h > @@ -678,6 +678,13 @@ static inline spinlock_t *huge_pte_lockptr(struct hstate *h, > return &mm->page_table_lock; > } > > +#ifndef arch_hugetlb_valid_size > +static inline bool arch_hugetlb_valid_size(unsigned long long size) > +{ > + return (size == HPAGE_SIZE);
Not sure the ( ) are necessary.
> +} > +#endif > + > #ifndef hugepages_supported > /* > * Some platform decide whether they support huge pages at boot > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index d8ebd876871d..2f99359b93af 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -3224,12 +3224,22 @@ static int __init hugetlb_nrpages_setup(char *s) > } > __setup("hugepages=", hugetlb_nrpages_setup); > > -static int __init hugetlb_default_setup(char *s) > +static int __init default_hugepagesz_setup(char *s) > { > - default_hstate_size = memparse(s, &s); > + unsigned long long size;
Why unsigned long long ?
default_hstate_size is long.
I can't imagine 32 bits platforms having a hugepage with a 64 bits size.
> + char *saved_s = s; > + > + size = memparse(s, &s);
The updated s is not reused after that so you can pass NULL instead of &s and then you don't need the saved_s.
> + > + if (!arch_hugetlb_valid_size(size)) { > + pr_err("HugeTLB: unsupported default_hugepagesz %s\n", saved_s); > + return 0; > + } > + > + default_hstate_size = size; > return 1; > } > -__setup("default_hugepagesz=", hugetlb_default_setup); > +__setup("default_hugepagesz=", default_hugepagesz_setup); > > static unsigned int cpuset_mems_nr(unsigned int *array) > { >
Christophe
| |