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.
