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

[Yosry Ahmed]

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

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