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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fa89fa8b-f5dd-4e01-a079-9b414efac30d@arm.com>
Date: Mon, 6 Oct 2025 14:10:14 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: linux-perf-users@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf/events: replace READ_ONCE() with standard page table
 accessors

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,