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

[David Hildenbrand (Arm)]

On 11/3/26 21:27, Lorenzo Stoakes (ARM) wrote:
> On Tue, Nov 03, 2026 at 08:51:38PM +0100, David Hildenbrand (Arm) wrote:
>> On 11/3/26 19:24, Lorenzo Stoakes (ARM) wrote:
>>> 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().
> 
> Yup, but also inconsistently, basically mostly interchangeably with
> pmdp_get(). Rarely is this value actually used.
> 
>>> * 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 ....
> 
> One of those things that, the more you look into it, the more stuff you find...
> 
>> 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
> 
> Yeah that'd be ideal.
> 
> But actually could we not just simplify and allow these arches to be a little
> bit slower (who cares for museum pieces)?
> 
> The only user of the start/end magic is __pte_offset_map(). And it does:
> 
> 	irqflags = pmdp_get_lockless_start();
> 	pmdval = pmdp_get_lockless(pmd);
> 	pmdp_get_lockless_end(irqflags);
> 
> So that could just get folded into pmdp_get_lockless() for these arches.
> 
> That would mean the places that don't require IRQs to be disabled (optimstic
> checks) would now be needlessly enabling/disabling, but hey - use a system that
> doesn't live in a museum :)

It would be interesting to know the performance impact. IOW, whether this is
just a slight slowdown or whether it makes these installations unusable.

It looks like an interesting thing.

> 
> That way we could safely convert every non-PMD lock holding caller to
> pmdp_get_lockless() without needing another pmdp_get*() variant + fix confusion
> about whether the start/end magic is required.
> 
> And this would work even in places where the IRQs are already disabled as it's
> fine with nesting.
> 
> So you'd just update thusly:
> 
> static inline pmd_t pmdp_get_lockless(pmd_t *pmdp)
> {
> 	pmd_t pmd;
> +	unsigned long flags;
> 
> +	local_irq_save(flags);
> 	do {
> 		pmd.pmd_low = pmdp->pmd_low;
> 		smp_rmb();
> 		pmd.pmd_high = pmdp->pmd_high;
> 		smp_rmb();
> 	} while (unlikely(pmd.pmd_low != pmdp->pmd_low));
> -
> +	local_irq_restore(flags);
> 	return pmd;
> }
> 
> ...
> 
> pte_t *__pte_offset_map(pmd_t *pmd, unsigned long addr, pmd_t *pmdvalp)
> {
> -	unsigned long irqflags;
> 	pmd_t pmdval;
> 
> 	rcu_read_lock();
> -	irqflags = pmdp_get_lockless_start();
> 	pmdval = pmdp_get_lockless(pmd);
> -	pmdp_get_lockless_end(irqflags);
> 
> 	...
> }
> 
> What do you think?

Sounds reasonable. I guess we should be able to disable IRQs on all callsites.


I am quite surprised that x86 shouldn't have an instruction to read/write PTEs
atomically.

I think if we require MMX we could just do atomic read+write, and let x86
implement pmdp_get_lockless() / pmdp_get() itself. But MMX seems to be a bit
younger than PAE. Apparently there is a hacky way to read through CMPXCHG8B.

Doesn't sound like a good use of our time ... but removing that handling
entirely would be very nice.

Simplifying is nice as well.

-- 
Cheers,

David