On 2024/6/14 18:07, Usama Arif 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@xxxxxxxxx> Looks good to me, only some small nits below. Reviewed-by: Chengming Zhou <chengming.zhou@xxxxxxxxx> > --- > include/linux/swap.h | 1 + > mm/page_io.c | 113 ++++++++++++++++++++++++++++++++++++++++++- > mm/swapfile.c | 15 ++++++ > 3 files changed, 128 insertions(+), 1 deletion(-) > [...] > + > +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)); It seems simpler to use: swp_entry_t entry = folio->swap; for (i = 0; i < folio_nr_pages(folio); i++, entry.val++) The current code is good too, no objection. > + set_bit(swp_offset(entry), sis->zeromap); > + } > +} > + [...] > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 9c6d8e557c0f..0b8270359bcf 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -747,6 +747,14 @@ static void swap_range_free(struct swap_info_struct *si, unsigned long offset, > unsigned long begin = offset; > unsigned long end = offset + nr_entries - 1; > void (*swap_slot_free_notify)(struct block_device *, unsigned long); > + unsigned int i; > + > + /* > + * Use atomic clear_bit operations only on zeromap instead of non-atomic > + * bitmap_clear to prevent adjacent bits corruption due to simultaneous writes. > + */ > + for (i = 0; i < nr_entries; i++) > + clear_bit(offset + i, si->zeromap); I'm wondering if we need to clear bits at all? Since the current locked folio is the owner of these bits, we always update correctly when swap_writepage(). So if these swap entries freed and reused by another folio, we won't load from backend until that another folio has gone swap_writepage(), which update these bits correctly. Maybe I missed something? Anyway, it should be no harm to clear here too. Thanks. > > if (offset < si->lowest_bit) > si->lowest_bit = offset; > @@ -2635,6 +2643,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); > @@ -3161,6 +3170,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; >
