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

[Usama Arif]

On 24/09/2026 21:28, David Hildenbrand (Arm) wrote:
> 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).

Ack, the next revision will have the code to duplicate a range of swap entries
as a patch before this one.

> 
>>
>> 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.

Dropped the first half, kept the cluster paragraph.


> 
>> +		 */
>> +		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?
> 

Have done it for next revision, but it makes the diff slightly longer.
The migration arm had to grow a nested if: the outer test is now
softleaf_is_migration(), because a plain readable migration entry
matched none of the old arms yet still needed MM_ANONPAGES.


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


Not zapped, just never copied.  copy_huge_pmd() bails long before this:

  /* Skip if can be re-fill on fault */
  if (!vma_is_anonymous(dst_vma))
          return 0;

so a file THP is simply refilled from the page cache on the child's
first fault.

> 
>>  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.

I actually implemented it similar to copy_pte_range() until v5, but changed it because
of sashiko feedback which looked reasonable to me at that time :)


> 
> 
>>  	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.

pmd_is_valid_softleaf() should actually be sufficient here.

I dropped thp_migration_supported() here.

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


Yeah sorry about this.

I have changed it now, copy_huge_pmd() now returns -EIO and the softleaf_t,
with copy_pmd_range() handling the retry like the PTE path. The next revision
will look like:


 int copy_huge_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)
+                 struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
+                 softleaf_t *entryp)
 {
        spinlock_t *dst_ptl, *src_ptl;
        struct page *src_page;
@@ -1995,11 +2020,14 @@ 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))) {
-               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) {
+                       *entryp = softleaf_from_pmd(pmd);
+                       pte_free(dst_mm, pgtable);
+               }
                goto out_unlock;
        }

diff --git a/mm/memory.c b/mm/memory.c
index 477d7e359b447..c0ad446d0cea4 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1437,11 +1437,21 @@ copy_pmd_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
        do {
                next = pmd_addr_end(addr, end);
                if (pmd_is_huge(*src_pmd)) {
+                       softleaf_t entry = softleaf_mk_none();
                        int err;

                        VM_BUG_ON_VMA(next-addr != HPAGE_PMD_SIZE, src_vma);
+again:
                        err = copy_huge_pmd(dst_mm, src_mm, dst_pmd, src_pmd,
-                                           addr, dst_vma, src_vma);
+                                           addr, dst_vma, src_vma, &entry);
+                       if (err == -EIO) {
+                               VM_WARN_ON_ONCE(!entry.val);
+                               if (swap_retry_table_alloc_nr(entry,
+                                                             HPAGE_PMD_NR,
+                                                             GFP_KERNEL) < 0)
+                                       return -ENOMEM;
+                               goto again;
+                       }
                        if (err == -ENOMEM)
                                return -ENOMEM;
                        if (!err)


and then in copy_huge_non_present_pmd(), I added:


+               err = swap_dup_entries_direct(entry, HPAGE_PMD_NR);
+               if (err)
+                       /* Only -ENOMEM is worth a GFP_KERNEL retry. */
+                       return err == -ENOMEM ? -EIO : -ENOMEM;

as returning -EIO causes a retry. Only -ENOMEM is made retryable,
malformed-entry errors remain terminal rather than potentially retrying indefinitely.


> 
>>  		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.


Done, I added:

static inline int swap_dup_entry_direct(swp_entry_t entry)
  {
          return swap_dup_entries_direct(entry, 1);
  }

  static inline int swap_retry_table_alloc(swp_entry_t entry, gfp_t gfp)
  {
          return swap_retry_table_alloc_nr(entry, 1, gfp);
  }

swap_retry_table_alloc_nr() is not a name I love - suggestions welcome.


> 
> [...]
> 
>>  
>>  /*
>> - * 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)
> 

Done!


Thanks for the reviews!