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

Minchan Kim
2022-11-11T17:10:10Z

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.