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

[Yosry Ahmed]

> @@ -270,6 +276,25 @@ static void acomp_ctx_free(struct crypto_acomp_ctx *acomp_ctx)
>         acomp_ctx->buffer = NULL;
>  }
>
> +static int zswap_pool_reserve_slot(struct zswap_pool *pool)
> +{
> +       int i, ret = -ENOSPC;
> +
> +       spin_lock_bh(&zswap_pools_lock);
> +       for (i = 0; i < ZSWAP_MAX_POOLS; i++) {
> +               if (!rcu_access_pointer(zswap_pools[i])) {
> +                       /* Set idx before publishing so readers never see it stale. */
> +                       pool->idx = i;
> +                       rcu_assign_pointer(zswap_pools[i], pool);
> +                       ret = i;
> +                       break;
> +               }
> +       }
> +       spin_unlock_bh(&zswap_pools_lock);

Can we use guard(spinlock_bh) here to avoid the intermediate 'ret'
variable and simplify the logic a bit?

> +
> +       return ret;
> +}
> +
>  static struct zswap_pool *zswap_pool_create(char *compressor)
>  {
>         struct zswap_pool *pool;
[..]
> @@ -1763,6 +1806,9 @@ static int zswap_setup(void)
>         struct zswap_pool *pool;
>         int ret;
>
> +       /* Slot indices are stored in a u8 (pool->idx). */
> +       BUILD_BUG_ON(ZSWAP_MAX_POOLS - 1 > U8_MAX);

Can this be a static_assert() placed right after zswap_pools (and
struct zswap_pool) are defined? It would be more obvious and we won't
need the comment.

I am also wondering whether we should keep slot 0 always unused
(NULL). I am usually a bit paranoid, but it worries me a bit that an
uninitialized (or incorrectly initialized) pool or entry will point at
slot 0 by default, potentially a different pool. What do you (and
others) think?

A nice side effect is that in patch 2, we can use entry->pool_idx for
the rcu_dereference_protected() check in zswap_entry_pool().