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

[Usama Arif]

On 06/10/2026 11:47, Nhat Pham wrote:
> On Tue, Oct 6, 2026 at 2:23 AM 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. 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].
> 
> Thanks, zram peeps :P
> 
>>
>> [1] https://lore.kernel.org/all/20261005122036.718976-10-senozhatsky@xxxxxxxxxxxx/
>>
>> Signed-off-by: Usama Arif <usama.arif@xxxxxxxxx>
> 
> Code mostly LGTM. Just one question:
> 
> [...]
> 
>> -        * 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;
> 
> Hmm do we not have to null check here anymore? Does
> acomp_request_free() handle NULL itself too?

Yes, since v6.15 it starts with "if (!req || ...) return;", so the
check in acomp_ctx_free() was redundant.


> 
> For instance, taking the code blob below:
> 
>> -       /* 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);
> 
> Here, we can success with the comp's req but fail with the decomp's req, right?

Right. decomp.req is then NULL, as zswap_acomp_req_init() stores what
acomp_request_alloc() returned, and acomp_ctx_free() frees comp.req
and skips decomp.req. After patch 2, decomp.req also stays NULL for
synchronous algorithms, since the per-CPU contexts are zeroed, and
acomp_ctx_free() relies on the same NULL handling.