linux-kernel - Re: [PATCHv4 2/9] zsmalloc: turn zspage order into runtime variable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 11 Nov 2022 19:38:10 +0900
From:   Sergey Senozhatsky <senozhatsky@...omium.org>
To:     Minchan Kim <minchan@...nel.org>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Nitin Gupta <ngupta@...are.org>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org
Subject: Re: [PATCHv4 2/9] zsmalloc: turn zspage order into runtime variable

On (22/11/10 13:59), Minchan Kim wrote:
[..]
> > +#define ZS_PAGE_ORDER_2		2
> > +#define ZS_PAGE_ORDER_4		4
> > +
> > +/*
> > + * A single 'zspage' is composed of up to 2^N discontiguous 0-order (single)
> > + * pages. ZS_MAX_PAGE_ORDER defines upper limit on N, ZS_MIN_PAGE_ORDER
> > + * defines lower limit on N. ZS_DEFAULT_PAGE_ORDER is recommended value.
> 
> It gives the impression:
> 
>    2^2 <= the page nubmer of zspage <= 2^4
> 
> I think that's not what you want to describe. How about?
> 
> A single 'zspage' is composed of up to 2^N discontiguous 0-order (single)
> pages and the N can be from ZS_MIN_PAGE_ORDER to ZS_MAX_PAGE_ORDER.

OK.

> > + */
> > +#define ZS_MIN_PAGE_ORDER	ZS_PAGE_ORDER_2
> > +#define ZS_MAX_PAGE_ORDER	ZS_PAGE_ORDER_4
> > +#define ZS_DEFAULT_PAGE_ORDER	ZS_PAGE_ORDER_2
> 
> #define ZS_MIN_PAGE_ORDER	2
> 
> We can use the number directly instead of another wrapping at least
> in this patch(Just in case: if you want to extent it later patch,
> please do it in the patch)

OK.

[..]
> > -#define MAX(a, b) ((a) >= (b) ? (a) : (b))
> > -/* ZS_MIN_ALLOC_SIZE must be multiple of ZS_ALIGN */
> > -#define ZS_MIN_ALLOC_SIZE \
> > -	MAX(32, (ZS_MAX_PAGES_PER_ZSPAGE << PAGE_SHIFT >> OBJ_INDEX_BITS))
> > +#define ZS_MIN_ALLOC_SIZE	32U
> 
> Let's have some comment here to say that's not the final vaule which
> is supposed to be pool->min_alloc_size.

OK.

[..]
> >  enum fullness_group {
> >  	ZS_EMPTY,
> > @@ -230,12 +221,15 @@ struct link_free {
> >  struct zs_pool {
> >  	const char *name;
> >  
> > -	struct size_class *size_class[ZS_SIZE_CLASSES];
> > +	struct size_class **size_class;
> >  	struct kmem_cache *handle_cachep;
> >  	struct kmem_cache *zspage_cachep;
> >  
> >  	atomic_long_t pages_allocated;
> >  
> > +	u32 num_size_classes;
> > +	u32 min_alloc_size;
> 
> Please use int.

OK. Any reason why we don't want u32? I thought that
s16/u16/s32/u32/etc. is the new normal.

> From this patch, I couldn't figure why we need
> variable in the pool. Let's have the change in the patch where
> you really need to have the usecase.

Let me take a look.

> > -static int get_pages_per_zspage(int class_size)
> > +static int get_pages_per_zspage(u32 class_size, u32 num_pages)
> 
> Let's just use int instead of u32
> 
> Why do you need num_pages argument instead of using 1UL << ZS_DEFAULT_PAGE_ORDER?
> It looks like static value.

It is static right now, but in the a couple of patches it'll change to
dynamic.