Received-SPF: pass (google.com: domain of linux-kernel+bounces-109341-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:28:30 +0000
In-Reply-To: <20240320095053.GA294822@cmpxchg.org>
Precedence: bulk
Mime-Version: 1.0
References: <20240320020823.337644-1-yosryahmed@google.com> <20240320095053.GA294822@cmpxchg.org>
Message-ID: <Zfs43iLE0RNcQPSj@google.com>
Subject: Re: [PATCH 1/2] mm: zswap: increase shrinking protection for zswap
 swapins only
From: Yosry Ahmed <yosryahmed@google.com>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Andrew Morton <akpm@linux-foundation.org>, Nhat Pham <nphamcs@gmail.com>, 
	Chengming Zhou <chengming.zhou@linux.dev>, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"

On Wed, Mar 20, 2024 at 05:50:53AM -0400, Johannes Weiner 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...?

Right, I think that's where I was confused. See below :)

> 
> > 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
> > shrinking protection can lead to more pages skipping zswap and going
> > to disk, eventually leading to even more swapins from disk and
> > starting a vicious circle.
> 
> How does shrinker protection affect zswap stores?
> 
> On the contrary, I would expect this patch to create a runaway
> shrinker. The more aggressively it moves entries out to disk, the
> lower the rate of zswap loads, the more aggressively it moves more
> entries out to disk.

I think I found the source of my confusion. As you described, the
intention of the protection is to detect that we are doing writeback
more aggressively than we should. This is indicated by swapins
(especially those from disk).

However, this assumes that pages swapped in from disk had gone there by
shrinking/writeback. What I was focused on was pages that end up on disk
because of the zswap limit. In this case, a disk swapin is actually a
sign that we swapped hot pages to disk instead of putting them in zswap,
which probably indicates that we should shrink zswap more aggressively
to make room for these pages.

I guess the general assumption is that with the shrinker at play, pages
skipping zswap due to the limit becomes the unlikely scenario -- which
makes sense. However, pages that end up on disk because they skipped
zswap should have a higher likelihood of being swapped in, because they
are hotter by definition.

Ideally, we would be able to tell during swapin if this page was sent
to disk due to writeback or skipping zswap and increase or decrease
protection accordingly -- but this isn't the case.

So perhaps the answer is to decrease the protection when pages skip
zswap instead? Or is this not necessary because we kick off a background
shrinker anyway? IIUC the latter will stop once we reach the acceptance
threshold, but it might be a good idea to increase shrinking beyond
that under memory pressure.

Also, in an ideal world I guess it would be better to distinguish
swapins from disk vs. zswap? Swapins from disk should lead to a bigger
increase in protection IIUC (assuming they happened due to writeback).

Sorry if my analysis is still off, I am still familiarizing myself with
the shrinker heuristics :)

> 
> > Instead, only increase the protection when pages are loaded from zswap.
> > This also has a nice side effect of removing zswap_folio_swapin() and
> > replacing it with a static helper that is only called from zswap_load().
> > 
> > No problems were observed in practice, this was found through code
> > inspection.
> 
> This is missing test results :)

I intentionally wanted to send out the patch first to get some feedback,
knowing that I probably missed something. In retrospect, this should
have been an RFC patch. Patch 2 should be irrelevant tho, I only
bundled them because they touch the same area.