Re: [PATCH 3/7] mm, swap: avoid redundant swap device pinning

[Baoquan He <bhe@redhat.com>]

On 02/15/25 at 01:57am, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> There are only two callers of __read_swap_cache_async not holding a swap
> device reference, so make them hold a reference instead, and drop the
> get/put_swap_device calls in __read_swap_cache_async. This should reduce
> the overhead for swap in during page fault slightly.

This looks good to me, while the log makes me take a little longer time
to understand. Maybe rephrasing them a little bit can facilitate the log
reading. Not sure if my understanding is correct.

=======
Currently, __read_swap_cache_async() has get/put_swap_device() calls to
increase/decrease a swap device reference. While some of its callers
have held the swap  device reference, e.g in do_swap_page() and
shmem_swapin_folio() where __read_swap_cache_async() will finally
called. Now there are only two excpetional callers not holding a swap
device reference, so make them hold a reference instead. And drop the
get/put_swap_device calls in __read_swap_cache_async. This should reduce
the overhead for swap in during page fault slightly.
========

Other than the nit in log, this looks good to me.

Reviewed-by: Baoquan He <bhe@redhat.com>

> 
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>  mm/swap_state.c | 14 ++++++++------
>  mm/zswap.c      |  6 ++++++
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index a54b035d6a6c..50840a2887a5 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -426,17 +426,13 @@ struct folio *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
>  		struct mempolicy *mpol, pgoff_t ilx, bool *new_page_allocated,
>  		bool skip_if_exists)
>  {
> -	struct swap_info_struct *si;
> +	struct swap_info_struct *si = swp_swap_info(entry);
>  	struct folio *folio;
>  	struct folio *new_folio = NULL;
>  	struct folio *result = NULL;
>  	void *shadow = NULL;
>  
>  	*new_page_allocated = false;
> -	si = get_swap_device(entry);
> -	if (!si)
> -		return NULL;
> -
>  	for (;;) {
>  		int err;
>  		/*
> @@ -532,7 +528,6 @@ struct folio *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
>  	put_swap_folio(new_folio, entry);
>  	folio_unlock(new_folio);
>  put_and_return:
> -	put_swap_device(si);
>  	if (!(*new_page_allocated) && new_folio)
>  		folio_put(new_folio);
>  	return result;
> @@ -552,11 +547,16 @@ struct folio *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
>  		struct vm_area_struct *vma, unsigned long addr,
>  		struct swap_iocb **plug)
>  {
> +	struct swap_info_struct *si;
>  	bool page_allocated;
>  	struct mempolicy *mpol;
>  	pgoff_t ilx;
>  	struct folio *folio;
>  
> +	si = get_swap_device(entry);
> +	if (!si)
> +		return NULL;
> +
>  	mpol = get_vma_policy(vma, addr, 0, &ilx);
>  	folio = __read_swap_cache_async(entry, gfp_mask, mpol, ilx,
>  					&page_allocated, false);
> @@ -564,6 +564,8 @@ struct folio *read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
>  
>  	if (page_allocated)
>  		swap_read_folio(folio, plug);
> +
> +	put_swap_device(si);
>  	return folio;
>  }
>  
> diff --git a/mm/zswap.c b/mm/zswap.c
> index ac9d299e7d0c..83dfa1f9e689 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1051,14 +1051,20 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
>  	struct folio *folio;
>  	struct mempolicy *mpol;
>  	bool folio_was_allocated;
> +	struct swap_info_struct *si;
>  	struct writeback_control wbc = {
>  		.sync_mode = WB_SYNC_NONE,
>  	};
>  
>  	/* try to allocate swap cache folio */
> +	si = get_swap_device(swpentry);
> +	if (!si)
> +		return -EEXIST;
> +
>  	mpol = get_task_policy(current);
>  	folio = __read_swap_cache_async(swpentry, GFP_KERNEL, mpol,
>  				NO_INTERLEAVE_INDEX, &folio_was_allocated, true);
> +	put_swap_device(si);
>  	if (!folio)
>  		return -ENOMEM;
>  
> -- 
> 2.48.1
>