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

[David Hildenbrand <david@redhat.com>]

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