Re: [PATCH v3 06/22] mm: Always use page table accessor functions

[Ryan Roberts <ryan.roberts@arm.com>]

On 13/11/2025 01:45, Samuel Holland wrote:
> Some platforms need to fix up the values when reading or writing page
> tables. Because of this, the accessors must always be used; it is not
> valid to simply dereference a pXX_t pointer.
> 
> Fix all of the instances of this pattern in generic code, mostly by
> applying the below coccinelle semantic patch, repeated for each page
> table level. Some additional fixes were applied manually, mostly to
> macros where type information is unavailable.
> 
> In a few places, a `pte_t *` or `pmd_t *` is actually a pointer to a PTE
> or PMDE value stored on the stack, not a pointer to a page table. In
> those cases, it is not appropriate to use the accessors, because the
> value is not globally visible, and any transformation from pXXp_get()
> has already been applied. Those places are marked by naming the pointer
> `ptentp` or `pmdvalp`, as opposed to `ptep` or `pmdp`.
> 
> @@
> pte_t *P;
> expression E;
> expression I;
> @@
> - P[I] = E
> + set_pte(P + I, E)
> 
> @@
> pte_t *P;
> expression E;
> @@
> (
> - WRITE_ONCE(*P, E)
> + set_pte(P, E)
> |
> - *P = E
> + set_pte(P, E)
> )

There should absolutely never be any instances of core code directly setting an
entry at any level. This *must* always go via the arch code helpers. Did you
find any instances of this? If so, I would consider these bugs and suggest
sending as a separate bugfix patch. Bad things could happen on arm64 because we
may need to break a contiguous mapping, which would not happen if the value is
set directly.

> 
> @@
> pte_t *P;
> expression I;
> @@
> (
>   &P[I]
> |
> - READ_ONCE(P[I])
> + ptep_get(P + I)
> |
> - P[I]
> + ptep_get(P + I)
> )
> 
> @@
> pte_t *P;
> @@
> (
> - READ_ONCE(*P)
> + ptep_get(P)
> |
> - *P
> + ptep_get(P)
> )

For reading the *PTE*, conversion over to ptep_get() should have already been
done (I did this a few years back when implementing support for arm64 contiguous
mappings). If you find any cases where direct dereference or READ_ONCE() is
being done in generic code for PTE, then that's a bug and should also be sent as
a separate patch.

FYI, my experience was that Coccinelle didn't find everything when I was
converting to ptep_get() - although it could have been that my Cochinelle skills
were not up to scratch! I ended up using an additional method where I did a
find/replace to convert "pte_t *" to "ptep_handle_t" and declared pte_handle_t
as a void* which causes a compiler error on dereference. Then in a few key
places I did a manual case from pte_handle_t to (pte_t *) and compiled allyesconfig.

I'm assuming the above Cocchinelle template was also used for pmd_t, pud_t,
p4d_t and pgd_t?

> 
> Additionally, the following semantic patch was used to convert PMD and
> PUD references inside struct vm_fault:
> 
> @@
> struct vm_fault vmf;
> @@
> (
> - *vmf.pmd
> + pmdp_get(vmf.pmd)
> |
> - *vmf.pud
> + pudp_get(vmf.pud)
> )
> 
> @@
> struct vm_fault *vmf;
> @@
> (
> - *vmf->pmd
> + pmdp_get(vmf->pmd)
> |
> - *vmf->pud
> + pudp_get(vmf->pud)
> )
> 
> 
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> This commit covers some of the same changes as an existing series from
> Anshuman Khandual[1]. Unlike that series, this commit is a purely
> mechanical conversion to demonstrate the RISC-V changes, so it does not
> insert local variables to avoid redundant calls to the accessors. A
> manual conversion like in that series could improve performance.
> 
> [1]: https://lore.kernel.org/linux-mm/20240917073117.1531207-1-anshuman.khandual@arm.com/

Hi,

I've just come across this patch and wanted to mention that we could also
benefit from this improved absraction for some features we are looking at for
arm64. As you mention, Anshuman had a go but hit some roadblocks.

The main issue is that the compiler was unable to optimize away the READ_ONCE()s
for the case where certain levels of the pgtable are folded. But it can optimize
the plain C dereferences. There were complaints the the generated code for arm
(32) and powerpc was significantly impacted due to having many more (redundant)
loads.

Were you able to solve this problem?

I think it stems from there really being 2 types of access; READ_ONCE() and
plain C dereference. The existing READ_ONCE() accesses require the
single-copy-atomic guarantees but the plain c dereferences don't actually need
that; they are usually just checking "is the value the same as a value I
previously read?", "is the pXd none?", etc. so it doesn't matter if they get torn.

It would be nice to have only a single flavour of accessor, but in that case it
obviously needs to be the strongest (READ_ONCE()). Perhaps we should settle for
2 flavours, and the compiler should then be able to continue to optimize out the
c dereferences. There are already 2 flavours for pte (which never gets folded);
ptep_get() and ptep_get_lockless(), but the semantics don't quite match.

Perhaps:

pXdp_get() -> READ_ONCE()
pXdp_get_weak() -> c dereference?

or pXdp_deref()? I'm not great with names...


The other problem is that there is nothing preventing new code from introducing
new direct dereferences or READ_ONCE()s. One possible solution to this is to use
an opaque handle type (similar to what I described above with pte_handle_t). But
that introduces a lot more code churn. I think that's a nice-to-have/incremental
improvement that could be done later.

> 
> Changes in v3:
>  - Rebased on top of torvalds/master (v6.18-rc5+)
> 
> Changes in v2:
>  - New patch for v2
> 
>  fs/dax.c                |  4 +-
>  fs/proc/task_mmu.c      | 27 +++++++------
>  fs/userfaultfd.c        |  6 +--
>  include/linux/huge_mm.h |  8 ++--
>  include/linux/mm.h      | 14 +++----
>  include/linux/pgtable.h | 42 +++++++++----------
>  mm/damon/vaddr.c        |  2 +-
>  mm/debug_vm_pgtable.c   |  4 +-
>  mm/filemap.c            |  6 +--
>  mm/gup.c                | 24 +++++------
>  mm/huge_memory.c        | 90 ++++++++++++++++++++---------------------
>  mm/hugetlb.c            | 10 ++---
>  mm/hugetlb_vmemmap.c    |  4 +-
>  mm/kasan/init.c         | 39 +++++++++---------
>  mm/kasan/shadow.c       | 12 +++---
>  mm/khugepaged.c         |  4 +-
>  mm/ksm.c                |  2 +-
>  mm/madvise.c            |  8 ++--
>  mm/memory-failure.c     | 14 +++----
>  mm/memory.c             | 76 +++++++++++++++++-----------------
>  mm/mempolicy.c          |  4 +-
>  mm/migrate.c            |  4 +-
>  mm/migrate_device.c     | 10 ++---
>  mm/mlock.c              |  6 +--
>  mm/mprotect.c           |  2 +-
>  mm/mremap.c             | 30 +++++++-------
>  mm/page_table_check.c   |  4 +-
>  mm/page_vma_mapped.c    |  6 +--
>  mm/pagewalk.c           | 14 +++----
>  mm/percpu.c             |  8 ++--
>  mm/pgalloc-track.h      |  8 ++--
>  mm/pgtable-generic.c    | 23 ++++++-----
>  mm/rmap.c               |  8 ++--
>  mm/sparse-vmemmap.c     |  8 ++--
>  mm/userfaultfd.c        | 10 ++---
>  mm/vmalloc.c            | 49 +++++++++++-----------
>  mm/vmscan.c             | 14 +++----
>  37 files changed, 304 insertions(+), 300 deletions(-)
> 

[...]

> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -1322,7 +1322,7 @@ static int write_protect_page(struct vm_area_struct *vma, struct folio *folio,
>  
>  		set_pte_at(mm, pvmw.address, pvmw.pte, entry);
>  	}
> -	*orig_pte = entry;
> +	set_pte(orig_pte, entry);

This is incorrect. orig_pte points to a location on stack. Please revert.

>  	err = 0;
>  
>  out_unlock:

[...]

> @@ -1116,7 +1116,7 @@ static int guard_install_set_pte(unsigned long addr, unsigned long next,
>  	unsigned long *nr_pages = (unsigned long *)walk->private;
>  
>  	/* Simply install a PTE marker, this causes segfault on access. */
> -	*ptep = make_pte_marker(PTE_MARKER_GUARD);
> +	set_pte(ptep, make_pte_marker(PTE_MARKER_GUARD));

I tried "fixing" this before. But it's correct as is. ptep is pointing to a
value on the stack. See [2].

https://lore.kernel.org/linux-mm/2308a4d0-273e-4cf8-9c9f-3008c42b6d18@arm.com/

>  	(*nr_pages)++;

[...]

It's all very difficult to review at the moment since it is so big. Any chance
of at least splitting by level in future versions?

Thanks,
Ryan