Re: [PATCH 1/2] mm/gup: pass an end address to follow_page_mask() and return a page count

[David Hildenbrand (Arm)]

On 7/29/26 05:02, Rik van Riel wrote:
> follow_page_mask() reports how many pages the returned page covers with a
> page_mask: a bitmask of the enclosing naturally-aligned huge page. Callers
> turn that into a stride, which assumes the run is a power-of-two block
> aligned to its own size.
> 
> That form cannot describe an arbitrary contiguous run, which a later patch
> needs to batch PTE-mapped large folios.
> 
> Replace the page_mask output with nr_pages, the number of contiguous pages
> the returned page covers starting at @address, and add an @end argument
> bounding how far the walk looks, so a small request does not scan an entire
> large folio.
> 
> The huge PMD and PUD paths report the same stride as before, now as a count
> clamped to @end. __get_user_pages() uses the count directly.
> 
> No functional change intended.
> 
> Assisted-by: Claude:claude-opus-4.8
> Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>


This patch moves us closer towards the GUP-fast path, however, that is about to
change:

	https://lore.kernel.org/all/20260219050250.266876166@xxxxxxxxx/

With that in mind, I guess in the future we want to have:

static long follow_page_mask(struct vm_area_struct *vma,
		unsigned long address, unsigned long end,
		unsigned int flags, struct page **pages)

or even better, now that follow_page() no longer exists, in light of gup_fast():


static long gup_slow(struct vm_area_struct *vma,
		unsigned long address, unsigned long end,
		unsigned int flags, struct page **pages)

Returning the number of pinned pages.


In particular, such an interface would allows us to process multiple
ptes/pmds/... in one go, without having to restart the page table walk all the
time for each non-batchable folio.

And make gup_slow make look more like gup_fast.

I think that's one of the suboptimal things that we inherited from the legacy
follow_page() interface I ripped out a while ago.

... and get rid of that nasty "if (page_increm > 1) {" handling outside of
follow_page_mask().


I'm ok with doing this patch here first, because what I describe is a much
bigger rework.

But being able to pin many pages in one page table walk sounds like another
excellent optimization opportunity, which would even reduce the batching benefit
you see today with patch #2 by simply making PTE processing easier.


I think I can find someone to work on this.

> ---
>  mm/gup.c | 94 +++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 52 insertions(+), 42 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 99902c15703b..3437cd3407d5 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -647,8 +647,9 @@ static inline bool can_follow_write_pud(pud_t pud, struct page *page,
>  }
>  
>  static struct page *follow_huge_pud(struct vm_area_struct *vma,
> -				    unsigned long addr, pud_t *pudp,
> -				    int flags, unsigned long *page_mask)
> +				    unsigned long addr, unsigned long end,
> +				    pud_t *pudp, int flags,
> +				    unsigned long *nr_pages)

While at it, please convert this end the other instances to two-tab indent on
the second+ line.

[...]

>  /**
>   * follow_page_mask - look up a page descriptor from a user-virtual address
>   * @vma: vm_area_struct mapping @address
>   * @address: virtual address to look up
> + * @end: virtual address at which to stop batching contiguous pages
>   * @flags: flags modifying lookup behaviour
> - * @page_mask: a pointer to output page_mask
> + * @nr_pages: output; number of contiguous pages the caller can read
>   *
>   * @flags can have FOLL_ flags set, defined in <linux/mm.h>
>   *
> @@ -998,15 +1007,17 @@ static struct page *follow_p4d_mask(struct vm_area_struct *vma,
>   * trigger a fault with FAULT_FLAG_UNSHARE set. Note that unsharing is only
>   * relevant with FOLL_PIN and !FOLL_WRITE.
>   *
> - * On output, @page_mask is set according to the size of the page.
> + * On output, @nr_pages holds how many contiguous pages the folio that includes
> + * the returned page has mapped into this process, so the caller can advance
> + * over a large folio in one step.
>   *
>   * Return: the mapped (struct page *), %NULL if no mapping exists, or
>   * an error pointer if there is a mapping to something not represented
>   * by a page descriptor (see also vm_normal_page()).
>   */
>  static struct page *follow_page_mask(struct vm_area_struct *vma,
> -			      unsigned long address, unsigned int flags,
> -			      unsigned long *page_mask)
> +			      unsigned long address, unsigned long end,
> +			      unsigned int flags, unsigned long *nr_pages)
>  {
>  	pgd_t *pgd;
>  	struct mm_struct *mm = vma->vm_mm;
> @@ -1014,13 +1025,13 @@ static struct page *follow_page_mask(struct vm_area_struct *vma,
>  
>  	vma_pgtable_walk_begin(vma);
>  
> -	*page_mask = 0;
> +	*nr_pages = 1;
>  	pgd = pgd_offset(mm, address);
>  
>  	if (pgd_none(*pgd) || unlikely(pgd_bad(*pgd)))
>  		page = no_page_table(vma, flags, address);
>  	else
> -		page = follow_p4d_mask(vma, address, pgd, flags, page_mask);
> +		page = follow_p4d_mask(vma, address, end, pgd, flags, nr_pages);
>  
>  	vma_pgtable_walk_end(vma);
>  
> @@ -1358,7 +1369,6 @@ static long __get_user_pages(struct mm_struct *mm,
>  {
>  	long ret = 0, i = 0;
>  	struct vm_area_struct *vma = NULL;
> -	unsigned long page_mask = 0;
>  
>  	if (!nr_pages)
>  		return 0;
> @@ -1373,7 +1383,7 @@ static long __get_user_pages(struct mm_struct *mm,
>  
>  	do {
>  		struct page *page;
> -		unsigned int page_increm;
> +		unsigned long page_increm;
>  
>  		/* first iteration or cross vma bound */
>  		if (!vma || start >= vma->vm_end) {
> @@ -1400,7 +1410,7 @@ static long __get_user_pages(struct mm_struct *mm,
>  						pages ? &page : NULL);
>  				if (ret)
>  					goto out;
> -				page_mask = 0;
> +				page_increm = 1;
>  				goto next_page;
>  			}

God is this (existing) page_increm handling a hacked-on mess :)

-- 
Cheers,

David