On Mon, Feb 27, 2017 at 9:40 AM, Mahipal Reddy
<[email protected]> wrote:
> Hi Dan,
> Thanks for your reply.
>
> On Sat, Feb 25, 2017 at 3:51 AM, Dan Streetman <[email protected]> wrote:
>> On Fri, Feb 24, 2017 at 11:05 AM, Mahipal Challa
>> <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Vishnu Nair <[email protected]>
>>> ---
>>> 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 <linux/rbtree.h>
>>> #include <linux/swap.h>
>>> #include <linux/crypto.h>
>>> +#include <linux/scatterlist.h>
>>> #include <linux/mempool.h>
>>> #include <linux/zpool.h>
>>> +#include <crypto/acompress.h>
>>>
>>> #include <linux/mm_types.h>
>>> #include <linux/page-flags.h>
>>> @@ -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(&result->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 crypto_comp *);
>>> - if (!pool->tfm) {
>>> + pool->acomp = alloc_percpu(struct crypto_acomp *);
>>> + if (!pool->acomp) {
>>> + pr_err("percpu alloc failed\n");
>>> + goto error;
>>> + }
>>> +
>>> + pool->acomp_req = alloc_percpu(struct acomp_req *);
>>> + if (!pool->acomp_req) {
>>> + pr_err("percpu alloc failed\n");
>>> + goto error;
>>> + }
>>> +
>>> + pool->result = alloc_percpu(struct zswap_acomp_result *);
>>> + if (!pool->result) {
>>> pr_err("percpu alloc failed\n");
>>> goto error;
>>> }
>>> @@ -535,7 +597,9 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
>>> return pool;
>>>
>>> error:
>>> - free_percpu(pool->tfm);
>>> + free_percpu(pool->result);
>>> + free_percpu(pool->acomp_req);
>>> + free_percpu(pool->acomp);
>>> if (pool->zpool)
>>> zpool_destroy_pool(pool->zpool);
>>> kfree(pool);
>>> @@ -575,7 +639,9 @@ static void zswap_pool_destroy(struct zswap_pool *pool)
>>> zswap_pool_debug("destroying", pool);
>>>
>>> cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &pool->node);
>>> - free_percpu(pool->tfm);
>>> + free_percpu(pool->result);
>>> + free_percpu(pool->acomp_req);
>>> + free_percpu(pool->acomp);
>>> zpool_destroy_pool(pool->zpool);
>>> kfree(pool);
>>> }
>>> @@ -622,6 +688,30 @@ static void zswap_pool_put(struct zswap_pool *pool)
>>> }
>>>
>>> /*********************************
>>> +* CRYPTO_ACOMPRESS wait and callbacks
>>> +**********************************/
>>> +static void zswap_acomp_callback(struct crypto_async_request *req, int err)
>>> +{
>>> + struct zswap_acomp_result *res = req->data;
>>> +
>>> + if (err == -EINPROGRESS)
>>> + return;
>>> +
>>> + res->err = err;
>>> + complete(&res->completion);
>>> +}
>>> +
>>> +static int zswap_wait_acomp(struct zswap_acomp_result *res, int ret)
>>> +{
>>> + if (ret == -EINPROGRESS || ret == -EBUSY) {
>>> + wait_for_completion(&res->completion);
>>> + reinit_completion(&res->completion);
>>> + ret = res->err;
>>> + }
>>> + return ret;
>>> +}
>>> +
>>> +/*********************************
>>> * param callbacks
>>> **********************************/
>>>
>>> @@ -788,7 +878,9 @@ static int zswap_writeback_entry(struct zpool *pool, unsigned long handle)
>>> pgoff_t offset;
>>> struct zswap_entry *entry;
>>> struct page *page;
>>> - struct crypto_comp *tfm;
>>> + struct scatterlist input, output;
>>> + struct acomp_req *req;
>>> + struct zswap_acomp_result *result;
>>> u8 *src, *dst;
>>> unsigned int dlen;
>>> int ret;
>>> @@ -828,14 +920,25 @@ static int zswap_writeback_entry(struct zpool *pool, unsigned long handle)
>>>
>>> case ZSWAP_SWAPCACHE_NEW: /* page is locked */
>>> /* decompress */
>>> + req = *get_cpu_ptr(entry->pool->acomp_req);
>>> dlen = PAGE_SIZE;
>>> src = (u8 *)zpool_map_handle(entry->pool->zpool, entry->handle,
>>> ZPOOL_MM_RO) + sizeof(struct zswap_header);
>>> dst = kmap_atomic(page);
>>> - tfm = *get_cpu_ptr(entry->pool->tfm);
>>> - ret = crypto_comp_decompress(tfm, src, entry->length,
>>> - dst, &dlen);
>>> - put_cpu_ptr(entry->pool->tfm);
>>> +
>>> + result = *get_cpu_ptr(entry->pool->result);
>>> + sg_init_one(&input, src, entry->length);
>>> + sg_init_one(&output, dst, dlen);
>>> + acomp_request_set_params(req, &input, &output, entry->length,
>>> + dlen);
>>> + acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
>>> + zswap_acomp_callback, result);
>>> +
>>> + ret = zswap_wait_acomp(result, crypto_acomp_decompress(req));
>>> +
>>> + dlen = req->dlen;
>>> + put_cpu_ptr(entry->pool->acomp_req);
>>> + put_cpu_ptr(entry->pool->result);
>>> kunmap_atomic(dst);
>>> zpool_unmap_handle(entry->pool->zpool, entry->handle);
>>> BUG_ON(ret);
>>> @@ -911,7 +1014,9 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
>>> {
>>> struct zswap_tree *tree = zswap_trees[type];
>>> struct zswap_entry *entry, *dupentry;
>>> - struct crypto_comp *tfm;
>>> + struct scatterlist input, output;
>>> + struct acomp_req *req;
>>> + struct zswap_acomp_result *result;
>>> int ret;
>>> unsigned int dlen = PAGE_SIZE, len;
>>> unsigned long handle;
>>> @@ -950,12 +1055,24 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
>>> }
>>>
>>> /* compress */
>>> + req = *get_cpu_ptr(entry->pool->acomp_req);
>>> + result = *get_cpu_ptr(entry->pool->result);
>>> +
>>> dst = get_cpu_var(zswap_dstmem);
>>> - tfm = *get_cpu_ptr(entry->pool->tfm);
>>> src = kmap_atomic(page);
>>> - ret = crypto_comp_compress(tfm, src, PAGE_SIZE, dst, &dlen);
>>> +
>>> + sg_init_one(&input, src, PAGE_SIZE);
>>> + /* zswap_dstmem is of size (PAGE_SIZE * 2). Reflect same in sg_list */
>>> + sg_init_one(&output, dst, PAGE_SIZE * 2);
>>> + acomp_request_set_params(req, &input, &output, PAGE_SIZE, dlen);
>>> + acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
>>> + zswap_acomp_callback, result);
>>> +
>>> + ret = zswap_wait_acomp(result, crypto_acomp_compress(req));
>>> kunmap_atomic(src);
>>> - put_cpu_ptr(entry->pool->tfm);
>>> + put_cpu_ptr(entry->pool->acomp_req);
>>> + put_cpu_ptr(entry->pool->result);
>>> + dlen = req->dlen;
>>> if (ret) {
>>> ret = -EINVAL;
>>> goto put_dstmem;
>>> @@ -1023,7 +1140,9 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
>>> {
>>> struct zswap_tree *tree = zswap_trees[type];
>>> struct zswap_entry *entry;
>>> - struct crypto_comp *tfm;
>>> + struct scatterlist input, output;
>>> + struct acomp_req *req;
>>> + struct zswap_acomp_result *result;
>>> u8 *src, *dst;
>>> unsigned int dlen;
>>> int ret;
>>> @@ -1039,13 +1158,25 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
>>> spin_unlock(&tree->lock);
>>>
>>> /* decompress */
>>> + req = *get_cpu_ptr(entry->pool->acomp_req);
>>> + result = *get_cpu_ptr(entry->pool->result);
>>> +
>>> dlen = PAGE_SIZE;
>>> src = (u8 *)zpool_map_handle(entry->pool->zpool, entry->handle,
>>> ZPOOL_MM_RO) + sizeof(struct zswap_header);
>>> dst = kmap_atomic(page);
>>> - tfm = *get_cpu_ptr(entry->pool->tfm);
>>> - ret = crypto_comp_decompress(tfm, src, entry->length, dst, &dlen);
>>> - put_cpu_ptr(entry->pool->tfm);
>>> +
>>> + sg_init_one(&input, src, entry->length);
>>> + sg_init_one(&output, dst, dlen);
>>> + acomp_request_set_params(req, &input, &output, entry->length, dlen);
>>> + acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
>>> + zswap_acomp_callback, result);
>>> +
>>> + ret = zswap_wait_acomp(result, crypto_acomp_decompress(req));
>>> +
>>> + dlen = req->dlen;
>>> + put_cpu_ptr(entry->pool->acomp_req);
>>> + put_cpu_ptr(entry->pool->result);
>>> kunmap_atomic(dst);
>>> zpool_unmap_handle(entry->pool->zpool, entry->handle);
>>> BUG_ON(ret);
>>> @@ -1237,3 +1368,4 @@ static int __init init_zswap(void)
>>> MODULE_LICENSE("GPL");
>>> MODULE_AUTHOR("Seth Jennings <[email protected]>");
>>> MODULE_DESCRIPTION("Compressed cache for swap pages");
>>> +
>>> --
>>> 1.8.3.1
>>>
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to [email protected]. For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, Mar 9, 2017 at 4:39 AM, Herbert Xu <[email protected]> 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 <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, Mar 16, 2017 at 12:33 PM, Herbert Xu
<[email protected]> 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!