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

[Vishal Moola]

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@xxxxxxxxxx wrote:
> >> From: Alex Shi <alexs@xxxxxxxxxx>
> > 
> >> +	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
>