On Fri, Aug 2, 2024 at 4:46 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote: > > 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). If the readahead logic is expected to adapt in these situations (and I think it is), then I think we are fine. Perhaps we should just leave a comment that we may increase the protection more than we should for those readahead cases. > > 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 :) I thought it's a simple change worth backporting, but if it doesn't move the needle without the second chance algorithm then it's probably not worth it. I would still add the "Fixes" tag because technically the logic is wrong without this patch, it increases the zswap protection when there swapins from zswap which doesn't make much sense.
