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@xxxxxxxxxx>
LGTM with one comment below:
Reviewed-by: Chengming Zhou <chengming.zhou@xxxxxxxxx>
> ---
> 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)
