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

[Usama Arif <usama.arif@linux.dev>]

On 07/10/2026 23:28, Yosry Ahmed wrote:
> On Mon, Oct 5, 2026 at 5:23 PM Usama Arif <usama.arif@linux.dev> 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@linux.dev>
>> ---
>>  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.


> 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.

> 
>> +               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?

Added for v2.

> 
>> +       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().

Added for v2.

> 
> 
>> +               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.


> 
>> +       }
>> +
>> +       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
>>