Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
From:   xu <xu.xin.sc@gmail.com>
To:     david@redhat.com
Cc:     akpm@linux-foundation.org, imbrenda@linux.ibm.com,
        jiang.xuexin@zte.com.cn, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, ran.xiaokai@zte.com.cn, wang.yong12@zte.com.cn,
        xu.xin.sc@gmail.com, xu.xin16@zte.com.cn, yang.yang29@zte.com.cn
Subject: Re: [PATCH] ksm: delay the check of splitting compound pages
Date:   Thu, 16 Nov 2023 12:17:15 +0000
Message-Id: <20231116121715.1974713-1-xu.xin16@zte.com.cn>
In-Reply-To: <d87bfcdd-cbaa-4485-a63b-6a524681fa08@redhat.com>
References: <d87bfcdd-cbaa-4485-a63b-6a524681fa08@redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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