| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20240904055522.2376-1-21cnbao@gmail.com>
Date: Wed, 4 Sep 2024 17:55:22 +1200
From: Barry Song <21cnbao@...il.com>
To: usamaarif642@...il.com
Cc: akpm@...ux-foundation.org,
chengming.zhou@...ux.dev,
david@...hat.com,
hannes@...xchg.org,
hughd@...gle.com,
kernel-team@...a.com,
linux-kernel@...r.kernel.org,
linux-mm@...ck.org,
nphamcs@...il.com,
shakeel.butt@...ux.dev,
willy@...radead.org,
ying.huang@...el.com,
yosryahmed@...gle.com,
hanchuanhua@...o.com
Subject: Re: [PATCH v4 1/2] mm: store zero pages to be swapped out in a bitmap
On Thu, Jun 13, 2024 at 12:48 AM Usama Arif <usamaarif642@...il.com> wrote:
>
> Approximately 10-20% of pages to be swapped out are zero pages [1].
> 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.
> With this patch, NVMe writes in Meta server fleet decreased
> by almost 10% with conventional swap setup (zswap disabled).
>
> [1] https://lore.kernel.org/all/20171018104832epcms5p1b2232e2236258de3d03d1344dde9fce0@epcms5p1/
>
> Signed-off-by: Usama Arif <usamaarif642@...il.com>
> ---
> include/linux/swap.h | 1 +
> mm/page_io.c | 114 ++++++++++++++++++++++++++++++++++++++++++-
> mm/swapfile.c | 24 ++++++++-
> 3 files changed, 136 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index a11c75e897ec..e88563978441 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -299,6 +299,7 @@ struct swap_info_struct {
> signed char type; /* strange name for an index */
> unsigned int max; /* extent of the swap_map */
> unsigned char *swap_map; /* vmalloc'ed array of usage counts */
> + unsigned long *zeromap; /* vmalloc'ed bitmap to track zero pages */
> struct swap_cluster_info *cluster_info; /* cluster info. Only for SSD */
> struct swap_cluster_list free_clusters; /* free clusters list */
> unsigned int lowest_bit; /* index of first free in swap_map */
> diff --git a/mm/page_io.c b/mm/page_io.c
> index a360857cf75d..39fc3919ce15 100644
> --- a/mm/page_io.c
> +++ b/mm/page_io.c
> @@ -172,6 +172,88 @@ int generic_swapfile_activate(struct swap_info_struct *sis,
> goto out;
> }
>
> +static bool is_folio_page_zero_filled(struct folio *folio, int i)
> +{
> + unsigned long *data;
> + unsigned int pos, last_pos = PAGE_SIZE / sizeof(*data) - 1;
> + bool ret = false;
> +
> + data = kmap_local_folio(folio, i * PAGE_SIZE);
> + if (data[last_pos])
> + goto out;
> + for (pos = 0; pos < PAGE_SIZE / sizeof(*data); pos++) {
> + if (data[pos])
> + goto out;
> + }
> + ret = true;
> +out:
> + kunmap_local(data);
> + return ret;
> +}
> +
> +static bool is_folio_zero_filled(struct folio *folio)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < folio_nr_pages(folio); i++) {
> + if (!is_folio_page_zero_filled(folio, i))
> + return false;
> + }
> + return true;
> +}
> +
> +static void folio_zero_fill(struct folio *folio)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < folio_nr_pages(folio); i++)
> + clear_highpage(folio_page(folio, i));
> +}
> +
> +static void swap_zeromap_folio_set(struct folio *folio)
> +{
> + struct swap_info_struct *sis = swp_swap_info(folio->swap);
> + swp_entry_t entry;
> + unsigned int i;
> +
> + for (i = 0; i < folio_nr_pages(folio); i++) {
> + entry = page_swap_entry(folio_page(folio, i));
> + set_bit(swp_offset(entry), sis->zeromap);
> + }
> +}
> +
> +static void swap_zeromap_folio_clear(struct folio *folio)
> +{
> + struct swap_info_struct *sis = swp_swap_info(folio->swap);
> + swp_entry_t entry;
> + unsigned int i;
> +
> + for (i = 0; i < folio_nr_pages(folio); i++) {
> + entry = page_swap_entry(folio_page(folio, i));
> + clear_bit(swp_offset(entry), sis->zeromap);
> + }
> +}
> +
> +/*
> + * Return the index of the first subpage which is not zero-filled
> + * according to swap_info_struct->zeromap.
> + * If all pages are zero-filled according to zeromap, it will return
> + * folio_nr_pages(folio).
> + */
> +static unsigned int swap_zeromap_folio_test(struct folio *folio)
> +{
> + struct swap_info_struct *sis = swp_swap_info(folio->swap);
> + swp_entry_t entry;
> + unsigned int i;
> +
> + for (i = 0; i < folio_nr_pages(folio); i++) {
> + entry = page_swap_entry(folio_page(folio, i));
> + if (!test_bit(swp_offset(entry), sis->zeromap))
> + return i;
> + }
> + return i;
> +}
> +
> /*
> * We may have stale swap cache pages in memory: notice
> * them here and get rid of the unnecessary final write.
> @@ -195,6 +277,13 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
> folio_unlock(folio);
> return ret;
> }
> +
> + if (is_folio_zero_filled(folio)) {
> + swap_zeromap_folio_set(folio);
> + folio_unlock(folio);
> + return 0;
> + }
> + swap_zeromap_folio_clear(folio);
> if (zswap_store(folio)) {
> folio_start_writeback(folio);
> folio_unlock(folio);
> @@ -426,6 +515,26 @@ static void sio_read_complete(struct kiocb *iocb, long ret)
> mempool_free(sio, sio_pool);
> }
>
> +static bool swap_read_folio_zeromap(struct folio *folio)
> +{
> + unsigned int idx = swap_zeromap_folio_test(folio);
> +
> + if (idx == 0)
> + return false;
> +
> + /*
> + * Swapping in a large folio that is partially in the zeromap is not
> + * currently handled. Return true without marking the folio uptodate so
> + * that an IO error is emitted (e.g. do_swap_page() will sigbus).
> + */
> + if (WARN_ON_ONCE(idx < folio_nr_pages(folio)))
> + return true;
Hi Usama, Yosry,
I feel the warning is wrong as we could have the case where idx==0
is not zeromap but idx=1 is zeromap. idx == 0 doesn't necessarily
mean we should return false.
What about the below change which both fixes the warning and unblocks
large folios swap-in?
diff --git a/mm/page_io.c b/mm/page_io.c
index 4bc77d1c6bfa..7d7ff7064e2b 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -226,26 +226,6 @@ static void swap_zeromap_folio_clear(struct folio *folio)
}
}
-/*
- * Return the index of the first subpage which is not zero-filled
- * according to swap_info_struct->zeromap.
- * If all pages are zero-filled according to zeromap, it will return
- * folio_nr_pages(folio).
- */
-static unsigned int swap_zeromap_folio_test(struct folio *folio)
-{
- struct swap_info_struct *sis = swp_swap_info(folio->swap);
- swp_entry_t entry;
- unsigned int i;
-
- for (i = 0; i < folio_nr_pages(folio); i++) {
- entry = page_swap_entry(folio_page(folio, i));
- if (!test_bit(swp_offset(entry), sis->zeromap))
- return i;
- }
- return i;
-}
-
/*
* We may have stale swap cache pages in memory: notice
* them here and get rid of the unnecessary final write.
@@ -524,9 +504,10 @@ static void sio_read_complete(struct kiocb *iocb, long ret)
static bool swap_read_folio_zeromap(struct folio *folio)
{
- unsigned int idx = swap_zeromap_folio_test(folio);
+ unsigned int nr_pages = folio_nr_pages(folio);
+ unsigned int nr = swap_zeromap_entries_count(folio->swap, nr_pages);
- if (idx == 0)
+ if (nr == 0)
return false;
/*
@@ -534,7 +515,7 @@ static bool swap_read_folio_zeromap(struct folio *folio)
* currently handled. Return true without marking the folio uptodate so
* that an IO error is emitted (e.g. do_swap_page() will sigbus).
*/
- if (WARN_ON_ONCE(idx < folio_nr_pages(folio)))
+ if (WARN_ON_ONCE(nr < nr_pages))
return true;
folio_zero_range(folio, 0, folio_size(folio));
diff --git a/mm/swap.h b/mm/swap.h
index f8711ff82f84..2d59e9d89e95 100644
--- a/mm/swap.h
+++ b/mm/swap.h
@@ -80,6 +80,32 @@ static inline unsigned int folio_swap_flags(struct folio *folio)
{
return swp_swap_info(folio->swap)->flags;
}
+
+/*
+ * Return the number of entries which are zero-filled according to
+ * swap_info_struct->zeromap. It isn't precise if the return value
+ * is larger than 0 and smaller than nr to avoid extra iterations,
+ * In this case, it means entries haven't consistent zeromap.
+ */
+static inline unsigned int swap_zeromap_entries_count(swp_entry_t entry, int nr)
+{
+ struct swap_info_struct *sis = swp_swap_info(entry);
+ unsigned long offset = swp_offset(entry);
+ unsigned int type = swp_type(entry);
+ unsigned int n = 0;
+
+ for (int i = 0; i < nr; i++) {
+ entry = swp_entry(type, offset + i);
+ if (test_bit(offset + i, sis->zeromap)) {
+ if (i != n)
+ return i;
+ n++;
+ }
+ }
+
+ return n;
+}
+
#else /* CONFIG_SWAP */
struct swap_iocb;
static inline void swap_read_folio(struct folio *folio, struct swap_iocb **plug)
@@ -171,6 +197,11 @@ static inline unsigned int folio_swap_flags(struct folio *folio)
{
return 0;
}
+
+static inline unsigned int swap_zeromap_entries_count(swp_entry_t entry, int nr)
+{
+ return 0;
+}
#endif /* CONFIG_SWAP */
#endif /* _MM_SWAP_H */
> +
> + folio_zero_fill(folio);
> + folio_mark_uptodate(folio);
> + return true;
> +}
> +
> static void swap_read_folio_fs(struct folio *folio, struct swap_iocb **plug)
> {
> struct swap_info_struct *sis = swp_swap_info(folio->swap);
> @@ -515,8 +624,9 @@ void swap_read_folio(struct folio *folio, bool synchronous,
> psi_memstall_enter(&pflags);
> }
> delayacct_swapin_start();
> -
> - if (zswap_load(folio)) {
> + if (swap_read_folio_zeromap(folio)) {
> + folio_unlock(folio);
> + } else if (zswap_load(folio)) {
> folio_mark_uptodate(folio);
> folio_unlock(folio);
> } else if (data_race(sis->flags & SWP_FS_OPS)) {
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index f1e559e216bd..48d8dca0b94b 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -453,6 +453,8 @@ static unsigned int cluster_list_del_first(struct swap_cluster_list *list,
> static void swap_cluster_schedule_discard(struct swap_info_struct *si,
> unsigned int idx)
> {
> + unsigned int i;
> +
> /*
> * If scan_swap_map_slots() can't find a free cluster, it will check
> * si->swap_map directly. To make sure the discarding cluster isn't
> @@ -461,6 +463,13 @@ static void swap_cluster_schedule_discard(struct swap_info_struct *si,
> */
> memset(si->swap_map + idx * SWAPFILE_CLUSTER,
> SWAP_MAP_BAD, SWAPFILE_CLUSTER);
> + /*
> + * zeromap can see updates from concurrent swap_writepage() and swap_read_folio()
> + * call on other slots, hence use atomic clear_bit for zeromap instead of the
> + * non-atomic bitmap_clear.
> + */
> + for (i = 0; i < SWAPFILE_CLUSTER; i++)
> + clear_bit(idx * SWAPFILE_CLUSTER + i, si->zeromap);
>
> cluster_list_add_tail(&si->discard_clusters, si->cluster_info, idx);
>
> @@ -482,7 +491,7 @@ static void __free_cluster(struct swap_info_struct *si, unsigned long idx)
> static void swap_do_scheduled_discard(struct swap_info_struct *si)
> {
> struct swap_cluster_info *info, *ci;
> - unsigned int idx;
> + unsigned int idx, i;
>
> info = si->cluster_info;
>
> @@ -498,6 +507,8 @@ static void swap_do_scheduled_discard(struct swap_info_struct *si)
> __free_cluster(si, idx);
> memset(si->swap_map + idx * SWAPFILE_CLUSTER,
> 0, SWAPFILE_CLUSTER);
> + for (i = 0; i < SWAPFILE_CLUSTER; i++)
> + clear_bit(idx * SWAPFILE_CLUSTER + i, si->zeromap);
> unlock_cluster(ci);
> }
> }
> @@ -1059,9 +1070,12 @@ static void swap_free_cluster(struct swap_info_struct *si, unsigned long idx)
> {
> unsigned long offset = idx * SWAPFILE_CLUSTER;
> struct swap_cluster_info *ci;
> + unsigned int i;
>
> ci = lock_cluster(si, offset);
> memset(si->swap_map + offset, 0, SWAPFILE_CLUSTER);
> + for (i = 0; i < SWAPFILE_CLUSTER; i++)
> + clear_bit(offset + i, si->zeromap);
> cluster_set_count_flag(ci, 0, 0);
> free_cluster(si, idx);
> unlock_cluster(ci);
> @@ -1336,6 +1350,7 @@ static void swap_entry_free(struct swap_info_struct *p, swp_entry_t entry)
> count = p->swap_map[offset];
> VM_BUG_ON(count != SWAP_HAS_CACHE);
> p->swap_map[offset] = 0;
> + clear_bit(offset, p->zeromap);
> dec_cluster_info_page(p, p->cluster_info, offset);
> unlock_cluster(ci);
>
> @@ -2597,6 +2612,7 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
> free_percpu(p->cluster_next_cpu);
> p->cluster_next_cpu = NULL;
> vfree(swap_map);
> + bitmap_free(p->zeromap);
> kvfree(cluster_info);
> /* Destroy swap account information */
> swap_cgroup_swapoff(p->type);
> @@ -3123,6 +3139,12 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> goto bad_swap_unlock_inode;
> }
>
> + p->zeromap = bitmap_zalloc(maxpages, GFP_KERNEL);
> + if (!p->zeromap) {
> + error = -ENOMEM;
> + goto bad_swap_unlock_inode;
> + }
> +
> if (p->bdev && bdev_stable_writes(p->bdev))
> p->flags |= SWP_STABLE_WRITES;
>
> --
> 2.43.0
>
>
Powered by blists - more mailing lists