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

[Jianyue Wu]

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.

I will reword the comment to explain that the slot must be published before
the current-pool pointer.

> Is this a pre-existing bug, or why do we need to hold the lock here now?

No, this was not a pre-existing bug.  The old list_add() ran during setup,
before any RCU readers or runtime compressor updates existed.

I added the lock only because zswap_setup() reused zswap_pool_publish_slot(),
and that helper expects zswap_pools_lock to be held.  The setup path does not
need that locking itself, so I will publish both zswap_pools[pool->idx] and
zswap_current_pool directly there instead.

Best regards,
Jianyue