[PATCH 0/4] Code simplification and clean-up for hugetlb vmemmap

[PATCH 0/4] Code simplification and clean-up for hugetlb vmemmap

[Muchun Song]

This series aims to simplify and clean the code of hugetlb vmemmap, please
look at the commit message of each individual patch for more details.

Thanks.

Muchun Song (4):
  mm: pagewalk: assert write mmap lock only for walking the user page
    tables
  mm: hugetlb_vmemmap: use walk_page_range_novma() to simplify the code
  mm: hugetlb_vmemmap: move PageVmemmapSelfHosted() check to
    split_vmemmap_huge_pmd()
  mm: hugetlb_vmemmap: convert page to folio

 mm/hugetlb_vmemmap.c | 259 ++++++++++++++-----------------------------
 mm/pagewalk.c        |  29 ++++-
 2 files changed, 111 insertions(+), 177 deletions(-)

-- 
2.20.1

[PATCH 1/4] mm: pagewalk: assert write mmap lock only for walking the user page tables

[Muchun Song]

The 8782fb61cc848 ("mm: pagewalk: Fix race between unmap and page walker")
introduces an assertion to walk_page_range_novma() to make all the users
of page table walker is safe. However, the race only exists for walking the
user page tables. And it is ridiculous to hold a particular user mmap write
lock against the changes of the kernel page tables. So only assert at least
mmap read lock when walking the kernel page tables. And some users matching
this case could downgrade to a mmap read lock to relief the contention of
mmap lock of init_mm, it will be nicer in hugetlb (only holding mmap read
lock) in the next patch.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/pagewalk.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/mm/pagewalk.c b/mm/pagewalk.c
index b7d7e4fcfad7a..f46c80b18ce4f 100644
--- a/mm/pagewalk.c
+++ b/mm/pagewalk.c
@@ -539,6 +539,11 @@ int walk_page_range(struct mm_struct *mm, unsigned long start,
  * not backed by VMAs. Because 'unusual' entries may be walked this function
  * will also not lock the PTEs for the pte_entry() callback. This is useful for
  * walking the kernel pages tables or page tables for firmware.
+ *
+ * Note: Be careful to walk the kernel pages tables, the caller may be need to
+ * take other effective approache (mmap lock may be insufficient) to prevent
+ * the intermediate kernel page tables belonging to the specified address range
+ * from being freed (e.g. memory hot-remove).
  */
 int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
 			  unsigned long end, const struct mm_walk_ops *ops,
@@ -556,7 +561,29 @@ int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
 	if (start >= end || !walk.mm)
 		return -EINVAL;
 
-	mmap_assert_write_locked(walk.mm);
+	/*
+	 * 1) For walking the user virtual address space:
+	 *
+	 * The mmap lock protects the page walker from changes to the page
+	 * tables during the walk.  However a read lock is insufficient to
+	 * protect those areas which don't have a VMA as munmap() detaches
+	 * the VMAs before downgrading to a read lock and actually tearing
+	 * down PTEs/page tables. In which case, the mmap write lock should
+	 * be hold.
+	 *
+	 * 2) For walking the kernel virtual address space:
+	 *
+	 * The kernel intermediate page tables usually do not be freed, so
+	 * the mmap map read lock is sufficient. But there are some exceptions.
+	 * E.g. memory hot-remove. In which case, the mmap lock is insufficient
+	 * to prevent the intermediate kernel pages tables belonging to the
+	 * specified address range from being freed. The caller should take
+	 * other actions to prevent this race.
+	 */
+	if (mm == &init_mm)
+		mmap_assert_locked(walk.mm);
+	else
+		mmap_assert_write_locked(walk.mm);
 
 	return walk_pgd_range(start, end, &walk);
 }
-- 
2.20.1

Re: [PATCH 1/4] mm: pagewalk: assert write mmap lock only for walking the user page tables

[Kefeng Wang]

On 2023/11/27 16:46, Muchun Song wrote:
> The 8782fb61cc848 ("mm: pagewalk: Fix race between unmap and page walker")
> introduces an assertion to walk_page_range_novma() to make all the users
> of page table walker is safe. However, the race only exists for walking the
> user page tables. And it is ridiculous to hold a particular user mmap write
> lock against the changes of the kernel page tables. So only assert at least
> mmap read lock when walking the kernel page tables. And some users matching
> this case could downgrade to a mmap read lock to relief the contention of
> mmap lock of init_mm, it will be nicer in hugetlb (only holding mmap read
> lock) in the next patch.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>   mm/pagewalk.c | 29 ++++++++++++++++++++++++++++-
>   1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> index b7d7e4fcfad7a..f46c80b18ce4f 100644
> --- a/mm/pagewalk.c
> +++ b/mm/pagewalk.c
> @@ -539,6 +539,11 @@ int walk_page_range(struct mm_struct *mm, unsigned long start,
>    * not backed by VMAs. Because 'unusual' entries may be walked this function
>    * will also not lock the PTEs for the pte_entry() callback. This is useful for
>    * walking the kernel pages tables or page tables for firmware.
> + *
> + * Note: Be careful to walk the kernel pages tables, the caller may be need to
> + * take other effective approache (mmap lock may be insufficient) to prevent
> + * the intermediate kernel page tables belonging to the specified address range
> + * from being freed (e.g. memory hot-remove).
>    */
>   int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
>   			  unsigned long end, const struct mm_walk_ops *ops,
> @@ -556,7 +561,29 @@ int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
>   	if (start >= end || !walk.mm)
>   		return -EINVAL;
>   
> -	mmap_assert_write_locked(walk.mm);
> +	/*
> +	 * 1) For walking the user virtual address space:
> +	 *
> +	 * The mmap lock protects the page walker from changes to the page
> +	 * tables during the walk.  However a read lock is insufficient to
> +	 * protect those areas which don't have a VMA as munmap() detaches
> +	 * the VMAs before downgrading to a read lock and actually tearing
> +	 * down PTEs/page tables. In which case, the mmap write lock should
> +	 * be hold.
> +	 *
> +	 * 2) For walking the kernel virtual address space:
> +	 *
> +	 * The kernel intermediate page tables usually do not be freed, so
> +	 * the mmap map read lock is sufficient. But there are some exceptions.
> +	 * E.g. memory hot-remove. In which case, the mmap lock is insufficient
> +	 * to prevent the intermediate kernel pages tables belonging to the
> +	 * specified address range from being freed. The caller should take
> +	 * other actions to prevent this race.
> +	 */
> +	if (mm == &init_mm)
> +		mmap_assert_locked(walk.mm);
> +	else
> +		mmap_assert_write_locked(walk.mm);

Maybe just use process_mm_walk_lock() and set correct page_walk_lock in 
struct mm_walk_ops?

>   
>   	return walk_pgd_range(start, end, &walk);
>   }

Re: [PATCH 1/4] mm: pagewalk: assert write mmap lock only for walking the user page tables

[Muchun Song]

> On Dec 1, 2023, at 19:09, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> 
> 
> 
> On 2023/11/27 16:46, Muchun Song wrote:
>> The 8782fb61cc848 ("mm: pagewalk: Fix race between unmap and page walker")
>> introduces an assertion to walk_page_range_novma() to make all the users
>> of page table walker is safe. However, the race only exists for walking the
>> user page tables. And it is ridiculous to hold a particular user mmap write
>> lock against the changes of the kernel page tables. So only assert at least
>> mmap read lock when walking the kernel page tables. And some users matching
>> this case could downgrade to a mmap read lock to relief the contention of
>> mmap lock of init_mm, it will be nicer in hugetlb (only holding mmap read
>> lock) in the next patch.
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>> ---
>>  mm/pagewalk.c | 29 ++++++++++++++++++++++++++++-
>>  1 file changed, 28 insertions(+), 1 deletion(-)
>> diff --git a/mm/pagewalk.c b/mm/pagewalk.c
>> index b7d7e4fcfad7a..f46c80b18ce4f 100644
>> --- a/mm/pagewalk.c
>> +++ b/mm/pagewalk.c
>> @@ -539,6 +539,11 @@ int walk_page_range(struct mm_struct *mm, unsigned long start,
>>   * not backed by VMAs. Because 'unusual' entries may be walked this function
>>   * will also not lock the PTEs for the pte_entry() callback. This is useful for
>>   * walking the kernel pages tables or page tables for firmware.
>> + *
>> + * Note: Be careful to walk the kernel pages tables, the caller may be need to
>> + * take other effective approache (mmap lock may be insufficient) to prevent
>> + * the intermediate kernel page tables belonging to the specified address range
>> + * from being freed (e.g. memory hot-remove).
>>   */
>>  int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
>>    unsigned long end, const struct mm_walk_ops *ops,
>> @@ -556,7 +561,29 @@ int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
>>   if (start >= end || !walk.mm)
>>   return -EINVAL;
>>  - mmap_assert_write_locked(walk.mm);
>> + /*
>> + * 1) For walking the user virtual address space:
>> + *
>> + * The mmap lock protects the page walker from changes to the page
>> + * tables during the walk.  However a read lock is insufficient to
>> + * protect those areas which don't have a VMA as munmap() detaches
>> + * the VMAs before downgrading to a read lock and actually tearing
>> + * down PTEs/page tables. In which case, the mmap write lock should
>> + * be hold.
>> + *
>> + * 2) For walking the kernel virtual address space:
>> + *
>> + * The kernel intermediate page tables usually do not be freed, so
>> + * the mmap map read lock is sufficient. But there are some exceptions.
>> + * E.g. memory hot-remove. In which case, the mmap lock is insufficient
>> + * to prevent the intermediate kernel pages tables belonging to the
>> + * specified address range from being freed. The caller should take
>> + * other actions to prevent this race.
>> + */
>> + if (mm == &init_mm)
>> + 	mmap_assert_locked(walk.mm);
>> + else
>> + 	mmap_assert_write_locked(walk.mm);
> 
> Maybe just use process_mm_walk_lock() and set correct page_walk_lock in struct mm_walk_ops?

No. You also need to make sure the users do not pass the wrong
walk_lock, so you also need to add something like following:

if (mm == &init_mm)
	VM_BUG_ON(walk_lock != PGWALK_RDLOCK);
else
	VM_BUG_ON(walk_lock == PGWALK_RDLOCK);

I do not think the code will be simple.

> 
>>     return walk_pgd_range(start, end, &walk);
>>  }

Re: [PATCH 1/4] mm: pagewalk: assert write mmap lock only for walking the user page tables

[Kefeng Wang]

On 2023/12/2 16:08, Muchun Song wrote:
> 
> 
>> On Dec 1, 2023, at 19:09, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>
>>
>>
>> On 2023/11/27 16:46, Muchun Song wrote:
>>> The 8782fb61cc848 ("mm: pagewalk: Fix race between unmap and page walker")
>>> introduces an assertion to walk_page_range_novma() to make all the users
>>> of page table walker is safe. However, the race only exists for walking the
>>> user page tables. And it is ridiculous to hold a particular user mmap write
>>> lock against the changes of the kernel page tables. So only assert at least
>>> mmap read lock when walking the kernel page tables. And some users matching
>>> this case could downgrade to a mmap read lock to relief the contention of
>>> mmap lock of init_mm, it will be nicer in hugetlb (only holding mmap read
>>> lock) in the next patch.
>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>> ---
>>>   mm/pagewalk.c | 29 ++++++++++++++++++++++++++++-
>>>   1 file changed, 28 insertions(+), 1 deletion(-)
>>> diff --git a/mm/pagewalk.c b/mm/pagewalk.c
>>> index b7d7e4fcfad7a..f46c80b18ce4f 100644
>>> --- a/mm/pagewalk.c
>>> +++ b/mm/pagewalk.c
>>> @@ -539,6 +539,11 @@ int walk_page_range(struct mm_struct *mm, unsigned long start,
>>>    * not backed by VMAs. Because 'unusual' entries may be walked this function
>>>    * will also not lock the PTEs for the pte_entry() callback. This is useful for
>>>    * walking the kernel pages tables or page tables for firmware.
>>> + *
>>> + * Note: Be careful to walk the kernel pages tables, the caller may be need to
>>> + * take other effective approache (mmap lock may be insufficient) to prevent
>>> + * the intermediate kernel page tables belonging to the specified address range
>>> + * from being freed (e.g. memory hot-remove).
>>>    */
>>>   int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
>>>     unsigned long end, const struct mm_walk_ops *ops,
>>> @@ -556,7 +561,29 @@ int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
>>>    if (start >= end || !walk.mm)
>>>    return -EINVAL;
>>>   - mmap_assert_write_locked(walk.mm);
>>> + /*
>>> + * 1) For walking the user virtual address space:
>>> + *
>>> + * The mmap lock protects the page walker from changes to the page
>>> + * tables during the walk.  However a read lock is insufficient to
>>> + * protect those areas which don't have a VMA as munmap() detaches
>>> + * the VMAs before downgrading to a read lock and actually tearing
>>> + * down PTEs/page tables. In which case, the mmap write lock should
>>> + * be hold.
>>> + *
>>> + * 2) For walking the kernel virtual address space:
>>> + *
>>> + * The kernel intermediate page tables usually do not be freed, so
>>> + * the mmap map read lock is sufficient. But there are some exceptions.
>>> + * E.g. memory hot-remove. In which case, the mmap lock is insufficient
>>> + * to prevent the intermediate kernel pages tables belonging to the
>>> + * specified address range from being freed. The caller should take
>>> + * other actions to prevent this race.
>>> + */
>>> + if (mm == &init_mm)
>>> + 	mmap_assert_locked(walk.mm);
>>> + else
>>> + 	mmap_assert_write_locked(walk.mm);
>>
>> Maybe just use process_mm_walk_lock() and set correct page_walk_lock in struct mm_walk_ops?
> 
> No. You also need to make sure the users do not pass the wrong
> walk_lock, so you also need to add something like following:
> 

But all other walk_page_XX has been converted，see more from commit
49b0638502da "mm: enable page walking API to lock vmas during the walk"),
there's nothing special about this one, the calls must pass the right
page_walk_lock to mm_walk_ops,

> if (mm == &init_mm)
> 	VM_BUG_ON(walk_lock != PGWALK_RDLOCK);
> else
> 	VM_BUG_ON(walk_lock == PGWALK_RDLOCK);
> 
> I do not think the code will be simple.

or adding the above lock check into process_mm_walk_lock too.

Re: [PATCH 1/4] mm: pagewalk: assert write mmap lock only for walking the user page tables

[Muchun Song]

> On Dec 2, 2023, at 17:25, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> 
> 
> 
> On 2023/12/2 16:08, Muchun Song wrote:
>>>> On Dec 1, 2023, at 19:09, Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>> 
>>> 
>>> 
>>> On 2023/11/27 16:46, Muchun Song wrote:
>>>> The 8782fb61cc848 ("mm: pagewalk: Fix race between unmap and page walker")
>>>> introduces an assertion to walk_page_range_novma() to make all the users
>>>> of page table walker is safe. However, the race only exists for walking the
>>>> user page tables. And it is ridiculous to hold a particular user mmap write
>>>> lock against the changes of the kernel page tables. So only assert at least
>>>> mmap read lock when walking the kernel page tables. And some users matching
>>>> this case could downgrade to a mmap read lock to relief the contention of
>>>> mmap lock of init_mm, it will be nicer in hugetlb (only holding mmap read
>>>> lock) in the next patch.
>>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>>> ---
>>>>  mm/pagewalk.c | 29 ++++++++++++++++++++++++++++-
>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>> diff --git a/mm/pagewalk.c b/mm/pagewalk.c
>>>> index b7d7e4fcfad7a..f46c80b18ce4f 100644
>>>> --- a/mm/pagewalk.c
>>>> +++ b/mm/pagewalk.c
>>>> @@ -539,6 +539,11 @@ int walk_page_range(struct mm_struct *mm, unsigned long start,
>>>>   * not backed by VMAs. Because 'unusual' entries may be walked this function
>>>>   * will also not lock the PTEs for the pte_entry() callback. This is useful for
>>>>   * walking the kernel pages tables or page tables for firmware.
>>>> + *
>>>> + * Note: Be careful to walk the kernel pages tables, the caller may be need to
>>>> + * take other effective approache (mmap lock may be insufficient) to prevent
>>>> + * the intermediate kernel page tables belonging to the specified address range
>>>> + * from being freed (e.g. memory hot-remove).
>>>>   */
>>>>  int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
>>>>    unsigned long end, const struct mm_walk_ops *ops,
>>>> @@ -556,7 +561,29 @@ int walk_page_range_novma(struct mm_struct *mm, unsigned long start,
>>>>   if (start >= end || !walk.mm)
>>>>   return -EINVAL;
>>>>  - mmap_assert_write_locked(walk.mm);
>>>> + /*
>>>> + * 1) For walking the user virtual address space:
>>>> + *
>>>> + * The mmap lock protects the page walker from changes to the page
>>>> + * tables during the walk.  However a read lock is insufficient to
>>>> + * protect those areas which don't have a VMA as munmap() detaches
>>>> + * the VMAs before downgrading to a read lock and actually tearing
>>>> + * down PTEs/page tables. In which case, the mmap write lock should
>>>> + * be hold.
>>>> + *
>>>> + * 2) For walking the kernel virtual address space:
>>>> + *
>>>> + * The kernel intermediate page tables usually do not be freed, so
>>>> + * the mmap map read lock is sufficient. But there are some exceptions.
>>>> + * E.g. memory hot-remove. In which case, the mmap lock is insufficient
>>>> + * to prevent the intermediate kernel pages tables belonging to the
>>>> + * specified address range from being freed. The caller should take
>>>> + * other actions to prevent this race.
>>>> + */
>>>> + if (mm == &init_mm)
>>>> +    mmap_assert_locked(walk.mm);
>>>> + else
>>>> +    mmap_assert_write_locked(walk.mm);
>>> 
>>> Maybe just use process_mm_walk_lock() and set correct page_walk_lock in struct mm_walk_ops?
>> No. You also need to make sure the users do not pass the wrong
>> walk_lock, so you also need to add something like following:
> 
> But all other walk_page_XX has been converted，see more from commit
> 49b0638502da "mm: enable page walking API to lock vmas during the walk"),
> there's nothing special about this one, the calls must pass the right
> page_walk_lock to mm_walk_ops,

If you think this one is not special, why it is not converted by that commit at that time? 

> 
>> if (mm == &init_mm)
>>    VM_BUG_ON(walk_lock != PGWALK_RDLOCK);
>> else
>>    VM_BUG_ON(walk_lock == PGWALK_RDLOCK);
>> I do not think the code will be simple.
> 
> or adding the above lock check into process_mm_walk_lock too.

No. it’s wrong. walk_page_range_novma is special compared with other variants, the check is only applicable for walk_page_range_novma, not for its variants.

Re: [PATCH 1/4] mm: pagewalk: assert write mmap lock only for walking the user page tables

[Mike Kravetz]

On 11/27/23 16:46, Muchun Song wrote:
> The 8782fb61cc848 ("mm: pagewalk: Fix race between unmap and page walker")
> introduces an assertion to walk_page_range_novma() to make all the users
> of page table walker is safe. However, the race only exists for walking the
> user page tables. And it is ridiculous to hold a particular user mmap write
> lock against the changes of the kernel page tables. So only assert at least
> mmap read lock when walking the kernel page tables. And some users matching
> this case could downgrade to a mmap read lock to relief the contention of
> mmap lock of init_mm, it will be nicer in hugetlb (only holding mmap read
> lock) in the next patch.

At first, I did not understand your motivation for this patch.  But, it
makes sense as your next patch will replace hugetlb vmemmap specific walk
routines with walk_page_range_novma.

Directly Cc'ing Steven and Jann in case they have comments.  

> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/pagewalk.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)

Looks fine to me,
Acked-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

[PATCH 2/4] mm: hugetlb_vmemmap: use walk_page_range_novma() to simplify the code

[Muchun Song]

It is unnecessary to implement a series of dedicated page table walking
helpers since there is already a general one walk_page_range_novma().
So use it to simplify the code.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/hugetlb_vmemmap.c | 148 ++++++++++++-------------------------------
 1 file changed, 39 insertions(+), 109 deletions(-)

diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
index 87818ee7f01d7..ef14356855d13 100644
--- a/mm/hugetlb_vmemmap.c
+++ b/mm/hugetlb_vmemmap.c
@@ -14,6 +14,7 @@
 #include <linux/moduleparam.h>
 #include <linux/bootmem_info.h>
 #include <linux/mmdebug.h>
+#include <linux/pagewalk.h>
 #include <asm/pgalloc.h>
 #include <asm/tlbflush.h>
 #include "hugetlb_vmemmap.h"
@@ -45,21 +46,14 @@ struct vmemmap_remap_walk {
 	unsigned long		flags;
 };
 
-static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, bool flush)
+static int vmemmap_split_pmd(pmd_t *pmd, struct page *head, unsigned long start,
+			     struct vmemmap_remap_walk *walk)
 {
 	pmd_t __pmd;
 	int i;
 	unsigned long addr = start;
-	struct page *head;
 	pte_t *pgtable;
 
-	spin_lock(&init_mm.page_table_lock);
-	head = pmd_leaf(*pmd) ? pmd_page(*pmd) : NULL;
-	spin_unlock(&init_mm.page_table_lock);
-
-	if (!head)
-		return 0;
-
 	pgtable = pte_alloc_one_kernel(&init_mm);
 	if (!pgtable)
 		return -ENOMEM;
@@ -88,7 +82,7 @@ static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, bool flush)
 		/* Make pte visible before pmd. See comment in pmd_install(). */
 		smp_wmb();
 		pmd_populate_kernel(&init_mm, pmd, pgtable);
-		if (flush)
+		if (!(walk->flags & VMEMMAP_SPLIT_NO_TLB_FLUSH))
 			flush_tlb_kernel_range(start, start + PMD_SIZE);
 	} else {
 		pte_free_kernel(&init_mm, pgtable);
@@ -98,123 +92,59 @@ static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, bool flush)
 	return 0;
 }
 
-static void vmemmap_pte_range(pmd_t *pmd, unsigned long addr,
-			      unsigned long end,
-			      struct vmemmap_remap_walk *walk)
-{
-	pte_t *pte = pte_offset_kernel(pmd, addr);
-
-	/*
-	 * The reuse_page is found 'first' in table walk before we start
-	 * remapping (which is calling @walk->remap_pte).
-	 */
-	if (!walk->reuse_page) {
-		walk->reuse_page = pte_page(ptep_get(pte));
-		/*
-		 * Because the reuse address is part of the range that we are
-		 * walking, skip the reuse address range.
-		 */
-		addr += PAGE_SIZE;
-		pte++;
-		walk->nr_walked++;
-	}
-
-	for (; addr != end; addr += PAGE_SIZE, pte++) {
-		walk->remap_pte(pte, addr, walk);
-		walk->nr_walked++;
-	}
-}
-
-static int vmemmap_pmd_range(pud_t *pud, unsigned long addr,
-			     unsigned long end,
-			     struct vmemmap_remap_walk *walk)
+static int vmemmap_pmd_entry(pmd_t *pmd, unsigned long addr,
+			     unsigned long next, struct mm_walk *walk)
 {
-	pmd_t *pmd;
-	unsigned long next;
-
-	pmd = pmd_offset(pud, addr);
-	do {
-		int ret;
-
-		ret = split_vmemmap_huge_pmd(pmd, addr & PMD_MASK,
-				!(walk->flags & VMEMMAP_SPLIT_NO_TLB_FLUSH));
-		if (ret)
-			return ret;
+	struct page *head;
+	struct vmemmap_remap_walk *vmemmap_walk = walk->private;
 
-		next = pmd_addr_end(addr, end);
+	/* Only splitting, not remapping the vmemmap pages. */
+	if (!vmemmap_walk->remap_pte)
+		walk->action = ACTION_CONTINUE;
 
-		/*
-		 * We are only splitting, not remapping the hugetlb vmemmap
-		 * pages.
-		 */
-		if (!walk->remap_pte)
-			continue;
-
-		vmemmap_pte_range(pmd, addr, next, walk);
-	} while (pmd++, addr = next, addr != end);
+	spin_lock(&init_mm.page_table_lock);
+	head = pmd_leaf(*pmd) ? pmd_page(*pmd) : NULL;
+	spin_unlock(&init_mm.page_table_lock);
+	if (!head)
+		return 0;
 
-	return 0;
+	return vmemmap_split_pmd(pmd, head, addr & PMD_MASK, vmemmap_walk);
 }
 
-static int vmemmap_pud_range(p4d_t *p4d, unsigned long addr,
-			     unsigned long end,
-			     struct vmemmap_remap_walk *walk)
+static int vmemmap_pte_entry(pte_t *pte, unsigned long addr,
+			     unsigned long next, struct mm_walk *walk)
 {
-	pud_t *pud;
-	unsigned long next;
-
-	pud = pud_offset(p4d, addr);
-	do {
-		int ret;
+	struct vmemmap_remap_walk *vmemmap_walk = walk->private;
 
-		next = pud_addr_end(addr, end);
-		ret = vmemmap_pmd_range(pud, addr, next, walk);
-		if (ret)
-			return ret;
-	} while (pud++, addr = next, addr != end);
+	/*
+	 * The reuse_page is found 'first' in page table walking before
+	 * starting remapping.
+	 */
+	if (!vmemmap_walk->reuse_page)
+		vmemmap_walk->reuse_page = pte_page(ptep_get(pte));
+	else
+		vmemmap_walk->remap_pte(pte, addr, vmemmap_walk);
+	vmemmap_walk->nr_walked++;
 
 	return 0;
 }
 
-static int vmemmap_p4d_range(pgd_t *pgd, unsigned long addr,
-			     unsigned long end,
-			     struct vmemmap_remap_walk *walk)
-{
-	p4d_t *p4d;
-	unsigned long next;
-
-	p4d = p4d_offset(pgd, addr);
-	do {
-		int ret;
-
-		next = p4d_addr_end(addr, end);
-		ret = vmemmap_pud_range(p4d, addr, next, walk);
-		if (ret)
-			return ret;
-	} while (p4d++, addr = next, addr != end);
-
-	return 0;
-}
+static const struct mm_walk_ops vmemmap_remap_ops = {
+	.pmd_entry	= vmemmap_pmd_entry,
+	.pte_entry	= vmemmap_pte_entry,
+};
 
 static int vmemmap_remap_range(unsigned long start, unsigned long end,
 			       struct vmemmap_remap_walk *walk)
 {
-	unsigned long addr = start;
-	unsigned long next;
-	pgd_t *pgd;
-
-	VM_BUG_ON(!PAGE_ALIGNED(start));
-	VM_BUG_ON(!PAGE_ALIGNED(end));
+	int ret;
 
-	pgd = pgd_offset_k(addr);
-	do {
-		int ret;
+	VM_BUG_ON(!PAGE_ALIGNED(start | end));
 
-		next = pgd_addr_end(addr, end);
-		ret = vmemmap_p4d_range(pgd, addr, next, walk);
-		if (ret)
-			return ret;
-	} while (pgd++, addr = next, addr != end);
+	ret = walk_page_range_novma(&init_mm, start, end, &vmemmap_remap_ops,
+				    NULL, walk);
+	if (ret)
+		return ret;
 
 	if (walk->remap_pte && !(walk->flags & VMEMMAP_REMAP_NO_TLB_FLUSH))
 		flush_tlb_kernel_range(start, end);
-- 
2.20.1

Re: [PATCH 2/4] mm: hugetlb_vmemmap: use walk_page_range_novma() to simplify the code

[Mike Kravetz]

On 11/27/23 16:46, Muchun Song wrote:
> It is unnecessary to implement a series of dedicated page table walking
> helpers since there is already a general one walk_page_range_novma().
> So use it to simplify the code.

That is a very nice cleanup!
Much code is removed, but I could not find any issues.

> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/hugetlb_vmemmap.c | 148 ++++++++++++-------------------------------
>  1 file changed, 39 insertions(+), 109 deletions(-)

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

[PATCH 3/4] mm: hugetlb_vmemmap: move PageVmemmapSelfHosted() check to split_vmemmap_huge_pmd()

[Muchun Song]

To check a page whether it is self-hosted needs to traverse the page table (e.g.
pmd_off_k()), however, we already have done this in the next calling of
vmemmap_remap_range().  Moving PageVmemmapSelfHosted() check to vmemmap_pmd_entry()
could simplify the code a bit.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/hugetlb_vmemmap.c | 70 +++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 46 deletions(-)

diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
index ef14356855d13..ce920ca6c90ee 100644
--- a/mm/hugetlb_vmemmap.c
+++ b/mm/hugetlb_vmemmap.c
@@ -95,6 +95,7 @@ static int vmemmap_split_pmd(pmd_t *pmd, struct page *head, unsigned long start,
 static int vmemmap_pmd_entry(pmd_t *pmd, unsigned long addr,
 			     unsigned long next, struct mm_walk *walk)
 {
+	int ret = 0;
 	struct page *head;
 	struct vmemmap_remap_walk *vmemmap_walk = walk->private;
 
@@ -104,9 +105,30 @@ static int vmemmap_pmd_entry(pmd_t *pmd, unsigned long addr,
 
 	spin_lock(&init_mm.page_table_lock);
 	head = pmd_leaf(*pmd) ? pmd_page(*pmd) : NULL;
+	/*
+	 * Due to HugeTLB alignment requirements and the vmemmap
+	 * pages being at the start of the hotplugged memory
+	 * region in memory_hotplug.memmap_on_memory case. Checking
+	 * the vmemmap page associated with the first vmemmap page
+	 * if it is self-hosted is sufficient.
+	 *
+	 * [                  hotplugged memory                  ]
+	 * [        section        ][...][        section        ]
+	 * [ vmemmap ][              usable memory               ]
+	 *   ^  | ^                        |
+	 *   +--+ |                        |
+	 *        +------------------------+
+	 */
+	if (unlikely(!vmemmap_walk->nr_walked)) {
+		struct page *page = head ? head + pte_index(addr) :
+				    pte_page(ptep_get(pte_offset_kernel(pmd, addr)));
+
+		if (PageVmemmapSelfHosted(page))
+			ret = -ENOTSUPP;
+	}
 	spin_unlock(&init_mm.page_table_lock);
-	if (!head)
-		return 0;
+	if (!head || ret)
+		return ret;
 
 	return vmemmap_split_pmd(pmd, head, addr & PMD_MASK, vmemmap_walk);
 }
@@ -524,50 +546,6 @@ static bool vmemmap_should_optimize(const struct hstate *h, const struct page *h
 	if (!hugetlb_vmemmap_optimizable(h))
 		return false;
 
-	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG)) {
-		pmd_t *pmdp, pmd;
-		struct page *vmemmap_page;
-		unsigned long vaddr = (unsigned long)head;
-
-		/*
-		 * Only the vmemmap page's vmemmap page can be self-hosted.
-		 * Walking the page tables to find the backing page of the
-		 * vmemmap page.
-		 */
-		pmdp = pmd_off_k(vaddr);
-		/*
-		 * The READ_ONCE() is used to stabilize *pmdp in a register or
-		 * on the stack so that it will stop changing under the code.
-		 * The only concurrent operation where it can be changed is
-		 * split_vmemmap_huge_pmd() (*pmdp will be stable after this
-		 * operation).
-		 */
-		pmd = READ_ONCE(*pmdp);
-		if (pmd_leaf(pmd))
-			vmemmap_page = pmd_page(pmd) + pte_index(vaddr);
-		else
-			vmemmap_page = pte_page(*pte_offset_kernel(pmdp, vaddr));
-		/*
-		 * Due to HugeTLB alignment requirements and the vmemmap pages
-		 * being at the start of the hotplugged memory region in
-		 * memory_hotplug.memmap_on_memory case. Checking any vmemmap
-		 * page's vmemmap page if it is marked as VmemmapSelfHosted is
-		 * sufficient.
-		 *
-		 * [                  hotplugged memory                  ]
-		 * [        section        ][...][        section        ]
-		 * [ vmemmap ][              usable memory               ]
-		 *   ^   |     |                                        |
-		 *   +---+     |                                        |
-		 *     ^       |                                        |
-		 *     +-------+                                        |
-		 *          ^                                           |
-		 *          +-------------------------------------------+
-		 */
-		if (PageVmemmapSelfHosted(vmemmap_page))
-			return false;
-	}
-
 	return true;
 }
 
-- 
2.20.1

Re: [PATCH 3/4] mm: hugetlb_vmemmap: move PageVmemmapSelfHosted() check to split_vmemmap_huge_pmd()

[Mike Kravetz]

On 11/27/23 16:46, Muchun Song wrote:
> To check a page whether it is self-hosted needs to traverse the page table (e.g.
> pmd_off_k()), however, we already have done this in the next calling of
> vmemmap_remap_range().  Moving PageVmemmapSelfHosted() check to vmemmap_pmd_entry()
> could simplify the code a bit.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/hugetlb_vmemmap.c | 70 +++++++++++++++-----------------------------
>  1 file changed, 24 insertions(+), 46 deletions(-)
> 
> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
> index ef14356855d13..ce920ca6c90ee 100644
> --- a/mm/hugetlb_vmemmap.c
> +++ b/mm/hugetlb_vmemmap.c
> @@ -95,6 +95,7 @@ static int vmemmap_split_pmd(pmd_t *pmd, struct page *head, unsigned long start,
>  static int vmemmap_pmd_entry(pmd_t *pmd, unsigned long addr,
>  			     unsigned long next, struct mm_walk *walk)
>  {
> +	int ret = 0;
>  	struct page *head;
>  	struct vmemmap_remap_walk *vmemmap_walk = walk->private;
>  
> @@ -104,9 +105,30 @@ static int vmemmap_pmd_entry(pmd_t *pmd, unsigned long addr,
>  
>  	spin_lock(&init_mm.page_table_lock);
>  	head = pmd_leaf(*pmd) ? pmd_page(*pmd) : NULL;

I was going to ask why you dropped the:
	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
However, without the extra page table reads it really is not necessary.

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

Re: [PATCH 3/4] mm: hugetlb_vmemmap: move PageVmemmapSelfHosted() check to split_vmemmap_huge_pmd()

[Muchun Song]

> On Dec 5, 2023, at 07:27, Mike Kravetz <mike.kravetz@oracle.com> wrote:
> 
> On 11/27/23 16:46, Muchun Song wrote:
>> To check a page whether it is self-hosted needs to traverse the page table (e.g.
>> pmd_off_k()), however, we already have done this in the next calling of
>> vmemmap_remap_range().  Moving PageVmemmapSelfHosted() check to vmemmap_pmd_entry()
>> could simplify the code a bit.
>> 
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>> ---
>> mm/hugetlb_vmemmap.c | 70 +++++++++++++++-----------------------------
>> 1 file changed, 24 insertions(+), 46 deletions(-)
>> 
>> diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
>> index ef14356855d13..ce920ca6c90ee 100644
>> --- a/mm/hugetlb_vmemmap.c
>> +++ b/mm/hugetlb_vmemmap.c
>> @@ -95,6 +95,7 @@ static int vmemmap_split_pmd(pmd_t *pmd, struct page *head, unsigned long start,
>> static int vmemmap_pmd_entry(pmd_t *pmd, unsigned long addr,
>>      unsigned long next, struct mm_walk *walk)
>> {
>> + int ret = 0;
>> struct page *head;
>> struct vmemmap_remap_walk *vmemmap_walk = walk->private;
>> 
>> @@ -104,9 +105,30 @@ static int vmemmap_pmd_entry(pmd_t *pmd, unsigned long addr,
>> 
>> spin_lock(&init_mm.page_table_lock);
>> head = pmd_leaf(*pmd) ? pmd_page(*pmd) : NULL;
> 
> I was going to ask why you dropped the:
> if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
> However, without the extra page table reads it really is not necessary.

I thought the compiler is smart enough to find out this code
is unnecessary so that it will be dropped. But I am wrong,
I did a test showing the compiler does optimize the code
when CONFIG_MEMORY_HOTPLUG is disabled, but it is not
enough compared with adding the check of "IS_ENABLED(CONFIG_MEMORY_HOTPLUG)".
I will add the check back in a separate patch (I suspect it is easy
for Andrew to merge).

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

Thanks.

> -- 
> Mike Kravetz

[PATCH 4/4] mm: hugetlb_vmemmap: convert page to folio

[Muchun Song]

There is still some places where it does not be converted to folio,
this patch convert all of them to folio. And this patch also does
some trival cleanup to fix the code style problems.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/hugetlb_vmemmap.c | 51 ++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
index ce920ca6c90ee..54f388aa361fb 100644
--- a/mm/hugetlb_vmemmap.c
+++ b/mm/hugetlb_vmemmap.c
@@ -280,7 +280,7 @@ static void vmemmap_restore_pte(pte_t *pte, unsigned long addr,
  * Return: %0 on success, negative error code otherwise.
  */
 static int vmemmap_remap_split(unsigned long start, unsigned long end,
-				unsigned long reuse)
+			       unsigned long reuse)
 {
 	int ret;
 	struct vmemmap_remap_walk walk = {
@@ -447,14 +447,14 @@ EXPORT_SYMBOL(hugetlb_optimize_vmemmap_key);
 static bool vmemmap_optimize_enabled = IS_ENABLED(CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP_DEFAULT_ON);
 core_param(hugetlb_free_vmemmap, vmemmap_optimize_enabled, bool, 0);
 
-static int __hugetlb_vmemmap_restore_folio(const struct hstate *h, struct folio *folio, unsigned long flags)
+static int __hugetlb_vmemmap_restore_folio(const struct hstate *h,
+					   struct folio *folio, unsigned long flags)
 {
 	int ret;
-	struct page *head = &folio->page;
-	unsigned long vmemmap_start = (unsigned long)head, vmemmap_end;
+	unsigned long vmemmap_start = (unsigned long)&folio->page, vmemmap_end;
 	unsigned long vmemmap_reuse;
 
-	VM_WARN_ON_ONCE(!PageHuge(head));
+	VM_WARN_ON_ONCE_FOLIO(!folio_test_hugetlb(folio), folio);
 	if (!folio_test_hugetlb_vmemmap_optimized(folio))
 		return 0;
 
@@ -517,7 +517,7 @@ long hugetlb_vmemmap_restore_folios(const struct hstate *h,
 	list_for_each_entry_safe(folio, t_folio, folio_list, lru) {
 		if (folio_test_hugetlb_vmemmap_optimized(folio)) {
 			ret = __hugetlb_vmemmap_restore_folio(h, folio,
-						VMEMMAP_REMAP_NO_TLB_FLUSH);
+							      VMEMMAP_REMAP_NO_TLB_FLUSH);
 			if (ret)
 				break;
 			restored++;
@@ -535,9 +535,9 @@ long hugetlb_vmemmap_restore_folios(const struct hstate *h,
 }
 
 /* Return true iff a HugeTLB whose vmemmap should and can be optimized. */
-static bool vmemmap_should_optimize(const struct hstate *h, const struct page *head)
+static bool vmemmap_should_optimize_folio(const struct hstate *h, struct folio *folio)
 {
-	if (HPageVmemmapOptimized((struct page *)head))
+	if (folio_test_hugetlb_vmemmap_optimized(folio))
 		return false;
 
 	if (!READ_ONCE(vmemmap_optimize_enabled))
@@ -550,17 +550,16 @@ static bool vmemmap_should_optimize(const struct hstate *h, const struct page *h
 }
 
 static int __hugetlb_vmemmap_optimize_folio(const struct hstate *h,
-					struct folio *folio,
-					struct list_head *vmemmap_pages,
-					unsigned long flags)
+					    struct folio *folio,
+					    struct list_head *vmemmap_pages,
+					    unsigned long flags)
 {
 	int ret = 0;
-	struct page *head = &folio->page;
-	unsigned long vmemmap_start = (unsigned long)head, vmemmap_end;
+	unsigned long vmemmap_start = (unsigned long)&folio->page, vmemmap_end;
 	unsigned long vmemmap_reuse;
 
-	VM_WARN_ON_ONCE(!PageHuge(head));
-	if (!vmemmap_should_optimize(h, head))
+	VM_WARN_ON_ONCE_FOLIO(!folio_test_hugetlb(folio), folio);
+	if (!vmemmap_should_optimize_folio(h, folio))
 		return ret;
 
 	static_branch_inc(&hugetlb_optimize_vmemmap_key);
@@ -588,7 +587,7 @@ static int __hugetlb_vmemmap_optimize_folio(const struct hstate *h,
 	 * the caller.
 	 */
 	ret = vmemmap_remap_free(vmemmap_start, vmemmap_end, vmemmap_reuse,
-							vmemmap_pages, flags);
+				 vmemmap_pages, flags);
 	if (ret) {
 		static_branch_dec(&hugetlb_optimize_vmemmap_key);
 		folio_clear_hugetlb_vmemmap_optimized(folio);
@@ -615,12 +614,12 @@ void hugetlb_vmemmap_optimize_folio(const struct hstate *h, struct folio *folio)
 	free_vmemmap_page_list(&vmemmap_pages);
 }
 
-static int hugetlb_vmemmap_split(const struct hstate *h, struct page *head)
+static int hugetlb_vmemmap_split_folio(const struct hstate *h, struct folio *folio)
 {
-	unsigned long vmemmap_start = (unsigned long)head, vmemmap_end;
+	unsigned long vmemmap_start = (unsigned long)&folio->page, vmemmap_end;
 	unsigned long vmemmap_reuse;
 
-	if (!vmemmap_should_optimize(h, head))
+	if (!vmemmap_should_optimize_folio(h, folio))
 		return 0;
 
 	vmemmap_end	= vmemmap_start + hugetlb_vmemmap_size(h);
@@ -640,7 +639,7 @@ void hugetlb_vmemmap_optimize_folios(struct hstate *h, struct list_head *folio_l
 	LIST_HEAD(vmemmap_pages);
 
 	list_for_each_entry(folio, folio_list, lru) {
-		int ret = hugetlb_vmemmap_split(h, &folio->page);
+		int ret = hugetlb_vmemmap_split_folio(h, folio);
 
 		/*
 		 * Spliting the PMD requires allocating a page, thus lets fail
@@ -655,9 +654,10 @@ void hugetlb_vmemmap_optimize_folios(struct hstate *h, struct list_head *folio_l
 	flush_tlb_all();
 
 	list_for_each_entry(folio, folio_list, lru) {
-		int ret = __hugetlb_vmemmap_optimize_folio(h, folio,
-						&vmemmap_pages,
-						VMEMMAP_REMAP_NO_TLB_FLUSH);
+		int ret;
+
+		ret = __hugetlb_vmemmap_optimize_folio(h, folio, &vmemmap_pages,
+						       VMEMMAP_REMAP_NO_TLB_FLUSH);
 
 		/*
 		 * Pages to be freed may have been accumulated.  If we
@@ -671,9 +671,8 @@ void hugetlb_vmemmap_optimize_folios(struct hstate *h, struct list_head *folio_l
 			flush_tlb_all();
 			free_vmemmap_page_list(&vmemmap_pages);
 			INIT_LIST_HEAD(&vmemmap_pages);
-			__hugetlb_vmemmap_optimize_folio(h, folio,
-						&vmemmap_pages,
-						VMEMMAP_REMAP_NO_TLB_FLUSH);
+			__hugetlb_vmemmap_optimize_folio(h, folio, &vmemmap_pages,
+							 VMEMMAP_REMAP_NO_TLB_FLUSH);
 		}
 	}
 
-- 
2.20.1

Re: [PATCH 4/4] mm: hugetlb_vmemmap: convert page to folio

[Mike Kravetz]

On 11/27/23 16:46, Muchun Song wrote:
> There is still some places where it does not be converted to folio,
> this patch convert all of them to folio. And this patch also does
> some trival cleanup to fix the code style problems.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/hugetlb_vmemmap.c | 51 ++++++++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
...
> @@ -535,9 +535,9 @@ long hugetlb_vmemmap_restore_folios(const struct hstate *h,
>  }
>  
>  /* Return true iff a HugeTLB whose vmemmap should and can be optimized. */
> -static bool vmemmap_should_optimize(const struct hstate *h, const struct page *head)
> +static bool vmemmap_should_optimize_folio(const struct hstate *h, struct folio *folio)
>  {
> -	if (HPageVmemmapOptimized((struct page *)head))
> +	if (folio_test_hugetlb_vmemmap_optimized(folio))

Nice!  After this, there is only one remaining use of 'page based'
hugetlb specific flag macros.

	mm/memory_hotplug.c:            if (HPageMigratable(head))

When that is converted, all those macros can be removed from linux/hugetlb.h

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