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

[Kanchana P. Sridhar]

On Thu, Oct 15, 2026 at 4:45 PM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
>
> > > > > > > How much of the performance benefits come from patch 6 vs patch 8?
> > > > > >
> > > > > > This is how the performance changes: Average throughput is averaged
> > > > > > from 10 iterations of usemem.
> > > > > >
> > > > > > patch 6: 19,119 KB/s
> > > > > > patch 8: 17,593 KB/s
> > > > > >
> > > > > > So the acomp_do_req_chain() path that ZSTD goes through with patch 8
> > > > > > causes a regression, even though it provides a generic batching
> > > > > > framework in acomp.
> > > > > >
> > > > > > I was able to retrieve performance with the following change in acompress.c:
> > > > >
> > > > > Why add it to begin with in this case?
> > > >
> > > > Patch 3 ("crypto: acomp - Add trivial segmentation wrapper.") was
> > > > co-developed with Herbert. The request chaining for non-segmentation
> > > > algorithms was as in Herbert's original implementation. Since this
> > > > causes a regression, I would like to get Herbert's Ok on replacing the
> > > > request chaining in this part of crypto_acomp_compress().
> > >
> > > I mean, why add patch 8 if it doesn't improve performance? Seems like
> > > even with your diff, we only recover performance, but we don't improve
> > > it after patch 6?
> >
> > Patch 8 does improve performance over patch 6 (19,578 KB/s vs. 19,119
> > KB/s) by 2.4%. Besides, patch 8 provides a compress batching framework
> > in zswap, which is the purpose of this patch series.
>
> No, the purpose of this patch series is to improve performance, be it
> through the compression batching framework or otherwise. The
> compression batching framework made sense as it provided greater
> performance gains with HW batching/concurrency. Without that, I am
> honestly skeptical. 2.4% can be within the noise.

I seem to have left out patch 8 after making the crypto_acomp change
to not use request chaining.
My apologies! This was an honest mistake..

I have regathered the 10 iterations average throughput of usemem after
applying patch 8:

                                        patch 6: 19,119 KB/s
crypto_acomp change + patch 8: 20,946 KB/s (+9.56%)

The purpose of this patch series is still the same: to set the
framework for batching of zswap compressions, and not just of zswap
stores of large folios which is what stopping at patch 6 would do.

With SW compressors (ZSTD), patch 8, i.e., batching of compressions
and storing the compressed data in a pool, demonstrates significant
performance improvement (sorry again for having missed applying the
patch earlier today). This is because we isolate the working set to
just these ops, which benefits from cache locality and minimizes
evictions: as against the vectorized implementation without the patch.

One of the immediate benefits of this framework is that I am using it
for bulk allocations of compressed data for a batch, for entries that
map to the same slab bin. This proves the forward-looking benefits of
isolating compression and storage from other zswap_store_pages() ops.

The crypto_acomp change I had mentioned earlier does not hinder HW
compressors in any way. As mentioned in the commit logs, patches 1-4
provide the API that can be used by HW compressors as they see fit, as
long as they register the segmentation bit in the algorithm creation.

If you are Ok with all this, we will need Herbert's Ok for this change
in patch 3:

diff --git a/crypto/acompress.c b/crypto/acompress.c
index 192ea996e779..db8e4c82b900 100644
--- a/crypto/acompress.c
+++ b/crypto/acompress.c
@@ -293,8 +293,12 @@ int crypto_acomp_compress(struct acomp_req *req)
                return -EAGAIN;

        if (likely(req->unit_size && acomp_request_issg(req))) {
-               if (!crypto_acomp_req_seg(tfm))
-                       return acomp_do_req_chain(req, true);
+               if (likely(!crypto_acomp_req_seg(tfm))) {
+                       int err = tfm->compress(req);
+
+                       req->dst->length = unlikely(err) ? err : req->dlen;
+                       return err;
+               }

                return tfm->compress(req);
        }

>
> > zswap is the
> > first user of the crypto_acomp compress batching API provided earlier
> > in this series, which I think, is a great starting point for us to
> > evolve this further, as mentioned in the cover letter.
>
> Without a non-trivial performance benefit (e.g. with HW batching), I
> don't see enough justification. My advice is to try to push 6 (and its
> dependencies) separately.

This doesn't penalize HW compressors, while representing the status of
what exists today (sequential).