Re: [PATCH v7 1/2] mm: store zero pages to be swapped out in a bitmap

[Usama Arif]

On 27/06/2024 19:18, Johannes Weiner wrote:
> Hi Usama,
>
> On Thu, Jun 27, 2024 at 11:55:29AM +0100, 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>
> > Reviewed-by: Chengming Zhou <chengming.zhou@xxxxxxxxx>
> > Reviewed-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
> > Reviewed-by: Nhat Pham <nphamcs@xxxxxxxxx>
> > Cc: David Hildenbrand <david@xxxxxxxxxx>
> > Cc: "Huang, Ying" <ying.huang@xxxxxxxxx>
> > Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> > Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
> > Cc: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> > Cc: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> > Cc: Usama Arif <usamaarif642@xxxxxxxxx>
> > Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> This looks great to me, and the numbers speak for themselves. A few
> minor comments below:
>
> > ---
> >   include/linux/swap.h |   1 +
> >   mm/page_io.c         | 113 ++++++++++++++++++++++++++++++++++++++++++-
> >   mm/swapfile.c        |  20 ++++++++
> >   3 files changed, 133 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > index 3df75d62a835..ed03d421febd 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 6c1c1828bb88..480b8f221d90 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;
> >   }
> >   
> It might be good to have a short comment that gives 1) an overview,
> that we're using a bitmap to avoid doing IO for zero-filled pages and
> 2) the locking, that the bits are protected by the locked swapcache
> folio and atomic updates are used to protect against RMW corruption
> due to other zero swap entries seeing concurrent updates.

Thanks! have addressed the comments and will include them in next 
revision. Just a couple of things.

Will add the overview in swap_writepage when the check is made if the 
folio is zero filled and zeromap bits are set, instead of at this point.

> > +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;
> > +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));
> > +}
> Should this be in highmem.h next to the other folio_zero_* functions?

Thanks for pointing to highmem.h. It already has folio_zero_range, which 
should do the same thing, so I think I can just do 
folio_zero_range(folio, 0, folio_size(folio)) and this function shouldnt 
be needed.