Re: [PATCH v5 3/3] mm/zswap: reference the pool by id to shrink struct zswap_entry

[Yosry Ahmed]

On Fri, Sep 4, 2026 at 6:25 AM Jianyue Wu <wujianyue000@xxxxxxxxx> wrote:
>
> struct zswap_entry is one allocation per stored page, so its size is
> pure overhead. It currently embeds an 8-byte pool pointer, even though
> the live pools now sit in an allocating xarray keyed by a small integer
> id that fits in a u8.
>
> Replace the per-entry pool pointer with that u8 id and resolve it
> through the xarray with xa_load(). A live entry holds a reference to its
> pool, so the id cannot be reused under it; xa_load() is lockless and
> only needs an rcu_read_lock() section, no zswap_pools_lock. A live entry
> never uses the reserved id 0, so looking up that id resolves to NULL
> and trips a WARN rather than aliasing a live pool.
>
> The u8 fits in the padding after the bool referenced field, shrinking
> the entry from 56 to 48 bytes on 64-bit. This raises objs_per_slab from
> 73 to 85 and saves about 2MiB of metadata per 1GiB of data held in
> zswap.
>
> Suggested-by: Chris Li <chrisl@xxxxxxxxxx>
> Signed-off-by: Jianyue Wu <wujianyue000@xxxxxxxxx>
> ---
>  mm/zswap.c | 45 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 74876acfa9dc..31cf0ef43d23 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -195,7 +195,7 @@ static struct shrinker *zswap_shrinker;
>   *              writeback logic. The entry is only reclaimed by the writeback
>   *              logic if referenced is unset. See comments in the shrinker
>   *              section for context.
> - * pool - the zswap_pool the entry's data is in
> + * pool_idx - id of the zswap_pool that the entry's data is in.
>   * handle - zsmalloc allocation handle that stores the compressed page data
>   * objcg - the obj_cgroup that the compressed memory is charged to
>   * lru - handle to the pool's lru used to evict pages.
> @@ -204,12 +204,27 @@ struct zswap_entry {
>         swp_entry_t swpentry;
>         unsigned int length;
>         bool referenced;
> -       struct zswap_pool *pool;
> +       u8 pool_idx;
>         unsigned long handle;
>         struct obj_cgroup *objcg;
>         struct list_head lru;
>  };
>
> +/*
> + * The pool stays alive after this returns because a stored entry holds a
> + * reference to its pool (taken in zswap_store_page()).
> + */
> +static struct zswap_pool *zswap_entry_pool(struct zswap_entry *entry)
> +{
> +       struct zswap_pool *pool;
> +
> +       rcu_read_lock();
> +       pool = xa_load(&zswap_pools, entry->pool_idx);
> +       rcu_read_unlock();

Doesn't xa_load() already call rcu_read_[un]lock()?

> +
> +       return pool;
> +}
> +
>  static struct xarray *zswap_trees[MAX_SWAPFILES];
>  static unsigned int nr_zswap_trees[MAX_SWAPFILES];
>
> @@ -770,9 +785,13 @@ static void zswap_entry_cache_free(struct zswap_entry *entry)
>   */
>  static void zswap_entry_free(struct zswap_entry *entry)
>  {
> +       struct zswap_pool *pool = zswap_entry_pool(entry);
> +
>         zswap_lru_del(entry);
> -       zs_free(entry->pool->zs_pool, entry->handle);
> -       zswap_pool_put(entry->pool);
> +       if (!WARN_ON_ONCE(!pool)) {
> +               zs_free(pool->zs_pool, entry->handle);
> +               zswap_pool_put(pool);
> +       }
>         if (entry->objcg) {
>                 obj_cgroup_uncharge_zswap(entry->objcg, entry->length);
>                 obj_cgroup_put(entry->objcg);
> @@ -929,12 +948,15 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
>
>  static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
>  {
> -       struct zswap_pool *pool = entry->pool;
> +       struct zswap_pool *pool = zswap_entry_pool(entry);
>         struct scatterlist input[2]; /* zsmalloc returns an SG list 1-2 entries */
>         struct scatterlist output;
>         struct crypto_acomp_ctx *acomp_ctx;
>         int ret = 0, dlen;
>
> +       if (WARN_ON_ONCE(!pool))
> +               return false;
> +
>         acomp_ctx = raw_cpu_ptr(pool->acomp_ctx);
>         mutex_lock(&acomp_ctx->mutex);
>         zs_obj_read_sg_begin(pool->zs_pool, entry->handle, input, entry->length);
> @@ -970,7 +992,7 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
>         pr_alert_ratelimited("Decompression error from zswap (%d:%lu %s %u->%d)\n",
>                                                 swp_type(entry->swpentry),
>                                                 swp_offset(entry->swpentry),
> -                                               entry->pool->tfm_name,
> +                                               pool->tfm_name,
>                                                 entry->length, dlen);
>         return false;
>  }
> @@ -1428,6 +1450,16 @@ static bool zswap_store_page(struct page *page,
>         if (!zswap_compress(page, entry, pool))
>                 goto compress_failed;
>
> +       /*
> +        * Set pool_idx before publishing the entry: compression has
> +        * succeeded and the pool is already pinned by this store, so the id is
> +        * final.  Doing it here (rather than after xa_store()) means the entry
> +        * is never briefly visible with a stale pool_idx left over from slab
> +        * reuse, which zswap_entry_pool() would otherwise resolve to an
> +        * unrelated live pool.
> +        */
> +       entry->pool_idx = pool->idx;
> +
>         old = xa_store(swap_zswap_tree(page_swpentry),
>                        swp_offset(page_swpentry),
>                        entry, GFP_KERNEL);
> @@ -1473,7 +1505,6 @@ static bool zswap_store_page(struct page *page,
>          *    The publishing order matters to prevent writeback from seeing
>          *    an incoherent entry.
>          */
> -       entry->pool = pool;
>         entry->swpentry = page_swpentry;
>         entry->objcg = objcg;
>         entry->referenced = true;
> --
> 2.43.0
>