Re: [PATCH v3 07/19] mm/shmem: never bypass the swap cache for SWP_SYNCHRONOUS_IO

[Kairui Song <ryncsn@gmail.com>]

On Tue, Dec 2, 2025 at 3:34 PM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
> Hi Kairui,
>
> On 2025/11/25 03:13, Kairui Song wrote:
> > From: Kairui Song <kasong@tencent.com>
> >
> > Now the overhead of the swap cache is trivial to none, bypassing the
> > swap cache is no longer a valid optimization.
> >
> > We have removed the cache bypass swapin for anon memory, now do the same
> > for shmem. Many helpers and functions can be dropped now.
> >
> > Signed-off-by: Kairui Song <kasong@tencent.com>
> > ---
>
> I'm glad to see we can remove the skip swapcache logic. I did a quick
> test, testing 1G shmem sequential swap-in with 64K mTHP and 2M mTHP, and
> I observed a slight drop, which could also be fluctuation. Can you also
> perform some measurements?
>
> 64K shmem mTHP:
> W/ patchset     W/o patchset
> 154 ms          148 ms
>
> 2M shmem mTHP
> W/ patchset     W/o patchset
> 117 ms          115 ms

Hi Baolin,

Thanks for testing! This patch (7/19) is still an intermediate step,
so we are still updating both swap_map and swap table with higher
overhead. And even with that, the performance change looks small
(~1-4% in the result you posted), close to noise level.

And after this whole series, the double update is *partially* dropped,
so the performance is almost identical to before:

tmpfs with transparent_hugepage_tmpfs=within_size, 3 test run on my machine:
Before       [PATCH 7/19]         [PATCH 19/19]
5.99s          6.29s            6.08s (~1%)

Note we are still using swap_map so there are double lookups
everywhere in this series, and I added more WARN_ON checks. Swap is
complex so being cautious is better I think. I've also mentioned
another valkey slight performance drop in the cover letter due to
this, which is also tiny and will be improved a lot in phase 3 by
removing swap_map and the double lookup, as demonstrated before:
https://lore.kernel.org/linux-mm/20250514201729.48420-1-ryncsn@gmail.com/

Last time I tested that branch it was a clear optimization for shmem,
some of the optimizations in that series were split or merged
separately so the performance may look go up / down in some
intermediate steps, the final result is good.

swap_cgroup_ctrl will be gone too, even later maybe though.

>
> Anyway I still hope we can remove the skip swapcache logic. The changes
> look good to me with one nit as below. Thanks for your work.
>
> >   mm/shmem.c    | 65 +++++++++++++++++------------------------------------------
> >   mm/swap.h     |  4 ----
> >   mm/swapfile.c | 35 +++++++++-----------------------
> >   3 files changed, 27 insertions(+), 77 deletions(-)
> >
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index ad18172ff831..d08248fd67ff 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2001,10 +2001,9 @@ static struct folio *shmem_swap_alloc_folio(struct inode *inode,
> >               swp_entry_t entry, int order, gfp_t gfp)
> >   {
> >       struct shmem_inode_info *info = SHMEM_I(inode);
> > +     struct folio *new, *swapcache;
> >       int nr_pages = 1 << order;
> > -     struct folio *new;
> >       gfp_t alloc_gfp;
> > -     void *shadow;
> >
> >       /*
> >        * We have arrived here because our zones are constrained, so don't
> > @@ -2044,34 +2043,19 @@ static struct folio *shmem_swap_alloc_folio(struct inode *inode,
> >               goto fallback;
> >       }
> >
> > -     /*
> > -      * Prevent parallel swapin from proceeding with the swap cache flag.
> > -      *
> > -      * Of course there is another possible concurrent scenario as well,
> > -      * that is to say, the swap cache flag of a large folio has already
> > -      * been set by swapcache_prepare(), while another thread may have
> > -      * already split the large swap entry stored in the shmem mapping.
> > -      * In this case, shmem_add_to_page_cache() will help identify the
> > -      * concurrent swapin and return -EEXIST.
> > -      */
> > -     if (swapcache_prepare(entry, nr_pages)) {
> > +     swapcache = swapin_folio(entry, new);
> > +     if (swapcache != new) {
> >               folio_put(new);
> > -             new = ERR_PTR(-EEXIST);
> > -             /* Try smaller folio to avoid cache conflict */
> > -             goto fallback;
> > +             if (!swapcache) {
> > +                     /*
> > +                      * The new folio is charged already, swapin can
> > +                      * only fail due to another raced swapin.
> > +                      */
> > +                     new = ERR_PTR(-EEXIST);
> > +                     goto fallback;
> > +             }
> >       }
> > -
> > -     __folio_set_locked(new);
> > -     __folio_set_swapbacked(new);
> > -     new->swap = entry;
> > -
> > -     memcg1_swapin(entry, nr_pages);
> > -     shadow = swap_cache_get_shadow(entry);
> > -     if (shadow)
> > -             workingset_refault(new, shadow);
> > -     folio_add_lru(new);
> > -     swap_read_folio(new, NULL);
> > -     return new;
> > +     return swapcache;
> >   fallback:
> >       /* Order 0 swapin failed, nothing to fallback to, abort */
> >       if (!order)
> > @@ -2161,8 +2145,7 @@ static int shmem_replace_folio(struct folio **foliop, gfp_t gfp,
> >   }
> >
> >   static void shmem_set_folio_swapin_error(struct inode *inode, pgoff_t index,
> > -                                      struct folio *folio, swp_entry_t swap,
> > -                                      bool skip_swapcache)
> > +                                      struct folio *folio, swp_entry_t swap)
> >   {
> >       struct address_space *mapping = inode->i_mapping;
> >       swp_entry_t swapin_error;
> > @@ -2178,8 +2161,7 @@ static void shmem_set_folio_swapin_error(struct inode *inode, pgoff_t index,
> >
> >       nr_pages = folio_nr_pages(folio);
> >       folio_wait_writeback(folio);
> > -     if (!skip_swapcache)
> > -             swap_cache_del_folio(folio);
> > +     swap_cache_del_folio(folio);
> >       /*
> >        * Don't treat swapin error folio as alloced. Otherwise inode->i_blocks
> >        * won't be 0 when inode is released and thus trigger WARN_ON(i_blocks)
> > @@ -2279,7 +2261,6 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
> >       softleaf_t index_entry;
> >       struct swap_info_struct *si;
> >       struct folio *folio = NULL;
> > -     bool skip_swapcache = false;
> >       int error, nr_pages, order;
> >       pgoff_t offset;
> >
> > @@ -2322,7 +2303,6 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
> >                               folio = NULL;
> >                               goto failed;
> >                       }
> > -                     skip_swapcache = true;
> >               } else {
> >                       /* Cached swapin only supports order 0 folio */
> >                       folio = shmem_swapin_cluster(swap, gfp, info, index);
> > @@ -2378,9 +2358,8 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
> >        * and swap cache folios are never partially freed.
> >        */
> >       folio_lock(folio);
> > -     if ((!skip_swapcache && !folio_test_swapcache(folio)) ||
> > -         shmem_confirm_swap(mapping, index, swap) < 0 ||
> > -         folio->swap.val != swap.val) {
> > +     if (!folio_matches_swap_entry(folio, swap) ||
> > +         shmem_confirm_swap(mapping, index, swap) < 0) {
>
> We should still keep the '!folio_test_swapcache(folio)' check here?

Thanks for the review, this one is OK because folio_test_swapcache is
included in folio_matches_swap_entry already.