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

David Hildenbrand
2024-06-04T08:19:46Z

On 04.06.24 06:24, alexs@kernel.org wrote:
> From: "Alex Shi (tencent)" <alexs@kernel.org>
>
> 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@kernel.org>
> ---
> 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