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

[Yosry Ahmed]

On Sat, Aug 29, 2026 at 5:10 AM Jianyue Wu <wujianyue000@xxxxxxxxx> wrote:
>
> On Thu, Aug 27, 2026 at 12:56 AM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
> >
> > Makes sense.
> >
> > Actually, this led me to rethink the slots reservation mechanism we
> > came up with and the issue that Sashiko brought up initially. I
> > initially thought it still existed, as two racing threads setting the
> > compressor could try to resurrect a newly created pool. However, I
> > realized that's not possible because a module-wide mutex is held while
> > any module param is set. This also covers zswap_enabled_param_set().
> >
> > So IIUC, we cannot really have any concurrency here. Pool creation
> > either happens through params setting (protected by a module-wide
> > mutex) or during initialization. Does this mean that Sashiko's review
> > was a false positive and we can go back to directly assigning array
> > slots at pool creation? If that's the case I am happy we can avoid the
> > complexity (but sorry I led you astray).
>
> Thanks for the patience here. I wanted to check the details before replying.
>
> Agreed, and I confirmed it. param_attr_store() takes
> kernel_param_lock(mk->mod) around ->set, and for a built-in module
> (zswap) that resolves to the single global param_lock
> (KPARAM_MUTEX(mod) -> &param_lock, "Protects all built-in parameters"
> in kernel/params.c). So zswap_compressor_param_set() and
> zswap_enabled_param_set() are mutually serialized, and the only other
> pool-creation path is single-threaded init. There is no window for two
> threads to resurrect or create the same pool concurrently. Sashiko's
> concern was a false positive.
>
> I also tried dropping the slot reserve and went back to assigning
> the real pointer directly at creation. The only invariant that matters
> is that the slot store happens after the pool is fully constructed,
> it's the last step of zswap_pool_create(). So array walkers only ever
> see a NULL slot or a ready pool. zswap_pools_lock
> still serializes that store against a retiring pool clearing its slot
> in __zswap_pool_empty().
>
> I tested with LOCKDEP + PROVE_RCU + DEBUG_ATOMIC_SLEEP: an e2e
> store/load workload + compressor switch (lzo -> zstd -> lzo) that
> exercises both the pool-retire and the pool-reuse/resurrect paths. All
> passed, no lockdep splats and no suspicious-RCU or sleeping-in-atomic
> warnings. These are functional tests only, 2 writer races can't be triggered,
> since param_lock already serializes concurrent param writes.
>
> No problem, the discussion made the concurrency model clearer, and we
> can drop the complexity. I'll refactor the code and send out the patch
> in the next version.
>
> > It also makes me wonder if we can just drop zswap_init_lock, but
> > that's a separate project. Most of the zswap init code needs to be
> > reworked imo.
>
> Agreed it's separate. zswap_init_lock guards the zswap_init_state
> machine and coordinates the two zswap_setup() entry points (the
> late_initcall vs. zswap_enabled_param_set()); the param lock doesn't
> cover the initcall path, so it can't just be dropped. I'll leave that
> out of this series.

Agreed on leaving it out anyway, but doesn't zswap_init_lock guard the
same paths we just convinced ourselves are already mutually exclusive
for the purposes of pool reservation?