Re: [PATCH v3] mm/memory: use pmdp_get_lockless() for the pmd read in do_fault_around()

[Lorenzo Stoakes (ARM)]

On Tue, Nov 03, 2026 at 05:15:27PM +0100, David Hildenbrand (Arm) wrote:
>
> >> +++ b/mm/memory.c
> >> @@ -6017,7 +6017,8 @@ static vm_fault_t do_fault_around(struct vm_fault *vmf)
> >>  	to_pte = min3(from_pte + nr_pages, (pgoff_t)PTRS_PER_PTE,
> >>  		      pte_off + vma_pages(vmf->vma) - vma_off) - 1;
> >>
> >> -	if (pmd_none(*vmf->pmd)) {
> >
> > This pattern is definitely bad I think, we have it in many other places so
> > should probably convert those to pmdp_get() at least?
>
> Usama is working on this as part of separating HW and SW PMDs (he only sent out
> the PTE part so far).
>
> >
> >> +	/* pmd_install() rechecks pmd_none() under the pmd lock. */
> >> +	if (pmd_none(pmdp_get_lockless(vmf->pmd))) {
> >
> > It definitely seems right to use at least pmdp_get() here, but I wonder
> > about the need for pmdp_get_lockless() here.
>
> Whenever we're not holding the page table lock we should be using
> pmdp_get_lockless().
>
> Even if they are semantically equivalent for most cases, it serves as a good
> reminder that there are no guarantees.
>
> And that you have to look again under PT lock.
>
> >
> > pmd_none() only checks the lower word, so a torn 64-bit word on 32-bit
> > (i.e. the PAE style cases we are worried about) makes no difference:
> >
> > From x86 pmd_none():
> >
> > 	/* Only check low word on 32-bit platforms, since it might be
> > 	   out of sync with upper half. */
> >
> > And anyway I guess there is no valid PMD in such systems that would have
> > the lower word cleared and upper word not cleared.
> >
>
> I don't understand the discussion really.
>
> No PT lock -> use the lockless variant.
>
> Until we can remove the lockless variant.
>
> Are we consistent with that? Likely not.
>
> But that's all independent of this patch here.

It turns out all of this is a complicated mess, I think I've nerd-sniped myself
into sending a series at some point about this...

It turns out we're as wildly inconsistent on pmdp_get_lockless() as we are on
pmdp_get(), so TL;DR my main concern is moot so:

Acked-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>

But for the non-TL;DR... (optional reading, will probably send something):

* Agreed that the semantics matter - and the 'use pXXp_get_lockless() if no
  locks, otherwise pXXp_get()' is preferable.

* But it turns out it's all a giant mess (of course)... for
  CONFIG_GUP_GET_PXX_LOW_HIGH arches you really actually need to also do:

pmdp_get_lockless_start();
pmd = pmdp_get_lockless(...);
pmdp_get_lockless_end();

And that's because, as Hugh points out in commit 146b42e07494 ("mm/pgtable: add
PAE safety to __pte_offset_map()")), you need IRQs disabled for these arches for
it to be safe.

The explanation is quite good:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=146b42e07494e45f7c7bcf2cbf7afd1424afd78e

And then of course there's also pmdp_get_lockless_sync() used by THP collapse
which is some kind of 'true' lockless usage that needs to synchronise with TLB
invalidation against GUP-fast and ugh.

* So I think what matters is whether you actually NEED the value un-torn or
  whether it's an optimistic check that you can live with (rechecked under lock
  as needed).

* So I wonder if we shouldn't just have e.g. pmdp_get_lockless_optimistic() or
  something like that which is just a pmdp_get() in all cases with correct kdoc
  description etc.

* Then we can convert ALL lockless users to using a lockless form without having
  to do the IRQ stuff. Including a ton of the existing lockless users.

* I am not sure why I did this in bullets. :)

--
Cheers, Lorenzo