[PATCH 00/10] A few cleanup patches for hugetlb

[PATCH 00/10] A few cleanup patches for hugetlb

[Miaohe Lin]

Hi everyone,
This series contains a few cleanup patches to use helper functions to
simplify the codes, remove unneeded nid parameter and so on. More
details can be found in the respective changelogs.
Thanks!

Miaohe Lin (10):
  hugetlb: make hugetlb_cma_check() static
  hugetlb: Use helper macro SZ_1K
  hugetlb: Use LIST_HEAD() to define a list head
  hugetlb: Use sizeof() to get the array size
  hugetlb: Use helper {huge_pte|pmd}_lock()
  hugetlb: pass NULL to kobj_to_hstate() if nid is unused
  hugetlb: kill hugetlbfs_pagecache_page()
  hugetlb: remove unneeded SetHPageVmemmapOptimized()
  hugetlb: remove meaningless BUG_ON(huge_pte_none())
  hugetlb: make hugetlb selects SYSFS if !SYSCTL

 fs/Kconfig              |  1 +
 include/linux/hugetlb.h |  4 ----
 mm/hugetlb.c            | 52 +++++++++++++++--------------------------
 3 files changed, 20 insertions(+), 37 deletions(-)

-- 
2.23.0

[PATCH 01/10] hugetlb: make hugetlb_cma_check() static

[Miaohe Lin]

Make hugetlb_cma_check() static as it's only used inside mm/hugetlb.c.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 include/linux/hugetlb.h |  4 ----
 mm/hugetlb.c            | 10 +++++++++-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 852f911d676e..c2abbd7950de 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -1155,14 +1155,10 @@ static inline spinlock_t *huge_pte_lock(struct hstate *h,
 
 #if defined(CONFIG_HUGETLB_PAGE) && defined(CONFIG_CMA)
 extern void __init hugetlb_cma_reserve(int order);
-extern void __init hugetlb_cma_check(void);
 #else
 static inline __init void hugetlb_cma_reserve(int order)
 {
 }
-static inline __init void hugetlb_cma_check(void)
-{
-}
 #endif
 
 bool want_pmd_share(struct vm_area_struct *vma, unsigned long addr);
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index f1dc80f47dd6..10d84abb1ca7 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4058,6 +4058,14 @@ static void hugetlb_register_all_nodes(void) { }
 
 #endif
 
+#ifdef CONFIG_CMA
+static void __init hugetlb_cma_check(void);
+#else
+static inline __init void hugetlb_cma_check(void)
+{
+}
+#endif
+
 static int __init hugetlb_init(void)
 {
 	int i;
@@ -7546,7 +7554,7 @@ void __init hugetlb_cma_reserve(int order)
 		hugetlb_cma_size = 0;
 }
 
-void __init hugetlb_cma_check(void)
+static void __init hugetlb_cma_check(void)
 {
 	if (!hugetlb_cma_size || cma_reserve_called)
 		return;
-- 
2.23.0

[PATCH 02/10] hugetlb: Use helper macro SZ_1K

[Miaohe Lin]

Use helper macro SZ_1K to do the size conversion to make code more
consistent in this file. Minor readability improvement.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/hugetlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 10d84abb1ca7..24f580ddf130 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4165,7 +4165,7 @@ void __init hugetlb_add_hstate(unsigned int order)
 	h->next_nid_to_alloc = first_memory_node;
 	h->next_nid_to_free = first_memory_node;
 	snprintf(h->name, HSTATE_NAME_LEN, "hugepages-%lukB",
-					huge_page_size(h)/1024);
+					huge_page_size(h)/SZ_1K);
 
 	parsed_hstate = h;
 }
-- 
2.23.0

[PATCH 03/10] hugetlb: Use LIST_HEAD() to define a list head

[Miaohe Lin]

We can avoid unneeded WRITE_ONCE() overhead by using LIST_HEAD() to define
a list head.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/hugetlb.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 24f580ddf130..b3e6592247a3 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -459,14 +459,12 @@ static int allocate_file_region_entries(struct resv_map *resv,
 					int regions_needed)
 	__must_hold(&resv->lock)
 {
-	struct list_head allocated_regions;
+	LIST_HEAD(allocated_regions);
 	int to_allocate = 0, i = 0;
 	struct file_region *trg = NULL, *rg = NULL;
 
 	VM_BUG_ON(regions_needed < 0);
 
-	INIT_LIST_HEAD(&allocated_regions);
-
 	/*
 	 * Check for sufficient descriptors in the cache to accommodate
 	 * the number of in progress add operations plus regions_needed.
@@ -2352,7 +2350,7 @@ struct page *alloc_huge_page_vma(struct hstate *h, struct vm_area_struct *vma,
 static int gather_surplus_pages(struct hstate *h, long delta)
 	__must_hold(&hugetlb_lock)
 {
-	struct list_head surplus_list;
+	LIST_HEAD(surplus_list);
 	struct page *page, *tmp;
 	int ret;
 	long i;
@@ -2367,7 +2365,6 @@ static int gather_surplus_pages(struct hstate *h, long delta)
 	}
 
 	allocated = 0;
-	INIT_LIST_HEAD(&surplus_list);
 
 	ret = -ENOMEM;
 retry:
-- 
2.23.0

[PATCH 04/10] hugetlb: Use sizeof() to get the array size

[Miaohe Lin]

It's better to use sizeof() to get the array size instead of manual
calculation. Minor readability improvement.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/hugetlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index b3e6592247a3..06293da96112 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4177,11 +4177,11 @@ static void __init hugepages_clear_pages_in_node(void)
 	if (!hugetlb_max_hstate) {
 		default_hstate_max_huge_pages = 0;
 		memset(default_hugepages_in_node, 0,
-			MAX_NUMNODES * sizeof(unsigned int));
+			sizeof(default_hugepages_in_node));
 	} else {
 		parsed_hstate->max_huge_pages = 0;
 		memset(parsed_hstate->max_huge_pages_node, 0,
-			MAX_NUMNODES * sizeof(unsigned int));
+			sizeof(parsed_hstate->max_huge_pages_node));
 	}
 }
 
-- 
2.23.0

[PATCH 05/10] hugetlb: Use helper {huge_pte|pmd}_lock()

[Miaohe Lin]

Use helper huge_pte_lock and pmd_lock to simplify the code. No functional
change intended.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/hugetlb.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 06293da96112..6ea9f73aea84 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6094,8 +6094,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 		page_in_pagecache = true;
 	}
 
-	ptl = huge_pte_lockptr(h, dst_mm, dst_pte);
-	spin_lock(ptl);
+	ptl = huge_pte_lock(h, dst_mm, dst_pte);
 
 	/*
 	 * We allow to overwrite a pte marker: consider when both MISSING|WP
@@ -7176,8 +7175,7 @@ follow_huge_pmd(struct mm_struct *mm, unsigned long address,
 		return NULL;
 
 retry:
-	ptl = pmd_lockptr(mm, pmd);
-	spin_lock(ptl);
+	ptl = pmd_lock(mm, pmd);
 	/*
 	 * make sure that the address range covered by this pmd is not
 	 * unmapped from other threads.
-- 
2.23.0

[PATCH 06/10] hugetlb: pass NULL to kobj_to_hstate() if nid is unused

[Miaohe Lin]

We can pass NULL to kobj_to_hstate() directly when nid is unused to
simplify the code. No functional change intended.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/hugetlb.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 6ea9f73aea84..58eeae934e09 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3779,8 +3779,7 @@ HSTATE_ATTR_WO(demote);
 static ssize_t demote_size_show(struct kobject *kobj,
 					struct kobj_attribute *attr, char *buf)
 {
-	int nid;
-	struct hstate *h = kobj_to_hstate(kobj, &nid);
+	struct hstate *h = kobj_to_hstate(kobj, NULL);
 	unsigned long demote_size = (PAGE_SIZE << h->demote_order) / SZ_1K;
 
 	return sysfs_emit(buf, "%lukB\n", demote_size);
@@ -3793,7 +3792,6 @@ static ssize_t demote_size_store(struct kobject *kobj,
 	struct hstate *h, *demote_hstate;
 	unsigned long demote_size;
 	unsigned int demote_order;
-	int nid;
 
 	demote_size = (unsigned long)memparse(buf, NULL);
 
@@ -3805,7 +3803,7 @@ static ssize_t demote_size_store(struct kobject *kobj,
 		return -EINVAL;
 
 	/* demote order must be smaller than hstate order */
-	h = kobj_to_hstate(kobj, &nid);
+	h = kobj_to_hstate(kobj, NULL);
 	if (demote_order >= h->order)
 		return -EINVAL;
 
-- 
2.23.0

[PATCH 07/10] hugetlb: kill hugetlbfs_pagecache_page()

[Miaohe Lin]

Fold hugetlbfs_pagecache_page() into its sole caller to remove some
duplicated code. No functional change intended.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/hugetlb.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 58eeae934e09..7934188bbed0 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5467,19 +5467,6 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
 	return ret;
 }
 
-/* Return the pagecache page at a given address within a VMA */
-static struct page *hugetlbfs_pagecache_page(struct hstate *h,
-			struct vm_area_struct *vma, unsigned long address)
-{
-	struct address_space *mapping;
-	pgoff_t idx;
-
-	mapping = vma->vm_file->f_mapping;
-	idx = vma_hugecache_offset(h, vma, address);
-
-	return find_lock_page(mapping, idx);
-}
-
 /*
  * Return whether there is a pagecache page to back given address within VMA.
  * Caller follow_hugetlb_page() holds page_table_lock so we cannot lock_page.
@@ -5885,7 +5872,7 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 		/* Just decrements count, does not deallocate */
 		vma_end_reservation(h, vma, haddr);
 
-		pagecache_page = hugetlbfs_pagecache_page(h, vma, haddr);
+		pagecache_page = find_lock_page(mapping, idx);
 	}
 
 	ptl = huge_pte_lock(h, mm, ptep);
-- 
2.23.0

[PATCH 08/10] hugetlb: remove unneeded SetHPageVmemmapOptimized()

[Miaohe Lin]

If code reaches here, it's guaranteed that HPageVmemmapOptimized is set
for the hugetlb page (or VM_BUG_ON_PAGE() will complain about it). It's
unnecessary to set it again.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/hugetlb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 7934188bbed0..b432a00061e3 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1520,7 +1520,6 @@ static void add_hugetlb_page(struct hstate *h, struct page *page,
 
 	set_compound_page_dtor(page, HUGETLB_PAGE_DTOR);
 	set_page_private(page, 0);
-	SetHPageVmemmapOptimized(page);
 
 	/*
 	 * This page is about to be managed by the hugetlb allocator and
-- 
2.23.0

[PATCH 09/10] hugetlb: remove meaningless BUG_ON(huge_pte_none())

[Miaohe Lin]

When code reaches here, invalid page would have been accessed if huge pte
is none. So this BUG_ON(huge_pte_none()) is meaningless. Remove it.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/hugetlb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index b432a00061e3..0ef26dbdc7f8 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5374,7 +5374,6 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
 			u32 hash;
 
 			put_page(old_page);
-			BUG_ON(huge_pte_none(pte));
 			/*
 			 * Drop hugetlb_fault_mutex and vma_lock before
 			 * unmapping.  unmapping needs to hold vma_lock
-- 
2.23.0

[PATCH 10/10] hugetlb: make hugetlb selects SYSFS if !SYSCTL

[Miaohe Lin]

If CONFIG_SYSFS and CONFIG_SYSCTL are both undefined, hugetlb doesn't work
now as there's no way to set max huge pages. Make sure at least one of the
above configs is defined to make hugetlb works as expected.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 fs/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/Kconfig b/fs/Kconfig
index a547307c1ae8..b266e7b5de85 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -235,6 +235,7 @@ config ARCH_SUPPORTS_HUGETLBFS
 config HUGETLBFS
 	bool "HugeTLB file system support"
 	depends on X86 || IA64 || SPARC64 || ARCH_SUPPORTS_HUGETLBFS || BROKEN
+	select SYSFS if !SYSCTL
 	help
 	  hugetlbfs is a filesystem backing for HugeTLB pages, based on
 	  ramfs. For architectures that support it, say Y here and read
-- 
2.23.0

Re: [PATCH 08/10] hugetlb: remove unneeded SetHPageVmemmapOptimized()

[Muchun Song]

> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> If code reaches here, it's guaranteed that HPageVmemmapOptimized is set
> for the hugetlb page (or VM_BUG_ON_PAGE() will complain about it). It's

Hi Miaohe,

Right.

> unnecessary to set it again.

No, I suppose you didn’t test this patch since it does not work. The
HPageVmemmapOptimized is cleared in the above code of line
(set_page_private(page, 0)). So NAck.

Thanks,
Muchun

> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
> mm/hugetlb.c | 1 -
> 1 file changed, 1 deletion(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 7934188bbed0..b432a00061e3 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1520,7 +1520,6 @@ static void add_hugetlb_page(struct hstate *h, struct page *page,
> 
> 	set_compound_page_dtor(page, HUGETLB_PAGE_DTOR);
> 	set_page_private(page, 0);
> -	SetHPageVmemmapOptimized(page);
> 
> 	/*
> 	 * This page is about to be managed by the hugetlb allocator and
> -- 
> 2.23.0
> 
> 

Re: [PATCH 01/10] hugetlb: make hugetlb_cma_check() static

[Muchun Song]

> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> Make hugetlb_cma_check() static as it's only used inside mm/hugetlb.c.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

Re: [PATCH 02/10] hugetlb: Use helper macro SZ_1K

[Muchun Song]

> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> Use helper macro SZ_1K to do the size conversion to make code more
> consistent in this file. Minor readability improvement.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

Re: [PATCH 04/10] hugetlb: Use sizeof() to get the array size

[Muchun Song]

> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> It's better to use sizeof() to get the array size instead of manual
> calculation. Minor readability improvement.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

Re: [PATCH 03/10] hugetlb: Use LIST_HEAD() to define a list head

[Muchun Song]

> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> We can avoid unneeded WRITE_ONCE() overhead by using LIST_HEAD() to define
> a list head.

IIUC, the overhead doesn’t change. Right?

I’m fine with your changes.

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
> mm/hugetlb.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 24f580ddf130..b3e6592247a3 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -459,14 +459,12 @@ static int allocate_file_region_entries(struct resv_map *resv,
> 					int regions_needed)
> 	__must_hold(&resv->lock)
> {
> -	struct list_head allocated_regions;
> +	LIST_HEAD(allocated_regions);
> 	int to_allocate = 0, i = 0;
> 	struct file_region *trg = NULL, *rg = NULL;
> 
> 	VM_BUG_ON(regions_needed < 0);
> 
> -	INIT_LIST_HEAD(&allocated_regions);
> -
> 	/*
> 	 * Check for sufficient descriptors in the cache to accommodate
> 	 * the number of in progress add operations plus regions_needed.
> @@ -2352,7 +2350,7 @@ struct page *alloc_huge_page_vma(struct hstate *h, struct vm_area_struct *vma,
> static int gather_surplus_pages(struct hstate *h, long delta)
> 	__must_hold(&hugetlb_lock)
> {
> -	struct list_head surplus_list;
> +	LIST_HEAD(surplus_list);
> 	struct page *page, *tmp;
> 	int ret;
> 	long i;
> @@ -2367,7 +2365,6 @@ static int gather_surplus_pages(struct hstate *h, long delta)
> 	}
> 
> 	allocated = 0;
> -	INIT_LIST_HEAD(&surplus_list);
> 
> 	ret = -ENOMEM;
> retry:
> -- 
> 2.23.0
> 
> 

Re: [PATCH 05/10] hugetlb: Use helper {huge_pte|pmd}_lock()

[Muchun Song]

> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> Use helper huge_pte_lock and pmd_lock to simplify the code. No functional
> change intended.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

Re: [PATCH 06/10] hugetlb: pass NULL to kobj_to_hstate() if nid is unused

[Muchun Song]

> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> We can pass NULL to kobj_to_hstate() directly when nid is unused to
> simplify the code. No functional change intended.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

Re: [PATCH 07/10] hugetlb: kill hugetlbfs_pagecache_page()

[Muchun Song]

> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> Fold hugetlbfs_pagecache_page() into its sole caller to remove some
> duplicated code. No functional change intended.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

Re: [PATCH 10/10] hugetlb: make hugetlb selects SYSFS if !SYSCTL

[Muchun Song]

> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> If CONFIG_SYSFS and CONFIG_SYSCTL are both undefined, hugetlb doesn't work
> now as there's no way to set max huge pages. Make sure at least one of the
> above configs is defined to make hugetlb works as expected.

Right. I think the majority of the people are usually use /proc knob to
reserve HugeTLB pages than /sys fs. So I suggest to add a “depends on SYSCTL”
item. What’s your thoughts?

Thanks.

> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
> fs/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/fs/Kconfig b/fs/Kconfig
> index a547307c1ae8..b266e7b5de85 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -235,6 +235,7 @@ config ARCH_SUPPORTS_HUGETLBFS
> config HUGETLBFS
> 	bool "HugeTLB file system support"
> 	depends on X86 || IA64 || SPARC64 || ARCH_SUPPORTS_HUGETLBFS || BROKEN
> +	select SYSFS if !SYSCTL
> 	help
> 	  hugetlbfs is a filesystem backing for HugeTLB pages, based on
> 	  ramfs. For architectures that support it, say Y here and read
> -- 
> 2.23.0
> 
> 

Re: [PATCH 09/10] hugetlb: remove meaningless BUG_ON(huge_pte_none())

[Muchun Song]

> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> When code reaches here, invalid page would have been accessed if huge pte
> is none. So this BUG_ON(huge_pte_none()) is meaningless. Remove it.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks.

Re: [PATCH 08/10] hugetlb: remove unneeded SetHPageVmemmapOptimized()

[Miaohe Lin]

On 2022/8/27 9:35, Muchun Song wrote:
> 
> 
>> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> If code reaches here, it's guaranteed that HPageVmemmapOptimized is set
>> for the hugetlb page (or VM_BUG_ON_PAGE() will complain about it). It's
> 
> Hi Miaohe,
> 
> Right.
> 
>> unnecessary to set it again.
> 
> No, I suppose you didn’t test this patch since it does not work. The
> HPageVmemmapOptimized is cleared in the above code of line
> (set_page_private(page, 0)). So NAck.

Sorry, I missed that and thanks for pointing this out. Does this code deserves a
comment like below?

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index d0617d64d718..3374c1d2b52e 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1522,6 +1522,10 @@ static void add_hugetlb_page(struct hstate *h, struct page *page,

        set_compound_page_dtor(page, HUGETLB_PAGE_DTOR);
        set_page_private(page, 0);
+       /*
+        * We have to set HPageVmemmapOptimized again as above
+        * set_page_private(page, 0) cleared it.
+        */
        SetHPageVmemmapOptimized(page);

        /*


Thanks,
Miaohe Lin

Re: [PATCH 03/10] hugetlb: Use LIST_HEAD() to define a list head

[Miaohe Lin]

On 2022/8/27 9:47, Muchun Song wrote:
> 
> 
>> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> We can avoid unneeded WRITE_ONCE() overhead by using LIST_HEAD() to define
>> a list head.
> 
> IIUC, the overhead doesn’t change. Right?

I think the overhead is changed. LIST_HEAD is initialized without using WRITE_ONCE():

#define LIST_HEAD_INIT(name) { &(name), &(name) }

#define LIST_HEAD(name) \
	struct list_head name = LIST_HEAD_INIT(name)

while INIT_LIST_HEAD has:

static inline void INIT_LIST_HEAD(struct list_head *list)
{
	WRITE_ONCE(list->next, list);
	WRITE_ONCE(list->prev, list);
}

Or am I miss something?

> 
> I’m fine with your changes.
> 
> Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Many thanks for your review and comment. :)

Thanks,
Miaohe Lin

Re: [PATCH 10/10] hugetlb: make hugetlb selects SYSFS if !SYSCTL

[Miaohe Lin]

On 2022/8/27 9:59, Muchun Song wrote:
> 
> 
>> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> If CONFIG_SYSFS and CONFIG_SYSCTL are both undefined, hugetlb doesn't work
>> now as there's no way to set max huge pages. Make sure at least one of the
>> above configs is defined to make hugetlb works as expected.
> 
> Right. I think the majority of the people are usually use /proc knob to
> reserve HugeTLB pages than /sys fs. So I suggest to add a “depends on SYSCTL”
> item. What’s your thoughts?

It seems some functions, e.g. demotion, is exported via SYSFS instead of SYSCTL.
So maybe "depends on SYSFS || SYSCTL", i.e.

	depends on (X86 || IA64 || SPARC64 || ARCH_SUPPORTS_HUGETLBFS || BROKEN) && (SYSFS || SYSCTL)

will be more suitable?

Thanks,
Miaohe Lin

Re: [PATCH 03/10] hugetlb: Use LIST_HEAD() to define a list head

[Muchun Song]

> On Aug 27, 2022, at 10:27, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> On 2022/8/27 9:47, Muchun Song wrote:
>> 
>> 
>>> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>> 
>>> We can avoid unneeded WRITE_ONCE() overhead by using LIST_HEAD() to define
>>> a list head.
>> 
>> IIUC, the overhead doesn’t change. Right?
> 
> I think the overhead is changed. LIST_HEAD is initialized without using WRITE_ONCE():

I think there is no special difference with "WRITE_ONCE(var, 0)" vs "var = 0” in
assembly code. Both code of line will be compiled to a mov or movq instruction.
I didn’t confirm if the assembly code is different (I tend to think it is similar).
Just some analysis from me.

> 
> #define LIST_HEAD_INIT(name) { &(name), &(name) }
> 
> #define LIST_HEAD(name) \
> 	struct list_head name = LIST_HEAD_INIT(name)
> 
> while INIT_LIST_HEAD has:
> 
> static inline void INIT_LIST_HEAD(struct list_head *list)
> {
> 	WRITE_ONCE(list->next, list);
> 	WRITE_ONCE(list->prev, list);
> }
> 
> Or am I miss something?
> 
>> 
>> I’m fine with your changes.
>> 
>> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> 
> Many thanks for your review and comment. :)
> 
> Thanks,
> Miaohe Lin
> 

Re: [PATCH 08/10] hugetlb: remove unneeded SetHPageVmemmapOptimized()

[Muchun Song]

> On Aug 27, 2022, at 10:18, Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
> On 2022/8/27 9:35, Muchun Song wrote:
>> 
>> 
>>> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>> 
>>> If code reaches here, it's guaranteed that HPageVmemmapOptimized is set
>>> for the hugetlb page (or VM_BUG_ON_PAGE() will complain about it). It's
>> 
>> Hi Miaohe,
>> 
>> Right.
>> 
>>> unnecessary to set it again.
>> 
>> No, I suppose you didn’t test this patch since it does not work. The
>> HPageVmemmapOptimized is cleared in the above code of line
>> (set_page_private(page, 0)). So NAck.
> 
> Sorry, I missed that and thanks for pointing this out. Does this code deserves a
> comment like below?
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index d0617d64d718..3374c1d2b52e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1522,6 +1522,10 @@ static void add_hugetlb_page(struct hstate *h, struct page *page,
> 
>        set_compound_page_dtor(page, HUGETLB_PAGE_DTOR);
>        set_page_private(page, 0);
> +       /*
> +        * We have to set HPageVmemmapOptimized again as above
> +        * set_page_private(page, 0) cleared it.
> +        */
>        SetHPageVmemmapOptimized(page);

I’m fine with this comment.

Reviewed-by: Muchun Song <songmuchun@bytedance.com>


> 
>        /*
> 
> 
> Thanks,
> Miaohe Lin

Re: [PATCH 03/10] hugetlb: Use LIST_HEAD() to define a list head

[Miaohe Lin]

On 2022/8/27 10:48, Muchun Song wrote:
> 
> 
>> On Aug 27, 2022, at 10:27, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> On 2022/8/27 9:47, Muchun Song wrote:
>>>
>>>
>>>> On Aug 26, 2022, at 17:24, Miaohe Lin <linmiaohe@huawei.com> wrote:
>>>>
>>>> We can avoid unneeded WRITE_ONCE() overhead by using LIST_HEAD() to define
>>>> a list head.
>>>
>>> IIUC, the overhead doesn’t change. Right?
>>
>> I think the overhead is changed. LIST_HEAD is initialized without using WRITE_ONCE():
> 
> I think there is no special difference with "WRITE_ONCE(var, 0)" vs "var = 0” in

It's not write var to 0 indeed. But there seems are no special difference.

> assembly code. Both code of line will be compiled to a mov or movq instruction.
> I didn’t confirm if the assembly code is different (I tend to think it is similar).
> Just some analysis from me.

I checked the generated code in x86, they're almost same. And in aarch64, there's difference
between one "stp" instruction vs two "str" instruction. So I think you're right. Thanks for
pointing this out. I should tweak the commit log in next version.

Thanks a lot,
Miaohe Lin