mm/gup: cleanup pgtable entry accessors

[PATCH v2] mm/gup: cleanup pgtable entry accessors

Posted by Alexander Gordeev 1 month, 3 weeks ago

PMD and PUD entries revalidation has the same semantics as PTE entry
revalidation. Convert the remaining direct entry dereferences to the
corresponding accessors.

The PTE validation in gup_fast_pte_range() is inconsistent with the
prior value acquisition in the sense that it drops the lockless
access semantics.

Use the lockless accessor not only for the PTE, but also for the PMD
validation, which is likewise inconsistent with the prior value
acquisition in gup_fast_pmd_range().

Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
 mm/gup.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 8e7dc2c6ee73..236450feea9a 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2865,8 +2865,8 @@ static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
 		if (!folio)
 			goto pte_unmap;
 
-		if (unlikely(pmd_val(pmd) != pmd_val(*pmdp)) ||
-		    unlikely(pte_val(pte) != pte_val(ptep_get(ptep)))) {
+		if (unlikely(pmd_val(pmd) != pmd_val(pmdp_get_lockless(pmdp))) ||
+		    unlikely(pte_val(pte) != pte_val(ptep_get_lockless(ptep)))) {
 			gup_put_folio(folio, 1, flags);
 			goto pte_unmap;
 		}
@@ -2942,7 +2942,7 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
 	if (!folio)
 		return 0;
 
-	if (unlikely(pmd_val(orig) != pmd_val(*pmdp))) {
+	if (unlikely(pmd_val(orig) != pmd_val(pmdp_get_lockless(pmdp)))) {
 		gup_put_folio(folio, refs, flags);
 		return 0;
 	}
@@ -2985,7 +2985,7 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
 	if (!folio)
 		return 0;
 
-	if (unlikely(pud_val(orig) != pud_val(*pudp))) {
+	if (unlikely(pud_val(orig) != pud_val(pudp_get(pudp)))) {
 		gup_put_folio(folio, refs, flags);
 		return 0;
 	}
-- 
2.51.0

Re: [PATCH v2] mm/gup: cleanup pgtable entry accessors

Posted by David Hildenbrand (Arm) 1 month, 2 weeks ago

On 4/21/26 07:17, Alexander Gordeev wrote:
> PMD and PUD entries revalidation has the same semantics as PTE entry
> revalidation. Convert the remaining direct entry dereferences to the
> corresponding accessors.
> 
> The PTE validation in gup_fast_pte_range() is inconsistent with the
> prior value acquisition in the sense that it drops the lockless
> access semantics.
> 
> Use the lockless accessor not only for the PTE, but also for the PMD
> validation, which is likewise inconsistent with the prior value
> acquisition in gup_fast_pmd_range().
> 
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
>  mm/gup.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 8e7dc2c6ee73..236450feea9a 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2865,8 +2865,8 @@ static int gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr,
>  		if (!folio)
>  			goto pte_unmap;
>  
> -		if (unlikely(pmd_val(pmd) != pmd_val(*pmdp)) ||
> -		    unlikely(pte_val(pte) != pte_val(ptep_get(ptep)))) {
> +		if (unlikely(pmd_val(pmd) != pmd_val(pmdp_get_lockless(pmdp))) ||
> +		    unlikely(pte_val(pte) != pte_val(ptep_get_lockless(ptep)))) {
>  			gup_put_folio(folio, 1, flags);
>  			goto pte_unmap;
>  		}
> @@ -2942,7 +2942,7 @@ static int gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp, unsigned long addr,
>  	if (!folio)
>  		return 0;
>  
> -	if (unlikely(pmd_val(orig) != pmd_val(*pmdp))) {
> +	if (unlikely(pmd_val(orig) != pmd_val(pmdp_get_lockless(pmdp)))) {
>  		gup_put_folio(folio, refs, flags);
>  		return 0;
>  	}
> @@ -2985,7 +2985,7 @@ static int gup_fast_pud_leaf(pud_t orig, pud_t *pudp, unsigned long addr,
>  	if (!folio)
>  		return 0;
>  
> -	if (unlikely(pud_val(orig) != pud_val(*pudp))) {
> +	if (unlikely(pud_val(orig) != pud_val(pudp_get(pudp)))) {
>  		gup_put_folio(folio, refs, flags);
>  		return 0;
>  	}

In general, LGTM.

I do wonder, though, if we want to switch to pte_same/pmd_same/pud_same while at it?

-- 
Cheers,

David

Re: [PATCH v2] mm/gup: cleanup pgtable entry accessors

Posted by Alexander Gordeev 1 month, 2 weeks ago

On Tue, Apr 28, 2026 at 09:45:59PM +0200, David Hildenbrand (Arm) wrote:
> In general, LGTM.
> 
> I do wonder, though, if we want to switch to pte_same/pmd_same/pud_same while at it?

I thought about that too, but decided to keep it focused on direct
dereferences (as it fixes a real issue with the pte kasan sanitizer).

I can send updated patch or a follow-up patch - as you prefer. But
if you like the follow-up I would suggest to un-squash the original
series to get it in three logical steps: 1 - pXdp_get() fix, 2 -
lockless access fix, 3 - pXd_same() cleanup.

> -- 
> Cheers,
> 
> David

Thanks!

Re: [PATCH v2] mm/gup: cleanup pgtable entry accessors

Posted by David Hildenbrand (Arm) 1 month, 2 weeks ago

On 4/29/26 07:17, Alexander Gordeev wrote:
> On Tue, Apr 28, 2026 at 09:45:59PM +0200, David Hildenbrand (Arm) wrote:
>> In general, LGTM.
>>
>> I do wonder, though, if we want to switch to pte_same/pmd_same/pud_same while at it?
> 
> I thought about that too, but decided to keep it focused on direct
> dereferences (as it fixes a real issue with the pte kasan sanitizer).
> 
> I can send updated patch or a follow-up patch - as you prefer. But
> if you like the follow-up I would suggest to un-squash the original
> series to get it in three logical steps: 1 - pXdp_get() fix, 2 -
> lockless access fix, 3 - pXd_same() cleanup.

I'd prefer to keep 1 and 2 squashed. I don't see a good reason to temporarily do
the wrong thing :)

A follow-up patch works for me, thanks!

Acked-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David