Re: [PATCH 1/2] mm: zswap: increase shrinking protection for zswap swapins only

[Yosry Ahmed]

On Wed, Mar 20, 2024 at 07:48:13AM -0700, Nhat Pham wrote:
> On Wed, Mar 20, 2024 at 2:51 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> >
> > On Wed, Mar 20, 2024 at 02:08:22AM +0000, Yosry Ahmed wrote:
> > > Currently, the number of protected zswap entries corresponding to an
> > > lruvec are incremented every time we swapin a page.
> >
> > Correct. This is the primary signal that the shrinker is being too
> > aggressive in moving entries to disk and should slow down...?
> 
> Yup. Currently, there are two scenarios in which we increase zswap
> protection area:
> 
> 1. zswap lru movement - for instance, if a page for some reasons is
> rotated in the LRU. When this happens, we increment the protection
> size, so that the page at the tail end of the protected area does not
> lose its protection because of (potentially) spurious LRU churnings.

I don't think we update the protected area during rotations anymore,
only when a new entry is added to the LRU (with potential decay).

> 2. swapin - when this happens, it is a signal that the shrinker is
> being too... enthusiastic in its reclaiming action. We should be more
> conservative, hence the increase in protection.
> 
> I think there's some confusion around this, because we use the
> same-ish mechanism for two different events. Maybe I should have put
> yet another fat comment at the callsites of zswap_folio_swapin() too
> :)

I think it makes sense. The confusion was mostly around the
interpretation of finding a page on disk. I was focused on pages that
skip zswap, but the main intention was for pages that were written back,
as I explained in my response to Johannes.

> 
> >
> > > This happens regardless of whether or not the page originated in
> > > zswap. Hence, swapins from disk will lead to increasing protection
> > > on potentially stale zswap entries. Furthermore, the increased
> 
> Hmmm my original thinking was that, had we protected the swapped-in
> page back when it was still in zswap, we would have prevented this IO.
> And since the pages in zswap are (at least conceptually) "warmer" than
> the swapped in page, it is appropriate to increase the zswap
> protection.
> 
> In fact, I was toying with the idea to max out the protection on
> swap-in to temporarily cease all zswap shrinking, but that is perhaps
> overkill :)

This would be problematic for pages that skip zswap IIUC, we'd want more
shrinking in this case.

> 
> > > shrinking protection can lead to more pages skipping zswap and going
> 
> I think this can only happen when the protection is so strong that the
> zswap pool is full, right?

Yeah that's what I had in mind. 

> 
> In practice I have never seen this happen though. Did you observe it?
> We have a fairly aggressive lru-size-based protection decaying as
> well, so that should not happen...

No this was all code inspection as I mentioned :)

> 
> Also technically the protection only applies to the dynamic shrinker -
> the capacity-driven shrinking is not affected (although it's not very
> effective - perhaps we can rework this?)

That logic is flawed anyway imo due to the acceptance threshold. We
should really get rid of that as we discussed before.