Re: [PATCH 2/3] mm/pagewalk: let folio_walk_start() run under the per-VMA lock

[Lorenzo Stoakes <ljs@kernel.org>]

On Thu, Jun 25, 2026 at 07:20:22AM -0400, Rik van Riel wrote:
> On Thu, 2026-06-25 at 08:34 +0100, Lorenzo Stoakes wrote:
> > Rik, it really would have helped if you'd replied to review :)
> >
> > On Wed, Jun 24, 2026 at 09:50:52PM -0400, Rik van Riel wrote:
> > > folio_walk_start() asserts the mmap lock is held.  For callers that
> > > only
> > > need to read a single, already-present page, the mmap lock is a
> > > heavy and
> > > often badly contended hammer.  Such a caller can instead hold the
> > > per-VMA
> > > lock, which keeps the VMA itself stable.
> >
> > <newline>
> >
> > > The per-VMA lock does not, however, keep the page tables walked
> > > below that
> > > VMA from being freed.  A concurrent munmap() or THP collapse of an
> > > adjacent region in the same mm can free a shared upper-level table,
> > > and
> >
> > Yeah I need to update the documentation on this at
> > https://docs.kernel.org/mm/process_addrs.html it's more subtle than
> > written
> > there.

Actually not so sure I do now :) I mean maybe I need to be explicit about
the need for PTL's too. I guess 'other than the PTLs your page table walker
will take' is kinda implicit there.

> >
> > Firstly you're wrong about munmap() - it acquires the VMA lock of the
> > VMAs freed
> > in the range and will only remove an upper level table if the entire
> > range is
> > spanned.
> >
> > And that's the only way higher level tables can be removed.
> >
> > PTE page tables can be removed via MADV_DONTNEED, but that a.
> > acquires the VMA
> > lock and b. frees the PTE page table under RCU.
> >
> > A THP collapse can happen concurrently, but PTEs are freed under RCU
> > so you
> > don't need to do this GUP fast imitating stuff.
> >
> > > THP collapse (collapse_huge_page() -> retract_page_tables()) frees
> > > page
> > > tables of VMAs whose lock it does not hold.  Page table freeing
> >
> > retract_page_tables() -> pte_free_defer() -> RCU
> > try_collapse_pte_mapped_thp() -> pte_free_defer() -> RCU
>
> One issue here is that while we can safely read
> the old page table under the RCU read lock, in
> the middle of a THP collapse there is no guarantee
> that the old page table points at the process's
> current memory.

See below.

>
> Khugepaged could fix this in one of two ways:
> - zap all readers with an IPI, and use that as
>   synchronization
> - make sure the old page table's PTEs point at
>   the individual pages inside the new PMD
>
> Right now khugepaged does the first.

This is confusing too. Khugepaged _could_ fix this one of two ways, right now it
_does_ the first?

You mean pmdp_get_lockless_sync() calls tlb_remove_table_sync_one()?

But that's for GUP fast right, not folio_walk_start/end()?

>
> Relying only on the RCU read lock to read the
> page table could result in us seeing old page
> table contents, that no longer point at the
> current process memory.
>
> Unless I'm missing something...

I think so! Otherwise we'd already have very broken page table walkers
right?

So:

pte_t *pte_offset_map_lock(struct mm_struct *mm, pmd_t *pmd,
			   unsigned long addr, spinlock_t **ptlp)
{
	spinlock_t *ptl;
	pmd_t pmdval;
	pte_t *pte;
again:
	pte = __pte_offset_map(pmd, addr, &pmdval);
	if (unlikely(!pte))
		return pte;
	ptl = pte_lockptr(mm, &pmdval);
	spin_lock(ptl);
	if (likely(pmd_same(pmdval, pmdp_get_lockless(pmd)))) {
		*ptlp = ptl;
		return pte;
	}
	pte_unmap_unlock(pte, ptl);
	goto again;
}

__pte_offset_map() takes the RCU lock, then locklessly retrieves a pointer
to the PTE page table.

As mentioned above, khugepaged retraction causing PTE page table freeing is
also under RCU, so this is safe:

	retract_page_tables() -> pte_free_defer() -> RCU
	try_collapse_pte_mapped_thp() -> pte_free_defer() -> RCU

And on page table retraction, importantly, the PMD entry is cleared _with
the PTE PTL held_, e.g. in retract_page_tables():

		ptl = pte_lockptr(mm, pmd);
		if (ptl != pml)
			spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
		...
		if (likely(file_backed_vma_is_retractable(vma))) {
			pgt_pmd = pmdp_collapse_flush(vma, addr, pmd);
			...
		}

		if (ptl != pml)
			spin_unlock(ptl);

For anon, we take the mmap write lock/VMA write lock anyway.

And going back to pte_offset_map_lock() - we _recheck the PMD under the
PTL_.

So if the PMD was since cleared, we don't get anything stale. If it wasn't,
then we hold the PTE PTL and exclude khugepaged from changing things under
us.

Thus folio_walk_start(), which holds the PTE PTL, is perfectly fine, and
folio_walk_end() marks the point at which things are no longer safe (but
then we're done with the folio).

So again I don't think there's any problem here, actually.

Thanks, Lorenzo