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

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

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


Hello Yosry!

Thanks for the reviews!

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

Independent acomp requests can share a transform. UBIFS already uses its
shared compr->cc for compression and decompression without caller
serialization. IPComp also submits per-packet requests on a shared
transform, and EROFS allows concurrent decompression on one transform.

Drivers synchronize their shared state internally. For example, HiSilicon
protects its transform-owned request bitmap with req_lock. Unsynchronized
shared state would break those existing callers too. Separate compression
and decompression transforms would still leave concurrent stack
decompressions sharing a transform.


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


Done for next revision. Added zswap_comp_ctx containing the compression
request and output buffer. Allocation, use and cleanup now go through that
context. The compression mutex protects the buffer until zs_obj_write()
finishes, and decompression has no buffer member.

> 
>> 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@chromium.org/
>>
>> Signed-off-by: Usama Arif <usama.arif@linux.dev>
>> ---
>>  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
>>