Re: [RFC PATCH v4 2/3] mm/zswap: replace the zswap_pools list with a fixed pools array

[Johannes Weiner]

On Sun, Aug 30, 2026 at 07:47:30PM +0800, Jianyue Wu wrote:
> Originally zswap holds its pools on an RCU list whose head also serves
> as the "current pool". Only a handful of pools are ever live at once,
> since a new pool is only created when the compressor is (re)set and
> pools are reused across compressor switches.
> 
> Hold the pools in a fixed ZSWAP_MAX_POOLS-element array so each pool
> has a stable slot number, and track the current pool with a separate
> rcu-protected pointer.
> 
> Slot 0 is intentionally left unused (always NULL): a zeroed or
> incorrectly initialized pool index then resolves to NULL and trips a
> WARN rather than silently aliasing a live pool in another slot.
> 
> The array keeps the same RCU publish/retire discipline the list had,
> so lookup and teardown stay equivalent. A fully-constructed pool is
> stored into its slot as the last step of zswap_pool_create(), so array
> walkers only ever observe a NULL slot or a ready pool. Pool creation
> is serialized by the module-wide kernel param mutex (all built-in
> params share one lock) and otherwise only happens during
> single-threaded init, so no two creators race for a slot.
> zswap_pools_lock still serializes the store against a retiring pool
> clearing its slot in __zswap_pool_empty().
> 
> Behavior change: the fixed array bounds the number of simultaneously
> live pools at ZSWAP_MAX_POOLS - 1 (15, since slot 0 is reserved),
> whereas the old list was unbounded. A pool is only live while it is
> the current pool or still has stored pages referencing it, and pools
> are reused across compressor switches, so 15 is far more than any real
> configuration needs. Once all slots are occupied, creating a pool for
> a 16th distinct compressor fails: zswap_pool_create() errors and
> returns NULL, and the compressor switch is rejected with -EINVAL
> rather than silently succeeding. The cap can be raised by increasing
> ZSWAP_MAX_POOLS (bounded by the u8 slot index, so up to 256).
> 
> Suggested-by: Nhat Pham <nphamcs@xxxxxxxxx>
> Suggested-by: Yosry Ahmed <yosry@xxxxxxxxxx>
> Signed-off-by: Jianyue Wu <wujianyue000@xxxxxxxxx>
> ---
>  mm/zswap.c | 97 ++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 72 insertions(+), 25 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 0bb30e58950a..b3b5e2887c00 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -13,6 +13,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include <linux/cleanup.h>
>  #include <linux/module.h>
>  #include <linux/cpu.h>
>  #include <linux/highmem.h>
> @@ -154,12 +155,27 @@ struct zswap_pool {
>  	struct zs_pool *zs_pool;
>  	struct crypto_acomp_ctx __percpu *acomp_ctx;
>  	struct percpu_ref ref;
> -	struct list_head list;
>  	struct rcu_work release_work;
>  	struct hlist_node node;
> +	u8 idx;
>  	char tfm_name[CRYPTO_MAX_ALG_NAME];
>  };
>  
> +#define ZSWAP_MAX_POOLS 16

It's unlikely to happen, but this is a super annoying failure
mode. User would have to kill something, delete shmem/tmpfs, or
swapoff. And it's not obvious which entries are in which pool.

Wouldn't an idr make more sense?

> @@ -270,6 +283,31 @@ static void acomp_ctx_free(struct crypto_acomp_ctx *acomp_ctx)
>  	acomp_ctx->buffer = NULL;
>  }
>  
> +/*
> + * Publish a fully-constructed pool into a free array slot.  Pool creation is
> + * serialized by the module-wide kernel param mutex (all built-in params share
> + * one lock) and only otherwise happens during single-threaded init, so no two
> + * creators race for a slot.  The pool is complete before it is stored, and
> + * zswap_pools_lock still serializes this store against a concurrent retiring
> + * pool clearing its slot in __zswap_pool_empty(), so array walkers only ever
> + * observe a NULL slot or a ready pool.
> + */
> +static int zswap_pool_assign_slot(struct zswap_pool *pool)
> +{
> +	int i;
> +
> +	guard(spinlock_bh)(&zswap_pools_lock);
> +	for (i = ZSWAP_FIRST_POOL_SLOT; i < ZSWAP_MAX_POOLS; i++) {
> +		if (!rcu_access_pointer(zswap_pools[i])) {
> +			pool->idx = i;
> +			rcu_assign_pointer(zswap_pools[i], pool);
> +			return i;
> +		}
> +	}
> +
> +	return -ENOSPC;
> +}

It was kind of overdue, but with this now requiring a pool walk as
well, it would be better to factor out a find_or_create function?

Something like:

static struct zswap_pool *zswap_pool_find_or_create(char *compressor)
{
	struct zswap_pool *pool, *new_pool = NULL;
	u8 id, new_id = 0;

insert_new:
	spin_lock_bh(&zswap_pools_lock);
	idr_for_each_entry(&zswap_pools, pool, id) {
		if (pool && !strcmp(pool->tfm_name, compressor) && zswap_pool_tryget(pool)) {
			if (new_pool) {
				pool_put(new_pool);
				idr_free(&zswap_pools, new_id);
			}
			spin_unlock_bh(&zswap_pools_lock);
			return pool;
		}
	}
	if (new_pool) {
		idr_replace(&zswap_pools, new_pool, new_id);
		spin_unlock_bh(&zswap_pools_lock);
		return new_pool;
	}
	spin_unlock_bh(&zswap_pools_lock);

	new_pool = pool_alloc();
	if (!new_pool)
		...
	new_id = idr_alloc(&zswap_pools, NULL, 1, 256, GFP_KERNEL);
	if (new_id < 0)
		...
	goto insert_new;
}