Re: [RESEND v7 12/29] mm: handle PMD swap entries in fork path

[David Hildenbrand (Arm)]

On 9/14/26 14:28, Usama Arif wrote:
> copy_huge_pmd() only knows about migration and device-private PMDs, so a
> PMD swap entry would fall through to the present-PMD path and fork() would
> duplicate it without taking a reference on the slots it points at.
> 
> Copy it the way copy_nonpresent_pte() copies a PTE swap entry: duplicate
> the swap references, clear the exclusive marker on the source, put the
> destination mm on mmlist, and account the child's slots to MM_SWAPENTS.
> 
> Duplicating HPAGE_PMD_NR slots one at a time would be wasteful, so give
> swap_dup_entry_direct() an nr argument and rename it accordingly. Unlike
> the put side it hands nr straight to the per-cluster helper, so the range
> has to sit inside one cluster - which it does, since SWAPFILE_CLUSTER ==
> HPAGE_PMD_NR under CONFIG_THP_SWAP and a PMD-order folio's slots are only
> ever allocated at a cluster head. Reject a crossing range with -EINVAL so a
> future caller cannot walk off the end of the swap table.

You have a lot of patches, put you could (should? :) ) consider moving the
swapfile.c stuff into a separate patch, such that we can more reliably catch
swap maintainers attention (and shrink this patch here).

> 
> The GFP_ATOMIC extend-table allocation inside the dup can fail;
> copy_huge_pmd() then drops both PMD locks and retries once with
> GFP_KERNEL. Bound it to one retry, because swap_retry_table_alloc() also
> returns 0 when it decides the table is not needed. Normalise any remaining
> failure to -ENOMEM: copy_pmd_range() treats every other error as "not a
> huge PMD" and would then reach pmd_none_or_clear_bad(), clearing the source
> PMD and leaking its swap slots.
> 
> Signed-off-by: Usama Arif <usama.arif@xxxxxxxxx>

[...]

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 0e347a545588c..6dfe8ef6dd371 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1894,7 +1894,7 @@ bool touch_pmd(struct vm_area_struct *vma, unsigned long addr,
>  	return false;
>  }
>  
> -static void copy_huge_non_present_pmd(
> +static int copy_huge_non_present_pmd(
>  		struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  		pmd_t *dst_pmd, pmd_t *src_pmd, unsigned long addr,
>  		struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> @@ -1940,14 +1940,40 @@ static void copy_huge_non_present_pmd(
>  		 */
>  		folio_try_dup_anon_rmap_pmd(src_folio, &src_folio->page,
>  					    dst_vma, src_vma);
> +	} else if (softleaf_is_swap(entry)) {
> +		int err;
> +
> +		/*
> +		 * PMD swap entry: duplicate swap references and clear
> +		 * exclusive on source, matching copy_nonpresent_pte().
> +		 *

I'd drop that, rather obvious.

> +		 * A PMD swap entry only exists under CONFIG_THP_SWAP, where
> +		 * SWAPFILE_CLUSTER == HPAGE_PMD_NR, and it is cluster aligned,
> +		 * so these HPAGE_PMD_NR slots are exactly one cluster - which
> +		 * is what swap_dup_entries_direct() requires.

This is the more relevant information.

> +		 */
> +		err = swap_dup_entries_direct(entry, HPAGE_PMD_NR);
> +		if (err < 0)
> +			return err;
> +
> +		mm_prepare_for_swap_entries(dst_mm);
> +
> +		if (pmd_swp_exclusive(pmd)) {
> +			pmd = pmd_swp_clear_exclusive(pmd);
> +			set_pmd_at(src_mm, addr, src_pmd, pmd);
> +		}
>  	}
>  
> -	add_mm_counter(dst_mm, MM_ANONPAGES, HPAGE_PMD_NR);
> +	if (softleaf_is_swap(entry))
> +		add_mm_counter(dst_mm, MM_SWAPENTS, HPAGE_PMD_NR);
> +	else
> +		add_mm_counter(dst_mm, MM_ANONPAGES, HPAGE_PMD_NR);

Can we instead just do it like the PTE variant and move these into the
respective cases?

BTW, I'm surprised that we don't have to handle file THP migration and always
assume MM_ANONPAGES. Maybe we always zap them instead of using migration entries
... maybe :)

>  int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> @@ -1957,6 +1983,7 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  	spinlock_t *dst_ptl, *src_ptl;
>  	struct page *src_page;
>  	struct folio *src_folio;
> +	bool retried = false;

Do we really need this retried logic? I cannot easily spot something similar in
copy_pte_range(). I'd assume once swap_retry_table_alloc() succeeds we should be
mostly good.


>  	pmd_t pmd;
>  	pgtable_t pgtable = NULL;
>  	int ret = -ENOMEM;
> @@ -1988,6 +2015,7 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  	if (unlikely(!pgtable))
>  		goto out;
>  
> +retry:
>  	dst_ptl = pmd_lock(dst_mm, dst_pmd);
>  	src_ptl = pmd_lockptr(src_mm, src_pmd);
>  	spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
> @@ -1995,11 +2023,34 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  	ret = -EAGAIN;
>  	pmd = *src_pmd;
>  
> -	if (unlikely(thp_migration_supported() &&
> -		     pmd_is_valid_softleaf(pmd))) {

That thp_migration_supported() thingy is one ugly function.

> -		copy_huge_non_present_pmd(dst_mm, src_mm, dst_pmd, src_pmd, addr,
> -					  dst_vma, src_vma, pmd, pgtable);
> -		ret = 0;
> +	if (unlikely(pmd_is_valid_softleaf(pmd))) {
> +		ret = copy_huge_non_present_pmd(dst_mm, src_mm, dst_pmd, src_pmd,
> +						addr, dst_vma, src_vma, pmd,
> +						pgtable);
> +		if (ret) {
> +			spin_unlock(src_ptl);
> +			spin_unlock(dst_ptl);
> +			/*
> +			 * For PMD swap entries -ENOMEM means the per-cluster
> +			 * swap-extend table couldn't be GFP_ATOMIC-allocated.
> +			 * Try the GFP_KERNEL fallback once before giving up.
> +			 * swap_retry_table_alloc() also returns 0 when it
> +			 * decides the table is not needed after all, so bound
> +			 * this to a single retry rather than looping on it.
> +			 */
> +			if (ret == -ENOMEM && !retried) {
> +				softleaf_t entry = softleaf_from_pmd(pmd);
> +
> +				retried = true;
> +				if (softleaf_is_swap(entry) &&

How can we suddenly not have a PMD

> +				    !swap_retry_table_alloc(entry, HPAGE_PMD_NR,
> +							    GFP_KERNEL))
> +					goto retry;
> +			}
> +			pte_free(dst_mm, pgtable);
> +			ret = -ENOMEM;
> +			goto out;
> +		}


That's .... messy :)


I was wondering whether we could handle it slightly like the PTE case: return
-EIO and let the caller do that for us. We only have to return the "softleaf_t
entry"

Would avoid the retry label, the manual unlocking etc.

Nit sure, just a thought. The code as is is definitely too messy :)

>  		goto out_unlock;
>  	}
>  
> diff --git a/mm/memory.c b/mm/memory.c
> index 477d7e359b447..84e1e1c22bffa 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -979,7 +979,7 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  	struct page *page;
>  
>  	if (likely(softleaf_is_swap(entry))) {
> -		if (swap_dup_entry_direct(entry) < 0)
> +		if (swap_dup_entries_direct(entry, 1) < 0)
>  			return -EIO;
>  
>  		mm_prepare_for_swap_entries(dst_mm);
> @@ -1394,7 +1394,7 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
>  
>  	if (ret == -EIO) {
>  		VM_WARN_ON_ONCE(!entry.val);
> -		if (swap_retry_table_alloc(entry, GFP_KERNEL) < 0) {
> +		if (swap_retry_table_alloc(entry, 1, GFP_KERNEL) < 0) {
>  			ret = -ENOMEM;
>  			goto out;


For both, I'd provide a simple inline helper that maintains the existing
interface. Less chrun in unrelated code.

[...]

>  
>  /*
> - * swap_dup_entry_direct() - Increase reference count of a swap entry by one.
> + * swap_dup_entries_direct() - Increase reference count of swap entries by one.
>   * @entry: first swap entry from which we want to increase the refcount.
> + * @nr: number of contiguous swap entries to duplicate.
>   *
>   * Returns 0 for success, or -ENOMEM if the extend table is required
>   * but could not be atomically allocated.  Returns -EINVAL if the swap
> @@ -3978,8 +3997,16 @@ void si_swapinfo(struct sysinfo *val)
>   * owner. e.g., locking the PTL of a PTE containing the entry being increased.
>   * Also the swap entry must have a count >= 1. Otherwise folio_dup_swap should
>   * be used.
> + *
> + * Unlike swap_put_entries_direct(), the whole range [entry, entry + nr) must
> + * lie within one swap cluster; a range that crosses a cluster boundary is
> + * rejected with -EINVAL. The only caller passing nr > 1 is the PMD swap entry
> + * fork path: a PMD swap entry can only exist with CONFIG_THP_SWAP, where
> + * SWAPFILE_CLUSTER == HPAGE_PMD_NR, and a PMD-order folio's slots are only ever
> + * allocated at a cluster head (see alloc_swap_scan_cluster()), so such a range
> + * is exactly one cluster.

I would reduce this drastically. This will bitrot easily and the details are
only relevant if this actually ever starts failing.

Just keep the first sentence and add the details to the patch description (which
you effectively already have)

-- 
Cheers,

David