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

[Usama Arif]

On 03/11/2026 22:05, Kanchana P. Sridhar wrote:
> On Tue, Nov 3, 2026 at 6:26 AM Usama Arif <usama.arif@xxxxxxxxx> wrote:
>>
>> On Mon, 12 Oct 2026 17:21:40 -0700 "Kanchana P. Sridhar" <kanchanapsridhar2026@xxxxxxxxx> wrote:
>>
>>> On Fri, Oct 9, 2026 at 6:31 PM 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, so group it with the compression request. Independent
>>>> requests can share the per-CPU transform; the codecs and drivers
>>>> synchronize their shared state internally. Loads can still wait for
>>>> each other on the decompression mutex, and stores still serialize on
>>>> the compression mutex.
>>>>
>>>> acomp_request_free() ignores NULL requests, so drop the check in
>>>> acomp_ctx_free().
>>>>
>>>> This follows the proposal from Sergey Senozhatsky for the same split
>>>> for zram [1].
>>>>
>>>> [1] https://lore.kernel.org/all/20261005122036.718976-10-senozhatsky@xxxxxxxxxxxx/
>>>>
>>>> Signed-off-by: Usama Arif <usama.arif@xxxxxxxxx>
>>>
>>> I have a question with regards to this patch. Is the intent to allow a
>>> decompress thread to run concurrently with a store thread for a given process?
>>
>> The request is shared by tasks using the same CPU and zswap pool.
>> These may be different processes or threads of the same process.
>> Each operation still runs synchronously in its caller.
>>
>> The motivating case can happen entirely on one CPU:
>> 1. Task A compresses page X and releases the codec's stream lock.
>> 2. A is preempted while still holding the zswap request mutex.
>> 3. Task B runs and faults in an already stored page Y.
>>
>> With the shared request, B blocks until A runs again and releases the
>> mutex. Separate requests let B load Y while A's store is unfinished.
>> The codec/driver still synchronizes its own shared state.
>>
>>> If so, I wonder if incurring context switch/PTE/TLB shootdowns performance
>>> penalty is worth it?
>>
>> The switch to B already occurs with the existing code. This patch adds
>> no worker threads, forced migration, PTE changes or TLB shootdowns.
>> It avoids B's subsequent wait on A's request mutex. The 1-vCPU
>> benchmark in the cover letter exercises this without CPU migration.
>>
>>>
>>> With the proposed change, will zswap_store() continue to run on the "old" CPU
>>> (i.e. the one to which the process got migrated), or is it a requirement that
>>> the "old" process is in sleep state? If so, should we codify/document this
>>> requirement?
>>>
>>
>> There is no requirement for A to sleep. A can remain runnable while
>> preempted. Its zswap_store() invocation resumes in the same task, on
>> whichever CPU the scheduler selects. If it migrates after selecting
>> the context, it retains that pointer and the corresponding mutex,
>> as the existing code does. The same task cannot execute on two CPUs
>> at once.
>>
>>> On the new CPU, is the same process "runnable", and if so, how do we ensure that
>>> the "higher priority" decompress is handled while the zswap_store() prior to
>>> preemption is still not done?
>>
>> Normal scheduler policy decides when B runs. Once it runs, the split
>> removes its dependency on A releasing the shared request mutex.
>> A load starting on another CPU already selects a different context
>> today; migration is unnecessary for the benefit described here.
>>
>> Existing folio/swap-cache locking still orders accesses to the same
>> page. The example above concerns two independent pages.
> 
> Thanks for explaining the motivation for this change, and the detailed
> analysis! Is it possible that this could cause compression contexts to
> be starved of compute resources under memory pressure, potentially
> causing deadlocks with page-faults in the worst-case scenario? 

The split removes an unnecessary dependency between unrelated pages.
Under fair scheduling, runnable reclaimers still get CPU time, and a
fault needing memory can perform direct reclaim itself. Faults on a page
being stored still wait for its folio lock, so I don't see a new deadlock
cycle here.

> Since
> compression is inherently slower than decompression, could this be
> making it easier for swap page-faults to preempt compression.

On non-PREEMPT_RT kernels, software compression already runs under the
codec's stream spinlock with preemption disabled. The delay addressed
here is after that lock is released; the split does not make active
compression newly preemptible.


> On a
> related note, should direct reclaim (only) be allowed to use a
> non-preemptible context via a spinlock?
> 
I would keep the current split. The codec already protects its stream,
while the outer context must remain sleepable for asynchronous
completion waits. A spinlock cannot safely cover those waits.