[PATCH] mm/zswap: Remove zswap_pools_counter

[PATCH] mm/zswap: Remove zswap_pools_counter

[Joshua Hahn]

Commit 8edc9c4 [1] reduced the number of pools used by zswap from 32 to 1.
As such, we no longer need to have unique names for zpool (zsmalloc).

Remove the atomic counter that keeps track of the number of allocated pools,
and replace the "zswap<n>" string formatting with "zswap".

Signed-off-by: Joshua Hahn <joshua.hahnjy@gmail.com>

[1] https://lore.kernel.org/all/20240617-zsmalloc-lock-mm-everything-v1-2-5e5081ea11b3@linux.dev/T/#u

---
 mm/zswap.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 5a27af8d86ea..bc43e807de29 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -210,8 +210,6 @@ static unsigned int nr_zswap_trees[MAX_SWAPFILES];
 static LIST_HEAD(zswap_pools);
 /* protects zswap_pools list modification */
 static DEFINE_SPINLOCK(zswap_pools_lock);
-/* pool counter to provide unique names to zpool */
-static atomic_t zswap_pools_count = ATOMIC_INIT(0);
 
 enum zswap_init_type {
 	ZSWAP_UNINIT,
@@ -249,7 +247,6 @@ static void __zswap_pool_empty(struct percpu_ref *ref);
 static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
 {
 	struct zswap_pool *pool;
-	char name[38]; /* 'zswap' + 32 char (max) num + \0 */
 	gfp_t gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
 	int ret;
 
@@ -268,9 +265,7 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
 	if (!pool)
 		return NULL;
 
-	/* unique name for each pool specifically required by zsmalloc */
-	snprintf(name, 38, "zswap%x", atomic_inc_return(&zswap_pools_count));
-	pool->zpool = zpool_create_pool(type, name, gfp);
+	pool->zpool = zpool_create_pool(type, "zswap", gfp);
 	if (!pool->zpool) {
 		pr_err("%s zpool not available\n", type);
 		goto error;
-- 
2.43.5

Re: [PATCH] mm/zswap: Remove zswap_pools_counter

[Yosry Ahmed]

On Fri, Jan 10, 2025 at 12:33 PM Joshua Hahn <joshua.hahnjy@gmail.com> wrote:
>
> Commit 8edc9c4 [1] reduced the number of pools used by zswap from 32 to 1.

When referring to commits we use the first 12 characters in the SHA1
and the commit subject, rather than the link (although adding the link
as well does not hurt). So this should be 'Commit 8edc9c4e72fe
("mm/zswap: use only one pool in zswap")'.

> As such, we no longer need to have unique names for zpool (zsmalloc).

More importantly, I don't think this is accurate. zswap_pools_count
was introduced by commit 32a4e1690399 ("mm/zswap: provide unique zpool
name") long before we increased the number of concurrent zpools to 32.
It is needed because even though we used to have a single zpool per
zswap_pool (as we returned to doing after [1]), we may have multiple
zswap_pool's (e.g. if the compressor is changed, a new zswap_pool is
created).

>
> Remove the atomic counter that keeps track of the number of allocated pools,
> and replace the "zswap<n>" string formatting with "zswap".
>
> Signed-off-by: Joshua Hahn <joshua.hahnjy@gmail.com>
>
> [1] https://lore.kernel.org/all/20240617-zsmalloc-lock-mm-everything-v1-2-5e5081ea11b3@linux.dev/T/#u
>
> ---
>  mm/zswap.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 5a27af8d86ea..bc43e807de29 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -210,8 +210,6 @@ static unsigned int nr_zswap_trees[MAX_SWAPFILES];
>  static LIST_HEAD(zswap_pools);
>  /* protects zswap_pools list modification */
>  static DEFINE_SPINLOCK(zswap_pools_lock);
> -/* pool counter to provide unique names to zpool */
> -static atomic_t zswap_pools_count = ATOMIC_INIT(0);
>
>  enum zswap_init_type {
>         ZSWAP_UNINIT,
> @@ -249,7 +247,6 @@ static void __zswap_pool_empty(struct percpu_ref *ref);
>  static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
>  {
>         struct zswap_pool *pool;
> -       char name[38]; /* 'zswap' + 32 char (max) num + \0 */
>         gfp_t gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
>         int ret;
>
> @@ -268,9 +265,7 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
>         if (!pool)
>                 return NULL;
>
> -       /* unique name for each pool specifically required by zsmalloc */
> -       snprintf(name, 38, "zswap%x", atomic_inc_return(&zswap_pools_count));
> -       pool->zpool = zpool_create_pool(type, name, gfp);
> +       pool->zpool = zpool_create_pool(type, "zswap", gfp);
>         if (!pool->zpool) {
>                 pr_err("%s zpool not available\n", type);
>                 goto error;
> --
> 2.43.5

Re: [PATCH] mm/zswap: Remove zswap_pools_counter

[Joshua Hahn]

Hi Yosry,
I hope you've had a great start to 2025! (Is it too late to still be doing
New Years greetings? : -))

On Fri, 10 Jan 2025 13:53:06 -0800 Yosry Ahmed <yosryahmed@google.com> wrote:

> On Fri, Jan 10, 2025 at 12:33 PM Joshua Hahn <joshua.hahnjy@gmail.com> wrote:
> >
> > Commit 8edc9c4 [1] reduced the number of pools used by zswap from 32 to 1.
> 
> When referring to commits we use the first 12 characters in the SHA1
> and the commit subject, rather than the link (although adding the link
> as well does not hurt). So this should be 'Commit 8edc9c4e72fe
> ("mm/zswap: use only one pool in zswap")'.

Thank you for letting me know. I grabbed the 6-character commit-ID from
Github, but I'll be sure to include the first 12 characters instead in
the future.

> > As such, we no longer need to have unique names for zpool (zsmalloc).
> 
> More importantly, I don't think this is accurate. zswap_pools_count
> was introduced by commit 32a4e1690399 ("mm/zswap: provide unique zpool
> name") long before we increased the number of concurrent zpools to 32.
> It is needed because even though we used to have a single zpool per
> zswap_pool (as we returned to doing after [1]), we may have multiple
> zswap_pool's (e.g. if the compressor is changed, a new zswap_pool is
> created).

Ah, thank you for the correction. In retrospect, this was a bit of a
naitve patch, it had completely passed my mind that we can still in fact
have multiple pools at the same time. This could have been avoided if I had
taken a look at where this code was introduced, rather than the patch
that (I thought) removed the last users.

Thank you for the review as always! Have a great day,
Joshua

Re: [PATCH] mm/zswap: Remove zswap_pools_counter

[Yosry Ahmed]

On Mon, Jan 13, 2025 at 7:21 AM Joshua Hahn <joshua.hahnjy@gmail.com> wrote:
>
> Hi Yosry,
> I hope you've had a great start to 2025! (Is it too late to still be doing
> New Years greetings? : -))

It's never too late, hope you had a great start too :)

>
> On Fri, 10 Jan 2025 13:53:06 -0800 Yosry Ahmed <yosryahmed@google.com> wrote:
>
> > On Fri, Jan 10, 2025 at 12:33 PM Joshua Hahn <joshua.hahnjy@gmail.com> wrote:
> > >
> > > Commit 8edc9c4 [1] reduced the number of pools used by zswap from 32 to 1.
> >
> > When referring to commits we use the first 12 characters in the SHA1
> > and the commit subject, rather than the link (although adding the link
> > as well does not hurt). So this should be 'Commit 8edc9c4e72fe
> > ("mm/zswap: use only one pool in zswap")'.
>
> Thank you for letting me know. I grabbed the 6-character commit-ID from
> Github, but I'll be sure to include the first 12 characters instead in
> the future.
>
> > > As such, we no longer need to have unique names for zpool (zsmalloc).
> >
> > More importantly, I don't think this is accurate. zswap_pools_count
> > was introduced by commit 32a4e1690399 ("mm/zswap: provide unique zpool
> > name") long before we increased the number of concurrent zpools to 32.
> > It is needed because even though we used to have a single zpool per
> > zswap_pool (as we returned to doing after [1]), we may have multiple
> > zswap_pool's (e.g. if the compressor is changed, a new zswap_pool is
> > created).
>
> Ah, thank you for the correction. In retrospect, this was a bit of a
> naitve patch, it had completely passed my mind that we can still in fact
> have multiple pools at the same time. This could have been avoided if I had
> taken a look at where this code was introduced, rather than the patch
> that (I thought) removed the last users.

It is a bit confusing that we have zswap pools with pools inside of
them to be fair :)