Re: [PATCH 1/3] crypto: scomp - Add setparam interface

[Sergey Senozhatsky <senozhatsky@chromium.org>]

On (24/05/31 14:47), Sergey Senozhatsky wrote:
> On (24/05/20 19:04), Herbert Xu wrote:
> [..]
> > +int crypto_scomp_setparam(struct crypto_scomp *tfm, const u8 *param,
> > +			  unsigned int len)
> > +{
> > +	struct scomp_alg *scomp = crypto_scomp_alg(tfm);
> > +	int err;
> > +
> > +	err = scomp->setparam(tfm, param, len);
> > +	if (unlikely(err)) {
> > +		scomp_set_need_param(tfm, scomp);
> > +		return err;
> > +	}
> > +
> > +	crypto_scomp_clear_flags(tfm, CRYPTO_TFM_NEED_KEY);
> > +	return 0;
> > +}
> 
> Is the idea here that each compression driver will have its own structure
> for params?
> 
> In other words, something like this?
> 
> static int setup_tfm(...)
> {
> ...
> 	this_cpu->tfm = crypto_alloc_comp(name, 0, 0);
> 
> 	if (!strcmp(name, "zstd")) {
> 		struct crypto_comp_param_zstd param;
> 
> 		param.dict = ...
> 		param.cleve = ...
> 
> 		crypto_scomp_setparam(tfm, &param, sizeof(param));
> 	}
> 
> 	if (!strcmp(name, "lz4")) {
> 		struct crupto_comp_param_lz4 param;
> 		...
> 	}
> 
> 	if (!strcmp(name, "lzo")) {
> 		struct crupto_comp_param_lzo param;
> 		...
> 	}
> ...
> }
> 
> Or should it be "struct crypto_comp_params param"?

So passing "raw" algorithm parameters to crypto_scomp_setparam(tfm) can be
suboptimal, depending on the compression driver. For instance, for zstd
(what is currently done in zram [1]) we pre-process "raw" parameters:
parse dictionary in order to get zstd_cdict and zstd_ddict which are then
shared by all tfm-s (as they access C/D dictionaries in read-only mode).
For zram/zswap doing this per-tfm would result in extra per-CPU
zstd_cdict/zstd_ddict allocations, which is a significant overhead.

Does this sound like adding two more callbacks to drivers
(e.g. parseparam/freeparam)?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/block/zram/backend_zstd.c?h=next-20240529