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

[Baolin Wang]

On 2025/12/3 13:33, Kairui Song wrote:
> On Tue, Dec 2, 2025 at 3:34 PM Baolin Wang
> <baolin.wang@xxxxxxxxxxxxxxxxx> wrote:
> > Hi Kairui,
> >
> > On 2025/11/25 03:13, Kairui Song wrote:
> > > From: Kairui Song <kasong@xxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxx>
> > > ---
> >
> > 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@xxxxxxxxx/
>
> 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.

Sounds good. Better to mention this (including your data) in the commit 
message. With that, please feel free to add:
Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
Tested-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>

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

OK.