[PATCH] ksm: delay the check of splitting compound pages

[PATCH] ksm: delay the check of splitting compound pages

[yang.yang29]

From: xu xin <xu.xin16@zte.com.cn>

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@zte.com.cn>
Reviewed-by: Yang Yang <yang.yang29@zte.com.cn>
---
 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.
-		 */
-		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);
+			put_page(tree_page);
+
+			if (!split)
+				return;
 			/*
 			 * We are here if we tried to merge two pages and
 			 * failed because they both belonged to the same
-- 
2.15.2

Re: [PATCH] ksm: delay the check of splitting compound pages

[David Hildenbrand]

On 14.11.23 13:36, yang.yang29@zte.com.cn wrote:
> From: xu xin <xu.xin16@zte.com.cn>
> 
> 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@zte.com.cn>
> Reviewed-by: Yang Yang <yang.yang29@zte.com.cn>

Can we please add a unit test to ksm_functional_tests.c so we actually 
get it right this time?

-- 
Cheers,

David / dhildenb

Re: [PATCH] ksm: delay the check of splitting compound pages

[David Hildenbrand]

On 14.11.23 13:36, yang.yang29@zte.com.cn wrote:
> From: xu xin <xu.xin16@zte.com.cn>
> 
> 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@zte.com.cn>
> Reviewed-by: Yang Yang <yang.yang29@zte.com.cn>
> ---
>   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

Re: [PATCH] ksm: delay the check of splitting compound pages

[xu]

>> 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?

I don't know the original reason, better ask Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>. 
Maybe because doing detection that ahead of time will break several funtions' semantic,
such as try_to_merge_two_pages(), try_to_merge_with_ksm_page() and try_to_merge_one_page()

Adding the backup code don't change the old code and fixing the old problem, it's good.

>
>> -		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.

In terms of function correctness, it should work correctly because here 'page' and 'tree_page' are never
the same page, which is guaranteed by unstable_tree_search_insert(). But it's not very intuitive, maybe
ww need to add some comment.

Re: [PATCH] ksm: delay the check of splitting compound pages

[xu]

>> From: xu xin <xu.xin16@zte.com.cn>
>> 
>> 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@zte.com.cn>
>> Reviewed-by: Yang Yang <yang.yang29@zte.com.cn>
>
>Can we please add a unit test to ksm_functional_tests.c so we actually 
>get it right this time?

Sure. Maybe we can simply refer to the reproducing way Claudio proposes in
77da2ba0648a4 ("mm/ksm: fix interaction with THP").

>-- 
>Cheers,
>
>David / dhildenb

Re: [PATCH] ksm: delay the check of splitting compound pages

[David Hildenbrand]

On 15.11.23 04:11, xu wrote:
>>> 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?
> 
> I don't know the original reason, better ask Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>.
> Maybe because doing detection that ahead of time will break several funtions' semantic,
> such as try_to_merge_two_pages(), try_to_merge_with_ksm_page() and try_to_merge_one_page()
> 
> Adding the backup code don't change the old code and fixing the old problem, it's good.

It's absolutely counter-intuitive to check for something that cannot 
possibly work after the effects. This better has a good reason to make 
that code more complicated.
-- 
Cheers,

David / dhildenb

Re: [PATCH] ksm: delay the check of splitting compound pages

[David Hildenbrand]

On 15.11.23 04:15, xu wrote:
>>> From: xu xin <xu.xin16@zte.com.cn>
>>>
>>> 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@zte.com.cn>
>>> Reviewed-by: Yang Yang <yang.yang29@zte.com.cn>
>>
>> Can we please add a unit test to ksm_functional_tests.c so we actually
>> get it right this time?
> 
> Sure. Maybe we can simply refer to the reproducing way Claudio proposes in
> 77da2ba0648a4 ("mm/ksm: fix interaction with THP").

So, was Claudio able to verify that his fix was correct, and how come we 
figure out 5 years later that that fix is insufficient?

Could it not have possibly worked, has something else changed in the 
meantime, or what's the deal here? Further elaborating on that in the 
patch description and adding a proper Fixes: tag will be appreciated ;)

-- 
Cheers,

David / dhildenb

Re: [PATCH] ksm: delay the check of splitting compound pages

[xu]

>>>> @@ -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?

Do you mean like this?

--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2229,23 +2229,21 @@ 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;
+               bool SameCompound;
+               /*
+                * If they belongs to the same compound page, its' reference
+                * get twice, so need to put_page once to avoid that
+                * split_huge_page fails in try_to_merge_two_pages().
+                */
+               if (SameCompound = Is_SameCompound(page, tree_page))
+                       put_page(tree_page);
 
                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.
-                */
-               split = PageTransCompound(page)
-                       && compound_head(page) == compound_head(tree_page);
-               put_page(tree_page);
+
+               if (!SameCompound)
+                       put_page(tree_page);
+
                if (kpage) {
                        /*
                         * The pages were successfully merged: insert new
@@ -2271,20 +2269,6 @@ 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) {
-                       /*
-                        * We are here if we tried to merge two pages and
-                        * failed because they both belonged to the same
-                        * compound page. We will split the page now, but no
-                        * merging will take place.
-                        * We do not want to add the cost of a full lock; if
-                        * the page is locked, it is better to skip it and
-                        * perhaps try again later.
-                        */
-                       if (!trylock_page(page))
-                               return;
-                       split_huge_page(page);
-                       unlock_page(page);
                }
        }
 }


>> 
>> I don't know the original reason, better ask Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>.
>> Maybe because doing detection that ahead of time will break several funtions' semantic,
>> such as try_to_merge_two_pages(), try_to_merge_with_ksm_page() and try_to_merge_one_page()
>> 
>> Adding the backup code don't change the old code and fixing the old problem, it's good.
>
>It's absolutely counter-intuitive to check for something that cannot 
>possibly work after the effects. This better has a good reason to make 
>that code more complicated.
>-- 

Re: [PATCH] ksm: delay the check of splitting compound pages

[David Hildenbrand]

On 16.11.23 13:17, xu wrote:
>>>>> @@ -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?
> 
> Do you mean like this?

Let me see if the refcounting here is sane:

(a) The caller has a reference on "page" that it will put just after the
     cmp_and_merge_page() call.
(b) unstable_tree_search_insert() obtained a reference to the
     "tree_page" using get_mergeable_page()->follow_page(). We will put
     that reference.

So indeed, if both references are to the same folio, *we* have two 
references to the folio and can safely drop one of both.

> 
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2229,23 +2229,21 @@ 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;
> +               bool SameCompound;
> +               /*
> +                * If they belongs to the same compound page, its' reference
> +                * get twice, so need to put_page once to avoid that
> +                * split_huge_page fails in try_to_merge_two_pages().
> +                */
> +               if (SameCompound = Is_SameCompound(page, tree_page))
> +                       put_page(tree_page);
>   

bool same_folio = page_folio(page) == page_folio(tree_page);

/*
  * If both pages belong to the same folio, we are holding two references
  * to the same large folio: splitting the folio in
  * try_to_merge_one_page() will fail for that reason. So let's just drop
  * one reference early.  Note that this is only possible if tree_page is
  * not a KSM page yet.
  */
if (same_folio)
	put_page(tree_page);

[we could use more folio operations here, but lets KIS]

-- 
Cheers,

David / dhildenb

Re: [PATCH] ksm: delay the check of splitting compound pages

[David Hildenbrand]

On 16.11.23 18:39, David Hildenbrand wrote:
> On 16.11.23 13:17, xu wrote:
>>>>>> @@ -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?
>>
>> Do you mean like this?
> 
> Let me see if the refcounting here is sane:
> 
> (a) The caller has a reference on "page" that it will put just after the
>       cmp_and_merge_page() call.
> (b) unstable_tree_search_insert() obtained a reference to the
>       "tree_page" using get_mergeable_page()->follow_page(). We will put
>       that reference.
> 
> So indeed, if both references are to the same folio, *we* have two
> references to the folio and can safely drop one of both.
> 
>>
>> --- a/mm/ksm.c
>> +++ b/mm/ksm.c
>> @@ -2229,23 +2229,21 @@ 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;
>> +               bool SameCompound;
>> +               /*
>> +                * If they belongs to the same compound page, its' reference
>> +                * get twice, so need to put_page once to avoid that
>> +                * split_huge_page fails in try_to_merge_two_pages().
>> +                */
>> +               if (SameCompound = Is_SameCompound(page, tree_page))
>> +                       put_page(tree_page);
>>    
> 
> bool same_folio = page_folio(page) == page_folio(tree_page);
> 
> /*
>    * If both pages belong to the same folio, we are holding two references
>    * to the same large folio: splitting the folio in
>    * try_to_merge_one_page() will fail for that reason. So let's just drop
>    * one reference early.  Note that this is only possible if tree_page is
>    * not a KSM page yet.
>    */
> if (same_folio)
> 	put_page(tree_page);
> 
> [we could use more folio operations here, but lets KIS]
> 

Looking into the details, that doesn't work unfortunately.

try_to_merge_one_page() will call split_huge_page(), but that
will only hold a reference to "page", not to "tree page" after the
split.

Long story short, after split_huge_page() tree_page could get freed
because we don't hold a reference to that one anymore.

So we most probably cannot do better than the following (untested):

diff --git a/mm/ksm.c b/mm/ksm.c
index 7efcc68ccc6e..afb079524585 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2226,25 +2226,30 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
  		if (!err)
  			return;
  	}
+retry:
  	tree_rmap_item =
  		unstable_tree_search_insert(rmap_item, page, &tree_page);
  	if (tree_rmap_item) {
-		bool split;
+		/*
+		 * If both pages belong to the same folio, we are holding two
+		 * references to the same large folio: splitting the folio in
+		 * try_to_merge_one_page() will fail for that reason.
+		 *
+		 * We have to split manually, and lookup the tree_page
+		 * again. Note that we'll only hold a reference to "page" after
+		 * the split.
+		 */
+		if (page_folio(page) == page_folio(tree_page)) {
+			put_page(tree_page);
+
+			if (!trylock_page(page))
+				return;
+			split_huge_page(page);
+			unlock_page(page);
+			goto retry;
+		}
  
  		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.
-		 */
-		split = PageTransCompound(page)
-			&& compound_head(page) == compound_head(tree_page);
  		put_page(tree_page);
  		if (kpage) {
  			/*
@@ -2271,20 +2276,6 @@ 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) {
-			/*
-			 * We are here if we tried to merge two pages and
-			 * failed because they both belonged to the same
-			 * compound page. We will split the page now, but no
-			 * merging will take place.
-			 * We do not want to add the cost of a full lock; if
-			 * the page is locked, it is better to skip it and
-			 * perhaps try again later.
-			 */
-			if (!trylock_page(page))
-				return;
-			split_huge_page(page);
-			unlock_page(page);
  		}
  	}
  }

-- 
Cheers,

David / dhildenb