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

[Chris Mason <clm@meta.com>]

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