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 8:50 AM Jianyue Wu <wujianyue000@xxxxxxxxx> wrote:
>
> 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

Hello Johannes,

I have this working locally, but I am still testing and some parts still
need clarification.

1. Could I use xarray here? Similar to idr, can naturally start from 1,
and zswap already uses xarray for the entry trees.
i.e. the pool table is an allocating xarray (XA_FLAGS_ALLOC1, ids
1..255, 0 reserved).

2. Compressor updates are already serialized by the param lock,
and the only other create path is zswap_setup() during init (or when
enabling zswap). That does not run alongside another creator, so I
think we can keep the simpler lookup-then-create flow. It is almost
the same as the original list code, just using the xarray API.

Best regards,
Jianyue