All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
@ 2025-08-07 10:39 Lokesh Gidra
  2025-08-07 19:16 ` Peter Xu
  2025-08-08  6:18 ` Dan Carpenter
  0 siblings, 2 replies; 11+ messages in thread
From: Lokesh Gidra @ 2025-08-07 10:39 UTC (permalink / raw)
  To: akpm
  Cc: aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
	Lokesh Gidra, Suren Baghdasaryan, Kalesh Singh, Barry Song,
	David Hildenbrand, Peter Xu

MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
for moving present pages. Mitigate this cost by opportunistically
batching present contiguous pages for TLB flushing.

Without batching, in our testing on an arm64 Android device with UFFD GC,
which uses MOVE ioctl for compaction, we observed that out of the total
time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
~20% in vm_normal_folio().

With batching, the proportion of vm_normal_folio() increases to over
70% of move_pages_pte() without any changes to vm_normal_folio().
Furthermore, time spent within move_pages_pte() is only ~20%, which
includes TLB-flush overhead.

Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Kalesh Singh <kaleshsingh@google.com>
Cc: Barry Song <v-songbaohua@oppo.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
---
Changes since v2 [1]
- Addressed VM_WARN_ON failure, per Lorenzo Stoakes
- Added check to ensure all batched pages share the same anon_vma

Changes since v1 [2]
- Removed flush_tlb_batched_pending(), per Barry Song
- Unified single and multi page case, per Barry Song

[1] https://lore.kernel.org/all/20250805121410.1658418-1-lokeshgidra@google.com/
[2] https://lore.kernel.org/all/20250731104726.103071-1-lokeshgidra@google.com/

 mm/userfaultfd.c | 179 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 128 insertions(+), 51 deletions(-)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index cbed91b09640..78c732100aec 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -1026,18 +1026,64 @@ static inline bool is_pte_pages_stable(pte_t *dst_pte, pte_t *src_pte,
 	       pmd_same(dst_pmdval, pmdp_get_lockless(dst_pmd));
 }
 
-static int move_present_pte(struct mm_struct *mm,
-			    struct vm_area_struct *dst_vma,
-			    struct vm_area_struct *src_vma,
-			    unsigned long dst_addr, unsigned long src_addr,
-			    pte_t *dst_pte, pte_t *src_pte,
-			    pte_t orig_dst_pte, pte_t orig_src_pte,
-			    pmd_t *dst_pmd, pmd_t dst_pmdval,
-			    spinlock_t *dst_ptl, spinlock_t *src_ptl,
-			    struct folio *src_folio)
+/*
+ * Checks if the two ptes and the corresponding folio are eligible for batched
+ * move. If so, then returns pointer to the folio, after locking it. Otherwise,
+ * returns NULL.
+ */
+static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
+						 unsigned long src_addr,
+						 pte_t *src_pte, pte_t *dst_pte,
+						 struct anon_vma *src_anon_vma)
+{
+	pte_t orig_dst_pte, orig_src_pte;
+	struct folio *folio;
+
+	orig_dst_pte = ptep_get(dst_pte);
+	if (!pte_none(orig_dst_pte))
+		return NULL;
+
+	orig_src_pte = ptep_get(src_pte);
+	if (pte_none(orig_src_pte) || !pte_present(orig_src_pte) ||
+	    is_zero_pfn(pte_pfn(orig_src_pte)))
+		return NULL;
+
+	folio = vm_normal_folio(src_vma, src_addr, orig_src_pte);
+	if (!folio || !folio_trylock(folio))
+		return NULL;
+	if (!PageAnonExclusive(&folio->page) || folio_test_large(folio) ||
+	    folio_anon_vma(folio) != src_anon_vma) {
+		folio_unlock(folio);
+		return NULL;
+	}
+	return folio;
+}
+
+static long move_present_ptes(struct mm_struct *mm,
+			      struct vm_area_struct *dst_vma,
+			      struct vm_area_struct *src_vma,
+			      unsigned long dst_addr, unsigned long src_addr,
+			      pte_t *dst_pte, pte_t *src_pte,
+			      pte_t orig_dst_pte, pte_t orig_src_pte,
+			      pmd_t *dst_pmd, pmd_t dst_pmdval,
+			      spinlock_t *dst_ptl, spinlock_t *src_ptl,
+			      struct folio *src_folio, unsigned long len,
+			      struct anon_vma *src_anon_vma)
 {
 	int err = 0;
+	unsigned long src_start = src_addr;
+	unsigned long addr_end;
 
+	if (len > PAGE_SIZE) {
+		addr_end = (dst_addr + PMD_SIZE) & PMD_MASK;
+		if (dst_addr + len > addr_end)
+			len = addr_end - dst_addr;
+
+		addr_end = (src_addr + PMD_SIZE) & PMD_MASK;
+		if (src_addr + len > addr_end)
+			len = addr_end - src_addr;
+	}
+	flush_cache_range(src_vma, src_addr, src_addr + len);
 	double_pt_lock(dst_ptl, src_ptl);
 
 	if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
@@ -1051,31 +1097,54 @@ static int move_present_pte(struct mm_struct *mm,
 		err = -EBUSY;
 		goto out;
 	}
+	arch_enter_lazy_mmu_mode();
+
+	addr_end = src_start + len;
+	while (true) {
+		orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
+		/* Folio got pinned from under us. Put it back and fail the move. */
+		if (folio_maybe_dma_pinned(src_folio)) {
+			set_pte_at(mm, src_addr, src_pte, orig_src_pte);
+			err = -EBUSY;
+			break;
+		}
 
-	orig_src_pte = ptep_clear_flush(src_vma, src_addr, src_pte);
-	/* Folio got pinned from under us. Put it back and fail the move. */
-	if (folio_maybe_dma_pinned(src_folio)) {
-		set_pte_at(mm, src_addr, src_pte, orig_src_pte);
-		err = -EBUSY;
-		goto out;
-	}
-
-	folio_move_anon_rmap(src_folio, dst_vma);
-	src_folio->index = linear_page_index(dst_vma, dst_addr);
+		folio_move_anon_rmap(src_folio, dst_vma);
+		src_folio->index = linear_page_index(dst_vma, dst_addr);
 
-	orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
-	/* Set soft dirty bit so userspace can notice the pte was moved */
+		orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
+		/* Set soft dirty bit so userspace can notice the pte was moved */
 #ifdef CONFIG_MEM_SOFT_DIRTY
-	orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
+		orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
 #endif
-	if (pte_dirty(orig_src_pte))
-		orig_dst_pte = pte_mkdirty(orig_dst_pte);
-	orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
+		if (pte_dirty(orig_src_pte))
+			orig_dst_pte = pte_mkdirty(orig_dst_pte);
+		orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
+		set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
+
+		src_addr += PAGE_SIZE;
+		if (src_addr == addr_end)
+			break;
+		src_pte++;
+		dst_pte++;
+
+		folio_unlock(src_folio);
+		src_folio = check_ptes_for_batched_move(src_vma, src_addr, src_pte,
+							dst_pte, src_anon_vma);
+		if (!src_folio)
+			break;
+		dst_addr += PAGE_SIZE;
+	}
+
+	arch_leave_lazy_mmu_mode();
+	if (src_addr > src_start)
+		flush_tlb_range(src_vma, src_start, src_addr);
 
-	set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
 out:
 	double_pt_unlock(dst_ptl, src_ptl);
-	return err;
+	if (src_folio)
+		folio_unlock(src_folio);
+	return src_addr > src_start ? src_addr - src_start : err;
 }
 
 static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
@@ -1140,7 +1209,7 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
 	set_pte_at(mm, dst_addr, dst_pte, orig_src_pte);
 	double_pt_unlock(dst_ptl, src_ptl);
 
-	return 0;
+	return PAGE_SIZE;
 }
 
 static int move_zeropage_pte(struct mm_struct *mm,
@@ -1154,6 +1223,7 @@ static int move_zeropage_pte(struct mm_struct *mm,
 {
 	pte_t zero_pte;
 
+	flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
 	double_pt_lock(dst_ptl, src_ptl);
 	if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
 				 dst_pmd, dst_pmdval)) {
@@ -1167,20 +1237,19 @@ static int move_zeropage_pte(struct mm_struct *mm,
 	set_pte_at(mm, dst_addr, dst_pte, zero_pte);
 	double_pt_unlock(dst_ptl, src_ptl);
 
-	return 0;
+	return PAGE_SIZE;
 }
 
 
 /*
- * The mmap_lock for reading is held by the caller. Just move the page
- * from src_pmd to dst_pmd if possible, and return true if succeeded
- * in moving the page.
+ * The mmap_lock for reading is held by the caller. Just move the page(s)
+ * from src_pmd to dst_pmd if possible, and return number of bytes moved.
  */
-static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
-			  struct vm_area_struct *dst_vma,
-			  struct vm_area_struct *src_vma,
-			  unsigned long dst_addr, unsigned long src_addr,
-			  __u64 mode)
+static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
+			    struct vm_area_struct *dst_vma,
+			    struct vm_area_struct *src_vma,
+			    unsigned long dst_addr, unsigned long src_addr,
+			    unsigned long len, __u64 mode)
 {
 	swp_entry_t entry;
 	struct swap_info_struct *si = NULL;
@@ -1196,9 +1265,8 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
 	struct mmu_notifier_range range;
 	int err = 0;
 
-	flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
 	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm,
-				src_addr, src_addr + PAGE_SIZE);
+				src_addr, src_addr + len);
 	mmu_notifier_invalidate_range_start(&range);
 retry:
 	/*
@@ -1257,7 +1325,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
 		if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES))
 			err = -ENOENT;
 		else /* nothing to do to move a hole */
-			err = 0;
+			err = PAGE_SIZE;
 		goto out;
 	}
 
@@ -1375,10 +1443,14 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
 			}
 		}
 
-		err = move_present_pte(mm,  dst_vma, src_vma,
-				       dst_addr, src_addr, dst_pte, src_pte,
-				       orig_dst_pte, orig_src_pte, dst_pmd,
-				       dst_pmdval, dst_ptl, src_ptl, src_folio);
+		err = move_present_ptes(mm, dst_vma, src_vma,
+					dst_addr, src_addr, dst_pte, src_pte,
+					orig_dst_pte, orig_src_pte, dst_pmd,
+					dst_pmdval, dst_ptl, src_ptl, src_folio,
+					len, src_anon_vma);
+		/* folio is already unlocked by move_present_ptes() */
+		folio_put(src_folio);
+		src_folio = NULL;
 	} else {
 		struct folio *folio = NULL;
 
@@ -1732,7 +1804,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
 {
 	struct mm_struct *mm = ctx->mm;
 	struct vm_area_struct *src_vma, *dst_vma;
-	unsigned long src_addr, dst_addr;
+	unsigned long src_addr, dst_addr, src_end;
 	pmd_t *src_pmd, *dst_pmd;
 	long err = -EINVAL;
 	ssize_t moved = 0;
@@ -1775,8 +1847,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
 	if (err)
 		goto out_unlock;
 
-	for (src_addr = src_start, dst_addr = dst_start;
-	     src_addr < src_start + len;) {
+	for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
+	     src_addr < src_end;) {
 		spinlock_t *ptl;
 		pmd_t dst_pmdval;
 		unsigned long step_size;
@@ -1841,6 +1913,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
 						  dst_addr, src_addr);
 			step_size = HPAGE_PMD_SIZE;
 		} else {
+			long ret;
+
 			if (pmd_none(*src_pmd)) {
 				if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
 					err = -ENOENT;
@@ -1857,10 +1931,13 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
 				break;
 			}
 
-			err = move_pages_pte(mm, dst_pmd, src_pmd,
-					     dst_vma, src_vma,
-					     dst_addr, src_addr, mode);
-			step_size = PAGE_SIZE;
+			ret = move_pages_ptes(mm, dst_pmd, src_pmd,
+					      dst_vma, src_vma, dst_addr,
+					      src_addr, src_end - src_addr, mode);
+			if (ret > 0)
+				step_size = ret;
+			else
+				err = ret;
 		}
 
 		cond_resched();

base-commit: 6e64f4580381e32c06ee146ca807c555b8f73e24
-- 
2.50.1.565.gc32cd1483b-goog



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
  2025-08-07 10:39 [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE Lokesh Gidra
@ 2025-08-07 19:16 ` Peter Xu
  2025-08-07 22:54   ` Andrew Morton
  2025-08-08 16:29   ` Lokesh Gidra
  2025-08-08  6:18 ` Dan Carpenter
  1 sibling, 2 replies; 11+ messages in thread
From: Peter Xu @ 2025-08-07 19:16 UTC (permalink / raw)
  To: Lokesh Gidra
  Cc: akpm, aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
	Suren Baghdasaryan, Kalesh Singh, Barry Song, David Hildenbrand

Hi, Lokesh,

On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> for moving present pages. Mitigate this cost by opportunistically
> batching present contiguous pages for TLB flushing.
> 
> Without batching, in our testing on an arm64 Android device with UFFD GC,
> which uses MOVE ioctl for compaction, we observed that out of the total
> time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> ~20% in vm_normal_folio().
> 
> With batching, the proportion of vm_normal_folio() increases to over
> 70% of move_pages_pte() without any changes to vm_normal_folio().

Do you know why vm_normal_folio() could be expensive? I still see quite
some other things this path needs to do.

> Furthermore, time spent within move_pages_pte() is only ~20%, which
> includes TLB-flush overhead.

Indeed this should already prove the optimization, I'm just curious whether
you've run some benchmark on the GC app to show the real world benefit.

> 
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Kalesh Singh <kaleshsingh@google.com>
> Cc: Barry Song <v-songbaohua@oppo.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> ---
> Changes since v2 [1]
> - Addressed VM_WARN_ON failure, per Lorenzo Stoakes
> - Added check to ensure all batched pages share the same anon_vma
> 
> Changes since v1 [2]
> - Removed flush_tlb_batched_pending(), per Barry Song
> - Unified single and multi page case, per Barry Song
> 
> [1] https://lore.kernel.org/all/20250805121410.1658418-1-lokeshgidra@google.com/
> [2] https://lore.kernel.org/all/20250731104726.103071-1-lokeshgidra@google.com/
> 
>  mm/userfaultfd.c | 179 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 128 insertions(+), 51 deletions(-)
> 
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index cbed91b09640..78c732100aec 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1026,18 +1026,64 @@ static inline bool is_pte_pages_stable(pte_t *dst_pte, pte_t *src_pte,
>  	       pmd_same(dst_pmdval, pmdp_get_lockless(dst_pmd));
>  }
>  
> -static int move_present_pte(struct mm_struct *mm,
> -			    struct vm_area_struct *dst_vma,
> -			    struct vm_area_struct *src_vma,
> -			    unsigned long dst_addr, unsigned long src_addr,
> -			    pte_t *dst_pte, pte_t *src_pte,
> -			    pte_t orig_dst_pte, pte_t orig_src_pte,
> -			    pmd_t *dst_pmd, pmd_t dst_pmdval,
> -			    spinlock_t *dst_ptl, spinlock_t *src_ptl,
> -			    struct folio *src_folio)
> +/*
> + * Checks if the two ptes and the corresponding folio are eligible for batched
> + * move. If so, then returns pointer to the folio, after locking it. Otherwise,
> + * returns NULL.
> + */
> +static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
> +						 unsigned long src_addr,
> +						 pte_t *src_pte, pte_t *dst_pte,
> +						 struct anon_vma *src_anon_vma)
> +{
> +	pte_t orig_dst_pte, orig_src_pte;
> +	struct folio *folio;
> +
> +	orig_dst_pte = ptep_get(dst_pte);
> +	if (!pte_none(orig_dst_pte))
> +		return NULL;
> +
> +	orig_src_pte = ptep_get(src_pte);
> +	if (pte_none(orig_src_pte) || !pte_present(orig_src_pte) ||

pte_none() check could be removed - the pte_present() check should make
sure it's !none.

> +	    is_zero_pfn(pte_pfn(orig_src_pte)))
> +		return NULL;
> +
> +	folio = vm_normal_folio(src_vma, src_addr, orig_src_pte);
> +	if (!folio || !folio_trylock(folio))
> +		return NULL;

So here we don't take a refcount anymore, while the 1st folio that got
passed in will still has the refcount boosted.  IMHO it would still be
better to keep the behavior the same on the 1st and continuous folios..

Or if this is intentional, maybe worth some comment.  More below on this..

> +	if (!PageAnonExclusive(&folio->page) || folio_test_large(folio) ||
> +	    folio_anon_vma(folio) != src_anon_vma) {
> +		folio_unlock(folio);
> +		return NULL;
> +	}
> +	return folio;
> +}
> +
> +static long move_present_ptes(struct mm_struct *mm,
> +			      struct vm_area_struct *dst_vma,
> +			      struct vm_area_struct *src_vma,
> +			      unsigned long dst_addr, unsigned long src_addr,
> +			      pte_t *dst_pte, pte_t *src_pte,
> +			      pte_t orig_dst_pte, pte_t orig_src_pte,
> +			      pmd_t *dst_pmd, pmd_t dst_pmdval,
> +			      spinlock_t *dst_ptl, spinlock_t *src_ptl,
> +			      struct folio *src_folio, unsigned long len,
> +			      struct anon_vma *src_anon_vma)

(Not an immediate concern, but this function has potential to win the
 max-num-of-parameters kernel function.. :)

>  {
>  	int err = 0;
> +	unsigned long src_start = src_addr;
> +	unsigned long addr_end;
>  
> +	if (len > PAGE_SIZE) {
> +		addr_end = (dst_addr + PMD_SIZE) & PMD_MASK;
> +		if (dst_addr + len > addr_end)
> +			len = addr_end - dst_addr;

Use something like ALIGN() and MIN()?

> +
> +		addr_end = (src_addr + PMD_SIZE) & PMD_MASK;
> +		if (src_addr + len > addr_end)
> +			len = addr_end - src_addr;

Same here.

> +	}
> +	flush_cache_range(src_vma, src_addr, src_addr + len);
>  	double_pt_lock(dst_ptl, src_ptl);
>  
>  	if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> @@ -1051,31 +1097,54 @@ static int move_present_pte(struct mm_struct *mm,
>  		err = -EBUSY;
>  		goto out;
>  	}
> +	arch_enter_lazy_mmu_mode();
> +
> +	addr_end = src_start + len;
> +	while (true) {
> +		orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
> +		/* Folio got pinned from under us. Put it back and fail the move. */
> +		if (folio_maybe_dma_pinned(src_folio)) {
> +			set_pte_at(mm, src_addr, src_pte, orig_src_pte);
> +			err = -EBUSY;
> +			break;
> +		}
>  
> -	orig_src_pte = ptep_clear_flush(src_vma, src_addr, src_pte);
> -	/* Folio got pinned from under us. Put it back and fail the move. */
> -	if (folio_maybe_dma_pinned(src_folio)) {
> -		set_pte_at(mm, src_addr, src_pte, orig_src_pte);
> -		err = -EBUSY;
> -		goto out;
> -	}
> -
> -	folio_move_anon_rmap(src_folio, dst_vma);
> -	src_folio->index = linear_page_index(dst_vma, dst_addr);
> +		folio_move_anon_rmap(src_folio, dst_vma);
> +		src_folio->index = linear_page_index(dst_vma, dst_addr);
>  
> -	orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
> -	/* Set soft dirty bit so userspace can notice the pte was moved */
> +		orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
> +		/* Set soft dirty bit so userspace can notice the pte was moved */
>  #ifdef CONFIG_MEM_SOFT_DIRTY
> -	orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
> +		orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
>  #endif
> -	if (pte_dirty(orig_src_pte))
> -		orig_dst_pte = pte_mkdirty(orig_dst_pte);
> -	orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> +		if (pte_dirty(orig_src_pte))
> +			orig_dst_pte = pte_mkdirty(orig_dst_pte);
> +		orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> +		set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
> +
> +		src_addr += PAGE_SIZE;
> +		if (src_addr == addr_end)
> +			break;
> +		src_pte++;
> +		dst_pte++;
> +
> +		folio_unlock(src_folio);
> +		src_folio = check_ptes_for_batched_move(src_vma, src_addr, src_pte,
> +							dst_pte, src_anon_vma);
> +		if (!src_folio)
> +			break;
> +		dst_addr += PAGE_SIZE;
> +	}
> +
> +	arch_leave_lazy_mmu_mode();
> +	if (src_addr > src_start)
> +		flush_tlb_range(src_vma, src_start, src_addr);
>  
> -	set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
>  out:
>  	double_pt_unlock(dst_ptl, src_ptl);
> -	return err;
> +	if (src_folio)
> +		folio_unlock(src_folio);
> +	return src_addr > src_start ? src_addr - src_start : err;
>  }
>  
>  static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> @@ -1140,7 +1209,7 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
>  	set_pte_at(mm, dst_addr, dst_pte, orig_src_pte);
>  	double_pt_unlock(dst_ptl, src_ptl);
>  
> -	return 0;
> +	return PAGE_SIZE;
>  }
>  
>  static int move_zeropage_pte(struct mm_struct *mm,
> @@ -1154,6 +1223,7 @@ static int move_zeropage_pte(struct mm_struct *mm,
>  {
>  	pte_t zero_pte;
>  
> +	flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);

If it's a zero page hence not writtable, do we still need to flush cache at
all?  Looks harmless, but looks like not needed either.

>  	double_pt_lock(dst_ptl, src_ptl);
>  	if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
>  				 dst_pmd, dst_pmdval)) {
> @@ -1167,20 +1237,19 @@ static int move_zeropage_pte(struct mm_struct *mm,
>  	set_pte_at(mm, dst_addr, dst_pte, zero_pte);
>  	double_pt_unlock(dst_ptl, src_ptl);
>  
> -	return 0;
> +	return PAGE_SIZE;
>  }
>  
>  
>  /*
> - * The mmap_lock for reading is held by the caller. Just move the page
> - * from src_pmd to dst_pmd if possible, and return true if succeeded
> - * in moving the page.
> + * The mmap_lock for reading is held by the caller. Just move the page(s)
> + * from src_pmd to dst_pmd if possible, and return number of bytes moved.
>   */
> -static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> -			  struct vm_area_struct *dst_vma,
> -			  struct vm_area_struct *src_vma,
> -			  unsigned long dst_addr, unsigned long src_addr,
> -			  __u64 mode)
> +static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> +			    struct vm_area_struct *dst_vma,
> +			    struct vm_area_struct *src_vma,
> +			    unsigned long dst_addr, unsigned long src_addr,
> +			    unsigned long len, __u64 mode)
>  {
>  	swp_entry_t entry;
>  	struct swap_info_struct *si = NULL;
> @@ -1196,9 +1265,8 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
>  	struct mmu_notifier_range range;
>  	int err = 0;
>  
> -	flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
>  	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm,
> -				src_addr, src_addr + PAGE_SIZE);
> +				src_addr, src_addr + len);
>  	mmu_notifier_invalidate_range_start(&range);
>  retry:
>  	/*
> @@ -1257,7 +1325,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
>  		if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES))
>  			err = -ENOENT;
>  		else /* nothing to do to move a hole */
> -			err = 0;
> +			err = PAGE_SIZE;
>  		goto out;
>  	}
>  
> @@ -1375,10 +1443,14 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
>  			}
>  		}
>  
> -		err = move_present_pte(mm,  dst_vma, src_vma,
> -				       dst_addr, src_addr, dst_pte, src_pte,
> -				       orig_dst_pte, orig_src_pte, dst_pmd,
> -				       dst_pmdval, dst_ptl, src_ptl, src_folio);
> +		err = move_present_ptes(mm, dst_vma, src_vma,
> +					dst_addr, src_addr, dst_pte, src_pte,
> +					orig_dst_pte, orig_src_pte, dst_pmd,
> +					dst_pmdval, dst_ptl, src_ptl, src_folio,
> +					len, src_anon_vma);
> +		/* folio is already unlocked by move_present_ptes() */
> +		folio_put(src_folio);
> +		src_folio = NULL;

So the function above now can move multiple folios but keep holding the
1st's refcount..  This still smells error prone, sooner or later.

Would it be slightly better if we take a folio pointer in
move_present_ptes(), and releae everything there (including reset the
pointer)?

Thanks,

>  	} else {
>  		struct folio *folio = NULL;
>  
> @@ -1732,7 +1804,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
>  {
>  	struct mm_struct *mm = ctx->mm;
>  	struct vm_area_struct *src_vma, *dst_vma;
> -	unsigned long src_addr, dst_addr;
> +	unsigned long src_addr, dst_addr, src_end;
>  	pmd_t *src_pmd, *dst_pmd;
>  	long err = -EINVAL;
>  	ssize_t moved = 0;
> @@ -1775,8 +1847,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
>  	if (err)
>  		goto out_unlock;
>  
> -	for (src_addr = src_start, dst_addr = dst_start;
> -	     src_addr < src_start + len;) {
> +	for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
> +	     src_addr < src_end;) {
>  		spinlock_t *ptl;
>  		pmd_t dst_pmdval;
>  		unsigned long step_size;
> @@ -1841,6 +1913,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
>  						  dst_addr, src_addr);
>  			step_size = HPAGE_PMD_SIZE;
>  		} else {
> +			long ret;
> +
>  			if (pmd_none(*src_pmd)) {
>  				if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
>  					err = -ENOENT;
> @@ -1857,10 +1931,13 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
>  				break;
>  			}
>  
> -			err = move_pages_pte(mm, dst_pmd, src_pmd,
> -					     dst_vma, src_vma,
> -					     dst_addr, src_addr, mode);
> -			step_size = PAGE_SIZE;
> +			ret = move_pages_ptes(mm, dst_pmd, src_pmd,
> +					      dst_vma, src_vma, dst_addr,
> +					      src_addr, src_end - src_addr, mode);
> +			if (ret > 0)
> +				step_size = ret;
> +			else
> +				err = ret;
>  		}
>  
>  		cond_resched();
> 
> base-commit: 6e64f4580381e32c06ee146ca807c555b8f73e24
> -- 
> 2.50.1.565.gc32cd1483b-goog
> 

-- 
Peter Xu



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
@ 2025-08-07 22:24 kernel test robot
  0 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2025-08-07 22:24 UTC (permalink / raw)
  To: oe-kbuild; +Cc: lkp, Dan Carpenter

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20250807103902.2242717-1-lokeshgidra@google.com>
References: <20250807103902.2242717-1-lokeshgidra@google.com>
TO: Lokesh Gidra <lokeshgidra@google.com>

Hi Lokesh,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 6e64f4580381e32c06ee146ca807c555b8f73e24]

url:    https://github.com/intel-lab-lkp/linux/commits/Lokesh-Gidra/userfaultfd-opportunistic-TLB-flush-batching-for-present-pages-in-MOVE/20250807-184025
base:   6e64f4580381e32c06ee146ca807c555b8f73e24
patch link:    https://lore.kernel.org/r/20250807103902.2242717-1-lokeshgidra%40google.com
patch subject: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
:::::: branch date: 12 hours ago
:::::: commit date: 12 hours ago
config: openrisc-randconfig-r071-20250808 (https://download.01.org/0day-ci/archive/20250808/202508080639.Cf7SyRwQ-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 10.5.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202508080639.Cf7SyRwQ-lkp@intel.com/

smatch warnings:
mm/userfaultfd.c:1959 move_pages() error: uninitialized symbol 'step_size'.

vim +/step_size +1959 mm/userfaultfd.c

adef440691bab82 Andrea Arcangeli        2023-12-06  1725  
adef440691bab82 Andrea Arcangeli        2023-12-06  1726  /**
adef440691bab82 Andrea Arcangeli        2023-12-06  1727   * move_pages - move arbitrary anonymous pages of an existing vma
adef440691bab82 Andrea Arcangeli        2023-12-06  1728   * @ctx: pointer to the userfaultfd context
adef440691bab82 Andrea Arcangeli        2023-12-06  1729   * @dst_start: start of the destination virtual memory range
adef440691bab82 Andrea Arcangeli        2023-12-06  1730   * @src_start: start of the source virtual memory range
adef440691bab82 Andrea Arcangeli        2023-12-06  1731   * @len: length of the virtual memory range
adef440691bab82 Andrea Arcangeli        2023-12-06  1732   * @mode: flags from uffdio_move.mode
adef440691bab82 Andrea Arcangeli        2023-12-06  1733   *
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1734   * It will either use the mmap_lock in read mode or per-vma locks
adef440691bab82 Andrea Arcangeli        2023-12-06  1735   *
adef440691bab82 Andrea Arcangeli        2023-12-06  1736   * move_pages() remaps arbitrary anonymous pages atomically in zero
adef440691bab82 Andrea Arcangeli        2023-12-06  1737   * copy. It only works on non shared anonymous pages because those can
adef440691bab82 Andrea Arcangeli        2023-12-06  1738   * be relocated without generating non linear anon_vmas in the rmap
adef440691bab82 Andrea Arcangeli        2023-12-06  1739   * code.
adef440691bab82 Andrea Arcangeli        2023-12-06  1740   *
adef440691bab82 Andrea Arcangeli        2023-12-06  1741   * It provides a zero copy mechanism to handle userspace page faults.
adef440691bab82 Andrea Arcangeli        2023-12-06  1742   * The source vma pages should have mapcount == 1, which can be
adef440691bab82 Andrea Arcangeli        2023-12-06  1743   * enforced by using madvise(MADV_DONTFORK) on src vma.
adef440691bab82 Andrea Arcangeli        2023-12-06  1744   *
adef440691bab82 Andrea Arcangeli        2023-12-06  1745   * The thread receiving the page during the userland page fault
adef440691bab82 Andrea Arcangeli        2023-12-06  1746   * will receive the faulting page in the source vma through the network,
adef440691bab82 Andrea Arcangeli        2023-12-06  1747   * storage or any other I/O device (MADV_DONTFORK in the source vma
adef440691bab82 Andrea Arcangeli        2023-12-06  1748   * avoids move_pages() to fail with -EBUSY if the process forks before
adef440691bab82 Andrea Arcangeli        2023-12-06  1749   * move_pages() is called), then it will call move_pages() to map the
adef440691bab82 Andrea Arcangeli        2023-12-06  1750   * page in the faulting address in the destination vma.
adef440691bab82 Andrea Arcangeli        2023-12-06  1751   *
adef440691bab82 Andrea Arcangeli        2023-12-06  1752   * This userfaultfd command works purely via pagetables, so it's the
adef440691bab82 Andrea Arcangeli        2023-12-06  1753   * most efficient way to move physical non shared anonymous pages
adef440691bab82 Andrea Arcangeli        2023-12-06  1754   * across different virtual addresses. Unlike mremap()/mmap()/munmap()
adef440691bab82 Andrea Arcangeli        2023-12-06  1755   * it does not create any new vmas. The mapping in the destination
adef440691bab82 Andrea Arcangeli        2023-12-06  1756   * address is atomic.
adef440691bab82 Andrea Arcangeli        2023-12-06  1757   *
adef440691bab82 Andrea Arcangeli        2023-12-06  1758   * It only works if the vma protection bits are identical from the
adef440691bab82 Andrea Arcangeli        2023-12-06  1759   * source and destination vma.
adef440691bab82 Andrea Arcangeli        2023-12-06  1760   *
adef440691bab82 Andrea Arcangeli        2023-12-06  1761   * It can remap non shared anonymous pages within the same vma too.
adef440691bab82 Andrea Arcangeli        2023-12-06  1762   *
adef440691bab82 Andrea Arcangeli        2023-12-06  1763   * If the source virtual memory range has any unmapped holes, or if
adef440691bab82 Andrea Arcangeli        2023-12-06  1764   * the destination virtual memory range is not a whole unmapped hole,
adef440691bab82 Andrea Arcangeli        2023-12-06  1765   * move_pages() will fail respectively with -ENOENT or -EEXIST. This
adef440691bab82 Andrea Arcangeli        2023-12-06  1766   * provides a very strict behavior to avoid any chance of memory
adef440691bab82 Andrea Arcangeli        2023-12-06  1767   * corruption going unnoticed if there are userland race conditions.
adef440691bab82 Andrea Arcangeli        2023-12-06  1768   * Only one thread should resolve the userland page fault at any given
adef440691bab82 Andrea Arcangeli        2023-12-06  1769   * time for any given faulting address. This means that if two threads
adef440691bab82 Andrea Arcangeli        2023-12-06  1770   * try to both call move_pages() on the same destination address at the
adef440691bab82 Andrea Arcangeli        2023-12-06  1771   * same time, the second thread will get an explicit error from this
adef440691bab82 Andrea Arcangeli        2023-12-06  1772   * command.
adef440691bab82 Andrea Arcangeli        2023-12-06  1773   *
adef440691bab82 Andrea Arcangeli        2023-12-06  1774   * The command retval will return "len" is successful. The command
adef440691bab82 Andrea Arcangeli        2023-12-06  1775   * however can be interrupted by fatal signals or errors. If
adef440691bab82 Andrea Arcangeli        2023-12-06  1776   * interrupted it will return the number of bytes successfully
adef440691bab82 Andrea Arcangeli        2023-12-06  1777   * remapped before the interruption if any, or the negative error if
adef440691bab82 Andrea Arcangeli        2023-12-06  1778   * none. It will never return zero. Either it will return an error or
adef440691bab82 Andrea Arcangeli        2023-12-06  1779   * an amount of bytes successfully moved. If the retval reports a
adef440691bab82 Andrea Arcangeli        2023-12-06  1780   * "short" remap, the move_pages() command should be repeated by
adef440691bab82 Andrea Arcangeli        2023-12-06  1781   * userland with src+retval, dst+reval, len-retval if it wants to know
adef440691bab82 Andrea Arcangeli        2023-12-06  1782   * about the error that interrupted it.
adef440691bab82 Andrea Arcangeli        2023-12-06  1783   *
adef440691bab82 Andrea Arcangeli        2023-12-06  1784   * The UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES flag can be specified to
adef440691bab82 Andrea Arcangeli        2023-12-06  1785   * prevent -ENOENT errors to materialize if there are holes in the
adef440691bab82 Andrea Arcangeli        2023-12-06  1786   * source virtual range that is being remapped. The holes will be
adef440691bab82 Andrea Arcangeli        2023-12-06  1787   * accounted as successfully remapped in the retval of the
adef440691bab82 Andrea Arcangeli        2023-12-06  1788   * command. This is mostly useful to remap hugepage naturally aligned
adef440691bab82 Andrea Arcangeli        2023-12-06  1789   * virtual regions without knowing if there are transparent hugepage
adef440691bab82 Andrea Arcangeli        2023-12-06  1790   * in the regions or not, but preventing the risk of having to split
adef440691bab82 Andrea Arcangeli        2023-12-06  1791   * the hugepmd during the remap.
adef440691bab82 Andrea Arcangeli        2023-12-06  1792   *
adef440691bab82 Andrea Arcangeli        2023-12-06  1793   * If there's any rmap walk that is taking the anon_vma locks without
adef440691bab82 Andrea Arcangeli        2023-12-06  1794   * first obtaining the folio lock (the only current instance is
adef440691bab82 Andrea Arcangeli        2023-12-06  1795   * folio_referenced), they will have to verify if the folio->mapping
adef440691bab82 Andrea Arcangeli        2023-12-06  1796   * has changed after taking the anon_vma lock. If it changed they
adef440691bab82 Andrea Arcangeli        2023-12-06  1797   * should release the lock and retry obtaining a new anon_vma, because
adef440691bab82 Andrea Arcangeli        2023-12-06  1798   * it means the anon_vma was changed by move_pages() before the lock
adef440691bab82 Andrea Arcangeli        2023-12-06  1799   * could be obtained. This is the only additional complexity added to
adef440691bab82 Andrea Arcangeli        2023-12-06  1800   * the rmap code to provide this anonymous page remapping functionality.
adef440691bab82 Andrea Arcangeli        2023-12-06  1801   */
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1802  ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1803  		   unsigned long src_start, unsigned long len, __u64 mode)
adef440691bab82 Andrea Arcangeli        2023-12-06  1804  {
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1805  	struct mm_struct *mm = ctx->mm;
adef440691bab82 Andrea Arcangeli        2023-12-06  1806  	struct vm_area_struct *src_vma, *dst_vma;
3abf06e0c484867 Lokesh Gidra            2025-08-07  1807  	unsigned long src_addr, dst_addr, src_end;
adef440691bab82 Andrea Arcangeli        2023-12-06  1808  	pmd_t *src_pmd, *dst_pmd;
adef440691bab82 Andrea Arcangeli        2023-12-06  1809  	long err = -EINVAL;
adef440691bab82 Andrea Arcangeli        2023-12-06  1810  	ssize_t moved = 0;
adef440691bab82 Andrea Arcangeli        2023-12-06  1811  
adef440691bab82 Andrea Arcangeli        2023-12-06  1812  	/* Sanitize the command parameters. */
31defc3b01d907e Tal Zussman             2025-06-19  1813  	VM_WARN_ON_ONCE(src_start & ~PAGE_MASK);
31defc3b01d907e Tal Zussman             2025-06-19  1814  	VM_WARN_ON_ONCE(dst_start & ~PAGE_MASK);
31defc3b01d907e Tal Zussman             2025-06-19  1815  	VM_WARN_ON_ONCE(len & ~PAGE_MASK);
adef440691bab82 Andrea Arcangeli        2023-12-06  1816  
adef440691bab82 Andrea Arcangeli        2023-12-06  1817  	/* Does the address range wrap, or is the span zero-sized? */
31defc3b01d907e Tal Zussman             2025-06-19  1818  	VM_WARN_ON_ONCE(src_start + len < src_start);
31defc3b01d907e Tal Zussman             2025-06-19  1819  	VM_WARN_ON_ONCE(dst_start + len < dst_start);
adef440691bab82 Andrea Arcangeli        2023-12-06  1820  
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1821  	err = uffd_move_lock(mm, dst_start, src_start, &dst_vma, &src_vma);
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1822  	if (err)
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1823  		goto out;
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1824  
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1825  	/* Re-check after taking map_changing_lock */
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1826  	err = -EAGAIN;
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1827  	down_read(&ctx->map_changing_lock);
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1828  	if (likely(atomic_read(&ctx->mmap_changing)))
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1829  		goto out_unlock;
adef440691bab82 Andrea Arcangeli        2023-12-06  1830  	/*
adef440691bab82 Andrea Arcangeli        2023-12-06  1831  	 * Make sure the vma is not shared, that the src and dst remap
adef440691bab82 Andrea Arcangeli        2023-12-06  1832  	 * ranges are both valid and fully within a single existing
adef440691bab82 Andrea Arcangeli        2023-12-06  1833  	 * vma.
adef440691bab82 Andrea Arcangeli        2023-12-06  1834  	 */
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1835  	err = -EINVAL;
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1836  	if (src_vma->vm_flags & VM_SHARED)
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1837  		goto out_unlock;
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1838  	if (src_start + len > src_vma->vm_end)
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1839  		goto out_unlock;
adef440691bab82 Andrea Arcangeli        2023-12-06  1840  
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1841  	if (dst_vma->vm_flags & VM_SHARED)
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1842  		goto out_unlock;
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1843  	if (dst_start + len > dst_vma->vm_end)
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1844  		goto out_unlock;
adef440691bab82 Andrea Arcangeli        2023-12-06  1845  
adef440691bab82 Andrea Arcangeli        2023-12-06  1846  	err = validate_move_areas(ctx, src_vma, dst_vma);
adef440691bab82 Andrea Arcangeli        2023-12-06  1847  	if (err)
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1848  		goto out_unlock;
adef440691bab82 Andrea Arcangeli        2023-12-06  1849  
3abf06e0c484867 Lokesh Gidra            2025-08-07  1850  	for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
3abf06e0c484867 Lokesh Gidra            2025-08-07  1851  	     src_addr < src_end;) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1852  		spinlock_t *ptl;
adef440691bab82 Andrea Arcangeli        2023-12-06  1853  		pmd_t dst_pmdval;
adef440691bab82 Andrea Arcangeli        2023-12-06  1854  		unsigned long step_size;
adef440691bab82 Andrea Arcangeli        2023-12-06  1855  
adef440691bab82 Andrea Arcangeli        2023-12-06  1856  		/*
adef440691bab82 Andrea Arcangeli        2023-12-06  1857  		 * Below works because anonymous area would not have a
adef440691bab82 Andrea Arcangeli        2023-12-06  1858  		 * transparent huge PUD. If file-backed support is added,
adef440691bab82 Andrea Arcangeli        2023-12-06  1859  		 * that case would need to be handled here.
adef440691bab82 Andrea Arcangeli        2023-12-06  1860  		 */
adef440691bab82 Andrea Arcangeli        2023-12-06  1861  		src_pmd = mm_find_pmd(mm, src_addr);
adef440691bab82 Andrea Arcangeli        2023-12-06  1862  		if (unlikely(!src_pmd)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1863  			if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1864  				err = -ENOENT;
adef440691bab82 Andrea Arcangeli        2023-12-06  1865  				break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1866  			}
adef440691bab82 Andrea Arcangeli        2023-12-06  1867  			src_pmd = mm_alloc_pmd(mm, src_addr);
adef440691bab82 Andrea Arcangeli        2023-12-06  1868  			if (unlikely(!src_pmd)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1869  				err = -ENOMEM;
adef440691bab82 Andrea Arcangeli        2023-12-06  1870  				break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1871  			}
adef440691bab82 Andrea Arcangeli        2023-12-06  1872  		}
adef440691bab82 Andrea Arcangeli        2023-12-06  1873  		dst_pmd = mm_alloc_pmd(mm, dst_addr);
adef440691bab82 Andrea Arcangeli        2023-12-06  1874  		if (unlikely(!dst_pmd)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1875  			err = -ENOMEM;
adef440691bab82 Andrea Arcangeli        2023-12-06  1876  			break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1877  		}
adef440691bab82 Andrea Arcangeli        2023-12-06  1878  
adef440691bab82 Andrea Arcangeli        2023-12-06  1879  		dst_pmdval = pmdp_get_lockless(dst_pmd);
adef440691bab82 Andrea Arcangeli        2023-12-06  1880  		/*
adef440691bab82 Andrea Arcangeli        2023-12-06  1881  		 * If the dst_pmd is mapped as THP don't override it and just
adef440691bab82 Andrea Arcangeli        2023-12-06  1882  		 * be strict. If dst_pmd changes into TPH after this check, the
adef440691bab82 Andrea Arcangeli        2023-12-06  1883  		 * move_pages_huge_pmd() will detect the change and retry
adef440691bab82 Andrea Arcangeli        2023-12-06  1884  		 * while move_pages_pte() will detect the change and fail.
adef440691bab82 Andrea Arcangeli        2023-12-06  1885  		 */
adef440691bab82 Andrea Arcangeli        2023-12-06  1886  		if (unlikely(pmd_trans_huge(dst_pmdval))) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1887  			err = -EEXIST;
adef440691bab82 Andrea Arcangeli        2023-12-06  1888  			break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1889  		}
adef440691bab82 Andrea Arcangeli        2023-12-06  1890  
adef440691bab82 Andrea Arcangeli        2023-12-06  1891  		ptl = pmd_trans_huge_lock(src_pmd, src_vma);
adef440691bab82 Andrea Arcangeli        2023-12-06  1892  		if (ptl) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1893  			/* Check if we can move the pmd without splitting it. */
adef440691bab82 Andrea Arcangeli        2023-12-06  1894  			if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
adef440691bab82 Andrea Arcangeli        2023-12-06  1895  			    !pmd_none(dst_pmdval)) {
e06d03d5590ae1c Matthew Wilcox (Oracle  2024-03-26  1896) 				struct folio *folio = pmd_folio(*src_pmd);
adef440691bab82 Andrea Arcangeli        2023-12-06  1897  
5beaee54a324ba1 Matthew Wilcox (Oracle  2024-03-26  1898) 				if (!folio || (!is_huge_zero_folio(folio) &&
eb1521dad8f391d Suren Baghdasaryan      2024-01-31  1899  					       !PageAnonExclusive(&folio->page))) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1900  					spin_unlock(ptl);
adef440691bab82 Andrea Arcangeli        2023-12-06  1901  					err = -EBUSY;
adef440691bab82 Andrea Arcangeli        2023-12-06  1902  					break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1903  				}
adef440691bab82 Andrea Arcangeli        2023-12-06  1904  
adef440691bab82 Andrea Arcangeli        2023-12-06  1905  				spin_unlock(ptl);
adef440691bab82 Andrea Arcangeli        2023-12-06  1906  				split_huge_pmd(src_vma, src_pmd, src_addr);
adef440691bab82 Andrea Arcangeli        2023-12-06  1907  				/* The folio will be split by move_pages_pte() */
adef440691bab82 Andrea Arcangeli        2023-12-06  1908  				continue;
adef440691bab82 Andrea Arcangeli        2023-12-06  1909  			}
adef440691bab82 Andrea Arcangeli        2023-12-06  1910  
adef440691bab82 Andrea Arcangeli        2023-12-06  1911  			err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
adef440691bab82 Andrea Arcangeli        2023-12-06  1912  						  dst_pmdval, dst_vma, src_vma,
adef440691bab82 Andrea Arcangeli        2023-12-06  1913  						  dst_addr, src_addr);
adef440691bab82 Andrea Arcangeli        2023-12-06  1914  			step_size = HPAGE_PMD_SIZE;
adef440691bab82 Andrea Arcangeli        2023-12-06  1915  		} else {
3abf06e0c484867 Lokesh Gidra            2025-08-07  1916  			long ret;
3abf06e0c484867 Lokesh Gidra            2025-08-07  1917  
adef440691bab82 Andrea Arcangeli        2023-12-06  1918  			if (pmd_none(*src_pmd)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1919  				if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1920  					err = -ENOENT;
adef440691bab82 Andrea Arcangeli        2023-12-06  1921  					break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1922  				}
adef440691bab82 Andrea Arcangeli        2023-12-06  1923  				if (unlikely(__pte_alloc(mm, src_pmd))) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1924  					err = -ENOMEM;
adef440691bab82 Andrea Arcangeli        2023-12-06  1925  					break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1926  				}
adef440691bab82 Andrea Arcangeli        2023-12-06  1927  			}
adef440691bab82 Andrea Arcangeli        2023-12-06  1928  
adef440691bab82 Andrea Arcangeli        2023-12-06  1929  			if (unlikely(pte_alloc(mm, dst_pmd))) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1930  				err = -ENOMEM;
adef440691bab82 Andrea Arcangeli        2023-12-06  1931  				break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1932  			}
adef440691bab82 Andrea Arcangeli        2023-12-06  1933  
3abf06e0c484867 Lokesh Gidra            2025-08-07  1934  			ret = move_pages_ptes(mm, dst_pmd, src_pmd,
3abf06e0c484867 Lokesh Gidra            2025-08-07  1935  					      dst_vma, src_vma, dst_addr,
3abf06e0c484867 Lokesh Gidra            2025-08-07  1936  					      src_addr, src_end - src_addr, mode);
3abf06e0c484867 Lokesh Gidra            2025-08-07  1937  			if (ret > 0)
3abf06e0c484867 Lokesh Gidra            2025-08-07  1938  				step_size = ret;
3abf06e0c484867 Lokesh Gidra            2025-08-07  1939  			else
3abf06e0c484867 Lokesh Gidra            2025-08-07  1940  				err = ret;
adef440691bab82 Andrea Arcangeli        2023-12-06  1941  		}
adef440691bab82 Andrea Arcangeli        2023-12-06  1942  
adef440691bab82 Andrea Arcangeli        2023-12-06  1943  		cond_resched();
adef440691bab82 Andrea Arcangeli        2023-12-06  1944  
adef440691bab82 Andrea Arcangeli        2023-12-06  1945  		if (fatal_signal_pending(current)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1946  			/* Do not override an error */
adef440691bab82 Andrea Arcangeli        2023-12-06  1947  			if (!err || err == -EAGAIN)
adef440691bab82 Andrea Arcangeli        2023-12-06  1948  				err = -EINTR;
adef440691bab82 Andrea Arcangeli        2023-12-06  1949  			break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1950  		}
adef440691bab82 Andrea Arcangeli        2023-12-06  1951  
adef440691bab82 Andrea Arcangeli        2023-12-06  1952  		if (err) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1953  			if (err == -EAGAIN)
adef440691bab82 Andrea Arcangeli        2023-12-06  1954  				continue;
adef440691bab82 Andrea Arcangeli        2023-12-06  1955  			break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1956  		}
adef440691bab82 Andrea Arcangeli        2023-12-06  1957  
adef440691bab82 Andrea Arcangeli        2023-12-06  1958  		/* Proceed to the next page */
adef440691bab82 Andrea Arcangeli        2023-12-06 @1959  		dst_addr += step_size;
adef440691bab82 Andrea Arcangeli        2023-12-06  1960  		src_addr += step_size;
adef440691bab82 Andrea Arcangeli        2023-12-06  1961  		moved += step_size;
adef440691bab82 Andrea Arcangeli        2023-12-06  1962  	}
adef440691bab82 Andrea Arcangeli        2023-12-06  1963  
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1964  out_unlock:
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1965  	up_read(&ctx->map_changing_lock);
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1966  	uffd_move_unlock(dst_vma, src_vma);
adef440691bab82 Andrea Arcangeli        2023-12-06  1967  out:
31defc3b01d907e Tal Zussman             2025-06-19  1968  	VM_WARN_ON_ONCE(moved < 0);
31defc3b01d907e Tal Zussman             2025-06-19  1969  	VM_WARN_ON_ONCE(err > 0);
31defc3b01d907e Tal Zussman             2025-06-19  1970  	VM_WARN_ON_ONCE(!moved && !err);
adef440691bab82 Andrea Arcangeli        2023-12-06  1971  	return moved ? moved : err;
adef440691bab82 Andrea Arcangeli        2023-12-06  1972  }
a17c7d8fd2b097a Lorenzo Stoakes         2024-07-29  1973  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
  2025-08-07 19:16 ` Peter Xu
@ 2025-08-07 22:54   ` Andrew Morton
  2025-08-08 16:41     ` Lokesh Gidra
  2025-08-08 16:29   ` Lokesh Gidra
  1 sibling, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2025-08-07 22:54 UTC (permalink / raw)
  To: Peter Xu
  Cc: Lokesh Gidra, aarcange, linux-mm, linux-kernel, 21cnbao,
	ngeoffray, Suren Baghdasaryan, Kalesh Singh, Barry Song,
	David Hildenbrand

On Thu, 7 Aug 2025 15:16:57 -0400 Peter Xu <peterx@redhat.com> wrote:

> Hi, Lokesh,
> 
> On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> > MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> > for moving present pages. Mitigate this cost by opportunistically
> > batching present contiguous pages for TLB flushing.
> > 
> > Without batching, in our testing on an arm64 Android device with UFFD GC,
> > which uses MOVE ioctl for compaction, we observed that out of the total
> > time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> > ~20% in vm_normal_folio().
> > 
> > With batching, the proportion of vm_normal_folio() increases to over
> > 70% of move_pages_pte() without any changes to vm_normal_folio().
> 
> Do you know why vm_normal_folio() could be expensive? I still see quite
> some other things this path needs to do.

Maybe as explained here?
https://lkml.kernel.org/r/20250807185819.199865-1-lorenzo.stoakes@oracle.com




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
  2025-08-07 10:39 [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE Lokesh Gidra
  2025-08-07 19:16 ` Peter Xu
@ 2025-08-08  6:18 ` Dan Carpenter
  2025-08-08 15:27   ` Lokesh Gidra
  1 sibling, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2025-08-08  6:18 UTC (permalink / raw)
  To: oe-kbuild, Lokesh Gidra; +Cc: lkp, oe-kbuild-all

Hi Lokesh,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Lokesh-Gidra/userfaultfd-opportunistic-TLB-flush-batching-for-present-pages-in-MOVE/20250807-184025
base:   6e64f4580381e32c06ee146ca807c555b8f73e24
patch link:    https://lore.kernel.org/r/20250807103902.2242717-1-lokeshgidra%40google.com
patch subject: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
config: openrisc-randconfig-r071-20250808 (https://download.01.org/0day-ci/archive/20250808/202508080639.Cf7SyRwQ-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 10.5.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202508080639.Cf7SyRwQ-lkp@intel.com/

smatch warnings:
mm/userfaultfd.c:1959 move_pages() error: uninitialized symbol 'step_size'.

vim +/step_size +1959 mm/userfaultfd.c

867a43a34ff8a38 Lokesh Gidra            2024-02-15  1802  ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1803  		   unsigned long src_start, unsigned long len, __u64 mode)
adef440691bab82 Andrea Arcangeli        2023-12-06  1804  {
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1805  	struct mm_struct *mm = ctx->mm;
adef440691bab82 Andrea Arcangeli        2023-12-06  1806  	struct vm_area_struct *src_vma, *dst_vma;
3abf06e0c484867 Lokesh Gidra            2025-08-07  1807  	unsigned long src_addr, dst_addr, src_end;
adef440691bab82 Andrea Arcangeli        2023-12-06  1808  	pmd_t *src_pmd, *dst_pmd;
adef440691bab82 Andrea Arcangeli        2023-12-06  1809  	long err = -EINVAL;
adef440691bab82 Andrea Arcangeli        2023-12-06  1810  	ssize_t moved = 0;
adef440691bab82 Andrea Arcangeli        2023-12-06  1811  
adef440691bab82 Andrea Arcangeli        2023-12-06  1812  	/* Sanitize the command parameters. */
31defc3b01d907e Tal Zussman             2025-06-19  1813  	VM_WARN_ON_ONCE(src_start & ~PAGE_MASK);
31defc3b01d907e Tal Zussman             2025-06-19  1814  	VM_WARN_ON_ONCE(dst_start & ~PAGE_MASK);
31defc3b01d907e Tal Zussman             2025-06-19  1815  	VM_WARN_ON_ONCE(len & ~PAGE_MASK);
adef440691bab82 Andrea Arcangeli        2023-12-06  1816  
adef440691bab82 Andrea Arcangeli        2023-12-06  1817  	/* Does the address range wrap, or is the span zero-sized? */
31defc3b01d907e Tal Zussman             2025-06-19  1818  	VM_WARN_ON_ONCE(src_start + len < src_start);
31defc3b01d907e Tal Zussman             2025-06-19  1819  	VM_WARN_ON_ONCE(dst_start + len < dst_start);
adef440691bab82 Andrea Arcangeli        2023-12-06  1820  
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1821  	err = uffd_move_lock(mm, dst_start, src_start, &dst_vma, &src_vma);
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1822  	if (err)
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1823  		goto out;
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1824  
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1825  	/* Re-check after taking map_changing_lock */
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1826  	err = -EAGAIN;
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1827  	down_read(&ctx->map_changing_lock);
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1828  	if (likely(atomic_read(&ctx->mmap_changing)))
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1829  		goto out_unlock;
adef440691bab82 Andrea Arcangeli        2023-12-06  1830  	/*
adef440691bab82 Andrea Arcangeli        2023-12-06  1831  	 * Make sure the vma is not shared, that the src and dst remap
adef440691bab82 Andrea Arcangeli        2023-12-06  1832  	 * ranges are both valid and fully within a single existing
adef440691bab82 Andrea Arcangeli        2023-12-06  1833  	 * vma.
adef440691bab82 Andrea Arcangeli        2023-12-06  1834  	 */
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1835  	err = -EINVAL;
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1836  	if (src_vma->vm_flags & VM_SHARED)
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1837  		goto out_unlock;
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1838  	if (src_start + len > src_vma->vm_end)
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1839  		goto out_unlock;
adef440691bab82 Andrea Arcangeli        2023-12-06  1840  
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1841  	if (dst_vma->vm_flags & VM_SHARED)
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1842  		goto out_unlock;
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1843  	if (dst_start + len > dst_vma->vm_end)
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1844  		goto out_unlock;
adef440691bab82 Andrea Arcangeli        2023-12-06  1845  
adef440691bab82 Andrea Arcangeli        2023-12-06  1846  	err = validate_move_areas(ctx, src_vma, dst_vma);
adef440691bab82 Andrea Arcangeli        2023-12-06  1847  	if (err)
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1848  		goto out_unlock;
adef440691bab82 Andrea Arcangeli        2023-12-06  1849  
3abf06e0c484867 Lokesh Gidra            2025-08-07  1850  	for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
3abf06e0c484867 Lokesh Gidra            2025-08-07  1851  	     src_addr < src_end;) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1852  		spinlock_t *ptl;
adef440691bab82 Andrea Arcangeli        2023-12-06  1853  		pmd_t dst_pmdval;
adef440691bab82 Andrea Arcangeli        2023-12-06  1854  		unsigned long step_size;
adef440691bab82 Andrea Arcangeli        2023-12-06  1855  
adef440691bab82 Andrea Arcangeli        2023-12-06  1856  		/*
adef440691bab82 Andrea Arcangeli        2023-12-06  1857  		 * Below works because anonymous area would not have a
adef440691bab82 Andrea Arcangeli        2023-12-06  1858  		 * transparent huge PUD. If file-backed support is added,
adef440691bab82 Andrea Arcangeli        2023-12-06  1859  		 * that case would need to be handled here.
adef440691bab82 Andrea Arcangeli        2023-12-06  1860  		 */
adef440691bab82 Andrea Arcangeli        2023-12-06  1861  		src_pmd = mm_find_pmd(mm, src_addr);
adef440691bab82 Andrea Arcangeli        2023-12-06  1862  		if (unlikely(!src_pmd)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1863  			if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1864  				err = -ENOENT;
adef440691bab82 Andrea Arcangeli        2023-12-06  1865  				break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1866  			}
adef440691bab82 Andrea Arcangeli        2023-12-06  1867  			src_pmd = mm_alloc_pmd(mm, src_addr);
adef440691bab82 Andrea Arcangeli        2023-12-06  1868  			if (unlikely(!src_pmd)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1869  				err = -ENOMEM;
adef440691bab82 Andrea Arcangeli        2023-12-06  1870  				break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1871  			}
adef440691bab82 Andrea Arcangeli        2023-12-06  1872  		}
adef440691bab82 Andrea Arcangeli        2023-12-06  1873  		dst_pmd = mm_alloc_pmd(mm, dst_addr);
adef440691bab82 Andrea Arcangeli        2023-12-06  1874  		if (unlikely(!dst_pmd)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1875  			err = -ENOMEM;
adef440691bab82 Andrea Arcangeli        2023-12-06  1876  			break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1877  		}
adef440691bab82 Andrea Arcangeli        2023-12-06  1878  
adef440691bab82 Andrea Arcangeli        2023-12-06  1879  		dst_pmdval = pmdp_get_lockless(dst_pmd);
adef440691bab82 Andrea Arcangeli        2023-12-06  1880  		/*
adef440691bab82 Andrea Arcangeli        2023-12-06  1881  		 * If the dst_pmd is mapped as THP don't override it and just
adef440691bab82 Andrea Arcangeli        2023-12-06  1882  		 * be strict. If dst_pmd changes into TPH after this check, the
adef440691bab82 Andrea Arcangeli        2023-12-06  1883  		 * move_pages_huge_pmd() will detect the change and retry
adef440691bab82 Andrea Arcangeli        2023-12-06  1884  		 * while move_pages_pte() will detect the change and fail.
adef440691bab82 Andrea Arcangeli        2023-12-06  1885  		 */
adef440691bab82 Andrea Arcangeli        2023-12-06  1886  		if (unlikely(pmd_trans_huge(dst_pmdval))) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1887  			err = -EEXIST;
adef440691bab82 Andrea Arcangeli        2023-12-06  1888  			break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1889  		}
adef440691bab82 Andrea Arcangeli        2023-12-06  1890  
adef440691bab82 Andrea Arcangeli        2023-12-06  1891  		ptl = pmd_trans_huge_lock(src_pmd, src_vma);
adef440691bab82 Andrea Arcangeli        2023-12-06  1892  		if (ptl) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1893  			/* Check if we can move the pmd without splitting it. */
adef440691bab82 Andrea Arcangeli        2023-12-06  1894  			if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
adef440691bab82 Andrea Arcangeli        2023-12-06  1895  			    !pmd_none(dst_pmdval)) {
e06d03d5590ae1c Matthew Wilcox (Oracle  2024-03-26  1896) 				struct folio *folio = pmd_folio(*src_pmd);
adef440691bab82 Andrea Arcangeli        2023-12-06  1897  
5beaee54a324ba1 Matthew Wilcox (Oracle  2024-03-26  1898) 				if (!folio || (!is_huge_zero_folio(folio) &&
eb1521dad8f391d Suren Baghdasaryan      2024-01-31  1899  					       !PageAnonExclusive(&folio->page))) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1900  					spin_unlock(ptl);
adef440691bab82 Andrea Arcangeli        2023-12-06  1901  					err = -EBUSY;
adef440691bab82 Andrea Arcangeli        2023-12-06  1902  					break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1903  				}
adef440691bab82 Andrea Arcangeli        2023-12-06  1904  
adef440691bab82 Andrea Arcangeli        2023-12-06  1905  				spin_unlock(ptl);
adef440691bab82 Andrea Arcangeli        2023-12-06  1906  				split_huge_pmd(src_vma, src_pmd, src_addr);
adef440691bab82 Andrea Arcangeli        2023-12-06  1907  				/* The folio will be split by move_pages_pte() */
adef440691bab82 Andrea Arcangeli        2023-12-06  1908  				continue;
adef440691bab82 Andrea Arcangeli        2023-12-06  1909  			}
adef440691bab82 Andrea Arcangeli        2023-12-06  1910  
adef440691bab82 Andrea Arcangeli        2023-12-06  1911  			err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
adef440691bab82 Andrea Arcangeli        2023-12-06  1912  						  dst_pmdval, dst_vma, src_vma,
adef440691bab82 Andrea Arcangeli        2023-12-06  1913  						  dst_addr, src_addr);
adef440691bab82 Andrea Arcangeli        2023-12-06  1914  			step_size = HPAGE_PMD_SIZE;
adef440691bab82 Andrea Arcangeli        2023-12-06  1915  		} else {
3abf06e0c484867 Lokesh Gidra            2025-08-07  1916  			long ret;
3abf06e0c484867 Lokesh Gidra            2025-08-07  1917  
adef440691bab82 Andrea Arcangeli        2023-12-06  1918  			if (pmd_none(*src_pmd)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1919  				if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1920  					err = -ENOENT;
adef440691bab82 Andrea Arcangeli        2023-12-06  1921  					break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1922  				}
adef440691bab82 Andrea Arcangeli        2023-12-06  1923  				if (unlikely(__pte_alloc(mm, src_pmd))) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1924  					err = -ENOMEM;
adef440691bab82 Andrea Arcangeli        2023-12-06  1925  					break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1926  				}
adef440691bab82 Andrea Arcangeli        2023-12-06  1927  			}
adef440691bab82 Andrea Arcangeli        2023-12-06  1928  
adef440691bab82 Andrea Arcangeli        2023-12-06  1929  			if (unlikely(pte_alloc(mm, dst_pmd))) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1930  				err = -ENOMEM;
adef440691bab82 Andrea Arcangeli        2023-12-06  1931  				break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1932  			}
adef440691bab82 Andrea Arcangeli        2023-12-06  1933  
3abf06e0c484867 Lokesh Gidra            2025-08-07  1934  			ret = move_pages_ptes(mm, dst_pmd, src_pmd,
3abf06e0c484867 Lokesh Gidra            2025-08-07  1935  					      dst_vma, src_vma, dst_addr,
3abf06e0c484867 Lokesh Gidra            2025-08-07  1936  					      src_addr, src_end - src_addr, mode);
3abf06e0c484867 Lokesh Gidra            2025-08-07  1937  			if (ret > 0)
3abf06e0c484867 Lokesh Gidra            2025-08-07  1938  				step_size = ret;
3abf06e0c484867 Lokesh Gidra            2025-08-07  1939  			else
3abf06e0c484867 Lokesh Gidra            2025-08-07  1940  				err = ret;

Imagine "ret == 0".

adef440691bab82 Andrea Arcangeli        2023-12-06  1941  		}
adef440691bab82 Andrea Arcangeli        2023-12-06  1942  
adef440691bab82 Andrea Arcangeli        2023-12-06  1943  		cond_resched();
adef440691bab82 Andrea Arcangeli        2023-12-06  1944  
adef440691bab82 Andrea Arcangeli        2023-12-06  1945  		if (fatal_signal_pending(current)) {
adef440691bab82 Andrea Arcangeli        2023-12-06  1946  			/* Do not override an error */
adef440691bab82 Andrea Arcangeli        2023-12-06  1947  			if (!err || err == -EAGAIN)
adef440691bab82 Andrea Arcangeli        2023-12-06  1948  				err = -EINTR;
adef440691bab82 Andrea Arcangeli        2023-12-06  1949  			break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1950  		}
adef440691bab82 Andrea Arcangeli        2023-12-06  1951  
adef440691bab82 Andrea Arcangeli        2023-12-06  1952  		if (err) {

err is zero so we don't break here.

adef440691bab82 Andrea Arcangeli        2023-12-06  1953  			if (err == -EAGAIN)
adef440691bab82 Andrea Arcangeli        2023-12-06  1954  				continue;
adef440691bab82 Andrea Arcangeli        2023-12-06  1955  			break;
adef440691bab82 Andrea Arcangeli        2023-12-06  1956  		}
adef440691bab82 Andrea Arcangeli        2023-12-06  1957  
adef440691bab82 Andrea Arcangeli        2023-12-06  1958  		/* Proceed to the next page */
adef440691bab82 Andrea Arcangeli        2023-12-06 @1959  		dst_addr += step_size;
                                                                                    ^^^^^^^^^
Uninitialized or from the previous iteration.

adef440691bab82 Andrea Arcangeli        2023-12-06  1960  		src_addr += step_size;
adef440691bab82 Andrea Arcangeli        2023-12-06  1961  		moved += step_size;
adef440691bab82 Andrea Arcangeli        2023-12-06  1962  	}
adef440691bab82 Andrea Arcangeli        2023-12-06  1963  
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1964  out_unlock:
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1965  	up_read(&ctx->map_changing_lock);
867a43a34ff8a38 Lokesh Gidra            2024-02-15  1966  	uffd_move_unlock(dst_vma, src_vma);
adef440691bab82 Andrea Arcangeli        2023-12-06  1967  out:
31defc3b01d907e Tal Zussman             2025-06-19  1968  	VM_WARN_ON_ONCE(moved < 0);
31defc3b01d907e Tal Zussman             2025-06-19  1969  	VM_WARN_ON_ONCE(err > 0);
31defc3b01d907e Tal Zussman             2025-06-19  1970  	VM_WARN_ON_ONCE(!moved && !err);
adef440691bab82 Andrea Arcangeli        2023-12-06  1971  	return moved ? moved : err;
adef440691bab82 Andrea Arcangeli        2023-12-06  1972  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
  2025-08-08  6:18 ` Dan Carpenter
@ 2025-08-08 15:27   ` Lokesh Gidra
  0 siblings, 0 replies; 11+ messages in thread
From: Lokesh Gidra @ 2025-08-08 15:27 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: oe-kbuild, lkp, oe-kbuild-all

On Thu, Aug 7, 2025 at 11:18 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Hi Lokesh,
>
> kernel test robot noticed the following build warnings:
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Lokesh-Gidra/userfaultfd-opportunistic-TLB-flush-batching-for-present-pages-in-MOVE/20250807-184025
> base:   6e64f4580381e32c06ee146ca807c555b8f73e24
> patch link:    https://lore.kernel.org/r/20250807103902.2242717-1-lokeshgidra%40google.com
> patch subject: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
> config: openrisc-randconfig-r071-20250808 (https://download.01.org/0day-ci/archive/20250808/202508080639.Cf7SyRwQ-lkp@intel.com/config)
> compiler: or1k-linux-gcc (GCC) 10.5.0
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202508080639.Cf7SyRwQ-lkp@intel.com/
>
Thanks Dan. I will fix the warning in the next version of the patch.

> smatch warnings:
> mm/userfaultfd.c:1959 move_pages() error: uninitialized symbol 'step_size'.
>
> vim +/step_size +1959 mm/userfaultfd.c
>
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1802  ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1803                   unsigned long src_start, unsigned long len, __u64 mode)
> adef440691bab82 Andrea Arcangeli        2023-12-06  1804  {
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1805        struct mm_struct *mm = ctx->mm;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1806        struct vm_area_struct *src_vma, *dst_vma;
> 3abf06e0c484867 Lokesh Gidra            2025-08-07  1807        unsigned long src_addr, dst_addr, src_end;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1808        pmd_t *src_pmd, *dst_pmd;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1809        long err = -EINVAL;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1810        ssize_t moved = 0;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1811
> adef440691bab82 Andrea Arcangeli        2023-12-06  1812        /* Sanitize the command parameters. */
> 31defc3b01d907e Tal Zussman             2025-06-19  1813        VM_WARN_ON_ONCE(src_start & ~PAGE_MASK);
> 31defc3b01d907e Tal Zussman             2025-06-19  1814        VM_WARN_ON_ONCE(dst_start & ~PAGE_MASK);
> 31defc3b01d907e Tal Zussman             2025-06-19  1815        VM_WARN_ON_ONCE(len & ~PAGE_MASK);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1816
> adef440691bab82 Andrea Arcangeli        2023-12-06  1817        /* Does the address range wrap, or is the span zero-sized? */
> 31defc3b01d907e Tal Zussman             2025-06-19  1818        VM_WARN_ON_ONCE(src_start + len < src_start);
> 31defc3b01d907e Tal Zussman             2025-06-19  1819        VM_WARN_ON_ONCE(dst_start + len < dst_start);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1820
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1821        err = uffd_move_lock(mm, dst_start, src_start, &dst_vma, &src_vma);
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1822        if (err)
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1823                goto out;
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1824
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1825        /* Re-check after taking map_changing_lock */
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1826        err = -EAGAIN;
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1827        down_read(&ctx->map_changing_lock);
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1828        if (likely(atomic_read(&ctx->mmap_changing)))
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1829                goto out_unlock;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1830        /*
> adef440691bab82 Andrea Arcangeli        2023-12-06  1831         * Make sure the vma is not shared, that the src and dst remap
> adef440691bab82 Andrea Arcangeli        2023-12-06  1832         * ranges are both valid and fully within a single existing
> adef440691bab82 Andrea Arcangeli        2023-12-06  1833         * vma.
> adef440691bab82 Andrea Arcangeli        2023-12-06  1834         */
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1835        err = -EINVAL;
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1836        if (src_vma->vm_flags & VM_SHARED)
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1837                goto out_unlock;
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1838        if (src_start + len > src_vma->vm_end)
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1839                goto out_unlock;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1840
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1841        if (dst_vma->vm_flags & VM_SHARED)
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1842                goto out_unlock;
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1843        if (dst_start + len > dst_vma->vm_end)
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1844                goto out_unlock;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1845
> adef440691bab82 Andrea Arcangeli        2023-12-06  1846        err = validate_move_areas(ctx, src_vma, dst_vma);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1847        if (err)
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1848                goto out_unlock;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1849
> 3abf06e0c484867 Lokesh Gidra            2025-08-07  1850        for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
> 3abf06e0c484867 Lokesh Gidra            2025-08-07  1851             src_addr < src_end;) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1852                spinlock_t *ptl;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1853                pmd_t dst_pmdval;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1854                unsigned long step_size;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1855
> adef440691bab82 Andrea Arcangeli        2023-12-06  1856                /*
> adef440691bab82 Andrea Arcangeli        2023-12-06  1857                 * Below works because anonymous area would not have a
> adef440691bab82 Andrea Arcangeli        2023-12-06  1858                 * transparent huge PUD. If file-backed support is added,
> adef440691bab82 Andrea Arcangeli        2023-12-06  1859                 * that case would need to be handled here.
> adef440691bab82 Andrea Arcangeli        2023-12-06  1860                 */
> adef440691bab82 Andrea Arcangeli        2023-12-06  1861                src_pmd = mm_find_pmd(mm, src_addr);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1862                if (unlikely(!src_pmd)) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1863                        if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1864                                err = -ENOENT;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1865                                break;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1866                        }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1867                        src_pmd = mm_alloc_pmd(mm, src_addr);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1868                        if (unlikely(!src_pmd)) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1869                                err = -ENOMEM;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1870                                break;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1871                        }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1872                }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1873                dst_pmd = mm_alloc_pmd(mm, dst_addr);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1874                if (unlikely(!dst_pmd)) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1875                        err = -ENOMEM;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1876                        break;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1877                }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1878
> adef440691bab82 Andrea Arcangeli        2023-12-06  1879                dst_pmdval = pmdp_get_lockless(dst_pmd);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1880                /*
> adef440691bab82 Andrea Arcangeli        2023-12-06  1881                 * If the dst_pmd is mapped as THP don't override it and just
> adef440691bab82 Andrea Arcangeli        2023-12-06  1882                 * be strict. If dst_pmd changes into TPH after this check, the
> adef440691bab82 Andrea Arcangeli        2023-12-06  1883                 * move_pages_huge_pmd() will detect the change and retry
> adef440691bab82 Andrea Arcangeli        2023-12-06  1884                 * while move_pages_pte() will detect the change and fail.
> adef440691bab82 Andrea Arcangeli        2023-12-06  1885                 */
> adef440691bab82 Andrea Arcangeli        2023-12-06  1886                if (unlikely(pmd_trans_huge(dst_pmdval))) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1887                        err = -EEXIST;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1888                        break;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1889                }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1890
> adef440691bab82 Andrea Arcangeli        2023-12-06  1891                ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1892                if (ptl) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1893                        /* Check if we can move the pmd without splitting it. */
> adef440691bab82 Andrea Arcangeli        2023-12-06  1894                        if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> adef440691bab82 Andrea Arcangeli        2023-12-06  1895                            !pmd_none(dst_pmdval)) {
> e06d03d5590ae1c Matthew Wilcox (Oracle  2024-03-26  1896)                               struct folio *folio = pmd_folio(*src_pmd);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1897
> 5beaee54a324ba1 Matthew Wilcox (Oracle  2024-03-26  1898)                               if (!folio || (!is_huge_zero_folio(folio) &&
> eb1521dad8f391d Suren Baghdasaryan      2024-01-31  1899                                               !PageAnonExclusive(&folio->page))) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1900                                        spin_unlock(ptl);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1901                                        err = -EBUSY;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1902                                        break;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1903                                }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1904
> adef440691bab82 Andrea Arcangeli        2023-12-06  1905                                spin_unlock(ptl);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1906                                split_huge_pmd(src_vma, src_pmd, src_addr);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1907                                /* The folio will be split by move_pages_pte() */
> adef440691bab82 Andrea Arcangeli        2023-12-06  1908                                continue;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1909                        }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1910
> adef440691bab82 Andrea Arcangeli        2023-12-06  1911                        err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
> adef440691bab82 Andrea Arcangeli        2023-12-06  1912                                                  dst_pmdval, dst_vma, src_vma,
> adef440691bab82 Andrea Arcangeli        2023-12-06  1913                                                  dst_addr, src_addr);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1914                        step_size = HPAGE_PMD_SIZE;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1915                } else {
> 3abf06e0c484867 Lokesh Gidra            2025-08-07  1916                        long ret;
> 3abf06e0c484867 Lokesh Gidra            2025-08-07  1917
> adef440691bab82 Andrea Arcangeli        2023-12-06  1918                        if (pmd_none(*src_pmd)) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1919                                if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1920                                        err = -ENOENT;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1921                                        break;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1922                                }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1923                                if (unlikely(__pte_alloc(mm, src_pmd))) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1924                                        err = -ENOMEM;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1925                                        break;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1926                                }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1927                        }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1928
> adef440691bab82 Andrea Arcangeli        2023-12-06  1929                        if (unlikely(pte_alloc(mm, dst_pmd))) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1930                                err = -ENOMEM;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1931                                break;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1932                        }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1933
> 3abf06e0c484867 Lokesh Gidra            2025-08-07  1934                        ret = move_pages_ptes(mm, dst_pmd, src_pmd,
> 3abf06e0c484867 Lokesh Gidra            2025-08-07  1935                                              dst_vma, src_vma, dst_addr,
> 3abf06e0c484867 Lokesh Gidra            2025-08-07  1936                                              src_addr, src_end - src_addr, mode);
> 3abf06e0c484867 Lokesh Gidra            2025-08-07  1937                        if (ret > 0)
> 3abf06e0c484867 Lokesh Gidra            2025-08-07  1938                                step_size = ret;
> 3abf06e0c484867 Lokesh Gidra            2025-08-07  1939                        else
> 3abf06e0c484867 Lokesh Gidra            2025-08-07  1940                                err = ret;
>
> Imagine "ret == 0".
>
> adef440691bab82 Andrea Arcangeli        2023-12-06  1941                }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1942
> adef440691bab82 Andrea Arcangeli        2023-12-06  1943                cond_resched();
> adef440691bab82 Andrea Arcangeli        2023-12-06  1944
> adef440691bab82 Andrea Arcangeli        2023-12-06  1945                if (fatal_signal_pending(current)) {
> adef440691bab82 Andrea Arcangeli        2023-12-06  1946                        /* Do not override an error */
> adef440691bab82 Andrea Arcangeli        2023-12-06  1947                        if (!err || err == -EAGAIN)
> adef440691bab82 Andrea Arcangeli        2023-12-06  1948                                err = -EINTR;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1949                        break;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1950                }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1951
> adef440691bab82 Andrea Arcangeli        2023-12-06  1952                if (err) {
>
> err is zero so we don't break here.
>
> adef440691bab82 Andrea Arcangeli        2023-12-06  1953                        if (err == -EAGAIN)
> adef440691bab82 Andrea Arcangeli        2023-12-06  1954                                continue;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1955                        break;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1956                }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1957
> adef440691bab82 Andrea Arcangeli        2023-12-06  1958                /* Proceed to the next page */
> adef440691bab82 Andrea Arcangeli        2023-12-06 @1959                dst_addr += step_size;
>                                                                                     ^^^^^^^^^
> Uninitialized or from the previous iteration.
>
> adef440691bab82 Andrea Arcangeli        2023-12-06  1960                src_addr += step_size;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1961                moved += step_size;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1962        }
> adef440691bab82 Andrea Arcangeli        2023-12-06  1963
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1964  out_unlock:
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1965        up_read(&ctx->map_changing_lock);
> 867a43a34ff8a38 Lokesh Gidra            2024-02-15  1966        uffd_move_unlock(dst_vma, src_vma);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1967  out:
> 31defc3b01d907e Tal Zussman             2025-06-19  1968        VM_WARN_ON_ONCE(moved < 0);
> 31defc3b01d907e Tal Zussman             2025-06-19  1969        VM_WARN_ON_ONCE(err > 0);
> 31defc3b01d907e Tal Zussman             2025-06-19  1970        VM_WARN_ON_ONCE(!moved && !err);
> adef440691bab82 Andrea Arcangeli        2023-12-06  1971        return moved ? moved : err;
> adef440691bab82 Andrea Arcangeli        2023-12-06  1972  }
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
  2025-08-07 19:16 ` Peter Xu
  2025-08-07 22:54   ` Andrew Morton
@ 2025-08-08 16:29   ` Lokesh Gidra
  2025-08-10  6:31     ` Lokesh Gidra
  2025-08-11 14:00     ` Peter Xu
  1 sibling, 2 replies; 11+ messages in thread
From: Lokesh Gidra @ 2025-08-08 16:29 UTC (permalink / raw)
  To: Peter Xu
  Cc: akpm, aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
	Suren Baghdasaryan, Kalesh Singh, Barry Song, David Hildenbrand

On Thu, Aug 7, 2025 at 12:17 PM Peter Xu <peterx@redhat.com> wrote:
>
> Hi, Lokesh,
>
> On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> > MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> > for moving present pages. Mitigate this cost by opportunistically
> > batching present contiguous pages for TLB flushing.
> >
> > Without batching, in our testing on an arm64 Android device with UFFD GC,
> > which uses MOVE ioctl for compaction, we observed that out of the total
> > time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> > ~20% in vm_normal_folio().
> >
> > With batching, the proportion of vm_normal_folio() increases to over
> > 70% of move_pages_pte() without any changes to vm_normal_folio().
>
> Do you know why vm_normal_folio() could be expensive? I still see quite
> some other things this path needs to do.
>
Let's discuss this in Andrew's reply thread.

> > Furthermore, time spent within move_pages_pte() is only ~20%, which
> > includes TLB-flush overhead.
>
> Indeed this should already prove the optimization, I'm just curious whether
> you've run some benchmark on the GC app to show the real world benefit.
>
I did! The same benchmark through which I gathered these numbers, when
run on cuttlefish (qemu android instance on x86_64), the completion
time of the benchmark went down from ~45mins to ~20mins. The benchmark
is very GC intensive and the overhead of IPI on vCPUs seems to be
enormous leading to this drastic improvement.

In another instance, system_server, one of the most critical system
processes on android, saw over 50% reduction in GC compaction time on
an arm64 android device.

> >
> > Cc: Suren Baghdasaryan <surenb@google.com>
> > Cc: Kalesh Singh <kaleshsingh@google.com>
> > Cc: Barry Song <v-songbaohua@oppo.com>
> > Cc: David Hildenbrand <david@redhat.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > ---
> > Changes since v2 [1]
> > - Addressed VM_WARN_ON failure, per Lorenzo Stoakes
> > - Added check to ensure all batched pages share the same anon_vma
> >
> > Changes since v1 [2]
> > - Removed flush_tlb_batched_pending(), per Barry Song
> > - Unified single and multi page case, per Barry Song
> >
> > [1] https://lore.kernel.org/all/20250805121410.1658418-1-lokeshgidra@google.com/
> > [2] https://lore.kernel.org/all/20250731104726.103071-1-lokeshgidra@google.com/
> >
> >  mm/userfaultfd.c | 179 +++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 128 insertions(+), 51 deletions(-)
> >
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index cbed91b09640..78c732100aec 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -1026,18 +1026,64 @@ static inline bool is_pte_pages_stable(pte_t *dst_pte, pte_t *src_pte,
> >              pmd_same(dst_pmdval, pmdp_get_lockless(dst_pmd));
> >  }
> >
> > -static int move_present_pte(struct mm_struct *mm,
> > -                         struct vm_area_struct *dst_vma,
> > -                         struct vm_area_struct *src_vma,
> > -                         unsigned long dst_addr, unsigned long src_addr,
> > -                         pte_t *dst_pte, pte_t *src_pte,
> > -                         pte_t orig_dst_pte, pte_t orig_src_pte,
> > -                         pmd_t *dst_pmd, pmd_t dst_pmdval,
> > -                         spinlock_t *dst_ptl, spinlock_t *src_ptl,
> > -                         struct folio *src_folio)
> > +/*
> > + * Checks if the two ptes and the corresponding folio are eligible for batched
> > + * move. If so, then returns pointer to the folio, after locking it. Otherwise,
> > + * returns NULL.
> > + */
> > +static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
> > +                                              unsigned long src_addr,
> > +                                              pte_t *src_pte, pte_t *dst_pte,
> > +                                              struct anon_vma *src_anon_vma)
> > +{
> > +     pte_t orig_dst_pte, orig_src_pte;
> > +     struct folio *folio;
> > +
> > +     orig_dst_pte = ptep_get(dst_pte);
> > +     if (!pte_none(orig_dst_pte))
> > +             return NULL;
> > +
> > +     orig_src_pte = ptep_get(src_pte);
> > +     if (pte_none(orig_src_pte) || !pte_present(orig_src_pte) ||
>
> pte_none() check could be removed - the pte_present() check should make
> sure it's !none.
>
Makes sense. I'll make the change in the next version of the patch.

> > +         is_zero_pfn(pte_pfn(orig_src_pte)))
> > +             return NULL;
> > +
> > +     folio = vm_normal_folio(src_vma, src_addr, orig_src_pte);
> > +     if (!folio || !folio_trylock(folio))
> > +             return NULL;
>
> So here we don't take a refcount anymore, while the 1st folio that got
> passed in will still has the refcount boosted.  IMHO it would still be
> better to keep the behavior the same on the 1st and continuous folios..
>
> Or if this is intentional, maybe worth some comment.  More below on this..
>
This is indeed intentional, and I'll add a comment in the next
version. But let me explain:

The first folio needed the refcount as we need to pin the page before
releasing the src ptl. Also, because split_folio(), if called, expects
the caller to hold the lock as well as reference on the folio.

The subsequent folios in the batch are always within the ptl critical
section and neither the splits are required. Therefore, I didn't want
to unnecessarily increase the work done within the critical section.
But, please correct me if I'm misunderstanding something.

> > +     if (!PageAnonExclusive(&folio->page) || folio_test_large(folio) ||
> > +         folio_anon_vma(folio) != src_anon_vma) {
> > +             folio_unlock(folio);
> > +             return NULL;
> > +     }
> > +     return folio;
> > +}
> > +
> > +static long move_present_ptes(struct mm_struct *mm,
> > +                           struct vm_area_struct *dst_vma,
> > +                           struct vm_area_struct *src_vma,
> > +                           unsigned long dst_addr, unsigned long src_addr,
> > +                           pte_t *dst_pte, pte_t *src_pte,
> > +                           pte_t orig_dst_pte, pte_t orig_src_pte,
> > +                           pmd_t *dst_pmd, pmd_t dst_pmdval,
> > +                           spinlock_t *dst_ptl, spinlock_t *src_ptl,
> > +                           struct folio *src_folio, unsigned long len,
> > +                           struct anon_vma *src_anon_vma)
>
> (Not an immediate concern, but this function has potential to win the
>  max-num-of-parameters kernel function.. :)

I noticed the same when I made the change :) Maybe in a subsequent
patch if we inline is_pte_pages_stable() and PTL acquire/release in
move_pages_ptes(), then quite a few parameters can be reduced. But
then, IMO, even move_pages_ptes() refactoring is required as well.
>
> >  {
> >       int err = 0;
> > +     unsigned long src_start = src_addr;
> > +     unsigned long addr_end;
> >
> > +     if (len > PAGE_SIZE) {
> > +             addr_end = (dst_addr + PMD_SIZE) & PMD_MASK;
> > +             if (dst_addr + len > addr_end)
> > +                     len = addr_end - dst_addr;
>
> Use something like ALIGN() and MIN()?

Will do in the next version.
>
> > +
> > +             addr_end = (src_addr + PMD_SIZE) & PMD_MASK;
> > +             if (src_addr + len > addr_end)
> > +                     len = addr_end - src_addr;
>
> Same here.
>
Will do.

> > +     }
> > +     flush_cache_range(src_vma, src_addr, src_addr + len);
> >       double_pt_lock(dst_ptl, src_ptl);
> >
> >       if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> > @@ -1051,31 +1097,54 @@ static int move_present_pte(struct mm_struct *mm,
> >               err = -EBUSY;
> >               goto out;
> >       }
> > +     arch_enter_lazy_mmu_mode();
> > +
> > +     addr_end = src_start + len;
> > +     while (true) {
> > +             orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
> > +             /* Folio got pinned from under us. Put it back and fail the move. */
> > +             if (folio_maybe_dma_pinned(src_folio)) {
> > +                     set_pte_at(mm, src_addr, src_pte, orig_src_pte);
> > +                     err = -EBUSY;
> > +                     break;
> > +             }
> >
> > -     orig_src_pte = ptep_clear_flush(src_vma, src_addr, src_pte);
> > -     /* Folio got pinned from under us. Put it back and fail the move. */
> > -     if (folio_maybe_dma_pinned(src_folio)) {
> > -             set_pte_at(mm, src_addr, src_pte, orig_src_pte);
> > -             err = -EBUSY;
> > -             goto out;
> > -     }
> > -
> > -     folio_move_anon_rmap(src_folio, dst_vma);
> > -     src_folio->index = linear_page_index(dst_vma, dst_addr);
> > +             folio_move_anon_rmap(src_folio, dst_vma);
> > +             src_folio->index = linear_page_index(dst_vma, dst_addr);
> >
> > -     orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
> > -     /* Set soft dirty bit so userspace can notice the pte was moved */
> > +             orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
> > +             /* Set soft dirty bit so userspace can notice the pte was moved */
> >  #ifdef CONFIG_MEM_SOFT_DIRTY
> > -     orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
> > +             orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
> >  #endif
> > -     if (pte_dirty(orig_src_pte))
> > -             orig_dst_pte = pte_mkdirty(orig_dst_pte);
> > -     orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> > +             if (pte_dirty(orig_src_pte))
> > +                     orig_dst_pte = pte_mkdirty(orig_dst_pte);
> > +             orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> > +             set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
> > +
> > +             src_addr += PAGE_SIZE;
> > +             if (src_addr == addr_end)
> > +                     break;
> > +             src_pte++;
> > +             dst_pte++;
> > +
> > +             folio_unlock(src_folio);
> > +             src_folio = check_ptes_for_batched_move(src_vma, src_addr, src_pte,
> > +                                                     dst_pte, src_anon_vma);
> > +             if (!src_folio)
> > +                     break;
> > +             dst_addr += PAGE_SIZE;
> > +     }
> > +
> > +     arch_leave_lazy_mmu_mode();
> > +     if (src_addr > src_start)
> > +             flush_tlb_range(src_vma, src_start, src_addr);
> >
> > -     set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
> >  out:
> >       double_pt_unlock(dst_ptl, src_ptl);
> > -     return err;
> > +     if (src_folio)
> > +             folio_unlock(src_folio);
> > +     return src_addr > src_start ? src_addr - src_start : err;
> >  }
> >
> >  static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> > @@ -1140,7 +1209,7 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> >       set_pte_at(mm, dst_addr, dst_pte, orig_src_pte);
> >       double_pt_unlock(dst_ptl, src_ptl);
> >
> > -     return 0;
> > +     return PAGE_SIZE;
> >  }
> >
> >  static int move_zeropage_pte(struct mm_struct *mm,
> > @@ -1154,6 +1223,7 @@ static int move_zeropage_pte(struct mm_struct *mm,
> >  {
> >       pte_t zero_pte;
> >
> > +     flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
>
> If it's a zero page hence not writtable, do we still need to flush cache at
> all?  Looks harmless, but looks like not needed either.
>
I just realized when reading your comment that it is indeed not
required. There is no cacheline to be flushed for the zero-page :)

> >       double_pt_lock(dst_ptl, src_ptl);
> >       if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> >                                dst_pmd, dst_pmdval)) {
> > @@ -1167,20 +1237,19 @@ static int move_zeropage_pte(struct mm_struct *mm,
> >       set_pte_at(mm, dst_addr, dst_pte, zero_pte);
> >       double_pt_unlock(dst_ptl, src_ptl);
> >
> > -     return 0;
> > +     return PAGE_SIZE;
> >  }
> >
> >
> >  /*
> > - * The mmap_lock for reading is held by the caller. Just move the page
> > - * from src_pmd to dst_pmd if possible, and return true if succeeded
> > - * in moving the page.
> > + * The mmap_lock for reading is held by the caller. Just move the page(s)
> > + * from src_pmd to dst_pmd if possible, and return number of bytes moved.
> >   */
> > -static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > -                       struct vm_area_struct *dst_vma,
> > -                       struct vm_area_struct *src_vma,
> > -                       unsigned long dst_addr, unsigned long src_addr,
> > -                       __u64 mode)
> > +static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > +                         struct vm_area_struct *dst_vma,
> > +                         struct vm_area_struct *src_vma,
> > +                         unsigned long dst_addr, unsigned long src_addr,
> > +                         unsigned long len, __u64 mode)
> >  {
> >       swp_entry_t entry;
> >       struct swap_info_struct *si = NULL;
> > @@ -1196,9 +1265,8 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> >       struct mmu_notifier_range range;
> >       int err = 0;
> >
> > -     flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
> >       mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm,
> > -                             src_addr, src_addr + PAGE_SIZE);
> > +                             src_addr, src_addr + len);
> >       mmu_notifier_invalidate_range_start(&range);
> >  retry:
> >       /*
> > @@ -1257,7 +1325,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> >               if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES))
> >                       err = -ENOENT;
> >               else /* nothing to do to move a hole */
> > -                     err = 0;
> > +                     err = PAGE_SIZE;
> >               goto out;
> >       }
> >
> > @@ -1375,10 +1443,14 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> >                       }
> >               }
> >
> > -             err = move_present_pte(mm,  dst_vma, src_vma,
> > -                                    dst_addr, src_addr, dst_pte, src_pte,
> > -                                    orig_dst_pte, orig_src_pte, dst_pmd,
> > -                                    dst_pmdval, dst_ptl, src_ptl, src_folio);
> > +             err = move_present_ptes(mm, dst_vma, src_vma,
> > +                                     dst_addr, src_addr, dst_pte, src_pte,
> > +                                     orig_dst_pte, orig_src_pte, dst_pmd,
> > +                                     dst_pmdval, dst_ptl, src_ptl, src_folio,
> > +                                     len, src_anon_vma);
> > +             /* folio is already unlocked by move_present_ptes() */
> > +             folio_put(src_folio);
> > +             src_folio = NULL;
>
> So the function above now can move multiple folios but keep holding the
> 1st's refcount..  This still smells error prone, sooner or later.
>
> Would it be slightly better if we take a folio pointer in
> move_present_ptes(), and releae everything there (including reset the
> pointer)?
>
Yeah this seems cleaner. I'll do so in the next version.

> Thanks,
>
> >       } else {
> >               struct folio *folio = NULL;
> >
> > @@ -1732,7 +1804,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> >  {
> >       struct mm_struct *mm = ctx->mm;
> >       struct vm_area_struct *src_vma, *dst_vma;
> > -     unsigned long src_addr, dst_addr;
> > +     unsigned long src_addr, dst_addr, src_end;
> >       pmd_t *src_pmd, *dst_pmd;
> >       long err = -EINVAL;
> >       ssize_t moved = 0;
> > @@ -1775,8 +1847,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> >       if (err)
> >               goto out_unlock;
> >
> > -     for (src_addr = src_start, dst_addr = dst_start;
> > -          src_addr < src_start + len;) {
> > +     for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
> > +          src_addr < src_end;) {
> >               spinlock_t *ptl;
> >               pmd_t dst_pmdval;
> >               unsigned long step_size;
> > @@ -1841,6 +1913,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> >                                                 dst_addr, src_addr);
> >                       step_size = HPAGE_PMD_SIZE;
> >               } else {
> > +                     long ret;
> > +
> >                       if (pmd_none(*src_pmd)) {
> >                               if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> >                                       err = -ENOENT;
> > @@ -1857,10 +1931,13 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> >                               break;
> >                       }
> >
> > -                     err = move_pages_pte(mm, dst_pmd, src_pmd,
> > -                                          dst_vma, src_vma,
> > -                                          dst_addr, src_addr, mode);
> > -                     step_size = PAGE_SIZE;
> > +                     ret = move_pages_ptes(mm, dst_pmd, src_pmd,
> > +                                           dst_vma, src_vma, dst_addr,
> > +                                           src_addr, src_end - src_addr, mode);
> > +                     if (ret > 0)
> > +                             step_size = ret;
> > +                     else
> > +                             err = ret;
> >               }
> >
> >               cond_resched();
> >
> > base-commit: 6e64f4580381e32c06ee146ca807c555b8f73e24
> > --
> > 2.50.1.565.gc32cd1483b-goog
> >
>
> --
> Peter Xu
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
  2025-08-07 22:54   ` Andrew Morton
@ 2025-08-08 16:41     ` Lokesh Gidra
  0 siblings, 0 replies; 11+ messages in thread
From: Lokesh Gidra @ 2025-08-08 16:41 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Peter Xu, aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
	Suren Baghdasaryan, Kalesh Singh, Barry Song, David Hildenbrand

On Thu, Aug 7, 2025 at 3:54 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 7 Aug 2025 15:16:57 -0400 Peter Xu <peterx@redhat.com> wrote:
>
> > Hi, Lokesh,
> >
> > On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> > > MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> > > for moving present pages. Mitigate this cost by opportunistically
> > > batching present contiguous pages for TLB flushing.
> > >
> > > Without batching, in our testing on an arm64 Android device with UFFD GC,
> > > which uses MOVE ioctl for compaction, we observed that out of the total
> > > time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> > > ~20% in vm_normal_folio().
> > >
> > > With batching, the proportion of vm_normal_folio() increases to over
> > > 70% of move_pages_pte() without any changes to vm_normal_folio().
> >
> > Do you know why vm_normal_folio() could be expensive? I still see quite
> > some other things this path needs to do.
>
> Maybe as explained here?
> https://lkml.kernel.org/r/20250807185819.199865-1-lorenzo.stoakes@oracle.com
>
Thanks for sharing this, Andrew. IMHO, this seems like the most likely
reason to me. There is nothing there other than a cold access to the
page struct.

>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
  2025-08-08 16:29   ` Lokesh Gidra
@ 2025-08-10  6:31     ` Lokesh Gidra
  2025-08-11 14:00     ` Peter Xu
  1 sibling, 0 replies; 11+ messages in thread
From: Lokesh Gidra @ 2025-08-10  6:31 UTC (permalink / raw)
  To: Peter Xu
  Cc: akpm, aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
	Suren Baghdasaryan, Kalesh Singh, Barry Song, David Hildenbrand

On Fri, Aug 8, 2025 at 9:29 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
>
> On Thu, Aug 7, 2025 at 12:17 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > Hi, Lokesh,
> >
> > On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> > > MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> > > for moving present pages. Mitigate this cost by opportunistically
> > > batching present contiguous pages for TLB flushing.
> > >
> > > Without batching, in our testing on an arm64 Android device with UFFD GC,
> > > which uses MOVE ioctl for compaction, we observed that out of the total
> > > time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> > > ~20% in vm_normal_folio().
> > >
> > > With batching, the proportion of vm_normal_folio() increases to over
> > > 70% of move_pages_pte() without any changes to vm_normal_folio().
> >
> > Do you know why vm_normal_folio() could be expensive? I still see quite
> > some other things this path needs to do.
> >
> Let's discuss this in Andrew's reply thread.
>
> > > Furthermore, time spent within move_pages_pte() is only ~20%, which
> > > includes TLB-flush overhead.
> >
> > Indeed this should already prove the optimization, I'm just curious whether
> > you've run some benchmark on the GC app to show the real world benefit.
> >
> I did! The same benchmark through which I gathered these numbers, when
> run on cuttlefish (qemu android instance on x86_64), the completion
> time of the benchmark went down from ~45mins to ~20mins. The benchmark
> is very GC intensive and the overhead of IPI on vCPUs seems to be
> enormous leading to this drastic improvement.
>
> In another instance, system_server, one of the most critical system
> processes on android, saw over 50% reduction in GC compaction time on
> an arm64 android device.
>
> > >
> > > Cc: Suren Baghdasaryan <surenb@google.com>
> > > Cc: Kalesh Singh <kaleshsingh@google.com>
> > > Cc: Barry Song <v-songbaohua@oppo.com>
> > > Cc: David Hildenbrand <david@redhat.com>
> > > Cc: Peter Xu <peterx@redhat.com>
> > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > > ---
> > > Changes since v2 [1]
> > > - Addressed VM_WARN_ON failure, per Lorenzo Stoakes
> > > - Added check to ensure all batched pages share the same anon_vma
> > >
> > > Changes since v1 [2]
> > > - Removed flush_tlb_batched_pending(), per Barry Song
> > > - Unified single and multi page case, per Barry Song
> > >
> > > [1] https://lore.kernel.org/all/20250805121410.1658418-1-lokeshgidra@google.com/
> > > [2] https://lore.kernel.org/all/20250731104726.103071-1-lokeshgidra@google.com/
> > >
> > >  mm/userfaultfd.c | 179 +++++++++++++++++++++++++++++++++--------------
> > >  1 file changed, 128 insertions(+), 51 deletions(-)
> > >
> > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > index cbed91b09640..78c732100aec 100644
> > > --- a/mm/userfaultfd.c
> > > +++ b/mm/userfaultfd.c
> > > @@ -1026,18 +1026,64 @@ static inline bool is_pte_pages_stable(pte_t *dst_pte, pte_t *src_pte,
> > >              pmd_same(dst_pmdval, pmdp_get_lockless(dst_pmd));
> > >  }
> > >
> > > -static int move_present_pte(struct mm_struct *mm,
> > > -                         struct vm_area_struct *dst_vma,
> > > -                         struct vm_area_struct *src_vma,
> > > -                         unsigned long dst_addr, unsigned long src_addr,
> > > -                         pte_t *dst_pte, pte_t *src_pte,
> > > -                         pte_t orig_dst_pte, pte_t orig_src_pte,
> > > -                         pmd_t *dst_pmd, pmd_t dst_pmdval,
> > > -                         spinlock_t *dst_ptl, spinlock_t *src_ptl,
> > > -                         struct folio *src_folio)
> > > +/*
> > > + * Checks if the two ptes and the corresponding folio are eligible for batched
> > > + * move. If so, then returns pointer to the folio, after locking it. Otherwise,
> > > + * returns NULL.
> > > + */
> > > +static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
> > > +                                              unsigned long src_addr,
> > > +                                              pte_t *src_pte, pte_t *dst_pte,
> > > +                                              struct anon_vma *src_anon_vma)
> > > +{
> > > +     pte_t orig_dst_pte, orig_src_pte;
> > > +     struct folio *folio;
> > > +
> > > +     orig_dst_pte = ptep_get(dst_pte);
> > > +     if (!pte_none(orig_dst_pte))
> > > +             return NULL;
> > > +
> > > +     orig_src_pte = ptep_get(src_pte);
> > > +     if (pte_none(orig_src_pte) || !pte_present(orig_src_pte) ||
> >
> > pte_none() check could be removed - the pte_present() check should make
> > sure it's !none.
> >
> Makes sense. I'll make the change in the next version of the patch.
>
> > > +         is_zero_pfn(pte_pfn(orig_src_pte)))
> > > +             return NULL;
> > > +
> > > +     folio = vm_normal_folio(src_vma, src_addr, orig_src_pte);
> > > +     if (!folio || !folio_trylock(folio))
> > > +             return NULL;
> >
> > So here we don't take a refcount anymore, while the 1st folio that got
> > passed in will still has the refcount boosted.  IMHO it would still be
> > better to keep the behavior the same on the 1st and continuous folios..
> >
> > Or if this is intentional, maybe worth some comment.  More below on this..
> >
> This is indeed intentional, and I'll add a comment in the next
> version. But let me explain:
>
> The first folio needed the refcount as we need to pin the page before
> releasing the src ptl. Also, because split_folio(), if called, expects
> the caller to hold the lock as well as reference on the folio.
>
> The subsequent folios in the batch are always within the ptl critical
> section and neither the splits are required. Therefore, I didn't want
> to unnecessarily increase the work done within the critical section.
> But, please correct me if I'm misunderstanding something.
>
> > > +     if (!PageAnonExclusive(&folio->page) || folio_test_large(folio) ||
> > > +         folio_anon_vma(folio) != src_anon_vma) {
> > > +             folio_unlock(folio);
> > > +             return NULL;
> > > +     }
> > > +     return folio;
> > > +}
> > > +
> > > +static long move_present_ptes(struct mm_struct *mm,
> > > +                           struct vm_area_struct *dst_vma,
> > > +                           struct vm_area_struct *src_vma,
> > > +                           unsigned long dst_addr, unsigned long src_addr,
> > > +                           pte_t *dst_pte, pte_t *src_pte,
> > > +                           pte_t orig_dst_pte, pte_t orig_src_pte,
> > > +                           pmd_t *dst_pmd, pmd_t dst_pmdval,
> > > +                           spinlock_t *dst_ptl, spinlock_t *src_ptl,
> > > +                           struct folio *src_folio, unsigned long len,
> > > +                           struct anon_vma *src_anon_vma)
> >
> > (Not an immediate concern, but this function has potential to win the
> >  max-num-of-parameters kernel function.. :)
>
> I noticed the same when I made the change :) Maybe in a subsequent
> patch if we inline is_pte_pages_stable() and PTL acquire/release in
> move_pages_ptes(), then quite a few parameters can be reduced. But
> then, IMO, even move_pages_ptes() refactoring is required as well.
> >
> > >  {
> > >       int err = 0;
> > > +     unsigned long src_start = src_addr;
> > > +     unsigned long addr_end;
> > >
> > > +     if (len > PAGE_SIZE) {
> > > +             addr_end = (dst_addr + PMD_SIZE) & PMD_MASK;
> > > +             if (dst_addr + len > addr_end)
> > > +                     len = addr_end - dst_addr;
> >
> > Use something like ALIGN() and MIN()?
>
> Will do in the next version.
> >
> > > +
> > > +             addr_end = (src_addr + PMD_SIZE) & PMD_MASK;
> > > +             if (src_addr + len > addr_end)
> > > +                     len = addr_end - src_addr;
> >
> > Same here.
> >
> Will do.
>
> > > +     }
> > > +     flush_cache_range(src_vma, src_addr, src_addr + len);
> > >       double_pt_lock(dst_ptl, src_ptl);
> > >
> > >       if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> > > @@ -1051,31 +1097,54 @@ static int move_present_pte(struct mm_struct *mm,
> > >               err = -EBUSY;
> > >               goto out;
> > >       }
> > > +     arch_enter_lazy_mmu_mode();
> > > +
> > > +     addr_end = src_start + len;
> > > +     while (true) {
> > > +             orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
> > > +             /* Folio got pinned from under us. Put it back and fail the move. */
> > > +             if (folio_maybe_dma_pinned(src_folio)) {
> > > +                     set_pte_at(mm, src_addr, src_pte, orig_src_pte);
> > > +                     err = -EBUSY;
> > > +                     break;
> > > +             }
> > >
> > > -     orig_src_pte = ptep_clear_flush(src_vma, src_addr, src_pte);
> > > -     /* Folio got pinned from under us. Put it back and fail the move. */
> > > -     if (folio_maybe_dma_pinned(src_folio)) {
> > > -             set_pte_at(mm, src_addr, src_pte, orig_src_pte);
> > > -             err = -EBUSY;
> > > -             goto out;
> > > -     }
> > > -
> > > -     folio_move_anon_rmap(src_folio, dst_vma);
> > > -     src_folio->index = linear_page_index(dst_vma, dst_addr);
> > > +             folio_move_anon_rmap(src_folio, dst_vma);
> > > +             src_folio->index = linear_page_index(dst_vma, dst_addr);
> > >
> > > -     orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
> > > -     /* Set soft dirty bit so userspace can notice the pte was moved */
> > > +             orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
> > > +             /* Set soft dirty bit so userspace can notice the pte was moved */
> > >  #ifdef CONFIG_MEM_SOFT_DIRTY
> > > -     orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
> > > +             orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
> > >  #endif
> > > -     if (pte_dirty(orig_src_pte))
> > > -             orig_dst_pte = pte_mkdirty(orig_dst_pte);
> > > -     orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> > > +             if (pte_dirty(orig_src_pte))
> > > +                     orig_dst_pte = pte_mkdirty(orig_dst_pte);
> > > +             orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> > > +             set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
> > > +
> > > +             src_addr += PAGE_SIZE;
> > > +             if (src_addr == addr_end)
> > > +                     break;
> > > +             src_pte++;
> > > +             dst_pte++;
> > > +
> > > +             folio_unlock(src_folio);
> > > +             src_folio = check_ptes_for_batched_move(src_vma, src_addr, src_pte,
> > > +                                                     dst_pte, src_anon_vma);
> > > +             if (!src_folio)
> > > +                     break;
> > > +             dst_addr += PAGE_SIZE;
> > > +     }
> > > +
> > > +     arch_leave_lazy_mmu_mode();
> > > +     if (src_addr > src_start)
> > > +             flush_tlb_range(src_vma, src_start, src_addr);
> > >
> > > -     set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
> > >  out:
> > >       double_pt_unlock(dst_ptl, src_ptl);
> > > -     return err;
> > > +     if (src_folio)
> > > +             folio_unlock(src_folio);
> > > +     return src_addr > src_start ? src_addr - src_start : err;
> > >  }
> > >
> > >  static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> > > @@ -1140,7 +1209,7 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> > >       set_pte_at(mm, dst_addr, dst_pte, orig_src_pte);
> > >       double_pt_unlock(dst_ptl, src_ptl);
> > >
> > > -     return 0;
> > > +     return PAGE_SIZE;
> > >  }
> > >
> > >  static int move_zeropage_pte(struct mm_struct *mm,
> > > @@ -1154,6 +1223,7 @@ static int move_zeropage_pte(struct mm_struct *mm,
> > >  {
> > >       pte_t zero_pte;
> > >
> > > +     flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
> >
> > If it's a zero page hence not writtable, do we still need to flush cache at
> > all?  Looks harmless, but looks like not needed either.
> >
> I just realized when reading your comment that it is indeed not
> required. There is no cacheline to be flushed for the zero-page :)
>
> > >       double_pt_lock(dst_ptl, src_ptl);
> > >       if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> > >                                dst_pmd, dst_pmdval)) {
> > > @@ -1167,20 +1237,19 @@ static int move_zeropage_pte(struct mm_struct *mm,
> > >       set_pte_at(mm, dst_addr, dst_pte, zero_pte);
> > >       double_pt_unlock(dst_ptl, src_ptl);
> > >
> > > -     return 0;
> > > +     return PAGE_SIZE;
> > >  }
> > >
> > >
> > >  /*
> > > - * The mmap_lock for reading is held by the caller. Just move the page
> > > - * from src_pmd to dst_pmd if possible, and return true if succeeded
> > > - * in moving the page.
> > > + * The mmap_lock for reading is held by the caller. Just move the page(s)
> > > + * from src_pmd to dst_pmd if possible, and return number of bytes moved.
> > >   */
> > > -static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > > -                       struct vm_area_struct *dst_vma,
> > > -                       struct vm_area_struct *src_vma,
> > > -                       unsigned long dst_addr, unsigned long src_addr,
> > > -                       __u64 mode)
> > > +static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > > +                         struct vm_area_struct *dst_vma,
> > > +                         struct vm_area_struct *src_vma,
> > > +                         unsigned long dst_addr, unsigned long src_addr,
> > > +                         unsigned long len, __u64 mode)
> > >  {
> > >       swp_entry_t entry;
> > >       struct swap_info_struct *si = NULL;
> > > @@ -1196,9 +1265,8 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > >       struct mmu_notifier_range range;
> > >       int err = 0;
> > >
> > > -     flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
> > >       mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm,
> > > -                             src_addr, src_addr + PAGE_SIZE);
> > > +                             src_addr, src_addr + len);
> > >       mmu_notifier_invalidate_range_start(&range);
> > >  retry:
> > >       /*
> > > @@ -1257,7 +1325,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > >               if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES))
> > >                       err = -ENOENT;
> > >               else /* nothing to do to move a hole */
> > > -                     err = 0;
> > > +                     err = PAGE_SIZE;
> > >               goto out;
> > >       }
> > >
> > > @@ -1375,10 +1443,14 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > >                       }
> > >               }
> > >
> > > -             err = move_present_pte(mm,  dst_vma, src_vma,
> > > -                                    dst_addr, src_addr, dst_pte, src_pte,
> > > -                                    orig_dst_pte, orig_src_pte, dst_pmd,
> > > -                                    dst_pmdval, dst_ptl, src_ptl, src_folio);
> > > +             err = move_present_ptes(mm, dst_vma, src_vma,
> > > +                                     dst_addr, src_addr, dst_pte, src_pte,
> > > +                                     orig_dst_pte, orig_src_pte, dst_pmd,
> > > +                                     dst_pmdval, dst_ptl, src_ptl, src_folio,
> > > +                                     len, src_anon_vma);
> > > +             /* folio is already unlocked by move_present_ptes() */
> > > +             folio_put(src_folio);
> > > +             src_folio = NULL;
> >
> > So the function above now can move multiple folios but keep holding the
> > 1st's refcount..  This still smells error prone, sooner or later.
> >
> > Would it be slightly better if we take a folio pointer in
> > move_present_ptes(), and releae everything there (including reset the
> > pointer)?
> >
> Yeah this seems cleaner. I'll do so in the next version.
>
Uploaded v4 addressing all the comments here:
https://lore.kernel.org/all/20250810062912.1096815-1-lokeshgidra@google.com/

Thanks.
> > Thanks,
> >
> > >       } else {
> > >               struct folio *folio = NULL;
> > >
> > > @@ -1732,7 +1804,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > >  {
> > >       struct mm_struct *mm = ctx->mm;
> > >       struct vm_area_struct *src_vma, *dst_vma;
> > > -     unsigned long src_addr, dst_addr;
> > > +     unsigned long src_addr, dst_addr, src_end;
> > >       pmd_t *src_pmd, *dst_pmd;
> > >       long err = -EINVAL;
> > >       ssize_t moved = 0;
> > > @@ -1775,8 +1847,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > >       if (err)
> > >               goto out_unlock;
> > >
> > > -     for (src_addr = src_start, dst_addr = dst_start;
> > > -          src_addr < src_start + len;) {
> > > +     for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
> > > +          src_addr < src_end;) {
> > >               spinlock_t *ptl;
> > >               pmd_t dst_pmdval;
> > >               unsigned long step_size;
> > > @@ -1841,6 +1913,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > >                                                 dst_addr, src_addr);
> > >                       step_size = HPAGE_PMD_SIZE;
> > >               } else {
> > > +                     long ret;
> > > +
> > >                       if (pmd_none(*src_pmd)) {
> > >                               if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> > >                                       err = -ENOENT;
> > > @@ -1857,10 +1931,13 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > >                               break;
> > >                       }
> > >
> > > -                     err = move_pages_pte(mm, dst_pmd, src_pmd,
> > > -                                          dst_vma, src_vma,
> > > -                                          dst_addr, src_addr, mode);
> > > -                     step_size = PAGE_SIZE;
> > > +                     ret = move_pages_ptes(mm, dst_pmd, src_pmd,
> > > +                                           dst_vma, src_vma, dst_addr,
> > > +                                           src_addr, src_end - src_addr, mode);
> > > +                     if (ret > 0)
> > > +                             step_size = ret;
> > > +                     else
> > > +                             err = ret;
> > >               }
> > >
> > >               cond_resched();
> > >
> > > base-commit: 6e64f4580381e32c06ee146ca807c555b8f73e24
> > > --
> > > 2.50.1.565.gc32cd1483b-goog
> > >
> >
> > --
> > Peter Xu
> >


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
  2025-08-08 16:29   ` Lokesh Gidra
  2025-08-10  6:31     ` Lokesh Gidra
@ 2025-08-11 14:00     ` Peter Xu
  2025-08-12 14:01       ` Lokesh Gidra
  1 sibling, 1 reply; 11+ messages in thread
From: Peter Xu @ 2025-08-11 14:00 UTC (permalink / raw)
  To: Lokesh Gidra
  Cc: akpm, aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
	Suren Baghdasaryan, Kalesh Singh, Barry Song, David Hildenbrand

On Fri, Aug 08, 2025 at 09:29:58AM -0700, Lokesh Gidra wrote:
> On Thu, Aug 7, 2025 at 12:17 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > Hi, Lokesh,
> >
> > On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> > > MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> > > for moving present pages. Mitigate this cost by opportunistically
> > > batching present contiguous pages for TLB flushing.
> > >
> > > Without batching, in our testing on an arm64 Android device with UFFD GC,
> > > which uses MOVE ioctl for compaction, we observed that out of the total
> > > time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> > > ~20% in vm_normal_folio().
> > >
> > > With batching, the proportion of vm_normal_folio() increases to over
> > > 70% of move_pages_pte() without any changes to vm_normal_folio().
> >
> > Do you know why vm_normal_folio() could be expensive? I still see quite
> > some other things this path needs to do.
> >
> Let's discuss this in Andrew's reply thread.

Sorry to get back to this late.  Thanks for the link, Andrew!

> 
> > > Furthermore, time spent within move_pages_pte() is only ~20%, which
> > > includes TLB-flush overhead.
> >
> > Indeed this should already prove the optimization, I'm just curious whether
> > you've run some benchmark on the GC app to show the real world benefit.
> >
> I did! The same benchmark through which I gathered these numbers, when
> run on cuttlefish (qemu android instance on x86_64), the completion
> time of the benchmark went down from ~45mins to ~20mins. The benchmark
> is very GC intensive and the overhead of IPI on vCPUs seems to be
> enormous leading to this drastic improvement.
> 
> In another instance, system_server, one of the most critical system
> processes on android, saw over 50% reduction in GC compaction time on
> an arm64 android device.

Would you mind add some of these numbers into the commit message when you
repost?

Thanks,

-- 
Peter Xu



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
  2025-08-11 14:00     ` Peter Xu
@ 2025-08-12 14:01       ` Lokesh Gidra
  0 siblings, 0 replies; 11+ messages in thread
From: Lokesh Gidra @ 2025-08-12 14:01 UTC (permalink / raw)
  To: Peter Xu
  Cc: akpm, aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
	Suren Baghdasaryan, Kalesh Singh, Barry Song, David Hildenbrand

On Mon, Aug 11, 2025 at 7:00 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Aug 08, 2025 at 09:29:58AM -0700, Lokesh Gidra wrote:
> > On Thu, Aug 7, 2025 at 12:17 PM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > Hi, Lokesh,
> > >
> > > On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> > > > MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> > > > for moving present pages. Mitigate this cost by opportunistically
> > > > batching present contiguous pages for TLB flushing.
> > > >
> > > > Without batching, in our testing on an arm64 Android device with UFFD GC,
> > > > which uses MOVE ioctl for compaction, we observed that out of the total
> > > > time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> > > > ~20% in vm_normal_folio().
> > > >
> > > > With batching, the proportion of vm_normal_folio() increases to over
> > > > 70% of move_pages_pte() without any changes to vm_normal_folio().
> > >
> > > Do you know why vm_normal_folio() could be expensive? I still see quite
> > > some other things this path needs to do.
> > >
> > Let's discuss this in Andrew's reply thread.
>
> Sorry to get back to this late.  Thanks for the link, Andrew!
>
> >
> > > > Furthermore, time spent within move_pages_pte() is only ~20%, which
> > > > includes TLB-flush overhead.
> > >
> > > Indeed this should already prove the optimization, I'm just curious whether
> > > you've run some benchmark on the GC app to show the real world benefit.
> > >
> > I did! The same benchmark through which I gathered these numbers, when
> > run on cuttlefish (qemu android instance on x86_64), the completion
> > time of the benchmark went down from ~45mins to ~20mins. The benchmark
> > is very GC intensive and the overhead of IPI on vCPUs seems to be
> > enormous leading to this drastic improvement.
> >
> > In another instance, system_server, one of the most critical system
> > processes on android, saw over 50% reduction in GC compaction time on
> > an arm64 android device.
>
> Would you mind add some of these numbers into the commit message when you
> repost?

I can certainly do that in v5. But I already sent v4 addressing all
your other feedback. I'll incorporate these numbers as well as any
additional changes suggested in v4 in v5.
>
> Thanks,
>
> --
> Peter Xu
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-08-12 14:02 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-07 10:39 [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE Lokesh Gidra
2025-08-07 19:16 ` Peter Xu
2025-08-07 22:54   ` Andrew Morton
2025-08-08 16:41     ` Lokesh Gidra
2025-08-08 16:29   ` Lokesh Gidra
2025-08-10  6:31     ` Lokesh Gidra
2025-08-11 14:00     ` Peter Xu
2025-08-12 14:01       ` Lokesh Gidra
2025-08-08  6:18 ` Dan Carpenter
2025-08-08 15:27   ` Lokesh Gidra
  -- strict thread matches above, loose matches on Subject: below --
2025-08-07 22:24 kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.