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

[Minchan Kim <minchan@kernel.org>]

On Fri, Nov 11, 2022 at 07:38:10PM +0900, Sergey Senozhatsky wrote:
< snip >

> [..]
> > >  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.

Oh, I didn't know the new normal.

# ag u32 mm/ | wc -l 
65

Then, I'd like to use int to be consistent with others.

> 
> > 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.

Then, plase have the change in the patch you will use to review easier.