Date: Thu, 27 Jun 2024 12:18:52 -0400
From: Johannes Weiner <hannes@...xchg.org>
To: Usama Arif <usamaarif642@...il.com>
Cc: akpm@...ux-foundation.org, shakeel.butt@...ux.dev, david@...hat.com,
	ying.huang@...el.com, hughd@...gle.com, willy@...radead.org,
	yosryahmed@...gle.com, nphamcs@...il.com, chengming.zhou@...ux.dev,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	kernel-team@...a.com, Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH v7 1/2] mm: store zero pages to be swapped out in a bitmap

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

> +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;

This shortcut is interesting. It's what zswap does, for which git
blame finds a commit with test results:

commit 62bf1258ec90554c3c0925951149cfe4b67a3e98
Author: Taejoon Song <taejoon.song@....com>
Date:   Mon Feb 6 04:00:36 2023 +0900

    mm/zswap: try to avoid worst-case scenario on same element pages
    
    The worst-case scenario on finding same element pages is that almost all
    elements are same at the first glance but only last few elements are
    different.
    
    Since the same element tends to be grouped from the beginning of the
    pages, if we check the first element with the last element before looping
    through all elements, we might have some chances to quickly detect
    non-same element pages.
    
    1. Test is done under LG webOS TV (64-bit arch)
    2. Dump the swap-out pages (~819200 pages)
    3. Analyze the pages with simple test script which counts the iteration
       number and measures the speed at off-line
    
    Under 64-bit arch, the worst iteration count is PAGE_SIZE / 8 bytes = 512.
    The speed is based on the time to consume page_same_filled() function
    only.  The result, on average, is listed as below:
    
                                       Num of Iter    Speed(MB/s)
    Looping-Forward (Orig)                 38            99265
    Looping-Backward                       36           102725
    Last-element-check (This Patch)        33           125072
    
    The result shows that the average iteration count decreases by 13% and the
    speed increases by 25% with this patch.  This patch does not increase the
    overall time complexity, though.
    
    I also ran simpler version which uses backward loop.  Just looping
    backward also makes some improvement, but less than this patch.
    
    A similar change has already been made to zram in 90f82cbfe502 ("zram: try
    to avoid worst-case scenario on same element pages").

Since you add it here, and then delete the zswap code in the next
patch, this link to the justification will be lost.

Can you please add a short comment that explains that this is a
measure to avoid worst-case behavior for tail-filled pages, which are
common in real life workloads?

> +	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;
> +}

I think these two functions can be merged. It's less code and fewer
similar-sounding names in the namespace.

> +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?

> +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);

Just for clarity, it would be good to put this into an else branch and
add a comment about overwrites due to changes in the in-memory data.

Losing this would cause nasty data corruption, so let's make sure the
reason for why it's there is spelled out, and stands out.

>  	if (zswap_store(folio)) {
>  		folio_unlock(folio);
>  		return 0;

> @@ -3161,6 +3170,17 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>  		goto bad_swap_unlock_inode;
>  	}
>  
> +	/*
> +	 * Use kvmalloc_array instead of bitmap_zalloc as the allocation order might
> +	 * be above MAX_PAGE_ORDER incase of a large swap file.
> +	 */
> +	p->zeromap = kvmalloc_array(BITS_TO_LONGS(maxpages), sizeof(unsigned long),
> +				    GFP_KERNEL | __GFP_ZERO);

sizeof(long) would get this below 80 characters ;)

> +	if (!p->zeromap) {
> +		error = -ENOMEM;
> +		goto bad_swap_unlock_inode;
> +	}
> +

With that,

Acked-by: Johannes Weiner <hannes@...xchg.org>

Powered by blists - more mailing lists