Re: [PATCH v2 4/4] mm: try to free swapcache for non-LRU folios

[Kairui Song <ryncsn@gmail.com>]

On Thu, Jun 25, 2026 at 05:14:45AM +0800, Barry Song wrote:
> On Wed, Jun 24, 2026 at 11:20 PM David Hildenbrand (Arm)
> <david@kernel.org> wrote:
> >
> > On 6/24/26 01:16, Barry Song (Xiaomi) wrote:
> > > Originally, we unconditionally called lru_add_drain() for write
> > > swap-in page faults. This might drop the reference held by the per-CPU
> > > LRU cache if the folio happened to reside there. However, there was no
> > > guarantee that the folio was actually cached on the current CPU.
> > >
> > > Now that lru_add_drain() has been removed, we have lost one
> > > opportunity to drop a reference held by the LRU cache. We could
> > > instead incorporate that possibility into the condition evaluated by
> > > should_try_to_free_swap().
> > >
> > > Suggested-by: Kairui Song <ryncsn@gmail.com>
> > > Signed-off-by: Barry Song (Xiaomi) <baohua@kernel.org>
> > > ---
> > >  mm/memory.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index 2983a6baf474..14577c67c61a 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -5087,8 +5087,11 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> > >        * Remove the swap entry and conditionally try to free up the swapcache.
> > >        * Do it after mapping, so raced page faults will likely see the folio
> > >        * in swap cache and wait on the folio lock.
> > > +      * Assume non-LRU folios may be queued in the LRU cache, which contributes
> > > +      * an additional reference to the folio.
> > >        */
> > > -     if (should_try_to_free_swap(si, folio, vma, nr_pages, vmf->flags))
> > > +     if (should_try_to_free_swap(si, folio, vma, nr_pages +
> > > +                     !folio_test_lru(folio), vmf->flags))
> > >               folio_free_swap(folio);
> > >
> > >       folio_unlock(folio);
> >
> > Hm, in wp_can_reuse_anon_folio() we'll try dropping the swapcache ourselves.
> >
> > So I wonder if we still need that handling ("If we want to map a page that's in
> > the swapcache writable, we ...") at all?
> >
> >
> > Ahh, I see the problem now:
> >
> > commit 4b34f1d82c6549837b2061096dea249e881a4495
> > Author: Kairui Song <kasong@tencent.com>
> > Date:   Sat Dec 20 03:43:35 2025 +0800
> >
> >     mm, swap: free the swap cache after folio is mapped
> >
> >     Currently, we remove the folio from the swap cache and free the swap cache
> >     before mapping the PTE.  To reduce repeated faults due to parallel swapins
> >     of the same PTE, change it to remove the folio from the swap cache after
> >     it is mapped.  So new faults from the swap PTE will be much more likely to
> >     see the folio in the swap cache and wait on it.
> >
> >     This does not eliminate all swapin races: an ongoing swapin fault may
> >     still see an empty swap cache.  That's harmless, as the PTE is changed
> >     before the swap cache is cleared, so it will just return and not trigger
> >     any repeated faults.  This does help to reduce the chance.
> >
> >
> > That changed that behavior such that we *must* now always fallback to do_wp_page().
> >
> > What a mess (I didn't ack)

That's awkward, my bad, terribly sorry about this :(

I sincerely apologize for the oversight and the trouble this has caused.

I haven't seen any performance regression in any workload recently
though, or any correctness issue, perhaps the round trip of
do_wp_page wasn't that bad. It should still catch the reuse folios,
just more costly than doing things in-place.

I think we should restore the original check first. We might also want to
avoid dropping the swap cache if the folio will not be reused, which
was discussed here:

https://lore.kernel.org/linux-mm/CAMgjq7BDfvNXdWH0cqarsujjUn3i3tDDhDkmSg01TR4h-tDorQ@mail.gmail.com/

Maybe extracting some common part into a helper can help make this
cleaner.

> 
> So it seems we need a patch before patch 4 to fix the missed
> swapcache freeing on write faults, because the write bit has already
> been cleared by the time the folio is mapped and reused?
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 14577c67c61a..e8fc5a86f6ea 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4752,6 +4752,7 @@ static void check_swap_exclusive(struct folio
> *folio, swp_entry_t entry,
>   */
>  vm_fault_t do_swap_page(struct vm_fault *vmf)
>  {
> +       enum fault_flag fault_flags = vmf->flags;
>         struct vm_area_struct *vma = vmf->vma;
>         struct folio *swapcache = NULL, *folio;
>         struct page *page;
> @@ -5091,7 +5092,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>          * an additional reference to the folio.
>          */
>         if (should_try_to_free_swap(si, folio, vma, nr_pages +
> -                       !folio_test_lru(folio), vmf->flags))
> +                       !folio_test_lru(folio), fault_flags))
>                 folio_free_swap(folio);
> 
>         folio_unlock(folio);
> 
> 
> Best Regards
> Barry

Hi Barry,

The problem is more than that, the `exclusive || folio_ref_count(folio) == 1`
in do_swap_page is also ineffective now.

But that earlier commit 4b34f1d82c65 is kind of meaningless after recent
refactor of swapin. It was trying to avoid redundant folio allocation
and thrashing from concurrent faulting process of the same PTE, but recent
swap table rework already gated that with unified folio allocation. We
are freeing the swap entry first here, that is effective enough so that
commit is no longer needed.

So perhaps we can first just revert it, how do you think?:

From 8009328e0782fc22365ef32474bae749beda637b Mon Sep 17 00:00:00 2001
From: Kairui Song <kasong@tencent.com>
Date: Thu, 25 Jun 2026 11:44:26 +0800
Subject: [PATCH] mm/swap: free swap cache before mapping to fix folio reuse

This is effectively a revert of commit 4b34f1d82c65 ("mm, swap: free the
swap cache after folio is mapped").

That commit was trying to reduce wasted fault and folio allocation
from parallel swapins of the same entry: keeping the folio in the swap
cache until the PTE is installed makes a racing fault more likely to find
it and wait on the folio lock, rather than allocate and charge a new folio
only to discover the race and throw it away, which leads to thrashing.

That benefit is marginal now. Folio allocation and swapin are both gated
on the swap entry count in the swap cache layer since commit 02d733a7ec1d
("mm, swap: unify large folio allocation"). Once the winning fault
drops the entry count, a racing fault that misses the cache will fail
the swap entry count check too and back off at the page table lock before
allocating anything. Note there is still a tiny window with or without
either patch, but it's already minimized now and should be fixed in
other ways later if needed.

Meanwhile the earlier commit has a real problem, as David pointed out [1].
The swap cache reference is still held during the write fault folio
reference check, so a sole-owned but non-exclusive write fault always
falls back to do_wp_page() instead of being reused in place. The
write-fault path of should_try_to_free_swap() is broken too as the
FAULT_FLAG_WRITE flag is missing.

There is no correctness problem though, only the reuse and cleanup
optimizations were lost.

So revert it. This makes the sole-owner check effective again and
recovers the reuse fast path. Also slightly adjust a sanity check
to be more rigorous.

Fixes: 4b34f1d82c65 ("mm, swap: free the swap cache after folio is mapped")
Reported-by: David Hildenbrand (Arm) <david@kernel.org>
Closes: https://lore.kernel.org/linux-mm/e56c4d73-ed4a-48bb-8d0a-97b1200d4a35@kernel.org/ [1]
Signed-off-by: Kairui Song <kasong@tencent.com>
---
 mm/memory.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index ff338c2abe92..f31d6b8e8c0b 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4512,7 +4512,6 @@ static vm_fault_t remove_device_exclusive_entry(struct vm_fault *vmf)
 static inline bool should_try_to_free_swap(struct swap_info_struct *si,
 					   struct folio *folio,
 					   struct vm_area_struct *vma,
-					   unsigned int extra_refs,
 					   unsigned int fault_flags)
 {
 	if (!folio_test_swapcache(folio))
@@ -4535,7 +4534,7 @@ static inline bool should_try_to_free_swap(struct swap_info_struct *si,
 	 * reference only in case it's likely that we'll be the exclusive user.
 	 */
 	return (fault_flags & FAULT_FLAG_WRITE) && !folio_test_ksm(folio) &&
-		folio_ref_count(folio) == (extra_refs + folio_nr_pages(folio));
+		folio_ref_count(folio) == (1 + folio_nr_pages(folio));
 }
 
 static vm_fault_t pte_marker_clear(struct vm_fault *vmf)
@@ -5033,6 +5032,24 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 	 */
 	arch_swap_restore(folio_swap(entry, folio), folio);
 
+	/*
+	 * Remove the swap entry and conditionally try to free up the swapcache.
+	 * We're already holding a reference on the page but haven't mapped it
+	 * yet.
+	 *
+	 * The swap count has to be freed to 0 first so folio_free_swap
+	 * can free exclusive clean cache. Concurrent fault is very unlikely
+	 * to trigger redundant folio alloc, nor will it cause redundant IO,
+	 * as the swap entry count gates it.
+	 */
+	VM_WARN_ON_ONCE(nr_pages != 1 && nr_pages != folio_nr_pages(folio));
+	if (folio != swapcache)
+		folio_put_swap(swapcache, NULL);
+	else
+		folio_put_swap(folio, nr_pages < folio_nr_pages(folio) ? page : NULL);
+	if (should_try_to_free_swap(si, folio, vma, vmf->flags))
+		folio_free_swap(folio);
+
 	add_mm_counter(vma->vm_mm, MM_ANONPAGES, nr_pages);
 	add_mm_counter(vma->vm_mm, MM_SWAPENTS, -nr_pages);
 	pte = mk_pte(page, vma->vm_page_prot);
@@ -5067,7 +5084,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 	if (unlikely(folio != swapcache)) {
 		folio_add_new_anon_rmap(folio, vma, address, RMAP_EXCLUSIVE);
 		folio_add_lru_vma(folio, vma);
-		folio_put_swap(swapcache, NULL);
 	} else if (!folio_test_anon(folio)) {
 		/*
 		 * We currently only expect !anon folios that are fully
@@ -5076,12 +5092,9 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 		VM_WARN_ON_ONCE_FOLIO(folio_nr_pages(folio) != nr_pages, folio);
 		VM_WARN_ON_ONCE_FOLIO(folio_mapped(folio), folio);
 		folio_add_new_anon_rmap(folio, vma, address, rmap_flags);
-		folio_put_swap(folio, NULL);
 	} else {
-		VM_WARN_ON_ONCE(nr_pages != 1 && nr_pages != folio_nr_pages(folio));
 		folio_add_anon_rmap_ptes(folio, page, nr_pages, vma, address,
 					 rmap_flags);
-		folio_put_swap(folio, nr_pages == 1 ? page : NULL);
 	}
 
 	VM_BUG_ON(!folio_test_anon(folio) ||
@@ -5090,14 +5103,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 	arch_do_swap_page_nr(vma->vm_mm, vma, address,
 			pte, pte, nr_pages);
 
-	/*
-	 * Remove the swap entry and conditionally try to free up the swapcache.
-	 * Do it after mapping, so raced page faults will likely see the folio
-	 * in swap cache and wait on the folio lock.
-	 */
-	if (should_try_to_free_swap(si, folio, vma, nr_pages, vmf->flags))
-		folio_free_swap(folio);
-
 	folio_unlock(folio);
 	if (unlikely(folio != swapcache)) {
 		/*
-- 
2.54.0