[PATCH v4 0/3] mm/zswap & crypto/compress: remove a couple of memcpy

[PATCH v4 0/3] mm/zswap & crypto/compress: remove a couple of memcpy

[Barry Song]

From: Barry Song <v-songbaohua@oppo.com>

The patchset removes a couple of memcpy in zswap and crypto
to improve zswap's performance.

Thanks for Chengming Zhou's test and perf data.
Quote from Chengming,
 I just tested these three patches on my server, found improvement in the
 kernel build testcase on a tmpfs with zswap (lz4 + zsmalloc) enabled.
 
         mm-stable 501a06fe8e4c  patched
 real    1m38.028s               1m32.317s
 user    19m11.482s              18m39.439s
 sys     19m26.445s              17m5.646s


This patchset applies to mm-unstable as recently zswap has
lots of change.

-v4:
  * introduce a helper for algorithm flags according to Herbert
  * fix cra_flags for intel and hisilicon async drivers

Barry Song (3):
  crypto: introduce crypto_acomp_get_alg_flags to expose algorithm flags
  mm/zswap: remove the memcpy if acomp is not sleepable
  crypto: scompress: remove memcpy if sg_nents is 1

 crypto/scompress.c                         | 36 +++++++++++++++++-----
 drivers/crypto/hisilicon/zip/zip_crypto.c  |  1 +
 drivers/crypto/intel/iaa/iaa_crypto_main.c |  1 +
 include/crypto/acompress.h                 |  5 +++
 include/linux/crypto.h                     |  5 +++
 mm/zswap.c                                 |  7 +++--
 6 files changed, 46 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] crypto: introduce crypto_acomp_get_alg_flags to expose algorithm flags

[Barry Song]

From: Barry Song <v-songbaohua@oppo.com>

acomp's users might want to know algorithm flags to optimize
themselves. One typical user which can benefit from exposed
alg flags is zswap.

In zswap, zsmalloc is the most commonly used allocator for
(and perhaps the only one). For zsmalloc, we cannot sleep
while we map the compressed memory, so we copy it to a
temporary buffer. By knowing the alg won't sleep can help
zswap to avoid the need for a buffer. This shows noticeable
improvement in load/store latency of zswap.

This patch also fixes the missing ASYNC cra_flags in Intel
iaa and Hisilicon zip drivers.

Cc: Yang Shen <shenyang39@huawei.com>
Cc: Zhou Wang <wangzhou1@hisilicon.com>
Cc: Tom Zanussi <tom.zanussi@linux.intel.com>
Signed-off-by: Barry Song <v-songbaohua@oppo.com>
---
 drivers/crypto/hisilicon/zip/zip_crypto.c  | 1 +
 drivers/crypto/intel/iaa/iaa_crypto_main.c | 1 +
 include/crypto/acompress.h                 | 5 +++++
 include/linux/crypto.h                     | 5 +++++
 4 files changed, 12 insertions(+)

diff --git a/drivers/crypto/hisilicon/zip/zip_crypto.c b/drivers/crypto/hisilicon/zip/zip_crypto.c
index c650c741a18d..94e2d66b04b6 100644
--- a/drivers/crypto/hisilicon/zip/zip_crypto.c
+++ b/drivers/crypto/hisilicon/zip/zip_crypto.c
@@ -591,6 +591,7 @@ static struct acomp_alg hisi_zip_acomp_deflate = {
 	.base			= {
 		.cra_name		= "deflate",
 		.cra_driver_name	= "hisi-deflate-acomp",
+		.cra_flags		= CRYPTO_ALG_ASYNC,
 		.cra_module		= THIS_MODULE,
 		.cra_priority		= HZIP_ALG_PRIORITY,
 		.cra_ctxsize		= sizeof(struct hisi_zip_ctx),
diff --git a/drivers/crypto/intel/iaa/iaa_crypto_main.c b/drivers/crypto/intel/iaa/iaa_crypto_main.c
index dfd3baf0a8d8..91adf9d76a2e 100644
--- a/drivers/crypto/intel/iaa/iaa_crypto_main.c
+++ b/drivers/crypto/intel/iaa/iaa_crypto_main.c
@@ -1916,6 +1916,7 @@ static struct acomp_alg iaa_acomp_fixed_deflate = {
 	.base			= {
 		.cra_name		= "deflate",
 		.cra_driver_name	= "deflate-iaa",
+		.cra_flags		= CRYPTO_ALG_ASYNC,
 		.cra_ctxsize		= sizeof(struct iaa_compression_ctx),
 		.cra_module		= THIS_MODULE,
 		.cra_priority		= IAA_ALG_PRIORITY,
diff --git a/include/crypto/acompress.h b/include/crypto/acompress.h
index 574cffc90730..07bd8f6bc79a 100644
--- a/include/crypto/acompress.h
+++ b/include/crypto/acompress.h
@@ -160,6 +160,11 @@ static inline void acomp_request_set_tfm(struct acomp_req *req,
 	req->base.tfm = crypto_acomp_tfm(tfm);
 }
 
+static inline u32 crypto_acomp_get_alg_flags(struct crypto_acomp *tfm)
+{
+	return crypto_tfm_alg_flags(crypto_acomp_tfm(tfm));
+}
+
 static inline struct crypto_acomp *crypto_acomp_reqtfm(struct acomp_req *req)
 {
 	return __crypto_acomp_tfm(req->base.tfm);
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index b164da5e129e..811bfaf8b6f8 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -467,6 +467,11 @@ static inline unsigned int crypto_tfm_alg_blocksize(struct crypto_tfm *tfm)
 	return tfm->__crt_alg->cra_blocksize;
 }
 
+static inline unsigned int crypto_tfm_alg_flags(struct crypto_tfm *tfm)
+{
+	return tfm->__crt_alg->cra_flags;
+}
+
 static inline unsigned int crypto_tfm_alg_alignmask(struct crypto_tfm *tfm)
 {
 	return tfm->__crt_alg->cra_alignmask;
-- 
2.34.1

Re: [PATCH v4 1/3] crypto: introduce crypto_acomp_get_alg_flags to expose algorithm flags

[Herbert Xu]

On Tue, Feb 20, 2024 at 03:55:43PM +1300, Barry Song wrote:
>
> diff --git a/drivers/crypto/hisilicon/zip/zip_crypto.c b/drivers/crypto/hisilicon/zip/zip_crypto.c
> index c650c741a18d..94e2d66b04b6 100644
> --- a/drivers/crypto/hisilicon/zip/zip_crypto.c
> +++ b/drivers/crypto/hisilicon/zip/zip_crypto.c
> @@ -591,6 +591,7 @@ static struct acomp_alg hisi_zip_acomp_deflate = {
>  	.base			= {
>  		.cra_name		= "deflate",
>  		.cra_driver_name	= "hisi-deflate-acomp",
> +		.cra_flags		= CRYPTO_ALG_ASYNC,
>  		.cra_module		= THIS_MODULE,
>  		.cra_priority		= HZIP_ALG_PRIORITY,
>  		.cra_ctxsize		= sizeof(struct hisi_zip_ctx),
> diff --git a/drivers/crypto/intel/iaa/iaa_crypto_main.c b/drivers/crypto/intel/iaa/iaa_crypto_main.c
> index dfd3baf0a8d8..91adf9d76a2e 100644
> --- a/drivers/crypto/intel/iaa/iaa_crypto_main.c
> +++ b/drivers/crypto/intel/iaa/iaa_crypto_main.c
> @@ -1916,6 +1916,7 @@ static struct acomp_alg iaa_acomp_fixed_deflate = {
>  	.base			= {
>  		.cra_name		= "deflate",
>  		.cra_driver_name	= "deflate-iaa",
> +		.cra_flags		= CRYPTO_ALG_ASYNC,
>  		.cra_ctxsize		= sizeof(struct iaa_compression_ctx),
>  		.cra_module		= THIS_MODULE,
>  		.cra_priority		= IAA_ALG_PRIORITY,

Good catch.  I think this should go into a separate bug-fix patch.

> diff --git a/include/crypto/acompress.h b/include/crypto/acompress.h
> index 574cffc90730..07bd8f6bc79a 100644
> --- a/include/crypto/acompress.h
> +++ b/include/crypto/acompress.h
> @@ -160,6 +160,11 @@ static inline void acomp_request_set_tfm(struct acomp_req *req,
>  	req->base.tfm = crypto_acomp_tfm(tfm);
>  }
>  
> +static inline u32 crypto_acomp_get_alg_flags(struct crypto_acomp *tfm)
> +{
> +	return crypto_tfm_alg_flags(crypto_acomp_tfm(tfm));
> +}

Sorry, my mistake.  I shouldn't have suggested copying skcipher
since that gets the tfm flags as opposed to the alg flags which
you've found out.

I think you should just go with your original function acomp_is_async
but do it like this:

static inline bool acomp_is_async(struct crypto_acomp *tfm)
{
	return crypto_comp_alg_common(tfm)->base.cra_flags &
	       CRYPTO_ALG_ASYNC;
}

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

Re: [PATCH v4 1/3] crypto: introduce crypto_acomp_get_alg_flags to expose algorithm flags

[Barry Song]

On Tue, Feb 20, 2024 at 5:14 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Tue, Feb 20, 2024 at 03:55:43PM +1300, Barry Song wrote:
> >
> > diff --git a/drivers/crypto/hisilicon/zip/zip_crypto.c b/drivers/crypto/hisilicon/zip/zip_crypto.c
> > index c650c741a18d..94e2d66b04b6 100644
> > --- a/drivers/crypto/hisilicon/zip/zip_crypto.c
> > +++ b/drivers/crypto/hisilicon/zip/zip_crypto.c
> > @@ -591,6 +591,7 @@ static struct acomp_alg hisi_zip_acomp_deflate = {
> >       .base                   = {
> >               .cra_name               = "deflate",
> >               .cra_driver_name        = "hisi-deflate-acomp",
> > +             .cra_flags              = CRYPTO_ALG_ASYNC,
> >               .cra_module             = THIS_MODULE,
> >               .cra_priority           = HZIP_ALG_PRIORITY,
> >               .cra_ctxsize            = sizeof(struct hisi_zip_ctx),
> > diff --git a/drivers/crypto/intel/iaa/iaa_crypto_main.c b/drivers/crypto/intel/iaa/iaa_crypto_main.c
> > index dfd3baf0a8d8..91adf9d76a2e 100644
> > --- a/drivers/crypto/intel/iaa/iaa_crypto_main.c
> > +++ b/drivers/crypto/intel/iaa/iaa_crypto_main.c
> > @@ -1916,6 +1916,7 @@ static struct acomp_alg iaa_acomp_fixed_deflate = {
> >       .base                   = {
> >               .cra_name               = "deflate",
> >               .cra_driver_name        = "deflate-iaa",
> > +             .cra_flags              = CRYPTO_ALG_ASYNC,
> >               .cra_ctxsize            = sizeof(struct iaa_compression_ctx),
> >               .cra_module             = THIS_MODULE,
> >               .cra_priority           = IAA_ALG_PRIORITY,
>
> Good catch.  I think this should go into a separate bug-fix patch.

done.

>
> > diff --git a/include/crypto/acompress.h b/include/crypto/acompress.h
> > index 574cffc90730..07bd8f6bc79a 100644
> > --- a/include/crypto/acompress.h
> > +++ b/include/crypto/acompress.h
> > @@ -160,6 +160,11 @@ static inline void acomp_request_set_tfm(struct acomp_req *req,
> >       req->base.tfm = crypto_acomp_tfm(tfm);
> >  }
> >
> > +static inline u32 crypto_acomp_get_alg_flags(struct crypto_acomp *tfm)
> > +{
> > +     return crypto_tfm_alg_flags(crypto_acomp_tfm(tfm));
> > +}
>
> Sorry, my mistake.  I shouldn't have suggested copying skcipher
> since that gets the tfm flags as opposed to the alg flags which
> you've found out.

no worries, Herbert :-)

>
> I think you should just go with your original function acomp_is_async
> but do it like this:
>
> static inline bool acomp_is_async(struct crypto_acomp *tfm)
> {
>         return crypto_comp_alg_common(tfm)->base.cra_flags &
>                CRYPTO_ALG_ASYNC;
> }

ok, I will do that. Besides, i'm also curious if we can open a
discussion, for example, letting offload drivers be able to work
in both sync mode and async.  A scenario I can imagine is as
below,

for zswap, and page size is configured as 4KiB. in this
case, offload hardware might be able to compress/decompress
much faster than CPU, but the event-wait, wake-up, schedule
latency might add the total time for a compression and
decompression. thus offload might work worse than CPU
though hardware-accelerator is much faster than CPU.
In this case, it seems good to let offload drivers poll
the completion of compression and decompression
compared with sleep and wait.

On the other hand, when PAGE SIZE is big, for example,
ARM64's PAGE_SIZE could be 64KiB. in the future,
mTHP/large folios project might also ask for larger data
to be swapped as a whole. we will only need a schedule/
wake-up for 64KiB or larger data,  the  compression/
decompression time is much longer, in this case, async
mode might help to decrease CPU usage while providing
lower comp/decomp latency.

So it could be something like:
if data is short, acomp driver works by polling; if data is
long, acomp driver works by sleeping and waiting.

it would be perfect if Zhou or Yang Shen can help collect some
data to back up the discussion.

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

Thanks
Barry

Re: [PATCH v4 1/3] crypto: introduce crypto_acomp_get_alg_flags to expose algorithm flags

[Herbert Xu]

On Tue, Feb 20, 2024 at 06:05:16PM +1300, Barry Song wrote:
> 
> So it could be something like:
> if data is short, acomp driver works by polling; if data is
> long, acomp driver works by sleeping and waiting.

This sort of logic is specific to each piece of hardware and
should go into the driver.

There is no reason why an async driver cannot return synchronously.
The API fully supports this mode of operation.

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

Re: [PATCH v4 1/3] crypto: introduce crypto_acomp_get_alg_flags to expose algorithm flags

[Barry Song]

On Tue, Feb 20, 2024 at 6:09 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Tue, Feb 20, 2024 at 06:05:16PM +1300, Barry Song wrote:
> >
> > So it could be something like:
> > if data is short, acomp driver works by polling; if data is
> > long, acomp driver works by sleeping and waiting.
>
> This sort of logic is specific to each piece of hardware and
> should go into the driver.
>
> There is no reason why an async driver cannot return synchronously.
> The API fully supports this mode of operation.

Nice to know! Thanks for clarification.

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

Thanks
Barry

[PATCH v4 2/3] mm/zswap: remove the memcpy if acomp is not sleepable

[Barry Song]

From: Barry Song <v-songbaohua@oppo.com>

Most compressors are actually CPU-based and won't sleep during
compression and decompression. We should remove the redundant
memcpy for them.
This patch checks the algorithm flags CRYPTO_ALG_ASYNC to know
if acomp is sleepable.
Generally speaking, async and sleepable are semantically similar
but not equal. But for compress drivers, they are actually equal
at least due to the below facts.
Firstly, scompress drivers - crypto/deflate.c, lz4.c, zstd.c,
lzo.c etc have no sleep. Secondly, zRAM has been using these
scompress drivers for years in atomic contexts, and never
worried those drivers going to sleep.

Signed-off-by: Barry Song <v-songbaohua@oppo.com>
Tested-by: Chengming Zhou <zhouchengming@bytedance.com>
Reviewed-by: Nhat Pham <nphamcs@gmail.com>
Acked-by: Yosry Ahmed <yosryahmed@google.com>
Reviewed-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 mm/zswap.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 51de79aa8659..115a780fa2c1 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -162,6 +162,7 @@ struct crypto_acomp_ctx {
 	struct crypto_wait wait;
 	u8 *buffer;
 	struct mutex mutex;
+	bool is_sleepable;
 };
 
 /*
@@ -973,6 +974,8 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
 		goto acomp_fail;
 	}
 	acomp_ctx->acomp = acomp;
+	acomp_ctx->is_sleepable = crypto_acomp_get_alg_flags(acomp) &
+				  CRYPTO_ALG_ASYNC;
 
 	req = acomp_request_alloc(acomp_ctx->acomp);
 	if (!req) {
@@ -1100,7 +1103,7 @@ static void zswap_decompress(struct zswap_entry *entry, struct page *page)
 	mutex_lock(&acomp_ctx->mutex);
 
 	src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
-	if (!zpool_can_sleep_mapped(zpool)) {
+	if (acomp_ctx->is_sleepable && !zpool_can_sleep_mapped(zpool)) {
 		memcpy(acomp_ctx->buffer, src, entry->length);
 		src = acomp_ctx->buffer;
 		zpool_unmap_handle(zpool, entry->handle);
@@ -1114,7 +1117,7 @@ static void zswap_decompress(struct zswap_entry *entry, struct page *page)
 	BUG_ON(acomp_ctx->req->dlen != PAGE_SIZE);
 	mutex_unlock(&acomp_ctx->mutex);
 
-	if (zpool_can_sleep_mapped(zpool))
+	if (!acomp_ctx->is_sleepable || zpool_can_sleep_mapped(zpool))
 		zpool_unmap_handle(zpool, entry->handle);
 }
 
-- 
2.34.1

[PATCH v4 3/3] crypto: scompress: remove memcpy if sg_nents is 1

[Barry Song]

From: Barry Song <v-songbaohua@oppo.com>

while sg_nents is 1 which is always true for the current kernel
as the only user - zswap is the case, we should remove two big
memcpy.

Signed-off-by: Barry Song <v-songbaohua@oppo.com>
Tested-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 crypto/scompress.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/crypto/scompress.c b/crypto/scompress.c
index b108a30a7600..50a487eac792 100644
--- a/crypto/scompress.c
+++ b/crypto/scompress.c
@@ -117,6 +117,7 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir)
 	struct crypto_scomp *scomp = *tfm_ctx;
 	void **ctx = acomp_request_ctx(req);
 	struct scomp_scratch *scratch;
+	void *src, *dst;
 	unsigned int dlen;
 	int ret;
 
@@ -134,13 +135,25 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir)
 	scratch = raw_cpu_ptr(&scomp_scratch);
 	spin_lock(&scratch->lock);
 
-	scatterwalk_map_and_copy(scratch->src, req->src, 0, req->slen, 0);
+	if (sg_nents(req->src) == 1) {
+		src = kmap_local_page(sg_page(req->src)) + req->src->offset;
+	} else {
+		scatterwalk_map_and_copy(scratch->src, req->src, 0,
+					 req->slen, 0);
+		src = scratch->src;
+	}
+
+	if (req->dst && sg_nents(req->dst) == 1)
+		dst = kmap_local_page(sg_page(req->dst)) + req->dst->offset;
+	else
+		dst = scratch->dst;
+
 	if (dir)
-		ret = crypto_scomp_compress(scomp, scratch->src, req->slen,
-					    scratch->dst, &req->dlen, *ctx);
+		ret = crypto_scomp_compress(scomp, src, req->slen,
+					    dst, &req->dlen, *ctx);
 	else
-		ret = crypto_scomp_decompress(scomp, scratch->src, req->slen,
-					      scratch->dst, &req->dlen, *ctx);
+		ret = crypto_scomp_decompress(scomp, src, req->slen,
+					      dst, &req->dlen, *ctx);
 	if (!ret) {
 		if (!req->dst) {
 			req->dst = sgl_alloc(req->dlen, GFP_ATOMIC, NULL);
@@ -152,10 +165,19 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir)
 			ret = -ENOSPC;
 			goto out;
 		}
-		scatterwalk_map_and_copy(scratch->dst, req->dst, 0, req->dlen,
-					 1);
+		if (dst == scratch->dst) {
+			scatterwalk_map_and_copy(scratch->dst, req->dst, 0,
+						 req->dlen, 1);
+		} else {
+			flush_dcache_page(sg_page(req->dst));
+		}
 	}
 out:
+	if (src != scratch->src)
+		kunmap_local(src);
+	if (dst != scratch->dst)
+		kunmap_local(dst);
+
 	spin_unlock(&scratch->lock);
 	return ret;
 }
-- 
2.34.1