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

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

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

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?