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

[David Hildenbrand (Arm)]

On 11/3/26 19:24, Lorenzo Stoakes (ARM) wrote:
> On Tue, Nov 03, 2026 at 05:15:27PM +0100, David Hildenbrand (Arm) wrote:
>>
>>>
>>> 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).
>>
>>>
>>>
>>> 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:

Yeah, I know that we use it in some core places already properly, like in
GUP-fast, __handle_mm_fault() or our complicated __pte_offset_map().

> 
> 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.
> 

Ack.

> * 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.

Yes, there is this pmdp_get_lockless_start() magic ....

> 
> 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. :)

It's really a mess. I wonder if we can remove that somehow completely ...

arch/mips/Kconfig:      select GUP_GET_PXX_LOW_HIGH if CPU_MIPS32 &&
PHYS_ADDR_T_64BIT
arch/sh/Kconfig:        select GUP_GET_PXX_LOW_HIGH if X2TLB
arch/x86/Kconfig:       select GUP_GET_PXX_LOW_HIGH             if X86_PAE

-- 
Cheers,

David