RE: zram: Optimize LZ4 dictionary compression performance

[gao xu <gaoxu2@honor.com>]

> 
> On (26/03/10 02:54), gao xu wrote:
> > +struct lz4_drv {
> > +	/* template compression stream with dictionary */
> > +	LZ4_stream_t base_cstream;
> > +	/* dictionary index of the current template */
> > +	u32 dict_gen;
> > +	bool base_c_valid;
> > +};
> > +
> >  static void lz4_release_params(struct zcomp_params *params)
> >  {
> > +	kfree(params->drv_data);
> > +	params->drv_data = NULL;
> >  }
> >
> >  static int lz4_setup_params(struct zcomp_params *params)
> >  {
> > +	struct lz4_drv *drv;
> > +
> >  	if (params->level == ZCOMP_PARAM_NOT_SET)
> >  		params->level = LZ4_ACCELERATION_DEFAULT;
> >
> > +	drv = kzalloc_obj(drv, GFP_KERNEL);
> > +	if (!drv)
> > +		return -ENOMEM;
> > +
> > +	drv->dict_gen = 0;
> > +	drv->base_c_valid = false;
> > +
> > +	params->drv_data = drv;
> > +
> >  	return 0;
> >  }
> >
> > @@ -67,10 +88,32 @@ static int lz4_create(struct zcomp_params *params,
> struct zcomp_ctx *ctx)
> >  	return -ENOMEM;
> >  }
> >
> > +static int lz4_build_base_cstream(struct zcomp_params *params)
> > +{
> > +	struct lz4_drv *drv = params->drv_data;
> > +	int ret;
> > +
> > +	if (!params->dict || !params->dict_sz)
> > +		return -EINVAL;
> > +
> > +	memset(&drv->base_cstream, 0, sizeof(drv->base_cstream));
> > +
> > +	ret = LZ4_loadDict(&drv->base_cstream,
> > +			   params->dict, params->dict_sz);
> > +	if (ret != params->dict_sz)
> > +		return -EINVAL;
> > +
> > +	drv->dict_gen = params->dict_gen;
> > +	drv->base_c_valid = true;
> > +
> > +	return 0;
> > +}
> 
> I think this simply can move to ->lz4_setup_params(), becase that
> function is supposed to loadDict.  I guess we can use backend_zstd.c
> as a reference.  Also probably can inherit naming scheme from zstd
> backend as well.
Should I modify it like this?
+static int lz4_create_cstream(struct zcomp_params *params)
+{
+	int ret;
+
+	if (!params->dict || !params->dict_sz)
+		return 0;
+
+	params->drv_data = kzalloc(sizeof(LZ4_stream_t), GFP_KERNEL);
+	if (!params->drv_data)
+		return -ENOMEM;
+
+	ret = LZ4_loadDict((LZ4_stream_t *)params->drv_data,
+			   params->dict, params->dict_sz);
+	if (ret != params->dict_sz) {
+		kfree(params->drv_data);
+		return -EINVAL;
+	}
+
+	return 0;
+}

static int lz4_setup_params(struct zcomp_params *params)
{
	if (params->level == ZCOMP_PARAM_NO_LEVEL)
		params->level = LZ4_ACCELERATION_DEFAULT;

-   return 0;
+	return lz4_create_cstream(params);
}
> 
> >  static int lz4_compress(struct zcomp_params *params, struct zcomp_ctx
> *ctx,
> >  			struct zcomp_req *req)
> >  {
> >  	struct lz4_ctx *zctx = ctx->context;
> > +	struct lz4_drv *drv  = params->drv_data;
> >  	int ret;
> >
> >  	if (!zctx->cstrm) {
> > @@ -78,10 +121,16 @@ static int lz4_compress(struct zcomp_params
> *params, struct zcomp_ctx *ctx,
> >  					req->dst_len, params->level,
> >  					zctx->mem);
> >  	} else {
> > +		/* rebuild base_cstream when the dictionary changes */
> > +		if (!drv->base_c_valid || drv->dict_gen != params->dict_gen) {
> > +			ret = lz4_build_base_cstream(params);
> > +			if (ret)
> > +				return ret;
> > +		}
> 
> We should not need this, unless I'm missing something.  Params should
> change once algorithms are setups and the device is init.
> 
> [..]
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index bca33403f..f34f3fa43 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -1709,6 +1709,7 @@ static int comp_params_store(struct zram *zram,
> u32 prio, s32 level,
> >  	zram->params[prio].dict_sz = sz;
> >  	zram->params[prio].level = level;
> >  	zram->params[prio].deflate.winbits = deflate_params->winbits;
> > +	zram->params[prio].dict_gen++;
> 
> We should not need this, as far as I can see.  It was a bug that we
> permitted params change after init.  I sent a fixup patch.
You are right.