From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BC9411C860C for ; Tue, 11 Aug 2026 00:12:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786407168; cv=none; b=rdp8kbOwZVmXgLw4t34wZPSZhuTajU52L+Zu/nwqyQKyqRxgjyQ2iDnhjdpVtbONi4Eb/okpKdQGXaP1iTgcUcAeN/jcY+nS0+F0GYs4oaULfACbgnNE7/xgIkjUA6/R0pX8936FaVw9hvKsC+xP1CVjy6gYvxTlIZFqzH7tgGA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786407168; c=relaxed/simple; bh=c1LBY0VHpgbHK3asDM1yCcI46Nrm+6ZmdNjqYoGa39s=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Qu9gSd854XqfBiNyZfCaRrAu27SPQaa9+3HpxgSGG3P2kYk6utmfwdAnEKqBlwSzkzgwv2L86aVS1sEz8tfd6yZCQ7MV/QDr5EWyWKnykV6A1yr099n8xaWrBuN60Y7dJ64ZBopXBiCNweW9oSKMiaY5i3zR75TzFN3KTG+3BcU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=MRYQoqK3; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="MRYQoqK3" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 591031F000E9; Tue, 11 Aug 2026 00:12:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1786407166; bh=3T6JgGC76v/Qc26IiCVVRPE74vRurEDwCMQZ6951TmA=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=MRYQoqK3kO6QB2hE3UIMp56L/IeDzXmghlQvijiJyv3LWBthh+y8WZSy4mgZqxIUj CAGzQKEWVjsHVFPDicczAJEnP+C74k8XIRtBNQzl1/w5w63Ic5GPga5CGpYVBYNpo6 pLMZEFMoQoec+Y13E5ohx5K5o2Bit1EOaqy+KIRiDLH3lWYIhl/oQujVw8tmtZImoT eQT/dWofozedMoHMbwafKKsG06JlByaFY4bIVEBakw23wxsE2yhXZHmSV/ODuRvqEM vJHQaOMg7Wsc3PiM808sATR24jC1Uxj1DB22QuVmb1c4XOsRrhreb6S/UFmhEKw925 RYDh8jBzlWefw== Date: Tue, 11 Aug 2026 00:12:44 +0000 From: Yosry Ahmed To: Jianyue Wu Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Johannes Weiner , Nhat Pham , Chengming Zhou , Andrew Morton , Chris Li Subject: Re: [PATCH RFC v2 1/2] mm/zswap: replace the zswap_pools list with a fixed pools array Message-ID: References: <20260731-shrink_zswap_entry_v2-0-0-v2-0-e72083aa8734@gmail.com> <20260731-shrink_zswap_entry_v2-0-0-v2-1-e72083aa8734@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260731-shrink_zswap_entry_v2-0-0-v2-1-e72083aa8734@gmail.com> 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 > Suggested-by: Yosry Ahmed > Signed-off-by: Jianyue Wu > --- > 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))