[09/11] mm/ksm: Convert stable_tree_search to use folio

Commit Message

[e-mail] March 20, 2024, 7:40 a.m. UTC

From: "Alex Shi (tencent)" <[e-mail]>

Although, the func may pass a tail page to check its contents, but only
single page exist in KSM stable tree, so we still can use folio in
stable_tree_search() to save a few compound_head calls.

Signed-off-by: Alex Shi (tencent) <[e-mail]>
Cc: Izik Eidus <[e-mail]>
Cc: Matthew Wilcox <[e-mail]>
Cc: Andrea Arcangeli <[e-mail]>
Cc: Hugh Dickins <[e-mail]>
Cc: Chris Wright <[e-mail]>
---
 mm/ksm.c | 60 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 31 insertions(+), 29 deletions(-)

Comments

Matthew Wilcox March 20, 2024, 3:26 p.m. UTC | #1

On Wed, Mar 20, 2024 at 03:40:45PM +0800, [e-mail] wrote:
> @@ -1820,28 +1820,30 @@ static struct page *stable_tree_search(struct page *page)
>  	struct rb_node *parent;
>  	struct ksm_stable_node *stable_node, *stable_node_dup, *stable_node_any;
>  	struct ksm_stable_node *page_node;
> +	struct folio *folio;
>  
> -	page_node = page_stable_node(page);
> +	folio = (struct folio *)page;

These casts make me nervous.  Remember that we're heading towards a
future where struct page and struct folio don't point to the same
memory, and part of that will be finding all these casts and removing
them.  Please, unless there's a good reason, just use page_folio().

Alex Shi March 21, 2024, 1:47 a.m. UTC | #2

On 3/20/24 11:26 PM, Matthew Wilcox wrote:
> On Wed, Mar 20, 2024 at 03:40:45PM +0800, [e-mail] wrote:
>> @@ -1820,28 +1820,30 @@ static struct page *stable_tree_search(struct page *page)
>>  	struct rb_node *parent;
>>  	struct ksm_stable_node *stable_node, *stable_node_dup, *stable_node_any;
>>  	struct ksm_stable_node *page_node;
>> +	struct folio *folio;
>>  
>> -	page_node = page_stable_node(page);
>> +	folio = (struct folio *)page;
> 
> These casts make me nervous.  Remember that we're heading towards a
> future where struct page and struct folio don't point to the same
> memory, and part of that will be finding all these casts and removing
> them.  Please, unless there's a good reason, just use page_folio().
> 

Hi willy,

Thanks a lot for all comments. Yes，all of them are very right. I will rewrite and send the patchset.

Best Regards!
Alex

Patch

diff --git a/mm/ksm.c b/mm/ksm.c
index 71d1a52f344d..75401b3bae5c 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1812,7 +1812,7 @@  static __always_inline void *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)
 {
 	int nid;
 	struct rb_root *root;
@@ -1820,28 +1820,30 @@  static struct page *stable_tree_search(struct page *page)
 	struct rb_node *parent;
 	struct ksm_stable_node *stable_node, *stable_node_dup, *stable_node_any;
 	struct ksm_stable_node *page_node;
+	struct folio *folio;
 
-	page_node = page_stable_node(page);
+	folio = (struct folio *)page;
+	page_node = folio_stable_node(folio);
 	if (page_node && page_node->head != &migrate_nodes) {
 		/* ksm page forked */
-		get_page(page);
-		return page;
+		folio_get(folio);
+		return folio;
 	}
 
-	nid = get_kpfn_nid(page_to_pfn(page));
+	nid = get_kpfn_nid(folio_pfn(folio));
 	root = root_stable_tree + nid;
 again:
 	new = &root->rb_node;
 	parent = NULL;
 
 	while (*new) {
-		struct page *tree_page;
+		struct folio *tree_folio;
 		int ret;
 
 		cond_resched();
 		stable_node = rb_entry(*new, struct ksm_stable_node, node);
 		stable_node_any = NULL;
-		tree_page = chain_prune(&stable_node_dup, &stable_node,	root);
+		tree_folio = chain_prune(&stable_node_dup, &stable_node, root);
 		/*
 		 * NOTE: stable_node may have been freed by
 		 * chain_prune() if the returned stable_node_dup is
@@ -1875,11 +1877,11 @@  static struct page *stable_tree_search(struct page *page)
 			 * write protected at all times. Any will work
 			 * fine to continue the walk.
 			 */
-			tree_page = get_ksm_page(stable_node_any,
-						 GET_KSM_PAGE_NOLOCK);
+			tree_folio = get_ksm_page(stable_node_any,
+						  GET_KSM_PAGE_NOLOCK);
 		}
 		VM_BUG_ON(!stable_node_dup ^ !!stable_node_any);
-		if (!tree_page) {
+		if (!tree_folio) {
 			/*
 			 * If we walked over a stale stable_node,
 			 * get_ksm_page() will call rb_erase() and it
@@ -1892,8 +1894,8 @@  static struct page *stable_tree_search(struct page *page)
 			goto again;
 		}
 
-		ret = memcmp_pages(page, tree_page);
-		put_page(tree_page);
+		ret = memcmp_pages(&folio->page, &tree_folio->page);
+		folio_put(tree_folio);
 
 		parent = *new;
 		if (ret < 0)
@@ -1936,26 +1938,26 @@  static struct page *stable_tree_search(struct page *page)
 			 * It would be more elegant to return stable_node
 			 * than kpage, but that involves more changes.
 			 */
-			tree_page = get_ksm_page(stable_node_dup,
-						 GET_KSM_PAGE_TRYLOCK);
+			tree_folio = get_ksm_page(stable_node_dup,
+						  GET_KSM_PAGE_TRYLOCK);
 
-			if (PTR_ERR(tree_page) == -EBUSY)
+			if (PTR_ERR(tree_folio) == -EBUSY)
 				return ERR_PTR(-EBUSY);
 
-			if (unlikely(!tree_page))
+			if (unlikely(!tree_folio))
 				/*
 				 * The tree may have been rebalanced,
 				 * so re-evaluate parent and new.
 				 */
 				goto again;
-			unlock_page(tree_page);
+			folio_unlock(tree_folio);
 
 			if (get_kpfn_nid(stable_node_dup->kpfn) !=
 			    NUMA(stable_node_dup->nid)) {
-				put_page(tree_page);
+				folio_put(tree_folio);
 				goto replace;
 			}
-			return tree_page;
+			return tree_folio;
 		}
 	}
 
@@ -1968,8 +1970,8 @@  static struct page *stable_tree_search(struct page *page)
 	rb_insert_color(&page_node->node, root);
 out:
 	if (is_page_sharing_candidate(page_node)) {
-		get_page(page);
-		return page;
+		folio_get(folio);
+		return folio;
 	} else
 		return NULL;
 
@@ -1994,12 +1996,12 @@  static struct page *stable_tree_search(struct page *page)
 					&page_node->node,
 					root);
 			if (is_page_sharing_candidate(page_node))
-				get_page(page);
+				folio_get(folio);
 			else
-				page = NULL;
+				folio = NULL;
 		} else {
 			rb_erase(&stable_node_dup->node, root);
-			page = NULL;
+			folio = NULL;
 		}
 	} else {
 		VM_BUG_ON(!is_stable_node_chain(stable_node));
@@ -2010,16 +2012,16 @@  static struct page *stable_tree_search(struct page *page)
 			DO_NUMA(page_node->nid = nid);
 			stable_node_chain_add_dup(page_node, stable_node);
 			if (is_page_sharing_candidate(page_node))
-				get_page(page);
+				folio_get(folio);
 			else
-				page = NULL;
+				folio = NULL;
 		} else {
-			page = NULL;
+			folio = NULL;
 		}
 	}
 	stable_node_dup->head = &migrate_nodes;
 	list_add(&stable_node_dup->list, stable_node_dup->head);
-	return page;
+	return folio;
 
 chain_append:
 	/* stable_node_dup could be null if it reached the limit */
@@ -2109,7 +2111,7 @@  static struct ksm_stable_node *stable_tree_insert(struct folio *kfolio)
 			 * fine to continue the walk.
 			 */
 			tree_folio = get_ksm_page(stable_node_any,
-						 GET_KSM_PAGE_NOLOCK);
+						  GET_KSM_PAGE_NOLOCK);
 		}
 		VM_BUG_ON(!stable_node_dup ^ !!stable_node_any);
 		if (!tree_folio) {