Re: [RESEND v7 11/29] mm: split PMD swap entries into PTE swap entries

[David Hildenbrand (Arm)]

On 9/14/26 14:28, Usama Arif wrote:
> Once a PMD can hold a swap entry, everything that splits a PMD - mprotect()
> or munmap() over part of the range, MADV_FREE, a pagewalk with no PMD
> handler - has to be able to split that entry too, or the callers that rely
> on split_huge_pmd() to hand them a PTE table would find the PMD unchanged.
> 
> No reference counting is needed: a swap entry pins no folio, and swap_map
> is already one per slot, so the PTEs simply take over what the PMD held.
> 
> The migration-only entry point cannot reach the new branch, because
> page_vma_mapped_walk() never hands back a swap PMD for the folio being
> migrated. Warn if that ever changes, and force the regular split anyway,
> since the branch leaves folio and page uninitialised.
> 
> Test the pre-split old_pmd rather than re-reading *pmd in the trailing
> folio_remove_rmap_pmd() gate, so every entry-type test in the function
> interrogates the same snapshot. That part is cosmetic: pmdp_invalidate()
> leaves the PMD present as far as software is concerned.
> 
> Signed-off-by: Usama Arif <usama.arif@xxxxxxxxx>
> ---
>  mm/huge_memory.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 873887aed0bc2..0e347a545588c 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3304,6 +3304,21 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  			folio_add_anon_rmap_ptes(folio, page, HPAGE_PMD_NR,
>  						 vma, haddr, rmap_flags);
>  		}
> +	} else if (pmd_is_swap_entry(*pmd)) {
> +		/*
> +		 * A PMD swap entry has no page, so it cannot be turned into
> +		 * PTE migration entries.  page_vma_mapped_walk() never hands
> +		 * one back for the folio being migrated, so this should not
> +		 * happen; warn, but also force the regular split so that a
> +		 * broken invariant cannot make the code below dereference the
> +		 * uninitialised folio and page.

I disagree with the force (and the comment). We cannot make each and every
assertion that never happens (unless someone messes up real bad and would find
this during early testing) have recovery code.

The real bug would be calling split_pmd_to_migration_entries() with something
unexpected. See my reply to #10 where we bail out earlier


> +		 */
> +		VM_WARN_ON_ONCE(use_migration_entries);
> +		use_migration_entries = false;

Can we just have on the beginning of the function a check that
use_migration_entries is only ever set on present PMDs or device-private entries.

> +		old_pmd = *pmd;
> +		soft_dirty = pmd_swp_soft_dirty(old_pmd);
> +		uffd_wp = pmd_swp_uffd(old_pmd);
> +		anon_exclusive = pmd_swp_exclusive(old_pmd);
>  	} else {
>  		/*
>  		 * Up to this point the pmd is present and huge and userland has
> @@ -3440,6 +3455,25 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  			VM_WARN_ON(!pte_none(ptep_get(pte + i)));
>  			set_pte_at(mm, addr, pte + i, entry);
>  		}
> +	} else if (pmd_is_swap_entry(old_pmd)) {
> +		const softleaf_t old_entry = softleaf_from_pmd(old_pmd);
> +		pte_t pte_swp_entry;
> +		swp_entry_t entry;
> +
> +		for (i = 0, addr = haddr; i < HPAGE_PMD_NR;
> +		     i++, addr += PAGE_SIZE) {

Just squeeze it into one line like the other instances.

> +			entry = swp_entry(swp_type(old_entry),
> +					  swp_offset(old_entry) + i);

Didn't we have a helper to advance by a delta? Ah, yes, pte_move_swp_offset().

I guess one could construct the initial pte and then advance one by one through
pte_move_swp_offset(). Won't remove a lot of code, though, so just a thought.

Apart from that LGTM.

-- 
Cheers,

David