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

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?

--
Cheers, Lorenzo