[PATCH v5 0/6] userfaultfd: convert userfaultfd functions to use folios

[PATCH v5 0/6] userfaultfd: convert userfaultfd functions to use folios

[Peng Zhang]

From: ZhangPeng <zhangpeng362@huawei.com>

This patch series converts several userfaultfd functions to use folios.

Change log:

v4->v5:
- Update commit description and change page_kaddr to kaddr, suggested by
  Matthew Wilcox. (patch 1,6)
- Remove pages_per_huge_page from copy_user_folio(), suggested by
  Matthew Wilcox. (patch 5)
- Add RB from Sidhartha Kumar. (patch 1,3,4)

v3->v4:
- Rebase onto mm-unstable per Andrew Morton. Update commit description
  because some function names are changed. (patch 1,4,6)

v2->v3:
- Split patch 2 into three patches, suggested by Mike Kravetz. (patch
  2-4)
- Add a new patch to convert copy_user_huge_page to copy_user_folio,
  suggested by Mike Kravetz. (patch 5)
- Fix two uninitialized bugs, thanks to Dan Carpenter. (patch 6)
- Do some indenting cleanups.

v1->v2:
Modified patch 2, suggested by Matthew Wilcox:
- Rename copy_large_folio_from_user() to copy_folio_from_user().
- Delete the inner_folio.
- kmap() and kmap_atomic() are converted to kmap_local_page(). Use
  pagefault_disable() to ensure that a deadlock will not occur.
- flush_dcache_folio() is placed outside the loop.

ZhangPeng (6):
  userfaultfd: convert mfill_atomic_pte_copy() to use a folio
  userfaultfd: use kmap_local_page() in copy_huge_page_from_user()
  userfaultfd: convert copy_huge_page_from_user() to
    copy_folio_from_user()
  userfaultfd: convert mfill_atomic_hugetlb() to use a folio
  mm: convert copy_user_huge_page() to copy_user_folio()
  userfaultfd: convert mfill_atomic() to use a folio

 include/linux/hugetlb.h  |  4 +-
 include/linux/mm.h       | 14 +++----
 include/linux/shmem_fs.h |  4 +-
 mm/hugetlb.c             | 39 ++++++++----------
 mm/memory.c              | 64 ++++++++++++++---------------
 mm/shmem.c               | 16 ++++----
 mm/userfaultfd.c         | 89 ++++++++++++++++++++--------------------
 7 files changed, 110 insertions(+), 120 deletions(-)

-- 
2.25.1

[PATCH v5 1/6] userfaultfd: convert mfill_atomic_pte_copy() to use a folio

[Peng Zhang]

From: ZhangPeng <zhangpeng362@huawei.com>

Call vma_alloc_folio() directly instead of alloc_page_vma() and convert
page_kaddr to kaddr in mfill_atomic_pte_copy(). Removes several calls to
compound_head().

Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
---
 mm/userfaultfd.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 7f1b5f8b712c..24d6ed7ff302 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -135,17 +135,18 @@ static int mfill_atomic_pte_copy(pmd_t *dst_pmd,
 				 uffd_flags_t flags,
 				 struct page **pagep)
 {
-	void *page_kaddr;
+	void *kaddr;
 	int ret;
-	struct page *page;
+	struct folio *folio;
 
 	if (!*pagep) {
 		ret = -ENOMEM;
-		page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, dst_vma, dst_addr);
-		if (!page)
+		folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, dst_vma,
+					dst_addr, false);
+		if (!folio)
 			goto out;
 
-		page_kaddr = kmap_local_page(page);
+		kaddr = kmap_local_folio(folio, 0);
 		/*
 		 * The read mmap_lock is held here.  Despite the
 		 * mmap_lock being read recursive a deadlock is still
@@ -162,45 +163,45 @@ static int mfill_atomic_pte_copy(pmd_t *dst_pmd,
 		 * and retry the copy outside the mmap_lock.
 		 */
 		pagefault_disable();
-		ret = copy_from_user(page_kaddr,
-				     (const void __user *) src_addr,
+		ret = copy_from_user(kaddr, (const void __user *) src_addr,
 				     PAGE_SIZE);
 		pagefault_enable();
-		kunmap_local(page_kaddr);
+		kunmap_local(kaddr);
 
 		/* fallback to copy_from_user outside mmap_lock */
 		if (unlikely(ret)) {
 			ret = -ENOENT;
-			*pagep = page;
+			*pagep = &folio->page;
 			/* don't free the page */
 			goto out;
 		}
 
-		flush_dcache_page(page);
+		flush_dcache_folio(folio);
 	} else {
-		page = *pagep;
+		folio = page_folio(*pagep);
+		VM_BUG_ON_FOLIO(folio_test_large(folio), folio);
 		*pagep = NULL;
 	}
 
 	/*
-	 * The memory barrier inside __SetPageUptodate makes sure that
+	 * The memory barrier inside __folio_mark_uptodate makes sure that
 	 * preceding stores to the page contents become visible before
 	 * the set_pte_at() write.
 	 */
-	__SetPageUptodate(page);
+	__folio_mark_uptodate(folio);
 
 	ret = -ENOMEM;
-	if (mem_cgroup_charge(page_folio(page), dst_vma->vm_mm, GFP_KERNEL))
+	if (mem_cgroup_charge(folio, dst_vma->vm_mm, GFP_KERNEL))
 		goto out_release;
 
 	ret = mfill_atomic_install_pte(dst_pmd, dst_vma, dst_addr,
-				       page, true, flags);
+				       &folio->page, true, flags);
 	if (ret)
 		goto out_release;
 out:
 	return ret;
 out_release:
-	put_page(page);
+	folio_put(folio);
 	goto out;
 }
 
-- 
2.25.1

Re: [PATCH v5 1/6] userfaultfd: convert mfill_atomic_pte_copy() to use a folio

[Mike Kravetz]

On 03/31/23 17:39, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@huawei.com>
> 
> Call vma_alloc_folio() directly instead of alloc_page_vma() and convert
> page_kaddr to kaddr in mfill_atomic_pte_copy(). Removes several calls to
> compound_head().
> 
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>  mm/userfaultfd.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)

Looks good,

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

>  	} else {
> -		page = *pagep;
> +		folio = page_folio(*pagep);
> +		VM_BUG_ON_FOLIO(folio_test_large(folio), folio);
>  		*pagep = NULL;
>  	}

However, I am still unsure about the reason for adding the VM_BUG_ON_FOLIO
here.
-- 
Mike Kravetz

Re: [PATCH v5 1/6] userfaultfd: convert mfill_atomic_pte_copy() to use a folio

[zhangpeng (AS)]

On 2023/4/7 5:31, Mike Kravetz wrote:

> On 03/31/23 17:39, Peng Zhang wrote:
>> From: ZhangPeng <zhangpeng362@huawei.com>
>>
>> Call vma_alloc_folio() directly instead of alloc_page_vma() and convert
>> page_kaddr to kaddr in mfill_atomic_pte_copy(). Removes several calls to
>> compound_head().
>>
>> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
>> Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> ---
>>   mm/userfaultfd.c | 33 +++++++++++++++++----------------
>>   1 file changed, 17 insertions(+), 16 deletions(-)
> Looks good,
>
> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
>
>>   	} else {
>> -		page = *pagep;
>> +		folio = page_folio(*pagep);
>> +		VM_BUG_ON_FOLIO(folio_test_large(folio), folio);
>>   		*pagep = NULL;
>>   	}
> However, I am still unsure about the reason for adding the VM_BUG_ON_FOLIO
> here.

VM_BUG_ON_FOLIO was added to ensure that folio is a single-page folio.
However, the folio corresponding to the foliop is always a single-page
folio. We just don't need this check. I'll drop VM_BUG_ON_FOLIO.

Thanks for your review.

Best Regards,
Peng

[PATCH v5 2/6] userfaultfd: use kmap_local_page() in copy_huge_page_from_user()

[Peng Zhang]

From: ZhangPeng <zhangpeng362@huawei.com>

kmap() and kmap_atomic() are being deprecated in favor of
kmap_local_page() which is appropriate for any thread local context.[1]

Let's replace the kmap() and kmap_atomic() with kmap_local_page() in
copy_huge_page_from_user(). When allow_pagefault is false, disable page
faults to prevent potential deadlock.[2]

[1] https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com/
[2] https://lkml.kernel.org/r/20221025220136.2366143-1-ira.weiny@intel.com

Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
---
 mm/memory.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 387226d6094d..808f354bce65 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5880,16 +5880,14 @@ long copy_huge_page_from_user(struct page *dst_page,
 
 	for (i = 0; i < pages_per_huge_page; i++) {
 		subpage = nth_page(dst_page, i);
-		if (allow_pagefault)
-			page_kaddr = kmap(subpage);
-		else
-			page_kaddr = kmap_atomic(subpage);
+		page_kaddr = kmap_local_page(subpage);
+		if (!allow_pagefault)
+			pagefault_disable();
 		rc = copy_from_user(page_kaddr,
 				usr_src + i * PAGE_SIZE, PAGE_SIZE);
-		if (allow_pagefault)
-			kunmap(subpage);
-		else
-			kunmap_atomic(page_kaddr);
+		if (!allow_pagefault)
+			pagefault_enable();
+		kunmap_local(page_kaddr);
 
 		ret_val -= (PAGE_SIZE - rc);
 		if (rc)
-- 
2.25.1

Re: [PATCH v5 2/6] userfaultfd: use kmap_local_page() in copy_huge_page_from_user()

[Mike Kravetz]

On 03/31/23 17:39, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@huawei.com>
> 
> kmap() and kmap_atomic() are being deprecated in favor of
> kmap_local_page() which is appropriate for any thread local context.[1]
> 
> Let's replace the kmap() and kmap_atomic() with kmap_local_page() in
> copy_huge_page_from_user(). When allow_pagefault is false, disable page
> faults to prevent potential deadlock.[2]
> 
> [1] https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com/
> [2] https://lkml.kernel.org/r/20221025220136.2366143-1-ira.weiny@intel.com
> 
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> ---
>  mm/memory.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

Thanks,

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

[PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user() to copy_folio_from_user()

[Peng Zhang]

From: ZhangPeng <zhangpeng362@huawei.com>

Replace copy_huge_page_from_user() with copy_folio_from_user().
copy_folio_from_user() does the same as copy_huge_page_from_user(), but
takes in a folio instead of a page. Convert page_kaddr to kaddr in
copy_folio_from_user() to do indenting cleanup.

Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
---
 include/linux/mm.h |  7 +++----
 mm/hugetlb.c       |  5 ++---
 mm/memory.c        | 26 ++++++++++++--------------
 mm/userfaultfd.c   |  6 ++----
 4 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index e249208f8fbe..cf4d773ca506 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3682,10 +3682,9 @@ extern void copy_user_huge_page(struct page *dst, struct page *src,
 				unsigned long addr_hint,
 				struct vm_area_struct *vma,
 				unsigned int pages_per_huge_page);
-extern long copy_huge_page_from_user(struct page *dst_page,
-				const void __user *usr_src,
-				unsigned int pages_per_huge_page,
-				bool allow_pagefault);
+long copy_folio_from_user(struct folio *dst_folio,
+			   const void __user *usr_src,
+			   bool allow_pagefault);
 
 /**
  * vma_is_special_huge - Are transhuge page-table entries considered special?
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 7e4a80769c9e..aade1b513474 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6217,9 +6217,8 @@ int hugetlb_mfill_atomic_pte(pte_t *dst_pte,
 			goto out;
 		}
 
-		ret = copy_huge_page_from_user(&folio->page,
-						(const void __user *) src_addr,
-						pages_per_huge_page(h), false);
+		ret = copy_folio_from_user(folio, (const void __user *) src_addr,
+					   false);
 
 		/* fallback to copy_from_user outside mmap_lock */
 		if (unlikely(ret)) {
diff --git a/mm/memory.c b/mm/memory.c
index 808f354bce65..4976422b6979 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5868,35 +5868,33 @@ void copy_user_huge_page(struct page *dst, struct page *src,
 	process_huge_page(addr_hint, pages_per_huge_page, copy_subpage, &arg);
 }
 
-long copy_huge_page_from_user(struct page *dst_page,
-				const void __user *usr_src,
-				unsigned int pages_per_huge_page,
-				bool allow_pagefault)
+long copy_folio_from_user(struct folio *dst_folio,
+			   const void __user *usr_src,
+			   bool allow_pagefault)
 {
-	void *page_kaddr;
+	void *kaddr;
 	unsigned long i, rc = 0;
-	unsigned long ret_val = pages_per_huge_page * PAGE_SIZE;
+	unsigned int nr_pages = folio_nr_pages(dst_folio);
+	unsigned long ret_val = nr_pages * PAGE_SIZE;
 	struct page *subpage;
 
-	for (i = 0; i < pages_per_huge_page; i++) {
-		subpage = nth_page(dst_page, i);
-		page_kaddr = kmap_local_page(subpage);
+	for (i = 0; i < nr_pages; i++) {
+		subpage = folio_page(dst_folio, i);
+		kaddr = kmap_local_page(subpage);
 		if (!allow_pagefault)
 			pagefault_disable();
-		rc = copy_from_user(page_kaddr,
-				usr_src + i * PAGE_SIZE, PAGE_SIZE);
+		rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
 		if (!allow_pagefault)
 			pagefault_enable();
-		kunmap_local(page_kaddr);
+		kunmap_local(kaddr);
 
 		ret_val -= (PAGE_SIZE - rc);
 		if (rc)
 			break;
 
-		flush_dcache_page(subpage);
-
 		cond_resched();
 	}
+	flush_dcache_folio(dst_folio);
 	return ret_val;
 }
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE || CONFIG_HUGETLBFS */
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 24d6ed7ff302..78fed9003cf7 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -422,10 +422,8 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
 			mmap_read_unlock(dst_mm);
 			BUG_ON(!page);
 
-			err = copy_huge_page_from_user(page,
-						(const void __user *)src_addr,
-						vma_hpagesize / PAGE_SIZE,
-						true);
+			err = copy_folio_from_user(page_folio(page),
+						   (const void __user *)src_addr, true);
 			if (unlikely(err)) {
 				err = -EFAULT;
 				goto out;
-- 
2.25.1

Re: [PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user() to copy_folio_from_user()

[Mike Kravetz]

On 03/31/23 17:39, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@huawei.com>
> 
> Replace copy_huge_page_from_user() with copy_folio_from_user().
> copy_folio_from_user() does the same as copy_huge_page_from_user(), but
> takes in a folio instead of a page. Convert page_kaddr to kaddr in
> copy_folio_from_user() to do indenting cleanup.
> 
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>  include/linux/mm.h |  7 +++----
>  mm/hugetlb.c       |  5 ++---
>  mm/memory.c        | 26 ++++++++++++--------------
>  mm/userfaultfd.c   |  6 ++----
>  4 files changed, 19 insertions(+), 25 deletions(-)

Thanks,

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

Re: [PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user() to copy_folio_from_user()

[Vishal Moola]

On Fri, Mar 31, 2023 at 2:41 AM Peng Zhang <zhangpeng362@huawei.com> wrote:
>
> From: ZhangPeng <zhangpeng362@huawei.com>
>
> Replace copy_huge_page_from_user() with copy_folio_from_user().
> copy_folio_from_user() does the same as copy_huge_page_from_user(), but
> takes in a folio instead of a page. Convert page_kaddr to kaddr in
> copy_folio_from_user() to do indenting cleanup.
>
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>  include/linux/mm.h |  7 +++----
>  mm/hugetlb.c       |  5 ++---
>  mm/memory.c        | 26 ++++++++++++--------------
>  mm/userfaultfd.c   |  6 ++----
>  4 files changed, 19 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index e249208f8fbe..cf4d773ca506 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3682,10 +3682,9 @@ extern void copy_user_huge_page(struct page *dst, struct page *src,
>                                 unsigned long addr_hint,
>                                 struct vm_area_struct *vma,
>                                 unsigned int pages_per_huge_page);
> -extern long copy_huge_page_from_user(struct page *dst_page,
> -                               const void __user *usr_src,
> -                               unsigned int pages_per_huge_page,
> -                               bool allow_pagefault);
> +long copy_folio_from_user(struct folio *dst_folio,
> +                          const void __user *usr_src,
> +                          bool allow_pagefault);
>
>  /**
>   * vma_is_special_huge - Are transhuge page-table entries considered special?
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 7e4a80769c9e..aade1b513474 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6217,9 +6217,8 @@ int hugetlb_mfill_atomic_pte(pte_t *dst_pte,
>                         goto out;
>                 }
>
> -               ret = copy_huge_page_from_user(&folio->page,
> -                                               (const void __user *) src_addr,
> -                                               pages_per_huge_page(h), false);
> +               ret = copy_folio_from_user(folio, (const void __user *) src_addr,
> +                                          false);
>
>                 /* fallback to copy_from_user outside mmap_lock */
>                 if (unlikely(ret)) {
> diff --git a/mm/memory.c b/mm/memory.c
> index 808f354bce65..4976422b6979 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5868,35 +5868,33 @@ void copy_user_huge_page(struct page *dst, struct page *src,
>         process_huge_page(addr_hint, pages_per_huge_page, copy_subpage, &arg);
>  }
>
> -long copy_huge_page_from_user(struct page *dst_page,
> -                               const void __user *usr_src,
> -                               unsigned int pages_per_huge_page,
> -                               bool allow_pagefault)
> +long copy_folio_from_user(struct folio *dst_folio,
> +                          const void __user *usr_src,
> +                          bool allow_pagefault)
>  {
> -       void *page_kaddr;
> +       void *kaddr;
>         unsigned long i, rc = 0;
> -       unsigned long ret_val = pages_per_huge_page * PAGE_SIZE;
> +       unsigned int nr_pages = folio_nr_pages(dst_folio);
> +       unsigned long ret_val = nr_pages * PAGE_SIZE;
>         struct page *subpage;
>
> -       for (i = 0; i < pages_per_huge_page; i++) {
> -               subpage = nth_page(dst_page, i);
> -               page_kaddr = kmap_local_page(subpage);
> +       for (i = 0; i < nr_pages; i++) {
> +               subpage = folio_page(dst_folio, i);
> +               kaddr = kmap_local_page(subpage);
>                 if (!allow_pagefault)
>                         pagefault_disable();
> -               rc = copy_from_user(page_kaddr,
> -                               usr_src + i * PAGE_SIZE, PAGE_SIZE);
> +               rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
>                 if (!allow_pagefault)
>                         pagefault_enable();
> -               kunmap_local(page_kaddr);
> +               kunmap_local(kaddr);
>
>                 ret_val -= (PAGE_SIZE - rc);
>                 if (rc)
>                         break;
>
> -               flush_dcache_page(subpage);
> -
>                 cond_resched();
>         }
> +       flush_dcache_folio(dst_folio);
>         return ret_val;
>  }

Moving the flush_dcache_page() outside the loop to be
flush_dcache_folio() changes the behavior of the function.

Initially, if it fails to copy the entire page, the function breaks out
of the loop and returns the number of unwritten bytes without
flushing the page from the cache. Now if it fails, it will still flush
out the page it failed on, as well as any later pages it may not
have gotten to yet.

Re: [PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user() to copy_folio_from_user()

[zhangpeng (AS)]

On 2023/4/7 10:28, Vishal Moola wrote:

> On Fri, Mar 31, 2023 at 2:41 AM Peng Zhang <zhangpeng362@huawei.com> wrote:
>> From: ZhangPeng <zhangpeng362@huawei.com>
>>
>> Replace copy_huge_page_from_user() with copy_folio_from_user().
>> copy_folio_from_user() does the same as copy_huge_page_from_user(), but
>> takes in a folio instead of a page. Convert page_kaddr to kaddr in
>> copy_folio_from_user() to do indenting cleanup.
>>
>> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
>> Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> ---
>>   include/linux/mm.h |  7 +++----
>>   mm/hugetlb.c       |  5 ++---
>>   mm/memory.c        | 26 ++++++++++++--------------
>>   mm/userfaultfd.c   |  6 ++----
>>   4 files changed, 19 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index e249208f8fbe..cf4d773ca506 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -3682,10 +3682,9 @@ extern void copy_user_huge_page(struct page *dst, struct page *src,
>>                                  unsigned long addr_hint,
>>                                  struct vm_area_struct *vma,
>>                                  unsigned int pages_per_huge_page);
>> -extern long copy_huge_page_from_user(struct page *dst_page,
>> -                               const void __user *usr_src,
>> -                               unsigned int pages_per_huge_page,
>> -                               bool allow_pagefault);
>> +long copy_folio_from_user(struct folio *dst_folio,
>> +                          const void __user *usr_src,
>> +                          bool allow_pagefault);
>>
>>   /**
>>    * vma_is_special_huge - Are transhuge page-table entries considered special?
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 7e4a80769c9e..aade1b513474 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -6217,9 +6217,8 @@ int hugetlb_mfill_atomic_pte(pte_t *dst_pte,
>>                          goto out;
>>                  }
>>
>> -               ret = copy_huge_page_from_user(&folio->page,
>> -                                               (const void __user *) src_addr,
>> -                                               pages_per_huge_page(h), false);
>> +               ret = copy_folio_from_user(folio, (const void __user *) src_addr,
>> +                                          false);
>>
>>                  /* fallback to copy_from_user outside mmap_lock */
>>                  if (unlikely(ret)) {
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 808f354bce65..4976422b6979 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -5868,35 +5868,33 @@ void copy_user_huge_page(struct page *dst, struct page *src,
>>          process_huge_page(addr_hint, pages_per_huge_page, copy_subpage, &arg);
>>   }
>>
>> -long copy_huge_page_from_user(struct page *dst_page,
>> -                               const void __user *usr_src,
>> -                               unsigned int pages_per_huge_page,
>> -                               bool allow_pagefault)
>> +long copy_folio_from_user(struct folio *dst_folio,
>> +                          const void __user *usr_src,
>> +                          bool allow_pagefault)
>>   {
>> -       void *page_kaddr;
>> +       void *kaddr;
>>          unsigned long i, rc = 0;
>> -       unsigned long ret_val = pages_per_huge_page * PAGE_SIZE;
>> +       unsigned int nr_pages = folio_nr_pages(dst_folio);
>> +       unsigned long ret_val = nr_pages * PAGE_SIZE;
>>          struct page *subpage;
>>
>> -       for (i = 0; i < pages_per_huge_page; i++) {
>> -               subpage = nth_page(dst_page, i);
>> -               page_kaddr = kmap_local_page(subpage);
>> +       for (i = 0; i < nr_pages; i++) {
>> +               subpage = folio_page(dst_folio, i);
>> +               kaddr = kmap_local_page(subpage);
>>                  if (!allow_pagefault)
>>                          pagefault_disable();
>> -               rc = copy_from_user(page_kaddr,
>> -                               usr_src + i * PAGE_SIZE, PAGE_SIZE);
>> +               rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
>>                  if (!allow_pagefault)
>>                          pagefault_enable();
>> -               kunmap_local(page_kaddr);
>> +               kunmap_local(kaddr);
>>
>>                  ret_val -= (PAGE_SIZE - rc);
>>                  if (rc)
>>                          break;
>>
>> -               flush_dcache_page(subpage);
>> -
>>                  cond_resched();
>>          }
>> +       flush_dcache_folio(dst_folio);
>>          return ret_val;
>>   }
> Moving the flush_dcache_page() outside the loop to be
> flush_dcache_folio() changes the behavior of the function.
>
> Initially, if it fails to copy the entire page, the function breaks out
> of the loop and returns the number of unwritten bytes without
> flushing the page from the cache. Now if it fails, it will still flush
> out the page it failed on, as well as any later pages it may not
> have gotten to yet.

Agreed. If it fails, could we just not flush the folio?
Like this:
long copy_folio_from_user(...)
{
	...
	for (i = 0; i < nr_pages; i++) {
		...
		rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
		...
		ret_val -= (PAGE_SIZE - rc);
		if (rc)
-                       break;
+                       return ret_val;
		cond_resched();
	}
	flush_dcache_folio(dst_folio);
	return ret_val;
}

Thanks for your review.

Best Regards,
Peng

Re: [PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user() to copy_folio_from_user()

[Mike Kravetz]

On 04/08/23 12:43, zhangpeng (AS) wrote:
> On 2023/4/7 10:28, Vishal Moola wrote:
> 
> > On Fri, Mar 31, 2023 at 2:41 AM Peng Zhang <zhangpeng362@huawei.com> wrote:
> > > From: ZhangPeng <zhangpeng362@huawei.com>
> > > 
> > > Replace copy_huge_page_from_user() with copy_folio_from_user().
> > > copy_folio_from_user() does the same as copy_huge_page_from_user(), but
> > > takes in a folio instead of a page. Convert page_kaddr to kaddr in
> > > copy_folio_from_user() to do indenting cleanup.
> > > 
> > > Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> > > Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> > > ---
> > > -                               bool allow_pagefault)
> > > +long copy_folio_from_user(struct folio *dst_folio,
> > > +                          const void __user *usr_src,
> > > +                          bool allow_pagefault)
> > >   {
> > > -       void *page_kaddr;
> > > +       void *kaddr;
> > >          unsigned long i, rc = 0;
> > > -       unsigned long ret_val = pages_per_huge_page * PAGE_SIZE;
> > > +       unsigned int nr_pages = folio_nr_pages(dst_folio);
> > > +       unsigned long ret_val = nr_pages * PAGE_SIZE;
> > >          struct page *subpage;
> > > 
> > > -       for (i = 0; i < pages_per_huge_page; i++) {
> > > -               subpage = nth_page(dst_page, i);
> > > -               page_kaddr = kmap_local_page(subpage);
> > > +       for (i = 0; i < nr_pages; i++) {
> > > +               subpage = folio_page(dst_folio, i);
> > > +               kaddr = kmap_local_page(subpage);
> > >                  if (!allow_pagefault)
> > >                          pagefault_disable();
> > > -               rc = copy_from_user(page_kaddr,
> > > -                               usr_src + i * PAGE_SIZE, PAGE_SIZE);
> > > +               rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
> > >                  if (!allow_pagefault)
> > >                          pagefault_enable();
> > > -               kunmap_local(page_kaddr);
> > > +               kunmap_local(kaddr);
> > > 
> > >                  ret_val -= (PAGE_SIZE - rc);
> > >                  if (rc)
> > >                          break;
> > > 
> > > -               flush_dcache_page(subpage);
> > > -
> > >                  cond_resched();
> > >          }
> > > +       flush_dcache_folio(dst_folio);
> > >          return ret_val;
> > >   }
> > Moving the flush_dcache_page() outside the loop to be
> > flush_dcache_folio() changes the behavior of the function.
> > 
> > Initially, if it fails to copy the entire page, the function breaks out
> > of the loop and returns the number of unwritten bytes without
> > flushing the page from the cache. Now if it fails, it will still flush
> > out the page it failed on, as well as any later pages it may not
> > have gotten to yet.
> 
> Agreed. If it fails, could we just not flush the folio?

I believe that should be OK.  If returning an error, nobody should be
depending on any part of the page being present or not in the cache.
-- 
Mike Kravetz

> Like this:
> long copy_folio_from_user(...)
> {
> 	...
> 	for (i = 0; i < nr_pages; i++) {
> 		...
> 		rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
> 		...
> 		ret_val -= (PAGE_SIZE - rc);
> 		if (rc)
> -                       break;
> +                       return ret_val;
> 		cond_resched();
> 	}
> 	flush_dcache_folio(dst_folio);
> 	return ret_val;
> }
> 
> Thanks for your review.
> 
> Best Regards,
> Peng
> 

Re: [PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user() to copy_folio_from_user()

[Yin, Fengwei]

On 4/11/2023 5:26 AM, Mike Kravetz wrote:
> On 04/08/23 12:43, zhangpeng (AS) wrote:
>> On 2023/4/7 10:28, Vishal Moola wrote:
>>
>>> On Fri, Mar 31, 2023 at 2:41 AM Peng Zhang <zhangpeng362@huawei.com> wrote:
>>>> From: ZhangPeng <zhangpeng362@huawei.com>
>>>>
>>>> Replace copy_huge_page_from_user() with copy_folio_from_user().
>>>> copy_folio_from_user() does the same as copy_huge_page_from_user(), but
>>>> takes in a folio instead of a page. Convert page_kaddr to kaddr in
>>>> copy_folio_from_user() to do indenting cleanup.
>>>>
>>>> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
>>>> Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>>>> ---
>>>> -                               bool allow_pagefault)
>>>> +long copy_folio_from_user(struct folio *dst_folio,
>>>> +                          const void __user *usr_src,
>>>> +                          bool allow_pagefault)
>>>>   {
>>>> -       void *page_kaddr;
>>>> +       void *kaddr;
>>>>          unsigned long i, rc = 0;
>>>> -       unsigned long ret_val = pages_per_huge_page * PAGE_SIZE;
>>>> +       unsigned int nr_pages = folio_nr_pages(dst_folio);
>>>> +       unsigned long ret_val = nr_pages * PAGE_SIZE;
>>>>          struct page *subpage;
>>>>
>>>> -       for (i = 0; i < pages_per_huge_page; i++) {
>>>> -               subpage = nth_page(dst_page, i);
>>>> -               page_kaddr = kmap_local_page(subpage);
>>>> +       for (i = 0; i < nr_pages; i++) {
>>>> +               subpage = folio_page(dst_folio, i);
>>>> +               kaddr = kmap_local_page(subpage);
>>>>                  if (!allow_pagefault)
>>>>                          pagefault_disable();
>>>> -               rc = copy_from_user(page_kaddr,
>>>> -                               usr_src + i * PAGE_SIZE, PAGE_SIZE);
>>>> +               rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
>>>>                  if (!allow_pagefault)
>>>>                          pagefault_enable();
>>>> -               kunmap_local(page_kaddr);
>>>> +               kunmap_local(kaddr);
>>>>
>>>>                  ret_val -= (PAGE_SIZE - rc);
>>>>                  if (rc)
>>>>                          break;
>>>>
>>>> -               flush_dcache_page(subpage);
>>>> -
>>>>                  cond_resched();
>>>>          }
>>>> +       flush_dcache_folio(dst_folio);
>>>>          return ret_val;
>>>>   }
>>> Moving the flush_dcache_page() outside the loop to be
>>> flush_dcache_folio() changes the behavior of the function.
>>>
>>> Initially, if it fails to copy the entire page, the function breaks out
>>> of the loop and returns the number of unwritten bytes without
>>> flushing the page from the cache. Now if it fails, it will still flush
>>> out the page it failed on, as well as any later pages it may not
>>> have gotten to yet.
>>
>> Agreed. If it fails, could we just not flush the folio?
> 
> I believe that should be OK.  If returning an error, nobody should be
> depending on any part of the page being present or not in the cache.
Maybe we should flush_dcache because this function returns the 
bytes copied successfully? flushing cache to make sure the copied
pieces to RAM for sure.

For the range not copied yet, flushing cache or not doesn't make
difference. Thanks.

Regards
Yin, Fengwei

Re: [PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user() to copy_folio_from_user()

[Matthew Wilcox]

On Thu, Apr 06, 2023 at 07:28:44PM -0700, Vishal Moola wrote:
> > -               flush_dcache_page(subpage);
> > -
> >                 cond_resched();
> >         }
> > +       flush_dcache_folio(dst_folio);
> >         return ret_val;
> >  }
> 
> Moving the flush_dcache_page() outside the loop to be
> flush_dcache_folio() changes the behavior of the function.
> 
> Initially, if it fails to copy the entire page, the function breaks out
> of the loop and returns the number of unwritten bytes without
> flushing the page from the cache. Now if it fails, it will still flush
> out the page it failed on, as well as any later pages it may not
> have gotten to yet.

I'm not sure this is worth worrying about.  Failing to copy the entire
folio is unlikely, and if we do, flushing the entire folio instead of just
a few pages in it is harmless.  Plus I have patches which significantly
optiise flush_dcache_folio() over flush_dcache_page() (for the majority
of architectures) and so I think this change is actually beneficial in
the long term.

Re: [PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user() to copy_folio_from_user()

[Andrew Morton]

On Tue, 11 Apr 2023 04:40:17 +0100 Matthew Wilcox <willy@infradead.org> wrote:

> On Thu, Apr 06, 2023 at 07:28:44PM -0700, Vishal Moola wrote:
> > > -               flush_dcache_page(subpage);
> > > -
> > >                 cond_resched();
> > >         }
> > > +       flush_dcache_folio(dst_folio);
> > >         return ret_val;
> > >  }
> > 
> > Moving the flush_dcache_page() outside the loop to be
> > flush_dcache_folio() changes the behavior of the function.
> > 
> > Initially, if it fails to copy the entire page, the function breaks out
> > of the loop and returns the number of unwritten bytes without
> > flushing the page from the cache. Now if it fails, it will still flush
> > out the page it failed on, as well as any later pages it may not
> > have gotten to yet.
> 
> I'm not sure this is worth worrying about.  Failing to copy the entire
> folio is unlikely, and if we do, flushing the entire folio instead of just
> a few pages in it is harmless.  Plus I have patches which significantly
> optiise flush_dcache_folio() over flush_dcache_page() (for the majority
> of architectures) and so I think this change is actually beneficial in
> the long term.

Thanks, I'll send the series in for the next merge window as-is.  If
others remain unhappy with the flushing issue, please propose something
during the next -rc cycle.

[PATCH v5 4/6] userfaultfd: convert mfill_atomic_hugetlb() to use a folio

[Peng Zhang]

From: ZhangPeng <zhangpeng362@huawei.com>

Convert hugetlb_mfill_atomic_pte() to take in a folio pointer instead of
a page pointer. Convert mfill_atomic_hugetlb() to use a folio.

Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
---
 include/linux/hugetlb.h |  4 ++--
 mm/hugetlb.c            | 26 +++++++++++++-------------
 mm/userfaultfd.c        | 16 ++++++++--------
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 2a758bcd6719..28703fe22386 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -163,7 +163,7 @@ int hugetlb_mfill_atomic_pte(pte_t *dst_pte,
 			     unsigned long dst_addr,
 			     unsigned long src_addr,
 			     uffd_flags_t flags,
-			     struct page **pagep);
+			     struct folio **foliop);
 #endif /* CONFIG_USERFAULTFD */
 bool hugetlb_reserve_pages(struct inode *inode, long from, long to,
 						struct vm_area_struct *vma,
@@ -397,7 +397,7 @@ static inline int hugetlb_mfill_atomic_pte(pte_t *dst_pte,
 					   unsigned long dst_addr,
 					   unsigned long src_addr,
 					   uffd_flags_t flags,
-					   struct page **pagep)
+					   struct folio **foliop)
 {
 	BUG();
 	return 0;
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index aade1b513474..c88f856ec2e2 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6178,7 +6178,7 @@ int hugetlb_mfill_atomic_pte(pte_t *dst_pte,
 			     unsigned long dst_addr,
 			     unsigned long src_addr,
 			     uffd_flags_t flags,
-			     struct page **pagep)
+			     struct folio **foliop)
 {
 	struct mm_struct *dst_mm = dst_vma->vm_mm;
 	bool is_continue = uffd_flags_mode_is(flags, MFILL_ATOMIC_CONTINUE);
@@ -6201,8 +6201,8 @@ int hugetlb_mfill_atomic_pte(pte_t *dst_pte,
 		if (IS_ERR(folio))
 			goto out;
 		folio_in_pagecache = true;
-	} else if (!*pagep) {
-		/* If a page already exists, then it's UFFDIO_COPY for
+	} else if (!*foliop) {
+		/* If a folio already exists, then it's UFFDIO_COPY for
 		 * a non-missing case. Return -EEXIST.
 		 */
 		if (vm_shared &&
@@ -6237,33 +6237,33 @@ int hugetlb_mfill_atomic_pte(pte_t *dst_pte,
 				ret = -ENOMEM;
 				goto out;
 			}
-			*pagep = &folio->page;
-			/* Set the outparam pagep and return to the caller to
+			*foliop = folio;
+			/* Set the outparam foliop and return to the caller to
 			 * copy the contents outside the lock. Don't free the
-			 * page.
+			 * folio.
 			 */
 			goto out;
 		}
 	} else {
 		if (vm_shared &&
 		    hugetlbfs_pagecache_present(h, dst_vma, dst_addr)) {
-			put_page(*pagep);
+			folio_put(*foliop);
 			ret = -EEXIST;
-			*pagep = NULL;
+			*foliop = NULL;
 			goto out;
 		}
 
 		folio = alloc_hugetlb_folio(dst_vma, dst_addr, 0);
 		if (IS_ERR(folio)) {
-			put_page(*pagep);
+			folio_put(*foliop);
 			ret = -ENOMEM;
-			*pagep = NULL;
+			*foliop = NULL;
 			goto out;
 		}
-		copy_user_huge_page(&folio->page, *pagep, dst_addr, dst_vma,
+		copy_user_huge_page(&folio->page, &(*foliop)->page, dst_addr, dst_vma,
 				    pages_per_huge_page(h));
-		put_page(*pagep);
-		*pagep = NULL;
+		folio_put(*foliop);
+		*foliop = NULL;
 	}
 
 	/*
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 78fed9003cf7..f9ebaa079a6a 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -322,7 +322,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
 	pte_t *dst_pte;
 	unsigned long src_addr, dst_addr;
 	long copied;
-	struct page *page;
+	struct folio *folio;
 	unsigned long vma_hpagesize;
 	pgoff_t idx;
 	u32 hash;
@@ -342,7 +342,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
 	src_addr = src_start;
 	dst_addr = dst_start;
 	copied = 0;
-	page = NULL;
+	folio = NULL;
 	vma_hpagesize = vma_kernel_pagesize(dst_vma);
 
 	/*
@@ -411,7 +411,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
 		}
 
 		err = hugetlb_mfill_atomic_pte(dst_pte, dst_vma, dst_addr,
-					       src_addr, flags, &page);
+					       src_addr, flags, &folio);
 
 		hugetlb_vma_unlock_read(dst_vma);
 		mutex_unlock(&hugetlb_fault_mutex_table[hash]);
@@ -420,9 +420,9 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
 
 		if (unlikely(err == -ENOENT)) {
 			mmap_read_unlock(dst_mm);
-			BUG_ON(!page);
+			BUG_ON(!folio);
 
-			err = copy_folio_from_user(page_folio(page),
+			err = copy_folio_from_user(folio,
 						   (const void __user *)src_addr, true);
 			if (unlikely(err)) {
 				err = -EFAULT;
@@ -433,7 +433,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
 			dst_vma = NULL;
 			goto retry;
 		} else
-			BUG_ON(page);
+			BUG_ON(folio);
 
 		if (!err) {
 			dst_addr += vma_hpagesize;
@@ -450,8 +450,8 @@ static __always_inline ssize_t mfill_atomic_hugetlb(
 out_unlock:
 	mmap_read_unlock(dst_mm);
 out:
-	if (page)
-		put_page(page);
+	if (folio)
+		folio_put(folio);
 	BUG_ON(copied < 0);
 	BUG_ON(err > 0);
 	BUG_ON(!copied && !err);
-- 
2.25.1

Re: [PATCH v5 4/6] userfaultfd: convert mfill_atomic_hugetlb() to use a folio

[Mike Kravetz]

On 03/31/23 17:39, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@huawei.com>
> 
> Convert hugetlb_mfill_atomic_pte() to take in a folio pointer instead of
> a page pointer. Convert mfill_atomic_hugetlb() to use a folio.
> 
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>  include/linux/hugetlb.h |  4 ++--
>  mm/hugetlb.c            | 26 +++++++++++++-------------
>  mm/userfaultfd.c        | 16 ++++++++--------
>  3 files changed, 23 insertions(+), 23 deletions(-)

Thanks,

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

[PATCH v5 5/6] mm: convert copy_user_huge_page() to copy_user_folio()

[Peng Zhang]

From: ZhangPeng <zhangpeng362@huawei.com>

Replace copy_user_huge_page() with copy_user_folio(). copy_user_folio()
does the same as copy_user_huge_page(), but takes in folios instead of
pages. Convert copy_user_gigantic_page() to take in folios.
Remove pages_per_huge_page from copy_user_folio(), because we can get
that from folio_nr_pages(dst).

Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
---
 include/linux/mm.h |  7 +++----
 mm/hugetlb.c       | 10 ++++------
 mm/memory.c        | 28 ++++++++++++++--------------
 3 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index cf4d773ca506..898ece0a3802 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3678,10 +3678,9 @@ extern const struct attribute_group memory_failure_attr_group;
 extern void clear_huge_page(struct page *page,
 			    unsigned long addr_hint,
 			    unsigned int pages_per_huge_page);
-extern void copy_user_huge_page(struct page *dst, struct page *src,
-				unsigned long addr_hint,
-				struct vm_area_struct *vma,
-				unsigned int pages_per_huge_page);
+void copy_user_folio(struct folio *dst, struct folio *src,
+		      unsigned long addr_hint,
+		      struct vm_area_struct *vma);
 long copy_folio_from_user(struct folio *dst_folio,
 			   const void __user *usr_src,
 			   bool allow_pagefault);
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index c88f856ec2e2..a7ed17cbc84e 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5097,8 +5097,8 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 					ret = PTR_ERR(new_folio);
 					break;
 				}
-				copy_user_huge_page(&new_folio->page, ptepage, addr, dst_vma,
-						    npages);
+				copy_user_folio(new_folio, page_folio(ptepage),
+						addr, dst_vma);
 				put_page(ptepage);
 
 				/* Install the new hugetlb folio if src pte stable */
@@ -5616,8 +5616,7 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
 		goto out_release_all;
 	}
 
-	copy_user_huge_page(&new_folio->page, old_page, address, vma,
-			    pages_per_huge_page(h));
+	copy_user_folio(new_folio, page_folio(old_page), address, vma);
 	__folio_mark_uptodate(new_folio);
 
 	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm, haddr,
@@ -6260,8 +6259,7 @@ int hugetlb_mfill_atomic_pte(pte_t *dst_pte,
 			*foliop = NULL;
 			goto out;
 		}
-		copy_user_huge_page(&folio->page, &(*foliop)->page, dst_addr, dst_vma,
-				    pages_per_huge_page(h));
+		copy_user_folio(folio, *foliop, dst_addr, dst_vma);
 		folio_put(*foliop);
 		*foliop = NULL;
 	}
diff --git a/mm/memory.c b/mm/memory.c
index 4976422b6979..0a5cefea9774 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5815,21 +5815,21 @@ void clear_huge_page(struct page *page,
 	process_huge_page(addr_hint, pages_per_huge_page, clear_subpage, page);
 }
 
-static void copy_user_gigantic_page(struct page *dst, struct page *src,
-				    unsigned long addr,
-				    struct vm_area_struct *vma,
-				    unsigned int pages_per_huge_page)
+static void copy_user_gigantic_page(struct folio *dst, struct folio *src,
+				     unsigned long addr,
+				     struct vm_area_struct *vma,
+				     unsigned int pages_per_huge_page)
 {
 	int i;
-	struct page *dst_base = dst;
-	struct page *src_base = src;
+	struct page *dst_page;
+	struct page *src_page;
 
 	for (i = 0; i < pages_per_huge_page; i++) {
-		dst = nth_page(dst_base, i);
-		src = nth_page(src_base, i);
+		dst_page = folio_page(dst, i);
+		src_page = folio_page(src, i);
 
 		cond_resched();
-		copy_user_highpage(dst, src, addr + i*PAGE_SIZE, vma);
+		copy_user_highpage(dst_page, src_page, addr + i*PAGE_SIZE, vma);
 	}
 }
 
@@ -5847,15 +5847,15 @@ static void copy_subpage(unsigned long addr, int idx, void *arg)
 			   addr, copy_arg->vma);
 }
 
-void copy_user_huge_page(struct page *dst, struct page *src,
-			 unsigned long addr_hint, struct vm_area_struct *vma,
-			 unsigned int pages_per_huge_page)
+void copy_user_folio(struct folio *dst, struct folio *src,
+		      unsigned long addr_hint, struct vm_area_struct *vma)
 {
+	unsigned int pages_per_huge_page = folio_nr_pages(dst);
 	unsigned long addr = addr_hint &
 		~(((unsigned long)pages_per_huge_page << PAGE_SHIFT) - 1);
 	struct copy_subpage_arg arg = {
-		.dst = dst,
-		.src = src,
+		.dst = &dst->page,
+		.src = &src->page,
 		.vma = vma,
 	};
 
-- 
2.25.1

Re: [PATCH v5 5/6] mm: convert copy_user_huge_page() to copy_user_folio()

[Mike Kravetz]

On 03/31/23 17:39, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@huawei.com>
> 
> Replace copy_user_huge_page() with copy_user_folio(). copy_user_folio()
> does the same as copy_user_huge_page(), but takes in folios instead of
> pages. Convert copy_user_gigantic_page() to take in folios.
> Remove pages_per_huge_page from copy_user_folio(), because we can get
> that from folio_nr_pages(dst).
> 
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> ---
>  include/linux/mm.h |  7 +++----
>  mm/hugetlb.c       | 10 ++++------
>  mm/memory.c        | 28 ++++++++++++++--------------
>  3 files changed, 21 insertions(+), 24 deletions(-)

No technical problems with the patch, but ...
>  
> @@ -5847,15 +5847,15 @@ static void copy_subpage(unsigned long addr, int idx, void *arg)
>  			   addr, copy_arg->vma);
>  }
>  
> -void copy_user_huge_page(struct page *dst, struct page *src,
> -			 unsigned long addr_hint, struct vm_area_struct *vma,
> -			 unsigned int pages_per_huge_page)
> +void copy_user_folio(struct folio *dst, struct folio *src,
> +		      unsigned long addr_hint, struct vm_area_struct *vma)
>  {
> +	unsigned int pages_per_huge_page = folio_nr_pages(dst);
>  	unsigned long addr = addr_hint &
>  		~(((unsigned long)pages_per_huge_page << PAGE_SHIFT) - 1);
>  	struct copy_subpage_arg arg = {
> -		.dst = dst,
> -		.src = src,
> +		.dst = &dst->page,
> +		.src = &src->page,
>  		.vma = vma,
>  	};
>  

I seem to recall that Matthew suggested changing the function name to
copy_user_folio.  My only concern is that the name now sounds like a
general purpose routine for copying folios.  It certainly would work
for a single page folio, but there is a bunch of unnecessary overhead
in that case.

That makes me think there should perhaps be an optimized path for single
page folios that just does copy_user_highpage().  But, the argument addr_hint
does not make much sense in the single page folio case.  So, I am not
sure if I agree with leaving large/huge out of the function name.

Just wondering if Matthew has any additional thoughts?
-- 
Mike Kravetz

Re: [PATCH v5 5/6] mm: convert copy_user_huge_page() to copy_user_folio()

[zhangpeng (AS)]

On 2023/4/7 7:55, Mike Kravetz wrote:

> On 03/31/23 17:39, Peng Zhang wrote:
>> From: ZhangPeng <zhangpeng362@huawei.com>
>>
>> Replace copy_user_huge_page() with copy_user_folio(). copy_user_folio()
>> does the same as copy_user_huge_page(), but takes in folios instead of
>> pages. Convert copy_user_gigantic_page() to take in folios.
>> Remove pages_per_huge_page from copy_user_folio(), because we can get
>> that from folio_nr_pages(dst).
>>
>> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
>> ---
>>   include/linux/mm.h |  7 +++----
>>   mm/hugetlb.c       | 10 ++++------
>>   mm/memory.c        | 28 ++++++++++++++--------------
>>   3 files changed, 21 insertions(+), 24 deletions(-)
> No technical problems with the patch, but ...
>>   
>> @@ -5847,15 +5847,15 @@ static void copy_subpage(unsigned long addr, int idx, void *arg)
>>   			   addr, copy_arg->vma);
>>   }
>>   
>> -void copy_user_huge_page(struct page *dst, struct page *src,
>> -			 unsigned long addr_hint, struct vm_area_struct *vma,
>> -			 unsigned int pages_per_huge_page)
>> +void copy_user_folio(struct folio *dst, struct folio *src,
>> +		      unsigned long addr_hint, struct vm_area_struct *vma)
>>   {
>> +	unsigned int pages_per_huge_page = folio_nr_pages(dst);
>>   	unsigned long addr = addr_hint &
>>   		~(((unsigned long)pages_per_huge_page << PAGE_SHIFT) - 1);
>>   	struct copy_subpage_arg arg = {
>> -		.dst = dst,
>> -		.src = src,
>> +		.dst = &dst->page,
>> +		.src = &src->page,
>>   		.vma = vma,
>>   	};
>>   
> I seem to recall that Matthew suggested changing the function name to
> copy_user_folio.  My only concern is that the name now sounds like a
> general purpose routine for copying folios.  It certainly would work
> for a single page folio, but there is a bunch of unnecessary overhead
> in that case.
>
> That makes me think there should perhaps be an optimized path for single
> page folios that just does copy_user_highpage().  But, the argument addr_hint
> does not make much sense in the single page folio case.  So, I am not
> sure if I agree with leaving large/huge out of the function name.
>
> Just wondering if Matthew has any additional thoughts?

Agreed. In my opinion, it's better to leave large/huge out of the
function name.
Also wondering if Matthew has any additional considerations?

Best Regards,
Peng

[PATCH v5 6/6] userfaultfd: convert mfill_atomic() to use a folio

[Peng Zhang]

From: ZhangPeng <zhangpeng362@huawei.com>

Convert mfill_atomic_pte_copy(), shmem_mfill_atomic_pte() and
mfill_atomic_pte() to take in a folio pointer.
Convert mfill_atomic() to use a folio. Convert page_kaddr to kaddr in
mfill_atomic().

Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
---
 include/linux/shmem_fs.h |  4 ++--
 mm/shmem.c               | 16 ++++++++--------
 mm/userfaultfd.c         | 40 ++++++++++++++++++++--------------------
 3 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
index 3bb8d21edbb3..9e151ba45068 100644
--- a/include/linux/shmem_fs.h
+++ b/include/linux/shmem_fs.h
@@ -158,10 +158,10 @@ extern int shmem_mfill_atomic_pte(pmd_t *dst_pmd,
 				  unsigned long dst_addr,
 				  unsigned long src_addr,
 				  uffd_flags_t flags,
-				  struct page **pagep);
+				  struct folio **foliop);
 #else /* !CONFIG_SHMEM */
 #define shmem_mfill_atomic_pte(dst_pmd, dst_vma, dst_addr, \
-			       src_addr, flags, pagep) ({ BUG(); 0; })
+			       src_addr, flags, foliop) ({ BUG(); 0; })
 #endif /* CONFIG_SHMEM */
 #endif /* CONFIG_USERFAULTFD */
 
diff --git a/mm/shmem.c b/mm/shmem.c
index 6c08f5a75d3a..9218c955f482 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2548,7 +2548,7 @@ int shmem_mfill_atomic_pte(pmd_t *dst_pmd,
 			   unsigned long dst_addr,
 			   unsigned long src_addr,
 			   uffd_flags_t flags,
-			   struct page **pagep)
+			   struct folio **foliop)
 {
 	struct inode *inode = file_inode(dst_vma->vm_file);
 	struct shmem_inode_info *info = SHMEM_I(inode);
@@ -2566,14 +2566,14 @@ int shmem_mfill_atomic_pte(pmd_t *dst_pmd,
 		 * and now we find ourselves with -ENOMEM. Release the page, to
 		 * avoid a BUG_ON in our caller.
 		 */
-		if (unlikely(*pagep)) {
-			put_page(*pagep);
-			*pagep = NULL;
+		if (unlikely(*foliop)) {
+			folio_put(*foliop);
+			*foliop = NULL;
 		}
 		return -ENOMEM;
 	}
 
-	if (!*pagep) {
+	if (!*foliop) {
 		ret = -ENOMEM;
 		folio = shmem_alloc_folio(gfp, info, pgoff);
 		if (!folio)
@@ -2605,7 +2605,7 @@ int shmem_mfill_atomic_pte(pmd_t *dst_pmd,
 
 			/* fallback to copy_from_user outside mmap_lock */
 			if (unlikely(ret)) {
-				*pagep = &folio->page;
+				*foliop = folio;
 				ret = -ENOENT;
 				/* don't free the page */
 				goto out_unacct_blocks;
@@ -2616,9 +2616,9 @@ int shmem_mfill_atomic_pte(pmd_t *dst_pmd,
 			clear_user_highpage(&folio->page, dst_addr);
 		}
 	} else {
-		folio = page_folio(*pagep);
+		folio = *foliop;
 		VM_BUG_ON_FOLIO(folio_test_large(folio), folio);
-		*pagep = NULL;
+		*foliop = NULL;
 	}
 
 	VM_BUG_ON(folio_test_locked(folio));
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index f9ebaa079a6a..be423189120d 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -133,13 +133,13 @@ static int mfill_atomic_pte_copy(pmd_t *dst_pmd,
 				 unsigned long dst_addr,
 				 unsigned long src_addr,
 				 uffd_flags_t flags,
-				 struct page **pagep)
+				 struct folio **foliop)
 {
 	void *kaddr;
 	int ret;
 	struct folio *folio;
 
-	if (!*pagep) {
+	if (!*foliop) {
 		ret = -ENOMEM;
 		folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, dst_vma,
 					dst_addr, false);
@@ -171,16 +171,16 @@ static int mfill_atomic_pte_copy(pmd_t *dst_pmd,
 		/* fallback to copy_from_user outside mmap_lock */
 		if (unlikely(ret)) {
 			ret = -ENOENT;
-			*pagep = &folio->page;
+			*foliop = folio;
 			/* don't free the page */
 			goto out;
 		}
 
 		flush_dcache_folio(folio);
 	} else {
-		folio = page_folio(*pagep);
+		folio = *foliop;
 		VM_BUG_ON_FOLIO(folio_test_large(folio), folio);
-		*pagep = NULL;
+		*foliop = NULL;
 	}
 
 	/*
@@ -471,7 +471,7 @@ static __always_inline ssize_t mfill_atomic_pte(pmd_t *dst_pmd,
 						unsigned long dst_addr,
 						unsigned long src_addr,
 						uffd_flags_t flags,
-						struct page **pagep)
+						struct folio **foliop)
 {
 	ssize_t err;
 
@@ -494,14 +494,14 @@ static __always_inline ssize_t mfill_atomic_pte(pmd_t *dst_pmd,
 		if (uffd_flags_mode_is(flags, MFILL_ATOMIC_COPY))
 			err = mfill_atomic_pte_copy(dst_pmd, dst_vma,
 						    dst_addr, src_addr,
-						    flags, pagep);
+						    flags, foliop);
 		else
 			err = mfill_atomic_pte_zeropage(dst_pmd,
 						 dst_vma, dst_addr);
 	} else {
 		err = shmem_mfill_atomic_pte(dst_pmd, dst_vma,
 					     dst_addr, src_addr,
-					     flags, pagep);
+					     flags, foliop);
 	}
 
 	return err;
@@ -519,7 +519,7 @@ static __always_inline ssize_t mfill_atomic(struct mm_struct *dst_mm,
 	pmd_t *dst_pmd;
 	unsigned long src_addr, dst_addr;
 	long copied;
-	struct page *page;
+	struct folio *folio;
 
 	/*
 	 * Sanitize the command parameters:
@@ -534,7 +534,7 @@ static __always_inline ssize_t mfill_atomic(struct mm_struct *dst_mm,
 	src_addr = src_start;
 	dst_addr = dst_start;
 	copied = 0;
-	page = NULL;
+	folio = NULL;
 retry:
 	mmap_read_lock(dst_mm);
 
@@ -630,28 +630,28 @@ static __always_inline ssize_t mfill_atomic(struct mm_struct *dst_mm,
 		BUG_ON(pmd_trans_huge(*dst_pmd));
 
 		err = mfill_atomic_pte(dst_pmd, dst_vma, dst_addr,
-				       src_addr, flags, &page);
+				       src_addr, flags, &folio);
 		cond_resched();
 
 		if (unlikely(err == -ENOENT)) {
-			void *page_kaddr;
+			void *kaddr;
 
 			mmap_read_unlock(dst_mm);
-			BUG_ON(!page);
+			BUG_ON(!folio);
 
-			page_kaddr = kmap_local_page(page);
-			err = copy_from_user(page_kaddr,
+			kaddr = kmap_local_folio(folio, 0);
+			err = copy_from_user(kaddr,
 					     (const void __user *) src_addr,
 					     PAGE_SIZE);
-			kunmap_local(page_kaddr);
+			kunmap_local(kaddr);
 			if (unlikely(err)) {
 				err = -EFAULT;
 				goto out;
 			}
-			flush_dcache_page(page);
+			flush_dcache_folio(folio);
 			goto retry;
 		} else
-			BUG_ON(page);
+			BUG_ON(folio);
 
 		if (!err) {
 			dst_addr += PAGE_SIZE;
@@ -668,8 +668,8 @@ static __always_inline ssize_t mfill_atomic(struct mm_struct *dst_mm,
 out_unlock:
 	mmap_read_unlock(dst_mm);
 out:
-	if (page)
-		put_page(page);
+	if (folio)
+		folio_put(folio);
 	BUG_ON(copied < 0);
 	BUG_ON(err > 0);
 	BUG_ON(!copied && !err);
-- 
2.25.1

Re: [PATCH v5 6/6] userfaultfd: convert mfill_atomic() to use a folio

[Mike Kravetz]

On 03/31/23 17:39, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@huawei.com>
> 
> Convert mfill_atomic_pte_copy(), shmem_mfill_atomic_pte() and
> mfill_atomic_pte() to take in a folio pointer.
> Convert mfill_atomic() to use a folio. Convert page_kaddr to kaddr in
> mfill_atomic().
> 
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> ---
>  include/linux/shmem_fs.h |  4 ++--
>  mm/shmem.c               | 16 ++++++++--------
>  mm/userfaultfd.c         | 40 ++++++++++++++++++++--------------------
>  3 files changed, 30 insertions(+), 30 deletions(-)

Looks good to me,

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