Re: [PATCH 01/11] mm/ksm: Convert get_ksm_page to return a folio

[Alex Shi <seakeel@gmail.com>]

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!