Received-SPF: pass (google.com: domain of linux-kernel+bounces-50670-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
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> <d5455b43-aa22-4931-a6b3-062128f4d30f@bytedance.com>
In-Reply-To: <d5455b43-aa22-4931-a6b3-062128f4d30f@bytedance.com>
From: Nhat Pham <nphamcs@gmail.com>
Date: Fri, 2 Feb 2024 14:31:31 -0800
Message-ID: <CAKEwX=ODvtTq1gs3KFzinCfssGeD4f0qWdPC9Ci1LBGwW6hSag@mail.gmail.com>
Subject: Re: [PATCH 5/6] mm/zswap: only support zswap_exclusive_loads_enabled
To: Chengming Zhou <zhouchengming@bytedance.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>, Andrew Morton <akpm@linux-foundation.org>, 
	Yosry Ahmed <yosryahmed@google.com>, linux-kernel@vger.kernel.org, linux-mm@kvack.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 2, 2024 at 4:57=E2=80=AFAM Chengming Zhou
<zhouchengming@bytedance.com> wrote:
>
> On 2024/2/2 02:12, 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_entr=
y()
> >>    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.
>
> Agree.
>
> >
> > 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.
> >
> > 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?
>
> I just A/B test kernel building in tmpfs directory, memory.max=3D2GB.
> (zswap writeback enabled and shrinker_enabled, one 50GB swapfile)
>
> From the below results, exclusive mode has fewer scan and refault.
>
>                               zswap-invalidate-entry        zswap-invalid=
ate-entry-exclusive
> real                          63.80                         63.01
> user                          1063.83                       1061.32
> sys                           290.31                        266.15
>                               zswap-invalidate-entry        zswap-invalid=
ate-entry-exclusive

This is one of those cases where something might make sense
conceptually, but does not pan out in practice. Removing
non-invalidate seems to simplify the code a bit, and that's one less
thing to worry about for users, so I like this :)

Reviewed-by: Nhat Pham <nphamcs@gmail.com>

> workingset_refault_anon       2383084.40                    1976397.40
> workingset_refault_file       44134.00                      45689.40
> workingset_activate_anon      837878.00                     728441.20
> workingset_activate_file      4710.00                       4085.20
> workingset_restore_anon       732622.60                     639428.40
> workingset_restore_file       1007.00                       926.80
> workingset_nodereclaim        0.00                          0.00
> pgscan                        14343003.40                   12409570.20
> pgscan_kswapd                 0.00                          0.00
> pgscan_direct                 14343003.40                   12409570.20
> pgscan_khugepaged             0.00                          0.00