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

[Jianyue Wu]

On Mon, Aug 31, 2026 at 11:18 PM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
>
> 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?

Ah, yes, exactly, param_lock already covered param writers, and mutually
exclusive. initcall also precedes userspace sysfs write, so:
1. zswap_init_state: initcall has resolved the state, reader is fine unlocked.
2. zswap_has_pool: param_lock covered. Writer also holds zswap_pools_lock.
3. *(char **)kp->arg: param_lock covered. Write by param_set_charp().

Best regards,
Jianyue