[PATCH RFC v2 0/2] mm/zswap: shrink zswap_entry via a fixed pool index

[PATCH RFC v2 0/2] mm/zswap: shrink zswap_entry via a fixed pool index

[Jianyue Wu]

Every stored page has a struct zswap_entry, so its size is pure per-page
overhead.  On x86_64 it is currently 56 bytes, of which 8 bytes are a
pointer to the owning zswap_pool.

Only a handful of pools are ever live: a new pool is created only when
the compressor is (re)set, and pools are reused across compressor
switches.  That makes a per-entry pool pointer more expensive than it
needs to be, and the RCU list that currently tracks pools is more
machinery than this needs once each pool already has a stable slot.

This series replaces the list with a fixed array of ZSWAP_MAX_POOLS (16)
slots, tracks the current pool with a separate RCU-protected pointer,
then stores a u8 slot index in each entry instead of the pool pointer.
The u8 fits in padding after the bool referenced field, so the entry
shrinks from 56 to 48 bytes on x86_64 (~2MiB of metadata saved per 1GiB
of data held in zswap).  Runtime compressor switching is preserved;
creating a 17th pool fails and warns.

Patch 1 is the list -> array conversion.  Patch 2 is the per-entry
shrink.  Looking for feedback on whether this is the right shape before
sending a non-RFC version.

Benchmark (x86_64, compressor=lzo, MADV_PAGEOUT store + fault-in load):

  - zswap_entry object_size: 56 -> 48 bytes
  - e2e store+load median latency: no measurable regression vs baseline
    at matched stored_delta

The extra cost per store/free/decompress is one array-index load instead
of a pointer dereference.  With a single (or few) live pool(s) that does
not show up against (de)compression.

Changes from RFC v1:
  - Drop the allocating xarray; use a fixed array of 16 pools indexed by
    a u8, as suggested by Nhat and Yosry.
  - Drop the zswap_pools list and the per-pool list_head; the array
    carries stable slot numbers for entries, and a separate RCU pointer
    tracks the current pool.
  - Document the new pool-count cap; fail/warn when the array is full.
  - Clear the array slot in __zswap_pool_empty() before scheduling the
    release work (equivalent to the old list_del_rcu()), then
    synchronize_rcu() before free.
  - Use rcu_assign_pointer() / rcu_dereference*() for slot and current-
    pool publish/load.  Entry lookup uses rcu_dereference_protected()
    because a live entry already pins its pool.
  - Add NULL checks on the resolved pool in free/decompress.

Link: https://lore.kernel.org/all/20260726-shrink_zswap_entry_v1-0-0-v1-1-30957e4d0cb6@gmail.com/

Signed-off-by: Jianyue Wu <wujianyue000@gmail.com>
---
Jianyue Wu (2):
      mm/zswap: replace the zswap_pools list with a fixed pools array
      mm/zswap: reference the pool by index to shrink struct zswap_entry

 mm/zswap.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 91 insertions(+), 28 deletions(-)
---
base-commit: 2ed26e8a624f41887d64e2e37a2f6ab36a118d1f
change-id: 20260731-shrink_zswap_entry_v2-0-0-76ac3af12ec4

Best regards,
-- 
Jianyue Wu <wujianyue000@gmail.com>

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

[Jianyue Wu]

Originally zswap holds its pools on an RCU list whose head also serves as
the "current pool".  Only a handful of pools are ever live at once, since
a new pool is only created when the compressor is (re)set and pools are
reused across compressor switches.

A later change wants to store a reference to each entry's pool in every
zswap_entry, where a pointer would cost 8 bytes but a small pool index
only one.  To make that index possible, the current change holds the
pools in a fixed ZSWAP_MAX_POOLS-element array so each pool has a stable
slot number, and tracks the current pool with a separate rcu-protected
pointer.

The array keeps the same RCU publish/retire discipline the list had, so
lookup and teardown stay equivalent.  Newly created pools are published
into the array before they become current, so zswap_total_pages() can
observe an empty pool briefly; that is harmless.  This also caps the
number of live pools at ZSWAP_MAX_POOLS (16), which is plenty in
practice; pool creation warns and fails if the array ever fills, and the
limit can be raised.

Suggested-by: Nhat Pham <nphamcs@gmail.com>
Suggested-by: Yosry Ahmed <yosry@kernel.org>
Signed-off-by: Jianyue Wu <wujianyue000@gmail.com>
---
 mm/zswap.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 67 insertions(+), 21 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 4e76a4a87cdc..b203934d3be8 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -154,12 +154,20 @@ struct zswap_pool {
 	struct zs_pool *zs_pool;
 	struct crypto_acomp_ctx __percpu *acomp_ctx;
 	struct percpu_ref ref;
-	struct list_head list;
 	struct work_struct release_work;
 	struct hlist_node node;
+	u8 idx;
 	char tfm_name[CRYPTO_MAX_ALG_NAME];
 };
 
+#define ZSWAP_MAX_POOLS 16
+static struct zswap_pool __rcu *zswap_pools[ZSWAP_MAX_POOLS];
+/*
+ * The current pool (NULL if none): an alias of one zswap_pools[] slot.  It
+ * always holds a ref, so it is never retired from under us.
+ */
+static struct zswap_pool __rcu *zswap_current_pool;
+
 /* Global LRU lists shared by all zswap pools. */
 static struct list_lru zswap_list_lru;
 
@@ -200,9 +208,7 @@ struct zswap_entry {
 static struct xarray *zswap_trees[MAX_SWAPFILES];
 static unsigned int nr_zswap_trees[MAX_SWAPFILES];
 
-/* RCU-protected iteration */
-static LIST_HEAD(zswap_pools);
-/* protects zswap_pools list modification */
+/* protects the zswap_pools array and zswap_current_pool */
 static DEFINE_SPINLOCK(zswap_pools_lock);
 /* pool counter to provide unique names to zsmalloc */
 static atomic_t zswap_pools_count = ATOMIC_INIT(0);
@@ -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);
+
+	return ret;
+}
+
 static struct zswap_pool *zswap_pool_create(char *compressor)
 {
 	struct zswap_pool *pool;
@@ -313,19 +338,27 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
 	if (ret)
 		goto cpuhp_add_fail;
 
-	/* being the current pool takes 1 ref; this func expects the
-	 * caller to always add the new pool as the current pool
+	/*
+	 * After a successful create, the caller makes this the current pool.
+	 * If the caller fails, it kills the ref to free the reserved slot.
 	 */
 	ret = percpu_ref_init(&pool->ref, __zswap_pool_empty,
 			      PERCPU_REF_ALLOW_REINIT, GFP_KERNEL);
 	if (ret)
 		goto ref_fail;
-	INIT_LIST_HEAD(&pool->list);
+
+	ret = zswap_pool_reserve_slot(pool);
+	if (ret < 0) {
+		pr_err("cannot create more than %d pools\n", ZSWAP_MAX_POOLS);
+		goto slot_fail;
+	}
 
 	zswap_pool_debug("created", pool);
 
 	return pool;
 
+slot_fail:
+	percpu_ref_exit(&pool->ref);
 ref_fail:
 	cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &pool->node);
 
@@ -388,7 +421,7 @@ static void __zswap_pool_release(struct work_struct *work)
 	WARN_ON(!percpu_ref_is_zero(&pool->ref));
 	percpu_ref_exit(&pool->ref);
 
-	/* pool is now off zswap_pools list and has no references. */
+	/* Slot cleared in __zswap_pool_empty(); synchronize_rcu() drained readers. */
 	zswap_pool_destroy(pool);
 }
 
@@ -404,7 +437,11 @@ static void __zswap_pool_empty(struct percpu_ref *ref)
 
 	WARN_ON(pool == zswap_pool_current());
 
-	list_del_rcu(&pool->list);
+	/*
+	 * Clear the slot before scheduling the release so new readers cannot
+	 * see it; __zswap_pool_release()'s synchronize_rcu() drains the rest.
+	 */
+	rcu_assign_pointer(zswap_pools[pool->idx], NULL);
 
 	INIT_WORK(&pool->release_work, __zswap_pool_release);
 	schedule_work(&pool->release_work);
@@ -435,7 +472,8 @@ static struct zswap_pool *__zswap_pool_current(void)
 {
 	struct zswap_pool *pool;
 
-	pool = list_first_or_null_rcu(&zswap_pools, typeof(*pool), list);
+	pool = rcu_dereference_check(zswap_current_pool,
+				     lockdep_is_held(&zswap_pools_lock));
 	WARN_ONCE(!pool && zswap_has_pool,
 		  "%s: no page storage pool!\n", __func__);
 
@@ -468,11 +506,14 @@ static struct zswap_pool *zswap_pool_current_get(void)
 static struct zswap_pool *zswap_pool_find_get(char *compressor)
 {
 	struct zswap_pool *pool;
+	int i;
 
 	assert_spin_locked(&zswap_pools_lock);
 
-	list_for_each_entry_rcu(pool, &zswap_pools, list) {
-		if (strcmp(pool->tfm_name, compressor))
+	for (i = 0; i < ZSWAP_MAX_POOLS; i++) {
+		pool = rcu_dereference_protected(zswap_pools[i],
+					lockdep_is_held(&zswap_pools_lock));
+		if (!pool || strcmp(pool->tfm_name, compressor))
 			continue;
 		/* if we can't get it, it's about to be destroyed */
 		if (!zswap_pool_tryget(pool))
@@ -497,10 +538,14 @@ unsigned long zswap_total_pages(void)
 {
 	struct zswap_pool *pool;
 	unsigned long total = 0;
+	int i;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(pool, &zswap_pools, list)
-		total += zs_get_total_pages(pool->zs_pool);
+	for (i = 0; i < ZSWAP_MAX_POOLS; i++) {
+		pool = rcu_dereference(zswap_pools[i]);
+		if (pool)
+			total += zs_get_total_pages(pool->zs_pool);
+	}
 	rcu_read_unlock();
 
 	return total;
@@ -562,7 +607,6 @@ static int zswap_compressor_param_set(const char *val, const struct kernel_param
 	if (pool) {
 		zswap_pool_debug("using existing", pool);
 		WARN_ON(pool == zswap_pool_current());
-		list_del_rcu(&pool->list);
 	}
 
 	spin_unlock_bh(&zswap_pools_lock);
@@ -589,16 +633,15 @@ static int zswap_compressor_param_set(const char *val, const struct kernel_param
 	spin_lock_bh(&zswap_pools_lock);
 
 	if (!ret) {
+		/* The new pool becomes current; drop the ref of the old one. */
 		put_pool = zswap_pool_current();
-		list_add_rcu(&pool->list, &zswap_pools);
+		rcu_assign_pointer(zswap_current_pool, pool);
 		zswap_has_pool = true;
 	} else if (pool) {
 		/*
-		 * Add the possibly pre-existing pool to the end of the pools
-		 * list; if it's new (and empty) then it'll be removed and
-		 * destroyed by the put after we drop the lock
+		 * Not current; drop the ref below.  This frees a new pool
+		 * (and its slot), or decommissions a reused one (slot kept).
 		 */
-		list_add_tail_rcu(&pool->list, &zswap_pools);
 		put_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);
+
 	zswap_entry_cache = KMEM_CACHE(zswap_entry, 0);
 	if (!zswap_entry_cache) {
 		pr_err("entry cache creation failed\n");
@@ -1793,7 +1839,7 @@ static int zswap_setup(void)
 	pool = __zswap_pool_create_fallback();
 	if (pool) {
 		pr_info("loaded using pool %s\n", pool->tfm_name);
-		list_add(&pool->list, &zswap_pools);
+		rcu_assign_pointer(zswap_current_pool, pool);
 		zswap_has_pool = true;
 		static_branch_enable(&zswap_ever_enabled);
 	} else {

-- 
2.43.0

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

[Jianyue Wu]

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 a small fixed array indexed by a u8 slot number.

Replace the per-entry pool pointer with that u8 slot index and resolve it
through a zswap_entry_pool() helper.  A live entry holds a reference to
its pool, so the slot cannot be reused under it; the lookup therefore
needs no RCU read-side section (rcu_dereference_protected(..., true)).

The u8 fits in the padding after the bool referenced field, shrinking the
entry from 56 to 48 bytes on x86_64.  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@kernel.org>
Signed-off-by: Jianyue Wu <wujianyue000@gmail.com>
---
 mm/zswap.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index b203934d3be8..d4f4db2999f2 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -190,7 +190,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 - slot 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.
@@ -199,12 +199,22 @@ 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;
 };
 
+static struct zswap_pool *zswap_entry_pool(struct zswap_entry *entry)
+{
+	/*
+	 * A live entry holds a reference to its pool, so the slot cannot be
+	 * cleared or reused under it.  This is not an RCU read-side walk.
+	 */
+	return rcu_dereference_protected(zswap_pools[entry->pool_idx],
+					 true /* entry pins pool */);
+}
+
 static struct xarray *zswap_trees[MAX_SWAPFILES];
 static unsigned int nr_zswap_trees[MAX_SWAPFILES];
 
@@ -807,9 +817,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(&zswap_list_lru, 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);
@@ -966,12 +980,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);
@@ -1007,7 +1024,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;
 }
@@ -1500,7 +1517,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;
@@ -1806,7 +1823,7 @@ static int zswap_setup(void)
 	struct zswap_pool *pool;
 	int ret;
 
-	/* Slot indices are stored in a u8 (pool->idx). */
+	/* Slot indices are stored in a u8 (pool->idx and entry->pool_idx). */
 	BUILD_BUG_ON(ZSWAP_MAX_POOLS - 1 > U8_MAX);
 
 	zswap_entry_cache = KMEM_CACHE(zswap_entry, 0);

-- 
2.43.0

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

[Yosry Ahmed]

On Fri, Jul 31, 2026 at 08:32:47AM +0800, Jianyue Wu wrote:
> Originally zswap holds its pools on an RCU list whose head also serves as
> the "current pool".  Only a handful of pools are ever live at once, since
> a new pool is only created when the compressor is (re)set and pools are
> reused across compressor switches.
> 
> A later change wants to store a reference to each entry's pool in every
> zswap_entry, where a pointer would cost 8 bytes but a small pool index
> only one.  To make that index possible, the current change holds the
> pools in a fixed ZSWAP_MAX_POOLS-element array so each pool has a stable
> slot number, and tracks the current pool with a separate rcu-protected
> pointer.
> 
> The array keeps the same RCU publish/retire discipline the list had, so
> lookup and teardown stay equivalent.  Newly created pools are published
> into the array before they become current, so zswap_total_pages() can
> observe an empty pool briefly; that is harmless.

I don't follow. A newly created pool is empty anyway, so not being
iterated in zswap_total_pages() should be normal. Why do we need to call
this out?

> This also caps the
> number of live pools at ZSWAP_MAX_POOLS (16), which is plenty in
> practice; pool creation warns and fails if the array ever fills, and the
> limit can be raised.
> 
> Suggested-by: Nhat Pham <nphamcs@gmail.com>
> Suggested-by: Yosry Ahmed <yosry@kernel.org>
> Signed-off-by: Jianyue Wu <wujianyue000@gmail.com>
> ---
>  mm/zswap.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 67 insertions(+), 21 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 4e76a4a87cdc..b203934d3be8 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -154,12 +154,20 @@ struct zswap_pool {
>  	struct zs_pool *zs_pool;
>  	struct crypto_acomp_ctx __percpu *acomp_ctx;
>  	struct percpu_ref ref;
> -	struct list_head list;
>  	struct work_struct release_work;
>  	struct hlist_node node;
> +	u8 idx;
>  	char tfm_name[CRYPTO_MAX_ALG_NAME];
>  };
>  
> +#define ZSWAP_MAX_POOLS 16
> +static struct zswap_pool __rcu *zswap_pools[ZSWAP_MAX_POOLS];
> +/*
> + * The current pool (NULL if none): an alias of one zswap_pools[] slot.  It
> + * always holds a ref, so it is never retired from under us.
> + */
> +static struct zswap_pool __rcu *zswap_current_pool;
> +
>  /* Global LRU lists shared by all zswap pools. */
>  static struct list_lru zswap_list_lru;
>  
> @@ -200,9 +208,7 @@ struct zswap_entry {
>  static struct xarray *zswap_trees[MAX_SWAPFILES];
>  static unsigned int nr_zswap_trees[MAX_SWAPFILES];
>  
> -/* RCU-protected iteration */
> -static LIST_HEAD(zswap_pools);
> -/* protects zswap_pools list modification */
> +/* protects the zswap_pools array and zswap_current_pool */
>  static DEFINE_SPINLOCK(zswap_pools_lock);
>  /* pool counter to provide unique names to zsmalloc */
>  static atomic_t zswap_pools_count = ATOMIC_INIT(0);
> @@ -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);

Sashiko points out a seemingly real problem here because we add the pool
to the array before actually making it the current pool.

https://sashiko.dev/#/patchset/20260731-shrink_zswap_entry_v2-0-0-v2-0-e72083aa8734%40gmail.com

What if we just reserve an index here but not actually assign the pool?
We can add a marker to the array or sth (e.g. (void *)-1UL)).


> +			ret = i;
> +			break;
> +		}
> +	}
> +	spin_unlock_bh(&zswap_pools_lock);
> +
> +	return ret;
> +}
> +
>  static struct zswap_pool *zswap_pool_create(char *compressor)
>  {
>  	struct zswap_pool *pool;
> @@ -313,19 +338,27 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
>  	if (ret)
>  		goto cpuhp_add_fail;
>  
> -	/* being the current pool takes 1 ref; this func expects the
> -	 * caller to always add the new pool as the current pool
> +	/*
> +	 * After a successful create, the caller makes this the current pool.
> +	 * If the caller fails, it kills the ref to free the reserved slot.
>  	 */
>  	ret = percpu_ref_init(&pool->ref, __zswap_pool_empty,
>  			      PERCPU_REF_ALLOW_REINIT, GFP_KERNEL);
>  	if (ret)
>  		goto ref_fail;
> -	INIT_LIST_HEAD(&pool->list);
> +
> +	ret = zswap_pool_reserve_slot(pool);
> +	if (ret < 0) {
> +		pr_err("cannot create more than %d pools\n", ZSWAP_MAX_POOLS);
> +		goto slot_fail;
> +	}
>  
>  	zswap_pool_debug("created", pool);
>  
>  	return pool;
>  
> +slot_fail:
> +	percpu_ref_exit(&pool->ref);
>  ref_fail:
>  	cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &pool->node);
>  
> @@ -388,7 +421,7 @@ static void __zswap_pool_release(struct work_struct *work)
>  	WARN_ON(!percpu_ref_is_zero(&pool->ref));
>  	percpu_ref_exit(&pool->ref);
>  
> -	/* pool is now off zswap_pools list and has no references. */
> +	/* Slot cleared in __zswap_pool_empty(); synchronize_rcu() drained readers. */
>  	zswap_pool_destroy(pool);
>  }
>  
> @@ -404,7 +437,11 @@ static void __zswap_pool_empty(struct percpu_ref *ref)
>  
>  	WARN_ON(pool == zswap_pool_current());
>  
> -	list_del_rcu(&pool->list);
> +	/*
> +	 * Clear the slot before scheduling the release so new readers cannot
> +	 * see it; __zswap_pool_release()'s synchronize_rcu() drains the rest.
> +	 */
> +	rcu_assign_pointer(zswap_pools[pool->idx], NULL);
>  
>  	INIT_WORK(&pool->release_work, __zswap_pool_release);
>  	schedule_work(&pool->release_work);

Not related to this change, but I wonder if we can use call_rcu() or
similar here instead of the manual synchronize_rcu().

> @@ -435,7 +472,8 @@ static struct zswap_pool *__zswap_pool_current(void)
>  {
>  	struct zswap_pool *pool;
>  
> -	pool = list_first_or_null_rcu(&zswap_pools, typeof(*pool), list);
> +	pool = rcu_dereference_check(zswap_current_pool,
> +				     lockdep_is_held(&zswap_pools_lock));
>  	WARN_ONCE(!pool && zswap_has_pool,
>  		  "%s: no page storage pool!\n", __func__);
>  
> @@ -468,11 +506,14 @@ static struct zswap_pool *zswap_pool_current_get(void)
>  static struct zswap_pool *zswap_pool_find_get(char *compressor)
>  {
>  	struct zswap_pool *pool;
> +	int i;
>  
>  	assert_spin_locked(&zswap_pools_lock);
>  
> -	list_for_each_entry_rcu(pool, &zswap_pools, list) {
> -		if (strcmp(pool->tfm_name, compressor))
> +	for (i = 0; i < ZSWAP_MAX_POOLS; i++) {
> +		pool = rcu_dereference_protected(zswap_pools[i],
> +					lockdep_is_held(&zswap_pools_lock));

We already have an assertion that we are holding the lock above.

> +		if (!pool || strcmp(pool->tfm_name, compressor))
>  			continue;
>  		/* if we can't get it, it's about to be destroyed */
>  		if (!zswap_pool_tryget(pool))
 

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

[Yosry Ahmed]

On Fri, Jul 31, 2026 at 08:32:48AM +0800, Jianyue Wu 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 a small fixed array indexed by a u8 slot number.
> 
> Replace the per-entry pool pointer with that u8 slot index and resolve it
> through a zswap_entry_pool() helper.  A live entry holds a reference to
> its pool, so the slot cannot be reused under it; the lookup therefore
> needs no RCU read-side section (rcu_dereference_protected(..., true)).
> 
> The u8 fits in the padding after the bool referenced field, shrinking the
> entry from 56 to 48 bytes on x86_64.  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@kernel.org>
> Signed-off-by: Jianyue Wu <wujianyue000@gmail.com>
> ---
>  mm/zswap.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index b203934d3be8..d4f4db2999f2 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -190,7 +190,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 - slot 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.
> @@ -199,12 +199,22 @@ 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;
>  };
>  
> +static struct zswap_pool *zswap_entry_pool(struct zswap_entry *entry)
> +{
> +	/*
> +	 * A live entry holds a reference to its pool, so the slot cannot be
> +	 * cleared or reused under it.  This is not an RCU read-side walk.
> +	 */
> +	return rcu_dereference_protected(zswap_pools[entry->pool_idx],
> +					 true /* entry pins pool */);

Probably doesn't matter in practice, but maybe entry->handle or
something instead of 'true' to make it clear we are checking for an
"active" entry?

> +}
> +
>  static struct xarray *zswap_trees[MAX_SWAPFILES];
>  static unsigned int nr_zswap_trees[MAX_SWAPFILES];
>  
> @@ -807,9 +817,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(&zswap_list_lru, 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);
> @@ -966,12 +980,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);
> @@ -1007,7 +1024,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;
>  }
> @@ -1500,7 +1517,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;
> @@ -1806,7 +1823,7 @@ static int zswap_setup(void)
>  	struct zswap_pool *pool;
>  	int ret;
>  
> -	/* Slot indices are stored in a u8 (pool->idx). */
> +	/* Slot indices are stored in a u8 (pool->idx and entry->pool_idx). */
>  	BUILD_BUG_ON(ZSWAP_MAX_POOLS - 1 > U8_MAX);
>  
>  	zswap_entry_cache = KMEM_CACHE(zswap_entry, 0);
> 
> -- 
> 2.43.0
> 

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

[Jianyue Wu]

On Tue, Aug 11, 2026 at 8:20 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> On Fri, Jul 31, 2026 at 08:32:48AM +0800, Jianyue Wu 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 a small fixed array indexed by a u8 slot number.
> >
> > Replace the per-entry pool pointer with that u8 slot index and resolve it
> > through a zswap_entry_pool() helper.  A live entry holds a reference to
> > its pool, so the slot cannot be reused under it; the lookup therefore
> > needs no RCU read-side section (rcu_dereference_protected(..., true)).
> >
> > The u8 fits in the padding after the bool referenced field, shrinking the
> > entry from 56 to 48 bytes on x86_64.  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@kernel.org>
> > Signed-off-by: Jianyue Wu <wujianyue000@gmail.com>
> > ---
> >  mm/zswap.c | 33 +++++++++++++++++++++++++--------
> >  1 file changed, 25 insertions(+), 8 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index b203934d3be8..d4f4db2999f2 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -190,7 +190,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 - slot 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.
> > @@ -199,12 +199,22 @@ 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;
> >  };
> >
> > +static struct zswap_pool *zswap_entry_pool(struct zswap_entry *entry)
> > +{
> > +     /*
> > +      * A live entry holds a reference to its pool, so the slot cannot be
> > +      * cleared or reused under it.  This is not an RCU read-side walk.
> > +      */
> > +     return rcu_dereference_protected(zswap_pools[entry->pool_idx],
> > +                                      true /* entry pins pool */);
>
> Probably doesn't matter in practice, but maybe entry->handle or
> something instead of 'true' to make it clear we are checking for an
> "active" entry?
>
Thanks, good point, I'll use entry->handle as the condition:
       return rcu_dereference_protected(zswap_pools[entry->pool_idx],


entry->handle /* live entry pins pool */);

Best regards,
Jianyue

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

[Jianyue Wu]

On Tue, Aug 11, 2026 at 8:12 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> On Fri, Jul 31, 2026 at 08:32:47AM +0800, Jianyue Wu wrote:
> > Originally zswap holds its pools on an RCU list whose head also serves as
> > the "current pool".  Only a handful of pools are ever live at once, since
> > a new pool is only created when the compressor is (re)set and pools are
> > reused across compressor switches.
> >
> > A later change wants to store a reference to each entry's pool in every
> > zswap_entry, where a pointer would cost 8 bytes but a small pool index
> > only one.  To make that index possible, the current change holds the
> > pools in a fixed ZSWAP_MAX_POOLS-element array so each pool has a stable
> > slot number, and tracks the current pool with a separate rcu-protected
> > pointer.
> >
> > The array keeps the same RCU publish/retire discipline the list had, so
> > lookup and teardown stay equivalent.  Newly created pools are published
> > into the array before they become current, so zswap_total_pages() can
> > observe an empty pool briefly; that is harmless.
>
> I don't follow. A newly created pool is empty anyway, so not being
> iterated in zswap_total_pages() should be normal. Why do we need to call
> this out?
You are right, zswap_total_pages() can observe an empty pool briefly
is not needed, it is obvious.

> > This also caps the
> > number of live pools at ZSWAP_MAX_POOLS (16), which is plenty in
> > practice; pool creation warns and fails if the array ever fills, and the
> > limit can be raised.
> >
> > Suggested-by: Nhat Pham <nphamcs@gmail.com>
> > Suggested-by: Yosry Ahmed <yosry@kernel.org>
> > Signed-off-by: Jianyue Wu <wujianyue000@gmail.com>
> > ---
> >  mm/zswap.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 67 insertions(+), 21 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 4e76a4a87cdc..b203934d3be8 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -154,12 +154,20 @@ struct zswap_pool {
> >       struct zs_pool *zs_pool;
> >       struct crypto_acomp_ctx __percpu *acomp_ctx;
> >       struct percpu_ref ref;
> > -     struct list_head list;
> >       struct work_struct release_work;
> >       struct hlist_node node;
> > +     u8 idx;
> >       char tfm_name[CRYPTO_MAX_ALG_NAME];
> >  };
> >
> > +#define ZSWAP_MAX_POOLS 16
> > +static struct zswap_pool __rcu *zswap_pools[ZSWAP_MAX_POOLS];
> > +/*
> > + * The current pool (NULL if none): an alias of one zswap_pools[] slot.  It
> > + * always holds a ref, so it is never retired from under us.
> > + */
> > +static struct zswap_pool __rcu *zswap_current_pool;
> > +
> >  /* Global LRU lists shared by all zswap pools. */
> >  static struct list_lru zswap_list_lru;
> >
> > @@ -200,9 +208,7 @@ struct zswap_entry {
> >  static struct xarray *zswap_trees[MAX_SWAPFILES];
> >  static unsigned int nr_zswap_trees[MAX_SWAPFILES];
> >
> > -/* RCU-protected iteration */
> > -static LIST_HEAD(zswap_pools);
> > -/* protects zswap_pools list modification */
> > +/* protects the zswap_pools array and zswap_current_pool */
> >  static DEFINE_SPINLOCK(zswap_pools_lock);
> >  /* pool counter to provide unique names to zsmalloc */
> >  static atomic_t zswap_pools_count = ATOMIC_INIT(0);
> > @@ -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);
>
> Sashiko points out a seemingly real problem here because we add the pool
> to the array before actually making it the current pool.
>
> https://sashiko.dev/#/patchset/20260731-shrink_zswap_entry_v2-0-0-v2-0-e72083aa8734%40gmail.com
>
> What if we just reserve an index here but not actually assign the pool?
> We can add a marker to the array or sth (e.g. (void *)-1UL)).
That's a good idea, thanks. This is a real race the list version didn't
have. Publishing the real pool pointer at reserve time makes it visible to
zswap_pool_find_get() before the pool is committed as current. A concurrent
writer to the compressor param can then find it, take the else branch,
percpu_ref_resurrect() an already-live ref (hitting
WARN_ON_ONCE(!percpu_ref_is_dead())), and go on to kill what it believes is
the old current pool -- exactly the use-after-free you describe.

I'll define ZSWAP_SLOT_RESERVED as -1UL to claim the index at reserve time,
the real pointer is stored only once the pool is committed as current,
under zswap_pools_lock.
All array walkers skip the placeholder, so a concurrent zswap_pool_find_get()
no longer sees the not-yet-ready pool and the resurrect/kill/UAF chain cannot
start.

>
> > +                     ret = i;
> > +                     break;
> > +             }
> > +     }
> > +     spin_unlock_bh(&zswap_pools_lock);
> > +
> > +     return ret;
> > +}
> > +
> >  static struct zswap_pool *zswap_pool_create(char *compressor)
> >  {
> >       struct zswap_pool *pool;
> > @@ -313,19 +338,27 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
> >       if (ret)
> >               goto cpuhp_add_fail;
> >
> > -     /* being the current pool takes 1 ref; this func expects the
> > -      * caller to always add the new pool as the current pool
> > +     /*
> > +      * After a successful create, the caller makes this the current pool.
> > +      * If the caller fails, it kills the ref to free the reserved slot.
> >        */
> >       ret = percpu_ref_init(&pool->ref, __zswap_pool_empty,
> >                             PERCPU_REF_ALLOW_REINIT, GFP_KERNEL);
> >       if (ret)
> >               goto ref_fail;
> > -     INIT_LIST_HEAD(&pool->list);
> > +
> > +     ret = zswap_pool_reserve_slot(pool);
> > +     if (ret < 0) {
> > +             pr_err("cannot create more than %d pools\n", ZSWAP_MAX_POOLS);
> > +             goto slot_fail;
> > +     }
> >
> >       zswap_pool_debug("created", pool);
> >
> >       return pool;
> >
> > +slot_fail:
> > +     percpu_ref_exit(&pool->ref);
> >  ref_fail:
> >       cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &pool->node);
> >
> > @@ -388,7 +421,7 @@ static void __zswap_pool_release(struct work_struct *work)
> >       WARN_ON(!percpu_ref_is_zero(&pool->ref));
> >       percpu_ref_exit(&pool->ref);
> >
> > -     /* pool is now off zswap_pools list and has no references. */
> > +     /* Slot cleared in __zswap_pool_empty(); synchronize_rcu() drained readers. */
> >       zswap_pool_destroy(pool);
> >  }
> >
> > @@ -404,7 +437,11 @@ static void __zswap_pool_empty(struct percpu_ref *ref)
> >
> >       WARN_ON(pool == zswap_pool_current());
> >
> > -     list_del_rcu(&pool->list);
> > +     /*
> > +      * Clear the slot before scheduling the release so new readers cannot
> > +      * see it; __zswap_pool_release()'s synchronize_rcu() drains the rest.
> > +      */
> > +     rcu_assign_pointer(zswap_pools[pool->idx], NULL);
> >
> >       INIT_WORK(&pool->release_work, __zswap_pool_release);
> >       schedule_work(&pool->release_work);
>
> Not related to this change, but I wonder if we can use call_rcu() or
> similar here instead of the manual synchronize_rcu().
Agree, will use call_rcu() to replace workqueue + synchronize_rcu(),
so it no longer synchronously waits on an RCU grace period.

> > @@ -435,7 +472,8 @@ static struct zswap_pool *__zswap_pool_current(void)
> >  {
> >       struct zswap_pool *pool;
> >
> > -     pool = list_first_or_null_rcu(&zswap_pools, typeof(*pool), list);
> > +     pool = rcu_dereference_check(zswap_current_pool,
> > +                                  lockdep_is_held(&zswap_pools_lock));
> >       WARN_ONCE(!pool && zswap_has_pool,
> >                 "%s: no page storage pool!\n", __func__);
> >
> > @@ -468,11 +506,14 @@ static struct zswap_pool *zswap_pool_current_get(void)
> >  static struct zswap_pool *zswap_pool_find_get(char *compressor)
> >  {
> >       struct zswap_pool *pool;
> > +     int i;
> >
> >       assert_spin_locked(&zswap_pools_lock);
> >
> > -     list_for_each_entry_rcu(pool, &zswap_pools, list) {
> > -             if (strcmp(pool->tfm_name, compressor))
> > +     for (i = 0; i < ZSWAP_MAX_POOLS; i++) {
> > +             pool = rcu_dereference_protected(zswap_pools[i],
> > +                                     lockdep_is_held(&zswap_pools_lock));
>
> We already have an assertion that we are holding the lock above.
Exactly, the lock is already asserted previously.
Will simplify to rcu_dereference_protected(zswap_pools[i], true);

Thanks for the review!

Best regards,
Jianyue

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

[Yosry Ahmed]

> > > @@ -404,7 +437,11 @@ static void __zswap_pool_empty(struct percpu_ref *ref)
> > >
> > >       WARN_ON(pool == zswap_pool_current());
> > >
> > > -     list_del_rcu(&pool->list);
> > > +     /*
> > > +      * Clear the slot before scheduling the release so new readers cannot
> > > +      * see it; __zswap_pool_release()'s synchronize_rcu() drains the rest.
> > > +      */
> > > +     rcu_assign_pointer(zswap_pools[pool->idx], NULL);
> > >
> > >       INIT_WORK(&pool->release_work, __zswap_pool_release);
> > >       schedule_work(&pool->release_work);
> >
> > Not related to this change, but I wonder if we can use call_rcu() or
> > similar here instead of the manual synchronize_rcu().
> Agree, will use call_rcu() to replace workqueue + synchronize_rcu(),
> so it no longer synchronously waits on an RCU grace period.

If you make this change, please do it in a separate patch. You can
keep it a part of this series if it conflicts with it, otherwise you
can send it separately. Thanks!

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().