On 14.11.23 13:36, yang.yang29@xxxxxxxxxx wrote:
From: xu xin <xu.xin16@xxxxxxxxxx>
Background
==========
When trying to merge two pages, it may fail because the two pages
belongs to the same compound page and split_huge_page fails due to
the incorrect reference to the page. To solve the problem, the commit
77da2ba0648a4 ("mm/ksm: fix interaction with THP") tries to split the
compound page after try_to_merge_two_pages() fails and put_page in
that case. However it is too early to calculate of the variable 'split' which
indicates whether the two pages belongs to the same compound page.
What to do
==========
If try_to_merge_two_pages() succeeds, there is no need to check whether
to splitting compound pages. So we delay the check of splitting compound
pages until try_to_merge_two_pages() fails, which can improve the
processing efficiency of cmp_and_merge_page() a little.
Signed-off-by: xu xin <xu.xin16@xxxxxxxxxx>
Reviewed-by: Yang Yang <yang.yang29@xxxxxxxxxx>
---
mm/ksm.c | 36 ++++++++++++++++++++----------------
1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/mm/ksm.c b/mm/ksm.c
index 7efcc68ccc6e..c952fe5d9e43 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2229,24 +2229,10 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
tree_rmap_item =
unstable_tree_search_insert(rmap_item, page, &tree_page);
if (tree_rmap_item) {
- bool split;
-
kpage = try_to_merge_two_pages(rmap_item, page,
tree_rmap_item, tree_page);
- /*
- * If both pages we tried to merge belong to the same compound
- * page, then we actually ended up increasing the reference
- * count of the same compound page twice, and split_huge_page
- * failed.
- * Here we set a flag if that happened, and we use it later to
- * try split_huge_page again. Since we call put_page right
- * afterwards, the reference count will be correct and
- * split_huge_page should succeed.
- */
I'm curious, why can't we detect that ahead of time and keep only a
single reference? Why do we need the backup code? Anything I am missing?
- split = PageTransCompound(page)
- && compound_head(page) == compound_head(tree_page);
- put_page(tree_page);
if (kpage) {
+ put_page(tree_page);
/*
* The pages were successfully merged: insert new
* node in the stable tree and add both rmap_items.
@@ -2271,7 +2257,25 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
break_cow(tree_rmap_item);
break_cow(rmap_item);
}
- } else if (split) {
+ } else {
+ bool split;
+ /*
+ * If both pages we tried to merge belong to the same compound
+ * page, then we actually ended up increasing the reference
+ * count of the same compound page twice, and split_huge_page
+ * failed.
+ * Here we set a flag if that happened, and we use it later to
+ * try split_huge_page again. Since we call put_page right
+ * afterwards, the reference count will be correct and
+ * split_huge_page should succeed.
+ */
+
+ split = PageTransCompound(page)
+ && compound_head(page) == compound_head(tree_page);
Would
split = page_folio(page) == page_folio(tree_page);
do the trick? No need to mess with compound pages.
--
Cheers,
David / dhildenb
