Re: [PATCH v5 00/21] mm/zsmalloc: add zpdesc memory descriptor for zswap.zpool

[Alex Shi <seakeel@gmail.com>]

On 8/28/24 7:19 AM, Vishal Moola wrote:
> On Wed, Aug 14, 2024 at 03:03:54PM +0900, Sergey Senozhatsky wrote:
>> On (24/08/08 04:37), Matthew Wilcox wrote:
>> [..]
>>>> So I guess if we have something
>>>>
>>>> struct zspage {
>>>> 	..
>>>> 	struct zpdesc *first_desc;
>>>> 	..
>>>> }
>>>>
>>>> and we "chain" zpdesc-s to form a zspage, and make each of them point to
>>>> a corresponding struct page (memdesc -> *page), then it'll resemble current
>>>> zsmalloc and should work for everyone? I also assume for zspdesc-s zsmalloc
>>>> will need to maintain a dedicated kmem_cache?
>>>
>>> Right, we could do that.  Each memdesc has to be a multiple of 16 bytes,
>>> sp we'd be doing something like allocating 32 bytes for each page.
>>> Is there really 32 bytes of information that we want to store for
>>> each page?  Or could we store all of the information in (a somewhat
>>> larger) zspage?  Assuming we allocate 3 pages per zspage, if we allocate
>>> an extra 64 bytes in the zspage, we've saved 32 bytes per zspage.
>>
>> I certainly like (and appreciate) the approach that saves us
>> some bytes here and there.  zsmalloc page can consist of 1 to
>> up to CONFIG_ZSMALLOC_CHAIN_SIZE (max 16) physical pages.  I'm
>> trying to understand (in pseudo-C code) what does a "somewhat larger
>> zspage" mean.  A fixed size array (given that we know the max number
>> of physical pages) per-zspage?
> 
> I haven't had the opportunity to respond until now as I was on vacation.
> 
> With the current approach in a memdesc world, we would do the following:
> 
> 1) kmem_cache_alloc() every single Zpdesc
> 2) Allocate a memdesc/page that points to its own Zpdesc
> 3) Access/Track Zpdescs directly
> 4) Use those Zpdescs to build a Zspage
> 
> An alternative approach would move more metadata storage from a Zpdesc
> into a Zspage instead. That extreme would leave us with:
> 
> 1) kmem_cache_alloc() once for a Zspage
> 2) Allocate a memdesc/page that points to the Zspage
> 3) Use the Zspage to access/track its own subpages (through some magic
> we would have to figure out)
> 4) Zpdescs are just Zspages (since all the information would be in a Zspage)
> 
> IMO, we should introduce zpdescs first, then start to shift
> metadata from "struct zpdesc" into "struct zspage" until we no longer
> need "struct zpdesc". My big concern is whether or not this patchset works
> towards those goals. Will it make consolidating the metadata easier? And are
> these goals feasible (while maintaining the wins of zsmalloc)? Or should we
> aim to leave zsmalloc as it is currently implemented?

Uh, correct me if I am wrong.

IMHO, regarding what this patchset does, it abstracts the memory descriptor usage
for zswap/zram. The descriptor still overlays the struct page; nothing has changed
in that regard. What this patchset accomplishes is the use of folios in the guts
to save some code size, and the introduction of a new concept, zpdesc. 
This patchset is just an initial step; it does not bias the potential changes to 
kmem_alloc or larger zspage modifications. In fact, both approaches require this
fundamental abstract concept: zpdesc.

So I believe this patchset is needed.

Thanks
Alex