Re: [PATCH RFC] mm/zswap: reference the pool by index to shrink struct zswap_entry

[Yosry Ahmed]

On Sun, Jul 26, 2026 at 8:18 AM Jianyue Wu <wujianyue000@xxxxxxxxx> wrote:
>
> Every stored page has a struct zswap_entry, so its size is a per-page cost
> that scales with how many pages zswap holds.  It currently embeds an
> 8-byte pool pointer.
>
> Replace the pointer with a u16 index into an allocating xarray of pools.
> The index is allocated with the pool and remains valid while entries hold
> references to that pool.

Technically we are introducing a new limit on the number of pools, so
let's mention that. I doubt anyone actually cares, you would need to
switch the compressor 65K times at runtime to run into a problem.

>
> On x86_64, the zswap_entry slab shrinks from 56 to 48 bytes per object,
> raising objs_per_slab from 73 to 85.  This saves about 2MiB of metadata
> per 1GiB of data held in zswap.

Nice :)

>
> Signed-off-by: Jianyue Wu <wujianyue000@xxxxxxxxx>
> ---
> This RFC proposes replacing the per-entry zswap_pool pointer with a u16
> index into an allocating xarray of pools, so struct zswap_entry shrinks
> on 64-bit.
>
> I am looking for feedback on whether the metadata saving justifies the
> extra indirection on entry decompression and freeing.

Did you try to benchmark it? I doubt the indirection would actually
show up as most use cases should have a single pool. It would be
interesting if some use cases actively switch the compressor at
runtime enough for this to be a problem.

> ---
>  mm/zswap.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 4e76a4a87cdc..6c29098da8d5 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -35,6 +35,7 @@
>  #include <linux/pagemap.h>
>  #include <linux/workqueue.h>
>  #include <linux/list_lru.h>
> +#include <linux/xarray.h>
>  #include <linux/zsmalloc.h>
>
>  #include "swap.h"
> @@ -157,9 +158,22 @@ struct zswap_pool {
>         struct list_head list;
>         struct work_struct release_work;
>         struct hlist_node node;
> +       u16 idx;
>         char tfm_name[CRYPTO_MAX_ALG_NAME];
>  };
>
> +/*
> + * Entries store a u16 pool index instead of an 8-byte pool pointer.
> + * Indices are allocated here at pool creation and released at destruction.
> + * Live entries pin the pool, so its index is not reused under them.
> + */
> +static DEFINE_XARRAY_ALLOC(zswap_pools_xa);
> +
> +static struct zswap_pool *zswap_pool_by_index(u16 idx)
> +{
> +       return xa_load(&zswap_pools_xa, idx);
> +}
> +
>  /* Global LRU lists shared by all zswap pools. */
>  static struct list_lru zswap_list_lru;
>
> @@ -182,7 +196,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 - zswap_pool index, resolved via zswap_pool_by_index().

No need to document the function name here, most probably will be
stale at some point.

>   * 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.
> @@ -191,12 +205,20 @@ struct zswap_entry {
>         swp_entry_t swpentry;
>         unsigned int length;
>         bool referenced;
> -       struct zswap_pool *pool;
> +       u16 pool_idx;
>         unsigned long handle;
>         struct obj_cgroup *objcg;
>         struct list_head lru;
>  };
>
> +static struct zswap_pool *zswap_entry_pool(struct zswap_entry *entry)
> +{
> +       struct zswap_pool *pool = zswap_pool_by_index(entry->pool_idx);

This seems like the only user of zswap_pool_by_index(), just open-code
the xa_load() here.

> +       VM_WARN_ON_ONCE(!pool);
> +       return pool;
> +}
> +
>  static struct xarray *zswap_trees[MAX_SWAPFILES];
>  static unsigned int nr_zswap_trees[MAX_SWAPFILES];
>
> @@ -275,6 +297,7 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
>         struct zswap_pool *pool;
>         char name[38]; /* 'zswap' + 32 char (max) num + \0 */
>         int ret, cpu;
> +       u32 idx;
>
>         if (!zswap_has_pool && !strcmp(compressor, ZSWAP_PARAM_UNSET))
>                 return NULL;
> @@ -322,10 +345,18 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
>                 goto ref_fail;
>         INIT_LIST_HEAD(&pool->list);
>
> +       ret = xa_alloc(&zswap_pools_xa, &idx, pool, xa_limit_16b,
> +                      GFP_KERNEL);

I would let this poke out, 80 chars is not a hard limit.

> +       if (ret)
> +               goto idx_fail;
> +       pool->idx = idx;
> +
>         zswap_pool_debug("created", pool);
>
>         return pool;
>
> +idx_fail:
> +       percpu_ref_exit(&pool->ref);
>  ref_fail:
>         cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &pool->node);
>
> @@ -366,6 +397,8 @@ static void zswap_pool_destroy(struct zswap_pool *pool)
>
>         zswap_pool_debug("destroying", pool);
>
> +       xa_erase(&zswap_pools_xa, pool->idx);
> +
>         cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &pool->node);
>
>         for_each_possible_cpu(cpu)
> @@ -764,9 +797,11 @@ 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(&zswap_list_lru, entry);
> -       zs_free(entry->pool->zs_pool, entry->handle);
> -       zswap_pool_put(entry->pool);
> +       zs_free(pool->zs_pool, entry->handle);
> +       zswap_pool_put(pool);

Let's add a NULL check here to avoid blowing up if something goes wrong.

>         if (entry->objcg) {
>                 obj_cgroup_uncharge_zswap(entry->objcg, entry->length);
>                 obj_cgroup_put(entry->objcg);
> @@ -923,7 +958,7 @@ 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;

We need a NULL check here as well.

> @@ -964,7 +999,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;
>  }
> @@ -1457,7 +1492,7 @@ static bool zswap_store_page(struct page *page,
>          *    The publishing order matters to prevent writeback from seeing
>          *    an incoherent entry.
>          */
> -       entry->pool = pool;
> +       entry->pool_idx = pool->idx;
>         entry->swpentry = page_swpentry;
>         entry->objcg = objcg;
>         entry->referenced = true;
>
> ---
> base-commit: c68210527d235e59afa54bc6aba6ecc532488a82
> change-id: 20260726-shrink_zswap_entry_v1-0-0-f21e34cc9214
>
> Best regards,
> --
> Jianyue Wu <wujianyue000@xxxxxxxxx>
>