Received-SPF: pass (google.com: domain of linux-kernel+bounces-214862-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <b5952ee4-3b86-40ff-a3de-8a08f09557bd@linux.dev>
Date: Fri, 14 Jun 2024 20:05:48 +0800
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH v5 1/2] mm: store zero pages to be swapped out in a bitmap
Content-Language: en-US
To: Usama Arif <usamaarif642@gmail.com>, akpm@linux-foundation.org
Cc: hannes@cmpxchg.org, shakeel.butt@linux.dev, david@redhat.com,
 ying.huang@intel.com, hughd@google.com, willy@infradead.org,
 yosryahmed@google.com, nphamcs@gmail.com, linux-mm@kvack.org,
 linux-kernel@vger.kernel.org, kernel-team@meta.com
References: <20240614100902.3469724-1-usamaarif642@gmail.com>
 <20240614100902.3469724-2-usamaarif642@gmail.com>
From: Chengming Zhou <chengming.zhou@linux.dev>
In-Reply-To: <20240614100902.3469724-2-usamaarif642@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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@gmail.com>

Looks good to me, only some small nits below.

Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev>

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