2019-03-27 06:37:43

by Alexandre Ghiti

[permalink] [raw]
Subject: [PATCH v8 0/4] Fix free/allocation of runtime gigantic pages

This series fixes sh and sparc that did not advertise their gigantic page
support and then were not able to allocate and free those pages at runtime.
It renames MEMORY_ISOLATION && COMPACTION || CMA condition into the more
accurate CONTIG_ALLOC, since it allows the definition of alloc_contig_range
function.
Finally, it then fixes the wrong definition of ARCH_HAS_GIGANTIC_PAGE config
that, without MEMORY_ISOLATION && COMPACTION || CMA defined, did not allow
architectures to free boottime allocated gigantic pages although unrelated.

Changes in v8:
This (hopefully last) version is rebased against v5.1-rc2 so that
it takes into account https://patchwork.ozlabs.org/patch/1047003/.
This version:
- factorizes gigantic_page_runtime_supported such as suggested
by Christophe.
- fix checkpath warning regarding the use of 'extern'
- fix s390 build that does not include asm-generic/hugetlb.h
And note that I did not add the reviewed-by and acked-by received in v6
since the patch differs a little.

Changes in v7:
I thought gigantic page support was settled at compile time, but Aneesh
and Michael have just come up with a patch proving me wrong for
powerpc: https://patchwork.ozlabs.org/patch/1047003/. So this version:
- reintroduces gigantic_page_supported renamed into
gigantic_page_runtime_supported
- reintroduces gigantic page page support corresponding checks (not
everywhere though: set_max_huge_pages check was redundant with
__nr_hugepages_store_common)
- introduces the possibility for arch to override this function
by using asm-generic/hugetlb.h current semantics although Aneesh
proposed something else.

Changes in v6:
- Remove unnecessary goto since the fallthrough path does the same and is
the 'normal' behaviour, as suggested by Dave Hensen
- Be more explicit in comment in set_max_huge_page: we return an error
if alloc_contig_range is not defined and the user tries to allocate a
gigantic page (we keep the same behaviour as before this patch), but we
now let her free boottime gigantic page, as suggested by Dave Hensen
- Add Acked-by, thanks.

Changes in v5:
- Fix bug in previous version thanks to Mike Kravetz
- Fix block comments that did not respect coding style thanks to Dave Hensen
- Define ARCH_HAS_GIGANTIC_PAGE only for sparc64 as advised by David Miller
- Factorize "def_bool" and "depends on" thanks to Vlastimil Babka

Changes in v4 as suggested by Dave Hensen:
- Split previous version into small patches
- Do not compile alloc_gigantic** functions for architectures that do not
support those pages
- Define correct ARCH_HAS_GIGANTIC_PAGE in all arch that support them to avoid
useless runtime check
- Add comment in set_max_huge_pages to explain that freeing is possible even
without CONTIG_ALLOC defined
- Remove gigantic_page_supported function across all archs

Changes in v3 as suggested by Vlastimil Babka and Dave Hansen:
- config definition was wrong and is now in mm/Kconfig
- COMPACTION_CORE was renamed in CONTIG_ALLOC

Changes in v2 as suggested by Vlastimil Babka:
- Get rid of ARCH_HAS_GIGANTIC_PAGE
- Get rid of architecture specific gigantic_page_supported
- Factorize CMA or (MEMORY_ISOLATION && COMPACTION) into COMPACTION_CORE

Alexandre Ghiti (4):
sh: Advertise gigantic page support
sparc: Advertise gigantic page support
mm: Simplify MEMORY_ISOLATION && COMPACTION || CMA into CONTIG_ALLOC
hugetlb: allow to free gigantic pages regardless of the configuration

arch/arm64/Kconfig | 2 +-
arch/arm64/include/asm/hugetlb.h | 4 --
arch/powerpc/include/asm/book3s/64/hugetlb.h | 5 +-
arch/powerpc/platforms/Kconfig.cputype | 2 +-
arch/s390/Kconfig | 2 +-
arch/s390/include/asm/hugetlb.h | 8 +--
arch/sh/Kconfig | 1 +
arch/sparc/Kconfig | 1 +
arch/x86/Kconfig | 2 +-
arch/x86/include/asm/hugetlb.h | 4 --
arch/x86/mm/hugetlbpage.c | 2 +-
include/asm-generic/hugetlb.h | 7 +++
include/linux/gfp.h | 4 +-
mm/Kconfig | 3 ++
mm/hugetlb.c | 54 ++++++++++++++------
mm/page_alloc.c | 7 ++-
16 files changed, 67 insertions(+), 41 deletions(-)

--
2.20.1



2019-03-27 06:38:46

by Alexandre Ghiti

[permalink] [raw]
Subject: [PATCH v8 1/4] sh: Advertise gigantic page support

sh actually supports gigantic pages and selecting
ARCH_HAS_GIGANTIC_PAGE allows it to allocate and free
gigantic pages at runtime.

At least sdk7786_defconfig exposes such a configuration with
huge pages of 64MB, pages of 4KB and MAX_ORDER = 11:
HPAGE_SHIFT (26) - PAGE_SHIFT (12) = 14 >= MAX_ORDER (11)

Signed-off-by: Alexandre Ghiti <[email protected]>
---
arch/sh/Kconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index b1c91ea9a958..0d9fb2468e0b 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -53,6 +53,7 @@ config SUPERH
select HAVE_FUTEX_CMPXCHG if FUTEX
select HAVE_NMI
select NEED_SG_DMA_LENGTH
+ select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA

help
The SuperH is a RISC processor targeted for use in embedded systems
--
2.20.1


2019-03-27 06:41:04

by Alexandre Ghiti

[permalink] [raw]
Subject: [PATCH v8 3/4] mm: Simplify MEMORY_ISOLATION && COMPACTION || CMA into CONTIG_ALLOC

This condition allows to define alloc_contig_range, so simplify
it into a more accurate naming.

Suggested-by: Vlastimil Babka <[email protected]>
Signed-off-by: Alexandre Ghiti <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
---
arch/arm64/Kconfig | 2 +-
arch/powerpc/platforms/Kconfig.cputype | 2 +-
arch/s390/Kconfig | 2 +-
arch/sh/Kconfig | 2 +-
arch/sparc/Kconfig | 2 +-
arch/x86/Kconfig | 2 +-
arch/x86/mm/hugetlbpage.c | 2 +-
include/linux/gfp.h | 2 +-
mm/Kconfig | 3 +++
mm/page_alloc.c | 3 +--
10 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7e34b9eba5de..a8380629cb3f 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 (MEMORY_ISOLATION && COMPACTION) || CMA
+ select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
select ARCH_HAS_KCOV
select ARCH_HAS_MEMBARRIER_SYNC_CORE
select ARCH_HAS_PTE_SPECIAL
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index 842b2c7e156a..eb0f592cde69 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -325,7 +325,7 @@ config ARCH_ENABLE_SPLIT_PMD_PTLOCK
config PPC_RADIX_MMU
bool "Radix MMU Support"
depends on PPC_BOOK3S_64
- select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
+ select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
default y
help
Enable support for the Power ISA 3.0 Radix style MMU. Currently this
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index b6e3d0653002..1c8cb55b4e5d 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -69,7 +69,7 @@ config S390
select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_FORTIFY_SOURCE
select ARCH_HAS_GCOV_PROFILE_ALL
- select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
+ select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
select ARCH_HAS_KCOV
select ARCH_HAS_PTE_SPECIAL
select ARCH_HAS_SET_MEMORY
diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 0d9fb2468e0b..67931bdaf32f 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -53,7 +53,7 @@ config SUPERH
select HAVE_FUTEX_CMPXCHG if FUTEX
select HAVE_NMI
select NEED_SG_DMA_LENGTH
- select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
+ select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC

help
The SuperH is a RISC processor targeted for use in embedded systems
diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index ebcc9435db08..52193cfb0f32 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -91,7 +91,7 @@ config SPARC64
select ARCH_CLOCKSOURCE_DATA
select ARCH_HAS_PTE_SPECIAL
select PCI_DOMAINS if PCI
- select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
+ select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC

config ARCH_DEFCONFIG
string
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index c1f9b3cf437c..749bab313dc1 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -21,7 +21,7 @@ config X86_64
def_bool y
depends on 64BIT
# Options that are inherently 64-bit kernel only:
- select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
+ select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
select ARCH_SUPPORTS_INT128
select ARCH_USE_CMPXCHG_LOCKREF
select HAVE_ARCH_SOFT_DIRTY
diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
index 92e4c4b85bba..fab095362c50 100644
--- a/arch/x86/mm/hugetlbpage.c
+++ b/arch/x86/mm/hugetlbpage.c
@@ -203,7 +203,7 @@ static __init int setup_hugepagesz(char *opt)
}
__setup("hugepagesz=", setup_hugepagesz);

-#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
+#ifdef CONFIG_CONTIG_ALLOC
static __init int gigantic_pages_init(void)
{
/* With compaction or CMA we can allocate gigantic pages at runtime */
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index fdab7de7490d..e77ab30e9328 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -585,7 +585,7 @@ static inline bool pm_suspended_storage(void)
}
#endif /* CONFIG_PM_SLEEP */

-#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
+#ifdef CONFIG_CONTIG_ALLOC
/* The below functions must be run on a range from a single zone. */
extern int alloc_contig_range(unsigned long start, unsigned long end,
unsigned migratetype, gfp_t gfp_mask);
diff --git a/mm/Kconfig b/mm/Kconfig
index 25c71eb8a7db..137eadc18732 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -258,6 +258,9 @@ config ARCH_ENABLE_HUGEPAGE_MIGRATION
config ARCH_ENABLE_THP_MIGRATION
bool

+config CONTIG_ALLOC
+ def_bool (MEMORY_ISOLATION && COMPACTION) || CMA
+
config PHYS_ADDR_T_64BIT
def_bool 64BIT

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 03fcf73d47da..ecb115a74a9d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -8109,8 +8109,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
return true;
}

-#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
-
+#ifdef CONFIG_CONTIG_ALLOC
static unsigned long pfn_max_align_down(unsigned long pfn)
{
return pfn & ~(max_t(unsigned long, MAX_ORDER_NR_PAGES,
--
2.20.1


2019-03-27 06:41:33

by Alexandre Ghiti

[permalink] [raw]
Subject: [PATCH v8 2/4] sparc: Advertise gigantic page support

sparc actually supports gigantic pages and selecting
ARCH_HAS_GIGANTIC_PAGE allows it to allocate and free
gigantic pages at runtime.

sparc allows configuration such as huge pages of 16GB,
pages of 8KB and MAX_ORDER = 13 (default):
HPAGE_SHIFT (34) - PAGE_SHIFT (13) = 21 >= MAX_ORDER (13)

Signed-off-by: Alexandre Ghiti <[email protected]>
Acked-by: David S. Miller <[email protected]>
---
arch/sparc/Kconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 40f8f4f73fe8..ebcc9435db08 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -91,6 +91,7 @@ config SPARC64
select ARCH_CLOCKSOURCE_DATA
select ARCH_HAS_PTE_SPECIAL
select PCI_DOMAINS if PCI
+ select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA

config ARCH_DEFCONFIG
string
--
2.20.1


2019-03-27 06:42:45

by Alexandre Ghiti

[permalink] [raw]
Subject: [PATCH v8 4/4] hugetlb: allow to free gigantic pages regardless of the configuration

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 <[email protected]>
Acked-by: David S. Miller <[email protected]> [sparc]
---
arch/arm64/Kconfig | 2 +-
arch/arm64/include/asm/hugetlb.h | 4 --
arch/powerpc/include/asm/book3s/64/hugetlb.h | 5 +-
arch/powerpc/platforms/Kconfig.cputype | 2 +-
arch/s390/Kconfig | 2 +-
arch/s390/include/asm/hugetlb.h | 8 +--
arch/sh/Kconfig | 2 +-
arch/sparc/Kconfig | 2 +-
arch/x86/Kconfig | 2 +-
arch/x86/include/asm/hugetlb.h | 4 --
include/asm-generic/hugetlb.h | 7 +++
include/linux/gfp.h | 2 +-
mm/hugetlb.c | 54 ++++++++++++++------
mm/page_alloc.c | 4 +-
14 files changed, 61 insertions(+), 39 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index a8380629cb3f..d33c6a7b9fc5 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 c6a07a3b433e..4aad6382f631 100644
--- a/arch/arm64/include/asm/hugetlb.h
+++ b/arch/arm64/include/asm/hugetlb.h
@@ -70,8 +70,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 ec2a55a553c7..7013284f0f1b 100644
--- a/arch/powerpc/include/asm/book3s/64/hugetlb.h
+++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h
@@ -36,8 +36,8 @@ static inline int hstate_get_psize(struct hstate *hstate)
}
}

-#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
-static inline bool gigantic_page_supported(void)
+#define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
+static inline bool gigantic_page_runtime_supported(void)
{
/*
* We used gigantic page reservation with hypervisor assist in some case.
@@ -49,7 +49,6 @@ static inline bool gigantic_page_supported(void)

return true;
}
-#endif

/* hugepd entry valid bit */
#define HUGEPD_VAL_BITS (0x8000000000000000UL)
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index eb0f592cde69..03ca91439473 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -325,7 +325,7 @@ config ARCH_ENABLE_SPLIT_PMD_PTLOCK
config PPC_RADIX_MMU
bool "Radix MMU Support"
depends on PPC_BOOK3S_64
- select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
+ select ARCH_HAS_GIGANTIC_PAGE
default y
help
Enable support for the Power ISA 3.0 Radix style MMU. Currently this
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 1c8cb55b4e5d..5a9cc12c32c6 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -69,7 +69,7 @@ config S390
select ARCH_HAS_ELF_RANDOMIZE
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_PTE_SPECIAL
select ARCH_HAS_SET_MEMORY
diff --git a/arch/s390/include/asm/hugetlb.h b/arch/s390/include/asm/hugetlb.h
index 2d1afa58a4b6..bb59dd964590 100644
--- a/arch/s390/include/asm/hugetlb.h
+++ b/arch/s390/include/asm/hugetlb.h
@@ -116,7 +116,9 @@ static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
return pte_modify(pte, newprot);
}

-#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
-static inline bool gigantic_page_supported(void) { return true; }
-#endif
+static inline bool gigantic_page_runtime_supported(void)
+{
+ return true;
+}
+
#endif /* _ASM_S390_HUGETLB_H */
diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 67931bdaf32f..6d3db745ab7c 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -53,7 +53,7 @@ config SUPERH
select HAVE_FUTEX_CMPXCHG if FUTEX
select HAVE_NMI
select NEED_SG_DMA_LENGTH
- select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
+ select ARCH_HAS_GIGANTIC_PAGE

help
The SuperH is a RISC processor targeted for use in embedded systems
diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 52193cfb0f32..98f8dd663fd8 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -91,7 +91,7 @@ config SPARC64
select ARCH_CLOCKSOURCE_DATA
select ARCH_HAS_PTE_SPECIAL
select PCI_DOMAINS if PCI
- select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
+ select ARCH_HAS_GIGANTIC_PAGE

config ARCH_DEFCONFIG
string
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 749bab313dc1..1b7584d17a5a 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -21,7 +21,7 @@ config X86_64
def_bool y
depends on 64BIT
# Options that are inherently 64-bit kernel only:
- select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
+ select ARCH_HAS_GIGANTIC_PAGE
select ARCH_SUPPORTS_INT128
select ARCH_USE_CMPXCHG_LOCKREF
select HAVE_ARCH_SOFT_DIRTY
diff --git a/arch/x86/include/asm/hugetlb.h b/arch/x86/include/asm/hugetlb.h
index 7469d321f072..f65cfb48cfdd 100644
--- a/arch/x86/include/asm/hugetlb.h
+++ b/arch/x86/include/asm/hugetlb.h
@@ -17,8 +17,4 @@ static inline void arch_clear_hugepage_flags(struct page *page)
{
}

-#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
-static inline bool gigantic_page_supported(void) { return true; }
-#endif
-
#endif /* _ASM_X86_HUGETLB_H */
diff --git a/include/asm-generic/hugetlb.h b/include/asm-generic/hugetlb.h
index 71d7b77eea50..822f433ac95c 100644
--- a/include/asm-generic/hugetlb.h
+++ b/include/asm-generic/hugetlb.h
@@ -126,4 +126,11 @@ static inline pte_t huge_ptep_get(pte_t *ptep)
}
#endif

+#ifndef __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
+static inline bool gigantic_page_runtime_supported(void)
+{
+ return IS_ENABLED(CONFIG_ARCH_HAS_GIGANTIC_PAGE);
+}
+#endif /* __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED */
+
#endif /* _ASM_GENERIC_HUGETLB_H */
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index e77ab30e9328..fb07b503dc45 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -589,8 +589,8 @@ static inline bool pm_suspended_storage(void)
/* The below functions must be run on a range from a single zone. */
extern int alloc_contig_range(unsigned long start, unsigned long end,
unsigned migratetype, gfp_t gfp_mask);
-extern void free_contig_range(unsigned long pfn, unsigned nr_pages);
#endif
+void free_contig_range(unsigned long pfn, unsigned int nr_pages);

#ifdef CONFIG_CMA
/* CMA stuff */
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 97b1e0290c66..f3e84c1bef11 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1059,6 +1059,7 @@ static void free_gigantic_page(struct page *page, unsigned int order)
free_contig_range(page_to_pfn(page), 1 << order);
}

+#ifdef CONFIG_CONTIG_ALLOC
static int __alloc_gigantic_page(unsigned long start_pfn,
unsigned long nr_pages, gfp_t gfp_mask)
{
@@ -1143,11 +1144,20 @@ static struct page *alloc_gigantic_page(struct hstate *h, gfp_t gfp_mask,

static void prep_new_huge_page(struct hstate *h, struct page *page, int nid);
static void prep_compound_gigantic_page(struct page *page, unsigned int order);
+#else /* !CONFIG_CONTIG_ALLOC */
+static struct page *alloc_gigantic_page(struct hstate *h, gfp_t gfp_mask,
+ int nid, nodemask_t *nodemask)
+{
+ return NULL;
+}
+#endif /* CONFIG_CONTIG_ALLOC */

#else /* !CONFIG_ARCH_HAS_GIGANTIC_PAGE */
-static inline bool gigantic_page_supported(void) { return false; }
static struct page *alloc_gigantic_page(struct hstate *h, gfp_t gfp_mask,
- int nid, nodemask_t *nodemask) { return NULL; }
+ int nid, nodemask_t *nodemask)
+{
+ return NULL;
+}
static inline void free_gigantic_page(struct page *page, unsigned int order) { }
static inline void destroy_compound_gigantic_page(struct page *page,
unsigned int order) { }
@@ -1157,7 +1167,7 @@ static void update_and_free_page(struct hstate *h, struct page *page)
{
int i;

- if (hstate_is_gigantic(h) && !gigantic_page_supported())
+ if (hstate_is_gigantic(h) && !gigantic_page_runtime_supported())
return;

h->nr_huge_pages--;
@@ -2277,13 +2287,27 @@ static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,
}

#define persistent_huge_pages(h) (h->nr_huge_pages - h->surplus_huge_pages)
-static unsigned long set_max_huge_pages(struct hstate *h, unsigned long count,
- nodemask_t *nodes_allowed)
+static int set_max_huge_pages(struct hstate *h, unsigned long count,
+ nodemask_t *nodes_allowed)
{
unsigned long min_count, ret;

- if (hstate_is_gigantic(h) && !gigantic_page_supported())
- return h->max_huge_pages;
+ spin_lock(&hugetlb_lock);
+
+ /*
+ * Gigantic pages runtime allocation depend on the capability for large
+ * page range allocation.
+ * If the system does not provide this feature, return an error when
+ * the user tries to allocate gigantic pages but let the user free the
+ * boottime allocated gigantic pages.
+ */
+ if (hstate_is_gigantic(h) && !IS_ENABLED(CONFIG_CONTIG_ALLOC)) {
+ if (count > persistent_huge_pages(h)) {
+ spin_unlock(&hugetlb_lock);
+ return -EINVAL;
+ }
+ /* Fall through to decrease pool */
+ }

/*
* Increase the pool size
@@ -2296,7 +2320,6 @@ static unsigned long set_max_huge_pages(struct hstate *h, unsigned long count,
* pool might be one hugepage larger than it needs to be, but
* within all the constraints specified by the sysctls.
*/
- spin_lock(&hugetlb_lock);
while (h->surplus_huge_pages && count > persistent_huge_pages(h)) {
if (!adjust_pool_surplus(h, nodes_allowed, -1))
break;
@@ -2351,9 +2374,10 @@ static unsigned long set_max_huge_pages(struct hstate *h, unsigned long count,
break;
}
out:
- ret = persistent_huge_pages(h);
+ h->max_huge_pages = persistent_huge_pages(h);
spin_unlock(&hugetlb_lock);
- return ret;
+
+ return 0;
}

#define HSTATE_ATTR_RO(_name) \
@@ -2405,7 +2429,7 @@ 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()) {
+ if (hstate_is_gigantic(h) && !gigantic_page_runtime_supported()) {
err = -EINVAL;
goto out;
}
@@ -2429,15 +2453,13 @@ static ssize_t __nr_hugepages_store_common(bool obey_mempolicy,
} else
nodes_allowed = &node_states[N_MEMORY];

- h->max_huge_pages = set_max_huge_pages(h, count, nodes_allowed);
+ err = set_max_huge_pages(h, count, nodes_allowed);

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

- return len;
-out:
- NODEMASK_FREE(nodes_allowed);
- return err;
+ return err ? err : len;
}

static ssize_t nr_hugepages_store_common(bool obey_mempolicy,
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ecb115a74a9d..cad000879f14 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -8318,8 +8318,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
pfn_max_align_up(end), migratetype);
return ret;
}
+#endif /* CONFIG_CONTIG_ALLOC */

-void free_contig_range(unsigned long pfn, unsigned nr_pages)
+void free_contig_range(unsigned long pfn, unsigned int nr_pages)
{
unsigned int count = 0;

@@ -8331,7 +8332,6 @@ void free_contig_range(unsigned long pfn, unsigned nr_pages)
}
WARN(count != 0, "%d pages are still in use!\n", count);
}
-#endif

#ifdef CONFIG_MEMORY_HOTPLUG
/*
--
2.20.1


2019-03-27 07:03:48

by Aneesh Kumar K.V

[permalink] [raw]
Subject: Re: [PATCH v8 4/4] hugetlb: allow to free gigantic pages regardless of the configuration

On 3/27/19 12:06 PM, Alexandre Ghiti wrote:
> 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 <[email protected]>
> Acked-by: David S. Miller <[email protected]> [sparc]
>
> diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h b/arch/powerpc/include/asm/book3s/64/hugetlb.h
> index ec2a55a553c7..7013284f0f1b 100644
> --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h
> +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h
> @@ -36,8 +36,8 @@ static inline int hstate_get_psize(struct hstate *hstate)
> }
> }
>
> -#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
> -static inline bool gigantic_page_supported(void)
> +#define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
> +static inline bool gigantic_page_runtime_supported(void)
> {
> /*
> * We used gigantic page reservation with hypervisor assist in some case.
> @@ -49,7 +49,6 @@ static inline bool gigantic_page_supported(void)
>
> return true;
> }
> -#endif
>
> /* hugepd entry valid bit */
> #define HUGEPD_VAL_BITS (0x8000000000000000UL)

Is that correct when CONTIG_ALLOC is not enabled? I guess we want

gigantic_page_runtime_supported to return false when CONTIG_ALLOC is not
enabled on all architectures and on POWER when it is enabled we want it
to be conditional as it is now.

-aneesh


2019-03-27 08:47:56

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH v8 4/4] hugetlb: allow to free gigantic pages regardless of the configuration



On 03/27/2019 08:01 AM, Aneesh Kumar K.V wrote:
> On 3/27/19 12:06 PM, Alexandre Ghiti wrote:
>> 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 <[email protected]>
>> Acked-by: David S. Miller <[email protected]> [sparc]
>>
>> diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h
>> b/arch/powerpc/include/asm/book3s/64/hugetlb.h
>> index ec2a55a553c7..7013284f0f1b 100644
>> --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h
>> +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h
>> @@ -36,8 +36,8 @@ static inline int hstate_get_psize(struct hstate
>> *hstate)
>>       }
>>   }
>>   -#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
>> -static inline bool gigantic_page_supported(void)
>> +#define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
>> +static inline bool gigantic_page_runtime_supported(void)
>>   {
>>       /*
>>        * We used gigantic page reservation with hypervisor assist in
>> some case.
>> @@ -49,7 +49,6 @@ static inline bool gigantic_page_supported(void)
>>         return true;
>>   }
>> -#endif
>>     /* hugepd entry valid bit */
>>   #define HUGEPD_VAL_BITS        (0x8000000000000000UL)
>
> Is that correct when CONTIG_ALLOC is not enabled? I guess we want
>
> gigantic_page_runtime_supported to return false when CONTIG_ALLOC is
> not enabled on all architectures and on POWER when it is enabled we
> want it to be conditional as it is now.
>
> -aneesh
>

CONFIG_ARCH_HAS_GIGANTIC_PAGE is set by default when an architecture
supports gigantic
pages: on its own, it allows to allocate boottime gigantic pages AND to
free them at runtime
(this is the goal of this series), but not to allocate runtime gigantic
pages.
If CONTIG_ALLOC is set, it allows in addition to allocate runtime
gigantic pages.

I re-introduced the runtime checks because we can't know at compile time
if powerpc can
or not support gigantic pages.

So for all architectures, gigantic_page_runtime_supported only depends on
CONFIG_ARCH_HAS_GIGANTIC_PAGE enabled or not. The possibility to
allocate runtime
gigantic pages is dealt with after those runtime checks.

By the way, I forgot to ask you why you think that if an arch cannot
allocate runtime gigantic
pages, it should not be able to free boottime gigantic pages ?

2019-03-27 08:58:06

by Aneesh Kumar K.V

[permalink] [raw]
Subject: Re: [PATCH v8 4/4] hugetlb: allow to free gigantic pages regardless of the configuration

On 3/27/19 2:14 PM, Alexandre Ghiti wrote:
>
>
> On 03/27/2019 08:01 AM, Aneesh Kumar K.V wrote:
>> On 3/27/19 12:06 PM, Alexandre Ghiti wrote:
>>> 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 <[email protected]>
>>> Acked-by: David S. Miller <[email protected]> [sparc]
>>>
>>> diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h
>>> b/arch/powerpc/include/asm/book3s/64/hugetlb.h
>>> index ec2a55a553c7..7013284f0f1b 100644
>>> --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h
>>> +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h
>>> @@ -36,8 +36,8 @@ static inline int hstate_get_psize(struct hstate
>>> *hstate)
>>>       }
>>>   }
>>>   -#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
>>> -static inline bool gigantic_page_supported(void)
>>> +#define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
>>> +static inline bool gigantic_page_runtime_supported(void)
>>>   {
>>>       /*
>>>        * We used gigantic page reservation with hypervisor assist in
>>> some case.
>>> @@ -49,7 +49,6 @@ static inline bool gigantic_page_supported(void)
>>>         return true;
>>>   }
>>> -#endif
>>>     /* hugepd entry valid bit */
>>>   #define HUGEPD_VAL_BITS        (0x8000000000000000UL)
>>
>> Is that correct when CONTIG_ALLOC is not enabled? I guess we want
>>
>> gigantic_page_runtime_supported to return false when CONTIG_ALLOC is
>> not enabled on all architectures and on POWER when it is enabled we
>> want it to be conditional as it is now.
>>
>> -aneesh
>>
>
> CONFIG_ARCH_HAS_GIGANTIC_PAGE is set by default when an architecture
> supports gigantic
> pages: on its own, it allows to allocate boottime gigantic pages AND to
> free them at runtime
> (this is the goal of this series), but not to allocate runtime gigantic
> pages.
> If CONTIG_ALLOC is set, it allows in addition to allocate runtime
> gigantic pages.
>
> I re-introduced the runtime checks because we can't know at compile time
> if powerpc can
> or not support gigantic pages.
>
> So for all architectures, gigantic_page_runtime_supported only depends on
> CONFIG_ARCH_HAS_GIGANTIC_PAGE enabled or not. The possibility to
> allocate runtime
> gigantic pages is dealt with after those runtime checks.
>

you removed that #ifdef in the patch above. ie we had
#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
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


This is now
#define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
static inline bool gigantic_page_runtime_supported(void)
{
if (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
return false;

return true;
}


I am wondering whether it should be

#define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
static inline bool gigantic_page_runtime_supported(void)
{

if (!IS_ENABLED(CONFIG_CONTIG_ALLOC))
return false;

if (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
return false;

return true;
}

or add that #ifdef back.

> By the way, I forgot to ask you why you think that if an arch cannot
> allocate runtime gigantic
> pages, it should not be able to free boottime gigantic pages ?
>

on virtualized platforms like powervm which use a paravirtualized page
table update mechanism (we dont' have two level table). The ability to
map a page huge depends on how hypervisor allocated the guest ram.
Hypervisor also allocates the guest specific page table of a specific
size depending on how many pages are going to be mapped by what page size.

on POWER we indicate possible guest real address that can be mapped via
hugepage (in this case 16G) using a device tree node
(ibm,expected#pages) . It is expected that we will map these pages only
as 16G pages. Hence we cannot free them back to the buddy where it could
get mapped via 64K page size.

-aneesh



2019-03-27 09:50:30

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH v8 4/4] hugetlb: allow to free gigantic pages regardless of the configuration

On 03/27/2019 09:55 AM, Aneesh Kumar K.V wrote:
> On 3/27/19 2:14 PM, Alexandre Ghiti wrote:
>>
>>
>> On 03/27/2019 08:01 AM, Aneesh Kumar K.V wrote:
>>> On 3/27/19 12:06 PM, Alexandre Ghiti wrote:
>>>> 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 <[email protected]>
>>>> Acked-by: David S. Miller <[email protected]> [sparc]
>>>>
>>>> diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h
>>>> b/arch/powerpc/include/asm/book3s/64/hugetlb.h
>>>> index ec2a55a553c7..7013284f0f1b 100644
>>>> --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h
>>>> +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h
>>>> @@ -36,8 +36,8 @@ static inline int hstate_get_psize(struct hstate
>>>> *hstate)
>>>>       }
>>>>   }
>>>>   -#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
>>>> -static inline bool gigantic_page_supported(void)
>>>> +#define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
>>>> +static inline bool gigantic_page_runtime_supported(void)
>>>>   {
>>>>       /*
>>>>        * We used gigantic page reservation with hypervisor assist
>>>> in some case.
>>>> @@ -49,7 +49,6 @@ static inline bool gigantic_page_supported(void)
>>>>         return true;
>>>>   }
>>>> -#endif
>>>>     /* hugepd entry valid bit */
>>>>   #define HUGEPD_VAL_BITS        (0x8000000000000000UL)
>>>
>>> Is that correct when CONTIG_ALLOC is not enabled? I guess we want
>>>
>>> gigantic_page_runtime_supported to return false when CONTIG_ALLOC is
>>> not enabled on all architectures and on POWER when it is enabled we
>>> want it to be conditional as it is now.
>>>
>>> -aneesh
>>>
>>
>> CONFIG_ARCH_HAS_GIGANTIC_PAGE is set by default when an architecture
>> supports gigantic
>> pages: on its own, it allows to allocate boottime gigantic pages AND
>> to free them at runtime
>> (this is the goal of this series), but not to allocate runtime
>> gigantic pages.
>> If CONTIG_ALLOC is set, it allows in addition to allocate runtime
>> gigantic pages.
>>
>> I re-introduced the runtime checks because we can't know at compile
>> time if powerpc can
>> or not support gigantic pages.
>>
>> So for all architectures, gigantic_page_runtime_supported only
>> depends on
>> CONFIG_ARCH_HAS_GIGANTIC_PAGE enabled or not. The possibility to
>> allocate runtime
>> gigantic pages is dealt with after those runtime checks.
>>
>
> you removed that #ifdef in the patch above. ie we had
> #ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
> 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

Yes, I removed the #ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE because it was
defined only
if CONTIG_ALLOC was set. But now, CONFIG_ARCH_HAS_GIGANTIC_PAGE is
inconditionally
set for powerpc so I think we don't need it anymore.
Actually I have doubts now, is this true for all configurations ? I see
that it is only set for
PPC_RADIX_MMU. I think the problem is here: instead of returning true,
it should do like
the generic version, ie return IS_ENABLED(CONFIG_ARCH_HAS_GIGANTIC_PAGE).
Do you agree ?

>
>
> This is now
> #define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
> static inline bool gigantic_page_runtime_supported(void)
> {
> if (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
>         return false;
>
>     return true;
> }
>
>
> I am wondering whether it should be
>
> #define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
> static inline bool gigantic_page_runtime_supported(void)
> {
>
>    if (!IS_ENABLED(CONFIG_CONTIG_ALLOC))
>         return false;

I don't think this test should happen here, CONFIG_CONTIG_ALLOC only allows
to allocate gigantic pages, doing that check here would prevent powerpc
to free boottime gigantic pages when not a guest. Note that this check
is actually done in set_max_huge_pages.


>
> if (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
>         return false;

Maybe I did not understand this check: I understood that, in the case
the system
is virtualized, we do not want it to hand back gigantic pages. Does this
check
test if the system is currently being virtualized ?
If yes, I think the patch is correct: it prevents freeing gigantic pages
when the system
is virtualized but allows a 'normal' system to free gigantic pages.


>
>     return true;
> }
>
> or add that #ifdef back.
>
>> By the way, I forgot to ask you why you think that if an arch cannot
>> allocate runtime gigantic
>> pages, it should not be able to free boottime gigantic pages ?
>>
>
> on virtualized platforms like powervm which use a paravirtualized page
> table update mechanism (we dont' have two level table). The ability to
> map a page huge depends on how hypervisor allocated the guest ram.
> Hypervisor also allocates the guest specific page table of a specific
> size depending on how many pages are going to be mapped by what page
> size.
>
> on POWER we indicate possible guest real address that can be mapped
> via hugepage (in this case 16G) using a device tree node
> (ibm,expected#pages) . It is expected that we will map these pages
> only as 16G pages. Hence we cannot free them back to the buddy where
> it could get mapped via 64K page size.

Thanks for the explanations.

Alex
>
> -aneesh
>
>


2019-03-27 10:07:15

by Aneesh Kumar K.V

[permalink] [raw]
Subject: Re: [PATCH v8 4/4] hugetlb: allow to free gigantic pages regardless of the configuration

Alexandre Ghiti <[email protected]> writes:

> On 03/27/2019 09:55 AM, Aneesh Kumar K.V wrote:
>> On 3/27/19 2:14 PM, Alexandre Ghiti wrote:
>>>
>>>
>>> On 03/27/2019 08:01 AM, Aneesh Kumar K.V wrote:
>>>> On 3/27/19 12:06 PM, Alexandre Ghiti wrote:
>

.....

>>
>> This is now
>> #define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
>> static inline bool gigantic_page_runtime_supported(void)
>> {
>> if (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
>>         return false;
>>
>>     return true;
>> }
>>
>>
>> I am wondering whether it should be
>>
>> #define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
>> static inline bool gigantic_page_runtime_supported(void)
>> {
>>
>>    if (!IS_ENABLED(CONFIG_CONTIG_ALLOC))
>>         return false;
>
> I don't think this test should happen here, CONFIG_CONTIG_ALLOC only allows
> to allocate gigantic pages, doing that check here would prevent powerpc
> to free boottime gigantic pages when not a guest. Note that this check
> is actually done in set_max_huge_pages.
>
>
>>
>> if (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
>>         return false;
>
> Maybe I did not understand this check: I understood that, in the case
> the system
> is virtualized, we do not want it to hand back gigantic pages. Does this
> check
> test if the system is currently being virtualized ?
> If yes, I think the patch is correct: it prevents freeing gigantic pages
> when the system
> is virtualized but allows a 'normal' system to free gigantic pages.
>
>
>>

Ok double checked the patch applying the the tree. I got confused by the
removal of that #ifdef. So we now disallow the runtime free by checking
for gigantic_page_runtime_supported() in __nr_hugepages_store_common.
Now if we allow and if CONFIG_CONTIG_ALLOC is disabled, we still should
allow to free the boot time allocated pages back to buddy.

The patch looks good. You can add for the series

Reviewed-by: Aneesh Kumar K.V <[email protected]>

-aneesh


2019-03-27 12:56:54

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH v8 4/4] hugetlb: allow to free gigantic pages regardless of the configuration

On 03/27/2019 11:05 AM, Aneesh Kumar K.V wrote:
> Alexandre Ghiti <[email protected]> writes:
>
>> On 03/27/2019 09:55 AM, Aneesh Kumar K.V wrote:
>>> On 3/27/19 2:14 PM, Alexandre Ghiti wrote:
>>>>
>>>> On 03/27/2019 08:01 AM, Aneesh Kumar K.V wrote:
>>>>> On 3/27/19 12:06 PM, Alexandre Ghiti wrote:
> .....
>
>>> This is now
>>> #define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
>>> static inline bool gigantic_page_runtime_supported(void)
>>> {
>>> if (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
>>>         return false;
>>>
>>>     return true;
>>> }
>>>
>>>
>>> I am wondering whether it should be
>>>
>>> #define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
>>> static inline bool gigantic_page_runtime_supported(void)
>>> {
>>>
>>>    if (!IS_ENABLED(CONFIG_CONTIG_ALLOC))
>>>         return false;
>> I don't think this test should happen here, CONFIG_CONTIG_ALLOC only allows
>> to allocate gigantic pages, doing that check here would prevent powerpc
>> to free boottime gigantic pages when not a guest. Note that this check
>> is actually done in set_max_huge_pages.
>>
>>
>>> if (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
>>>         return false;
>> Maybe I did not understand this check: I understood that, in the case
>> the system
>> is virtualized, we do not want it to hand back gigantic pages. Does this
>> check
>> test if the system is currently being virtualized ?
>> If yes, I think the patch is correct: it prevents freeing gigantic pages
>> when the system
>> is virtualized but allows a 'normal' system to free gigantic pages.
>>
>>
> Ok double checked the patch applying the the tree. I got confused by the
> removal of that #ifdef. So we now disallow the runtime free by checking
> for gigantic_page_runtime_supported() in __nr_hugepages_store_common.
> Now if we allow and if CONFIG_CONTIG_ALLOC is disabled, we still should
> allow to free the boot time allocated pages back to buddy.
>
> The patch looks good. You can add for the series
>
> Reviewed-by: Aneesh Kumar K.V <[email protected]>
>
> -aneesh
>

Thanks for your time Aneesh,

Alex

2019-03-28 20:46:52

by Mike Kravetz

[permalink] [raw]
Subject: Re: [PATCH v8 4/4] hugetlb: allow to free gigantic pages regardless of the configuration

On 3/26/19 11:36 PM, Alexandre Ghiti wrote:
> 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 <[email protected]>
> Acked-by: David S. Miller <[email protected]> [sparc]

Thanks for all the updates

Reviewed-by: Mike Kravetz <[email protected]>

--
Mike Kravetz

2019-03-29 06:55:33

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH v8 4/4] hugetlb: allow to free gigantic pages regardless of the configuration

On 3/28/19 4:43 PM, Mike Kravetz wrote:
> On 3/26/19 11:36 PM, Alexandre Ghiti wrote:
>> 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 <[email protected]>
>> Acked-by: David S. Miller <[email protected]> [sparc]
> Thanks for all the updates
>
> Reviewed-by: Mike Kravetz <[email protected]>

Thanks for all your reviews :)

Alex