Received-SPF: pass (google.com: domain of linux-kernel+bounces-46339-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 31 Jan 2024 06:23:11 -0500
From: Johannes Weiner <hannes@cmpxchg.org>
To: Nhat Pham <nphamcs@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Yosry Ahmed <yosryahmed@google.com>,
	Chengming Zhou <zhouchengming@bytedance.com>, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 11/20] mm: zswap: function ordering: pool refcounting
Message-ID: <20240131112311.GA1227330@cmpxchg.org>
References: <20240130014208.565554-1-hannes@cmpxchg.org>
 <20240130014208.565554-12-hannes@cmpxchg.org>
 <CAKEwX=PN3vFZO1VkFVoRJGOnwgRdfuyOrFPA6DvfkM761o1B9g@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAKEwX=PN3vFZO1VkFVoRJGOnwgRdfuyOrFPA6DvfkM761o1B9g@mail.gmail.com>

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@cmpxchg.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?