Re: [PATCH v5 2/3] mm/zswap: replace the zswap_pools list with an allocating xarray

[Jianyue Wu]

On Sat, Sep 5, 2026 at 12:04 AM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
>
> > -       list_del_rcu(&pool->list);
> > +       __xa_erase(&zswap_pools, pool->idx);
> >
> >         INIT_RCU_WORK(&pool->release_rwork, __zswap_pool_release);
> >         queue_rcu_work(system_percpu_wq, &pool->release_rwork);
> >
> > -       spin_unlock_bh(&zswap_pools_lock);
> > +       xa_unlock_bh(&zswap_pools);
>
> Do we need to call queue_rcu_work() under the lock? I assume not. Can
> we just call xa_erase_bh()?

Right, no need. queue_rcu_work() only touches the retiring pool's own
release_rwork. xa_erase_bh(&zswap_pools, pool->idx) is enough.

>
> Doesn't xa_for_each() already handle RCU locking?
>
> >         rcu_read_lock();
> > -       list_for_each_entry_rcu(pool, &zswap_pools, list)
> > +       xa_for_each(&zswap_pools, id, pool)
> >                 total += zs_get_total_pages(pool->zs_pool);
> >         rcu_read_unlock();

xa_for_each() only takes RCU around each internal lookup, not across the
loop body, so the explicit rcu_read_lock() is still needed to keep a
concurrently retired pool alive while we dereference pool->zs_pool.

> > @@ -554,20 +584,17 @@ static int zswap_compressor_param_set(const char *val, const struct kernel_param
> >                 return -ENOENT;
> >         }
> >
> > -       spin_lock_bh(&zswap_pools_lock);
> > -
> > +       xa_lock_bh(&zswap_pools);
> >         pool = zswap_pool_find_get(s);
>
> This is the only caller of zswap_pool_find_get(), and since we remove
> list_del_rcu() below we have no reason for holding the lock here other
> than zswap_pool_find_get(). So let's move the locking inside? I am
> also not really sure if we actually need to hold the lock here anymore
> to begin with.

Agreed, and it turns out we don't need xa_lock here at all.

> > @@ -584,23 +611,17 @@ static int zswap_compressor_param_set(const char *val, const struct kernel_param
> >         else
> >                 ret = -EINVAL;
> >
> > -       spin_lock_bh(&zswap_pools_lock);
> > +       xa_lock_bh(&zswap_pools);
>
> Do we still need to hold the lock here?
>

Agreed, no need lock here. Compressor switches are serialized by the
module parameter lock, so this is the only writer of zswap_current_pool.

Thanks for the review!

Best regards,
Jianyue