Re: zram: Optimize LZ4 dictionary compression performance

[Sergey Senozhatsky <senozhatsky@chromium.org>]

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.

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