diff mbox series

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

Message ID 84523e14722d0629b2ee9c8e7e3c04aa223c5fb5.1716202860.git.herbert@gondor.apana.org.au
State New
Headers show
Series [1/3] crypto: scomp - Add setparam interface | expand

Commit Message

Herbert Xu May 20, 2024, 11:04 a.m. UTC 
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(-)

Comments

Sergey Senozhatsky May 31, 2024, 5:47 a.m. UTC | #1 
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"?
Herbert Xu May 31, 2024, 8:29 a.m. UTC | #2 
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,
Herbert Xu May 31, 2024, 8:30 a.m. UTC | #3 
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,
Sergey Senozhatsky June 1, 2024, 12:24 a.m. UTC | #4 
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.
Herbert Xu June 1, 2024, 3:54 a.m. UTC | #5 
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,
diff mbox series

Patch

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);
 }