Re: [PATCH 1/2] mm: zswap: use separate compression and decompression requests

[Yosry Ahmed]

On Mon, Oct 5, 2026 at 5:23 PM Usama Arif <usama.arif@xxxxxxxxx> wrote:
>
> Stores and loads serialize on the same per-CPU acomp request and mutex.
> A low-priority store can be preempted as soon as the compressor drops
> its stream lock, while it still holds the mutex. A higher-priority load
> on that CPU then waits until the store runs again, which can take a
> long time when other tasks are runnable.
>
> Give compression and decompression their own request, completion wait
> and mutex. Since commit e2c3b6b21c77f ("mm: zswap: use SG list
> decompression APIs from zsmalloc"), the per-CPU buffer is only used for
> compression. The two requests can share the per-CPU transform: no
> in-tree implementation modifies transform state while (de)compressing,
> and shared codec state has its own locking.

I am a bit uncomfortable with this. If future changes modify the
transform state while (de)compressing, it may result in nasty bugs.

As for the buffer, I would also prefer some protection, but I feel
less strongly about this. For example, we can put it inside
zswap_acomp_req and not initialize it for the decompression request.
Alternatively, we can have an intermediary struct that contains
zswap_acomp_req + buffer, and use that for the compression request.

> Loads can still wait for
> each other on the decompression mutex, and stores still serialize on
> the compression mutex.
>
> This follows the proposal from Sergey Senozhatsky for the same split
> for zram [1].
>
> [1] https://lore.kernel.org/all/20261005122036.718976-10-senozhatsky@xxxxxxxxxxxx/
>
> Signed-off-by: Usama Arif <usama.arif@xxxxxxxxx>
> ---
>  mm/zswap.c | 89 +++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 52 insertions(+), 37 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index ae19e301fced7..54187b1ef751d 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -137,14 +137,20 @@ bool zswap_never_enabled(void)
>  * data structures
>  **********************************/
>
> -struct crypto_acomp_ctx {
> -       struct crypto_acomp *acomp;
> +struct zswap_acomp_req {
>         struct acomp_req *req;
>         struct crypto_wait wait;
> -       u8 *buffer;
>         struct mutex mutex;
>  };
>
> +/* Separate requests, so that decompression does not wait for compression. */
> +struct crypto_acomp_ctx {
> +       struct crypto_acomp *acomp;
> +       struct zswap_acomp_req comp;
> +       struct zswap_acomp_req decomp;
> +       u8 *buffer;
> +};
> +
>  /*
>   * The lock ordering is zswap_tree.lock -> zswap_pool.lru_lock.
>   * The only case where lru_lock is not acquired while holding tree.lock is
> @@ -270,14 +276,10 @@ static void acomp_ctx_free(struct crypto_acomp_ctx *acomp_ctx)
>         if (!acomp_ctx)
>                 return;
>
> -       /*
> -        * If there was an error in allocating @acomp_ctx->req, it
> -        * would be set to NULL.
> -        */
> -       if (acomp_ctx->req)
> -               acomp_request_free(acomp_ctx->req);
> -
> -       acomp_ctx->req = NULL;
> +       acomp_request_free(acomp_ctx->comp.req);
> +       acomp_ctx->comp.req = NULL;
> +       acomp_request_free(acomp_ctx->decomp.req);
> +       acomp_ctx->decomp.req = NULL;
>
>         /*
>          * We have to handle both cases here: an error pointer return from
> @@ -796,6 +798,28 @@ static void zswap_entry_free(struct zswap_entry *entry)
>  /*********************************
>  * compressed storage functions
>  **********************************/
> +static int zswap_acomp_req_init(struct zswap_acomp_req *areq,
> +                               struct crypto_acomp *acomp)
> +{
> +       /* acomp_request_alloc() returns NULL in case of an error. */
> +       areq->req = acomp_request_alloc(acomp);
> +       if (!areq->req)
> +               return -ENOMEM;
> +
> +       crypto_init_wait(&areq->wait);
> +
> +       /*
> +        * if the backend of acomp is async zip, crypto_req_done() will wakeup
> +        * crypto_wait_req(); if the backend of acomp is scomp, the callback
> +        * won't be called, crypto_wait_req() will return without blocking.
> +        */
> +       acomp_request_set_callback(areq->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> +                                  crypto_req_done, &areq->wait);
> +
> +       mutex_init(&areq->mutex);
> +       return 0;
> +}
> +
>  static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
>  {
>         struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);
> @@ -827,25 +851,13 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
>                 goto fail;
>         }
>
> -       /* acomp_request_alloc() returns NULL in case of an error. */
> -       acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> -       if (!acomp_ctx->req) {
> +       if (zswap_acomp_req_init(&acomp_ctx->comp, acomp_ctx->acomp) ||
> +           zswap_acomp_req_init(&acomp_ctx->decomp, acomp_ctx->acomp)) {
>                 pr_err("could not alloc crypto acomp_request %s\n",
>                        pool->tfm_name);
>                 goto fail;
>         }
>
> -       crypto_init_wait(&acomp_ctx->wait);
> -
> -       /*
> -        * if the backend of acomp is async zip, crypto_req_done() will wakeup
> -        * crypto_wait_req(); if the backend of acomp is scomp, the callback
> -        * won't be called, crypto_wait_req() will return without blocking.
> -        */
> -       acomp_request_set_callback(acomp_ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> -                                  crypto_req_done, &acomp_ctx->wait);
> -
> -       mutex_init(&acomp_ctx->mutex);
>         return 0;
>
>  fail:
> @@ -866,14 +878,15 @@ static bool zswap_compress(struct folio *folio, long index,
>         bool mapped = false;
>
>         acomp_ctx = raw_cpu_ptr(pool->acomp_ctx);
> -       mutex_lock(&acomp_ctx->mutex);
> +       mutex_lock(&acomp_ctx->comp.mutex);
>
>         dst = acomp_ctx->buffer;
>         sg_init_table(&input, 1);
>         sg_set_folio(&input, folio, PAGE_SIZE, index * PAGE_SIZE);
>
>         sg_init_one(&output, dst, PAGE_SIZE);
> -       acomp_request_set_params(acomp_ctx->req, &input, &output, PAGE_SIZE, dlen);
> +       acomp_request_set_params(acomp_ctx->comp.req, &input, &output,
> +                                PAGE_SIZE, dlen);
>
>         /*
>          * it maybe looks a little bit silly that we send an asynchronous request,
> @@ -885,10 +898,12 @@ static bool zswap_compress(struct folio *folio, long index,
>          * existing method to send the second page before the first page is done
>          * in one thread doing zswap.
>          * but in different threads running on different cpu, we have different
> -        * acomp instance, so multiple threads can do (de)compression in parallel.
> +        * acomp instance, and compression and decompression use separate
> +        * requests, so multiple threads can do (de)compression in parallel.
>          */
> -       comp_ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
> -       dlen = acomp_ctx->req->dlen;
> +       comp_ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->comp.req),
> +                                  &acomp_ctx->comp.wait);
> +       dlen = acomp_ctx->comp.req->dlen;
>
>         /*
>          * If a page cannot be compressed into a size smaller than PAGE_SIZE,
> @@ -932,7 +947,7 @@ static bool zswap_compress(struct folio *folio, long index,
>         else if (alloc_ret)
>                 zswap_reject_alloc_fail++;
>
> -       mutex_unlock(&acomp_ctx->mutex);
> +       mutex_unlock(&acomp_ctx->comp.mutex);
>         return comp_ret == 0 && alloc_ret == 0;
>  }
>
> @@ -948,7 +963,7 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
>                 return false;
>
>         acomp_ctx = raw_cpu_ptr(pool->acomp_ctx);
> -       mutex_lock(&acomp_ctx->mutex);
> +       mutex_lock(&acomp_ctx->decomp.mutex);
>         zs_obj_read_sg_begin(pool->zs_pool, entry->handle, input, entry->length);
>
>         /* zswap entries of length PAGE_SIZE are not compressed. */
> @@ -965,15 +980,15 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
>         } else {
>                 sg_init_table(&output, 1);
>                 sg_set_folio(&output, folio, PAGE_SIZE, 0);
> -               acomp_request_set_params(acomp_ctx->req, input, &output,
> +               acomp_request_set_params(acomp_ctx->decomp.req, input, &output,
>                                          entry->length, PAGE_SIZE);
> -               ret = crypto_acomp_decompress(acomp_ctx->req);
> -               ret = crypto_wait_req(ret, &acomp_ctx->wait);
> -               dlen = acomp_ctx->req->dlen;
> +               ret = crypto_acomp_decompress(acomp_ctx->decomp.req);
> +               ret = crypto_wait_req(ret, &acomp_ctx->decomp.wait);
> +               dlen = acomp_ctx->decomp.req->dlen;
>         }
>
>         zs_obj_read_sg_end(pool->zs_pool, entry->handle);
> -       mutex_unlock(&acomp_ctx->mutex);
> +       mutex_unlock(&acomp_ctx->decomp.mutex);
>
>         if (!ret && dlen == PAGE_SIZE)
>                 return true;
> --
> 2.53.0-Meta
>