linux-kernel - Re: [PATCH 11/20] mm: zswap: function ordering: pool refcounting

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: <20240131112311.GA1227330@cmpxchg.org>
Date: Wed, 31 Jan 2024 06:23:11 -0500
From: Johannes Weiner <hannes@...xchg.org>
To: Nhat Pham <nphamcs@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Yosry Ahmed <yosryahmed@...gle.com>,
	Chengming Zhou <zhouchengming@...edance.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 11/20] mm: zswap: function ordering: pool refcounting

On Tue, Jan 30, 2024 at 12:13:30PM -0800, Nhat Pham wrote:
> On Mon, Jan 29, 2024 at 5:42 PM Johannes Weiner <hannes@...xchg.org> wrote:
> >
> > Move pool refcounting functions into the pool section. First the
> > destroy functions, then the get and put which uses them.
> >
> > __zswap_pool_empty() has an upward reference to the global
> > zswap_pools, to sanity check it's not the currently active pool that's
> > being freed. That gets the forward decl for zswap_pool_cuyrrent().
> 
> nit: zswap_pool_cuyrrent() -> zswap_pool_current() :-)

Whoops, my bad.

Andrew, would you mind removing that typo inside your copy?

> Also, would it make sense to move zswap_pool_current() above
> __zswap_pool_empty() to get rid of the forward declaration? I guess
> it's now grouped with current_get() etc. - those don't seem to use the
> empty check, so maybe they can also go above __zswap_pool_empty()?

There is a grouping to these functions:

- low-level functions that create and destroy individual struct zswap_pool
  (create, destroy, get, release, empty, put)
- high-level functions that operate on pool collections, i.e. zswap_pools
  (current, last, find)

They were actually already grouped like that, just in the reverse
order. The only way to avoid ALL forward decls would be to interleave
the layers, but I think the grouping makes sense so I wanted to
preserve that. I went with low to high ordering, and forward decl the
odd one where a low-level function does one high-level sanity check.

Does that make sense?