Re: [PATCH v4 01/22] mm/zsmalloc: add zpdesc memory descriptor for zswap.zpool

Vishal Moola
2024-08-08T18:21:52Z

On Mon, Aug 05, 2024 at 12:06:24PM +0800, Alex Shi wrote:
>
>
> On 8/3/24 2:52 AM, Vishal Moola wrote:
> > On Mon, Jul 29, 2024 at 07:25:13PM +0800, alexs@kernel.org wrote:
> >> From: Alex Shi <alexs@kernel.org>
> >
> >> + static_assert(offsetof(struct page, pg) == offsetof(struct zpdesc, zp))
> >> +
> >> +ZPDESC_MATCH(flags, flags);
> >> +ZPDESC_MATCH(lru, lru);
> >> +ZPDESC_MATCH(mapping, mops);
> >> +ZPDESC_MATCH(index, next);
> >> +ZPDESC_MATCH(index, handle);
> >> +ZPDESC_MATCH(private, zspage);
> >> +#ifdef CONFIG_MEMCG
> >> +ZPDESC_MATCH(memcg_data, memcg_data);
> >> +#endif
> >> +#undef ZPDESC_MATCH
> >> +static_assert(sizeof(struct zpdesc) <= sizeof(struct page));
> >> +
> >> +#define zpdesc_page(zp) (_Generic((zp), \
> >> + const struct zpdesc *: (const struct page *)(zp), \
> >> + struct zpdesc *: (struct page *)(zp)))
> >> +
> >> +#define zpdesc_folio(zp) (_Generic((zp), \
> >> + const struct zpdesc *: (const struct folio *)(zp), \
> >> + struct zpdesc *: (struct folio *)(zp)))
> >> +
> >> +#define page_zpdesc(p) (_Generic((p), \
> >> + const struct page *: (const struct zpdesc *)(p), \
> >> + struct page *: (struct zpdesc *)(p)))
> >> +
> >> +#endif
> >
> > I'm don't think we need both page and folio cast functions for zpdescs.
> > Sticking to pages will probably suffice (and be easiest) since all APIs
> > zsmalloc cares about are already defined.
> >
> > We can stick to 1 "middle-man" descriptor for zpdescs since zsmalloc
> > uses those pages as space to track zspages and nothing more. We'll likely
> > end up completely removing it from zsmalloc once we can allocate
> > memdescs on their own: It seems most (if not all) of the "indirect" members
> > of zpdesc are used as indicators to the rest of core-mm telling them not to
> > mess with that memory.
>
> Yes, that is also my first attempt to skip folio part, but I found we could got
> 6.3% object size reduced on zsmalloc.o file, from 37.2KB to 34.9KB, if we use
> folio series lock and folio_get/put functions. That saving come from compound_head
> check skipping.
> So I wrapped them carefully in zpdesc series functions in zpdesc.h file.
> They should be easy replaced when we use memdescs in the future. Could we keep them
> a while, or ?

IMO, Its alright to keep both pages and folios due to the size reduction.
However if we do keep both, it should be clearer that we Want zpdescs to
be order-0 pages, and the only reason we have folios is that
compound_head() size reduction (and nothing more). I think a comment by
the zpdesc_folio() macro will suffice.

> Thanks
> Alex
>