Received-SPF: pass (google.com: domain of linux-kernel+bounces-137866-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <c35fcdca-bec9-4a68-99dd-dbc9b3ad97a5@gmail.com>
Date: Wed, 10 Apr 2024 11:47:12 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 8/9] mm/ksm: Convert chain series funcs and replace
 get_ksm_page
To: David Hildenbrand <david@redhat.com>, alexs@kernel.org,
 Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
 linux-kernel@vger.kernel.org
Cc: Izik Eidus <izik.eidus@ravellosystems.com>,
 Matthew Wilcox <willy@infradead.org>, Andrea Arcangeli
 <aarcange@redhat.com>, Hugh Dickins <hughd@google.com>,
 Chris Wright <chrisw@sous-sol.org>
References: <20240409092826.1733637-1-alexs@kernel.org>
 <20240409092826.1733637-9-alexs@kernel.org>
 <7a8d8005-3cec-4647-82b0-2d55d0ef34fc@redhat.com>
Content-Language: en-US
From: Alex Shi <seakeel@gmail.com>
In-Reply-To: <7a8d8005-3cec-4647-82b0-2d55d0ef34fc@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


On 4/9/24 7:02 PM, David Hildenbrand wrote:
> On 09.04.24 11:28, alexs@kernel.org wrote:
>> From: "Alex Shi (tencent)" <alexs@kernel.org>
>>
>> In ksm stable tree all page are single, let's convert them to use and
>> folios as well as stable_tree_insert/stable_tree_search funcs.
>> And replace get_ksm_page() by ksm_get_folio() since there is no more
>> needs.
>>
>> It could save a few compound_head calls.
>>
>> Signed-off-by: Alex Shi (tencent) <alexs@kernel.org>
>> Cc: Izik Eidus <izik.eidus@ravellosystems.com>
>> Cc: Matthew Wilcox <willy@infradead.org>
>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: Chris Wright <chrisw@sous-sol.org>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> I don't recall giving that yet :)

Ops... 
Sorry for misunderstand!

> 
> You could have kept some get_ksm_page()->ksm_get_folio() into a separate patch.
> 
> i.e., "[PATCH v3 11/14] mm/ksm: remove get_ksm_page and related info" from your old series could have mostly stayed separately.
> 

Yes, but the 12th and 11th patches are kind of depends each other, like after merge 8,9,10,12th with get_ksm_page replaced,

./mm/ksm.c:993:21: error: ‘get_ksm_page’ defined but not used [-Werror=unused-function]
  993 | static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
      |                     ^~~~~~~~~~~~

so we have to squash the 11th and 12th if we want to merge 12th with 8,9,10...
or we can do just merge the 8,9,10 and keep 11th, 12th as your first suggestion?

> [...]
> 
>>   /*
>> @@ -1829,7 +1821,7 @@ static __always_inline struct page *chain(struct ksm_stable_node **s_n_d,
>>    * This function returns the stable tree node of identical content if found,
>>    * NULL otherwise.
>>    */
>> -static struct page *stable_tree_search(struct page *page)
>> +static void *stable_tree_search(struct page *page)
> 
> There is one caller of stable_tree_search() in cmp_and_merge_page().
> 
> Why the change from page* to void* ?

Uh, a bit more changes needs if we want to remove void*. 

diff --git a/mm/ksm.c b/mm/ksm.c
index 0d703c3da9d8..cd414a9c33ad 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1815,7 +1815,7 @@ static __always_inline struct folio *chain(struct ksm_stable_node **s_n_d,
  * This function returns the stable tree node of identical content if found,
  * NULL otherwise.
  */
-static void *stable_tree_search(struct page *page)
+static struct folio *stable_tree_search(struct page *page)
 {
        int nid;
        struct rb_root *root;
@@ -2308,6 +2308,7 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
        struct page *tree_page = NULL;
        struct ksm_stable_node *stable_node;
        struct page *kpage;
+       struct folio *folio;
        unsigned int checksum;
        int err;
        bool max_page_sharing_bypass = false;
@@ -2333,7 +2334,8 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
        }
 
        /* We first start with searching the page inside the stable tree */
-       kpage = stable_tree_search(page);
+       folio = stable_tree_search(page);
+       kpage = &folio->page;
        if (kpage == page && rmap_item->head == stable_node) {
                put_page(kpage);
                return;

> I suspect cmp_and_merge_page() could similarly be converted to some degree to let kpage be a folio (separate patch).
>

Yes, there are couple of changes needed for cmp_and_merge_page() and series try_to_merge_xx_pages(), I am going to change them on the next series patches. Guess 2 phases patches are better for a big/huge one, is this right?

Thanks
Alex