Re: [PATCH v3 14/15] mm, swap: implement dynamic allocation of swap table

Chris Mason
2025-09-15T15:55:08Z

On Thu, 11 Sep 2025 00:08:32 +0800 Kairui Song <ryncsn@gmail.com> wrote:

> From: Kairui Song <kasong@tencent.com>
>
> Now swap table is cluster based, which means free clusters can free its
> table since no one should modify it.
>
> There could be speculative readers, like swap cache look up, protect
> them by making them RCU protected. All swap table should be filled with
> null entries before free, so such readers will either see a NULL pointer
> or a null filled table being lazy freed.
>
> On allocation, allocate the table when a cluster is used by any order.
>
> This way, we can reduce the memory usage of large swap device
> significantly.
>
> This idea to dynamically release unused swap cluster data was initially
> suggested by Chris Li while proposing the cluster swap allocator and
> it suits the swap table idea very well.
>
> Co-developed-by: Chris Li <chrisl@kernel.org>
> Signed-off-by: Chris Li <chrisl@kernel.org>
> Signed-off-by: Kairui Song <kasong@tencent.com>
> Acked-by: Chris Li <chrisl@kernel.org>
> ---
> mm/swap.h | 2 +-
> mm/swap_state.c | 9 +--
> mm/swap_table.h | 37 ++++++++-
> mm/swapfile.c | 202 ++++++++++++++++++++++++++++++++++++++----------
> 4 files changed, 199 insertions(+), 51 deletions(-)
>

[ ... ]

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 89659928465e..faf867a6c5c1 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
>

[ ... ]

> +/*
> + * Allocate a swap table may need to sleep, which leads to migration,
> + * so attempt an atomic allocation first then fallback and handle
> + * potential race.
> + */
> +static struct swap_cluster_info *
> +swap_cluster_alloc_table(struct swap_info_struct *si,
> + struct swap_cluster_info *ci,
> + int order)
> {
> - unsigned int ci_off;
> - unsigned long swp_tb;
> + struct swap_cluster_info *pcp_ci;
> + struct swap_table *table;
> + unsigned long offset;
>
> - if (!ci->table)
> - return;
> + /*
> + * Only cluster isolation from the allocator does table allocation.
> + * Swap allocator uses a percpu cluster and holds the local lock.
> + */
> + lockdep_assert_held(&ci->lock);
> + lockdep_assert_held(&this_cpu_ptr(&percpu_swap_cluster)->lock);
> +
> + table = kmem_cache_zalloc(swap_table_cachep,
> + __GFP_HIGH | __GFP_NOMEMALLOC | __GFP_NOWARN);
> + if (table) {
> + rcu_assign_pointer(ci->table, table);
> + return ci;
> + }
> +
> + /*
> + * Try a sleep allocation. Each isolated free cluster may cause
> + * a sleep allocation, but there is a limited number of them, so
> + * the potential recursive allocation should be limited.
> + */
> + spin_unlock(&ci->lock);
> + if (!(si->flags & SWP_SOLIDSTATE))
> + spin_unlock(&si->global_cluster_lock);
> + local_unlock(&percpu_swap_cluster.lock);
> + table = kmem_cache_zalloc(swap_table_cachep, __GFP_HIGH | GFP_KERNEL);
>
> - for (ci_off = 0; ci_off < SWAPFILE_CLUSTER; ci_off++) {
> - swp_tb = __swap_table_get(ci, ci_off);
> - if (!swp_tb_is_null(swp_tb))
> - pr_err_once("swap: unclean swap space on swapoff: 0x%lx",
> - swp_tb);
> + local_lock(&percpu_swap_cluster.lock);
> + if (!(si->flags & SWP_SOLIDSTATE))
> + spin_lock(&si->global_cluster_lock);
> + /*
> + * Back to atomic context. First, check if we migrated to a new
> + * CPU with a usable percpu cluster. If so, try using that instead.
> + * No need to check it for the spinning device, as swap is
> + * serialized by the global lock on them.
> + *
> + * The is_usable check is a bit rough, but ensures order 0 success.
> + */
> + offset = this_cpu_read(percpu_swap_cluster.offset[order]);
> + if ((si->flags & SWP_SOLIDSTATE) && offset) {
> + pcp_ci = swap_cluster_lock(si, offset);
> + if (cluster_is_usable(pcp_ci, order) &&
> + pcp_ci->count < SWAPFILE_CLUSTER) {
> + ci = pcp_ci;
^^^^^^^^^^^^^
ci came from the caller, and in the case of isolate_lock_cluster() they
had just removed it from a list. We overwrite ci and return something
different.

> + goto free_table;
> + }
> + swap_cluster_unlock(pcp_ci);
> }
>
> - kfree(ci->table);
> - ci->table = NULL;
> + if (!table)
> + return NULL;
> +
> + spin_lock(&ci->lock);
> + /* Nothing should have touched the dangling empty cluster. */
> + if (WARN_ON_ONCE(cluster_table_is_alloced(ci)))
> + goto free_table;
> +
> + rcu_assign_pointer(ci->table, table);
> + return ci;
> +
> +free_table:
> + if (table)
> + kmem_cache_free(swap_table_cachep, table);
> + return ci;
> }
>
> static void move_cluster(struct swap_info_struct *si,

[ ... ]

> @@ -513,13 +584,19 @@ static struct swap_cluster_info *isolate_lock_cluster(
>
> list_del(&ci->list);
> ci->flags = CLUSTER_FLAG_NONE;
> - ret = ci;
> + found = ci;

We've pulled ci off the list here.

> break;
> }
> -out:
> spin_unlock(&si->lock);
>
> - return ret;
> + if (found && !cluster_table_is_alloced(found)) {
> + /* Only an empty free cluster's swap table can be freed. */
> + VM_WARN_ON_ONCE(list != &si->free_clusters);
> + VM_WARN_ON_ONCE(!cluster_is_empty(found));
> + return swap_cluster_alloc_table(si, found, order);

swap_cluster_alloc_table() may have switched for a different ci? What
happens to the one we pulled off the list?

-chris