Re: [PATCH 1/4] hugetlbfs: add arch_hugetlb_valid_size
From: Christophe Leroy
Date: Thu Mar 19 2020 - 03:01:11 EST
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@xxxxxxxxxx>
---
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