Re: [PATCH 7/9] mm, swap: remove contention workaround for swap cache

[Chris Li <chrisl@kernel.org>]

Hi Kairui,

It feels so good to remove that 64M swap cache space. Thank you for
making it happen.

Some nitpick follows. I am fine as is as well.

Acked-by: Chris Li <chrisl@kernel.org>

Chris

On Fri, Aug 22, 2025 at 12:21 PM Kairui Song <ryncsn@gmail.com> wrote:
>
> From: Kairui Song <kasong@tencent.com>
>
> Swap cluster setup will try to shuffle the clusters on initialization.
> It was helpful to avoid contention for the swap cache space. The cluster
> size (2M) was much smaller than each swap cache space (64M), so shuffling
> the cluster means the allocator will try to allocate swap slots that are
> in different swap cache spaces for each CPU, reducing the chance of two
> CPUs using the same swap cache space, and hence reducing the contention.
>
> Now, swap cache is managed by swap clusters, this shuffle is pointless.
> Just remove it, and clean up related macros.
>
> This should also improve the HDD swap performance as shuffling IO is a
> bad idea for HDD, and now the shuffling is gone.

Did you have any numbers to prove that :-). Last time the swap
allocator stress testing has already destroyed two of my SAS drives
dedicated for testing. So I am not very keen on running the HDD swap
stress test. The HDD swap stress test are super slow to run, it takes
ages.

>
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202504241621.f27743ec-lkp@intel.com
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>  mm/swap.h     |  4 ----
>  mm/swapfile.c | 32 ++++++++------------------------
>  mm/zswap.c    |  7 +++++--
>  3 files changed, 13 insertions(+), 30 deletions(-)
>
> diff --git a/mm/swap.h b/mm/swap.h
> index 4af42bc2cd72..ce3ec62cc05e 100644
> --- a/mm/swap.h
> +++ b/mm/swap.h
> @@ -153,10 +153,6 @@ int swap_writeout(struct folio *folio, struct swap_iocb **swap_plug);
>  void __swap_writepage(struct folio *folio, struct swap_iocb **swap_plug);
>
>  /* linux/mm/swap_state.c */
> -/* One swap address space for each 64M swap space */
> -#define SWAP_ADDRESS_SPACE_SHIFT       14
> -#define SWAP_ADDRESS_SPACE_PAGES       (1 << SWAP_ADDRESS_SPACE_SHIFT)
> -#define SWAP_ADDRESS_SPACE_MASK                (SWAP_ADDRESS_SPACE_PAGES - 1)
>  extern struct address_space swap_space __ro_after_init;
>  static inline struct address_space *swap_address_space(swp_entry_t entry)
>  {
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index df68b5e242a6..0c8001c99f30 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3203,21 +3203,14 @@ static int setup_swap_map(struct swap_info_struct *si,
>         return 0;
>  }
>
> -#define SWAP_CLUSTER_INFO_COLS                                         \
> -       DIV_ROUND_UP(L1_CACHE_BYTES, sizeof(struct swap_cluster_info))
> -#define SWAP_CLUSTER_SPACE_COLS                                                \
> -       DIV_ROUND_UP(SWAP_ADDRESS_SPACE_PAGES, SWAPFILE_CLUSTER)
> -#define SWAP_CLUSTER_COLS                                              \
> -       max_t(unsigned int, SWAP_CLUSTER_INFO_COLS, SWAP_CLUSTER_SPACE_COLS)
> -
>  static struct swap_cluster_info *setup_clusters(struct swap_info_struct *si,
>                                                 union swap_header *swap_header,
>                                                 unsigned long maxpages)
>  {
>         unsigned long nr_clusters = DIV_ROUND_UP(maxpages, SWAPFILE_CLUSTER);
>         struct swap_cluster_info *cluster_info;
> -       unsigned long i, j, idx;
>         int err = -ENOMEM;
> +       unsigned long i;

Nitpick: This line location change is not necessary.

>
>         cluster_info = kvcalloc(nr_clusters, sizeof(*cluster_info), GFP_KERNEL);
>         if (!cluster_info)
> @@ -3266,22 +3259,13 @@ static struct swap_cluster_info *setup_clusters(struct swap_info_struct *si,
>                 INIT_LIST_HEAD(&si->frag_clusters[i]);
>         }
>
> -       /*
> -        * Reduce false cache line sharing between cluster_info and
> -        * sharing same address space.
> -        */
> -       for (j = 0; j < SWAP_CLUSTER_COLS; j++) {
> -               for (i = 0; i < DIV_ROUND_UP(nr_clusters, SWAP_CLUSTER_COLS); i++) {
> -                       struct swap_cluster_info *ci;
> -                       idx = i * SWAP_CLUSTER_COLS + j;
> -                       ci = cluster_info + idx;
> -                       if (idx >= nr_clusters)
> -                               continue;
> -                       if (ci->count) {
> -                               ci->flags = CLUSTER_FLAG_NONFULL;
> -                               list_add_tail(&ci->list, &si->nonfull_clusters[0]);
> -                               continue;
> -                       }
> +       for (i = 0; i < nr_clusters; i++) {
> +               struct swap_cluster_info *ci = &cluster_info[i];

struct swap_cluster_info *ci = cluster_info + i;
looks simpler. Pure nitpick and personal preference, you don't have to
follow it.

> +
> +               if (ci->count) {
> +                       ci->flags = CLUSTER_FLAG_NONFULL;
> +                       list_add_tail(&ci->list, &si->nonfull_clusters[0]);
> +               } else {
>                         ci->flags = CLUSTER_FLAG_FREE;
>                         list_add_tail(&ci->list, &si->free_clusters);
>                 }
> diff --git a/mm/zswap.c b/mm/zswap.c
> index c869859eec77..c0a9be14a725 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -237,10 +237,13 @@ static bool zswap_has_pool;
>  * helpers and fwd declarations
>  **********************************/
>
> +/* One swap address space for each 64M swap space */
> +#define ZSWAP_ADDRESS_SPACE_SHIFT 14
> +#define ZSWAP_ADDRESS_SPACE_PAGES (1 << ZSWAP_ADDRESS_SPACE_SHIFT)
>  static inline struct xarray *swap_zswap_tree(swp_entry_t swp)
>  {
>         return &zswap_trees[swp_type(swp)][swp_offset(swp)
> -               >> SWAP_ADDRESS_SPACE_SHIFT];
> +               >> ZSWAP_ADDRESS_SPACE_SHIFT];
>  }
>
>  #define zswap_pool_debug(msg, p)                               \
> @@ -1771,7 +1774,7 @@ int zswap_swapon(int type, unsigned long nr_pages)
>         struct xarray *trees, *tree;
>         unsigned int nr, i;
>
> -       nr = DIV_ROUND_UP(nr_pages, SWAP_ADDRESS_SPACE_PAGES);
> +       nr = DIV_ROUND_UP(nr_pages, ZSWAP_ADDRESS_SPACE_PAGES);
>         trees = kvcalloc(nr, sizeof(*tree), GFP_KERNEL);
>         if (!trees) {
>                 pr_err("alloc failed, zswap disabled for swap type %d\n", type);
> --
> 2.51.0
>