Received-SPF: pass (google.com: domain of linux-kernel+bounces-49044-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 2 Feb 2024 01:04:01 +0000
In-Reply-To: <20240201181240.GE321148@cmpxchg.org>
Precedence: bulk
Mime-Version: 1.0
References: <20240201-b4-zswap-invalidate-entry-v1-0-56ed496b6e55@bytedance.com>
 <20240201-b4-zswap-invalidate-entry-v1-5-56ed496b6e55@bytedance.com> <20240201181240.GE321148@cmpxchg.org>
Message-ID: <Zbw_gZIyEHquaxho@google.com>
Subject: Re: [PATCH 5/6] mm/zswap: only support zswap_exclusive_loads_enabled
From: Yosry Ahmed <yosryahmed@google.com>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Chengming Zhou <zhouchengming@bytedance.com>, Nhat Pham <nphamcs@gmail.com>, 
	Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org, 
	linux-mm@kvack.org
Content-Type: text/plain; charset="us-ascii"

On Thu, Feb 01, 2024 at 01:12:40PM -0500, Johannes Weiner wrote:
> On Thu, Feb 01, 2024 at 03:49:05PM +0000, Chengming Zhou wrote:
> > The !zswap_exclusive_loads_enabled mode will leave compressed copy in
> > the zswap tree and lru list after the folio swapin.
> > 
> > There are some disadvantages in this mode:
> > 1. It's a waste of memory since there are two copies of data, one is
> >    folio, the other one is compressed data in zswap. And it's unlikely
> >    the compressed data is useful in the near future.
> > 
> > 2. If that folio is dirtied, the compressed data must be not useful,
> >    but we don't know and don't invalidate the trashy memory in zswap.
> > 
> > 3. It's not reclaimable from zswap shrinker since zswap_writeback_entry()
> >    will always return -EEXIST and terminate the shrinking process.
> > 
> > On the other hand, the only downside of zswap_exclusive_loads_enabled
> > is a little more cpu usage/latency when compression, and the same if
> > the folio is removed from swapcache or dirtied.
> > 
> > Not sure if we should accept the above disadvantages in the case of
> > !zswap_exclusive_loads_enabled, so send this out for disscusion.
> > 
> > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> 
> This is interesting.
> 
> First, I will say that I never liked this config option, because it's
> nearly impossible for a user to answer this question. Much better to
> just pick a reasonable default.
> 
> What should the default be?
> 
> Caching "swapout work" is helpful when the system is thrashing. Then
> recently swapped in pages might get swapped out again very soon. It
> certainly makes sense with conventional swap, because keeping a clean
> copy on the disk saves IO work and doesn't cost any additional memory.
> 
> But with zswap, it's different. It saves some compression work on a
> thrashing page. But the act of keeping compressed memory contributes
> to a higher rate of thrashing. And that can cause IO in other places
> like zswap writeback and file memory.

Agreed.

At Google, we have been using exclusive loads for a very long time in
production, so I have no objections to this. The user interface is also
relatively new, so I don't think it will have accumulated users.

> 
> It would be useful to have an A/B test to confirm that not caching is
> better. Can you run your test with and without keeping the cache, and
> in addition to the timings also compare the deltas for pgscan_anon,
> pgscan_file, workingset_refault_anon, workingset_refault_file?

That would be interesting :)