Re: [PATCH 03/10] mm/ksm: use folio in try_to_merge_one_page

[David Hildenbrand]

On 04.06.24 06:24, alexs@xxxxxxxxxx wrote:
> From: "Alex Shi (tencent)" <alexs@xxxxxxxxxx>
>
> scan_get_next_rmap_item() return folio actually now. So in the calling
> path to try_to_merge_one_page() parameter pages are actually folios.
> So let's use folio instead of of page in the function to save few
> compound checking in callee functions.
>
> The 'page' left here since flush functions still not support folios yet.
>
> Signed-off-by: Alex Shi (tencent) <alexs@xxxxxxxxxx>
> ---
>   mm/ksm.c | 61 ++++++++++++++++++++++++++++++++------------------------
>   1 file changed, 35 insertions(+), 26 deletions(-)
>
> diff --git a/mm/ksm.c b/mm/ksm.c
> index e2fdb9dd98e2..21bfa9bfb210 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -1462,24 +1462,29 @@ static int replace_page(struct vm_area_struct *vma, struct page *page,
>   }
>   
>   /*
> - * try_to_merge_one_page - take two pages and merge them into one
> - * @vma: the vma that holds the pte pointing to page
> - * @page: the PageAnon page that we want to replace with kpage
> - * @kpage: the PageKsm page that we want to map instead of page,
> - *         or NULL the first time when we want to use page as kpage.
> + * try_to_merge_one_page - take two folios and merge them into one
> + * @vma: the vma that holds the pte pointing to folio
> + * @folio: the PageAnon page that we want to replace with kfolio
> + * @kfolio: the PageKsm page that we want to map instead of folio,
> + *         or NULL the first time when we want to use folio as kfolio.
>    *
> - * This function returns 0 if the pages were merged, -EFAULT otherwise.
> + * This function returns 0 if the folios were merged, -EFAULT otherwise.
>    */
> -static int try_to_merge_one_page(struct vm_area_struct *vma, struct page *page,
> -				 struct ksm_rmap_item *rmap_item, struct page *kpage)
> +static int try_to_merge_one_page(struct vm_area_struct *vma, struct folio *folio,
> +				 struct ksm_rmap_item *rmap_item, struct folio *kfolio)
>   {
>   	pte_t orig_pte = __pte(0);
>   	int err = -EFAULT;
> +	struct page *page = folio_page(folio, 0);
> +	struct page *kpage;
>   
> -	if (page == kpage)			/* ksm page forked */
> +	if (kfolio)
> +		kpage = folio_page(kfolio, 0);
> +
> +	if (folio == kfolio)			/* ksm page forked */
>   		return 0;
>   
> -	if (!PageAnon(page))
> +	if (!folio_test_anon(folio))
>   		goto out;
>   
>   	/*
> @@ -1489,11 +1494,11 @@ static int try_to_merge_one_page(struct vm_area_struct *vma, struct page *page,
>   	 * prefer to continue scanning and merging different pages,
>   	 * then come back to this page when it is unlocked.
>   	 */
> -	if (!trylock_page(page))
> +	if (!folio_trylock(folio))
>   		goto out;
>   
> -	if (PageTransCompound(page)) {
> -		if (split_huge_page(page))
> +	if (folio_test_large(folio)) {
> +		if (split_folio(folio))
>   			goto out_unlock;
>   	}

Careful: there is a subtle change:

In the old code, after split_huge_page() succeeded, you would have a ref 
on *page* that you have to drop.

Now, you would have a ref on *folio* -- IOW the head page that calling 
code has to drop.

Is that handled accordingly? IOW, is there no code that would assume it 
would drop the reference on the *page* instead of on the *folio* (that, 
after split succeeds, would be an order-0 folio)

If so, worth spelling out in the description (you say "So in the calling 
path to try_to_merge_one_page() parameter pages are actually folios", 
but I am not sure if that means that all page refcounting code was 
changed to folio refcounting code).

--
Cheers,

David / dhildenb