Re: [PATCH 8/8] mm/gup: batch contiguous same-folio PTEs into one refcount grab

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm/gup: inverted dirty merge in gup_pte_batch_len() skips folio_mark_dirty()
--

commit 67f7a0d0fa0dd1ef10a67f56f3a836ca56f23a7f
Author: Rik van Riel <riel@surriel.com>

mm/gup: batch contiguous same-folio PTEs into one refcount grab

This commit introduces gup_pte_batch_len() in mm/gup.c to batch contiguous
PTEs mapping the same large folio in follow_page_pte(), committing each run
with a single refcount grab and folio_mark_accessed() call.

> diff --git a/mm/gup.c b/mm/gup.c
> index 67e20b326be6e..47f204e21a3a7 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -937,6 +937,35 @@ static long gup_lookup_pte(struct vm_area_struct *vma, unsigned long address,
>  	return 0;
>  }
>  
> +/*
> + * Return how many PTEs map consecutive pages of the same folio and can be
> + * committed as one run. Always at least 1.
> + *
> + * The write-fault and unshare checks in gup_lookup_pte() are per PTE, but a
> + * writable run needs no repeat: a writable anon page is exclusive. A read-only
> + * run under FOLL_WRITE or FOLL_PIN does need the per-page check, so it stays
> + * one page at a time.
> + */
> +static unsigned long gup_pte_batch_len(struct vm_area_struct *vma,
> +		unsigned long address, unsigned long walk_end,
> +		struct folio *folio, pte_t *ptep, pte_t *batch_pte, unsigned int flags)
> +{
> +	unsigned long max;
> +
> +	if (!folio_test_large(folio))
> +		return 1;
> +	if (!pte_write(*batch_pte) && (flags & (FOLL_WRITE | FOLL_PIN)))
> +		return 1;
> +
> +	max = (walk_end - address) >> PAGE_SHIFT;
> +	if (max <= 1)
> +		return 1;
> +
> +	/* Merge young/dirty across batch so folio_mark_dirty sees any dirty. */
> +	return folio_pte_batch_flags(folio, vma, ptep, batch_pte, max,
> +				     FPB_RESPECT_WRITE | FPB_MERGE_YOUNG_DIRTY);
> +}
> +
>  /*
>   * Walk the PTEs from the start address to the end of this page table or VMA,
>   * whichever comes first, and commit every page found.
> @@ -968,7 +997,10 @@ static long follow_page_pte(struct vm_area_struct *vma,
>  
>  	for (; address < walk_end; address += PAGE_SIZE, ptep++) {
>  		pte_t pte = ptep_get(ptep);
> +		struct folio *folio;
>  		struct page *page;
> +		unsigned long batch;
> +		pte_t batch_pte;
>  
>  		ret = gup_lookup_pte(vma, address, ptep, pte, flags, &page);
>  		if (ret || !page) {
> @@ -983,12 +1015,23 @@ static long follow_page_pte(struct vm_area_struct *vma,
>  				need_no_page_table = true;
>  			break;
>  		}
> -		ret = gup_grab_folio_pages(page_folio(page), page,
> -						     1, pte_dirty(pte), flags,
> +		folio = page_folio(page);
> +		batch_pte = pte;
> +
> +		batch = gup_pte_batch_len(vma, address, walk_end, folio,
> +					 ptep, &batch_pte, flags);
> +		ret = gup_grab_folio_pages(folio, page, batch,
> +						     pte_dirty(batch_pte), flags,
>  						     pages ? pages + nr : NULL);

[Severity: High]
As reported by Sashiko on RFC v3 [1], is the dirty bit merging in
gup_pte_batch_len() inverted relative to what gup_grab_folio_pages() expects?

In mm/gup.c:gup_grab_folio_pages(), the folio is marked dirty when the
passed-in dirty argument is false:

	if (flags & FOLL_TOUCH) {
		if ((flags & FOLL_WRITE) && !dirty && !folio_test_dirty(folio))
			folio_mark_dirty(folio);

Because GUP does not set the dirty bit in the PTE itself, a clean PTE
(!pte_dirty(pte)) requires calling folio_mark_dirty(folio) directly, whereas
an already-dirty PTE only avoids that call because the page table already
tracks its dirtiness.

Prior to this change, follow_page_pte() called gup_grab_folio_pages() for
each PTE individually, so folio_mark_dirty(folio) was called whenever any PTE
in the walked range was clean, and was only skipped if all PTEs in the range
were already dirty.

With FPB_MERGE_YOUNG_DIRTY in gup_pte_batch_len(), folio_pte_batch_flags()
sets the dirty bit on batch_pte if any PTE in the batch is dirty (and even
without FPB_MERGE_YOUNG_DIRTY, batch_pte starts with the first PTE's dirty
bit rather than the logical AND across the batch). When a batch contains at
least one dirty PTE alongside one or more clean PTEs, pte_dirty(batch_pte) is
true, so gup_grab_folio_pages() sees !dirty as false and skips
folio_mark_dirty(folio).

For example, if a PTE-mapped large anonymous folio has been marked MADV_FREE
(leaving PTEs writable and clean with PG_dirty and PG_swapbacked clear), a
CPU write to a single subpage sets pte_dirty() on that one PTE. If the range
is then walked with FOLL_WRITE | FOLL_TOUCH (such as via MADV_POPULATE_WRITE,
process_vm_writev(), or O_DIRECT), gup_grab_folio_pages() will skip
folio_mark_dirty(folio) while the remaining PTEs stay clean. If the single
dirty subpage is later unmapped via MADV_DONTNEED or munmap(), the remaining
PTEs and the folio are still clean and lazyfree, allowing reclaim to discard
the modified folio.

Should follow_page_pte() and gup_pte_batch_len() pass dirty = true to
gup_grab_folio_pages() only when all PTEs in the batch are dirty, rather than
when any PTE in the batch is dirty?

[1] https://lore.kernel.org/r/20260811025157.1632867-9-riel@surriel.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102193026.3860700-1-riel@surriel.com?part=8