Re: [PATCH v3 01/12] mm, swap: simplify swap cache allocation helper

[Chris Li]

On Tue, Apr 21, 2026 at 8:16 AM Kairui Song via B4 Relay
<devnull+kasong.tencent.com@xxxxxxxxxx> wrote:
>
> From: Kairui Song <kasong@xxxxxxxxxxx>
>
> Instead of trying to return the existing folio if the entry is already
> cached, simply return an error code if the allocation fails and drop the

Nitpick: Spell out which function changes the return type here. It is
__swap_cache_prepare_and_add()

> output argument. And introduce proper wrappers that handle the

Nitpick: Spell out the helper function. It is swap_cache_read_folio().
> allocation failure in different ways.

>
> For async swapin and readahead, the caller only wants to ensure that a
> swap-in read is issued when the allocation succeeded. And for zswap swap
> out, the caller will abort if the allocation failed because the entry is
> gone or cached already.

Should you add no functional change expected?

>
> Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>

Very nice clean ups. I like it. Here are some nitpicks; feel free to
ignore them.

Acked-by: Chris Li <chrisl@xxxxxxxxx>

> ---
>  mm/swap.h       |   3 +-
>  mm/swap_state.c | 180 +++++++++++++++++++++++++++++---------------------------
>  mm/zswap.c      |  23 +++-----
>  3 files changed, 103 insertions(+), 103 deletions(-)
>
> diff --git a/mm/swap.h b/mm/swap.h
> index a77016f2423b..ad8b17a93758 100644
> --- a/mm/swap.h
> +++ b/mm/swap.h
> @@ -281,8 +281,7 @@ struct folio *swap_cache_get_folio(swp_entry_t entry);
>  void *swap_cache_get_shadow(swp_entry_t entry);
>  void swap_cache_del_folio(struct folio *folio);
>  struct folio *swap_cache_alloc_folio(swp_entry_t entry, gfp_t gfp_flags,
> -                                    struct mempolicy *mpol, pgoff_t ilx,
> -                                    bool *alloced);
> +                                    struct mempolicy *mpol, pgoff_t ilx);
>  /* Below helpers require the caller to lock and pass in the swap cluster. */
>  void __swap_cache_add_folio(struct swap_cluster_info *ci,
>                             struct folio *folio, swp_entry_t entry);
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 1415a5c54a43..204a9499d50c 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -459,54 +459,38 @@ void swap_update_readahead(struct folio *folio, struct vm_area_struct *vma,
>   * All swap slots covered by the folio must have a non-zero swap count.
>   *
>   * Context: Caller must protect the swap device with reference count or locks.
> - * Return: Returns the folio being added on success. Returns the existing folio
> - * if @entry is already cached. Returns NULL if raced with swapin or swapoff.
> + * Return: 0 if success, error code if failed.
>   */
> -static struct folio *__swap_cache_prepare_and_add(swp_entry_t entry,
> -                                                 struct folio *folio,
> -                                                 gfp_t gfp, bool charged)
> +static int __swap_cache_prepare_and_add(swp_entry_t entry,
> +                                       struct folio *folio,
> +                                       gfp_t gfp, bool charged)
>  {
> -       struct folio *swapcache = NULL;
>         void *shadow;
>         int ret;
>
>         __folio_set_locked(folio);
>         __folio_set_swapbacked(folio);
>
> -       if (!charged && mem_cgroup_swapin_charge_folio(folio, NULL, gfp, entry))
> +       if (!charged && mem_cgroup_swapin_charge_folio(folio, NULL, gfp, entry)) {
> +               ret = -ENOMEM;
>                 goto failed;
> -
> -       for (;;) {
> -               ret = swap_cache_add_folio(folio, entry, &shadow);
> -               if (!ret)
> -                       break;
> -
> -               /*
> -                * Large order allocation needs special handling on
> -                * race: if a smaller folio exists in cache, swapin needs
> -                * to fallback to order 0, and doing a swap cache lookup
> -                * might return a folio that is irrelevant to the faulting
> -                * entry because @entry is aligned down. Just return NULL.
> -                */
> -               if (ret != -EEXIST || folio_test_large(folio))
> -                       goto failed;
> -
> -               swapcache = swap_cache_get_folio(entry);
> -               if (swapcache)
> -                       goto failed;
>         }
>
> +       ret = swap_cache_add_folio(folio, entry, &shadow);
> +       if (ret)
> +               goto failed;
> +
>         memcg1_swapin(entry, folio_nr_pages(folio));
>         if (shadow)
>                 workingset_refault(folio, shadow);
>
>         /* Caller will initiate read into locked folio */
>         folio_add_lru(folio);
> -       return folio;
> +       return 0;
>
>  failed:
>         folio_unlock(folio);
> -       return swapcache;
> +       return ret;
>  }
>
>  /**
> @@ -515,7 +499,6 @@ static struct folio *__swap_cache_prepare_and_add(swp_entry_t entry,
>   * @gfp_mask: memory allocation flags
>   * @mpol: NUMA memory allocation policy to be applied
>   * @ilx: NUMA interleave index, for use only when MPOL_INTERLEAVE
> - * @new_page_allocated: sets true if allocation happened, false otherwise
>   *
>   * Allocate a folio in the swap cache for one swap slot, typically before
>   * doing IO (e.g. swap in or zswap writeback). The swap slot indicated by
> @@ -523,18 +506,40 @@ static struct folio *__swap_cache_prepare_and_add(swp_entry_t entry,
>   * Currently only supports order 0.
>   *
>   * Context: Caller must protect the swap device with reference count or locks.
> - * Return: Returns the existing folio if @entry is cached already. Returns
> - * NULL if failed due to -ENOMEM or @entry have a swap count < 1.
> + * Return: Returns the folio if allocation succeeded and folio is added to
> + * swap cache. Returns error code if allocation failed due to race.
>   */
>  struct folio *swap_cache_alloc_folio(swp_entry_t entry, gfp_t gfp_mask,
> -                                    struct mempolicy *mpol, pgoff_t ilx,
> -                                    bool *new_page_allocated)
> +                                    struct mempolicy *mpol, pgoff_t ilx)
> +{
> +       int ret;

Nitpick: Suggest renaming it to "err" to make it obvious that it is an
int type for the error code. Because this function previously returned
a folio pointer, I have to remind myself that it is an int type not a
folio.

> +       struct folio *folio;
> +
> +       /* Allocate a new folio to be added into the swap cache. */
> +       folio = folio_alloc_mpol(gfp_mask, 0, mpol, ilx, numa_node_id());
> +       if (!folio)
> +               return ERR_PTR(-ENOMEM);
> +
> +       /*
> +        * Try to add the new folio to the swap cache. It returns
> +        * -EEXIST if the entry is already cached.
> +        */
> +       ret = __swap_cache_prepare_and_add(entry, folio, gfp_mask, false);
> +       if (ret) {
> +               folio_put(folio);
> +               return ERR_PTR(ret);
> +       }
> +
> +       return folio;
> +}
> +
> +static struct folio *swap_cache_read_folio(swp_entry_t entry, gfp_t gfp,
> +                                          struct mempolicy *mpol, pgoff_t ilx,
> +                                          struct swap_iocb **plug, bool readahead)
>  {
>         struct swap_info_struct *si = __swap_entry_to_info(entry);
>         struct folio *folio;
> -       struct folio *result = NULL;
>
> -       *new_page_allocated = false;
>         /* Check the swap cache again for readahead path. */
>         folio = swap_cache_get_folio(entry);
>         if (folio)
> @@ -544,17 +549,24 @@ struct folio *swap_cache_alloc_folio(swp_entry_t entry, gfp_t gfp_mask,
>         if (!swap_entry_swapped(si, entry))
>                 return NULL;
>
> -       /* Allocate a new folio to be added into the swap cache. */
> -       folio = folio_alloc_mpol(gfp_mask, 0, mpol, ilx, numa_node_id());
> -       if (!folio)
> +       do {
> +               folio = swap_cache_get_folio(entry);
> +               if (folio)
> +                       return folio;
> +
> +               folio = swap_cache_alloc_folio(entry, gfp, mpol, ilx);
> +       } while (IS_ERR(folio) && PTR_ERR(folio) == -EEXIST);

Nitpick: IS_ERR() only checks that the pointer is in the error code
range. If the pointer is -EEXIST, it will always be in the error code
range. I think the "IS_ERR(folio)" test can be dropped.

Chris