Re: [PATCH v1 0/8] Batched zswap_store() with compression batching

[Kanchana P. Sridhar]

On Mon, Oct 19, 2026 at 3:50 PM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
>
> On Sun, Oct 18, 2026 at 06:35:01PM -0700, Kanchana P. Sridhar wrote:
> > On Fri, Oct 16, 2026 at 10:07 PM Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Fri, Oct 16, 2026 at 10:02:13PM -0700, Kanchana P. Sridhar wrote:
> > > [ .. ]
> > >
> > > That isn't quite right though.  We need to ensure that it really
> > > is a singleton before taking the short-cut.  So you need at
> > > least one conditional:
> > >
> > >         if (likely(req->unit_size == req->slen))
> > >                 take short-cut
> >
> > I added the check for the singleton req [2]. I hope this looks Ok.
> >
> > With the latest crypto_acomp_compress(), the batch compress latency
> > improves considerably:
> >
> > usemem 10 iterations average throughput:
> >
> >    patch 6 (store batching only): 19,119 KB/s
> > patch 8 (+ compress batching): 24,076 KB/s (+25.93%)
>
> This is exactly what I mean by the numbers varying too much to draw
> any conclusions. I am not sure if it's the VM setup or the usemem test
> itself, but previously it was argued that the short-circuit in
> crypto_acomp_compress() was ~10% faster than the one in
> acomp_do_req_chain() (20,946 vs 19,058 KB/s) due to avoiding a function
> call and two conditionals, and now *adding* a conditional improves
> performance by 15% (20,946 -> 24,076 KB/s)?

I would like to clarify a few things:

1) The short-circuit in crypto_acomp_compress() that gave the 20,946
KB/s was missing the req->unit_size == req->slen "singleton check".
However, it gave better performance than the baseline patch 3 in v1,
which went through the complete acomp_do_req_chain() protocol (save
req, do one req, finish req, etc.).

2) When I added the same short-circuit in acomp_do_req_chain(), it did
not give as good performance as (1).

3) Herbert pointed out the short-circuit was missing the singleton check.

4) I added the singleton check into the original short-circuit (1),
and this did not degrade performance. It meets the functional
requirements for crypto, per Herbert's feedback.

5) I cannot make comparisons between "a function call + 2
conditionals" (2) and the functionally correct implementation in (4).
The functionally correct solution is qualified with "likely"
annotations, which can explain the performance improvement.

>
> Moreover, for zswap with ZSTD the added check is always true: the batch
> size is 1, so slen == unit_size == PAGE_SIZE for every request. The
> previous version and this one execute the same code, so the 15%
> difference can only be noise. With the stddev you reported (~5.4k KB/s
> over 10 iterations), both differences are well within the noise.
>
> I think something is wrong with the performance testing methodology
> here. The results are self-contradicting and do not make much sense to
> me. Did I miss something?

The previous version and this one don't execute the same code, as
clarified above.

I don't see how the results are self-contradicting. Can you clarify
what you mean by this?

OTOH, the zswap_compress() bpftrace results are clearly reinforcing
the benefits of compress batching.

"Sequential_zswap_compress" is stopping after patch 6.
"Batched_zswap_compress" is stopping after patch 8.

I hope we can agree based on the bpftrace data that
Batched_zswap_compress outperforms Sequential_zswap_compress.

>
> >
> > bpftrace measurement of zswap compress latency per-page:
> >
> > Sequential_zswap_compress  nr_samples     p25     p50      p75
> > p95       p99
> > =================================================================================
> > zswap_compress_ns_per_page  1,722,489  43,947  57,791   88,489
> > 130,165   323,961
> >
> > Batched_zswap_compress     nr_samples     p25     p50      p75
> > p95       p99
> > =================================================================================
> > zswap_compress_ns_per_page    665,267  41,511  54,915   77,707
> > 124,648   213,962
> > =================================================================================
> >  Latency saving (ns)                   -2,435  -2,876  -10,782
> > -5,517  -109,999
> > =================================================================================
>
> I measured zswap_store() (not zswap_compress()), to get the per-folio
> latency, and it didn't seem to change much beyond noise in my testing. I
> also notice that the number of samples is very different between cases.
>

I was addressing your earlier question about why not just stop after
store batching, and why batching of compressions is needed. I would
greatly appreciate it if you can review the zswap_compress()
sequential vs. batching per-page latency data from the bpftrace and
let me know if this convinces you?

I thought the overall zswap_store_pages() architecture had general
buy-in, since we bulk allocate entries, etc optimizations to minimize
locks. Please correct me if I am wrong. IOW, I am focusing on
gathering bpftrace data of zswap_compress() with batching since this
was the main patch that needs buy-in.

If we are on the same page thus far, can you please let me know why
bpftrace of zswap_store() is important, and if I should gather this in
my environment. I am happy to do so, but not sure what it will
prove/disprove.

Thanks,
Kanchana