Change the return value for page isolation functions

Now the folio_isolate_lru() did not return a boolean value to indicate isolation success or not, however below code checking the return value can make people think that it was a boolean success/failure thing, which makes people easy to make mistakes (see the fix patch[1]).

if (folio_isolate_lru(folio))

	continue;

Thus it's better to check the negative error value expilictly returned by folio_isolate_lru(), which makes code more clear per Linus's suggestion[2]. Moreover Matthew suggested we can convert the isolation functions to return a boolean[3], since most users did not care about the negative error value, and can also remove the confusing of checking return value.

So this patch converts the folio_isolate_lru() to return a boolean value, which means return 'true' to indicate the folio isolation is successful, and 'false' means a failure to isolation. Meanwhile changing all users' logic of checking the isolation state.

No functional changes intended.

[1] https://lore.kernel.org/all/[email protected]/T/#u
[2] https://lore.kernel.org/all/CAHk-=wiBrY+O-4=2mrbVyxR+hOqfdJ=Do6xoucfJ9_5az01L4Q@mail.gmail.com/
[3] https://lore.kernel.org/all/[email protected]/

Signed-off-by Baolin Wang <[email protected]>

---
 mm/damon/paddr.c  |  2 +-
 mm/folio-compat.c |  8 +++++++-
 mm/gup.c          |  2 +-
 mm/internal.h     |  2 +-
 mm/khugepaged.c   |  2 +-
 mm/madvise.c      |  4 ++--
 mm/mempolicy.c    |  2 +-
 mm/vmscan.c       | 10 +++++-----
 8 files changed, 19 insertions(+), 13 deletions(-)

mm/damon/paddr.c +1 -1

index b4df9b9bcc0a..607bb69e526c 100644
@@ -246,7 +246,7 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s)
 
 		folio_clear_referenced(folio);
 		folio_test_clear_young(folio);
-		if (folio_isolate_lru(folio)) {
+		if (!folio_isolate_lru(folio)) {
 			folio_put(folio);
 			continue;
 		}

mm/folio-compat.c +7 -1

index 18c48b557926..540373cf904e 100644
@@ -115,9 +115,15 @@ EXPORT_SYMBOL(grab_cache_page_write_begin);
 
 int isolate_lru_page(struct page *page)
 {
+	bool ret;
+
 	if (WARN_RATELIMIT(PageTail(page), "trying to isolate tail page"))
 		return -EBUSY;
-	return folio_isolate_lru((struct folio *)page);
+	ret = folio_isolate_lru((struct folio *)page);
+	if (ret)
+		return 0;
+
+	return -EBUSY;
 }
 
 void putback_lru_page(struct page *page)

mm/gup.c +1 -1

index b0885f70579c..eab18ba045db 100644
@@ -1939,7 +1939,7 @@ static unsigned long collect_longterm_unpinnable_pages(
 			drain_allow = false;
 		}
 
-		if (folio_isolate_lru(folio))
+		if (!folio_isolate_lru(folio))
 			continue;
 
 		list_add_tail(&folio->lru, movable_page_list);

mm/internal.h +1 -1

index dfb37e94e140..8645e8496537 100644
@@ -188,7 +188,7 @@ pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
  * in mm/vmscan.c:
  */
 int isolate_lru_page(struct page *page);
-int folio_isolate_lru(struct folio *folio);
+bool folio_isolate_lru(struct folio *folio);
 void putback_lru_page(struct page *page);
 void folio_putback_lru(struct folio *folio);
 extern void reclaim_throttle(pg_data_t *pgdat, enum vmscan_throttle_state reason);

mm/khugepaged.c +1 -1

index a5d32231bfad..cee659cfa3c1 100644
@@ -2047,7 +2047,7 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,
 			goto out_unlock;
 		}
 
-		if (folio_isolate_lru(folio)) {
+		if (!folio_isolate_lru(folio)) {
 			result = SCAN_DEL_PAGE_LRU;
 			goto out_unlock;
 		}

mm/madvise.c +2 -2

index 5a5a687d03c2..c2202f51e9dd 100644
@@ -406,7 +406,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
 		folio_clear_referenced(folio);
 		folio_test_clear_young(folio);
 		if (pageout) {
-			if (!folio_isolate_lru(folio)) {
+			if (folio_isolate_lru(folio)) {
 				if (folio_test_unevictable(folio))
 					folio_putback_lru(folio);
 				else

@@ -500,7 +500,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
 		folio_clear_referenced(folio);
 		folio_test_clear_young(folio);
 		if (pageout) {
-			if (!folio_isolate_lru(folio)) {
+			if (folio_isolate_lru(folio)) {
 				if (folio_test_unevictable(folio))
 					folio_putback_lru(folio);
 				else

mm/mempolicy.c +1 -1

index 0919c7a719d4..2751bc3310fd 100644
@@ -1033,7 +1033,7 @@ static int migrate_folio_add(struct folio *folio, struct list_head *foliolist,
 	 * expensive, so check the estimated mapcount of the folio instead.
 	 */
 	if ((flags & MPOL_MF_MOVE_ALL) || folio_estimated_sharers(folio) == 1) {
-		if (!folio_isolate_lru(folio)) {
+		if (folio_isolate_lru(folio)) {
 			list_add_tail(&folio->lru, foliolist);
 			node_stat_mod_folio(folio,
 				NR_ISOLATED_ANON + folio_is_file_lru(folio),

mm/vmscan.c +5 -5

index 34535bbd4fe9..7658b40df947 100644
@@ -2337,12 +2337,12 @@ static unsigned long isolate_lru_folios(unsigned long nr_to_scan,
  * (2) The lru_lock must not be held.
  * (3) Interrupts must be enabled.
  *
- * Return: 0 if the folio was removed from an LRU list.
- * -EBUSY if the folio was not on an LRU list.
+ * Return: true if the folio was removed from an LRU list.
+ * false if the folio was not on an LRU list.
  */
-int folio_isolate_lru(struct folio *folio)
+bool folio_isolate_lru(struct folio *folio)
 {
-	int ret = -EBUSY;
+	bool ret = false;
 
 	VM_BUG_ON_FOLIO(!folio_ref_count(folio), folio);
 

@@ -2353,7 +2353,7 @@ int folio_isolate_lru(struct folio *folio)
 		lruvec = folio_lruvec_lock_irq(folio);
 		lruvec_del_folio(lruvec, folio);
 		unlock_page_lruvec_irq(lruvec);
-		ret = 0;
+		ret = true;
 	}
 
 	return ret;

Signature · 1 line

2.27.0

The isolate_lru_page() can only return 0 or -EBUSY, and most users did not care about the negative error of isolate_lru_page(), except one user in add_page_for_migration(). So we can convert the isolate_lru_page() to return a boolean value, which can help to make the code more clear when checking the return value of isolate_lru_page().

Also convert all users' logic of checking the isolation state.

No functional changes intended.

Signed-off-by Baolin Wang <[email protected]>

---
 mm/folio-compat.c   | 12 +++---------
 mm/internal.h       |  2 +-
 mm/khugepaged.c     |  2 +-
 mm/memcontrol.c     |  4 ++--
 mm/memory-failure.c |  4 ++--
 mm/memory_hotplug.c |  2 +-
 mm/migrate.c        |  9 ++++++---
 mm/migrate_device.c |  2 +-
 8 files changed, 17 insertions(+), 20 deletions(-)

mm/memory_hotplug.c +0 -0

 		else
 			ret = isolate_movable_page(page, ISOLATE_UNEVICTABLE);
 		if (!ret) { /* Success */

Quoted 22 lines · On Tue, 14 Feb 2023 21:59:30 +0800 Baolin Wang…

The isolate_lru_page() can only return 0 or -EBUSY, and most users did not care about the negative error of isolate_lru_page(), except one user in add_page_for_migration(). So we can convert the isolate_lru_page() to return a boolean value, which can help to make the code more clear when checking the return value of isolate_lru_page().

Also convert all users' logic of checking the isolation state.

No functional changes intended.

Signed-off-by Baolin Wang <baolin.wang-KPsoFbNs7GizrGE5bRqYAgC/[email protected]>

---
  mm/folio-compat.c   | 12 +++---------
  mm/internal.h       |  2 +-
  mm/khugepaged.c     |  2 +-
  mm/memcontrol.c     |  4 ++--
  mm/memory-failure.c |  4 ++--
  mm/memory_hotplug.c |  2 +-
  mm/migrate.c        |  9 ++++++---
  mm/migrate_device.c |  2 +-
  8 files changed, 17 insertions(+), 20 deletions(-)

[...]

mm/memory_hotplug.c do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) · line 1671

Quoted 10 lines

mm/memory_hotplug.c +1 -1

index a1e8c3e9ab08..17ed80707518 100644
@@ -1668,7 +1668,7 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
  		 * LRU and non-lru movable pages.
  		 */
  		if (PageLRU(page))
-			ret = isolate_lru_page(page);
+			ret = !isolate_lru_page(page);

This may change return value of this function. That is, this function will return 1 instead of -EBUSY after this change. It's not a real issue as no caller of this function takes care of the return value, though.

Now the isolate_hugetlb() only returns 0 or -EBUSY, and most users did not care about the negative value, thus we can convert the isolate_hugetlb() to return a boolean value to make code more clear when checking the hugetlb isolation state. Moreover converts 2 users which will consider the negative value returned by isolate_hugetlb().

No functional changes intended.

Signed-off-by Baolin Wang <baolin.wang-KPsoFbNs7GizrGE5bRqYAgC/[email protected]>

---
 include/linux/hugetlb.h |  6 +++---
 mm/hugetlb.c            | 12 ++++++++----
 mm/memory-failure.c     |  2 +-
 mm/mempolicy.c          |  2 +-
 mm/migrate.c            |  2 +-
 5 files changed, 14 insertions(+), 10 deletions(-)

include/linux/hugetlb.h +3 -3

index df6dd624ccfe..5f5e4177b2e0 100644
@@ -171,7 +171,7 @@ bool hugetlb_reserve_pages(struct inode *inode, long from, long to,
 						vm_flags_t vm_flags);
 long hugetlb_unreserve_pages(struct inode *inode, long start, long end,
 						long freed);
-int isolate_hugetlb(struct folio *folio, struct list_head *list);
+bool isolate_hugetlb(struct folio *folio, struct list_head *list);
 int get_hwpoison_hugetlb_folio(struct folio *folio, bool *hugetlb, bool unpoison);
 int get_huge_page_for_hwpoison(unsigned long pfn, int flags,
 				bool *migratable_cleared);

@@ -413,9 +413,9 @@ static inline pte_t *huge_pte_offset(struct mm_struct *mm, unsigned long addr,
 	return NULL;
 }
 
-static inline int isolate_hugetlb(struct folio *folio, struct list_head *list)
+static inline bool isolate_hugetlb(struct folio *folio, struct list_head *list)
 {
-	return -EBUSY;
+	return false;
 }
 
 static inline int get_hwpoison_hugetlb_folio(struct folio *folio, bool *hugetlb, bool unpoison)

mm/hugetlb.c +0 -0

index 3a01a9dbf445..75097e3abc18 100644

mm/hugetlb.c +4 -4

 		goto free_new;
 	} else if (!folio_test_hugetlb_freed(old_folio)) {
 		/*

@@ -3005,7 +3009,7 @@ int isolate_or_dissolve_huge_page(struct page *page, struct list_head *list)
 	if (hstate_is_gigantic(h))
 		return -ENOMEM;
 
-	if (folio_ref_count(folio) && !isolate_hugetlb(folio, list))
+	if (folio_ref_count(folio) && isolate_hugetlb(folio, list))
 		ret = 0;
 	else if (!folio_ref_count(folio))
 		ret = alloc_and_dissolve_hugetlb_folio(h, folio, list);

@@ -7251,15 +7255,15 @@ __weak unsigned long hugetlb_mask_last_page(struct hstate *h)
  * These functions are overwritable if your architecture needs its own
  * behavior.
  */
-int isolate_hugetlb(struct folio *folio, struct list_head *list)
+bool isolate_hugetlb(struct folio *folio, struct list_head *list)
 {
-	int ret = 0;
+	bool ret = true;
 
 	spin_lock_irq(&hugetlb_lock);
 	if (!folio_test_hugetlb(folio) ||
 	    !folio_test_hugetlb_migratable(folio) ||
 	    !folio_try_get(folio)) {
-		ret = -EBUSY;
+		ret = false;
 		goto unlock;
 	}
 	folio_clear_hugetlb_migratable(folio);

mm/memory-failure.c +1 -1

index e504362fdb23..8604753bc644 100644
@@ -2508,7 +2508,7 @@ static bool isolate_page(struct page *page, struct list_head *pagelist)
 	bool isolated = false;
 
 	if (PageHuge(page)) {
-		isolated = !isolate_hugetlb(page_folio(page), pagelist);
+		isolated = isolate_hugetlb(page_folio(page), pagelist);
 	} else {
 		bool lru = !__PageMovable(page);
 

mm/mempolicy.c +1 -1

index 2751bc3310fd..a256a241fd1d 100644
@@ -609,7 +609,7 @@ static int queue_folios_hugetlb(pte_t *pte, unsigned long hmask,
 	if (flags & (MPOL_MF_MOVE_ALL) ||
 	    (flags & MPOL_MF_MOVE && folio_estimated_sharers(folio) == 1 &&
 	     !hugetlb_pmd_shared(pte))) {
-		if (isolate_hugetlb(folio, qp->pagelist) &&
+		if (!isolate_hugetlb(folio, qp->pagelist) &&
 			(flags & MPOL_MF_STRICT))
 			/*
 			 * Failed to isolate folio but allow migrating pages

mm/migrate.c +0 -0

index 53010a142e7f..c5136fa48638 100644

mm/migrate.c +0 -0

 		}
 	} else {
 		struct page *head;

Signature · 1 line

2.27.0

Quoted 19 lines · On Tue, 14 Feb 2023 21:59:31 +0800 Baolin Wang <[email protected]>…

Now the isolate_hugetlb() only returns 0 or -EBUSY, and most users did not care about the negative value, thus we can convert the isolate_hugetlb() to return a boolean value to make code more clear when checking the hugetlb isolation state. Moreover converts 2 users which will consider the negative value returned by isolate_hugetlb().

No functional changes intended.

Signed-off-by Baolin Wang <[email protected]>

---
 include/linux/hugetlb.h |  6 +++---
 mm/hugetlb.c            | 12 ++++++++----
 mm/memory-failure.c     |  2 +-
 mm/mempolicy.c          |  2 +-
 mm/migrate.c            |  2 +-
 5 files changed, 14 insertions(+), 10 deletions(-)

include/linux/hugetlb.h +0 -0

index df6dd624ccfe..5f5e4177b2e0 100644

Quoted 4 lines

mm/migrate.c +0 -0

index 53010a142e7f..c5136fa48638 100644

mm/migrate.c static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, · line 2131

Quoted 6 lines

mm/migrate.c +1 -1

@@ -2128,7 +2128,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
 		if (PageHead(page)) {
 			err = isolate_hugetlb(page_folio(page), pagelist);
 			if (!err)
-				err = 1;
+				err = -EBUSY;

Again, I think this is confusing. 'err' is 'bool', not 'int'.

Thanks,
SJ

Quoted 6 lines

mm/migrate.c +0 -0

 		}
 	} else {
 		struct page *head;

Signature · 1 line

2.27.0

Quoted 21 lines · On Tue, 14 Feb 2023 18:03:24 +0000 SeongJae Park…

Quoted 19 lines · On Tue, 14 Feb 2023 21:59:31 +0800 Baolin Wang…

Now the isolate_hugetlb() only returns 0 or -EBUSY, and most users did not care about the negative value, thus we can convert the isolate_hugetlb() to return a boolean value to make code more clear when checking the hugetlb isolation state. Moreover converts 2 users which will consider the negative value returned by isolate_hugetlb().

No functional changes intended.

Signed-off-by Baolin Wang <baolin.wang-KPsoFbNs7GizrGE5bRqYAgC/[email protected]>

---
 include/linux/hugetlb.h |  6 +++---
 mm/hugetlb.c            | 12 ++++++++----
 mm/memory-failure.c     |  2 +-
 mm/mempolicy.c          |  2 +-
 mm/migrate.c            |  2 +-
 5 files changed, 14 insertions(+), 10 deletions(-)

include/linux/hugetlb.h +0 -0

index df6dd624ccfe..5f5e4177b2e0 100644

[...]

Quoted 4 lines

Quoted 4 lines

mm/migrate.c +0 -0

index 53010a142e7f..c5136fa48638 100644

Quoted 8 lines

mm/migrate.c static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, · line 2131

Quoted 6 lines

mm/migrate.c +1 -1

@@ -2128,7 +2128,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
 		if (PageHead(page)) {
 			err = isolate_hugetlb(page_folio(page), pagelist);
 			if (!err)
-				err = 1;
+				err = -EBUSY;

Again, I think this is confusing. 'err' is 'bool', not 'int'.

I mean, 'err' is not 'bool' but 'int', sorry. See? This confuses me ;)

Quoted 37 lines · SeongJae Park

Quoted 21 lines · On Tue, 14 Feb 2023 18:03:24 +0000 SeongJae Park <[email protected]> wrote:

Quoted 19 lines · On Tue, 14 Feb 2023 21:59:31 +0800 Baolin Wang <[email protected]>…

Now the isolate_hugetlb() only returns 0 or -EBUSY, and most users did not care about the negative value, thus we can convert the isolate_hugetlb() to return a boolean value to make code more clear when checking the hugetlb isolation state. Moreover converts 2 users which will consider the negative value returned by isolate_hugetlb().

No functional changes intended.

Signed-off-by Baolin Wang <[email protected]>

---
  include/linux/hugetlb.h |  6 +++---
  mm/hugetlb.c            | 12 ++++++++----
  mm/memory-failure.c     |  2 +-
  mm/mempolicy.c          |  2 +-
  mm/migrate.c            |  2 +-
  5 files changed, 14 insertions(+), 10 deletions(-)

include/linux/hugetlb.h +0 -0

index df6dd624ccfe..5f5e4177b2e0 100644

[...]

Quoted 4 lines

Quoted 4 lines

mm/migrate.c +0 -0

index 53010a142e7f..c5136fa48638 100644

Quoted 8 lines

mm/migrate.c static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, · line 2131

Quoted 6 lines

mm/migrate.c +1 -1

@@ -2128,7 +2128,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
  		if (PageHead(page)) {
  			err = isolate_hugetlb(page_folio(page), pagelist);
  			if (!err)
-				err = 1;
+				err = -EBUSY;

Again, I think this is confusing. 'err' is 'bool', not 'int'.

Quoted 2 lines

I mean, 'err' is not 'bool' but 'int', sorry. See? This confuses me ;)

Yes,
in the case here (and elsewhere) I like David's suggestion of using a separate bool such as 'isolated' to capture the return value of the isolate function. Then, the statement:

	err = isolated ? 0 : -EBUSY;

would be pretty clear.

Now the page isolation functions did not return a boolean to indicate success or not, instead it will return a negative error when failed to isolate a page. So below code used in most places seem a boolean success/failure thing, which can confuse people whether the isolation is successful.

if (folio_isolate_lru(folio))

         continue;

Moreover the page isolation functions only return 0 or -EBUSY, and most users did not care about the negative error except for few users, thus we can convert all page isolation functions to return a boolean value, which can remove the confusion to make code more clear.

No functional changes intended in this patch series.

Changes from v1:
- Convert all isolation functions to return bool.

Quoted 14 lines · Baolin Wang

Now the page isolation functions did not return a boolean to indicate success or not, instead it will return a negative error when failed to isolate a page. So below code used in most places seem a boolean success/failure thing, which can confuse people whether the isolation is successful.

if (folio_isolate_lru(folio))
         continue;

Moreover the page isolation functions only return 0 or -EBUSY, and most users did not care about the negative error except for few users, thus we can convert all page isolation functions to return a boolean value, which can remove the confusion to make code more clear.

Quoted 4 lines

No functional changes intended in this patch series.

Changes from v1:
  - Convert all isolation functions to return bool.

Acked-by David Hildenbrand <[email protected]>

Although it's controversial if

if (!ret)
    ret = -EBUSY;
else
    ret = 0;

is really appealing to the readers eye :)

ret = ret ? 0 : -EBUSY;

It's still confusing.

would be better as

ret = isolated ? 0 : -EBUSY;

IOW, not reusing the "int ret" variable.