Re: [PATCH v3 2/4] mm: drop stale folio_ref_count()==1 check in do_swap_page reuse logic

[Barry Song]

On Thu, Jul 2, 2026 at 4:14 PM David Hildenbrand (Arm) <david@xxxxxxxxxx> wrote:
>
> On 7/2/26 01:59, Barry Song (Xiaomi) wrote:
> > The "we just allocated them without exposing them to the swapcache"
> > case no longer exists, as Kairui has routed synchronous I/O through
> > the swapcache as well in his series "unify swapin use swap cache and
> > cleanup flags"[1]. As a result, folio_ref_count() should never be 1
> > in this path, since at least two references are held (base ref plus
> > swapcache). Remove the folio_ref_count()==1 check and update the
> > comment accordingly.
>
> Sashiko points out two minor things (one flagged as medium, lol, sure sure).
>
> Here, you can clarify that folio_ref_count()==1 is true for freshly allocated
> pages (due to the KSM check) in which case exclusive=true already.

Thanks. Sashiko's comment below seems valid. While there's no
need to change the code, the changelog should be updated to
reflect it. I'll do that in v4.

"Is the claim that folio_ref_count() should never be 1 in this path accurate?
When ksm_might_need_to_copy() allocates a fresh folio to break KSM sharing,
the new folio bypasses the swap cache:
do_swap_page() {
 ...
 if (!folio_test_ksm(folio)) {
   ...
 if (folio != swapcache) {
    /*
     * We have a fresh page that is not exposed to the
     * swapcache -> certainly exclusive.
     */
  exclusive = true;
  }
  ...
}
In this specific case, the folio refcount would still be exactly 1 when it
reaches this path.
While the code change itself is safe because the KSM copy path explicitly
sets exclusive = true earlier (satisfying the new if (exclusive) check
without needing the refcount check), the rationale in the commit message
seems to overlook this scenario."

>
> >
> > [1] https://lore.kernel.org/all/20251220-swap-table-p2-v5-0-8862a265a033@xxxxxxxxxxx/
> >
> > Acked-by: Usama Arif <usama.arif@xxxxxxxxx>
> > Reviewed-by: Kairui Song <kasong@xxxxxxxxxxx>
> > Reviewed-by: Baoquan He <baoquan.he@xxxxxxxxx>
> > Acked-by: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> > Signed-off-by: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> > ---
> >  mm/memory.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 87da78eb1abd..71e9d394816b 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -5046,13 +5046,10 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> >               pte = pte_mkuffd_wp(pte);
> >
> >       /*
> > -      * Same logic as in do_wp_page(); however, optimize for pages that are
> > -      * certainly not shared either because we just allocated them without
> > -      * exposing them to the swapcache or because the swap entry indicates
> > -      * exclusivity.
> > +      * Similar logic as in do_wp_page(); however, optimize for pages that are
> > +      * certainly not because the swap entry indicates exclusivity.
>
> s/not/not shared/
>
> but likely you should just simplify to "... pages that are certainly exclusive."

Thanks. I will fix the doc in v4.

Best Regards
Barry