| Message ID | 84523e14722d0629b2ee9c8e7e3c04aa223c5fb5.1716202860.git.herbert@gondor.apana.org.au |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] crypto: scomp - Add setparam interface | expand |
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, ¶m, 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"?
On Fri, May 31, 2024 at 03:34:44PM +0900, Sergey Senozhatsky wrote: > > 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. If they share the dictionary, why can't they just share the tfm directly? Or do you actually need to vary the other parameters while keeping the dictionary the same? Cheers,
On Fri, May 31, 2024 at 02:47:59PM +0900, Sergey Senozhatsky wrote: > > Is the idea here that each compression driver will have its own structure > for params? The API is agnostic. You can either have a common format shared by all (your) algorithms, or you can do individual structures. Since you're the only user, you get to make up the rules :) Thanks,
On (24/05/31 16:29), Herbert Xu wrote: > On Fri, May 31, 2024 at 03:34:44PM +0900, Sergey Senozhatsky wrote: > > > > 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. > > If they share the dictionary, why can't they just share the > tfm directly? Or do you actually need to vary the other parameters > while keeping the dictionary the same? Is it possible to share a tfm? I thought that tfm-s carry some state (compression workmem/scratch buffer) so one cannot do parallel compressions on different CPUs using the same tfm.
On Sat, Jun 01, 2024 at 09:24:15AM +0900, Sergey Senozhatsky wrote: > > Is it possible to share a tfm? I thought that tfm-s carry some state > (compression workmem/scratch buffer) so one cannot do parallel compressions > on different CPUs using the same tfm. Yes the tfm can be shared. The data state is kept in the request object. Cheers,
On Mon, Jun 03, 2024 at 05:28:56PM +0900, Sergey Senozhatsky wrote: > > Herbert, I'm not sure I see how tfm sharing is working. > > crypto_tfm carries a pointer to __crt_ctx, which e.g. for zstd > is struct zstd_ctx, where it keeps all the state (zstd_cctx, zstd_dctx, > and compression/decompression workmem buffers). That's the legacy compression interface. You should be looking at the crypto_acomp interface which handles this properly. Cheers,
On (24/06/03 16:34), Herbert Xu wrote: > On Mon, Jun 03, 2024 at 05:28:56PM +0900, Sergey Senozhatsky wrote: > > > > Herbert, I'm not sure I see how tfm sharing is working. > > > > crypto_tfm carries a pointer to __crt_ctx, which e.g. for zstd > > is struct zstd_ctx, where it keeps all the state (zstd_cctx, zstd_dctx, > > and compression/decompression workmem buffers). > > That's the legacy compression interface. You should be looking > at the crypto_acomp interface which handles this properly. Oh, I see, thanks, I didn't know about that, okay now I see what you meant when you said that you'd not add setparams to legacy scomp interface. Alright, so this means 1) zram needs to be converted to acomp interface 2) scomp drivers that zram is using needs to become acomp compatible (for example, I don't think I see acomp support in crypto/zstd.c) 3) zram needs to support setparam 4) zram needs to support tfm sharing, so that setparam can be called once 5) crypto comp drivers need to start support setparam That's quite a bit of work, I should admit.
On Tue, Jun 04, 2024 at 02:09:15PM +0900, Sergey Senozhatsky wrote: > > Alright, so this means > > 1) zram needs to be converted to acomp interface Oops, somehow I was thinking of zswap rather than zram. > 2) scomp drivers that zram is using needs to become acomp compatible > (for example, I don't think I see acomp support in crypto/zstd.c) I think you're still confusing scomp with the legacy compress interface that zram uses. Any algorithm marked as scomp is using the new acomp interface. So zstd is fully available through acomp. > 3) zram needs to support setparam > 4) zram needs to support tfm sharing, so that setparam can be called > once > 5) crypto comp drivers need to start support setparam > > That's quite a bit of work, I should admit. At least we won't be duplicating the compression algorithms. Cheers,
diff --git a/crypto/scompress.c b/crypto/scompress.c index 1cef6bb06a81..283fbea8336e 100644 --- a/crypto/scompress.c +++ b/crypto/scompress.c @@ -37,6 +37,51 @@ static const struct crypto_type crypto_scomp_type; static int scomp_scratch_users; static DEFINE_MUTEX(scomp_lock); +static inline struct crypto_scomp *__crypto_scomp_cast(struct crypto_tfm *tfm) +{ + return container_of(tfm, struct crypto_scomp, base); +} + +static int scomp_no_setparam(struct crypto_scomp *tfm, const u8 *param, + unsigned int len) +{ + return -ENOSYS; +} + +static bool crypto_scomp_alg_has_setparam(struct scomp_alg *alg) +{ + return alg->setparam != scomp_no_setparam; +} + +static bool crypto_scomp_alg_needs_param(struct scomp_alg *alg) +{ + return crypto_scomp_alg_has_setparam(alg) && + !(alg->base.cra_flags & CRYPTO_ALG_OPTIONAL_KEY); +} + +static void scomp_set_need_param(struct crypto_scomp *tfm, + struct scomp_alg *alg) +{ + if (crypto_scomp_alg_needs_param(alg)) + crypto_scomp_set_flags(tfm, CRYPTO_TFM_NEED_KEY); +} + +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; +} + static int __maybe_unused crypto_scomp_report( struct sk_buff *skb, struct crypto_alg *alg) { @@ -100,8 +145,12 @@ static int crypto_scomp_alloc_scratches(void) static int crypto_scomp_init_tfm(struct crypto_tfm *tfm) { + struct crypto_scomp *comp = __crypto_scomp_cast(tfm); + struct scomp_alg *alg = crypto_scomp_alg(comp); int ret = 0; + scomp_set_need_param(comp, alg); + mutex_lock(&scomp_lock); if (!scomp_scratch_users++) ret = crypto_scomp_alloc_scratches(); @@ -277,11 +326,19 @@ static const struct crypto_type crypto_scomp_type = { .tfmsize = offsetof(struct crypto_scomp, base), }; +static void scomp_prepare_alg(struct scomp_alg *alg) +{ + comp_prepare_alg(&alg->calg); + + if (!alg->setparam) + alg->setparam = scomp_no_setparam; +} + int crypto_register_scomp(struct scomp_alg *alg) { struct crypto_alg *base = &alg->calg.base; - comp_prepare_alg(&alg->calg); + scomp_prepare_alg(alg); base->cra_type = &crypto_scomp_type; base->cra_flags |= CRYPTO_ALG_TYPE_SCOMPRESS; diff --git a/include/crypto/internal/scompress.h b/include/crypto/internal/scompress.h index 07a10fd2d321..4a9cf2174c7a 100644 --- a/include/crypto/internal/scompress.h +++ b/include/crypto/internal/scompress.h @@ -27,6 +27,7 @@ struct crypto_scomp { * @free_ctx: Function frees context allocated with alloc_ctx * @compress: Function performs a compress operation * @decompress: Function performs a de-compress operation + * @setparam: Set parameters of the algorithm (e.g., compression level) * @base: Common crypto API algorithm data structure * @calg: Cmonn algorithm data structure shared with acomp */ @@ -39,6 +40,8 @@ struct scomp_alg { int (*decompress)(struct crypto_scomp *tfm, const u8 *src, unsigned int slen, u8 *dst, unsigned int *dlen, void *ctx); + int (*setparam)(struct crypto_scomp *tfm, const u8 *param, + unsigned int len); union { struct COMP_ALG_COMMON; @@ -71,6 +74,21 @@ static inline struct scomp_alg *crypto_scomp_alg(struct crypto_scomp *tfm) return __crypto_scomp_alg(crypto_scomp_tfm(tfm)->__crt_alg); } +static inline u32 crypto_scomp_get_flags(struct crypto_scomp *tfm) +{ + return crypto_tfm_get_flags(crypto_scomp_tfm(tfm)); +} + +static inline void crypto_scomp_set_flags(struct crypto_scomp *tfm, u32 flags) +{ + crypto_tfm_set_flags(crypto_scomp_tfm(tfm), flags); +} + +static inline void crypto_scomp_clear_flags(struct crypto_scomp *tfm, u32 flags) +{ + crypto_tfm_clear_flags(crypto_scomp_tfm(tfm), flags); +} + static inline void *crypto_scomp_alloc_ctx(struct crypto_scomp *tfm) { return crypto_scomp_alg(tfm)->alloc_ctx(tfm); @@ -82,10 +100,16 @@ static inline void crypto_scomp_free_ctx(struct crypto_scomp *tfm, return crypto_scomp_alg(tfm)->free_ctx(tfm, ctx); } +int crypto_scomp_setparam(struct crypto_scomp *tfm, const u8 *param, + unsigned int len); + static inline int crypto_scomp_compress(struct crypto_scomp *tfm, const u8 *src, unsigned int slen, u8 *dst, unsigned int *dlen, void *ctx) { + if (crypto_scomp_get_flags(tfm) & CRYPTO_TFM_NEED_KEY) + return -ENOKEY; + return crypto_scomp_alg(tfm)->compress(tfm, src, slen, dst, dlen, ctx); } @@ -94,6 +118,9 @@ static inline int crypto_scomp_decompress(struct crypto_scomp *tfm, u8 *dst, unsigned int *dlen, void *ctx) { + if (crypto_scomp_get_flags(tfm) & CRYPTO_TFM_NEED_KEY) + return -ENOKEY; + return crypto_scomp_alg(tfm)->decompress(tfm, src, slen, dst, dlen, ctx); }
Add the scompress plumbing for setparam. This is modelled after setkey for shash. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- crypto/scompress.c | 59 ++++++++++++++++++++++++++++- include/crypto/internal/scompress.h | 27 +++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-)