On Mon, Mar 23, 2020 at 5:17 PM Mike Kravetz [off-list ref] wrote:

On 3/23/20 5:01 PM, Mina Almasry wrote:

quoted

On Wed, Mar 18, 2020 at 3:07 PM Mike Kravetz [off-list ref] wrote:

quoted

The routine hugetlb_add_hstate prints a warning if the hstate already
exists.  This was originally done as part of kernel command line
parsing.  If 'hugepagesz=' was specified more than once, the warning
        pr_warn("hugepagesz= specified twice, ignoring\n");
would be printed.

Some architectures want to enable all huge page sizes.  They would
call hugetlb_add_hstate for all supported sizes.  However, this was
done after command line processing and as a result hstates could have
already been created for some sizes.  To make sure no warning were
printed, there would often be code like:
        if (!size_to_hstate(size)
                hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT)

The only time we want to print the warning is as the result of command
line processing.  So, remove the warning from hugetlb_add_hstate and
add it to the single arch independent routine processing "hugepagesz=".
After this, calls to size_to_hstate() in arch specific code can be
removed and hugetlb_add_hstate can be called without worrying about
warning messages.

Signed-off-by: Mike Kravetz <redacted>
---
 arch/arm64/mm/hugetlbpage.c   | 16 ++++------------
 arch/powerpc/mm/hugetlbpage.c |  3 +--
 arch/riscv/mm/hugetlbpage.c   |  2 +-
 arch/sparc/mm/init_64.c       | 19 ++++---------------
 arch/x86/mm/hugetlbpage.c     |  2 +-
 mm/hugetlb.c                  | 10 +++++++---
 6 files changed, 18 insertions(+), 34 deletions(-)

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index 4aa9534a45d7..050809e6f0a9 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c

@@ -441,22 +441,14 @@ void huge_ptep_clear_flush(struct vm_area_struct *vma,
        clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);
 }

-static void __init add_huge_page_size(unsigned long size)
-{
-       if (size_to_hstate(size))
-               return;
-
-       hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT);
-}
-
 static int __init hugetlbpage_init(void)
 {
 #ifdef CONFIG_ARM64_4K_PAGES
-       add_huge_page_size(PUD_SIZE);
+       hugetlb_add_hstate(ilog2(PUD_SIZE) - PAGE_SHIFT);
 #endif
-       add_huge_page_size(CONT_PMD_SIZE);
-       add_huge_page_size(PMD_SIZE);
-       add_huge_page_size(CONT_PTE_SIZE);
+       hugetlb_add_hstate(ilog2(CONT_PMD_SIZE) - PAGE_SHIFT);
+       hugetlb_add_hstate(ilog2(PMD_SIZE) - PAGE_SHIFT);
+       hugetlb_add_hstate(ilog2(CONT_PTE_SIZE) - PAGE_SHIFT);

        return 0;
 }

diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
index 166960ba1236..f46464ba6fb4 100644
--- a/arch/powerpc/mm/hugetlbpage.c
+++ b/arch/powerpc/mm/hugetlbpage.c

@@ -584,8 +584,7 @@ static int __init add_huge_page_size(unsigned long long size)
        if (!arch_hugetlb_valid_size(size))
                return -EINVAL;

-       if (!size_to_hstate(size))
-               hugetlb_add_hstate(shift - PAGE_SHIFT);
+       hugetlb_add_hstate(shift - PAGE_SHIFT);
        return 0;
 }

diff --git a/arch/riscv/mm/hugetlbpage.c b/arch/riscv/mm/hugetlbpage.c
index bdf89d7eb714..beaa91941db8 100644
--- a/arch/riscv/mm/hugetlbpage.c
+++ b/arch/riscv/mm/hugetlbpage.c

@@ -26,7 +26,7 @@ bool __init arch_hugetlb_valid_size(unsigned long long size)
 static __init int gigantic_pages_init(void)
 {
        /* With CONTIG_ALLOC, we can allocate gigantic pages at runtime */
-       if (IS_ENABLED(CONFIG_64BIT) && !size_to_hstate(1UL << PUD_SHIFT))
+       if (IS_ENABLED(CONFIG_64BIT))
                hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
        return 0;
 }

diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
index 5c29203fd460..8f619edc8f8c 100644
--- a/arch/sparc/mm/init_64.c
+++ b/arch/sparc/mm/init_64.c

@@ -325,23 +325,12 @@ static void __update_mmu_tsb_insert(struct mm_struct *mm, unsigned long tsb_inde
 }

 #ifdef CONFIG_HUGETLB_PAGE
-static void __init add_huge_page_size(unsigned long size)
-{
-       unsigned int order;
-
-       if (size_to_hstate(size))
-               return;
-
-       order = ilog2(size) - PAGE_SHIFT;
-       hugetlb_add_hstate(order);
-}
-
 static int __init hugetlbpage_init(void)
 {
-       add_huge_page_size(1UL << HPAGE_64K_SHIFT);
-       add_huge_page_size(1UL << HPAGE_SHIFT);
-       add_huge_page_size(1UL << HPAGE_256MB_SHIFT);
-       add_huge_page_size(1UL << HPAGE_2GB_SHIFT);
+       hugetlb_add_hstate(HPAGE_64K_SHIFT - PAGE_SHIFT);
+       hugetlb_add_hstate(HPAGE_SHIFT - PAGE_SHIFT);
+       hugetlb_add_hstate(HPAGE_256MB_SHIFT - PAGE_SHIFT);
+       hugetlb_add_hstate(HPAGE_2GB_SHIFT - PAGE_SHIFT);

        return 0;
 }

diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
index dd3ed09f6c23..8a3f586e1217 100644
--- a/arch/x86/mm/hugetlbpage.c
+++ b/arch/x86/mm/hugetlbpage.c

@@ -195,7 +195,7 @@ bool __init arch_hugetlb_valid_size(unsigned long long size)
 static __init int gigantic_pages_init(void)
 {
        /* With compaction or CMA we can allocate gigantic pages at runtime */
-       if (boot_cpu_has(X86_FEATURE_GBPAGES) && !size_to_hstate(1UL << PUD_SHIFT))
+       if (boot_cpu_has(X86_FEATURE_GBPAGES))
                hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
        return 0;
 }

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index cd4ec07080fb..cc85b4f156ca 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c

@@ -3116,8 +3116,7 @@ static int __init hugetlb_init(void)
                }

                default_hstate_size = HPAGE_SIZE;
-               if (!size_to_hstate(default_hstate_size))
-                       hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
+               hugetlb_add_hstate(HUGETLB_PAGE_ORDER);
        }
        default_hstate_idx = hstate_index(size_to_hstate(default_hstate_size));
        if (default_hstate_max_huge_pages) {

@@ -3155,7 +3154,6 @@ void __init hugetlb_add_hstate(unsigned int order)
        unsigned long i;

        if (size_to_hstate(PAGE_SIZE << order)) {
-               pr_warn("hugepagesz= specified twice, ignoring\n");
                return;
        }
        BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);

@@ -3231,6 +3229,12 @@ static int __init hugepagesz_setup(char *s)
                return 0;
        }

+       if (size_to_hstate(size)) {
+               pr_warn("HugeTLB: hugepagesz %s specified twice, ignoring\n",
+                       saved_s);
+               return 0;
+       }
+

Not too familiar with the code but I'm a bit confused by this print.
AFAICT this prints the warning when hugepagesz= refers to a hugepage
that is already added via hugetlb_add_hstate, but there is a default
hstate added here without the user specifying hugepagesz, no?

Correct.

quoted

                                                              Does
that mean the warning prints if you specify this size?

The code which adds the default hstate (in hugetlb_init) runs after this
code which is reading/processing command line options.  So, the case you
are concerned with will not happen.

Thanks for taking a look,
--
Mike Kravetz

quoted

'HugeTLB: hugepage size already supported: xxxxx' or
'HugeTLB: hugepage size xxxx specified twice or is default size, ignoring.'

Or don't print anything if it's the default size.

quoted

        hugetlb_add_hstate(ilog2(size) - PAGE_SHIFT);
        return 1;
 }
--
2.24.1

Acked-By: Mina Almasry <redacted>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel