Received-SPF: pass (google.com: domain of linux-kernel+bounces-108919-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 20 Mar 2024 12:54:34 +0000
From: Matthew Wilcox <willy@infradead.org>
To: alexs@kernel.org
Cc: Izik Eidus <izik.eidus@ravellosystems.com>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Hugh Dickins <hughd@google.com>, Chris Wright <chrisw@sous-sol.org>,
	kasong@tencent.com, Andrew Morton <akpm@linux-foundation.org>,
	"open list:MEMORY MANAGEMENT" <linux-mm@kvack.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 01/11] mm/ksm: Convert get_ksm_page to return a folio
Message-ID: <ZfrcihNz74lDDg1J@casper.infradead.org>
References: <20240320074049.4130552-1-alexs@kernel.org>
 <20240320074049.4130552-2-alexs@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240320074049.4130552-2-alexs@kernel.org>

On Wed, Mar 20, 2024 at 03:40:37PM +0800, alexs@kernel.org wrote:
> -static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
> +static void *get_ksm_page(struct ksm_stable_node *stable_node,
>  				 enum get_ksm_page_flags flags)

I am really not a fan of returning void * instead of a page or a
folio.  Particularly since you rename this function at the end anyway!

You should do it like this:

In this patch, convert get_ksm_page() to get_ksm_folio() and add:

static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
		enum get_ksm_page_flags flags)
{
	struct folio *folio = get_ksm_folio(node, flags);
	return &folio->page;
}

Then convert each call-site to get_ksm_folio(), and finally delete
get_ksm_page().  That way you're always converting each caller to
the exact code you want it to look like, and your reiewrs don't have to
keep three patches in their head at once as they review each place.

Also, I think this should be ksm_get_folio(), not get_ksm_folio().
Seems to fit better.

> @@ -949,32 +949,32 @@ static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
>  		 * in the ref_freeze section of __remove_mapping(); but Anon
>  		 * page->mapping reset to NULL later, in free_pages_prepare().

Could you fix page->mapping to folio->mapping in the comment?