Re: [PATCH 2/5] zsmalloc: Consolidate zs_pool's migrate_lock and size_class's locks

[Minchan Kim]

On Thu, Nov 03, 2022 at 02:08:01PM -0400, Johannes Weiner wrote:
< snip >

> > > > I am also worry about that LRU stuff should be part of allocator
> > > > instead of higher level.
> > > 
> > > I'm sorry, but that's not a reasonable objection.
> > > 
> > > These patches implement a core feature of being a zswap backend, using
> > > standard LRU and locking techniques established by the other backends.
> > > 
> > > I don't disagree that it would nicer if zswap had a strong abstraction
> > > for backend pages and a generalized LRU. But that is major surgery on
> > > a codebase of over 6,500 lines. It's not a reasonable ask to change
> > > all that first before implementing a basic feature that's useful now.
> > 
> > With same logic, folks added the LRU logic into their allocators
> > without the effort considering moving the LRU into upper layer.
> > 
> > And then trend is still going on since I have seen multiple times
> > people are trying to add more allocators. So if it's not a reasonable
> > ask to consier, we couldn't stop the trend in the end.
> 
> So there is actually an ongoing effort to do that. Yosry and I have
> spent quite some time on coming up with an LRU design that's
> independent from compression policy over email and at Plumbers.

I am really glad to hear somebody is working toward right direction.

> 
> My concern is more about the order of doing things:
> 
> 1. The missing writeback support is a gaping hole in zsmalloc, which
>    affects production systems. A generalized LRU list is a good idea,
>    but it's a huge task that from a user pov really is not
>    critical. Even from a kernel dev / maintainer POV, there are bigger
>    fish to fry in the zswap code base and the backends than this.

Even though I believe the general LRU in the swap subsystem is way to go,
I was about to suggesting putting the LRU logic in the zswap layer 
to stop this trend since it's not too difficult at my first glance(Sure,
I might miss something clear there). However, if you guys are working
toward the generalized direction, I am totally in favor of the approach
and looking forward to seeing the project under expectation that we will
clean up all the duplicated logic, fixing the weird layering and then
finally supports hierarchical swap writeback for any combinations of swap
devices.

> 
> 2. Refactoring existing functionality is much easier than writing
>    generalized code that simultaneously enables new behavior. zsmalloc
>    is the most complex of our backends. To make its LRU writeback work
>    we had to patch zswap's ->map ordering to accomodate it, e.g. Such
>    tricky changes are easier to make and test incrementally.
> 
>    The generalized LRU project will hugely benefit from already having
>    a proven writeback implementation in zsmalloc, because then all the
>    requirements in zswap and zsmalloc will be in black and white.

Agreed if we are working toward the right direction and this work could
help to fill the gap of the hole until we can see all the requirements
and achieve it.


> 
> > > I get that your main interest is zram, and so this feature isn't of
> > > interest to you. But zram isn't the only user, nor is it the primary
> > 
> > I am interest to the feature but my interest is more of general swap
> > layer to manage the LRU so that it could support any hierarchy among
> > swap devices, not only zswap.
> 
> I think we're on the same page about the longer term goals.

Fingers crossed.