Re: [PATCH v6 4/4] hugetlb: allow to free gigantic pages regardless of the configuration

From: Alexandre Ghiti
Date: Thu Mar 14 2019 - 09:52:57 EST



On 03/14/2019 02:17 PM, Aneesh Kumar K.V wrote:
On 3/14/19 5:13 PM, Alexandre Ghiti wrote:
On 03/14/2019 06:52 AM, Aneesh Kumar K.V wrote:
Alexandre Ghiti <alex@xxxxxxxx> writes:

On systems without CONTIG_ALLOC activated but that support gigantic pages,
boottime reserved gigantic pages can not be freed at all. This patch
simply enables the possibility to hand back those pages to memory
allocator.

Signed-off-by: Alexandre Ghiti <alex@xxxxxxxx>
Acked-by: David S. Miller <davem@xxxxxxxxxxxxx> [sparc]
---
 arch/arm64/Kconfig | 2 +-
 arch/arm64/include/asm/hugetlb.h | 4 --
 arch/powerpc/include/asm/book3s/64/hugetlb.h | 7 ---
 arch/powerpc/platforms/Kconfig.cputype | 2 +-
 arch/s390/Kconfig | 2 +-
 arch/s390/include/asm/hugetlb.h | 3 --
 arch/sh/Kconfig | 2 +-
 arch/sparc/Kconfig | 2 +-
 arch/x86/Kconfig | 2 +-
 arch/x86/include/asm/hugetlb.h | 4 --
 include/linux/gfp.h | 2 +-
 mm/hugetlb.c | 57 ++++++++++++--------
 mm/page_alloc.c | 4 +-
 13 files changed, 44 insertions(+), 49 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 091a513b93e9..af687eff884a 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -18,7 +18,7 @@ config ARM64
ÂÂÂÂÂ select ARCH_HAS_FAST_MULTIPLIER
ÂÂÂÂÂ select ARCH_HAS_FORTIFY_SOURCE
ÂÂÂÂÂ select ARCH_HAS_GCOV_PROFILE_ALL
-ÂÂÂ select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
+ÂÂÂ select ARCH_HAS_GIGANTIC_PAGE
ÂÂÂÂÂ select ARCH_HAS_KCOV
ÂÂÂÂÂ select ARCH_HAS_MEMBARRIER_SYNC_CORE
ÂÂÂÂÂ select ARCH_HAS_PTE_SPECIAL
diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
index fb6609875455..59893e766824 100644
--- a/arch/arm64/include/asm/hugetlb.h
+++ b/arch/arm64/include/asm/hugetlb.h
@@ -65,8 +65,4 @@ extern void set_huge_swap_pte_at(struct mm_struct *mm, unsigned long addr,
 #include <asm-generic/hugetlb.h>
-#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
-static inline bool gigantic_page_supported(void) { return true; }
-#endif
-
 #endif /* __ASM_HUGETLB_H */
diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h b/arch/powerpc/include/asm/book3s/64/hugetlb.h
index 5b0177733994..d04a0bcc2f1c 100644
--- a/arch/powerpc/include/asm/book3s/64/hugetlb.h
+++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h
@@ -32,13 +32,6 @@ static inline int hstate_get_psize(struct hstate *hstate)
ÂÂÂÂÂ }
 }
-#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
-static inline bool gigantic_page_supported(void)
-{
-ÂÂÂ return true;
-}
-#endif
-
 /* hugepd entry valid bit */
 #define HUGEPD_VAL_BITS (0x8000000000000000UL)
As explained in https://patchwork.ozlabs.org/patch/1047003/
architectures like ppc64 have a hypervisor assisted mechanism to indicate
where to find gigantic huge pages(16G pages). At this point, we don't use this
reserved pages for anything other than hugetlb backing and hence there
is no runtime free of this pages needed ( Also we don't do
runtime allocation of them).

I guess you can still achieve what you want to do in this patch by
keeping gigantic_page_supported()?

NOTE: We should rename gigantic_page_supported to be more specific to
support for runtime_alloc/free of gigantic pages

-aneesh

Thanks for noticing Aneesh.

I can't find a better solution than bringing back gigantic_page_supported check,
since it is must be done at runtime in your case.
I'm not sure of one thing though: you say that freeing boottime gigantic pages
is not needed, but is it forbidden ? Just to know where the check and what its
new name should be.

You did not answer this question: is freeing boottime gigantic pages "forbidden" or just
not needed ?


Is something like that (on top of this series) ok for you (and everyone else) before
I send a v7:

diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h b/arch/powerpc/include/asm/book3s/64/hugetlb.h
index d04a0bc..d121559 100644
--- a/arch/powerpc/include/asm/book3s/64/hugetlb.h
+++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h
@@ -35,4 +35,20 @@ static inline int hstate_get_psize(struct hstate *hstate)
ÂÂ/* hugepd entry valid bit */
ÂÂ#define HUGEPD_VAL_BITSÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (0x8000000000000000UL)

+#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
+#define __HAVE_ARCH_GIGANTIC_PAGE_SUPPORTED
+static inline bool gigantic_page_supported(void)
+{
+ÂÂÂÂÂÂ /*
+ÂÂÂÂÂÂÂ * We used gigantic page reservation with hypervisor assist in some case.
+ÂÂÂÂÂÂÂ * We cannot use runtime allocation of gigantic pages in those platforms
+ÂÂÂÂÂÂÂ * This is hash translation mode LPARs.
+ÂÂÂÂÂÂÂ */
+ÂÂÂÂÂÂ if (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return false;
+
+ÂÂÂÂÂÂ return true;
+}
+#endif
+
ÂÂ#endif
diff --git a/include/asm-generic/hugetlb.h b/include/asm-generic/hugetlb.h
index 71d7b77..7d12e73 100644
--- a/include/asm-generic/hugetlb.h
+++ b/include/asm-generic/hugetlb.h
@@ -126,4 +126,18 @@ static inline pte_t huge_ptep_get(pte_t *ptep)
ÂÂ}
ÂÂ#endif

+#ifndef __HAVE_ARCH_GIGANTIC_PAGE_SUPPORTED
+#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE


The pattern i like is

#ifndef gigantic_page_supported
#define gigantic_page_supported gigantic_page_supported

static inline bool gigantic_page_supported(void)
{
ÂÂÂÂÂÂÂ return true;
}

#endif

instead of _HAVE_ARCH_GIGANTIC_PAGE_SUPPORTED.


I see, that avoids a new define. However, it is not consistent with the rest of function definitions
in generic hugetlb.h. What do you think ? Should I follow the same format ? Or use yours ?


+static inline bool gigantic_page_supported(void)
+{
+ÂÂÂÂÂÂÂ return true;
+}
+#else
+static inline bool gigantic_page_supported(void)
+{
+ÂÂÂÂÂÂÂ return false;
+}
+#endif /* CONFIG_ARCH_HAS_GIGANTIC_PAGE */
+#endif /* __HAVE_ARCH_GIGANTIC_PAGE_SUPPORTED */
+
ÂÂ#endif /* _ASM_GENERIC_HUGETLB_H */
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 9fc96ef..cfbbafe 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2425,6 +2425,11 @@ static ssize_t __nr_hugepages_store_common(bool obey_mempolicy,
ÂÂÂÂÂÂÂÂ int err;
ÂÂÂÂÂÂÂÂ NODEMASK_ALLOC(nodemask_t, nodes_allowed, GFP_KERNEL | __GFP_NORETRY);

+ÂÂÂÂÂÂ if (hstate_is_gigantic(h) && !gigantic_page_supported()) {
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ err = -EINVAL;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto out;
+ÂÂÂÂÂÂ }


you should restore other users of gigantic_page_supported() not just this. That will just make your earlier patch as removing gigantic_page_supported from every architecture other than ppc64 and have a generic version as above.



I'll restore the check in update_and_free_page too depending on your answer to the above question,
since adding this check back would not allow to free boottime gigantic pages.

+
ÂÂÂÂÂÂÂÂ if (nid == NUMA_NO_NODE) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /*
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ * global hstate attribute
@@ -2446,6 +2451,7 @@ static ssize_t __nr_hugepages_store_common(bool obey_mempolicy,

ÂÂÂÂÂÂÂÂ err = set_max_huge_pages(h, count, nodes_allowed);

+out:
ÂÂÂÂÂÂÂÂ if (nodes_allowed != &node_states[N_MEMORY])
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ NODEMASK_FREE(nodes_allowed);





-aneesh.