Re: [PATCH v2 10/15] mm/gup: add fast-GUP specific lockless PTE helpers

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

+cc literally everybody you failed to contact...!

Why on earth are you doing sending a series like this which changes core mm,
non-RFC and at v2 to boot, without bothering to Cc relevant people?

I am also very confused as to why you are sending an ostensible RISC-V series
then changing how core mm works to fit it, that's not how that works.

And if you're seeking to change core mm send it as an RFC!

On Thu, Jul 16, 2026 at 08:41:45PM +0800, Yunhui Cui wrote:
> fast-GUP snapshots a PTE without holding the PTL, pins the page, and
> then revalidates that the PTE did not change. Keep both reads under the
> same lockless PTE semantics by defaulting the fast-GUP revalidation to
> ptep_get_lockless().

Why?...

>
> Introduce fast-GUP specific PTE snapshot and revalidation helpers. The
> default implementation preserves the existing ptep_get_lockless()
> semantics, while allowing architectures to override the helpers when the
> public lockless getter provides extra semantics that fast-GUP does not
> consume.
>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>  include/linux/pgtable.h | 18 ++++++++++++++++++
>  mm/gup.c                |  6 ++++--
>  2 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index da14328093a86..62943fcbf7046 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -813,6 +813,24 @@ static inline pte_t ptep_get_lockless(pte_t *ptep)
>  }
>  #endif
>

Of course, no comments, helpful!

> +#ifndef gup_ptep_get_lockless
> +static inline pte_t gup_ptep_get_lockless(pte_t *ptep, pte_t *rawp)

rawp? This is terrible naming.

And 'gup_ptep_get_lockless()' is worse. What on earth is this function meant to
do? And why is 'gup lockless' considered different from lockless?

And you return the value twice, because of course you do. Why? Who knows.

> +{
> +	pte_t pte = ptep_get_lockless(ptep);
> +
> +	*rawp = pte;
> +
> +	return pte;
> +}
> +#endif
> +
> +#ifndef gup_ptep_revalidate
> +static inline bool gup_ptep_revalidate(pte_t *ptep, pte_t raw_pte)
> +{
> +	return pte_val(raw_pte) == pte_val(ptep_get_lockless(ptep));
> +}

Again this naming is utterly terrible and I'm at a loss as to why on earth
you're doing this?

We don't need to abstract equality checks?

> +#endif
> +
>  #ifndef pmdp_get_lockless
>  static inline pmd_t pmdp_get_lockless(pmd_t *pmdp)
>  {
> diff --git a/mm/gup.c b/mm/gup.c
> index 99902c15703b0..72fb147193e55 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2842,10 +2842,12 @@ static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
>  	if (!ptep)
>  		return 0;
>  	do {
> -		pte_t pte = ptep_get_lockless(ptep);
> +		pte_t raw_pte, pte;

What on earth is a 'raw' PTE?

>  		struct page *page;
>  		struct folio *folio;
>
> +		pte = gup_ptep_get_lockless(ptep, &raw_pte);
> +
>  		/*
>  		 * Always fallback to ordinary GUP on PROT_NONE-mapped pages:
>  		 * pte_access_permitted() better should reject these pages
> @@ -2871,7 +2873,7 @@ static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
>  			goto pte_unmap;
>
>  		if (unlikely(pmd_val(pmd) != pmd_val(pmdp_get_lockless(pmdp))) ||
> -		    unlikely(pte_val(pte) != pte_val(ptep_get_lockless(ptep)))) {
> +		    unlikely(!gup_ptep_revalidate(ptep, raw_pte))) {

Why do we need to refadctor a comparison between a local variable and a lockless
get?

I'm just so confused by what on earth this patch is meant to be or why it's here...

>  			gup_put_folio(folio, 1, flags);
>  			goto pte_unmap;
>  		}
> --
> 2.39.5
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

--
Cheers, Lorenzo