On 3/20/24 8:54 PM, Matthew Wilcox wrote:
> On Wed, Mar 20, 2024 at 03:40:37PM +0800, alexs@xxxxxxxxxx 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!
