Received-SPF: pass (google.com: domain of linux-kernel+bounces-214078-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240610121820.328876-1-usamaarif642@gmail.com>
 <CAJD7tkZdx7xJiDcvayH1aRW9Q6GQaZnF58UhspEOe=GQMBqFiQ@mail.gmail.com> <etjzs45r7derztzgrb5b4tub4c72vobo6esc3ndksyke6kpiov@qj3ar7xwh6ee>
In-Reply-To: <etjzs45r7derztzgrb5b4tub4c72vobo6esc3ndksyke6kpiov@qj3ar7xwh6ee>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Thu, 13 Jun 2024 15:59:53 -0700
Message-ID: <CAJD7tkYCr1cUf7jeUPiXQk+bQBzYCB-yHTm-8Ns9SJJB5QgCpQ@mail.gmail.com>
Subject: Re: [PATCH v3 0/2] mm: store zero pages to be swapped out in a bitmap
To: Shakeel Butt <shakeel.butt@linux.dev>
Cc: Usama Arif <usamaarif642@gmail.com>, akpm@linux-foundation.org, hannes@cmpxchg.org, 
	david@redhat.com, ying.huang@intel.com, hughd@google.com, willy@infradead.org, 
	nphamcs@gmail.com, chengming.zhou@linux.dev, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org, kernel-team@meta.com, 
	Minchan Kim <minchan@kernel.org>, Sergey Senozhatsky <senozhatsky@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jun 13, 2024 at 3:41=E2=80=AFPM Shakeel Butt <shakeel.butt@linux.de=
v> wrote:
>
> On Thu, Jun 13, 2024 at 02:50:31PM GMT, Yosry Ahmed wrote:
> > On Mon, Jun 10, 2024 at 5:18=E2=80=AFAM Usama Arif <usamaarif642@gmail.=
com> wrote:
> > >
> > > Going back to the v1 implementation of the patchseries. The main reas=
on
> > > is that a correct version of v2 implementation requires another rmap
> > > walk in shrink_folio_list to change the ptes from swap entry to zero =
pages to
> > > work (i.e. more CPU used) [1], is more complex to implement compared =
to v1
> > > and is harder to verify correctness compared to v1, where everything =
is
> > > handled by swap.
> > >
> > > ---
> > > As shown in the patchseries that introduced the zswap same-filled
> > > optimization [2], 10-20% of the pages stored in zswap are same-filled=
.
> > > This is also observed across Meta's server fleet.
> > > By using VM counters in swap_writepage (not included in this
> > > patchseries) it was found that less than 1% of the same-filled
> > > pages to be swapped out are non-zero pages.
> > >
> > > For conventional swap setup (without zswap), rather than reading/writ=
ing
> > > these pages to flash resulting in increased I/O and flash wear, a bit=
map
> > > can be used to mark these pages as zero at write time, and the pages =
can
> > > be filled at read time if the bit corresponding to the page is set.
> > >
> > > When using zswap with swap, this also means that a zswap_entry does n=
ot
> > > need to be allocated for zero filled pages resulting in memory saving=
s
> > > which would offset the memory used for the bitmap.
> > >
> > > A similar attempt was made earlier in [3] where zswap would only trac=
k
> > > zero-filled pages instead of same-filled.
> > > This patchseries adds zero-filled pages optimization to swap
> > > (hence it can be used even if zswap is disabled) and removes the
> > > same-filled code from zswap (as only 1% of the same-filled pages are
> > > non-zero), simplifying code.
> >
> > There is also code to handle same-filled pages in zram, should we
> > remove this as well? It is worth noting that the handling in zram was
> > initially for zero-filled pages only, but it was extended to cover
> > same-filled pages as well by commit 8e19d540d107 ("zram: extend zero
> > pages to same element pages"). Apparently in a test on Android, about
> > 2.5% of the swapped out pages were non-zero same-filled pages.
> >
> > However, the leap from handling zero-filled pages to handling all
> > same-filled pages in zram wasn't a stretch. But now that zero-filled
> > pages handling in zram is redundant with this series, I wonder if it's
> > still worth keeping the same-filled pages handling.
>
> Please correct me if I am wrong but zram same-filled page handling is
> not just limited to swap-on-zram use-case and any zram as block device
> user can benefit from it. Also zram might not see any simplification
> similar to zswap in this patch series. I would say motivation behind
> zswap changes seems quite different from possible zram changes. I would
> recommed to evaluate these cases independently.

Uh yes. I keep forgetting that zram is used for other use cases than
swap. Please dismiss my comments then (unless it's uncommon to have
zero-filled / same-filled pages in other use cases).