Re: [PATCH v2 08/11] zsmalloc: encode 1-based obj_idx so valid obj is never 0

[Sergey Senozhatsky <senozhatsky@chromium.org>]

On (26/11/10 18:37), sashiko-bot@kernel.org wrote:
> > @@ -169,9 +170,6 @@
> >  static_assert(ZS_OBJ_IDX_BITS >= ZS_PAGES_PER_ZSPAGE_BITS,
> >  	      "zsmalloc: ZS_MIN_ALLOC_SIZE would exceed ZS_MAX_ALLOC_SIZE");
> >  
> > -/* ZS_MIN_ALLOC_SIZE must be multiple of ZS_ALIGN */
> > -#define ZS_MIN_ALLOC_SIZE \
> > -	MAX(32, (ZS_MAX_PAGES_PER_ZSPAGE << PAGE_SHIFT >> ZS_OBJ_IDX_BITS))
> >  /* each chunk includes extra space to keep handle */
> >  #define ZS_MAX_ALLOC_SIZE	PAGE_SIZE
> >  
> > @@ -189,6 +187,11 @@ static_assert(ZS_OBJ_IDX_BITS >= ZS_PAGES_PER_ZSPAGE_BITS,
> >   *  (reason above)
> >   */
> >  #define ZS_SIZE_CLASS_DELTA	(PAGE_SIZE >> CLASS_BITS)
> > +#define _ZS_MIN_ALLOC_RAW \
> > +	DIV_ROUND_UP(ZS_MAX_PAGES_PER_ZSPAGE << PAGE_SHIFT, ZS_OBJ_IDX_MASK)
> > +#define ZS_MIN_ALLOC_SIZE \
> > +	MIN(ZS_MAX_ALLOC_SIZE, \
> > +	    MAX(32UL, ALIGN(_ZS_MIN_ALLOC_RAW, ZS_SIZE_CLASS_DELTA)))
> >  #define ZS_SIZE_CLASSES	(DIV_ROUND_UP(ZS_MAX_ALLOC_SIZE - ZS_MIN_ALLOC_SIZE, \
> >  				      ZS_SIZE_CLASS_DELTA) + 1)
> 
> [Severity: Medium]
> Should ZS_MIN_ALLOC_SIZE align _ZS_MIN_ALLOC_RAW to ZS_ALIGN instead of
> ZS_SIZE_CLASS_DELTA?
> 
> Because _ZS_MIN_ALLOC_RAW uses DIV_ROUND_UP() with a non-zero numerator, it
> is always at least 1.  On 16KB and 64KB page kernels, ZS_SIZE_CLASS_DELTA is
> 64 and 256 respectively, so ALIGN(_ZS_MIN_ALLOC_RAW, ZS_SIZE_CLASS_DELTA)
> rounds _ZS_MIN_ALLOC_RAW up to 64 or 256 and overrides the 32-byte floor in
> MAX(32UL, ...), eliminating small size classes even when ZS_OBJ_IDX_MASK can
> index 32-byte objects.  As noted in the comment above ZS_SIZE_CLASS_DELTA,
> ZS_MIN_ALLOC_SIZE only needs to be a multiple of ZS_ALIGN.

Hmm need to think about it.

Those min-size-classes are never used anyway, e.g. compressing 16KB to 64
bytes is just not happening often enough (if ever).