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

[Jianyue Wu]

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.

Best regards,
Jianyue