Re: [PATCH v2 2/2] zswap: increment swapin count for non-pivot swapped in pages

[Nhat Pham <nphamcs@gmail.com>]

On Thu, Aug 1, 2024 at 1:02 PM Yosry Ahmed <yosryahmed@google.com> wrote:
>
>
> Hmm, but there is a chance that these pages are not actually needed,
> in which case we will unnecessarily increase the zswap protection.
> Does the readahead window self-correct if the pages were not used?

Hmm yeah it's kinda hard to predict if a swapped in page is strictly
necessary in this context. We don't have this information at the time
of the read.

That said, I think erring on the side of safety is OK here - my
understanding that readahead, while predictive in nature, only gets
progressively more aggressive if we get signals that it's helpful (i.e
the memory access patterns display sequential behavior).

I think we also accept this slight inaccuracy (i.e for pages in the
readahead window that might not necessarily be needed) the in
workingset refault handling behavior. Could you fact check me,
Johannes?


>
> > are also incrementing when the pages are read from the zswap pool, which
> > is inaccurate.
>
> I feel like this is the more important part. It should be the focus of
> the commit log with more details (i.e. why is it wrong to increment
> the zswap protection in this case).

Yeah this is pretty important too :) Maybe I should make it clearer in
the patch commit.

>
> Do we need a Fixes and cc:stable for this one? Maybe it can be moved
> first to make backports easy.

Hmm.

*Technically*, this is broken in older versions of the shrinker as
well, but it's more of an optimization than a bug that can crash the
kernel, so I don't know if it qualifies for a Fixes tag?

Another factor is, under the old scheme, this does not move the needle
much - at least in my benchmarks. This is because the decaying
behavior is so aggressive that incrementing the counter in a couple
places does not matter, when it will be rapidly divided by half later.
This fix only shows clear improvements when applied on top of the new
second chance scheme.

I don't have a strong opinion here, but it doesn't seem worth it to
backport IMHO :)