Re: [RFC PATCH 19/57] mm/collapse: install a PMD leaf as the terminal layer

[Jann Horn]

On Mon, Aug 17, 2026 at 12:46 AM Kiryl Shutsemau <kirill@xxxxxxxxxxxxx> wrote:
> Fill in the PMD install.  Under the pmd lock, with the pte ptl nested
> inside it: verify, detach the table with pmdp_collapse_flush(), deposit a
> fresh one and map the leaf.
>
> That is one atomic section, so no pmd_none() window ever exists: faults

What do you mean by "atomic" and "no pmd_none() window ever exists"?
Is that supposed to be with respect to a subset of readers?

While the PMD table spinlock is held, you clear the PMD entry
(pmdp_collapse_flush) and set it to a new value
(map_anon_folio_pmd_nopf). But codepaths that walk page tables don't
take that spinlock unless they already know they're in a THP case.

zap_pmd_range() does not lock the PMD table before checking
pmd_none(), and if that is true, it skips over the PMD. I think this
means that THP collapse can race with MADV_DONTNEED or zap_vma_range()
such that the zap wrongly has no effect?

> stay held down at pte level by the migration entries throughout.  It is

But you don't have a migration entry at the PMD level, right?

> what lets PMD collapse run under mmap_read like everything else here.
>
> Two things force that nesting, which is the one the tree already uses to

I'm lost, what is "the tree"?

> reinstall a table.  A racing zap of a frozen entry takes the pte ptl, so
> the verify has to hold it.  And the table must not come apart between
> verify and detach, which is the pmd lock's job.
>
> Nothing leaves the section early, aborts included.  An abort only
> restores PTEs and would need no pmd-level exclusion of its own, except
> that its pte pointer came from pte_offset_map_rw_nolock(), whose caller
> must establish that the pmd is stable.
>
> The deposited table is the freshly allocated one, never the table just
> detached.  A deposited table has to be quiescent, because whoever
> withdraws it frees it immediately with nothing to hold a lockless walker
> off first, and a table that has never been reachable is quiescent by
> construction.
>
> The detached one is not: GUP-fast and RCU pte walks that read the old PMD
> may still be inside it, and on broadcast-TLBI architectures the flush
> expels nobody.  Quiescing it would need an IPI, which has nowhere to go
> here -- outside the pmd lock it opens the pmd_none() window this design
> does not have, inside it is a broadcast under a spinlock.  So the
> detached table goes to pte_free_defer(), which holds the free until those
> walkers finish.  One transient table page per PMD collapse is the cost.
>
> No anon_vma_lock_write() is taken, unlike the mechanism being replaced:
>
>  - rmap walks on the sources are unreachable, their refcounts frozen and
>    their folio locks held from freeze to putback;

To be clear, we can concurrently rmap-walk into the PMD, right?
Because we might be looking at a different folio which was created in
the parent process or something like that? And so a
page_vma_mapped_walk() without PVMW_SYNC might look at stale PTEs?
(Which may or may not be fine, but would not be what this commit
message claims.)

>  - non-rmap pte walkers see migration entries;
>  - pmd-level observers see either the old table or the leaf, never an
>    intermediate;

(or pmd_none(), see above)

> +       old_pmd = pmdp_collapse_flush(vma, cand->addr, pmd);
> +       old_table = pmd_pgtable(old_pmd);
[...]
> +       pgtable_trans_huge_deposit(mm, pmd, cand->deposit);
> +       map_anon_folio_pmd_nopf(cand->new_folio, pmd, vma, cand->addr);
[...]
> +out_unlock:
> +       if (pte_ptl != pmd_ptl)
> +               spin_unlock(pte_ptl);
> +       pte_unmap(pte);
> +       spin_unlock(pmd_ptl);
> +
> +       /* The deposit balanced the detached table, so the count is already right */
> +       if (old_table)
> +               pte_free_defer(mm, old_table);

What does old_table contain at this point - migration entries? I
wonder if we should zero all the PTEs inside it before dropping its
spinlock and scheduling its freeing, so that a concurrent
pte_offset_map_rw_nolock()+spin_lock() wouldn't see dangling migration
entries. (I'm not sure if we actually have a codepath that does that,
but even if not, this seems like it might cause problems later.)