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

[Yosry Ahmed]

On Wed, Aug 26, 2026 at 9:08 AM Jianyue Wu <wujianyue000@xxxxxxxxx> wrote:
>
> Thanks for the detailed review.
>
> > I think the reservation logic is self-explaining, the comment here is
> > not warranted. Especially the idx part, I believe we only set
> > pool->idx here to keep track of the reserved index, not really to do
> > it before publishing as publishing happens later. If anything, maybe
> > replace this with a smaller comment to the reader that we reserve the
> > slot at pool creation but only assign it when we make it the current
> > pool, with a small explanation of why that is needed (the actually
> > issue Sashiko pointed out)?
>
> Agreed.  The important part is the race from the v2 review, where
> zswap_pool_find_get() could observe a pool before it was committed as
> current.  I will shorten the comment to only explain the placeholder:
> claim the slot at creation time, publish the real pool only when it becomes
> current.
>
> > I think we can drop the assertion since we already have the lockdep
> > check below, especially because it uses BUG_ON().
>
> Agreed, I will drop it.
>
> > Hmm let's read this into a variable first then compare it to
> > ZSWAP_SLOT_RESERVED instead? It's not very readable.
>
> Agreed, I will read the slot into a local variable first.
>
> > I think this comment is unnecessary.
>
> Agreed, I will drop it.
>
> > Let's drop the assertion here as well since we're adding a lockdep check below.
>
> Agreed, I will drop this one too.
>
> > I would align the second parameter with the first one if it stays
> > below 100 characters.
>
> Sure, I will fix the alignment.
>
> > This doesn't really explain why. Also, does ordering really matter
> > with the lock held?
>
> Yes, the ordering matters because zswap_pools_lock only serializes the
> writers.  zswap_current_pool can be observed by RCU readers without that
> lock.  Publishing the current pointer first would let a reader observe the
> new current pool while its array slot is still ZSWAP_SLOT_RESERVED.

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).

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.