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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2025/12/3 13:33, Kairui Song wrote:
> 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.

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@linux.alibaba.com>
Tested-by: Baolin Wang <baolin.wang@linux.alibaba.com>

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