Re: [PATCH v3 0/8] Batched zswap_store() framework with compression batching.

[Yosry Ahmed]

> > As with previous versions, I cannot reproduce these results. I tried
> > with a QEMU VM on a Cascade Lake host. I saw marginal improvements with
> > patch 6 and no significant gain with patches 7-8.
> >
> > I noticed that you omitted high-level metrics (e.g. throughput), I
> > didn't see significant changes in throughput, and I also ran a kernel
> > build and saw a small regression in some cases (probably with order-0
> > folios) and no significant improvement otherwise. Even if latency for
> > some functions does improve slightly, it doesn't matter if it doesn't
> > show up on higher-level metrics.
> >
> > It could be the difference in setups, or the overhead of bpftrace hooks
> > for zswap_compress() might affect measured zswap_store() latencies, as
> > you will have fewer zswap_compress() calls with patch 8.
> >
> > As advised previously, I would trim down patch 6 to the bare minimum
> > (without all the scaffolding and prep work for patches 7-8), and send it
> > separately (with patch 5), and we can check if it's worth adding, I am
> > still concerned it might be adding overhead for order-0 to be honest.
> >
> > I am questioning the benefit of batching here without HW batching, so
> > far the results are more concerning than encouraging. I already spent
> > too much time trying to reproduce the performance gains. Also, sending
> > new versions without resolving discussions on previous ones is not
> > really productive.
>
> Thanks for taking the time to test and provide feedback, Yosry.
>
> I omitted high-level metrics like throughput since you raised a
> concern about variation. Hence, I decided to go with the "ultimate
> truth" which is bpftrace latency measurements.

I didn't mean that we shouldn't look at high-level metrics, it's just
that the test setup had too much variance for the high-level metrics
to be useful so I checked the low-level latency. Ideally we'd test
performance with benchmarks that should have stable high-level
metrics.

> For performance validation, I only have access to a 16G Chromebook
> with a Crostini VM to which 13G is allocated. I am setting aside 3G
> for swap and 7G for QEMU. Within the QEMU VM, I am selecting 2 E-cores
> to run the 2 usemem processes to minimize variation due to frequency
> fluctuations. This setup has its own limitations such as being unable
> to fix the frequency.
>
> Despite all this, I have been able to consistently replicate the
> bpftrace latency improvements with patch 8. My results show regression
> with patch 6-7, and improvement only with patch 8. As noted in the
> cover-letter, patch 8 adds significant latency improvement for order-0
> folios. This is with ZSTD. FWIW, these are bpftrace latency
> improvements in a significantly memory constrained environment and
> this ought to be a useful data point. I have been posting general
> vmstat "system health" metrics to prove that nothing unexpected is
> going on.
>
> My data shows that order-0 zswap_store() tail latency improves by 11%
> over mm-unstable with patch 8, although p50 is roughly similar between
> patches 6-7 and 8. PMD store latency is significantly better at p50
> and p99 with patch 8.
>
> Likewise, p99 latency of zswap_compress() alone improves by 3% with
> patch 8, while it degrades with patch 6-7. p50 is almost the same with
> patch 8, although patch 6-7 regresses. The page-normalized
> batch_compress() p99 latency is better by 15%. All comparisons are wrt
> mm-unstable.
>
> Hence, I believe even in a memory-constrained setup such as mine,
> batching significantly (7-15%) improves p99 in all cases, with 4-5%
> gains in p50: for order-0 and PMD folios.
>
> In addition to the latency benefits shown through my validation, I
> would like to also emphasize the strategic benefit of moving zswap to
> use batching, as noted under "Conclusions" in my cover letter. IOW,
> batching within large folios to improve workload metrics can only do
> so much (in proportion to the # of large folios reclaimed and
> contribution of zswap_store() to the overall allocation-reclaim path).
> What this patch series attempts to provide is a zswap batching
> framework upon which additional improvements can be built, such as
> reclaiming multiple folios as a batch. I believe that compress
> batching is essential to support these next steps, which improve
> workload throughput/latency metrics even more than simply batching
> within folios. This is with ZSTD.

Without a direct line of sight for such features and consensus from
the community, landing the framework in zswap is not very useful, it
actually increases maintenance burden.

> Due to the environmental constraints, I haven't been able to reliably
> use kernel compilation for validation.
>
> I have been doing my best to resolve previous discussions by
> incorporating the calculate_high_delay() hack that you suggested to
> reduce variation. The data posted in v3 comprehends this suggestion.

Oh I wasn't suggesting you generally test with this hack, I just
noticed that the variance was resulting from memory.high throttling
and nullified it to see what the numbers look like without it. As I
mentioned above, ideally we see stable improvement in high-level
metrics. usemem is already a microbenchmark, if we can't demonstrate
stable improvements in its metrics and have to measure latency of
specific functions, the significance of the optimizations is
questionable.

It's a tradeoff between maintainability and performance. It doesn't
really make sense to add complexity for marginal performance benefits
on a small subset of setups.

>
> I would like to suggest/request the following, as next steps:
>
> 1) Can we reconcile our methodologies for usemem bpftrace
> measurements: # of vCPU, usemem command line, reading Silesia, etc.
> Also swap space and QEMU memory allocation, using linear lhists() in
> bpftrace, etc. If you can share these details, it would be very
> helpful. Thanks again for your time!

I ran a VM with 2 vCPUs, 8G of memory, and a 25G swapfile (although
the size of the swapfile doesn't matter much).

I used zstd with the default pool limit (20%) and the shrinker off,
with 2M THP enabled. I set memory.high to 3G for usemem like you.

I used this usemem command with the diff you posted in the last version:
$ usemem -w --read-silesia -O -b 1 -s 10 -n 2 2400M

For kernel build, I used:
$ kcbench --llvm -s 6.8 --nodownload -j 8 -i 1 -o <PATH>--detailed-results

I don't have the bpftrace commands but it should be pretty standard (I
think I used hist() not lhist() tho).

All that being said, I don't think I have more time to spend on this tbh.

>
> 2) Does the kernel community offer servers for contributors to test
> their optimizations at scale?

I don't believe so, but I am not sure.

> 3) Can I please request the help of other zswap maintainers/reviewers
> to validate this series with ZSTD on a server setup with usemem
> running on 30+ cores, under high over-commit ratios; through bpftrace
> and/or throughput metrics.

I can't ask people to spend time testing this, I already spent more
time than I have and I am sure other people are also busy. If someone
is interested in testing this and collecting performance numbers, that
would be up to them.