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

From: David Hildenbrand
Date: Tue Jun 04 2024 - 04:19:54 EST


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