RE: [PATCH] crypto: add asynchronous compression support

[Joonsoo Kim]

Hello, Herbert.

> -Original Message-
> From: Herbert Xu [mailto:herb...@gondor.apana.org.au]
> Sent: Thursday, November 19, 2015 6:43 PM
> To: Li, Weigang
> Cc: linux-crypto@vger.kernel.org; Struk, Tadeusz; Joonsoo Kim; Sergey
> Senozhatsky
> Subject: Re: [PATCH] crypto: add asynchronous compression support
> 
> On Thu, Nov 19, 2015 at 05:52:41AM +, Li, Weigang wrote:
> >
> > After sync-up with Joonsoo Kim, we think it may be not feasible for a
> s/w implementation of the sg-list based asynchronous interface, we propose
> separate interfaces (patches) for acomp & ccomp. The reasons are:
> > 1. to support sg-list in the ccomp (like what shash/ahash did), the
> partial update is required, some algorithms do not support partial update
> (i.e., lzo), that means:
> 
> No this is not true.  For the ones that don't support partial
> updates you can always linearise the input and then feed it in
> as one chunk.  Because the overall interface you're proposing
> does not allow partial updates the underlying implementation
> doesn't need to do it either.  Only linearisation is necessary.

Linearization would be enough to use sg-list but it has a problem.
Linearization needs sleepable function such as vmap() and it makes
sync (de)compression in atomic context impossible. Currently, zram
did sync (de)compression in atomic context. It uses map_vm_area() which
isn't sleepable function to linearize two separate pages. This is possible
because zram already knows that maximum number of spread pages is just two
and have allocated vm area in advance. But, if we implement linearization
in general API, we can't be sure of request input size so we need
sleepable function, vmap().

And, this sleep could degrade performance.

> > 2. the format of output buffer (sg-list) will be different, e.g., the
> lzo need contain the "length" info for each block in the output sg-list in
> order to de-compression, while zlib doesn't need, then it is difficult to
> have a single async sg-list i/f.
> 
> I have no idea what you mean here.  Please explain.
> 
> > 3. to compress a sg-list buffer, the lzo also requires an intermediate
> buffer to save the output of a block, and copy it back to the sg-list
> output buffer, it will introduce the complexity and cost, we don't see
> value for sg-list support in a s/w compression.
> 
> Such an intermediate buffer would only  be needed if the SG list is
> actually non-linear.  So I don't see this as an issue.

Intermediate buffer size could vary greatly so it would be allocated and
Freed whenever requested. This could affect performance.

I think that supporting unified API has more loss than gain.
I'm not expert on this area so please let me know what I missed.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: add asynchronous compression support

[Herbert Xu]

On Fri, Nov 20, 2015 at 03:04:47PM +0900, Joonsoo Kim wrote:
>
> Linearization would be enough to use sg-list but it has a problem.
> Linearization needs sleepable function such as vmap() and it makes
> sync (de)compression in atomic context impossible. Currently, zram
> did sync (de)compression in atomic context. It uses map_vm_area() which
> isn't sleepable function to linearize two separate pages. This is possible
> because zram already knows that maximum number of spread pages is just two
> and have allocated vm area in advance. But, if we implement linearization
> in general API, we can't be sure of request input size so we need
> sleepable function, vmap().
> 
> And, this sleep could degrade performance.

Obviously you would only perform linearisation where it's needed.
And if you are in an atomic context, then clearly linearisation
can only be attempted using kmalloc/alloc_pages with GFP_ATOMIC.

I don't understand your concern with zram because either zram is
already feeding us linear buffers in which case linearisation is
never needed.  Or it can only be used with algorithms that support
SG lists or partial updates, which we can easily mark using a flag.

> Intermediate buffer size could vary greatly so it would be allocated and
> Freed whenever requested. This could affect performance.

That's for the crypto user to figure out.  Either they should
supply a completely linear buffer if they want to be able to
support every algorithm in an efficient manner, or they will
either have to eat the cost of linearisation or only use algorithms
that can deal with SG lists efficiently.

We have the same problem with network drivers and it's dealt with
in exactly the same way.  An skb can be an SG list and will be
linearised when necessary.

> I think that supporting unified API has more loss than gain.

I disagree.  I have seen no valid reason so far for adding two
compression interfaces.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: add asynchronous compression support

[Li, Weigang]

On 11/20/2015 2:19 PM, Herbert Xu wrote:

On Fri, Nov 20, 2015 at 03:04:47PM +0900, Joonsoo Kim wrote:


Linearization would be enough to use sg-list but it has a problem.
Linearization needs sleepable function such as vmap() and it makes
sync (de)compression in atomic context impossible. Currently, zram
did sync (de)compression in atomic context. It uses map_vm_area() which
isn't sleepable function to linearize two separate pages. This is possible
because zram already knows that maximum number of spread pages is just two
and have allocated vm area in advance. But, if we implement linearization
in general API, we can't be sure of request input size so we need
sleepable function, vmap().

And, this sleep could degrade performance.


Obviously you would only perform linearisation where it's needed.
And if you are in an atomic context, then clearly linearisation
can only be attempted using kmalloc/alloc_pages with GFP_ATOMIC.

I don't understand your concern with zram because either zram is
already feeding us linear buffers in which case linearisation is
never needed.  Or it can only be used with algorithms that support
SG lists or partial updates, which we can easily mark using a flag.


Intermediate buffer size could vary greatly so it would be allocated and
Freed whenever requested. This could affect performance.


That's for the crypto user to figure out.  Either they should
supply a completely linear buffer if they want to be able to
support every algorithm in an efficient manner, or they will
either have to eat the cost of linearisation or only use algorithms
that can deal with SG lists efficiently.

We have the same problem with network drivers and it's dealt with
in exactly the same way.  An skb can be an SG list and will be
linearised when necessary.


I think that supporting unified API has more loss than gain.


I disagree.  I have seen no valid reason so far for adding two
compression interfaces.

Cheers,


Thanks Herbert.

If we assume the sg-list can be linearized - no "holes" in the sg-list, 
all chunks in middle of the list are of PAGE_SIZE, it seems ok to 
support sg-list in the s/w implementation, linearize the sg-list and 
compress it as one chunk.

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] crypto: add asynchronous compression support

[Li, Weigang]

On 10/16/2015 11:13 PM, Herbert Xu wrote:
> On Fri, Oct 16, 2015 at 11:11:00PM +0800, Weigang Li wrote:
>> This patch set introduces Asynchronous Compression API.
>> What is proposed is a new crypto type called crypto_acomp_type,
>> plus new struct acomp_alg and struct crypto_acomp, together with number
>> of helper functions to register acomp type algorithms and allocate tfm
>> instances. This is to make it similar to how the existing crypto API
>> works for the ablkcipher, and akcipher types.
>> The operations the new interface will provide are:
>>
>>  int (*compress)(struct acompress_request *req);
>>  int (*decompress)(struct acompress_request *req);
>>
>> The benefits it gives interface are:
>> - the new interface allows for asynchronous implementations and
>>scatterlist buffer that can use hardware to offload the compression
>>operations, the new asynchronous API can be called by the linux kernel
>>components (i.e., btrfs) who want to use hardware acceleration for data
>>compression.
>>
>> New helper functions have been added to allocate crypto_acomp instances
>> and invoke the operations to make it easier to use.
>>
>> Signed-off-by: Weigang Li 
> 
> Thanks for the patch! Joonsoo Kim is also working on the compression
> interface for zram.  Could you two collaborate and come up with one
> interface rather than two?
> 
> Cheers,
> 
Hello Herbert,

After sync-up with Joonsoo Kim, we think it may be not feasible for a s/w 
implementation of the sg-list based asynchronous interface, we propose separate 
interfaces (patches) for acomp & ccomp. The reasons are:
1. to support sg-list in the ccomp (like what shash/ahash did), the partial 
update is required, some algorithms do not support partial update (i.e., lzo), 
that means:
2. the format of output buffer (sg-list) will be different, e.g., the lzo need 
contain the "length" info for each block in the output sg-list in order to 
de-compression, while zlib doesn't need, then it is difficult to have a single 
async sg-list i/f.
3. to compress a sg-list buffer, the lzo also requires an intermediate buffer 
to save the output of a block, and copy it back to the sg-list output buffer, 
it will introduce the complexity and cost, we don't see value for sg-list 
support in a s/w compression.

Any suggestions?
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] crypto: add asynchronous compression support

[Li, Weigang]

After sync with Joonsoo Kim offline, he agreed to merge this acomp patch with 
his ccomp patch, thanks Joonsoo!

-Original Message-
From: Herbert Xu [mailto:herb...@gondor.apana.org.au] 
Sent: Friday, October 16, 2015 11:14 PM
To: Li, Weigang
Cc: linux-crypto@vger.kernel.org; Struk, Tadeusz; Joonsoo Kim; Sergey 
Senozhatsky
Subject: Re: [PATCH] crypto: add asynchronous compression support

On Fri, Oct 16, 2015 at 11:11:00PM +0800, Weigang Li wrote:
> This patch set introduces Asynchronous Compression API.
> What is proposed is a new crypto type called crypto_acomp_type, plus 
> new struct acomp_alg and struct crypto_acomp, together with number of 
> helper functions to register acomp type algorithms and allocate tfm 
> instances. This is to make it similar to how the existing crypto API 
> works for the ablkcipher, and akcipher types.
> The operations the new interface will provide are:
> 
>   int (*compress)(struct acompress_request *req);
>   int (*decompress)(struct acompress_request *req);
> 
> The benefits it gives interface are:
> - the new interface allows for asynchronous implementations and
>   scatterlist buffer that can use hardware to offload the compression
>   operations, the new asynchronous API can be called by the linux kernel
>   components (i.e., btrfs) who want to use hardware acceleration for data
>   compression.
> 
> New helper functions have been added to allocate crypto_acomp 
> instances and invoke the operations to make it easier to use.
> 
> Signed-off-by: Weigang Li <weigang...@intel.com>

Thanks for the patch! Joonsoo Kim is also working on the compression interface 
for zram.  Could you two collaborate and come up with one interface rather than 
two?

Cheers,
--
Email: Herbert Xu <herb...@gondor.apana.org.au> Home Page: 
http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: add asynchronous compression support

[Dan Streetman]

On Wed, Oct 21, 2015 at 3:59 AM, Li, Weigang <weigang...@intel.com> wrote:
>> -Original Message-
>> From: Herbert Xu [mailto:herb...@gondor.apana.org.au]
>> Sent: Wednesday, October 21, 2015 3:34 PM
>> To: Sergey Senozhatsky
>> Cc: Minchan Kim; Joonsoo Kim; Dan Streetman; Seth Jennings; Li, Weigang;
>> Struk, Tadeusz
>> Subject: Re: [PATCH] crypto: add asynchronous compression support
>>
>> On Wed, Oct 21, 2015 at 04:33:22PM +0900, Sergey Senozhatsky wrote:
>> >
>> > the thing is -- I still want to have/use SW compressors; and they [the
>> > existing S/W algorithms] want virtual addresses. so we need to
>> > kmap_atomic pages in every SW algorithm? isn't it simpler to keep
>> > kmap_atomic_to_page around?
>>
>> The Crypto API will do it for you.  Have a look at how we handle it for aead
>> and ahash for example.
>>
>> Cheers,
>> --
>> Email: Herbert Xu <herb...@gondor.apana.org.au> Home Page:
>> http://gondor.apana.org.au/~herbert/
>> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>
> Adding the "linux-crypto" list back to the latest discussion.

re: zswap, I suspect it won't be able to use async compression,
because when its store function returns the page must be available for
reallocation.  Of course, async compression can be useful elsewhere.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] crypto: add asynchronous compression support

[Li, Weigang]

> -Original Message-
> From: Herbert Xu [mailto:herb...@gondor.apana.org.au]
> Sent: Wednesday, October 21, 2015 3:34 PM
> To: Sergey Senozhatsky
> Cc: Minchan Kim; Joonsoo Kim; Dan Streetman; Seth Jennings; Li, Weigang;
> Struk, Tadeusz
> Subject: Re: [PATCH] crypto: add asynchronous compression support
> 
> On Wed, Oct 21, 2015 at 04:33:22PM +0900, Sergey Senozhatsky wrote:
> >
> > the thing is -- I still want to have/use SW compressors; and they [the
> > existing S/W algorithms] want virtual addresses. so we need to
> > kmap_atomic pages in every SW algorithm? isn't it simpler to keep
> > kmap_atomic_to_page around?
> 
> The Crypto API will do it for you.  Have a look at how we handle it for aead
> and ahash for example.
> 
> Cheers,
> --
> Email: Herbert Xu <herb...@gondor.apana.org.au> Home Page:
> http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Adding the "linux-crypto" list back to the latest discussion.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] crypto: add asynchronous compression support

[Weigang Li]

This patch set introduces Asynchronous Compression API.
What is proposed is a new crypto type called crypto_acomp_type, 
plus new struct acomp_alg and struct crypto_acomp, together with number 
of helper functions to register acomp type algorithms and allocate tfm 
instances. This is to make it similar to how the existing crypto API 
works for the ablkcipher, and akcipher types.
The operations the new interface will provide are:

int (*compress)(struct acompress_request *req);
int (*decompress)(struct acompress_request *req);

The benefits it gives interface are:
- the new interface allows for asynchronous implementations and
  scatterlist buffer that can use hardware to offload the compression
  operations, the new asynchronous API can be called by the linux kernel
  components (i.e., btrfs) who want to use hardware acceleration for data
  compression.

New helper functions have been added to allocate crypto_acomp instances 
and invoke the operations to make it easier to use.

Signed-off-by: Weigang Li 
---
 crypto/Kconfig  |5 +
 crypto/Makefile |1 +
 crypto/acompress.c  |  116 
 crypto/crypto_user.c|   21 +++
 include/crypto/acompress.h  |  251 +++
 include/crypto/internal/acompress.h |   62 +
 include/linux/crypto.h  |1 +
 include/uapi/linux/cryptouser.h |5 +
 8 files changed, 462 insertions(+), 0 deletions(-)
 create mode 100644 crypto/acompress.c
 create mode 100644 include/crypto/acompress.h
 create mode 100644 include/crypto/internal/acompress.h

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 7240821..8b8d2d6 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -93,6 +93,10 @@ config CRYPTO_PCOMP2
tristate
select CRYPTO_ALGAPI2
 
+config CRYPTO_ACOMP2
+tristate
+select CRYPTO_ALGAPI2
+
 config CRYPTO_AKCIPHER2
tristate
select CRYPTO_ALGAPI2
@@ -123,6 +127,7 @@ config CRYPTO_MANAGER2
select CRYPTO_HASH2
select CRYPTO_BLKCIPHER2
select CRYPTO_PCOMP2
+   select CRYPTO_ACOMP2
select CRYPTO_AKCIPHER2
 
 config CRYPTO_USER
diff --git a/crypto/Makefile b/crypto/Makefile
index f7aba92..6ad718f 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -29,6 +29,7 @@ crypto_hash-y += shash.o
 obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o
 
 obj-$(CONFIG_CRYPTO_PCOMP2) += pcompress.o
+obj-$(CONFIG_CRYPTO_ACOMP2) += acompress.o
 obj-$(CONFIG_CRYPTO_AKCIPHER2) += akcipher.o
 
 $(obj)/rsapubkey-asn1.o: $(obj)/rsapubkey-asn1.c $(obj)/rsapubkey-asn1.h
diff --git a/crypto/acompress.c b/crypto/acompress.c
new file mode 100644
index 000..0becbf1
--- /dev/null
+++ b/crypto/acompress.c
@@ -0,0 +1,116 @@
+/*
+ * Asynchronous compression operations
+ *
+ * Copyright (c) 2015, Intel Corporation
+ * Authors: Weigang Li 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "internal.h"
+
+#ifdef CONFIG_NET
+static int crypto_acompress_report(struct sk_buff *skb, struct crypto_alg *alg)
+{
+   struct crypto_report_acomp racomp;
+
+   strncpy(racomp.type, "acompress", sizeof(racomp.type));
+
+   if (nla_put(skb, CRYPTOCFGA_REPORT_ACOMPRESS,
+   sizeof(struct crypto_report_acomp), ))
+   goto nla_put_failure;
+   return 0;
+
+nla_put_failure:
+   return -EMSGSIZE;
+}
+#else
+static int crypto_acompress_report(struct sk_buff *skb, struct crypto_alg *alg)
+{
+   return -ENOSYS;
+}
+#endif
+
+static void crypto_acompress_show(struct seq_file *m, struct crypto_alg *alg)
+   __attribute__ ((unused));
+
+static void crypto_acompress_show(struct seq_file *m, struct crypto_alg *alg)
+{
+   seq_puts(m, "type : acompress\n");
+}
+
+static void crypto_acompress_exit_tfm(struct crypto_tfm *tfm)
+{
+   struct crypto_acomp *acomp = __crypto_acomp_tfm(tfm);
+   struct acomp_alg *alg = crypto_acomp_alg(acomp);
+
+   alg->exit(acomp);
+}
+
+static int crypto_acompress_init_tfm(struct crypto_tfm *tfm)
+{
+   struct crypto_acomp *acomp = __crypto_acomp_tfm(tfm);
+   struct acomp_alg *alg = crypto_acomp_alg(acomp);
+
+   if (alg->exit)
+   acomp->base.exit = crypto_acompress_exit_tfm;
+
+   if (alg->init)
+   return alg->init(acomp);
+
+   return 0;
+}
+
+static const struct crypto_type crypto_acomp_type = {
+   .extsize = crypto_alg_extsize,
+   .init_tfm = crypto_acompress_init_tfm,
+#ifdef CONFIG_PROC_FS
+   .show = crypto_acompress_show,
+#endif
+   .report