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

[Yosry Ahmed]

On Fri, Jul 31, 2026 at 08:32:47AM +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.
> 
> A later change wants to store a reference to each entry's pool in every
> zswap_entry, where a pointer would cost 8 bytes but a small pool index
> only one.  To make that index possible, the current change holds the
> pools in a fixed ZSWAP_MAX_POOLS-element array so each pool has a stable
> slot number, and tracks the current pool with a separate rcu-protected
> pointer.
> 
> The array keeps the same RCU publish/retire discipline the list had, so
> lookup and teardown stay equivalent.  Newly created pools are published
> into the array before they become current, so zswap_total_pages() can
> observe an empty pool briefly; that is harmless.

I don't follow. A newly created pool is empty anyway, so not being
iterated in zswap_total_pages() should be normal. Why do we need to call
this out?

> This also caps the
> number of live pools at ZSWAP_MAX_POOLS (16), which is plenty in
> practice; pool creation warns and fails if the array ever fills, and the
> limit can be raised.
> 
> Suggested-by: Nhat Pham <nphamcs@xxxxxxxxx>
> Suggested-by: Yosry Ahmed <yosry@xxxxxxxxxx>
> Signed-off-by: Jianyue Wu <wujianyue000@xxxxxxxxx>
> ---
>  mm/zswap.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 67 insertions(+), 21 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 4e76a4a87cdc..b203934d3be8 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -154,12 +154,20 @@ struct zswap_pool {
>  	struct zs_pool *zs_pool;
>  	struct crypto_acomp_ctx __percpu *acomp_ctx;
>  	struct percpu_ref ref;
> -	struct list_head list;
>  	struct work_struct release_work;
>  	struct hlist_node node;
> +	u8 idx;
>  	char tfm_name[CRYPTO_MAX_ALG_NAME];
>  };
>  
> +#define ZSWAP_MAX_POOLS 16
> +static struct zswap_pool __rcu *zswap_pools[ZSWAP_MAX_POOLS];
> +/*
> + * The current pool (NULL if none): an alias of one zswap_pools[] slot.  It
> + * always holds a ref, so it is never retired from under us.
> + */
> +static struct zswap_pool __rcu *zswap_current_pool;
> +
>  /* Global LRU lists shared by all zswap pools. */
>  static struct list_lru zswap_list_lru;
>  
> @@ -200,9 +208,7 @@ struct zswap_entry {
>  static struct xarray *zswap_trees[MAX_SWAPFILES];
>  static unsigned int nr_zswap_trees[MAX_SWAPFILES];
>  
> -/* RCU-protected iteration */
> -static LIST_HEAD(zswap_pools);
> -/* protects zswap_pools list modification */
> +/* protects the zswap_pools array and zswap_current_pool */
>  static DEFINE_SPINLOCK(zswap_pools_lock);
>  /* pool counter to provide unique names to zsmalloc */
>  static atomic_t zswap_pools_count = ATOMIC_INIT(0);
> @@ -270,6 +276,25 @@ static void acomp_ctx_free(struct crypto_acomp_ctx *acomp_ctx)
>  	acomp_ctx->buffer = NULL;
>  }
>  
> +static int zswap_pool_reserve_slot(struct zswap_pool *pool)
> +{
> +	int i, ret = -ENOSPC;
> +
> +	spin_lock_bh(&zswap_pools_lock);
> +	for (i = 0; i < ZSWAP_MAX_POOLS; i++) {
> +		if (!rcu_access_pointer(zswap_pools[i])) {
> +			/* Set idx before publishing so readers never see it stale. */
> +			pool->idx = i;
> +			rcu_assign_pointer(zswap_pools[i], pool);

Sashiko points out a seemingly real problem here because we add the pool
to the array before actually making it the current pool.

https://sashiko.dev/#/patchset/20260731-shrink_zswap_entry_v2-0-0-v2-0-e72083aa8734%40gmail.com

What if we just reserve an index here but not actually assign the pool?
We can add a marker to the array or sth (e.g. (void *)-1UL)).


> +			ret = i;
> +			break;
> +		}
> +	}
> +	spin_unlock_bh(&zswap_pools_lock);
> +
> +	return ret;
> +}
> +
>  static struct zswap_pool *zswap_pool_create(char *compressor)
>  {
>  	struct zswap_pool *pool;
> @@ -313,19 +338,27 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
>  	if (ret)
>  		goto cpuhp_add_fail;
>  
> -	/* being the current pool takes 1 ref; this func expects the
> -	 * caller to always add the new pool as the current pool
> +	/*
> +	 * After a successful create, the caller makes this the current pool.
> +	 * If the caller fails, it kills the ref to free the reserved slot.
>  	 */
>  	ret = percpu_ref_init(&pool->ref, __zswap_pool_empty,
>  			      PERCPU_REF_ALLOW_REINIT, GFP_KERNEL);
>  	if (ret)
>  		goto ref_fail;
> -	INIT_LIST_HEAD(&pool->list);
> +
> +	ret = zswap_pool_reserve_slot(pool);
> +	if (ret < 0) {
> +		pr_err("cannot create more than %d pools\n", ZSWAP_MAX_POOLS);
> +		goto slot_fail;
> +	}
>  
>  	zswap_pool_debug("created", pool);
>  
>  	return pool;
>  
> +slot_fail:
> +	percpu_ref_exit(&pool->ref);
>  ref_fail:
>  	cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &pool->node);
>  
> @@ -388,7 +421,7 @@ static void __zswap_pool_release(struct work_struct *work)
>  	WARN_ON(!percpu_ref_is_zero(&pool->ref));
>  	percpu_ref_exit(&pool->ref);
>  
> -	/* pool is now off zswap_pools list and has no references. */
> +	/* Slot cleared in __zswap_pool_empty(); synchronize_rcu() drained readers. */
>  	zswap_pool_destroy(pool);
>  }
>  
> @@ -404,7 +437,11 @@ static void __zswap_pool_empty(struct percpu_ref *ref)
>  
>  	WARN_ON(pool == zswap_pool_current());
>  
> -	list_del_rcu(&pool->list);
> +	/*
> +	 * Clear the slot before scheduling the release so new readers cannot
> +	 * see it; __zswap_pool_release()'s synchronize_rcu() drains the rest.
> +	 */
> +	rcu_assign_pointer(zswap_pools[pool->idx], NULL);
>  
>  	INIT_WORK(&pool->release_work, __zswap_pool_release);
>  	schedule_work(&pool->release_work);

Not related to this change, but I wonder if we can use call_rcu() or
similar here instead of the manual synchronize_rcu().

> @@ -435,7 +472,8 @@ static struct zswap_pool *__zswap_pool_current(void)
>  {
>  	struct zswap_pool *pool;
>  
> -	pool = list_first_or_null_rcu(&zswap_pools, typeof(*pool), list);
> +	pool = rcu_dereference_check(zswap_current_pool,
> +				     lockdep_is_held(&zswap_pools_lock));
>  	WARN_ONCE(!pool && zswap_has_pool,
>  		  "%s: no page storage pool!\n", __func__);
>  
> @@ -468,11 +506,14 @@ static struct zswap_pool *zswap_pool_current_get(void)
>  static struct zswap_pool *zswap_pool_find_get(char *compressor)
>  {
>  	struct zswap_pool *pool;
> +	int i;
>  
>  	assert_spin_locked(&zswap_pools_lock);
>  
> -	list_for_each_entry_rcu(pool, &zswap_pools, list) {
> -		if (strcmp(pool->tfm_name, compressor))
> +	for (i = 0; i < ZSWAP_MAX_POOLS; i++) {
> +		pool = rcu_dereference_protected(zswap_pools[i],
> +					lockdep_is_held(&zswap_pools_lock));

We already have an assertion that we are holding the lock above.

> +		if (!pool || strcmp(pool->tfm_name, compressor))
>  			continue;
>  		/* if we can't get it, it's about to be destroyed */
>  		if (!zswap_pool_tryget(pool))