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

[Yosry Ahmed]

On Fri, Oct 9, 2026 at 8:27 AM Usama Arif <usama.arif@xxxxxxxxx> wrote:
>
>
>
> On 07/10/2026 23:28, Yosry Ahmed wrote:
> > 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().
>
> ACOMP_REQUEST_ON_STACK() reserves sizeof(struct acomp_req) plus
> MAX_SYNC_COMP_REQSIZE, which is currently zero. A synchronous acomp
> algorithm can advertise a larger request context via cra_reqsize. Checking
> only acomp_is_async() would then leave that context outside the reserved
> stack storage.
>
> The core already applies the same size restriction to synchronous fallback
> transforms in crypto_acomp_init_tfm(). The operation functions only reject
> asynchronous stack requests; they do not validate the amount of storage
> provided.
>
> >
> > 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.
> >
>
> Done for v2. Added documented acomp_can_use_stack_req() in the public acomp
> header. It checks both synchronous completion and request-context size;
> zswap now uses that helper.

Thanks!

>
>
> > Also, could you please CC Herbert on future iterations? I would like
> > to get his eyes on any crypto-related changes if possible.
>
> Will do! Thanks! get_maintainers.pl didnt add him.

Yeah because it didn't actually change crypto files, but I usually
prefer someone who actually understands crypto (aka not me) to take a
look :P

[..]
>
> >
> >
> >> +               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?
>
>
> I would keep req and wait as separate arguments. zswap_acomp_req includes a
> mutex that stack decompression never uses, and taking the address of its
> wait can still reserve the entire wrapper on the stack. Removing the mutex
> from that type would add more restructuring. The shared callback helper and
> path comment make the setup consistent while keeping the existing stack
> footprint.

I am fine with that.