Received-SPF: pass (google.com: domain of linux-kernel+bounces-109352-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 20 Mar 2024 19:32:33 +0000
In-Reply-To: <CAKEwX=ODDeetg_iv2kcds_DziJ5og2F6STsLSXE85qufYET=eg@mail.gmail.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240320020823.337644-1-yosryahmed@google.com>
 <20240320095053.GA294822@cmpxchg.org> <CAKEwX=ODDeetg_iv2kcds_DziJ5og2F6STsLSXE85qufYET=eg@mail.gmail.com>
Message-ID: <Zfs50cdvcOpvnd8F@google.com>
Subject: Re: [PATCH 1/2] mm: zswap: increase shrinking protection for zswap
 swapins only
From: Yosry Ahmed <yosryahmed@google.com>
To: Nhat Pham <nphamcs@gmail.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>, Andrew Morton <akpm@linux-foundation.org>, 
	Chengming Zhou <chengming.zhou@linux.dev>, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 20, 2024 at 07:48:13AM -0700, Nhat Pham wrote:
> On Wed, Mar 20, 2024 at 2:51=E2=80=AFAM Johannes Weiner <hannes@cmpxchg.o=
rg> 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...?
>=20
> Yup. Currently, there are two scenarios in which we increase zswap
> protection area:
>=20
> 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.
>=20
> 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.

>=20
> >
> > > 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
>=20
> 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.
>=20
> 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.

>=20
> > > shrinking protection can lead to more pages skipping zswap and going
>=20
> 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.=20

>=20
> 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 :)

>=20
> 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.