[PATCH 0/3] cleanup hugetlb_vmemmap

[PATCH 0/3] cleanup hugetlb_vmemmap

[Muchun Song]

This series is based on next-20220404.

The word of "free" is not expressive enough to express the feature of optimizing
vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze" is
more clear.  In this series, cheanup related codes to make it more clear and
expressive.  This is suggested by David.

Muchun Song (3):
  mm: hugetlb_vmemmap: cleanup hugetlb_vmemmap related functions
  mm: hugetlb_vmemmap: cleanup hugetlb_free_vmemmap_enabled*
  mm: hugetlb_vmemmap: cleanup CONFIG_HUGETLB_PAGE_FREE_VMEMMAP*

 Documentation/admin-guide/kernel-parameters.txt |  4 +-
 Documentation/admin-guide/mm/hugetlbpage.rst    |  2 +-
 arch/arm64/Kconfig                              |  2 +-
 arch/arm64/mm/flush.c                           |  4 +-
 arch/x86/Kconfig                                |  2 +-
 arch/x86/mm/init_64.c                           |  2 +-
 fs/Kconfig                                      | 16 +++----
 include/linux/hugetlb.h                         |  4 +-
 include/linux/mm.h                              |  2 +-
 include/linux/page-flags.h                      | 18 ++++----
 mm/Makefile                                     |  2 +-
 mm/hugetlb.c                                    | 10 ++---
 mm/hugetlb_vmemmap.c                            | 56 ++++++++++++-------------
 mm/hugetlb_vmemmap.h                            | 24 +++++------
 mm/memory_hotplug.c                             |  2 +-
 mm/sparse-vmemmap.c                             |  4 +-
 16 files changed, 76 insertions(+), 78 deletions(-)

-- 
2.11.0

[PATCH 1/3] mm: hugetlb_vmemmap: cleanup hugetlb_vmemmap related functions

[Muchun Song]

The word of "free" is not expressive enough to express the feature of optimizing
vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
And some function names are prefixed with "huge_page" instead of "hugetlb", it is
easily to be confused with THP.  In this patch , cheanup related functions to make
code more clear and expressive.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 include/linux/hugetlb.h |  2 +-
 mm/hugetlb.c            | 10 +++++-----
 mm/hugetlb_vmemmap.c    | 42 ++++++++++++++++++++----------------------
 mm/hugetlb_vmemmap.h    | 20 ++++++++++----------
 4 files changed, 36 insertions(+), 38 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 53c1b6082a4c..c16fbb1228a3 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -618,7 +618,7 @@ struct hstate {
 	unsigned int free_huge_pages_node[MAX_NUMNODES];
 	unsigned int surplus_huge_pages_node[MAX_NUMNODES];
 #ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
-	unsigned int nr_free_vmemmap_pages;
+	unsigned int optimize_vmemmap_pages;
 #endif
 #ifdef CONFIG_CGROUP_HUGETLB
 	/* cgroup control files */
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index dd642cfc538b..1f9fbdddc86b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1540,7 +1540,7 @@ static void __update_and_free_page(struct hstate *h, struct page *page)
 	if (hstate_is_gigantic(h) && !gigantic_page_runtime_supported())
 		return;
 
-	if (alloc_huge_page_vmemmap(h, page)) {
+	if (hugetlb_vmemmap_alloc(h, page)) {
 		spin_lock_irq(&hugetlb_lock);
 		/*
 		 * If we cannot allocate vmemmap pages, just refuse to free the
@@ -1617,7 +1617,7 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn);
 
 static inline void flush_free_hpage_work(struct hstate *h)
 {
-	if (free_vmemmap_pages_per_hpage(h))
+	if (hugetlb_optimize_vmemmap_pages(h))
 		flush_work(&free_hpage_work);
 }
 
@@ -1739,7 +1739,7 @@ static void __prep_account_new_huge_page(struct hstate *h, int nid)
 
 static void __prep_new_huge_page(struct hstate *h, struct page *page)
 {
-	free_huge_page_vmemmap(h, page);
+	hugetlb_vmemmap_free(h, page);
 	INIT_LIST_HEAD(&page->lru);
 	set_compound_page_dtor(page, HUGETLB_PAGE_DTOR);
 	hugetlb_set_page_subpool(page, NULL);
@@ -2112,7 +2112,7 @@ int dissolve_free_huge_page(struct page *page)
 		 * Attempt to allocate vmemmmap here so that we can take
 		 * appropriate action on failure.
 		 */
-		rc = alloc_huge_page_vmemmap(h, head);
+		rc = hugetlb_vmemmap_alloc(h, head);
 		if (!rc) {
 			/*
 			 * Move PageHWPoison flag from head page to the raw
@@ -3427,7 +3427,7 @@ static int demote_free_huge_page(struct hstate *h, struct page *page)
 	remove_hugetlb_page_for_demote(h, page, false);
 	spin_unlock_irq(&hugetlb_lock);
 
-	rc = alloc_huge_page_vmemmap(h, page);
+	rc = hugetlb_vmemmap_alloc(h, page);
 	if (rc) {
 		/* Allocation of vmemmmap failed, we can not demote page */
 		spin_lock_irq(&hugetlb_lock);
diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
index 791626983c2e..91b79b9d9e25 100644
--- a/mm/hugetlb_vmemmap.c
+++ b/mm/hugetlb_vmemmap.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Free some vmemmap pages of HugeTLB
+ * Optimize vmemmap pages associated with HugeTLB
  *
  * Copyright (c) 2020, Bytedance. All rights reserved.
  *
@@ -192,7 +192,7 @@ DEFINE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON,
 			hugetlb_free_vmemmap_enabled_key);
 EXPORT_SYMBOL(hugetlb_free_vmemmap_enabled_key);
 
-static int __init early_hugetlb_free_vmemmap_param(char *buf)
+static int __init hugetlb_vmemmap_early_param(char *buf)
 {
 	/* We cannot optimize if a "struct page" crosses page boundaries. */
 	if (!is_power_of_2(sizeof(struct page))) {
@@ -212,29 +212,26 @@ static int __init early_hugetlb_free_vmemmap_param(char *buf)
 
 	return 0;
 }
-early_param("hugetlb_free_vmemmap", early_hugetlb_free_vmemmap_param);
-
-static inline unsigned long free_vmemmap_pages_size_per_hpage(struct hstate *h)
-{
-	return (unsigned long)free_vmemmap_pages_per_hpage(h) << PAGE_SHIFT;
-}
+early_param("hugetlb_free_vmemmap", hugetlb_vmemmap_early_param);
 
 /*
  * Previously discarded vmemmap pages will be allocated and remapping
  * after this function returns zero.
  */
-int alloc_huge_page_vmemmap(struct hstate *h, struct page *head)
+int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
 {
 	int ret;
 	unsigned long vmemmap_addr = (unsigned long)head;
-	unsigned long vmemmap_end, vmemmap_reuse;
+	unsigned long vmemmap_end, vmemmap_reuse, vmemmap_pages;
 
 	if (!HPageVmemmapOptimized(head))
 		return 0;
 
-	vmemmap_addr += RESERVE_VMEMMAP_SIZE;
-	vmemmap_end = vmemmap_addr + free_vmemmap_pages_size_per_hpage(h);
-	vmemmap_reuse = vmemmap_addr - PAGE_SIZE;
+	vmemmap_addr	+= RESERVE_VMEMMAP_SIZE;
+	vmemmap_pages	= hugetlb_optimize_vmemmap_pages(h);
+	vmemmap_end	= vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
+	vmemmap_reuse	= vmemmap_addr - PAGE_SIZE;
+
 	/*
 	 * The pages which the vmemmap virtual address range [@vmemmap_addr,
 	 * @vmemmap_end) are mapped to are freed to the buddy allocator, and
@@ -250,17 +247,18 @@ int alloc_huge_page_vmemmap(struct hstate *h, struct page *head)
 	return ret;
 }
 
-void free_huge_page_vmemmap(struct hstate *h, struct page *head)
+void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
 {
 	unsigned long vmemmap_addr = (unsigned long)head;
-	unsigned long vmemmap_end, vmemmap_reuse;
+	unsigned long vmemmap_end, vmemmap_reuse, vmemmap_pages;
 
-	if (!free_vmemmap_pages_per_hpage(h))
+	vmemmap_pages = hugetlb_optimize_vmemmap_pages(h);
+	if (!vmemmap_pages)
 		return;
 
-	vmemmap_addr += RESERVE_VMEMMAP_SIZE;
-	vmemmap_end = vmemmap_addr + free_vmemmap_pages_size_per_hpage(h);
-	vmemmap_reuse = vmemmap_addr - PAGE_SIZE;
+	vmemmap_addr	+= RESERVE_VMEMMAP_SIZE;
+	vmemmap_end	= vmemmap_addr + (vmemmap_pages << PAGE_SHIFT);
+	vmemmap_reuse	= vmemmap_addr - PAGE_SIZE;
 
 	/*
 	 * Remap the vmemmap virtual address range [@vmemmap_addr, @vmemmap_end)
@@ -297,8 +295,8 @@ void __init hugetlb_vmemmap_init(struct hstate *h)
 	 * hugetlbpage.rst for more details.
 	 */
 	if (likely(vmemmap_pages > RESERVE_VMEMMAP_NR))
-		h->nr_free_vmemmap_pages = vmemmap_pages - RESERVE_VMEMMAP_NR;
+		h->optimize_vmemmap_pages = vmemmap_pages - RESERVE_VMEMMAP_NR;
 
-	pr_info("can free %d vmemmap pages for %s\n", h->nr_free_vmemmap_pages,
-		h->name);
+	pr_info("can optimize %d vmemmap pages for %s\n",
+		h->optimize_vmemmap_pages, h->name);
 }
diff --git a/mm/hugetlb_vmemmap.h b/mm/hugetlb_vmemmap.h
index cb2bef8f9e73..9760537849b5 100644
--- a/mm/hugetlb_vmemmap.h
+++ b/mm/hugetlb_vmemmap.h
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Free some vmemmap pages of HugeTLB
+ * Optimize vmemmap pages associated with HugeTLB
  *
  * Copyright (c) 2020, Bytedance. All rights reserved.
  *
@@ -11,25 +11,25 @@
 #include <linux/hugetlb.h>
 
 #ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
-int alloc_huge_page_vmemmap(struct hstate *h, struct page *head);
-void free_huge_page_vmemmap(struct hstate *h, struct page *head);
+int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head);
+void hugetlb_vmemmap_free(struct hstate *h, struct page *head);
 void hugetlb_vmemmap_init(struct hstate *h);
 
 /*
- * How many vmemmap pages associated with a HugeTLB page that can be freed
- * to the buddy allocator.
+ * How many vmemmap pages associated with a HugeTLB page that can be
+ * optimized and freed to the buddy allocator.
  */
-static inline unsigned int free_vmemmap_pages_per_hpage(struct hstate *h)
+static inline unsigned int hugetlb_optimize_vmemmap_pages(struct hstate *h)
 {
-	return h->nr_free_vmemmap_pages;
+	return h->optimize_vmemmap_pages;
 }
 #else
-static inline int alloc_huge_page_vmemmap(struct hstate *h, struct page *head)
+static inline int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head)
 {
 	return 0;
 }
 
-static inline void free_huge_page_vmemmap(struct hstate *h, struct page *head)
+static inline void hugetlb_vmemmap_free(struct hstate *h, struct page *head)
 {
 }
 
@@ -37,7 +37,7 @@ static inline void hugetlb_vmemmap_init(struct hstate *h)
 {
 }
 
-static inline unsigned int free_vmemmap_pages_per_hpage(struct hstate *h)
+static inline unsigned int hugetlb_optimize_vmemmap_pages(struct hstate *h)
 {
 	return 0;
 }
-- 
2.11.0

Re: [PATCH 1/3] mm: hugetlb_vmemmap: cleanup hugetlb_vmemmap related functions

[Mike Kravetz]

On 4/4/22 00:46, Muchun Song wrote:
> The word of "free" is not expressive enough to express the feature of optimizing
> vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
> And some function names are prefixed with "huge_page" instead of "hugetlb", it is
> easily to be confused with THP.  In this patch , cheanup related functions to make
> code more clear and expressive.

Some typos in the commit message.  Might be better as:

The keyword "free" is not expressive enough to describe the feature of optimizing vmemmap pages associated with each HugeTLB page.  Rename the
keyword to "optimize".

Some function names are prefixed with "huge_page" instead of "hugetlb".  These can easily be confused with THP.  Change function names to make
code more clear and expressive.

> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  include/linux/hugetlb.h |  2 +-
>  mm/hugetlb.c            | 10 +++++-----
>  mm/hugetlb_vmemmap.c    | 42 ++++++++++++++++++++----------------------
>  mm/hugetlb_vmemmap.h    | 20 ++++++++++----------
>  4 files changed, 36 insertions(+), 38 deletions(-)

Chnages look fine,

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

Re: [PATCH 1/3] mm: hugetlb_vmemmap: cleanup hugetlb_vmemmap related functions

[Mike Kravetz]

On 5/31/22 16:47, Mike Kravetz wrote:
> On 4/4/22 00:46, Muchun Song wrote:
>> The word of "free" is not expressive enough to express the feature of optimizing
>> vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
>> And some function names are prefixed with "huge_page" instead of "hugetlb", it is
>> easily to be confused with THP.  In this patch , cheanup related functions to make
>> code more clear and expressive.
> 
> Some typos in the commit message.  Might be better as:
> 
> The keyword "free" is not expressive enough to describe the feature of optimizing vmemmap pages associated with each HugeTLB page.  Rename the
> keyword to "optimize".
> 
> Some function names are prefixed with "huge_page" instead of "hugetlb".  These can easily be confused with THP.  Change function names to make
> code more clear and expressive.
> 

Sorry for the long lines :(

-- 
Mike Kravetz

Re: [PATCH 1/3] mm: hugetlb_vmemmap: cleanup hugetlb_vmemmap related functions

[David Hildenbrand]

On 04.04.22 09:46, Muchun Song wrote:
> The word of "free" is not expressive enough to express the feature of optimizing
> vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
> And some function names are prefixed with "huge_page" instead of "hugetlb", it is
> easily to be confused with THP.  In this patch , cheanup related functions to make
> code more clear and expressive.

No strong opinion (I remember I kicked of the discussion), but I was
wondering if instead of alloc vs. free we could be using something like
optimize vs. restore/rollback.

E.g., hugetlb_vmemmap_optimize() vs. hugetlb_vmemmap_restore().


Maybe there are other suggestions?

> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  include/linux/hugetlb.h |  2 +-
>  mm/hugetlb.c            | 10 +++++-----
>  mm/hugetlb_vmemmap.c    | 42 ++++++++++++++++++++----------------------
>  mm/hugetlb_vmemmap.h    | 20 ++++++++++----------
>  4 files changed, 36 insertions(+), 38 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 53c1b6082a4c..c16fbb1228a3 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -618,7 +618,7 @@ struct hstate {
>  	unsigned int free_huge_pages_node[MAX_NUMNODES];
>  	unsigned int surplus_huge_pages_node[MAX_NUMNODES];
>  #ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
> -	unsigned int nr_free_vmemmap_pages;
> +	unsigned int optimize_vmemmap_pages;

I suggest converting that into a bool and just calling it

"bool optimize_vmemmap_pages".

You can easily compute what hugetlb_vmemmap_init() at runtime from the
page and RESERVE_VMEMMAP_NR, right?

At least the calculation in alloc_huge_page_vmemmap() and
free_huge_page_vmemmap() become *less* weird for me if the magic value
RESERVE_VMEMMAP_NR isn't used explicitly for vmemmap_addr but implicitly
for vmemmap_end.

>  #endif
>  #ifdef CONFIG_CGROUP_HUGETLB
>  	/* cgroup control files */
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index dd642cfc538b..1f9fbdddc86b 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1540,7 +1540,7 @@ static void __update_and_free_page(struct hstate *h, struct page *page)
>  	if (hstate_is_gigantic(h) && !gigantic_page_runtime_supported())
>  		return;
>  
> -	if (alloc_huge_page_vmemmap(h, page)) {
> +	if (hugetlb_vmemmap_alloc(h, page)) {
>  		spin_lock_irq(&hugetlb_lock);
>  		/*
>  		 * If we cannot allocate vmemmap pages, just refuse to free the
> @@ -1617,7 +1617,7 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn);
>  
>  static inline void flush_free_hpage_work(struct hstate *h)
>  {
> -	if (free_vmemmap_pages_per_hpage(h))
> +	if (hugetlb_optimize_vmemmap_pages(h))

It might be reasonable to call that hugetlb_should_optimize_vmemmap()
then, letting it return a bool.



-- 
Thanks,

David / dhildenb

Re: [PATCH 1/3] mm: hugetlb_vmemmap: cleanup hugetlb_vmemmap related functions

[Muchun Song]

On Wed, Jun 01, 2022 at 11:28:44AM +0200, David Hildenbrand wrote:
> On 04.04.22 09:46, Muchun Song wrote:
> > The word of "free" is not expressive enough to express the feature of optimizing
> > vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
> > And some function names are prefixed with "huge_page" instead of "hugetlb", it is
> > easily to be confused with THP.  In this patch , cheanup related functions to make
> > code more clear and expressive.
> 
> No strong opinion (I remember I kicked of the discussion), but I was
> wondering if instead of alloc vs. free we could be using something like
> optimize vs. restore/rollback.
> 
> E.g., hugetlb_vmemmap_optimize() vs. hugetlb_vmemmap_restore().
>

I think it is a good suggestion.
 
> 
> Maybe there are other suggestions?
>
> > 
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  include/linux/hugetlb.h |  2 +-
> >  mm/hugetlb.c            | 10 +++++-----
> >  mm/hugetlb_vmemmap.c    | 42 ++++++++++++++++++++----------------------
> >  mm/hugetlb_vmemmap.h    | 20 ++++++++++----------
> >  4 files changed, 36 insertions(+), 38 deletions(-)
> > 
> > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> > index 53c1b6082a4c..c16fbb1228a3 100644
> > --- a/include/linux/hugetlb.h
> > +++ b/include/linux/hugetlb.h
> > @@ -618,7 +618,7 @@ struct hstate {
> >  	unsigned int free_huge_pages_node[MAX_NUMNODES];
> >  	unsigned int surplus_huge_pages_node[MAX_NUMNODES];
> >  #ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
> > -	unsigned int nr_free_vmemmap_pages;
> > +	unsigned int optimize_vmemmap_pages;
> 
> I suggest converting that into a bool and just calling it
> 
> "bool optimize_vmemmap_pages".
> 
> You can easily compute what hugetlb_vmemmap_init() at runtime from the
> page and RESERVE_VMEMMAP_NR, right?
> 

Right. A little overhead, but hugetlb_vmemmap_alloc() is not hot path,
maybe we can accept the increased overhead of calculating at runtime.

Hi Mike,

Do you have any objections on this? If no, I think we can do this in a
separate patch.

> At least the calculation in alloc_huge_page_vmemmap() and
> free_huge_page_vmemmap() become *less* weird for me if the magic value
> RESERVE_VMEMMAP_NR isn't used explicitly for vmemmap_addr but implicitly
> for vmemmap_end.
> 
> >  #endif
> >  #ifdef CONFIG_CGROUP_HUGETLB
> >  	/* cgroup control files */
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index dd642cfc538b..1f9fbdddc86b 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -1540,7 +1540,7 @@ static void __update_and_free_page(struct hstate *h, struct page *page)
> >  	if (hstate_is_gigantic(h) && !gigantic_page_runtime_supported())
> >  		return;
> >  
> > -	if (alloc_huge_page_vmemmap(h, page)) {
> > +	if (hugetlb_vmemmap_alloc(h, page)) {
> >  		spin_lock_irq(&hugetlb_lock);
> >  		/*
> >  		 * If we cannot allocate vmemmap pages, just refuse to free the
> > @@ -1617,7 +1617,7 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn);
> >  
> >  static inline void flush_free_hpage_work(struct hstate *h)
> >  {
> > -	if (free_vmemmap_pages_per_hpage(h))
> > +	if (hugetlb_optimize_vmemmap_pages(h))
> 
> It might be reasonable to call that hugetlb_should_optimize_vmemmap()
> then, letting it return a bool.
>

How about the name of "hugetlb_vmemmap_optimizable()"?  "should" seems to
tell the user that this hugetlb should be optimized, however, optimization
also depends on "hugetlb_free_vmemmap=on".  "optimizable" seems to be more
appropriate, right?

Thanks.  

Re: [PATCH 1/3] mm: hugetlb_vmemmap: cleanup hugetlb_vmemmap related functions

[David Hildenbrand]

>>>  #endif
>>>  #ifdef CONFIG_CGROUP_HUGETLB
>>>  	/* cgroup control files */
>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>> index dd642cfc538b..1f9fbdddc86b 100644
>>> --- a/mm/hugetlb.c
>>> +++ b/mm/hugetlb.c
>>> @@ -1540,7 +1540,7 @@ static void __update_and_free_page(struct hstate *h, struct page *page)
>>>  	if (hstate_is_gigantic(h) && !gigantic_page_runtime_supported())
>>>  		return;
>>>  
>>> -	if (alloc_huge_page_vmemmap(h, page)) {
>>> +	if (hugetlb_vmemmap_alloc(h, page)) {
>>>  		spin_lock_irq(&hugetlb_lock);
>>>  		/*
>>>  		 * If we cannot allocate vmemmap pages, just refuse to free the
>>> @@ -1617,7 +1617,7 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn);
>>>  
>>>  static inline void flush_free_hpage_work(struct hstate *h)
>>>  {
>>> -	if (free_vmemmap_pages_per_hpage(h))
>>> +	if (hugetlb_optimize_vmemmap_pages(h))
>>
>> It might be reasonable to call that hugetlb_should_optimize_vmemmap()
>> then, letting it return a bool.
>>
> 
> How about the name of "hugetlb_vmemmap_optimizable()"?  "should" seems to
> tell the user that this hugetlb should be optimized, however, optimization
> also depends on "hugetlb_free_vmemmap=on".  "optimizable" seems to be more
> appropriate, right?

No strong opinion. Either is clearer to me compared to what we have
right now :)


-- 
Thanks,

David / dhildenb

[PATCH 2/3] mm: hugetlb_vmemmap: cleanup hugetlb_free_vmemmap_enabled*

[Muchun Song]

The word of "free" is not expressive enough to express the feature of optimizing
vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
In this patch , cheanup the static key and hugetlb_free_vmemmap_enabled() to make
code more expressive.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 arch/arm64/mm/flush.c      |  2 +-
 include/linux/page-flags.h | 12 ++++++------
 mm/hugetlb_vmemmap.c       | 10 +++++-----
 mm/memory_hotplug.c        |  2 +-
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
index 1efd01e10cba..d19a13234a81 100644
--- a/arch/arm64/mm/flush.c
+++ b/arch/arm64/mm/flush.c
@@ -85,7 +85,7 @@ void flush_dcache_page(struct page *page)
 	 * set since the head vmemmap page frame is reused (more details can
 	 * refer to the comments above page_fixed_fake_head()).
 	 */
-	if (hugetlb_free_vmemmap_enabled() && PageHuge(page))
+	if (hugetlb_optimize_vmemmap_enabled() && PageHuge(page))
 		page = compound_head(page);
 
 	if (test_bit(PG_dcache_clean, &page->flags))
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 9f488668a1d7..557d15ef3dc0 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -201,16 +201,16 @@ enum pageflags {
 
 #ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
 DECLARE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON,
-			 hugetlb_free_vmemmap_enabled_key);
+			 hugetlb_optimize_vmemmap_key);
 
-static __always_inline bool hugetlb_free_vmemmap_enabled(void)
+static __always_inline bool hugetlb_optimize_vmemmap_enabled(void)
 {
 	return static_branch_maybe(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON,
-				   &hugetlb_free_vmemmap_enabled_key);
+				   &hugetlb_optimize_vmemmap_key);
 }
 
 /*
- * If the feature of freeing some vmemmap pages associated with each HugeTLB
+ * If the feature of optimizing vmemmap pages associated with each HugeTLB
  * page is enabled, the head vmemmap page frame is reused and all of the tail
  * vmemmap addresses map to the head vmemmap page frame (furture details can
  * refer to the figure at the head of the mm/hugetlb_vmemmap.c).  In other
@@ -227,7 +227,7 @@ static __always_inline bool hugetlb_free_vmemmap_enabled(void)
  */
 static __always_inline const struct page *page_fixed_fake_head(const struct page *page)
 {
-	if (!hugetlb_free_vmemmap_enabled())
+	if (!hugetlb_optimize_vmemmap_enabled())
 		return page;
 
 	/*
@@ -256,7 +256,7 @@ static inline const struct page *page_fixed_fake_head(const struct page *page)
 	return page;
 }
 
-static inline bool hugetlb_free_vmemmap_enabled(void)
+static inline bool hugetlb_optimize_vmemmap_enabled(void)
 {
 	return false;
 }
diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
index 91b79b9d9e25..f25294973398 100644
--- a/mm/hugetlb_vmemmap.c
+++ b/mm/hugetlb_vmemmap.c
@@ -189,8 +189,8 @@
 #define RESERVE_VMEMMAP_SIZE		(RESERVE_VMEMMAP_NR << PAGE_SHIFT)
 
 DEFINE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON,
-			hugetlb_free_vmemmap_enabled_key);
-EXPORT_SYMBOL(hugetlb_free_vmemmap_enabled_key);
+			hugetlb_optimize_vmemmap_key);
+EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key);
 
 static int __init hugetlb_vmemmap_early_param(char *buf)
 {
@@ -204,9 +204,9 @@ static int __init hugetlb_vmemmap_early_param(char *buf)
 		return -EINVAL;
 
 	if (!strcmp(buf, "on"))
-		static_branch_enable(&hugetlb_free_vmemmap_enabled_key);
+		static_branch_enable(&hugetlb_optimize_vmemmap_key);
 	else if (!strcmp(buf, "off"))
-		static_branch_disable(&hugetlb_free_vmemmap_enabled_key);
+		static_branch_disable(&hugetlb_optimize_vmemmap_key);
 	else
 		return -EINVAL;
 
@@ -282,7 +282,7 @@ void __init hugetlb_vmemmap_init(struct hstate *h)
 	BUILD_BUG_ON(__NR_USED_SUBPAGE >=
 		     RESERVE_VMEMMAP_SIZE / sizeof(struct page));
 
-	if (!hugetlb_free_vmemmap_enabled())
+	if (!hugetlb_optimize_vmemmap_enabled())
 		return;
 
 	vmemmap_pages = (nr_pages * sizeof(struct page)) >> PAGE_SHIFT;
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 3fb4196094d9..74430f88853d 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1273,7 +1273,7 @@ bool mhp_supports_memmap_on_memory(unsigned long size)
 	 *       populate a single PMD.
 	 */
 	return memmap_on_memory &&
-	       !hugetlb_free_vmemmap_enabled() &&
+	       !hugetlb_optimize_vmemmap_enabled() &&
 	       IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
 	       size == memory_block_size_bytes() &&
 	       IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
-- 
2.11.0

Re: [PATCH 2/3] mm: hugetlb_vmemmap: cleanup hugetlb_free_vmemmap_enabled*

[Mike Kravetz]

On 4/4/22 00:46, Muchun Song wrote:
> The word of "free" is not expressive enough to express the feature of optimizing
> vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
> In this patch , cheanup the static key and hugetlb_free_vmemmap_enabled() to make
> code more expressive.

Commit message might look better as:

The keyword "free" is not expressive enough to describe the feature of
optimizing vmemmap pages associated with each HugeTLB page.  Rename the
keyword to "optimize".

Rename the static key and hugetlb_free_vmemmap_enabled() to make the code
more expressive.

> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  arch/arm64/mm/flush.c      |  2 +-
>  include/linux/page-flags.h | 12 ++++++------
>  mm/hugetlb_vmemmap.c       | 10 +++++-----
>  mm/memory_hotplug.c        |  2 +-
>  4 files changed, 13 insertions(+), 13 deletions(-)

Code changes look fine,

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

Re: [PATCH 2/3] mm: hugetlb_vmemmap: cleanup hugetlb_free_vmemmap_enabled*

[David Hildenbrand]

On 04.04.22 09:46, Muchun Song wrote:
> The word of "free" is not expressive enough to express the feature of optimizing
> vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
> In this patch , cheanup the static key and hugetlb_free_vmemmap_enabled() to make
> code more expressive.
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH 3/3] mm: hugetlb_vmemmap: cleanup CONFIG_HUGETLB_PAGE_FREE_VMEMMAP*

[Muchun Song]

The word of "free" is not expressive enough to express the feature of optimizing
vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
In this patch , cheanup configs to make code more expressive.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  4 ++--
 Documentation/admin-guide/mm/hugetlbpage.rst    |  2 +-
 arch/arm64/Kconfig                              |  2 +-
 arch/arm64/mm/flush.c                           |  2 +-
 arch/x86/Kconfig                                |  2 +-
 arch/x86/mm/init_64.c                           |  2 +-
 fs/Kconfig                                      | 16 ++++++++--------
 include/linux/hugetlb.h                         |  2 +-
 include/linux/mm.h                              |  2 +-
 include/linux/page-flags.h                      |  6 +++---
 mm/Makefile                                     |  2 +-
 mm/hugetlb_vmemmap.c                            |  4 ++--
 mm/hugetlb_vmemmap.h                            |  4 ++--
 mm/sparse-vmemmap.c                             |  4 ++--
 14 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 56edbe3f458f..89a58ac49d5f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1660,7 +1660,7 @@
 			Format: size[KMG]
 
 	hugetlb_free_vmemmap=
-			[KNL] Reguires CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
+			[KNL] Reguires CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
 			enabled.
 			Allows heavy hugetlb users to free up some more
 			memory (7 * PAGE_SIZE for each 2MB hugetlb page).
@@ -1669,7 +1669,7 @@
 			on:  enable the feature
 			off: disable the feature
 
-			Built with CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON=y,
+			Built with CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON=y,
 			the default is on.
 
 			This is not compatible with memory_hotplug.memmap_on_memory.
diff --git a/Documentation/admin-guide/mm/hugetlbpage.rst b/Documentation/admin-guide/mm/hugetlbpage.rst
index 0166f9de3428..a90330d0a837 100644
--- a/Documentation/admin-guide/mm/hugetlbpage.rst
+++ b/Documentation/admin-guide/mm/hugetlbpage.rst
@@ -164,7 +164,7 @@ default_hugepagesz
 	will all result in 256 2M huge pages being allocated.  Valid default
 	huge page size is architecture dependent.
 hugetlb_free_vmemmap
-	When CONFIG_HUGETLB_PAGE_FREE_VMEMMAP is set, this enables freeing
+	When CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP is set, this enables optimizing
 	unused vmemmap pages associated with each HugeTLB page.
 
 When multiple huge page sizes are supported, ``/proc/sys/vm/nr_hugepages``
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index bb7fc91d14f4..42ff6d3dbfb5 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -96,7 +96,7 @@ config ARM64
 	select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
 	select ARCH_WANT_FRAME_POINTERS
 	select ARCH_WANT_HUGE_PMD_SHARE if ARM64_4K_PAGES || (ARM64_16K_PAGES && !ARM64_VA_BITS_36)
-	select ARCH_WANT_HUGETLB_PAGE_FREE_VMEMMAP
+	select ARCH_WANT_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
 	select ARCH_WANT_LD_ORPHAN_WARN
 	select ARCH_WANTS_NO_INSTR
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
index d19a13234a81..9e39598bbc21 100644
--- a/arch/arm64/mm/flush.c
+++ b/arch/arm64/mm/flush.c
@@ -78,7 +78,7 @@ void flush_dcache_page(struct page *page)
 	/*
 	 * Only the head page's flags of HugeTLB can be cleared since the tail
 	 * vmemmap pages associated with each HugeTLB page are mapped with
-	 * read-only when CONFIG_HUGETLB_PAGE_FREE_VMEMMAP is enabled (more
+	 * read-only when CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP is enabled (more
 	 * details can refer to vmemmap_remap_pte()).  Although
 	 * __sync_icache_dcache() only set PG_dcache_clean flag on the head
 	 * page struct, some tail page structs still can be seen the flag is
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index a4cc8dd871f2..7ec9a91e2386 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -121,7 +121,7 @@ config X86
 	select ARCH_WANTS_NO_INSTR
 	select ARCH_WANT_GENERAL_HUGETLB
 	select ARCH_WANT_HUGE_PMD_SHARE
-	select ARCH_WANT_HUGETLB_PAGE_FREE_VMEMMAP	if X86_64
+	select ARCH_WANT_HUGETLB_PAGE_OPTIMIZE_VMEMMAP	if X86_64
 	select ARCH_WANT_LD_ORPHAN_WARN
 	select ARCH_WANTS_THP_SWAP		if X86_64
 	select ARCH_HAS_PARANOID_L1D_FLUSH
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 96d34ebb20a9..af34dd1510d6 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -1269,7 +1269,7 @@ static struct kcore_list kcore_vsyscall;
 
 static void __init register_page_bootmem_info(void)
 {
-#if defined(CONFIG_NUMA) || defined(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP)
+#if defined(CONFIG_NUMA) || defined(CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP)
 	int i;
 
 	for_each_online_node(i)
diff --git a/fs/Kconfig b/fs/Kconfig
index 8f6ab1cf5115..5976eb33535f 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -250,22 +250,22 @@ config HUGETLB_PAGE
 # to enable the feature of minimizing overhead of struct page associated with
 # each HugeTLB page.
 #
-config ARCH_WANT_HUGETLB_PAGE_FREE_VMEMMAP
+config ARCH_WANT_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
 	bool
 
-config HUGETLB_PAGE_FREE_VMEMMAP
+config HUGETLB_PAGE_OPTIMIZE_VMEMMAP
 	def_bool HUGETLB_PAGE
-	depends on ARCH_WANT_HUGETLB_PAGE_FREE_VMEMMAP
+	depends on ARCH_WANT_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
 	depends on SPARSEMEM_VMEMMAP
 
-config HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON
-	bool "Default freeing vmemmap pages of HugeTLB to on"
+config HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON
+	bool "Default optimizing vmemmap pages of HugeTLB to on"
 	default n
-	depends on HUGETLB_PAGE_FREE_VMEMMAP
+	depends on HUGETLB_PAGE_OPTIMIZE_VMEMMAP
 	help
-	  When using HUGETLB_PAGE_FREE_VMEMMAP, the freeing unused vmemmap
+	  When using HUGETLB_PAGE_OPTIMIZE_VMEMMAP, the optimizing unused vmemmap
 	  pages associated with each HugeTLB page is default off. Say Y here
-	  to enable freeing vmemmap pages of HugeTLB by default. It can then
+	  to enable optimizing vmemmap pages of HugeTLB by default. It can then
 	  be disabled on the command line via hugetlb_free_vmemmap=off.
 
 config MEMFD_CREATE
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index c16fbb1228a3..c40dda4daf51 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -617,7 +617,7 @@ struct hstate {
 	unsigned int nr_huge_pages_node[MAX_NUMNODES];
 	unsigned int free_huge_pages_node[MAX_NUMNODES];
 	unsigned int surplus_huge_pages_node[MAX_NUMNODES];
-#ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
+#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
 	unsigned int optimize_vmemmap_pages;
 #endif
 #ifdef CONFIG_CGROUP_HUGETLB
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 26428ff262fc..75f7d309a9cf 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3188,7 +3188,7 @@ static inline void print_vma_addr(char *prefix, unsigned long rip)
 }
 #endif
 
-#ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
+#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
 int vmemmap_remap_free(unsigned long start, unsigned long end,
 		       unsigned long reuse);
 int vmemmap_remap_alloc(unsigned long start, unsigned long end,
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 557d15ef3dc0..b70124b9c7c1 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -199,13 +199,13 @@ enum pageflags {
 
 #ifndef __GENERATING_BOUNDS_H
 
-#ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
-DECLARE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON,
+#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
+DECLARE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON,
 			 hugetlb_optimize_vmemmap_key);
 
 static __always_inline bool hugetlb_optimize_vmemmap_enabled(void)
 {
-	return static_branch_maybe(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON,
+	return static_branch_maybe(CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON,
 				   &hugetlb_optimize_vmemmap_key);
 }
 
diff --git a/mm/Makefile b/mm/Makefile
index 4cc13f3179a5..6f9ffa968a1a 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -77,7 +77,7 @@ obj-$(CONFIG_FRONTSWAP)	+= frontswap.o
 obj-$(CONFIG_ZSWAP)	+= zswap.o
 obj-$(CONFIG_HAS_DMA)	+= dmapool.o
 obj-$(CONFIG_HUGETLBFS)	+= hugetlb.o
-obj-$(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP)	+= hugetlb_vmemmap.o
+obj-$(CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP)	+= hugetlb_vmemmap.o
 obj-$(CONFIG_NUMA) 	+= mempolicy.o
 obj-$(CONFIG_SPARSEMEM)	+= sparse.o
 obj-$(CONFIG_SPARSEMEM_VMEMMAP) += sparse-vmemmap.o
diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
index f25294973398..2655434a946b 100644
--- a/mm/hugetlb_vmemmap.c
+++ b/mm/hugetlb_vmemmap.c
@@ -188,7 +188,7 @@
 #define RESERVE_VMEMMAP_NR		1U
 #define RESERVE_VMEMMAP_SIZE		(RESERVE_VMEMMAP_NR << PAGE_SHIFT)
 
-DEFINE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON,
+DEFINE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON,
 			hugetlb_optimize_vmemmap_key);
 EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key);
 
@@ -276,7 +276,7 @@ void __init hugetlb_vmemmap_init(struct hstate *h)
 
 	/*
 	 * There are only (RESERVE_VMEMMAP_SIZE / sizeof(struct page)) struct
-	 * page structs that can be used when CONFIG_HUGETLB_PAGE_FREE_VMEMMAP,
+	 * page structs that can be used when CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP,
 	 * so add a BUILD_BUG_ON to catch invalid usage of the tail struct page.
 	 */
 	BUILD_BUG_ON(__NR_USED_SUBPAGE >=
diff --git a/mm/hugetlb_vmemmap.h b/mm/hugetlb_vmemmap.h
index 9760537849b5..109b0a53b6fe 100644
--- a/mm/hugetlb_vmemmap.h
+++ b/mm/hugetlb_vmemmap.h
@@ -10,7 +10,7 @@
 #define _LINUX_HUGETLB_VMEMMAP_H
 #include <linux/hugetlb.h>
 
-#ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
+#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
 int hugetlb_vmemmap_alloc(struct hstate *h, struct page *head);
 void hugetlb_vmemmap_free(struct hstate *h, struct page *head);
 void hugetlb_vmemmap_init(struct hstate *h);
@@ -41,5 +41,5 @@ static inline unsigned int hugetlb_optimize_vmemmap_pages(struct hstate *h)
 {
 	return 0;
 }
-#endif /* CONFIG_HUGETLB_PAGE_FREE_VMEMMAP */
+#endif /* CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP */
 #endif /* _LINUX_HUGETLB_VMEMMAP_H */
diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
index 8aecd6b3896c..52f36527bab3 100644
--- a/mm/sparse-vmemmap.c
+++ b/mm/sparse-vmemmap.c
@@ -34,7 +34,7 @@
 #include <asm/pgalloc.h>
 #include <asm/tlbflush.h>
 
-#ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
+#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
 /**
  * struct vmemmap_remap_walk - walk vmemmap page table
  *
@@ -420,7 +420,7 @@ int vmemmap_remap_alloc(unsigned long start, unsigned long end,
 
 	return 0;
 }
-#endif /* CONFIG_HUGETLB_PAGE_FREE_VMEMMAP */
+#endif /* CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP */
 
 /*
  * Allocate a block of memory to be used to back the virtual memory map
-- 
2.11.0

Re: [PATCH 3/3] mm: hugetlb_vmemmap: cleanup CONFIG_HUGETLB_PAGE_FREE_VMEMMAP*

[Vlastimil Babka]

On 4/4/22 09:46, Muchun Song wrote:
> --- a/mm/hugetlb_vmemmap.c
> +++ b/mm/hugetlb_vmemmap.c
> @@ -188,7 +188,7 @@
>  #define RESERVE_VMEMMAP_NR		1U
>  #define RESERVE_VMEMMAP_SIZE		(RESERVE_VMEMMAP_NR << PAGE_SHIFT)
>  
> -DEFINE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON,
> +DEFINE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON,
>  			hugetlb_optimize_vmemmap_key);
>  EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key);

Hi,

looks like just below here you forgot to update:

static enum vmemmap_optimize_mode vmemmap_optimize_mode =
        IS_ENABLED(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON);

so it refers to CONFIG name that no longer exists?

Vlastimil

Re: [PATCH 3/3] mm: hugetlb_vmemmap: cleanup CONFIG_HUGETLB_PAGE_FREE_VMEMMAP*

[Muchun Song]

On Fri, May 27, 2022 at 09:39:41AM +0200, Vlastimil Babka wrote:
> On 4/4/22 09:46, Muchun Song wrote:
> > --- a/mm/hugetlb_vmemmap.c
> > +++ b/mm/hugetlb_vmemmap.c
> > @@ -188,7 +188,7 @@
> >  #define RESERVE_VMEMMAP_NR		1U
> >  #define RESERVE_VMEMMAP_SIZE		(RESERVE_VMEMMAP_NR << PAGE_SHIFT)
> >  
> > -DEFINE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON,
> > +DEFINE_STATIC_KEY_MAYBE(CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON,
> >  			hugetlb_optimize_vmemmap_key);
> >  EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key);
> 
> Hi,
> 
> looks like just below here you forgot to update:
> 
> static enum vmemmap_optimize_mode vmemmap_optimize_mode =
>         IS_ENABLED(CONFIG_HUGETLB_PAGE_FREE_VMEMMAP_DEFAULT_ON);
>

Good catch!

> so it refers to CONFIG name that no longer exists?
>

Right. My bad. I forgot to replace it.  Will fix it ASAP.

Thanks.
 

Re: [PATCH 3/3] mm: hugetlb_vmemmap: cleanup CONFIG_HUGETLB_PAGE_FREE_VMEMMAP*

[Mike Kravetz]

On 4/4/22 00:46, Muchun Song wrote:
> The word of "free" is not expressive enough to express the feature of optimizing
> vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
> In this patch , cheanup configs to make code more expressive.

Commit message might look better as:

The keyword "free" is not expressive enough to describe the feature of
optimizing vmemmap pages associated with each HugeTLB page.  Rename the
keyword to "optimize".

Change configs to make code more expressive.

> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  4 ++--
>  Documentation/admin-guide/mm/hugetlbpage.rst    |  2 +-
>  arch/arm64/Kconfig                              |  2 +-
>  arch/arm64/mm/flush.c                           |  2 +-
>  arch/x86/Kconfig                                |  2 +-
>  arch/x86/mm/init_64.c                           |  2 +-
>  fs/Kconfig                                      | 16 ++++++++--------
>  include/linux/hugetlb.h                         |  2 +-
>  include/linux/mm.h                              |  2 +-
>  include/linux/page-flags.h                      |  6 +++---
>  mm/Makefile                                     |  2 +-
>  mm/hugetlb_vmemmap.c                            |  4 ++--
>  mm/hugetlb_vmemmap.h                            |  4 ++--
>  mm/sparse-vmemmap.c                             |  4 ++--
>  14 files changed, 27 insertions(+), 27 deletions(-)

I see the issue found by Vlastimil is already fixed here [1].  With that
this looks good.

[1] https://lore.kernel.org/linux-mm/20220527081948.68832-1-songmuchun@bytedance.com/

Acked-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

Re: [PATCH 3/3] mm: hugetlb_vmemmap: cleanup CONFIG_HUGETLB_PAGE_FREE_VMEMMAP*

[David Hildenbrand]

On 04.04.22 09:46, Muchun Song wrote:
> The word of "free" is not expressive enough to express the feature of optimizing
> vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
> In this patch , cheanup configs to make code more expressive.

Nit: why not simply CONFIG_HUGETLB_OPTIMIZE_VMEMMAP if we're touching
this already? At least I don't see value in the additional "PAGE" :)

> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  4 ++--
>  Documentation/admin-guide/mm/hugetlbpage.rst    |  2 +-
>  arch/arm64/Kconfig                              |  2 +-
>  arch/arm64/mm/flush.c                           |  2 +-
>  arch/x86/Kconfig                                |  2 +-
>  arch/x86/mm/init_64.c                           |  2 +-
>  fs/Kconfig                                      | 16 ++++++++--------
>  include/linux/hugetlb.h                         |  2 +-
>  include/linux/mm.h                              |  2 +-
>  include/linux/page-flags.h                      |  6 +++---
>  mm/Makefile                                     |  2 +-
>  mm/hugetlb_vmemmap.c                            |  4 ++--
>  mm/hugetlb_vmemmap.h                            |  4 ++--
>  mm/sparse-vmemmap.c                             |  4 ++--
>  14 files changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 56edbe3f458f..89a58ac49d5f 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1660,7 +1660,7 @@
>  			Format: size[KMG]
>  
>  	hugetlb_free_vmemmap=

We didn't change the parameter name in #2, correct?


-- 
Thanks,

David / dhildenb

Re: [PATCH 3/3] mm: hugetlb_vmemmap: cleanup CONFIG_HUGETLB_PAGE_FREE_VMEMMAP*

[Muchun Song]

On Wed, Jun 01, 2022 at 11:32:37AM +0200, David Hildenbrand wrote:
> On 04.04.22 09:46, Muchun Song wrote:
> > The word of "free" is not expressive enough to express the feature of optimizing
> > vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
> > In this patch , cheanup configs to make code more expressive.
> 
> Nit: why not simply CONFIG_HUGETLB_OPTIMIZE_VMEMMAP if we're touching
> this already? At least I don't see value in the additional "PAGE" :)
>

I thought it keep consistent with CONFIG_HUGETLB_PAGE.  If you think
CONFIG_HUGETLB_OPTIMIZE_VMEMMAP is a better name, maybe we need to
another separate patch since this series is already on mm-stable branch.
 
> > 
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt |  4 ++--
> >  Documentation/admin-guide/mm/hugetlbpage.rst    |  2 +-
> >  arch/arm64/Kconfig                              |  2 +-
> >  arch/arm64/mm/flush.c                           |  2 +-
> >  arch/x86/Kconfig                                |  2 +-
> >  arch/x86/mm/init_64.c                           |  2 +-
> >  fs/Kconfig                                      | 16 ++++++++--------
> >  include/linux/hugetlb.h                         |  2 +-
> >  include/linux/mm.h                              |  2 +-
> >  include/linux/page-flags.h                      |  6 +++---
> >  mm/Makefile                                     |  2 +-
> >  mm/hugetlb_vmemmap.c                            |  4 ++--
> >  mm/hugetlb_vmemmap.h                            |  4 ++--
> >  mm/sparse-vmemmap.c                             |  4 ++--
> >  14 files changed, 27 insertions(+), 27 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 56edbe3f458f..89a58ac49d5f 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -1660,7 +1660,7 @@
> >  			Format: size[KMG]
> >  
> >  	hugetlb_free_vmemmap=
> 
> We didn't change the parameter name in #2, correct?
>

I think this is a interface to users. Is it correct to
change it?

Thanks.

Re: [PATCH 3/3] mm: hugetlb_vmemmap: cleanup CONFIG_HUGETLB_PAGE_FREE_VMEMMAP*

[David Hildenbrand]

On 01.06.22 11:54, Muchun Song wrote:
> On Wed, Jun 01, 2022 at 11:32:37AM +0200, David Hildenbrand wrote:
>> On 04.04.22 09:46, Muchun Song wrote:
>>> The word of "free" is not expressive enough to express the feature of optimizing
>>> vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
>>> In this patch , cheanup configs to make code more expressive.
>>
>> Nit: why not simply CONFIG_HUGETLB_OPTIMIZE_VMEMMAP if we're touching
>> this already? At least I don't see value in the additional "PAGE" :)
>>
> 
> I thought it keep consistent with CONFIG_HUGETLB_PAGE.  If you think
> CONFIG_HUGETLB_OPTIMIZE_VMEMMAP is a better name, maybe we need to
> another separate patch since this series is already on mm-stable branch.

I see, makes sense then.

>>>
>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>> ---
>>>  Documentation/admin-guide/kernel-parameters.txt |  4 ++--
>>>  Documentation/admin-guide/mm/hugetlbpage.rst    |  2 +-
>>>  arch/arm64/Kconfig                              |  2 +-
>>>  arch/arm64/mm/flush.c                           |  2 +-
>>>  arch/x86/Kconfig                                |  2 +-
>>>  arch/x86/mm/init_64.c                           |  2 +-
>>>  fs/Kconfig                                      | 16 ++++++++--------
>>>  include/linux/hugetlb.h                         |  2 +-
>>>  include/linux/mm.h                              |  2 +-
>>>  include/linux/page-flags.h                      |  6 +++---
>>>  mm/Makefile                                     |  2 +-
>>>  mm/hugetlb_vmemmap.c                            |  4 ++--
>>>  mm/hugetlb_vmemmap.h                            |  4 ++--
>>>  mm/sparse-vmemmap.c                             |  4 ++--
>>>  14 files changed, 27 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>> index 56edbe3f458f..89a58ac49d5f 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>> @@ -1660,7 +1660,7 @@
>>>  			Format: size[KMG]
>>>  
>>>  	hugetlb_free_vmemmap=
>>
>> We didn't change the parameter name in #2, correct?
>>
> 
> I think this is a interface to users. Is it correct to
> change it?

I guess so ... I was just wanted to confirm that this is still correct :)

-- 
Thanks,

David / dhildenb

Re: [PATCH 3/3] mm: hugetlb_vmemmap: cleanup CONFIG_HUGETLB_PAGE_FREE_VMEMMAP*

[Muchun Song]

On Wed, Jun 01, 2022 at 12:10:29PM +0200, David Hildenbrand wrote:
> On 01.06.22 11:54, Muchun Song wrote:
> > On Wed, Jun 01, 2022 at 11:32:37AM +0200, David Hildenbrand wrote:
> >> On 04.04.22 09:46, Muchun Song wrote:
> >>> The word of "free" is not expressive enough to express the feature of optimizing
> >>> vmemmap pages associated with each HugeTLB, rename this keywork to "optimeze".
> >>> In this patch , cheanup configs to make code more expressive.
> >>
> >> Nit: why not simply CONFIG_HUGETLB_OPTIMIZE_VMEMMAP if we're touching
> >> this already? At least I don't see value in the additional "PAGE" :)
> >>
> > 
> > I thought it keep consistent with CONFIG_HUGETLB_PAGE.  If you think
> > CONFIG_HUGETLB_OPTIMIZE_VMEMMAP is a better name, maybe we need to
> > another separate patch since this series is already on mm-stable branch.
> 
> I see, makes sense then.
> 
> >>>
> >>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> >>> ---
> >>>  Documentation/admin-guide/kernel-parameters.txt |  4 ++--
> >>>  Documentation/admin-guide/mm/hugetlbpage.rst    |  2 +-
> >>>  arch/arm64/Kconfig                              |  2 +-
> >>>  arch/arm64/mm/flush.c                           |  2 +-
> >>>  arch/x86/Kconfig                                |  2 +-
> >>>  arch/x86/mm/init_64.c                           |  2 +-
> >>>  fs/Kconfig                                      | 16 ++++++++--------
> >>>  include/linux/hugetlb.h                         |  2 +-
> >>>  include/linux/mm.h                              |  2 +-
> >>>  include/linux/page-flags.h                      |  6 +++---
> >>>  mm/Makefile                                     |  2 +-
> >>>  mm/hugetlb_vmemmap.c                            |  4 ++--
> >>>  mm/hugetlb_vmemmap.h                            |  4 ++--
> >>>  mm/sparse-vmemmap.c                             |  4 ++--
> >>>  14 files changed, 27 insertions(+), 27 deletions(-)
> >>>
> >>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> >>> index 56edbe3f458f..89a58ac49d5f 100644
> >>> --- a/Documentation/admin-guide/kernel-parameters.txt
> >>> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >>> @@ -1660,7 +1660,7 @@
> >>>  			Format: size[KMG]
> >>>  
> >>>  	hugetlb_free_vmemmap=
> >>
> >> We didn't change the parameter name in #2, correct?
> >>
> > 
> > I think this is a interface to users. Is it correct to
> > change it?
> 
> I guess so ... I was just wanted to confirm that this is still correct :)
>

Well, I think it is not correct to change it since it
could change behavior to users. :-)

Thanks.