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

[Yosry Ahmed]

On Fri, Aug 14, 2026 at 11:54 PM Jianyue Wu <wujianyue000@xxxxxxxxx> 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 pool first reserves its
> slot with a placeholder marker and only stores the real pointer once
> it is committed as the current pool, so array walkers never observe a
> not-yet-ready pool as live.
>
> 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 | 140 +++++++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 114 insertions(+), 26 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index cc4243356e21..603fdc418041 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,13 +155,42 @@ 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_head rcu_head;
>         struct work_struct release_work;
>         struct hlist_node node;
> +       u8 idx;
>         char tfm_name[CRYPTO_MAX_ALG_NAME];
>  };
>
> +#define ZSWAP_MAX_POOLS 16
> +/*
> + * Slot 0 is intentionally never used: it stays NULL so that a zeroed or
> + * incorrectly initialized pool->idx resolves to NULL (and trips a WARN)
> + * instead of silently aliasing a live pool in another slot.
> + */
> +#define ZSWAP_FIRST_POOL_SLOT 1
> +static struct zswap_pool __rcu *zswap_pools[ZSWAP_MAX_POOLS];
> +static_assert(ZSWAP_MAX_POOLS - 1 <= U8_MAX);
> +/*
> + * The current pool (NULL if none): an alias of one zswap_pools[] slot.
> + * It always holds a ref, so a pool is never retired while it is current.
> + */
> +static struct zswap_pool __rcu *zswap_current_pool;
> +
> +/*
> + * A slot placeholder used to reserve an index before the pool is committed.
> + * A reserving pool publishes this marker first and only stores the real
> + * pointer once it is ready to become current; walkers of zswap_pools[] treat
> + * a reserved slot as empty and skip it, so a not-yet-ready pool is never
> + * observed as live.
> + */
> +#define ZSWAP_SLOT_RESERVED ((struct zswap_pool *)-1UL)
> +
> +static inline bool zswap_slot_is_pool(struct zswap_pool *pool)
> +{
> +       return pool && pool != ZSWAP_SLOT_RESERVED;
> +}
> +
>  /* Global LRU lists shared by all zswap pools. */
>  static struct list_lru zswap_list_lru;
>
> @@ -201,9 +231,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);
> @@ -271,6 +299,40 @@ 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;
> +
> +       guard(spinlock_bh)(&zswap_pools_lock);
> +       for (i = ZSWAP_FIRST_POOL_SLOT; i < ZSWAP_MAX_POOLS; i++) {
> +               if (!rcu_access_pointer(zswap_pools[i])) {
> +                       /*
> +                        * Reserve the slot with a placeholder rather than the
> +                        * pool itself: the pool is not ready to be current yet
> +                        * and must not be observed as live by array walkers.
> +                        * Set idx before publishing so readers never see it
> +                        * stale.  zswap_pool_publish_slot() stores the real
> +                        * pointer once the pool is committed.
> +                        */

I think the reservation logic is self-explaining, the comment here is
not warranted. Especially the idx part, I believe we only set
pool->idx here to keep track of the reserved index, not really to do
it before publishing as publishing happens later. If anything, maybe
replace this with a smaller comment to the reader that we reserve the
slot at pool creation but only assign it when we make it the current
pool, with a small explanation of why that is needed (the actually
issue Sashiko pointed out)?


> +                       pool->idx = i;
> +                       rcu_assign_pointer(zswap_pools[i], ZSWAP_SLOT_RESERVED);
> +                       return i;
> +               }
> +       }
> +
> +       return -ENOSPC;
> +}
> +
> +static void zswap_pool_publish_slot(struct zswap_pool *pool)
> +{
> +       assert_spin_locked(&zswap_pools_lock);

I think we can drop the assertion since we already have the lockdep
check below, especially because it uses BUG_ON().

> +
> +       if (rcu_dereference_protected(zswap_pools[pool->idx],
> +                                     lockdep_is_held(&zswap_pools_lock)) ==
> +           ZSWAP_SLOT_RESERVED)

Hmm let's read this into a variable first then compare it to
ZSWAP_SLOT_RESERVED instead? It's not very readable.

> +               rcu_assign_pointer(zswap_pools[pool->idx], pool);
> +}
> +
>  static struct zswap_pool *zswap_pool_create(char *compressor)
>  {
>         struct zswap_pool *pool;
> @@ -314,19 +376,28 @@ 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
> +       /*
> +        * The initial ref is for the current-pool role.  On success the
> +        * caller must install this pool as the current pool.
>          */
>         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 - ZSWAP_FIRST_POOL_SLOT);
> +               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);
>
> @@ -387,7 +458,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. */
> +       /* readers drained by the grace period */
>         zswap_pool_destroy(pool);
>  }
>
> @@ -397,9 +468,9 @@ static void __zswap_pool_release_rcu(struct rcu_head *head)
>
>         /*
>          * The grace period has elapsed, so no RCU reader can still observe the
> -        * pool through the list it was removed from in __zswap_pool_empty().
> -        * Hand off to a worker for the sleepable teardown, since this callback
> -        * runs in softirq context.
> +        * pool through the array slot cleared in __zswap_pool_empty().  Hand off
> +        * to a worker for the sleepable teardown, since this callback runs in
> +        * softirq context.
>          */
>         INIT_WORK(&pool->release_work, __zswap_pool_release);
>         schedule_work(&pool->release_work);
> @@ -417,7 +488,13 @@ static void __zswap_pool_empty(struct percpu_ref *ref)
>
>         WARN_ON(pool == zswap_pool_current());
>
> -       list_del_rcu(&pool->list);
> +       /*
> +        * Clear the slot before retiring the pool so new readers cannot see
> +        * it; the call_rcu() below drains readers that already observed it.
> +        * The slot may still hold the ZSWAP_SLOT_RESERVED placeholder if the
> +        * pool is torn down before it was ever published as current.
> +        */

I think this comment is unnecessary.

> +       rcu_assign_pointer(zswap_pools[pool->idx], NULL);
>
>         call_rcu(&pool->rcu_head, __zswap_pool_release_rcu);
>
> @@ -447,7 +524,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__);
>
> @@ -480,11 +558,15 @@ 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);

Let's drop the assertion here as well since we're adding a lockdep check below.

>
> -       list_for_each_entry_rcu(pool, &zswap_pools, list) {
> -               if (strcmp(pool->tfm_name, compressor))
> +       for (i = ZSWAP_FIRST_POOL_SLOT; i < ZSWAP_MAX_POOLS; i++) {
> +               pool = rcu_dereference_protected(zswap_pools[i],
> +                                                lockdep_is_held(&zswap_pools_lock));

I would align the second parameter with the first one if it stays
below 100 characters.

> +               if (!zswap_slot_is_pool(pool) ||
> +                   strcmp(pool->tfm_name, compressor))
>                         continue;
>                 /* if we can't get it, it's about to be destroyed */
>                 if (!zswap_pool_tryget(pool))
> @@ -509,10 +591,14 @@ unsigned long zswap_total_pages(void)
>  {
>         struct zswap_pool *pool;
>         unsigned long total = 0;
> +       int i;
>
>         rcu_read_lock();
> -       list_for_each_entry_rcu(pool, &zswap_pools, list)
> -               total += zs_get_total_pages(pool->zs_pool);
> +       for (i = ZSWAP_FIRST_POOL_SLOT; i < ZSWAP_MAX_POOLS; i++) {
> +               pool = rcu_dereference(zswap_pools[i]);
> +               if (zswap_slot_is_pool(pool))
> +                       total += zs_get_total_pages(pool->zs_pool);
> +       }
>         rcu_read_unlock();
>
>         return total;
> @@ -574,7 +660,6 @@ static int zswap_compressor_param_set(const char *val, const struct kernel_param
>         if (pool) {
>                 zswap_pool_debug("using existing", pool);
>                 WARN_ON(pool == zswap_pool_current());
> -               list_del_rcu(&pool->list);
>         }
>
>         spin_unlock_bh(&zswap_pools_lock);
> @@ -601,16 +686,16 @@ static int zswap_compressor_param_set(const char *val, const struct kernel_param
>         spin_lock_bh(&zswap_pools_lock);
>
>         if (!ret) {
> +               /*
> +                * Remember the old current, then publish the new pool into its
> +                * slot before making it current: array walkers must never see
> +                * the current pool as a reserved (not-yet-ready) slot.
> +                */

This doesn't really explain why. Also, does ordering really matter
with the lock held?

>                 put_pool = zswap_pool_current();
> -               list_add_rcu(&pool->list, &zswap_pools);
> +               zswap_pool_publish_slot(pool);
> +               rcu_assign_pointer(zswap_current_pool, pool);
>                 zswap_has_pool = true;
>         } else if (pool) {
> -               /*
> -                * Add the possibly pre-existing pool to the end of the pools
> -                * list; if it's new (and empty) then it'll be removed and
> -                * destroyed by the put after we drop the lock
> -                */
> -               list_add_tail_rcu(&pool->list, &zswap_pools);
>                 put_pool = pool;
>         }
>
> @@ -1815,7 +1900,10 @@ static int zswap_setup(void)
>         pool = __zswap_pool_create_fallback();
>         if (pool) {
>                 pr_info("loaded using pool %s\n", pool->tfm_name);
> -               list_add(&pool->list, &zswap_pools);
> +               spin_lock_bh(&zswap_pools_lock);
> +               zswap_pool_publish_slot(pool);
> +               rcu_assign_pointer(zswap_current_pool, pool);
> +               spin_unlock_bh(&zswap_pools_lock);

Is this a pre-existing bug, or why do we need to hold the lock here now?

>                 zswap_has_pool = true;
>                 static_branch_enable(&zswap_ever_enabled);
>         } else {
>
> --
> 2.43.0
>