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

[Jianyue Wu]

On Wed, Sep 2, 2026 at 12:13 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
>
> 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;
> }
Agreed. Hitting the 15-pool cap would be an annoying failure mode, even
if it should be rare.
I will switch this to an IDR with IDs in the 1..255 range, keeping 0 as
the invalid entry value, and factor the lookup/allocation path into a
zswap_pool_find_or_create() helper.

Best regards,
Jianyue