Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Dan Streetman]

On Thu, Mar 16, 2017 at 12:33 PM, Herbert Xu
 wrote:
> On Wed, Mar 08, 2017 at 12:38:40PM -0500, Dan Streetman wrote:
>>
>>
>> setting the ASYNC bit makes it synchronous?  that seems backwards...?
>
> You set the ASYNC bit in the mask and leave it clear in the type.
> That way only algorithms with the ASYNC bit off will match.

aha, ok i get it now.

>> zswap gets the fun of being the first crypto compression consumer to
>> switch to the new api? ;-)
>
> BTW I think we should hold off on converting zswap for now.

ok that sounds good, we can wait.  Thanks!

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Dan Streetman]

On Thu, Mar 16, 2017 at 12:33 PM, Herbert Xu
 wrote:
> On Wed, Mar 08, 2017 at 12:38:40PM -0500, Dan Streetman wrote:
>>
>>
>> setting the ASYNC bit makes it synchronous?  that seems backwards...?
>
> You set the ASYNC bit in the mask and leave it clear in the type.
> That way only algorithms with the ASYNC bit off will match.

aha, ok i get it now.

>> zswap gets the fun of being the first crypto compression consumer to
>> switch to the new api? ;-)
>
> BTW I think we should hold off on converting zswap for now.

ok that sounds good, we can wait.  Thanks!

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Herbert Xu]

On Wed, Mar 08, 2017 at 12:38:40PM -0500, Dan Streetman wrote:
>
> zswap gets the fun of being the first crypto compression consumer to
> switch to the new api? ;-)

BTW I think we should hold off on converting zswap for now.

The reason is that I'd like to try out the new interface on IPcomp
and make sure that it actually is able to do decompression piecemeal
which is the main advantage over the existing interface for IPcomp
where we allocate for the worst-case (64K vs average packet size
of 1.5K).

In that process we may have to tweak the interface.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Herbert Xu]

On Wed, Mar 08, 2017 at 12:38:40PM -0500, Dan Streetman wrote:
>
> zswap gets the fun of being the first crypto compression consumer to
> switch to the new api? ;-)

BTW I think we should hold off on converting zswap for now.

The reason is that I'd like to try out the new interface on IPcomp
and make sure that it actually is able to do decompression piecemeal
which is the main advantage over the existing interface for IPcomp
where we allocate for the worst-case (64K vs average packet size
of 1.5K).

In that process we may have to tweak the interface.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Herbert Xu]

On Thu, Mar 16, 2017 at 11:54:43AM -0400, Dan Streetman wrote:
>
> setting the ASYNC bit makes it synchronous?  that seems backwards...?

You set the ASYNC bit in the mask and leave it clear in the type.
That way only algorithms with the ASYNC bit off will match.
 
> Is the acomp interface fully ready for use?

The interface itself is ready but the drivers aren't yet.

So for now only sync implementations are available.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Herbert Xu]

On Thu, Mar 16, 2017 at 11:54:43AM -0400, Dan Streetman wrote:
>
> setting the ASYNC bit makes it synchronous?  that seems backwards...?

You set the ASYNC bit in the mask and leave it clear in the type.
That way only algorithms with the ASYNC bit off will match.
 
> Is the acomp interface fully ready for use?

The interface itself is ready but the drivers aren't yet.

So for now only sync implementations are available.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Dan Streetman]

On Thu, Mar 9, 2017 at 4:39 AM, Herbert Xu  wrote:
> On Wed, Mar 08, 2017 at 12:38:40PM -0500, Dan Streetman wrote:
>>
>> It looks like the crypto_scomp interface is buried under
>> include/crypto/internal/scompress.h, however that's exactly what zswap
>> should be using.  We don't need to switch to an asynchronous interface
>> that's rather significantly more complicated, and then use it in a
>> synchronous way.  The crypto_scomp interface should probably be made
>> public, not an implementation internal.
>
> No scomp is not meant to be used externally.  We provide exactly
> one compression interface and it's acomp.  acomp can be used
> synchronously by setting the CRYPTO_ALG_ASYNC bit in the mask
> field when allocating the algorithm.

setting the ASYNC bit makes it synchronous?  that seems backwards...?

Anyway, I have a few concerns about moving over to using that first,
specifically:

- no docs on acomp in Documentation/crypto/ that I can see
- no place in the crypto code that I can see that parses ALG_ASYNC to
make the crypto_acomp_compress() call synchronous
- no synchronous test in crypto/testmgr.c

Maybe I'm reading the code wrong, but it looks like any compression
backend that is actually scomp, actually does the (de)compression
synchronously.  In crypto_acomp_init_tfm(), if the tfm is not
crypto_acomp_type (and I assume because all the current
implementations register as scomp, they aren't acomp_type) it calls
crypto_init_scomp_ops_async(), which then sets ->compress to
scomp_acomp_compress() and that function appears to directly call the
scomp compression function.  This is just after a very quick look, so
maybe I'm reading it wrong.  I'll look some more, and also add a
synchronous testmgr test so i can understand how it works better.

Is the acomp interface fully ready for use?

>
> The existing compression interface will be phased out.
>
> Cheers,
> --
> Email: Herbert Xu 
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Dan Streetman]

On Thu, Mar 9, 2017 at 4:39 AM, Herbert Xu  wrote:
> On Wed, Mar 08, 2017 at 12:38:40PM -0500, Dan Streetman wrote:
>>
>> It looks like the crypto_scomp interface is buried under
>> include/crypto/internal/scompress.h, however that's exactly what zswap
>> should be using.  We don't need to switch to an asynchronous interface
>> that's rather significantly more complicated, and then use it in a
>> synchronous way.  The crypto_scomp interface should probably be made
>> public, not an implementation internal.
>
> No scomp is not meant to be used externally.  We provide exactly
> one compression interface and it's acomp.  acomp can be used
> synchronously by setting the CRYPTO_ALG_ASYNC bit in the mask
> field when allocating the algorithm.

setting the ASYNC bit makes it synchronous?  that seems backwards...?

Anyway, I have a few concerns about moving over to using that first,
specifically:

- no docs on acomp in Documentation/crypto/ that I can see
- no place in the crypto code that I can see that parses ALG_ASYNC to
make the crypto_acomp_compress() call synchronous
- no synchronous test in crypto/testmgr.c

Maybe I'm reading the code wrong, but it looks like any compression
backend that is actually scomp, actually does the (de)compression
synchronously.  In crypto_acomp_init_tfm(), if the tfm is not
crypto_acomp_type (and I assume because all the current
implementations register as scomp, they aren't acomp_type) it calls
crypto_init_scomp_ops_async(), which then sets ->compress to
scomp_acomp_compress() and that function appears to directly call the
scomp compression function.  This is just after a very quick look, so
maybe I'm reading it wrong.  I'll look some more, and also add a
synchronous testmgr test so i can understand how it works better.

Is the acomp interface fully ready for use?

>
> The existing compression interface will be phased out.
>
> Cheers,
> --
> Email: Herbert Xu 
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Herbert Xu]

On Wed, Mar 08, 2017 at 12:38:40PM -0500, Dan Streetman wrote:
> 
> It looks like the crypto_scomp interface is buried under
> include/crypto/internal/scompress.h, however that's exactly what zswap
> should be using.  We don't need to switch to an asynchronous interface
> that's rather significantly more complicated, and then use it in a
> synchronous way.  The crypto_scomp interface should probably be made
> public, not an implementation internal.

No scomp is not meant to be used externally.  We provide exactly
one compression interface and it's acomp.  acomp can be used
synchronously by setting the CRYPTO_ALG_ASYNC bit in the mask
field when allocating the algorithm.

The existing compression interface will be phased out.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Herbert Xu]

On Wed, Mar 08, 2017 at 12:38:40PM -0500, Dan Streetman wrote:
> 
> It looks like the crypto_scomp interface is buried under
> include/crypto/internal/scompress.h, however that's exactly what zswap
> should be using.  We don't need to switch to an asynchronous interface
> that's rather significantly more complicated, and then use it in a
> synchronous way.  The crypto_scomp interface should probably be made
> public, not an implementation internal.

No scomp is not meant to be used externally.  We provide exactly
one compression interface and it's acomp.  acomp can be used
synchronously by setting the CRYPTO_ALG_ASYNC bit in the mask
field when allocating the algorithm.

The existing compression interface will be phased out.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Dan Streetman]

On Mon, Feb 27, 2017 at 9:40 AM, Mahipal Reddy
 wrote:
> Hi Dan,
> Thanks for your reply.
>
> On Sat, Feb 25, 2017 at 3:51 AM, Dan Streetman  wrote:
>> On Fri, Feb 24, 2017 at 11:05 AM, Mahipal Challa
>>  wrote:
>>> This adds support for kernel's new crypto acomp/scomp framework
>>> to zswap.
>>
>> I don't understand the point of this, zswap can't compress pages
>> asynchronously, so what benefit do we get from using the async crypto
>> api and then immediately waiting for it to finish?  This seems like
>> it's just adding complexity for no reason?
>
> 1) The new crypto acomp/scomp framework, provides both synchronous and
> asynchronous comp/decomp
> functionality with the same async-crypto(acomp) 
> api(include/crypto/acompress.h).
>
> 2) Currently with new crypto acomp/scomp framework, the crypto
> sub-system(crypto/lzo.c, crypto/deflate.c)
> only supports synchronous mode of compression/decompression which
> meets the zswap requirement.
>
> 3) The new crypto acomp/scomp framework is introduced in the 4.10.xx kernel.
> With this new framework, according to Herbert Xu, existing crypto
> comp(CRYPTO_ALG_TYPE_COMPRESS ) api
> is going to be deprecated (which zswap uses).

zswap gets the fun of being the first crypto compression consumer to
switch to the new api? ;-)

It looks like the crypto_scomp interface is buried under
include/crypto/internal/scompress.h, however that's exactly what zswap
should be using.  We don't need to switch to an asynchronous interface
that's rather significantly more complicated, and then use it in a
synchronous way.  The crypto_scomp interface should probably be made
public, not an implementation internal.


>
> 4) Applications like zswap, which use comp/decomp of crypto subsystem,
> at some point will have to be ported to
> the new framework.
>
> Regards,
> -Mahipal
>
>>> Signed-off-by: Mahipal Challa 
>>> Signed-off-by: Vishnu Nair 
>>> ---
>>>  mm/zswap.c | 192 
>>> +++--
>>>  1 file changed, 162 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/mm/zswap.c b/mm/zswap.c
>>> index cabf09e..b29d109 100644
>>> --- a/mm/zswap.c
>>> +++ b/mm/zswap.c
>>> @@ -33,8 +33,10 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #include 
>>>  #include 
>>> @@ -118,9 +120,21 @@ static int zswap_compressor_param_set(const char *,
>>>  * data structures
>>>  **/
>>>
>>> +/**
>>> + * struct zswap_acomp_result - Data structure to store result of acomp 
>>> callback
>>> + * @completion: zswap will wait for completion on this entry
>>> + * @err   : return value from acomp algorithm will be stored here
>>> + */
>>> +struct zswap_acomp_result {
>>> +   struct completion completion;
>>> +   int err;
>>> +};
>>> +
>>>  struct zswap_pool {
>>> struct zpool *zpool;
>>> -   struct crypto_comp * __percpu *tfm;
>>> +   struct crypto_acomp * __percpu *acomp;
>>> +   struct acomp_req * __percpu *acomp_req;
>>> +   struct zswap_acomp_result * __percpu *result;
>>> struct kref kref;
>>> struct list_head list;
>>> struct work_struct work;
>>> @@ -388,30 +402,66 @@ static int zswap_dstmem_dead(unsigned int cpu)
>>>  static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node 
>>> *node)
>>>  {
>>> struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, 
>>> node);
>>> -   struct crypto_comp *tfm;
>>> +   struct crypto_acomp *acomp;
>>> +   struct acomp_req *acomp_req;
>>> +   struct zswap_acomp_result *result;
>>>
>>> -   if (WARN_ON(*per_cpu_ptr(pool->tfm, cpu)))
>>> +   if (WARN_ON(*per_cpu_ptr(pool->acomp, cpu)))
>>> return 0;
>>> +   if (WARN_ON(*per_cpu_ptr(pool->acomp_req, cpu)))
>>> +   return 0;
>>> +   if (WARN_ON(*per_cpu_ptr(pool->result, cpu)))
>>> +   return 0;
>>> +
>>> +   acomp = crypto_alloc_acomp(pool->tfm_name, 0, 0);
>>> +   if (IS_ERR_OR_NULL(acomp)) {
>>> +   pr_err("could not alloc crypto acomp %s : %ld\n",
>>> +  pool->tfm_name, PTR_ERR(acomp));
>>> +   return -ENOMEM;
>>> +   }
>>> +   *per_cpu_ptr(pool->acomp, cpu) = acomp;
>>> +
>>> +   acomp_req = acomp_request_alloc(acomp);
>>> +   if (IS_ERR_OR_NULL(acomp_req)) {
>>> +   pr_err("could not alloc crypto acomp %s : %ld\n",
>>> +  pool->tfm_name, PTR_ERR(acomp));
>>> +   return -ENOMEM;
>>> +   }
>>> +   *per_cpu_ptr(pool->acomp_req, cpu) = acomp_req;
>>>
>>> -   tfm = crypto_alloc_comp(pool->tfm_name, 0, 0);
>>> -   if (IS_ERR_OR_NULL(tfm)) {
>>> -   pr_err("could not alloc crypto comp %s : %ld\n",
>>> -  pool->tfm_name, PTR_ERR(tfm));
>>> +   

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Dan Streetman]

On Mon, Feb 27, 2017 at 9:40 AM, Mahipal Reddy
 wrote:
> Hi Dan,
> Thanks for your reply.
>
> On Sat, Feb 25, 2017 at 3:51 AM, Dan Streetman  wrote:
>> On Fri, Feb 24, 2017 at 11:05 AM, Mahipal Challa
>>  wrote:
>>> This adds support for kernel's new crypto acomp/scomp framework
>>> to zswap.
>>
>> I don't understand the point of this, zswap can't compress pages
>> asynchronously, so what benefit do we get from using the async crypto
>> api and then immediately waiting for it to finish?  This seems like
>> it's just adding complexity for no reason?
>
> 1) The new crypto acomp/scomp framework, provides both synchronous and
> asynchronous comp/decomp
> functionality with the same async-crypto(acomp) 
> api(include/crypto/acompress.h).
>
> 2) Currently with new crypto acomp/scomp framework, the crypto
> sub-system(crypto/lzo.c, crypto/deflate.c)
> only supports synchronous mode of compression/decompression which
> meets the zswap requirement.
>
> 3) The new crypto acomp/scomp framework is introduced in the 4.10.xx kernel.
> With this new framework, according to Herbert Xu, existing crypto
> comp(CRYPTO_ALG_TYPE_COMPRESS ) api
> is going to be deprecated (which zswap uses).

zswap gets the fun of being the first crypto compression consumer to
switch to the new api? ;-)

It looks like the crypto_scomp interface is buried under
include/crypto/internal/scompress.h, however that's exactly what zswap
should be using.  We don't need to switch to an asynchronous interface
that's rather significantly more complicated, and then use it in a
synchronous way.  The crypto_scomp interface should probably be made
public, not an implementation internal.


>
> 4) Applications like zswap, which use comp/decomp of crypto subsystem,
> at some point will have to be ported to
> the new framework.
>
> Regards,
> -Mahipal
>
>>> Signed-off-by: Mahipal Challa 
>>> Signed-off-by: Vishnu Nair 
>>> ---
>>>  mm/zswap.c | 192 
>>> +++--
>>>  1 file changed, 162 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/mm/zswap.c b/mm/zswap.c
>>> index cabf09e..b29d109 100644
>>> --- a/mm/zswap.c
>>> +++ b/mm/zswap.c
>>> @@ -33,8 +33,10 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #include 
>>>  #include 
>>> @@ -118,9 +120,21 @@ static int zswap_compressor_param_set(const char *,
>>>  * data structures
>>>  **/
>>>
>>> +/**
>>> + * struct zswap_acomp_result - Data structure to store result of acomp 
>>> callback
>>> + * @completion: zswap will wait for completion on this entry
>>> + * @err   : return value from acomp algorithm will be stored here
>>> + */
>>> +struct zswap_acomp_result {
>>> +   struct completion completion;
>>> +   int err;
>>> +};
>>> +
>>>  struct zswap_pool {
>>> struct zpool *zpool;
>>> -   struct crypto_comp * __percpu *tfm;
>>> +   struct crypto_acomp * __percpu *acomp;
>>> +   struct acomp_req * __percpu *acomp_req;
>>> +   struct zswap_acomp_result * __percpu *result;
>>> struct kref kref;
>>> struct list_head list;
>>> struct work_struct work;
>>> @@ -388,30 +402,66 @@ static int zswap_dstmem_dead(unsigned int cpu)
>>>  static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node 
>>> *node)
>>>  {
>>> struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, 
>>> node);
>>> -   struct crypto_comp *tfm;
>>> +   struct crypto_acomp *acomp;
>>> +   struct acomp_req *acomp_req;
>>> +   struct zswap_acomp_result *result;
>>>
>>> -   if (WARN_ON(*per_cpu_ptr(pool->tfm, cpu)))
>>> +   if (WARN_ON(*per_cpu_ptr(pool->acomp, cpu)))
>>> return 0;
>>> +   if (WARN_ON(*per_cpu_ptr(pool->acomp_req, cpu)))
>>> +   return 0;
>>> +   if (WARN_ON(*per_cpu_ptr(pool->result, cpu)))
>>> +   return 0;
>>> +
>>> +   acomp = crypto_alloc_acomp(pool->tfm_name, 0, 0);
>>> +   if (IS_ERR_OR_NULL(acomp)) {
>>> +   pr_err("could not alloc crypto acomp %s : %ld\n",
>>> +  pool->tfm_name, PTR_ERR(acomp));
>>> +   return -ENOMEM;
>>> +   }
>>> +   *per_cpu_ptr(pool->acomp, cpu) = acomp;
>>> +
>>> +   acomp_req = acomp_request_alloc(acomp);
>>> +   if (IS_ERR_OR_NULL(acomp_req)) {
>>> +   pr_err("could not alloc crypto acomp %s : %ld\n",
>>> +  pool->tfm_name, PTR_ERR(acomp));
>>> +   return -ENOMEM;
>>> +   }
>>> +   *per_cpu_ptr(pool->acomp_req, cpu) = acomp_req;
>>>
>>> -   tfm = crypto_alloc_comp(pool->tfm_name, 0, 0);
>>> -   if (IS_ERR_OR_NULL(tfm)) {
>>> -   pr_err("could not alloc crypto comp %s : %ld\n",
>>> -  pool->tfm_name, PTR_ERR(tfm));
>>> +   result = kzalloc(sizeof(*result), GFP_KERNEL);
>>> +   if (IS_ERR_OR_NULL(result)) {
>>> +   pr_err("Could 

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Mahipal Reddy]

Hi Dan,
Thanks for your reply.

On Sat, Feb 25, 2017 at 3:51 AM, Dan Streetman  wrote:
> On Fri, Feb 24, 2017 at 11:05 AM, Mahipal Challa
>  wrote:
>> This adds support for kernel's new crypto acomp/scomp framework
>> to zswap.
>
> I don't understand the point of this, zswap can't compress pages
> asynchronously, so what benefit do we get from using the async crypto
> api and then immediately waiting for it to finish?  This seems like
> it's just adding complexity for no reason?

1) The new crypto acomp/scomp framework, provides both synchronous and
asynchronous comp/decomp
functionality with the same async-crypto(acomp) api(include/crypto/acompress.h).

2) Currently with new crypto acomp/scomp framework, the crypto
sub-system(crypto/lzo.c, crypto/deflate.c)
only supports synchronous mode of compression/decompression which
meets the zswap requirement.

3) The new crypto acomp/scomp framework is introduced in the 4.10.xx kernel.
With this new framework, according to Herbert Xu, existing crypto
comp(CRYPTO_ALG_TYPE_COMPRESS ) api
is going to be deprecated (which zswap uses).

4) Applications like zswap, which use comp/decomp of crypto subsystem,
at some point will have to be ported to
the new framework.

Regards,
-Mahipal

>> Signed-off-by: Mahipal Challa 
>> Signed-off-by: Vishnu Nair 
>> ---
>>  mm/zswap.c | 192 
>> +++--
>>  1 file changed, 162 insertions(+), 30 deletions(-)
>>
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index cabf09e..b29d109 100644
>> --- a/mm/zswap.c
>> +++ b/mm/zswap.c
>> @@ -33,8 +33,10 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #include 
>> @@ -118,9 +120,21 @@ static int zswap_compressor_param_set(const char *,
>>  * data structures
>>  **/
>>
>> +/**
>> + * struct zswap_acomp_result - Data structure to store result of acomp 
>> callback
>> + * @completion: zswap will wait for completion on this entry
>> + * @err   : return value from acomp algorithm will be stored here
>> + */
>> +struct zswap_acomp_result {
>> +   struct completion completion;
>> +   int err;
>> +};
>> +
>>  struct zswap_pool {
>> struct zpool *zpool;
>> -   struct crypto_comp * __percpu *tfm;
>> +   struct crypto_acomp * __percpu *acomp;
>> +   struct acomp_req * __percpu *acomp_req;
>> +   struct zswap_acomp_result * __percpu *result;
>> struct kref kref;
>> struct list_head list;
>> struct work_struct work;
>> @@ -388,30 +402,66 @@ static int zswap_dstmem_dead(unsigned int cpu)
>>  static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
>>  {
>> struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);
>> -   struct crypto_comp *tfm;
>> +   struct crypto_acomp *acomp;
>> +   struct acomp_req *acomp_req;
>> +   struct zswap_acomp_result *result;
>>
>> -   if (WARN_ON(*per_cpu_ptr(pool->tfm, cpu)))
>> +   if (WARN_ON(*per_cpu_ptr(pool->acomp, cpu)))
>> return 0;
>> +   if (WARN_ON(*per_cpu_ptr(pool->acomp_req, cpu)))
>> +   return 0;
>> +   if (WARN_ON(*per_cpu_ptr(pool->result, cpu)))
>> +   return 0;
>> +
>> +   acomp = crypto_alloc_acomp(pool->tfm_name, 0, 0);
>> +   if (IS_ERR_OR_NULL(acomp)) {
>> +   pr_err("could not alloc crypto acomp %s : %ld\n",
>> +  pool->tfm_name, PTR_ERR(acomp));
>> +   return -ENOMEM;
>> +   }
>> +   *per_cpu_ptr(pool->acomp, cpu) = acomp;
>> +
>> +   acomp_req = acomp_request_alloc(acomp);
>> +   if (IS_ERR_OR_NULL(acomp_req)) {
>> +   pr_err("could not alloc crypto acomp %s : %ld\n",
>> +  pool->tfm_name, PTR_ERR(acomp));
>> +   return -ENOMEM;
>> +   }
>> +   *per_cpu_ptr(pool->acomp_req, cpu) = acomp_req;
>>
>> -   tfm = crypto_alloc_comp(pool->tfm_name, 0, 0);
>> -   if (IS_ERR_OR_NULL(tfm)) {
>> -   pr_err("could not alloc crypto comp %s : %ld\n",
>> -  pool->tfm_name, PTR_ERR(tfm));
>> +   result = kzalloc(sizeof(*result), GFP_KERNEL);
>> +   if (IS_ERR_OR_NULL(result)) {
>> +   pr_err("Could not initialize completion on result\n");
>> return -ENOMEM;
>> }
>> -   *per_cpu_ptr(pool->tfm, cpu) = tfm;
>> +   init_completion(>completion);
>> +   *per_cpu_ptr(pool->result, cpu) = result;
>> +
>> return 0;
>>  }
>>
>>  static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node)
>>  {
>> struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);
>> -   struct crypto_comp *tfm;
>> +   struct crypto_acomp *acomp;
>> +   struct acomp_req *acomp_req;
>> +   struct zswap_acomp_result 

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Mahipal Reddy]

Hi Dan,
Thanks for your reply.

On Sat, Feb 25, 2017 at 3:51 AM, Dan Streetman  wrote:
> On Fri, Feb 24, 2017 at 11:05 AM, Mahipal Challa
>  wrote:
>> This adds support for kernel's new crypto acomp/scomp framework
>> to zswap.
>
> I don't understand the point of this, zswap can't compress pages
> asynchronously, so what benefit do we get from using the async crypto
> api and then immediately waiting for it to finish?  This seems like
> it's just adding complexity for no reason?

1) The new crypto acomp/scomp framework, provides both synchronous and
asynchronous comp/decomp
functionality with the same async-crypto(acomp) api(include/crypto/acompress.h).

2) Currently with new crypto acomp/scomp framework, the crypto
sub-system(crypto/lzo.c, crypto/deflate.c)
only supports synchronous mode of compression/decompression which
meets the zswap requirement.

3) The new crypto acomp/scomp framework is introduced in the 4.10.xx kernel.
With this new framework, according to Herbert Xu, existing crypto
comp(CRYPTO_ALG_TYPE_COMPRESS ) api
is going to be deprecated (which zswap uses).

4) Applications like zswap, which use comp/decomp of crypto subsystem,
at some point will have to be ported to
the new framework.

Regards,
-Mahipal

>> Signed-off-by: Mahipal Challa 
>> Signed-off-by: Vishnu Nair 
>> ---
>>  mm/zswap.c | 192 
>> +++--
>>  1 file changed, 162 insertions(+), 30 deletions(-)
>>
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index cabf09e..b29d109 100644
>> --- a/mm/zswap.c
>> +++ b/mm/zswap.c
>> @@ -33,8 +33,10 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #include 
>> @@ -118,9 +120,21 @@ static int zswap_compressor_param_set(const char *,
>>  * data structures
>>  **/
>>
>> +/**
>> + * struct zswap_acomp_result - Data structure to store result of acomp 
>> callback
>> + * @completion: zswap will wait for completion on this entry
>> + * @err   : return value from acomp algorithm will be stored here
>> + */
>> +struct zswap_acomp_result {
>> +   struct completion completion;
>> +   int err;
>> +};
>> +
>>  struct zswap_pool {
>> struct zpool *zpool;
>> -   struct crypto_comp * __percpu *tfm;
>> +   struct crypto_acomp * __percpu *acomp;
>> +   struct acomp_req * __percpu *acomp_req;
>> +   struct zswap_acomp_result * __percpu *result;
>> struct kref kref;
>> struct list_head list;
>> struct work_struct work;
>> @@ -388,30 +402,66 @@ static int zswap_dstmem_dead(unsigned int cpu)
>>  static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
>>  {
>> struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);
>> -   struct crypto_comp *tfm;
>> +   struct crypto_acomp *acomp;
>> +   struct acomp_req *acomp_req;
>> +   struct zswap_acomp_result *result;
>>
>> -   if (WARN_ON(*per_cpu_ptr(pool->tfm, cpu)))
>> +   if (WARN_ON(*per_cpu_ptr(pool->acomp, cpu)))
>> return 0;
>> +   if (WARN_ON(*per_cpu_ptr(pool->acomp_req, cpu)))
>> +   return 0;
>> +   if (WARN_ON(*per_cpu_ptr(pool->result, cpu)))
>> +   return 0;
>> +
>> +   acomp = crypto_alloc_acomp(pool->tfm_name, 0, 0);
>> +   if (IS_ERR_OR_NULL(acomp)) {
>> +   pr_err("could not alloc crypto acomp %s : %ld\n",
>> +  pool->tfm_name, PTR_ERR(acomp));
>> +   return -ENOMEM;
>> +   }
>> +   *per_cpu_ptr(pool->acomp, cpu) = acomp;
>> +
>> +   acomp_req = acomp_request_alloc(acomp);
>> +   if (IS_ERR_OR_NULL(acomp_req)) {
>> +   pr_err("could not alloc crypto acomp %s : %ld\n",
>> +  pool->tfm_name, PTR_ERR(acomp));
>> +   return -ENOMEM;
>> +   }
>> +   *per_cpu_ptr(pool->acomp_req, cpu) = acomp_req;
>>
>> -   tfm = crypto_alloc_comp(pool->tfm_name, 0, 0);
>> -   if (IS_ERR_OR_NULL(tfm)) {
>> -   pr_err("could not alloc crypto comp %s : %ld\n",
>> -  pool->tfm_name, PTR_ERR(tfm));
>> +   result = kzalloc(sizeof(*result), GFP_KERNEL);
>> +   if (IS_ERR_OR_NULL(result)) {
>> +   pr_err("Could not initialize completion on result\n");
>> return -ENOMEM;
>> }
>> -   *per_cpu_ptr(pool->tfm, cpu) = tfm;
>> +   init_completion(>completion);
>> +   *per_cpu_ptr(pool->result, cpu) = result;
>> +
>> return 0;
>>  }
>>
>>  static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node)
>>  {
>> struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);
>> -   struct crypto_comp *tfm;
>> +   struct crypto_acomp *acomp;
>> +   struct acomp_req *acomp_req;
>> +   struct zswap_acomp_result *result;
>> +
>> +   acomp_req = *per_cpu_ptr(pool->acomp_req, cpu);
>> +   if 

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Dan Streetman]

On Fri, Feb 24, 2017 at 11:05 AM, Mahipal Challa
 wrote:
> This adds support for kernel's new crypto acomp/scomp framework
> to zswap.

I don't understand the point of this, zswap can't compress pages
asynchronously, so what benefit do we get from using the async crypto
api and then immediately waiting for it to finish?  This seems like
it's just adding complexity for no reason?

>
> Signed-off-by: Mahipal Challa 
> Signed-off-by: Vishnu Nair 
> ---
>  mm/zswap.c | 192 
> +++--
>  1 file changed, 162 insertions(+), 30 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index cabf09e..b29d109 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -33,8 +33,10 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -118,9 +120,21 @@ static int zswap_compressor_param_set(const char *,
>  * data structures
>  **/
>
> +/**
> + * struct zswap_acomp_result - Data structure to store result of acomp 
> callback
> + * @completion: zswap will wait for completion on this entry
> + * @err   : return value from acomp algorithm will be stored here
> + */
> +struct zswap_acomp_result {
> +   struct completion completion;
> +   int err;
> +};
> +
>  struct zswap_pool {
> struct zpool *zpool;
> -   struct crypto_comp * __percpu *tfm;
> +   struct crypto_acomp * __percpu *acomp;
> +   struct acomp_req * __percpu *acomp_req;
> +   struct zswap_acomp_result * __percpu *result;
> struct kref kref;
> struct list_head list;
> struct work_struct work;
> @@ -388,30 +402,66 @@ static int zswap_dstmem_dead(unsigned int cpu)
>  static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
>  {
> struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);
> -   struct crypto_comp *tfm;
> +   struct crypto_acomp *acomp;
> +   struct acomp_req *acomp_req;
> +   struct zswap_acomp_result *result;
>
> -   if (WARN_ON(*per_cpu_ptr(pool->tfm, cpu)))
> +   if (WARN_ON(*per_cpu_ptr(pool->acomp, cpu)))
> return 0;
> +   if (WARN_ON(*per_cpu_ptr(pool->acomp_req, cpu)))
> +   return 0;
> +   if (WARN_ON(*per_cpu_ptr(pool->result, cpu)))
> +   return 0;
> +
> +   acomp = crypto_alloc_acomp(pool->tfm_name, 0, 0);
> +   if (IS_ERR_OR_NULL(acomp)) {
> +   pr_err("could not alloc crypto acomp %s : %ld\n",
> +  pool->tfm_name, PTR_ERR(acomp));
> +   return -ENOMEM;
> +   }
> +   *per_cpu_ptr(pool->acomp, cpu) = acomp;
> +
> +   acomp_req = acomp_request_alloc(acomp);
> +   if (IS_ERR_OR_NULL(acomp_req)) {
> +   pr_err("could not alloc crypto acomp %s : %ld\n",
> +  pool->tfm_name, PTR_ERR(acomp));
> +   return -ENOMEM;
> +   }
> +   *per_cpu_ptr(pool->acomp_req, cpu) = acomp_req;
>
> -   tfm = crypto_alloc_comp(pool->tfm_name, 0, 0);
> -   if (IS_ERR_OR_NULL(tfm)) {
> -   pr_err("could not alloc crypto comp %s : %ld\n",
> -  pool->tfm_name, PTR_ERR(tfm));
> +   result = kzalloc(sizeof(*result), GFP_KERNEL);
> +   if (IS_ERR_OR_NULL(result)) {
> +   pr_err("Could not initialize completion on result\n");
> return -ENOMEM;
> }
> -   *per_cpu_ptr(pool->tfm, cpu) = tfm;
> +   init_completion(>completion);
> +   *per_cpu_ptr(pool->result, cpu) = result;
> +
> return 0;
>  }
>
>  static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node)
>  {
> struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);
> -   struct crypto_comp *tfm;
> +   struct crypto_acomp *acomp;
> +   struct acomp_req *acomp_req;
> +   struct zswap_acomp_result *result;
> +
> +   acomp_req = *per_cpu_ptr(pool->acomp_req, cpu);
> +   if (!IS_ERR_OR_NULL(acomp_req))
> +   acomp_request_free(acomp_req);
> +   *per_cpu_ptr(pool->acomp_req, cpu) = NULL;
> +
> +   acomp = *per_cpu_ptr(pool->acomp, cpu);
> +   if (!IS_ERR_OR_NULL(acomp))
> +   crypto_free_acomp(acomp);
> +   *per_cpu_ptr(pool->acomp, cpu) = NULL;
> +
> +   result = *per_cpu_ptr(pool->result, cpu);
> +   if (!IS_ERR_OR_NULL(result))
> +   kfree(result);
> +   *per_cpu_ptr(pool->result, cpu) = NULL;
>
> -   tfm = *per_cpu_ptr(pool->tfm, cpu);
> -   if (!IS_ERR_OR_NULL(tfm))
> -   crypto_free_comp(tfm);
> -   *per_cpu_ptr(pool->tfm, cpu) = NULL;
> return 0;
>  }
>
> @@ -512,8 +562,20 @@ static struct zswap_pool *zswap_pool_create(char *type, 
> char *compressor)
> pr_debug("using %s zpool\n", zpool_get_type(pool->zpool));
>
> strlcpy(pool->tfm_name, 

Re: [PATCH v2 1/1] mm: zswap - Add crypto acomp/scomp framework support

[Dan Streetman]

On Fri, Feb 24, 2017 at 11:05 AM, Mahipal Challa
 wrote:
> This adds support for kernel's new crypto acomp/scomp framework
> to zswap.

I don't understand the point of this, zswap can't compress pages
asynchronously, so what benefit do we get from using the async crypto
api and then immediately waiting for it to finish?  This seems like
it's just adding complexity for no reason?

>
> Signed-off-by: Mahipal Challa 
> Signed-off-by: Vishnu Nair 
> ---
>  mm/zswap.c | 192 
> +++--
>  1 file changed, 162 insertions(+), 30 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index cabf09e..b29d109 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -33,8 +33,10 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -118,9 +120,21 @@ static int zswap_compressor_param_set(const char *,
>  * data structures
>  **/
>
> +/**
> + * struct zswap_acomp_result - Data structure to store result of acomp 
> callback
> + * @completion: zswap will wait for completion on this entry
> + * @err   : return value from acomp algorithm will be stored here
> + */
> +struct zswap_acomp_result {
> +   struct completion completion;
> +   int err;
> +};
> +
>  struct zswap_pool {
> struct zpool *zpool;
> -   struct crypto_comp * __percpu *tfm;
> +   struct crypto_acomp * __percpu *acomp;
> +   struct acomp_req * __percpu *acomp_req;
> +   struct zswap_acomp_result * __percpu *result;
> struct kref kref;
> struct list_head list;
> struct work_struct work;
> @@ -388,30 +402,66 @@ static int zswap_dstmem_dead(unsigned int cpu)
>  static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
>  {
> struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);
> -   struct crypto_comp *tfm;
> +   struct crypto_acomp *acomp;
> +   struct acomp_req *acomp_req;
> +   struct zswap_acomp_result *result;
>
> -   if (WARN_ON(*per_cpu_ptr(pool->tfm, cpu)))
> +   if (WARN_ON(*per_cpu_ptr(pool->acomp, cpu)))
> return 0;
> +   if (WARN_ON(*per_cpu_ptr(pool->acomp_req, cpu)))
> +   return 0;
> +   if (WARN_ON(*per_cpu_ptr(pool->result, cpu)))
> +   return 0;
> +
> +   acomp = crypto_alloc_acomp(pool->tfm_name, 0, 0);
> +   if (IS_ERR_OR_NULL(acomp)) {
> +   pr_err("could not alloc crypto acomp %s : %ld\n",
> +  pool->tfm_name, PTR_ERR(acomp));
> +   return -ENOMEM;
> +   }
> +   *per_cpu_ptr(pool->acomp, cpu) = acomp;
> +
> +   acomp_req = acomp_request_alloc(acomp);
> +   if (IS_ERR_OR_NULL(acomp_req)) {
> +   pr_err("could not alloc crypto acomp %s : %ld\n",
> +  pool->tfm_name, PTR_ERR(acomp));
> +   return -ENOMEM;
> +   }
> +   *per_cpu_ptr(pool->acomp_req, cpu) = acomp_req;
>
> -   tfm = crypto_alloc_comp(pool->tfm_name, 0, 0);
> -   if (IS_ERR_OR_NULL(tfm)) {
> -   pr_err("could not alloc crypto comp %s : %ld\n",
> -  pool->tfm_name, PTR_ERR(tfm));
> +   result = kzalloc(sizeof(*result), GFP_KERNEL);
> +   if (IS_ERR_OR_NULL(result)) {
> +   pr_err("Could not initialize completion on result\n");
> return -ENOMEM;
> }
> -   *per_cpu_ptr(pool->tfm, cpu) = tfm;
> +   init_completion(>completion);
> +   *per_cpu_ptr(pool->result, cpu) = result;
> +
> return 0;
>  }
>
>  static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node)
>  {
> struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);
> -   struct crypto_comp *tfm;
> +   struct crypto_acomp *acomp;
> +   struct acomp_req *acomp_req;
> +   struct zswap_acomp_result *result;
> +
> +   acomp_req = *per_cpu_ptr(pool->acomp_req, cpu);
> +   if (!IS_ERR_OR_NULL(acomp_req))
> +   acomp_request_free(acomp_req);
> +   *per_cpu_ptr(pool->acomp_req, cpu) = NULL;
> +
> +   acomp = *per_cpu_ptr(pool->acomp, cpu);
> +   if (!IS_ERR_OR_NULL(acomp))
> +   crypto_free_acomp(acomp);
> +   *per_cpu_ptr(pool->acomp, cpu) = NULL;
> +
> +   result = *per_cpu_ptr(pool->result, cpu);
> +   if (!IS_ERR_OR_NULL(result))
> +   kfree(result);
> +   *per_cpu_ptr(pool->result, cpu) = NULL;
>
> -   tfm = *per_cpu_ptr(pool->tfm, cpu);
> -   if (!IS_ERR_OR_NULL(tfm))
> -   crypto_free_comp(tfm);
> -   *per_cpu_ptr(pool->tfm, cpu) = NULL;
> return 0;
>  }
>
> @@ -512,8 +562,20 @@ static struct zswap_pool *zswap_pool_create(char *type, 
> char *compressor)
> pr_debug("using %s zpool\n", zpool_get_type(pool->zpool));
>
> strlcpy(pool->tfm_name, compressor, sizeof(pool->tfm_name));
> -   pool->tfm = alloc_percpu(struct