Re: [RFC PATCH v3 4/8] mm/gup: break out follow_one_pte() helper

[Suren Baghdasaryan]

On Mon, Aug 10, 2026 at 8:07 PM Rik van Riel <riel@xxxxxxxxxxx> wrote:
>
> follow_page_pte() is 92 lines and does two separate things: work out
> which page a PTE maps, if any, and commit to the page it found. The
> first half reaches the second through five exit paths, two of which
> unwind the PTE lock in different ways.
>
> Split the resolve half into follow_one_pte(), which returns the page it
> resolved, NULL when the PTE cannot be followed, or the errno the caller
> must report. follow_page_pte() is left with one unlock and one exit.
>
> no_page_table() can look up the page cache, so the case that needs it is
> recorded and the call made after dropping the PTE lock, as before.
>
> No functional changes intended.
>
> Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
> ---
>  mm/gup.c | 100 +++++++++++++++++++++++++++++++------------------------
>  1 file changed, 56 insertions(+), 44 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index b755ceaac0f5..5af6a23285de 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -868,74 +868,86 @@ static long follow_page_pte_commit(struct vm_area_struct *vma,
>         return 0;
>  }
>
> -static long follow_page_pte(struct vm_area_struct *vma,
> -               unsigned long address, pmd_t *pmd, unsigned int flags,
> -               struct page **pages)
> +/*
> + * Resolve one present PTE to the page it maps. Returns no page and no error
> + * when the PTE cannot be followed but the caller may fault it in, and a
> + * negative errno when the caller must report the failure.
> + */
> +static long follow_one_pte(struct vm_area_struct *vma, unsigned long address,
> +               pte_t *ptep, pte_t pte, unsigned int flags, struct page **pagep)
>  {
> -       struct mm_struct *mm = vma->vm_mm;
> -       struct folio *folio;
>         struct page *page;
> -       spinlock_t *ptl;
> -       pte_t *ptep, pte;
> -       long ret;
>
> -       ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
> -       if (!ptep)
> -               return no_page_table(vma, flags, address);
> -       pte = ptep_get(ptep);
> +       *pagep = NULL;
> +
>         if (!pte_present(pte))
> -               goto no_page;
> +               return 0;
>         if (pte_protnone(pte) && !gup_can_follow_protnone(vma, flags))
> -               goto no_page;
> +               return 0;
>
>         page = vm_normal_page(vma, address, pte);
>
>         /*
>          * We only care about anon pages in can_follow_write_pte().
>          */
> -       if ((flags & FOLL_WRITE) &&
> -           !can_follow_write_pte(pte, page, vma, flags)) {
> -               ret = 0;
> -               goto out;
> -       }
> +       if ((flags & FOLL_WRITE) && !can_follow_write_pte(pte, page, vma, flags))
> +               return 0;

Before refactoring in the above case we would "goto out" and
no_page_table() would not be called even if pte_none(pte). Now I think
you will call it if pte_none(pte). I'm not sure if this does not
matter but this seems like a functional change.


>
>         if (unlikely(!page)) {
>                 if (flags & FOLL_DUMP) {
>                         /* Avoid special (like zero) pages in core dumps */
> -                       ret = -EFAULT;
> -                       goto out;
> -               }
> -
> -               if (is_zero_pfn(pte_pfn(pte))) {
> -                       page = pte_page(pte);
> -               } else {
> -                       ret = follow_pfn_pte(vma, address, ptep, flags);
> -                       goto out;
> +                       return -EFAULT;
>                 }
> +               if (!is_zero_pfn(pte_pfn(pte)))
> +                       return follow_pfn_pte(vma, address, ptep, flags);
> +               page = pte_page(pte);
>         }
> -       folio = page_folio(page);
>
> -       if (!pte_write(pte) && gup_must_unshare(vma, flags, page)) {
> -               ret = -EMLINK;
> -               goto out;
> -       }
> +       if (!pte_write(pte) && gup_must_unshare(vma, flags, page))
> +               return -EMLINK;
>
>         VM_WARN_ON_ONCE_PAGE((flags & FOLL_PIN) && PageAnon(page) &&
>                              !PageAnonExclusive(page), page);
>
> -       ret = follow_page_pte_commit(vma, address, folio, page, pte, flags,
> -                                    pages);
> -       if (ret)
> -               goto out;
> -       ret = 1;
> -out:
> +       *pagep = page;
> +       return 0;
> +}
> +
> +static long follow_page_pte(struct vm_area_struct *vma,
> +               unsigned long address, pmd_t *pmd, unsigned int flags,
> +               struct page **pages)
> +{
> +       struct mm_struct *mm = vma->vm_mm;
> +       bool need_no_page_table = false;
> +       struct page *page;
> +       spinlock_t *ptl;
> +       pte_t *ptep, pte;
> +       long ret;
> +
> +       ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
> +       if (!ptep)
> +               return no_page_table(vma, flags, address);
> +       pte = ptep_get(ptep);
> +
> +       ret = follow_one_pte(vma, address, ptep, pte, flags, &page);
> +       if (!ret && page) {
> +               ret = follow_page_pte_commit(vma, address, page_folio(page),
> +                                            page, pte, flags, pages);
> +               if (!ret)
> +                       ret = 1;
> +       } else if (!ret && pte_none(pte)) {
> +               /*
> +                * no_page_table() may look up the page cache, so it cannot run
> +                * under the PTE lock.
> +                */
> +               need_no_page_table = true;
> +       }
> +
>         pte_unmap_unlock(ptep, ptl);
> +
> +       if (need_no_page_table)
> +               return no_page_table(vma, flags, address);
>         return ret;
> -no_page:
> -       pte_unmap_unlock(ptep, ptl);
> -       if (!pte_none(pte))
> -               return 0;
> -       return no_page_table(vma, flags, address);
>  }
>
>  static long follow_pmd_mask(struct vm_area_struct *vma,
> --
> 2.55.0
>
>