Received-SPF: pass (google.com: domain of linux-kernel+bounces-44867-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 30 Jan 2024 10:46:32 -0500
From: Johannes Weiner <hannes@cmpxchg.org>
To: Yosry Ahmed <yosryahmed@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Nhat Pham <nphamcs@gmail.com>,
	Chengming Zhou <zhouchengming@bytedance.com>, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 00/20] mm: zswap: cleanups
Message-ID: <20240130154632.GB772725@cmpxchg.org>
References: <20240130014208.565554-1-hannes@cmpxchg.org>
 <ZbiwW5BJhFeGc2Bd@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZbiwW5BJhFeGc2Bd@google.com>

On Tue, Jan 30, 2024 at 08:16:27AM +0000, Yosry Ahmed wrote:
> Hey Johannes,
> 
> On Mon, Jan 29, 2024 at 08:36:36PM -0500, Johannes Weiner wrote:
> > Cleanups and maintenance items that accumulated while reviewing zswap
> > patches. Based on akpm/mm-unstable + the UAF fix I sent just now.
> 
> Patches 1 to 9 LGTM, thanks for the great cleanups!
> 
> I am less excited about patches 10 to 20 though. Don't get me wrong, I
> am all of logically ordering the code. However, it feels like in this
> case, we will introduce unnecessary layers in the git history in a lot
> of places where I find myself checking the history regularly.
> Personally, I tend to jump around the file using vim search or using a
> cscope extension to find references/definitions, so I don't feel a need
> for such reordering.

I agree that sweeping non-functional changes can be somewhat
painful. However, moves are among the easiest of those because the
code itself doesn't change. git log is not really affected, git blame
<ref-just-before-move> mm/zswap.c works as well. Backports are easy to
adjust and verify - mostly, patch will just warn about line offsets.

What motivated this was figuring out the writeback/swapoff race. I
also use search and multiple buffers in my editor, but keeping track
of the dependencies between shrink_memcg_cb, zswap_writeback_entry and
third places like load and invalidate became quite unwieldy. There is
also the search in the logical direction not finding things, or mostly
unrelated stuff. It's just less error prone to read existing code and
write new code if related layers are grouped together and the order is
logical, despite having those tools.

The problem will also only get worse if there are no natural anchor
points for new code, and the layering isn't clear. The new shrinker
code is a case in point. We missed the opportunity in the memcg
codebase, and I've regretted it for years. It just resulted in more
fragile, less readable and debuggable code. The zswap code has been
stagnant in the last few years, and there are relatively few commits
behind us (git log --pretty=format:"%as %h %s" mm/zswap.c). I figure
now is a good chance, before the more major changes we have planned.

> I am not objecting to it, but I just find it less appealing that the
> rest of the series.

Understood.