Re: [PATCH 2/2] mm: zswap: use stack requests for synchronous decompression

[Yosry Ahmed]

On Mon, Oct 5, 2026 at 5:23 PM Usama Arif <usama.arif@xxxxxxxxx> wrote:
>
> With separate requests for compression and decompression, loads still
> serialize on the per-CPU decompression mutex. A low-priority load that
> is preempted after the codec drops its stream lock keeps holding the mutex
> and stalls every other load on that CPU, including higher-priority ones.
>
> Synchronous algorithms whose requests need no extra context can use an
> on-stack request, so decompress with one and take no zswap lock. All
> in-tree software compressors qualify. Asynchronous algorithms, and
> synchronous ones with request context, keep the per-CPU request and
> mutex, which is still taken before the zsmalloc read lock.
>
> Reading the per-CPU context without the mutex is safe. Since
> commit ef3c0f6cb798e ("mm: zswap: tie per-CPU acomp_ctx lifetime to the
> pool"), it is set up before its CPU comes online and is not torn down
> until the pool is destroyed. The codecs keep their own stream locks, and
> crypto_acomp_decompress() rejects on-stack requests only for
> asynchronous transforms, which never take this path.
>
> For software compressors this drops the heap request added by the
> previous patch. The on-stack request and wait take 216 bytes, which
> makes the load path about 270 bytes deeper on x86-64. Asynchronous
> algorithms pay this too.
>
> Signed-off-by: Usama Arif <usama.arif@xxxxxxxxx>
> ---
>  mm/zswap.c | 65 +++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 54187b1ef751d..7e7fb6e7ec24c 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -147,7 +147,7 @@ struct zswap_acomp_req {
>  struct crypto_acomp_ctx {
>         struct crypto_acomp *acomp;
>         struct zswap_acomp_req comp;
> -       struct zswap_acomp_req decomp;
> +       struct zswap_acomp_req decomp;  /* unused by synchronous algorithms */
>         u8 *buffer;
>  };
>
> @@ -851,15 +851,20 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
>                 goto fail;
>         }
>
> -       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;
> +       if (zswap_acomp_req_init(&acomp_ctx->comp, acomp_ctx->acomp))
> +               goto req_fail;
> +
> +       /* Synchronous algorithms decompress with an on-stack request. */
> +       if (acomp_is_async(acomp_ctx->acomp) ||
> +           crypto_acomp_reqsize(acomp_ctx->acomp) > MAX_SYNC_COMP_REQSIZE) {

Where does the requirement on crypto_acomp_reqsize() come from? I see
crypto_acomp_compress() and crypto_acomp_decompress() only checking
acomp_is_async().

Regardless, these are crypto-specific details that shouldn't be
checked directly by zswap. Ideally we'd have something like
acomp_can_use_stack_req() or something.

Also, could you please CC Herbert on future iterations? I would like
to get his eyes on any crypto-related changes if possible.

> +               if (zswap_acomp_req_init(&acomp_ctx->decomp, acomp_ctx->acomp))
> +                       goto req_fail;
>         }
>
>         return 0;
>
> +req_fail:
> +       pr_err("could not alloc crypto acomp_request %s\n", pool->tfm_name);
>  fail:
>         acomp_ctx_free(acomp_ctx);
>         return ret;
> @@ -951,19 +956,14 @@ static bool zswap_compress(struct folio *folio, long index,
>         return comp_ret == 0 && alloc_ret == 0;
>  }
>
> -static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
> +static bool __zswap_decompress(struct zswap_entry *entry,
> +                              struct zswap_pool *pool, struct acomp_req *req,
> +                              struct crypto_wait *wait, struct folio *folio)
>  {
> -       struct zswap_pool *pool = zswap_entry_pool(entry);
>         struct scatterlist input[2]; /* zsmalloc returns an SG list 1-2 entries */
>         struct scatterlist output;
> -       struct crypto_acomp_ctx *acomp_ctx;
>         int ret = 0, dlen;
>
> -       if (WARN_ON_ONCE(!pool))
> -               return false;
> -
> -       acomp_ctx = raw_cpu_ptr(pool->acomp_ctx);
> -       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. */
> @@ -980,15 +980,14 @@ 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->decomp.req, input, &output,
> -                                        entry->length, PAGE_SIZE);
> -               ret = crypto_acomp_decompress(acomp_ctx->decomp.req);
> -               ret = crypto_wait_req(ret, &acomp_ctx->decomp.wait);
> -               dlen = acomp_ctx->decomp.req->dlen;
> +               acomp_request_set_params(req, input, &output, entry->length,
> +                                        PAGE_SIZE);

Just use a single line :)

> +               ret = crypto_acomp_decompress(req);
> +               ret = crypto_wait_req(ret, wait);
> +               dlen = req->dlen;
>         }
>
>         zs_obj_read_sg_end(pool->zs_pool, entry->handle);
> -       mutex_unlock(&acomp_ctx->decomp.mutex);
>
>         if (!ret && dlen == PAGE_SIZE)
>                 return true;
> @@ -1002,6 +1001,32 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
>         return false;
>  }
>
> +static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
> +{
> +       struct zswap_pool *pool = zswap_entry_pool(entry);
> +       struct crypto_acomp_ctx *acomp_ctx;
> +       bool ret;
> +
> +       if (WARN_ON_ONCE(!pool))
> +               return false;
> +
> +       acomp_ctx = raw_cpu_ptr(pool->acomp_ctx);

We probably want a comment here explaining the two possible paths?

> +       if (!acomp_ctx->decomp.req) {
> +               ACOMP_REQUEST_ON_STACK(req, acomp_ctx->acomp);
> +               DECLARE_CRYPTO_WAIT(wait);
> +
> +               acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> +                                          crypto_req_done, &wait);

I would rather add a wrapper for this (e.g.
zswap_set_acomp_req_callback()) to avoid the mental toil of checking
that we are passing in the same things as zswap_cpu_comp_prepare().


> +               return __zswap_decompress(entry, pool, req, &wait, folio);

Hmm would it be more readable if we create a dummy zswap_acomp_req
object here and have __zswap_decompress() take in __zswap_decompress
instead of taking in the req and wait separately?

> +       }
> +
> +       mutex_lock(&acomp_ctx->decomp.mutex);
> +       ret = __zswap_decompress(entry, pool, acomp_ctx->decomp.req,
> +                                &acomp_ctx->decomp.wait, folio);
> +       mutex_unlock(&acomp_ctx->decomp.mutex);
> +       return ret;
> +}
> +
>  /*********************************
>  * writeback code
>  **********************************/
> --
> 2.53.0-Meta
>