Received-SPF: pass (google.com: domain of linux-crypto+bounces-4581-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 31 May 2024 15:34:44 +0900
From: Sergey Senozhatsky <senozhatsky@chromium.org>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
	Sergey Senozhatsky <senozhatsky@chromium.org>
Subject: Re: [PATCH 1/3] crypto: scomp - Add setparam interface
Message-ID: <20240531063444.GG8400@google.com>
References: <cover.1716202860.git.herbert@gondor.apana.org.au>
 <84523e14722d0629b2ee9c8e7e3c04aa223c5fb5.1716202860.git.herbert@gondor.apana.org.au>
 <20240531054759.GE8400@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240531054759.GE8400@google.com>

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