Received-SPF: pass (google.com: domain of linux-kernel+bounces-188291-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 24 May 2024 04:59:31 +0100
From: Matthew Wilcox <willy@infradead.org>
To: Yosry Ahmed <yosryahmed@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Johannes Weiner <hannes@cmpxchg.org>, Nhat Pham <nphamcs@gmail.com>,
	Chengming Zhou <chengming.zhou@linux.dev>, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/3] mm: zswap: trivial folio conversions
Message-ID: <ZlAQo0P4Z-dgVHn6@casper.infradead.org>
References: <20240524033819.1953587-1-yosryahmed@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240524033819.1953587-1-yosryahmed@google.com>

On Fri, May 24, 2024 at 03:38:15AM +0000, Yosry Ahmed wrote:
> Some trivial folio conversions in zswap code.

The three patches themselves look good.

> The mean reason I included a cover letter is that I wanted to get
> feedback on what other trivial conversions can/should be done in
> mm/zswap.c (keeping in mind that only order-0 folios are supported
> anyway).  These are the things I came across while searching for 'page'
> in mm/zswap.c, and chose not to do anything about for now:

I think there's a deeper question to answer before answering these
questions, which is what we intend to do with large folios and zswap in
the future.  Do we intend to split them?  Compress them as a large
folio?  Compress each page in a large folio separately?  I can see an
argument for choices 2 and 3, but I think choice 1 is going to be
increasingly untenable.

> 1. zswap_max_pages(), zswap_accept_thr_pages(), zswap_total_pages():
>   - We can use 'size' instead of 'pages' and shift the return by
>     PAGE_SHIFT. This adds an unnecessary shift, but I doubt it matters
>     at all. The motivation is to get rid of 'page' to find things that
>     should be converted more easily.
> 
> 2. Counters names: zswap_stored_pages, zswap_written_back_pages, etc.
> 
> 3. Comments all over the place reference 'page' instead of 'folio'.
> 
> 4. shrink_memcg_cb(), zswap_shrinker_scan():
>   - Rename encountered_page_in_swap_cache to
>     encounterd_folio_in_swap_cache, or even better: folio_eexist or
>     hit_swap_cache.
> 
> 5. entry_to_nid():
>   - It's tempting to try to use folio_to_nid(virt_to_folio()), but I
>     think this adds an unnecessary call to compound_head(). It may not
>     matter in practice though because the page is always a head page.
> 
> Yosry Ahmed (3):
>   mm: zswap: use sg_set_folio() in zswap_{compress/decompress}()
>   mm :zswap: use kmap_local_folio() in zswap_load()
>   mm: zswap: make same_filled functions folio-friendly
> 
>  mm/zswap.c | 41 ++++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
> 
> -- 
> 2.45.1.288.g0e0cd299f1-goog
> 
>