[PATCH v6 0/2] zswap: remove the memcpy if acomp is not sleepable

[PATCH v6 0/2] zswap: remove the memcpy if acomp is not sleepable

[Barry Song]

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

In zswap, if we use 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
memcpy.
Thus we introduce an API in crypto to expose if acomp is async,
and zswap can use it to decide if it can remove copying to the
tmp buffer.

-v6:
 * add acked-by of Herbert, Thanks!
 * remove patch 3/3 from the series, as that one will go
   through crypto

Barry Song (2):
  crypto: introduce: acomp_is_async to expose if comp drivers might
    sleep
  mm/zswap: remove the memcpy if acomp is not sleepable

 include/crypto/acompress.h | 6 ++++++
 mm/zswap.c                 | 6 ++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v6 1/2] crypto: introduce: acomp_is_async to expose if comp drivers might sleep

[Barry Song]

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

acomp's users might want to know if acomp is really async to
optimize themselves. One typical user which can benefit from
exposed async stat 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.

Signed-off-by: Barry Song <v-songbaohua@oppo.com>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 include/crypto/acompress.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/crypto/acompress.h b/include/crypto/acompress.h
index 574cffc90730..80e243611fe2 100644
--- a/include/crypto/acompress.h
+++ b/include/crypto/acompress.h
@@ -160,6 +160,12 @@ static inline void acomp_request_set_tfm(struct acomp_req *req,
 	req->base.tfm = crypto_acomp_tfm(tfm);
 }
 
+static inline bool acomp_is_async(struct crypto_acomp *tfm)
+{
+	return crypto_comp_alg_common(tfm)->base.cra_flags &
+	       CRYPTO_ALG_ASYNC;
+}
+
 static inline struct crypto_acomp *crypto_acomp_reqtfm(struct acomp_req *req)
 {
 	return __crypto_acomp_tfm(req->base.tfm);
-- 
2.34.1

Re: [PATCH v6 1/2] crypto: introduce: acomp_is_async to expose if comp drivers might sleep

[Chris Li]

On Thu, Feb 22, 2024 at 12:12 AM Barry Song <21cnbao@gmail.com> wrote:
>
> From: Barry Song <v-songbaohua@oppo.com>
>
> acomp's users might want to know if acomp is really async to
> optimize themselves. One typical user which can benefit from
> exposed async stat 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.
>
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: Chris Li <chrisl@kernel.org>

Chris

> ---
>  include/crypto/acompress.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/include/crypto/acompress.h b/include/crypto/acompress.h
> index 574cffc90730..80e243611fe2 100644
> --- a/include/crypto/acompress.h
> +++ b/include/crypto/acompress.h
> @@ -160,6 +160,12 @@ static inline void acomp_request_set_tfm(struct acomp_req *req,
>         req->base.tfm = crypto_acomp_tfm(tfm);
>  }
>
> +static inline bool acomp_is_async(struct crypto_acomp *tfm)
> +{
> +       return crypto_comp_alg_common(tfm)->base.cra_flags &
> +              CRYPTO_ALG_ASYNC;
> +}
> +
>  static inline struct crypto_acomp *crypto_acomp_reqtfm(struct acomp_req *req)
>  {
>         return __crypto_acomp_tfm(req->base.tfm);
> --
> 2.34.1
>

[PATCH v6 2/2] 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 if the algorithm is sleepable by testing the
CRYPTO_ALG_ASYNC algorithm flag.
Generally speaking, async and sleepable are semantically similar
but not equal. But for compress drivers, they are basically 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.
One exception is that an async driver can sometimes still return
synchronously per Herbert's clarification. In this case, we are
still having a redundant memcpy. But we can't know if one
particular acomp request will sleep or not unless crypto can
expose more details for each specific request from offload
drivers.

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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 011e068eb355..de3c9e30bed7 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;
 };
 
 /*
@@ -950,6 +951,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
 		goto acomp_fail;
 	}
 	acomp_ctx->acomp = acomp;
+	acomp_ctx->is_sleepable = acomp_is_async(acomp);
 
 	req = acomp_request_alloc(acomp_ctx->acomp);
 	if (!req) {
@@ -1077,7 +1079,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);
@@ -1091,7 +1093,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

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

[Chris Li]

Acked-by: Chris Li <chrisl@kernel.org>

Chris

On Thu, Feb 22, 2024 at 12:12 AM Barry Song <21cnbao@gmail.com> wrote:
>
> 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 if the algorithm is sleepable by testing the
> CRYPTO_ALG_ASYNC algorithm flag.
> Generally speaking, async and sleepable are semantically similar
> but not equal. But for compress drivers, they are basically 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.
> One exception is that an async driver can sometimes still return
> synchronously per Herbert's clarification. In this case, we are
> still having a redundant memcpy. But we can't know if one
> particular acomp request will sleep or not unless crypto can
> expose more details for each specific request from offload
> drivers.
>
> 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 | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 011e068eb355..de3c9e30bed7 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;
>  };
>
>  /*
> @@ -950,6 +951,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
>                 goto acomp_fail;
>         }
>         acomp_ctx->acomp = acomp;
> +       acomp_ctx->is_sleepable = acomp_is_async(acomp);
>
>         req = acomp_request_alloc(acomp_ctx->acomp);
>         if (!req) {
> @@ -1077,7 +1079,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);
> @@ -1091,7 +1093,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
>

Re: [PATCH v6 0/2] zswap: remove the memcpy if acomp is not sleepable

[Barry Song]

Hi Andrew,

On Thu, Feb 22, 2024 at 4:11 PM Barry Song <21cnbao@gmail.com> wrote:
>
> From: Barry Song <v-songbaohua@oppo.com>
>
> In zswap, if we use 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
> memcpy.
> Thus we introduce an API in crypto to expose if acomp is async,
> and zswap can use it to decide if it can remove copying to the
> tmp buffer.
>
> -v6:
>  * add acked-by of Herbert, Thanks!
>  * remove patch 3/3 from the series, as that one will go
>    through crypto

Can you please pull this into mm-tree? This used to have 3 patches.

3/3 was separated according to Herbert's requirements and has
been in a crypto tree.
crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=77292bb8ca

Two drivers fixes(patch 1 needs) have also been in crypto tree:
crypto: hisilicon/zip - fix the missing CRYPTO_ALG_ASYNC in cra_flags
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=db8ac88385

crypto: iaa - fix the missing CRYPTO_ALG_ASYNC in cra_flags
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=30dd94dba35

So it should be quite safe to pull this series into mm-tree now.

>
> Barry Song (2):
>   crypto: introduce: acomp_is_async to expose if comp drivers might
>     sleep
>   mm/zswap: remove the memcpy if acomp is not sleepable
>
>  include/crypto/acompress.h | 6 ++++++
>  mm/zswap.c                 | 6 ++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> --
> 2.34.1

Thanks
Barry

Re: [PATCH v6 0/2] zswap: remove the memcpy if acomp is not sleepable

[Andrew Morton]

On Fri, 8 Mar 2024 19:57:38 +0800 Barry Song <21cnbao@gmail.com> wrote:

> Hi Andrew,
> 
> On Thu, Feb 22, 2024 at 4:11 PM Barry Song <21cnbao@gmail.com> wrote:
> >
> > From: Barry Song <v-songbaohua@oppo.com>
> >
> > In zswap, if we use 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
> > memcpy.
> > Thus we introduce an API in crypto to expose if acomp is async,
> > and zswap can use it to decide if it can remove copying to the
> > tmp buffer.
> >
> > -v6:
> >  * add acked-by of Herbert, Thanks!
> >  * remove patch 3/3 from the series, as that one will go
> >    through crypto
> 
> Can you please pull this into mm-tree? This used to have 3 patches.
> 
> 3/3 was separated according to Herbert's requirements and has
> been in a crypto tree.
> crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem
> https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=77292bb8ca
> 
> Two drivers fixes(patch 1 needs) have also been in crypto tree:
> crypto: hisilicon/zip - fix the missing CRYPTO_ALG_ASYNC in cra_flags
> https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=db8ac88385
> 
> crypto: iaa - fix the missing CRYPTO_ALG_ASYNC in cra_flags
> https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=30dd94dba35
> 
> So it should be quite safe to pull this series into mm-tree now.

But this zswap chage requires the presence of the other patches, yes?

So the mm.git tree alone will be buggy?  And if mm.git merges ahead of
the other trees, there will be a window where mainline will be buggy?

If so, I think it wuold be better to merge the zswap patch in the next
merge window.

Re: [PATCH v6 0/2] zswap: remove the memcpy if acomp is not sleepable

[Barry Song]

On Sat, Mar 9, 2024 at 11:23 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 8 Mar 2024 19:57:38 +0800 Barry Song <21cnbao@gmail.com> wrote:
>
> > Hi Andrew,
> >
> > On Thu, Feb 22, 2024 at 4:11 PM Barry Song <21cnbao@gmail.com> wrote:
> > >
> > > From: Barry Song <v-songbaohua@oppo.com>
> > >
> > > In zswap, if we use 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
> > > memcpy.
> > > Thus we introduce an API in crypto to expose if acomp is async,
> > > and zswap can use it to decide if it can remove copying to the
> > > tmp buffer.
> > >
> > > -v6:
> > >  * add acked-by of Herbert, Thanks!
> > >  * remove patch 3/3 from the series, as that one will go
> > >    through crypto
> >
> > Can you please pull this into mm-tree? This used to have 3 patches.
> >
> > 3/3 was separated according to Herbert's requirements and has
> > been in a crypto tree.
> > crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem
> > https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=77292bb8ca
> >
> > Two drivers fixes(patch 1 needs) have also been in crypto tree:
> > crypto: hisilicon/zip - fix the missing CRYPTO_ALG_ASYNC in cra_flags
> > https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=db8ac88385
> >
> > crypto: iaa - fix the missing CRYPTO_ALG_ASYNC in cra_flags
> > https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=30dd94dba35
> >
> > So it should be quite safe to pull this series into mm-tree now.
>
> But this zswap chage requires the presence of the other patches, yes?

As far as I understand, we rely on two driver fixes because those drivers didn't
set the correct cra_flags needed by our patch1. Without those fixes implemented,
two platforms might encounter issues: Intel with IAA (Intel Analytics
Accelerator)
and Hisilicon with ZIP. Other platforms should be unaffected.

The two driver fixes have been merged into the crypto tree.

>
> So the mm.git tree alone will be buggy?  And if mm.git merges ahead of
> the other trees, there will be a window where mainline will be buggy?

Before 6.9-rc1, there might be issues if mm enters Linus' tree before Herbert's
crypto tree. However, by 6.9-rc1, everything should be fine.

>
> If so, I think it wuold be better to merge the zswap patch in the next
> merge window.
>

Okay, I understand. Since this patch improves zswap's performance, I wanted
it to be integrated sooner to contribute. However, I'm perfectly willing to
respect your concerns and adhere to the community's best practices.

So please handle it in the best way you think :-)

Thanks
Barry

Re: [PATCH v6 0/2] zswap: remove the memcpy if acomp is not sleepable

[Andrew Morton]

On Sat, 9 Mar 2024 11:58:39 +0800 Barry Song <21cnbao@gmail.com> wrote:

> > >
> > > So it should be quite safe to pull this series into mm-tree now.
> >
> > But this zswap chage requires the presence of the other patches, yes?
> 
> As far as I understand, we rely on two driver fixes because those drivers didn't
> set the correct cra_flags needed by our patch1. Without those fixes implemented,
> two platforms might encounter issues: Intel with IAA (Intel Analytics
> Accelerator)
> and Hisilicon with ZIP. Other platforms should be unaffected.
> 
> The two driver fixes have been merged into the crypto tree.
> 
> >
> > So the mm.git tree alone will be buggy?  And if mm.git merges ahead of
> > the other trees, there will be a window where mainline will be buggy?
> 
> Before 6.9-rc1, there might be issues if mm enters Linus' tree before Herbert's
> crypto tree. However, by 6.9-rc1, everything should be fine.
> 
> >
> > If so, I think it wuold be better to merge the zswap patch in the next
> > merge window.
> >
> 
> Okay, I understand. Since this patch improves zswap's performance, I wanted
> it to be integrated sooner to contribute. However, I'm perfectly willing to
> respect your concerns and adhere to the community's best practices.
> 

OK.  I very much doubt if anyone is running those drivers on mm.git, so
adding it now isn't likely to hurt.

So I'll merge it now and shall aim to get it upstream very late in the
next merge window.

Re: [PATCH v6 0/2] zswap: remove the memcpy if acomp is not sleepable

[Andrew Morton]

On Fri, 8 Mar 2024 20:36:41 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:

> > Okay, I understand. Since this patch improves zswap's performance, I wanted
> > it to be integrated sooner to contribute. However, I'm perfectly willing to
> > respect your concerns and adhere to the community's best practices.
> > 
> 
> OK.  I very much doubt if anyone is running those drivers on mm.git, so
> adding it now isn't likely to hurt.
> 
> So I'll merge it now and shall aim to get it upstream very late in the
> next merge window.

Nope.  mm.git won't build without acomp_is_async().

We can merge the zswap patch via the crypto tree.  Acked-by: me.

Or please just resend the zswap change after 6.9-rc1 is released.

Re: [PATCH v6 0/2] zswap: remove the memcpy if acomp is not sleepable

[Barry Song]

On Sat, Mar 9, 2024 at 12:42 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 8 Mar 2024 20:36:41 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:
>
> > > Okay, I understand. Since this patch improves zswap's performance, I wanted
> > > it to be integrated sooner to contribute. However, I'm perfectly willing to
> > > respect your concerns and adhere to the community's best practices.
> > >
> >
> > OK.  I very much doubt if anyone is running those drivers on mm.git, so
> > adding it now isn't likely to hurt.
> >
> > So I'll merge it now and shall aim to get it upstream very late in the
> > next merge window.
>
> Nope.  mm.git won't build without acomp_is_async().
>
> We can merge the zswap patch via the crypto tree.  Acked-by: me.

Herbert Acked the acomp_is_async() patch in v5 instead of picking it up
into crypto:
https://lore.kernel.org/linux-mm/ZdWKz43tTz2XY4ca@gondor.apana.org.au/

>
> Or please just resend the zswap change after 6.9-rc1 is released.
>

Thanks
Barry



Thanks
Barry

Re: [PATCH v6 0/2] zswap: remove the memcpy if acomp is not sleepable

[Barry Song]

On Sat, Mar 9, 2024 at 12:56 PM Barry Song <21cnbao@gmail.com> wrote:
>
> On Sat, Mar 9, 2024 at 12:42 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Fri, 8 Mar 2024 20:36:41 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > > > Okay, I understand. Since this patch improves zswap's performance, I wanted
> > > > it to be integrated sooner to contribute. However, I'm perfectly willing to
> > > > respect your concerns and adhere to the community's best practices.
> > > >
> > >
> > > OK.  I very much doubt if anyone is running those drivers on mm.git, so
> > > adding it now isn't likely to hurt.
> > >
> > > So I'll merge it now and shall aim to get it upstream very late in the
> > > next merge window.
> >
> > Nope.  mm.git won't build without acomp_is_async().
> >
> > We can merge the zswap patch via the crypto tree.  Acked-by: me.
>
> Herbert Acked the acomp_is_async() patch in v5 instead of picking it up
> into crypto:
> https://lore.kernel.org/linux-mm/ZdWKz43tTz2XY4ca@gondor.apana.org.au/

More details: Herbert acked the acomp_is_async in v5 [1], while he requested
that patch 3/3 of v5 be split from the series and applied by crypto
[2]. Patch 3/3
of v5 can function independently of 1/3 and 2/3, and it has already
been included
in the crypto tree.

That is why v6 has only two left.

[1] https://lore.kernel.org/linux-mm/ZdWKz43tTz2XY4ca@gondor.apana.org.au/
[2] https://lore.kernel.org/linux-mm/ZdWLim6zYSl%2Fx1Bk@gondor.apana.org.au/


>
> >
> > Or please just resend the zswap change after 6.9-rc1 is released.

Thanks
Barry

Re: [PATCH v6 0/2] zswap: remove the memcpy if acomp is not sleepable

[Johannes Weiner]

On Thu, Feb 22, 2024 at 09:11:33PM +1300, Barry Song wrote:
> From: Barry Song <v-songbaohua@oppo.com>
> 
> In zswap, if we use 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
> memcpy.
> Thus we introduce an API in crypto to expose if acomp is async,
> and zswap can use it to decide if it can remove copying to the
> tmp buffer.
> 
> -v6:
>  * add acked-by of Herbert, Thanks!
>  * remove patch 3/3 from the series, as that one will go
>    through crypto
> 
> Barry Song (2):
>   crypto: introduce: acomp_is_async to expose if comp drivers might
>     sleep
>   mm/zswap: remove the memcpy if acomp is not sleepable
> 
>  include/crypto/acompress.h | 6 ++++++
>  mm/zswap.c                 | 6 ++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Looks good to me.

One small question: why cache is_sleepable in zswap instead of
checking acomp_is_async() directly? It doesn't look expensive.

Re: [PATCH v6 0/2] zswap: remove the memcpy if acomp is not sleepable

[Barry Song]

On Fri, Mar 8, 2024 at 8:11 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Thu, Feb 22, 2024 at 09:11:33PM +1300, Barry Song wrote:
> > From: Barry Song <v-songbaohua@oppo.com>
> >
> > In zswap, if we use 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
> > memcpy.
> > Thus we introduce an API in crypto to expose if acomp is async,
> > and zswap can use it to decide if it can remove copying to the
> > tmp buffer.
> >
> > -v6:
> >  * add acked-by of Herbert, Thanks!
> >  * remove patch 3/3 from the series, as that one will go
> >    through crypto
> >
> > Barry Song (2):
> >   crypto: introduce: acomp_is_async to expose if comp drivers might
> >     sleep
> >   mm/zswap: remove the memcpy if acomp is not sleepable
> >
> >  include/crypto/acompress.h | 6 ++++++
> >  mm/zswap.c                 | 6 ++++--
> >  2 files changed, 10 insertions(+), 2 deletions(-)
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>
> Looks good to me.
>
> One small question: why cache is_sleepable in zswap instead of
> checking acomp_is_async() directly? It doesn't look expensive.

Thank you, Johannes. Your question sparked an idea for me. Drivers, such as
drivers/crypto/intel/iaa/iaa_crypto_main.c, have the capability to dynamically
switch between synchronous and asynchronous modes. Consequently, they
also have the opportunity to dynamically modify cra_flags. This means that
we may not require the cache. right now, iaa always has an ASYNC flag and
needs a memcpy. If we can dynamically change the flag, the iaa platform might
be able to further remove a memcpy in zswap if sync mode is set for it.

/*
 * The iaa crypto driver supports three 'sync' methods determining how
 * compressions and decompressions are performed:
 *
 * - sync:      the compression or decompression completes before
 *              returning.  This is the mode used by the async crypto
 *              interface when the sync mode is set to 'sync' and by
 *              the sync crypto interface regardless of setting.
 *
 * - async:     the compression or decompression is submitted and returns
 *              immediately.  Completion interrupts are not used so
 *              the caller is responsible for polling the descriptor
 *              for completion.  This mode is applicable to only the
 *              async crypto interface and is ignored for anything
 *              else.
 *
 * - async_irq: the compression or decompression is submitted and
 *              returns immediately.  Completion interrupts are
 *              enabled so the caller can wait for the completion and
 *              yield to other threads.  When the compression or
 *              decompression completes, the completion is signaled
 *              and the caller awakened.  This mode is applicable to
 *              only the async crypto interface and is ignored for
 *              anything else.
 *
 * These modes can be set using the iaa_crypto sync_mode driver
 * attribute.
 */

But it seems we will get lots of synchronized issues particularly when we are
changing the mode by sysfs.

static ssize_t sync_mode_store(struct device_driver *driver,
      const char *buf, size_t count)
{
           ...
}

I don't have any iaa hardware. But I'd like to ask if this can interest Tom, the
maintainer of INTEL IAA CRYPTO DRIVER to have a go :-)

+Tom Zanussi

Thanks
Barry