[PATCH] perf/events: replace READ_ONCE() with standard page table accessors

[PATCH] perf/events: replace READ_ONCE() with standard page table accessors

[Anshuman Khandual]

Replace READ_ONCE() with standard page table accessors i.e pxdp_get() which
anyways default into READ_ONCE() in cases where platform does not override.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: linux-perf-users@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 kernel/events/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 820127536e62..952ba4e3d881 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8110,7 +8110,7 @@ static u64 perf_get_pgtable_size(struct mm_struct *mm, unsigned long addr)
 	pte_t *ptep, pte;
 
 	pgdp = pgd_offset(mm, addr);
-	pgd = READ_ONCE(*pgdp);
+	pgd = pgdp_get(pgdp);
 	if (pgd_none(pgd))
 		return 0;
 
@@ -8118,7 +8118,7 @@ static u64 perf_get_pgtable_size(struct mm_struct *mm, unsigned long addr)
 		return pgd_leaf_size(pgd);
 
 	p4dp = p4d_offset_lockless(pgdp, pgd, addr);
-	p4d = READ_ONCE(*p4dp);
+	p4d = p4dp_get(p4dp);
 	if (!p4d_present(p4d))
 		return 0;
 
@@ -8126,7 +8126,7 @@ static u64 perf_get_pgtable_size(struct mm_struct *mm, unsigned long addr)
 		return p4d_leaf_size(p4d);
 
 	pudp = pud_offset_lockless(p4dp, p4d, addr);
-	pud = READ_ONCE(*pudp);
+	pud = pudp_get(pudp);
 	if (!pud_present(pud))
 		return 0;
 
-- 
2.30.2

Re: [PATCH] perf/events: replace READ_ONCE() with standard page table accessors

[Peter Zijlstra]

On Mon, Oct 06, 2025 at 05:26:22AM +0100, Anshuman Khandual wrote:
> Replace READ_ONCE() with standard page table accessors i.e pxdp_get() which
> anyways default into READ_ONCE() in cases where platform does not override.

Note that these accessors are more recent than the code in question.

Furthermore, I can only find two pointless overrides and suggest the
below.

This then raises the question of the purpose of these accessors; why
isn't a plain READ_ONCE() preferred if that is really all it is?

[ this skips over the ptep_get() issue, which does seem a little more
involved -- but also has some definite want of cleanups ]

---
diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 86378eec7757..c4c23dc4dc77 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -150,8 +150,6 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
 
 extern pgd_t swapper_pg_dir[PTRS_PER_PGD];
 
-#define pgdp_get(pgpd)		READ_ONCE(*pgdp)
-
 #define pud_page(pud)		pmd_page(__pmd(pud_val(pud)))
 #define pud_write(pud)		pmd_write(__pmd(pud_val(pud)))
 
diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
index bd128696e96d..8184e8c44db6 100644
--- a/arch/loongarch/include/asm/pgtable.h
+++ b/arch/loongarch/include/asm/pgtable.h
@@ -107,7 +107,6 @@ extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)];
 #define KFENCE_AREA_END		(KFENCE_AREA_START + KFENCE_AREA_SIZE - 1)
 
 #define ptep_get(ptep) READ_ONCE(*(ptep))
-#define pmdp_get(pmdp) READ_ONCE(*(pmdp))
 
 #define pte_ERROR(e) \
 	pr_err("%s:%d: bad pte %016lx.\n", __FILE__, __LINE__, pte_val(e))
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index 25a7257052ff..bc76db9974e4 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -341,33 +341,25 @@ static inline pte_t ptep_get(pte_t *ptep)
 }
 #endif
 
-#ifndef pmdp_get
 static inline pmd_t pmdp_get(pmd_t *pmdp)
 {
 	return READ_ONCE(*pmdp);
 }
-#endif
 
-#ifndef pudp_get
 static inline pud_t pudp_get(pud_t *pudp)
 {
 	return READ_ONCE(*pudp);
 }
-#endif
 
-#ifndef p4dp_get
 static inline p4d_t p4dp_get(p4d_t *p4dp)
 {
 	return READ_ONCE(*p4dp);
 }
-#endif
 
-#ifndef pgdp_get
 static inline pgd_t pgdp_get(pgd_t *pgdp)
 {
 	return READ_ONCE(*pgdp);
 }
-#endif
 
 #ifndef __HAVE_ARCH_PTEP_TEST_AND_CLEAR_YOUNG
 static inline int ptep_test_and_clear_young(struct vm_area_struct *vma,

Re: [PATCH] perf/events: replace READ_ONCE() with standard page table accessors

[Anshuman Khandual]

On 06/10/25 1:52 PM, Peter Zijlstra wrote:
> On Mon, Oct 06, 2025 at 05:26:22AM +0100, Anshuman Khandual wrote:
>> Replace READ_ONCE() with standard page table accessors i.e pxdp_get() which
>> anyways default into READ_ONCE() in cases where platform does not override.
> 
> Note that these accessors are more recent than the code in question.

Sure.
> 
> Furthermore, I can only find two pointless overrides and suggest the
> below.
> 
> This then raises the question of the purpose of these accessors; why
> isn't a plain READ_ONCE() preferred if that is really all it is?

These accessors provide platforms the opportunity to override and have different
implementations other than READ_ONCE() when required.

Currently there is a mix of READ_ONCE() and pxdp_get() accessors across generic
MM and else where. Hence converting the READ_ONCE() on page table pointers into
pxdp_get() helpers thus improving consistency, as well as it offers flexibility
to platforms to override the implementation later when required.

Besides these static inline helper replacements does not add any more cost.

> > [ this skips over the ptep_get() issue, which does seem a little more
> involved -- but also has some definite want of cleanups ]
> 
> ---
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index 86378eec7757..c4c23dc4dc77 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -150,8 +150,6 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>  
>  extern pgd_t swapper_pg_dir[PTRS_PER_PGD];
>  
> -#define pgdp_get(pgpd)		READ_ONCE(*pgdp)
> -
>  #define pud_page(pud)		pmd_page(__pmd(pud_val(pud)))
>  #define pud_write(pud)		pmd_write(__pmd(pud_val(pud)))
>  
> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
> index bd128696e96d..8184e8c44db6 100644
> --- a/arch/loongarch/include/asm/pgtable.h
> +++ b/arch/loongarch/include/asm/pgtable.h
> @@ -107,7 +107,6 @@ extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)];
>  #define KFENCE_AREA_END		(KFENCE_AREA_START + KFENCE_AREA_SIZE - 1)
>  
>  #define ptep_get(ptep) READ_ONCE(*(ptep))
> -#define pmdp_get(pmdp) READ_ONCE(*(pmdp))
>  
>  #define pte_ERROR(e) \
>  	pr_err("%s:%d: bad pte %016lx.\n", __FILE__, __LINE__, pte_val(e))
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index 25a7257052ff..bc76db9974e4 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -341,33 +341,25 @@ static inline pte_t ptep_get(pte_t *ptep)
>  }
>  #endif
>  
> -#ifndef pmdp_get
>  static inline pmd_t pmdp_get(pmd_t *pmdp)
>  {
>  	return READ_ONCE(*pmdp);
>  }
> -#endif
>  
> -#ifndef pudp_get
>  static inline pud_t pudp_get(pud_t *pudp)
>  {
>  	return READ_ONCE(*pudp);
>  }
> -#endif
>  
> -#ifndef p4dp_get
>  static inline p4d_t p4dp_get(p4d_t *p4dp)
>  {
>  	return READ_ONCE(*p4dp);
>  }
> -#endif
>  
> -#ifndef pgdp_get
>  static inline pgd_t pgdp_get(pgd_t *pgdp)
>  {
>  	return READ_ONCE(*pgdp);
>  }
> -#endif
>  
>  #ifndef __HAVE_ARCH_PTEP_TEST_AND_CLEAR_YOUNG
>  static inline int ptep_test_and_clear_young(struct vm_area_struct *vma,

Re: [PATCH] perf/events: replace READ_ONCE() with standard page table accessors

[Peter Zijlstra]

On Mon, Oct 06, 2025 at 02:10:14PM +0530, Anshuman Khandual wrote:

> These accessors provide platforms the opportunity to override and have different
> implementations other than READ_ONCE() when required.

When would this be required? Traditionally the only case that was weird
was Alpha, since it needed the dereference barrier, but since we folded
that into READ_ONCE(), that has all gone away.

[ also, historically we trusted GCC to emit singe loads for word sized
accesses -- something we've walked back from as C compilers have started
to be more agressive ]

Anyway, READ_ONCE() is the only sane way to dereference a page-table
pointers, doubly so in the face of lockless access.

There should be no need for architectures to be 'funny' here.