Received-SPF: pass (google.com: domain of linux-kernel+bounces-120233-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <fef4556f-5d22-4941-9990-a4a3cf003dc4@linux.dev>
Date: Wed, 27 Mar 2024 10:39:06 +0800
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [RFC PATCH 4/9] mm: zswap: move more same-filled pages checks
 outside of zswap_store()
Content-Language: en-US
To: Yosry Ahmed <yosryahmed@google.com>,
 Andrew Morton <akpm@linux-foundation.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>, Nhat Pham <nphamcs@gmail.com>,
 linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20240325235018.2028408-1-yosryahmed@google.com>
 <20240325235018.2028408-5-yosryahmed@google.com>
From: Chengming Zhou <chengming.zhou@linux.dev>
In-Reply-To: <20240325235018.2028408-5-yosryahmed@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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)