On Thu, Aug 1, 2024 at 1:02 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> 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 :)
