linux-kernel - Re: [PATCH] mm/zswap: Remove zswap_pools

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250113152132.2372632-1-joshua.hahnjy@gmail.com>
Date: Mon, 13 Jan 2025 07:21:29 -0800
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: Johannes Weiner <hannes@...xchg.org>,
	Chengming Zhou <chengming.zhou@...ux.dev>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Nhat Pham <nphamcs@...il.com>,
	linux-kernel@...r.kernel.org,
	linux-mm@...ck.org,
	kernel-team@...a.com
Subject: Re: [PATCH] mm/zswap: Remove zswap_pools_counter

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@...gle.com> wrote:

> On Fri, Jan 10, 2025 at 12:33 PM Joshua Hahn <joshua.hahnjy@...il.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