[01/11] mm/ksm: Convert get_ksm_page to return a folio

Commit Message

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

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

The ksm only contains single pages, so use folio instead of pages to
save a couple of 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 | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Comments

Matthew Wilcox March 20, 2024, 12:54 p.m. UTC | #1

On Wed, Mar 20, 2024 at 03:40:37PM +0800, [e-mail] wrote:
> -static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
> +static void *get_ksm_page(struct ksm_stable_node *stable_node,
>  				 enum get_ksm_page_flags flags)

I am really not a fan of returning void * instead of a page or a
folio.  Particularly since you rename this function at the end anyway!

You should do it like this:

In this patch, convert get_ksm_page() to get_ksm_folio() and add:

static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
		enum get_ksm_page_flags flags)
{
	struct folio *folio = get_ksm_folio(node, flags);
	return &folio->page;
}

Then convert each call-site to get_ksm_folio(), and finally delete
get_ksm_page().  That way you're always converting each caller to
the exact code you want it to look like, and your reiewrs don't have to
keep three patches in their head at once as they review each place.

Also, I think this should be ksm_get_folio(), not get_ksm_folio().
Seems to fit better.

> @@ -949,32 +949,32 @@ static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
>  		 * in the ref_freeze section of __remove_mapping(); but Anon
>  		 * page->mapping reset to NULL later, in free_pages_prepare().

Could you fix page->mapping to folio->mapping in the comment?

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

On 3/20/24 8:54 PM, Matthew Wilcox wrote:
> On Wed, Mar 20, 2024 at 03:40:37PM +0800, [e-mail] wrote:
>> -static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
>> +static void *get_ksm_page(struct ksm_stable_node *stable_node,
>>  				 enum get_ksm_page_flags flags)
> 
> I am really not a fan of returning void * instead of a page or a
> folio.  Particularly since you rename this function at the end anyway!
> 
> You should do it like this:
> 
> In this patch, convert get_ksm_page() to get_ksm_folio() and add:
> 
> static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
> 		enum get_ksm_page_flags flags)
> {
> 	struct folio *folio = get_ksm_folio(node, flags);
> 	return &folio->page;
> }
> 
> Then convert each call-site to get_ksm_folio(), and finally delete
> get_ksm_page().  That way you're always converting each caller to
> the exact code you want it to look like, and your reiewrs don't have to
> keep three patches in their head at once as they review each place.
> 
> Also, I think this should be ksm_get_folio(), not get_ksm_folio().
> Seems to fit better.
> 
>> @@ -949,32 +949,32 @@ static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
>>  		 * in the ref_freeze section of __remove_mapping(); but Anon
>>  		 * page->mapping reset to NULL later, in free_pages_prepare().
> 
> Could you fix page->mapping to folio->mapping in the comment?
> 

Thanks for comment! I will take your suggestion and resend soon. 

Best regards!

Patch

diff --git a/mm/ksm.c b/mm/ksm.c
index 8c001819cf10..fda291b054c2 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -915,10 +915,10 @@  enum get_ksm_page_flags {
  * a page to put something that might look like our key in page->mapping.
  * is on its way to being freed; but it is an anomaly to bear in mind.
  */
-static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
+static void *get_ksm_page(struct ksm_stable_node *stable_node,
 				 enum get_ksm_page_flags flags)
 {
-	struct page *page;
+	struct folio *folio;
 	void *expected_mapping;
 	unsigned long kpfn;
 
@@ -926,8 +926,8 @@  static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
 					PAGE_MAPPING_KSM);
 again:
 	kpfn = READ_ONCE(stable_node->kpfn); /* Address dependency. */
-	page = pfn_to_page(kpfn);
-	if (READ_ONCE(page->mapping) != expected_mapping)
+	folio = pfn_folio(kpfn);
+	if (READ_ONCE(folio->mapping) != expected_mapping)
 		goto stale;
 
 	/*
@@ -940,7 +940,7 @@  static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
 	 * in folio_migrate_mapping(), it might still be our page,
 	 * in which case it's essential to keep the node.
 	 */
-	while (!get_page_unless_zero(page)) {
+	while (!folio_try_get(folio)) {
 		/*
 		 * Another check for page->mapping != expected_mapping would
 		 * work here too.  We have chosen the !PageSwapCache test to
@@ -949,32 +949,32 @@  static struct page *get_ksm_page(struct ksm_stable_node *stable_node,
 		 * in the ref_freeze section of __remove_mapping(); but Anon
 		 * page->mapping reset to NULL later, in free_pages_prepare().
 		 */
-		if (!PageSwapCache(page))
+		if (!folio_test_swapcache(folio))
 			goto stale;
 		cpu_relax();
 	}
 
-	if (READ_ONCE(page->mapping) != expected_mapping) {
-		put_page(page);
+	if (READ_ONCE(folio->mapping) != expected_mapping) {
+		folio_put(folio);
 		goto stale;
 	}
 
 	if (flags == GET_KSM_PAGE_TRYLOCK) {
-		if (!trylock_page(page)) {
-			put_page(page);
+		if (!folio_trylock(folio)) {
+			folio_put(folio);
 			return ERR_PTR(-EBUSY);
 		}
 	} else if (flags == GET_KSM_PAGE_LOCK)
-		lock_page(page);
+		folio_lock(folio);
 
 	if (flags != GET_KSM_PAGE_NOLOCK) {
-		if (READ_ONCE(page->mapping) != expected_mapping) {
-			unlock_page(page);
-			put_page(page);
+		if (READ_ONCE(folio->mapping) != expected_mapping) {
+			folio_unlock(folio);
+			folio_put(folio);
 			goto stale;
 		}
 	}
-	return page;
+	return folio;
 
 stale:
 	/*