Received-SPF: pass (google.com: domain of linux-kernel+bounces-109587-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <1499f4f3-db06-4330-b2d6-fde57a3fbe04@gmail.com>
Date: Thu, 21 Mar 2024 10:07:58 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 01/11] mm/ksm: Convert get_ksm_page to return a folio
Content-Language: en-US
To: Matthew Wilcox <willy@infradead.org>, 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>
References: <20240320074049.4130552-1-alexs@kernel.org>
 <20240320074049.4130552-2-alexs@kernel.org>
 <ZfrcihNz74lDDg1J@casper.infradead.org>
From: Alex Shi <seakeel@gmail.com>
In-Reply-To: <ZfrcihNz74lDDg1J@casper.infradead.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 3/20/24 8:54 PM, Matthew Wilcox wrote:
> 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?
> 

Thanks for comment! I will take your suggestion and resend soon. 

Best regards!