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

[Usama Arif <usamaarif642@gmail.com>]

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@gmail.com>
>> Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev>
>> Reviewed-by: Yosry Ahmed <yosryahmed@google.com>
>> Reviewed-by: Nhat Pham <nphamcs@gmail.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: "Huang, Ying" <ying.huang@intel.com>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
>> Cc: Shakeel Butt <shakeel.butt@linux.dev>
>> Cc: Usama Arif <usamaarif642@gmail.com>
>> Cc: Andi Kleen <ak@linux.intel.com>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 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.