Received-SPF: pass (google.com: domain of linux-kernel+bounces-214040-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240610121820.328876-1-usamaarif642@gmail.com>
In-Reply-To: <20240610121820.328876-1-usamaarif642@gmail.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Thu, 13 Jun 2024 14:50:31 -0700
Message-ID: <CAJD7tkZdx7xJiDcvayH1aRW9Q6GQaZnF58UhspEOe=GQMBqFiQ@mail.gmail.com>
Subject: Re: [PATCH v3 0/2] mm: store zero pages to be swapped out in a bitmap
To: Usama Arif <usamaarif642@gmail.com>
Cc: 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 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 reason
> 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 page=
s to
> work (i.e. more CPU used) [1], is more complex to implement compared to v=
1
> 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/writing
> these pages to flash resulting in increased I/O and flash wear, a bitmap
> 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 not
> need to be allocated for zero filled pages resulting in memory savings
> which would offset the memory used for the bitmap.
>
> A similar attempt was made earlier in [3] where zswap would only track
> 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.

Adding Minchan and Sergey here.

>
> This patchseries is based on mm-unstable.
>
>
> [1] https://lore.kernel.org/all/e4d167fe-cb1e-41d1-a144-00bfa14b7148@gmai=
l.com/
> [2] https://lore.kernel.org/all/20171018104832epcms5p1b2232e2236258de3d03=
d1344dde9fce0@epcms5p1/
> [3] https://lore.kernel.org/lkml/20240325235018.2028408-1-yosryahmed@goog=
le.com/
>
> ---
> v2->v3:
> - Going back to the v1 version of the implementation (David and Shakeel)
> - convert unatomic bitmap_set/clear to atomic set/clear_bit (Johannes)
> - use clear_highpage instead of folio_page_zero_fill (Yosry)
>
> v1 -> v2:
> - instead of using a bitmap in swap, clear pte for zero pages and let
>   do_pte_missing handle this page at page fault. (Yosry and Matthew)
> - Check end of page first when checking if folio is zero filled as
>   it could lead to better performance. (Yosry)
>
> Usama Arif (2):
>   mm: store zero pages to be swapped out in a bitmap
>   mm: remove code to handle same filled pages
>
>  include/linux/swap.h |  1 +
>  mm/page_io.c         | 92 +++++++++++++++++++++++++++++++++++++++++++-
>  mm/swapfile.c        | 21 +++++++++-
>  mm/zswap.c           | 86 ++++-------------------------------------
>  4 files changed, 119 insertions(+), 81 deletions(-)
>
> --
> 2.43.0
>