[00/20] mm: zswap: cleanups

  Cleanups and maintenance items that accumulated while reviewing zswap
patches. Based on akpm/mm-unstable + the UAF fix I sent just now.

 mm/zswap.c | 1961 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 971 insertions(+), 990 deletions(-)
  

On 2024/1/30 09:36, Johannes Weiner wrote:
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Reviewed-by: Chengming Zhou <zhouchengming@bytedance.com>

> ---
>  mm/zswap.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 7a7e8da2b4f8..bdc9f82fe4b9 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1316,7 +1316,7 @@ static int zswap_enabled_param_set(const char *val,
>  	return ret;
>  }
>  
> -static void __zswap_load(struct zswap_entry *entry, struct page *page)
> +static void zswap_decompress(struct zswap_entry *entry, struct page *page)
>  {
>  	struct zpool *zpool = zswap_find_zpool(entry);
>  	struct scatterlist input, output;
> @@ -1411,7 +1411,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
>  	zswap_entry_get(entry);
>  	spin_unlock(&tree->lock);
>  
> -	__zswap_load(entry, &folio->page);
> +	zswap_decompress(entry, &folio->page);
>  
>  	count_vm_event(ZSWPWB);
>  	if (entry->objcg)
> @@ -1702,7 +1702,7 @@ bool zswap_load(struct folio *folio)
>  	spin_unlock(&tree->lock);
>  
>  	if (entry->length)
> -		__zswap_load(entry, page);
> +		zswap_decompress(entry, page);
>  	else {
>  		dst = kmap_local_page(page);
>  		zswap_fill_page(dst, entry->value);
  

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 am not objecting to it, but I just find it less appealing that the
rest of the series.

> 
>  mm/zswap.c | 1961 +++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 971 insertions(+), 990 deletions(-)
>
  

On (24/01/30 08:16), 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

This also can complicate cherry-picking of patches to stable, prod, .etc
  

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.
  

On Tue, Jan 30, 2024 at 09:21:31PM +0900, Sergey Senozhatsky wrote:
> On (24/01/30 08:16), 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
> 
> This also can complicate cherry-picking of patches to stable, prod, .etc

I'm sensitive to that argument, because we run our own kernel at Meta
as well.

But moves are pretty easy. The code doesn't actually change, just the
line offsets. So patch will mostly work with offset warnings. And if
not, it's easy to fix up and verify. Refactoring and API restructuring
(folios e.g.) make it much harder when it comes to this.
  

On Tue, Jan 30, 2024 at 10:46:32AM -0500, Johannes Weiner wrote:
> 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.

IIUC with git blame -L <lines> <ref-just-before-move> won't really work
because the lines will have changed significantly. In the future, going
through several layers of git blame will be less convenient because the
function moving will invalidate the lines.

> Backports are easy to
> adjust and verify - mostly, patch will just warn about line offsets.

I usually use git cherry-pick, and it sometimes gets confused if things
are moved far enough IIRC.

> 
> 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 agree that if we want to do it, it's much better now than later, just
presenting a differet perspective. No strong feelings.
  

On Tue, Jan 30, 2024 at 12:16 AM Yosry Ahmed <yosryahmed@google.com> 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 am not objecting to it, but I just find it less appealing that the
> rest of the series.

As a frequent user of git blame, I kinda agree with it.

That said, zswap functions ordering hurts my brain a lot. So I vote
for the reordering, and for paying the price sooner rather than later.
The alternative is reordering sometimes in the future (which is just
delaying the pain), or never re-order at all (which sucks).

>
> >
> >  mm/zswap.c | 1961 +++++++++++++++++++++++++++++-----------------------------
> >  1 file changed, 971 insertions(+), 990 deletions(-)
> >
  

On (24/01/30 10:52), Johannes Weiner wrote:
> On Tue, Jan 30, 2024 at 09:21:31PM +0900, Sergey Senozhatsky wrote:
> > On (24/01/30 08:16), 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
> > 
> > This also can complicate cherry-picking of patches to stable, prod, .etc
> 
> I'm sensitive to that argument, because we run our own kernel at Meta
> as well.

Well, it was less of an argument and more of a "let's consider that too".

> But moves are pretty easy. The code doesn't actually change, just the
> line offsets. So patch will mostly work with offset warnings. And if
> not, it's easy to fix up and verify. Refactoring and API restructuring
> (folios e.g.) make it much harder when it comes to this.

If pros of doing it are more significant that cons, then OK.
Either way I'm not against the patches.