Re: [RFC PATCH 4/9] mm: zswap: move more same-filled pages checks outside of zswap_store()

[Chengming Zhou <chengming.zhou@linux.dev>]

On 2024/3/26 07:50, Yosry Ahmed wrote:
> Currently, zswap_store() check zswap_same_filled_pages_enabled, kmaps
> the folio, then calls zswap_is_page_same_filled() to check the folio
> contents. Move this logic into zswap_is_page_same_filled() as well (and
> rename it to use 'folio' while we are at it).
> 
> This makes zswap_store() cleaner, and makes following changes to that
> logic contained within the helper.
> 
> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

LGTM with one comment below:

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

> ---
>  mm/zswap.c | 45 ++++++++++++++++++++++++---------------------
>  1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 6b890c8590ef7..498a6c5839bef 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1385,26 +1385,36 @@ static void shrink_worker(struct work_struct *w)
>  	} while (zswap_total_pages() > thr);
>  }
>  
> -static int zswap_is_page_same_filled(void *ptr, unsigned long *value)
> +static bool zswap_is_folio_same_filled(struct folio *folio, unsigned long *value)
>  {
>  	unsigned long *page;
>  	unsigned long val;
>  	unsigned int pos, last_pos = PAGE_SIZE / sizeof(*page) - 1;
> +	bool ret;
>  
> -	page = (unsigned long *)ptr;
> +	if (!zswap_same_filled_pages_enabled)
> +		return false;
> +
> +	page = kmap_local_folio(folio, 0);
>  	val = page[0];
>  
> -	if (val != page[last_pos])
> -		return 0;
> +	if (val != page[last_pos]) {
> +		ret = false;
> +		goto out;
> +	}
>  
>  	for (pos = 1; pos < last_pos; pos++) {
> -		if (val != page[pos])
> -			return 0;
> +		if (val != page[pos]) {
> +			ret = false;

nit: ret can be initialized to false, so

> +			goto out;
> +		}
>  	}
>  
>  	*value = val;
> -
> -	return 1;
> +	ret = true;

only need to set to true here.

Thanks.

> +out:
> +	kunmap_local(page);
> +	return ret;
>  }
>  
>  static void zswap_fill_page(void *ptr, unsigned long value)
> @@ -1438,6 +1448,7 @@ bool zswap_store(struct folio *folio)
>  	struct obj_cgroup *objcg = NULL;
>  	struct mem_cgroup *memcg = NULL;
>  	struct zswap_entry *entry;
> +	unsigned long value;
>  
>  	VM_WARN_ON_ONCE(!folio_test_locked(folio));
>  	VM_WARN_ON_ONCE(!folio_test_swapcache(folio));
> @@ -1470,19 +1481,11 @@ bool zswap_store(struct folio *folio)
>  		goto reject;
>  	}
>  
> -	if (zswap_same_filled_pages_enabled) {
> -		unsigned long value;
> -		u8 *src;
> -
> -		src = kmap_local_folio(folio, 0);
> -		if (zswap_is_page_same_filled(src, &value)) {
> -			kunmap_local(src);
> -			entry->length = 0;
> -			entry->value = value;
> -			atomic_inc(&zswap_same_filled_pages);
> -			goto insert_entry;
> -		}
> -		kunmap_local(src);
> +	if (zswap_is_folio_same_filled(folio, &value)) {
> +		entry->length = 0;
> +		entry->value = value;
> +		atomic_inc(&zswap_same_filled_pages);
> +		goto insert_entry;
>  	}
>  
>  	if (!zswap_non_same_filled_pages_enabled)