Message-ID: <CAMgjq7AkRmb5ote-VZErM_2UdEC575j9WcrstcQOypEb+T-DLA@mail.gmail.com>
Date: Fri, 7 Mar 2025 10:54:52 +0800
From: Kairui Song <ryncsn@...il.com>
To: linux-mm@...ck.org
Cc: Andrew Morton <akpm@...ux-foundation.org>, Chris Li <chrisl@...nel.org>, 
	Barry Song <v-songbaohua@...o.com>, Hugh Dickins <hughd@...gle.com>, 
	Yosry Ahmed <yosryahmed@...gle.com>, "Huang, Ying" <ying.huang@...ux.alibaba.com>, 
	Baoquan He <bhe@...hat.com>, Nhat Pham <nphamcs@...il.com>, Johannes Weiner <hannes@...xchg.org>, 
	Baolin Wang <baolin.wang@...ux.alibaba.com>, Kalesh Singh <kaleshsingh@...gle.com>, 
	Matthew Wilcox <willy@...radead.org>, linux-kernel@...r.kernel.org, 
	Kairui Song <kasong@...cent.com>
Subject: Re: [PATCH v2 5/7] mm, swap: use percpu cluster as allocation fast path

On Tue, Feb 25, 2025 at 2:03 AM Kairui Song <ryncsn@...il.com> wrote:
>
> From: Kairui Song <kasong@...cent.com>
>
> Current allocation workflow first traverses the plist with a global lock
> held, after choosing a device, it uses the percpu cluster on that swap
> device. This commit moves the percpu cluster variable out of being tied
> to individual swap devices, making it a global percpu variable, and will
> be used directly for allocation as a fast path.
>
> The global percpu cluster variable will never point to a HDD device, and
> allocations on a HDD device are still globally serialized.
>
> This improves the allocator performance and prepares for removal of the
> slot cache in later commits. There shouldn't be much observable behavior
> change, except one thing: this changes how swap device allocation
> rotation works.
>
> Currently, each allocation will rotate the plist, and because of the
> existence of slot cache (one order 0 allocation usually returns 64
> entries), swap devices of the same priority are rotated for every 64
> order 0 entries consumed. High order allocations are different, they
> will bypass the slot cache, and so swap device is rotated for every
> 16K, 32K, or up to 2M allocation.
>
> The rotation rule was never clearly defined or documented, it was changed
> several times without mentioning.
>
> After this commit, and once slot cache is gone in later commits, swap
> device rotation will happen for every consumed cluster. Ideally non-HDD
> devices will be rotated if 2M space has been consumed for each order.
> Fragmented clusters will rotate the device faster, which seems OK.
> HDD devices is rotated for every allocation regardless of the allocation
> order, which should be OK too and trivial.
>
> This commit also slightly changes allocation behaviour for slot cache.
> The new added cluster allocation fast path may allocate entries from
> different device to the slot cache, this is not observable from user
> space, only impact performance very slightly, and slot cache will be
> just gone in next commit, so this can be ignored.
>
> Signed-off-by: Kairui Song <kasong@...cent.com>
> ---
>  include/linux/swap.h |  11 ++--
>  mm/swapfile.c        | 136 +++++++++++++++++++++++++++++--------------
>  2 files changed, 95 insertions(+), 52 deletions(-)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 2fe91c293636..374bffc87427 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -284,12 +284,10 @@ enum swap_cluster_flags {
>  #endif
>
>  /*
> - * We assign a cluster to each CPU, so each CPU can allocate swap entry from
> - * its own cluster and swapout sequentially. The purpose is to optimize swapout
> - * throughput.
> + * We keep using same cluster for rotational device so IO will be sequential.
> + * The purpose is to optimize SWAP throughput on these device.
>   */
> -struct percpu_cluster {
> -       local_lock_t lock; /* Protect the percpu_cluster above */
> +struct swap_sequential_cluster {
>         unsigned int next[SWAP_NR_ORDERS]; /* Likely next allocation offset */
>  };
>
> @@ -315,8 +313,7 @@ struct swap_info_struct {
>         atomic_long_t frag_cluster_nr[SWAP_NR_ORDERS];
>         unsigned int pages;             /* total of usable pages of swap */
>         atomic_long_t inuse_pages;      /* number of those currently in use */
> -       struct percpu_cluster __percpu *percpu_cluster; /* per cpu's swap location */
> -       struct percpu_cluster *global_cluster; /* Use one global cluster for rotating device */
> +       struct swap_sequential_cluster *global_cluster; /* Use one global cluster for rotating device */
>         spinlock_t global_cluster_lock; /* Serialize usage of global cluster */
>         struct rb_root swap_extent_root;/* root of the swap extent rbtree */
>         struct block_device *bdev;      /* swap device or bdev of swap file */
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index db836670c334..7caaaea95408 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -116,6 +116,18 @@ static atomic_t proc_poll_event = ATOMIC_INIT(0);
>
>  atomic_t nr_rotate_swap = ATOMIC_INIT(0);
>
> +struct percpu_swap_cluster {
> +       struct swap_info_struct *si[SWAP_NR_ORDERS];
> +       unsigned long offset[SWAP_NR_ORDERS];
> +       local_lock_t lock;
> +};
> +
> +static DEFINE_PER_CPU(struct percpu_swap_cluster, percpu_swap_cluster) = {
> +       .si = { NULL },
> +       .offset = { SWAP_ENTRY_INVALID },
> +       .lock = INIT_LOCAL_LOCK(),
> +};
> +
>  static struct swap_info_struct *swap_type_to_swap_info(int type)
>  {
>         if (type >= MAX_SWAPFILES)
> @@ -539,7 +551,7 @@ static bool swap_do_scheduled_discard(struct swap_info_struct *si)
>                 ci = list_first_entry(&si->discard_clusters, struct swap_cluster_info, list);
>                 /*
>                  * Delete the cluster from list to prepare for discard, but keep
> -                * the CLUSTER_FLAG_DISCARD flag, there could be percpu_cluster
> +                * the CLUSTER_FLAG_DISCARD flag, percpu_swap_cluster could be
>                  * pointing to it, or ran into by relocate_cluster.
>                  */
>                 list_del(&ci->list);
> @@ -805,10 +817,12 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si,
>  out:
>         relocate_cluster(si, ci);
>         unlock_cluster(ci);
> -       if (si->flags & SWP_SOLIDSTATE)
> -               __this_cpu_write(si->percpu_cluster->next[order], next);
> -       else
> +       if (si->flags & SWP_SOLIDSTATE) {
> +               __this_cpu_write(percpu_swap_cluster.si[order], si);
> +               __this_cpu_write(percpu_swap_cluster.offset[order], next);
> +       } else {
>                 si->global_cluster->next[order] = next;
> +       }
>         return found;
>  }
>
> @@ -862,9 +876,8 @@ static void swap_reclaim_work(struct work_struct *work)
>  }
>
>  /*
> - * Try to get swap entries with specified order from current cpu's swap entry
> - * pool (a cluster). This might involve allocating a new cluster for current CPU
> - * too.
> + * Try to allocate swap entries with specified order and try set a new
> + * cluster for current CPU too.
>   */
>  static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int order,
>                                               unsigned char usage)
> @@ -872,18 +885,12 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o
>         struct swap_cluster_info *ci;
>         unsigned int offset, found = 0;
>
> -       if (si->flags & SWP_SOLIDSTATE) {
> -               /* Fast path using per CPU cluster */
> -               local_lock(&si->percpu_cluster->lock);
> -               offset = __this_cpu_read(si->percpu_cluster->next[order]);
> -       } else {
> +       if (!(si->flags & SWP_SOLIDSTATE)) {
>                 /* Serialize HDD SWAP allocation for each device. */
>                 spin_lock(&si->global_cluster_lock);
>                 offset = si->global_cluster->next[order];
> -       }
> -
> -       if (offset) {
>                 ci = lock_cluster(si, offset);
> +
>                 /* Cluster could have been used by another order */
>                 if (cluster_is_usable(ci, order)) {
>                         if (cluster_is_empty(ci))
> @@ -973,9 +980,7 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o
>                 }
>         }
>  done:
> -       if (si->flags & SWP_SOLIDSTATE)
> -               local_unlock(&si->percpu_cluster->lock);
> -       else
> +       if (!(si->flags & SWP_SOLIDSTATE))
>                 spin_unlock(&si->global_cluster_lock);
>         return found;
>  }
> @@ -1196,6 +1201,49 @@ static bool get_swap_device_info(struct swap_info_struct *si)
>         return true;
>  }
>
> +/*
> + * Fast path try to get swap entries with specified order from current
> + * CPU's swap entry pool (a cluster).
> + */
> +static int swap_alloc_fast(swp_entry_t entries[],
> +                          unsigned char usage,
> +                          int order, int n_goal)
> +{
> +       struct swap_cluster_info *ci;
> +       struct swap_info_struct *si;
> +       unsigned int offset, found;
> +       int n_ret = 0;
> +
> +       n_goal = min(n_goal, SWAP_BATCH);
> +
> +       /*
> +        * Once allocated, swap_info_struct will never be completely freed,
> +        * so checking it's liveness by get_swap_device_info is enough.
> +        */
> +       si = __this_cpu_read(percpu_swap_cluster.si[order]);
> +       offset = __this_cpu_read(percpu_swap_cluster.offset[order]);
> +       if (!si || !offset || !get_swap_device_info(si))
> +               return 0;

Found one issue with this intermediate patch, "si" will be reused upon
swapoff & swapon again. So after the reuse, get_swap_device_info()
returns true and `offset` is not 0, but `offset` is invalid, it could
point to a value larger than si->max if the previous device is larger.

To fix it, need to squash this onto it:

(also include a code fix for cluster_is_usable check, this code error
is fixed by 7/7 but shouldn't be here in the first place)
(replacing __this_cpu_xx with this_cpu_xx because the swapoff flush
will access other CPU's variable now)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 7caaaea95408..68b40e74be93 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -818,8 +818,8 @@ static unsigned int alloc_swap_scan_cluster(struct
swap_info_struct *si,
        relocate_cluster(si, ci);
        unlock_cluster(ci);
        if (si->flags & SWP_SOLIDSTATE) {
-               __this_cpu_write(percpu_swap_cluster.si[order], si);
-               __this_cpu_write(percpu_swap_cluster.offset[order], next);
+               this_cpu_write(percpu_swap_cluster.si[order], si);
+               this_cpu_write(percpu_swap_cluster.offset[order], next);
        } else {
                si->global_cluster->next[order] = next;
        }
@@ -1220,15 +1220,17 @@ static int swap_alloc_fast(swp_entry_t entries[],
         * Once allocated, swap_info_struct will never be completely freed,
         * so checking it's liveness by get_swap_device_info is enough.
         */
-       si = __this_cpu_read(percpu_swap_cluster.si[order]);
-       offset = __this_cpu_read(percpu_swap_cluster.offset[order]);
+       si = this_cpu_read(percpu_swap_cluster.si[order]);
+       offset = this_cpu_read(percpu_swap_cluster.offset[order]);
        if (!si || !offset || !get_swap_device_info(si))
                return 0;

        while (offset) {
                ci = lock_cluster(si, offset);
-               if (!cluster_is_usable(ci, order))
+               if (!cluster_is_usable(ci, order)) {
+                       unlock_cluster(ci);
                        break;
+               }
                if (cluster_is_empty(ci))
                        offset = cluster_offset(si, ci);
                found = alloc_swap_scan_cluster(si, ci, offset, order, usage);
@@ -1237,7 +1239,7 @@ static int swap_alloc_fast(swp_entry_t entries[],
                entries[n_ret++] = swp_entry(si->type, found);
                if (n_ret == n_goal)
                        break;
-               offset = __this_cpu_read(percpu_swap_cluster.offset[order]);
+               offset = this_cpu_read(percpu_swap_cluster.offset[order]);
        }

        put_swap_device(si);
@@ -2660,6 +2662,27 @@ static void wait_for_allocation(struct
swap_info_struct *si)
        }
 }

+/*
+ * Called after swap devices reference count is dead, so
+ * neither scan or allocation will go into it.
+ */
+static void flush_percpu_swap_cluster(struct swap_info_struct *si)
+{
+       int cpu;
+       struct swap_info_struct **pcp_si;
+
+       for_each_possible_cpu(cpu) {
+               pcp_si = per_cpu_ptr(percpu_swap_cluster.si, cpu);
+               /*
+                * Invalidate the percpu swap cluster, si->users
+                * is dead, so no new users will point to it, flush any
+                * existing cache is enough.
+                */
+               cmpxchg(pcp_si, si, NULL);
+       }
+}
+
+
 SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
 {
        struct swap_info_struct *p = NULL;
@@ -2761,6 +2784,7 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)

        flush_work(&p->discard_work);
        flush_work(&p->reclaim_work);
+       flush_percpu_swap_cluster(p);

        destroy_swap_extents(p);
        if (p->flags & SWP_CONTINUED)

---

There will be minor conflict in the next commit after this squash, I
will send a V3 shortly to resolve this if there is no objection on
this.

Powered by blists - more mailing lists