Change the return value for page isolation functions

PATCH Applied Baolin Wang 6 participants 16 messages Feb 15, 2023 – Feb 16, 2023

Subsystems (16)

Memory management MM core Page allocator cgroup +12 more subsystems

lore

Share & tools

lore

Baolin Wang

author reviewer

2023-02-15 10:39 UTC #0 lore raw

v3 cover Change the return value for page isolation functions

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 v2:
- Add a new boolean 'isolated' variable for functions that require the negative error value.
- Keep the same return value logic in do_migrate_range(), that means it will return -EBUSY if all pages are failed to be isolated.
- Collect Acked-by and Reviewed-by tags. Thanks David and SeongJae.

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

Baolin Wang (4):
mm: change to return bool for folio_isolate_lru()
mm: change to return bool for isolate_lru_page()
mm: hugetlb: change to return bool for isolate_hugetlb()
mm: change to return bool for isolate_movable_page()

 include/linux/hugetlb.h |  6 +++---
 include/linux/migrate.h |  6 +++---
 mm/compaction.c         |  2 +-
 mm/damon/paddr.c        |  2 +-
 mm/folio-compat.c       |  4 ++--
 mm/gup.c                |  2 +-
 mm/hugetlb.c            | 13 ++++++++-----
 mm/internal.h           |  4 ++--
 mm/khugepaged.c         |  4 ++--
 mm/madvise.c            |  4 ++--
 mm/memcontrol.c         |  4 ++--
 mm/memory-failure.c     | 10 +++++-----
 mm/memory_hotplug.c     |  8 +++++---
 mm/mempolicy.c          |  4 ++--
 mm/migrate.c            | 20 +++++++++++---------
 mm/migrate_device.c     |  2 +-
 mm/vmscan.c             | 10 +++++-----
 17 files changed, 56 insertions(+), 49 deletions(-)

Signature · 1 line

2.27.0

Baolin Wang

author reviewer

2023-02-15 10:39 UTC #1 lore raw

PATCH v3 1/4 mm: change to return bool for folio_isolate_lru()

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/20230131063206.28820-1-Kuan-Ying.Lee-NuS5LvNUpcJWk0Htik3J/[email protected]/T/#u
[2] https://lore.kernel.org/all/CAHk-=wiBrY+O-4=2mrbVyxR+hOqfdJ=Do6xoucfJ9_5az01L4Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org/
[3] https://lore.kernel.org/all/Y+sTFqwMNAjDvxw3-FZi0V3Vbi30CUdFEqe4BF2D2FQJk+8+b@public.gmane.org/

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

Reviewed-by SeongJae Park <[email protected]>

Acked-by David Hildenbrand <[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

Matthew Wilcox

Reviewed-by on v3 1/4

2023-02-15 15:35 UTC #2 lore raw

Re: [PATCH v3 1/4] mm: change to return bool for folio_isolate_lru()

Quoted 21 lines · Baolin Wang

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.

Quoted 7 lines

[1] https://lore.kernel.org/all/20230131063206.28820-1-Kuan-Ying.Lee-NuS5LvNUpcJWk0Htik3J/[email protected]/T/#u
[2] https://lore.kernel.org/all/CAHk-=wiBrY+O-4=2mrbVyxR+hOqfdJ=Do6xoucfJ9_5az01L4Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org/
[3] https://lore.kernel.org/all/Y+sTFqwMNAjDvxw3-FZi0V3Vbi30CUdFEqe4BF2D2FQJk+8+b@public.gmane.org/
Signed-off-by Baolin Wang <baolin.wang-KPsoFbNs7GizrGE5bRqYAgC/[email protected]>
Reviewed-by SeongJae Park <[email protected]>
Acked-by David Hildenbrand <[email protected]>

Reviewed-by Matthew Wilcox (Oracle) <[email protected]>

Baolin Wang

author reviewer

2023-02-15 10:39 UTC #3 lore raw

PATCH v3 2/4 mm: change to return bool for isolate_lru_page()

In reply to 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]>

Acked-by David Hildenbrand <[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 |  8 +++++---
 mm/migrate.c        |  9 ++++++---
 mm/migrate_device.c |  2 +-
 8 files changed, 21 insertions(+), 22 deletions(-)

mm/folio-compat.c +3 -9

index 540373cf904e..cabcd1de9ecb 100644
@@ -113,17 +113,11 @@ struct page *grab_cache_page_write_begin(struct address_space *mapping,
 }
 EXPORT_SYMBOL(grab_cache_page_write_begin);
 
-int isolate_lru_page(struct page *page)
+bool isolate_lru_page(struct page *page)
 {
-	bool ret;
-
 	if (WARN_RATELIMIT(PageTail(page), "trying to isolate tail page"))
-		return -EBUSY;
-	ret = folio_isolate_lru((struct folio *)page);
-	if (ret)
-		return 0;
-
-	return -EBUSY;
+		return false;
+	return folio_isolate_lru((struct folio *)page);
 }
 
 void putback_lru_page(struct page *page)

mm/internal.h +1 -1

index 8645e8496537..fc01fd092ea5 100644
@@ -187,7 +187,7 @@ pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
 /*
  * in mm/vmscan.c:
  */
-int isolate_lru_page(struct page *page);
+bool isolate_lru_page(struct page *page);
 bool folio_isolate_lru(struct folio *folio);
 void putback_lru_page(struct page *page);
 void folio_putback_lru(struct folio *folio);

mm/khugepaged.c +1 -1

index cee659cfa3c1..8dbc39896811 100644
@@ -659,7 +659,7 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
 		 * Isolate the page to avoid collapsing an hugepage
 		 * currently in use by the VM.
 		 */
-		if (isolate_lru_page(page)) {
+		if (!isolate_lru_page(page)) {
 			unlock_page(page);
 			result = SCAN_DEL_PAGE_LRU;
 			goto out;

mm/memcontrol.c +2 -2

index 17335459d8dc..e8fd42be5fab 100644
@@ -6176,7 +6176,7 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
 		target_type = get_mctgt_type_thp(vma, addr, *pmd, &target);
 		if (target_type == MC_TARGET_PAGE) {
 			page = target.page;
-			if (!isolate_lru_page(page)) {
+			if (isolate_lru_page(page)) {
 				if (!mem_cgroup_move_account(page, true,
 							     mc.from, mc.to)) {
 					mc.precharge -= HPAGE_PMD_NR;

@@ -6226,7 +6226,7 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
 			 */
 			if (PageTransCompound(page))
 				goto put;
-			if (!device && isolate_lru_page(page))
+			if (!device && !isolate_lru_page(page))
 				goto put;
 			if (!mem_cgroup_move_account(page, false,
 						mc.from, mc.to)) {

mm/memory-failure.c +2 -2

index db85c2d37f70..e504362fdb23 100644
@@ -846,7 +846,7 @@ static const char * const action_page_types[] = {
  */
 static int delete_from_lru_cache(struct page *p)
 {
-	if (!isolate_lru_page(p)) {
+	if (isolate_lru_page(p)) {
 		/*
 		 * Clear sensible page flags, so that the buddy system won't
 		 * complain when the page is unpoison-and-freed.

@@ -2513,7 +2513,7 @@ static bool isolate_page(struct page *page, struct list_head *pagelist)
 		bool lru = !__PageMovable(page);
 
 		if (lru)
-			isolated = !isolate_lru_page(page);
+			isolated = isolate_lru_page(page);
 		else
 			isolated = !isolate_movable_page(page,
 							 ISOLATE_UNEVICTABLE);

mm/memory_hotplug.c +5 -3

index a1e8c3e9ab08..5fc2dcf4e3ab 100644
@@ -1632,6 +1632,7 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 
 	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
 		struct folio *folio;
+		bool isolated;
 
 		if (!pfn_valid(pfn))
 			continue;

@@ -1667,9 +1668,10 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		 * We can skip free pages. And we can deal with pages on
 		 * LRU and non-lru movable pages.
 		 */
-		if (PageLRU(page))
-			ret = isolate_lru_page(page);
-		else
+		if (PageLRU(page)) {
+			isolated = isolate_lru_page(page);
+			ret = isolated ? 0 : -EBUSY;
+		} else
 			ret = isolate_movable_page(page, ISOLATE_UNEVICTABLE);
 		if (!ret) { /* Success */
 			list_add_tail(&page->lru, &source);

mm/migrate.c +6 -3

index ef68a1aff35c..53010a142e7f 100644
@@ -2132,11 +2132,14 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
 		}
 	} else {
 		struct page *head;
+		bool isolated;
 
 		head = compound_head(page);
-		err = isolate_lru_page(head);
-		if (err)
+		isolated = isolate_lru_page(head);
+		if (!isolated) {
+			err = -EBUSY;
 			goto out_putpage;
+		}
 
 		err = 1;
 		list_add_tail(&head->lru, pagelist);

@@ -2541,7 +2544,7 @@ static int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 		return 0;
 	}
 
-	if (isolate_lru_page(page))
+	if (!isolate_lru_page(page))
 		return 0;
 
 	mod_node_page_state(page_pgdat(page), NR_ISOLATED_ANON + page_is_file_lru(page),

mm/migrate_device.c +1 -1

index 6c3740318a98..d30c9de60b0d 100644
@@ -388,7 +388,7 @@ static unsigned long migrate_device_unmap(unsigned long *src_pfns,
 				allow_drain = false;
 			}
 
-			if (isolate_lru_page(page)) {
+			if (!isolate_lru_page(page)) {
 				src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
 				restore++;
 				continue;

Signature · 1 line

2.27.0

Matthew Wilcox

Reviewed-by on v3 2/4

2023-02-15 15:39 UTC #4 lore raw

Re: [PATCH v3 2/4] mm: change to return bool for isolate_lru_page()

Quoted 6 lines · 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().

Quoted 6 lines

Also convert all users' logic of checking the isolation state.
No functional changes intended.
Signed-off-by Baolin Wang <baolin.wang-KPsoFbNs7GizrGE5bRqYAgC/[email protected]>
Acked-by David Hildenbrand <[email protected]>

Reviewed-by Matthew Wilcox (Oracle) <[email protected]>

Baolin Wang

author reviewer

2023-02-15 10:39 UTC #5 lore raw

PATCH v3 3/4 mm: hugetlb: change to return bool for isolate_hugetlb()

In reply to 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]>

Acked-by David Hildenbrand <[email protected]>

---
 include/linux/hugetlb.h |  6 +++---
 mm/hugetlb.c            | 13 ++++++++-----
 mm/memory-failure.c     |  2 +-
 mm/mempolicy.c          |  2 +-
 mm/migrate.c            |  7 +++----
 5 files changed, 16 insertions(+), 14 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 +8 -5

index 3a01a9dbf445..16513cd23d5d 100644
@@ -2925,13 +2925,16 @@ static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
 		 */
 		goto free_new;
 	} else if (folio_ref_count(old_folio)) {
+		bool isolated;
+
 		/*
 		 * Someone has grabbed the folio, try to isolate it here.
 		 * Fail with -EBUSY if not possible.
 		 */
 		spin_unlock_irq(&hugetlb_lock);
-		ret = isolate_hugetlb(old_folio, list);
+		isolated = isolate_hugetlb(old_folio, list);
 		spin_lock_irq(&hugetlb_lock);
+		ret = isolated ? 0 : -EBUSY;
 		goto free_new;
 	} else if (!folio_test_hugetlb_freed(old_folio)) {
 		/*

@@ -3005,7 +3008,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 +7254,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 +3 -4

index 53010a142e7f..2db546a0618c 100644
@@ -2095,6 +2095,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
 	struct vm_area_struct *vma;
 	struct page *page;
 	int err;
+	bool isolated;
 
 	mmap_read_lock(mm);
 	err = -EFAULT;

@@ -2126,13 +2127,11 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
 
 	if (PageHuge(page)) {
 		if (PageHead(page)) {
-			err = isolate_hugetlb(page_folio(page), pagelist);
-			if (!err)
-				err = 1;
+			isolated = isolate_hugetlb(page_folio(page), pagelist);
+			err = isolated ? 1 : -EBUSY;
 		}
 	} else {
 		struct page *head;
-		bool isolated;
 
 		head = compound_head(page);
 		isolated = isolate_lru_page(head);

Signature · 1 line

2.27.0

Matthew Wilcox

Reviewed-by on v3 3/4

2023-02-15 15:41 UTC #6 lore raw

Re: [PATCH v3 3/4] mm: hugetlb: change to return bool for isolate_hugetlb()

Quoted 10 lines · 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]>
Acked-by David Hildenbrand <[email protected]>

Reviewed-by Matthew Wilcox (Oracle) <[email protected]>

Mike Kravetz

Reviewed-by on v3 3/4

2023-02-15 19:22 UTC #7 lore raw

Re: [PATCH v3 3/4] mm: hugetlb: change to return bool for isolate_hugetlb()

In reply to Baolin Wang

Quoted 10 lines · 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]>
Acked-by David Hildenbrand <[email protected]>

Quoted 7 lines

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

Thanks,

Reviewed-by Mike Kravetz <[email protected]>

Signature · 1 line

Mike Kravetz

SeongJae Park

maintainer

2023-02-15 20:25 UTC #8 lore raw

Re: [PATCH v3 3/4] mm: hugetlb: change to return bool for isolate_hugetlb()

In reply to Baolin Wang

Quoted 11 lines · On Wed, 15 Feb 2023 18:39:36 +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]>
Acked-by David Hildenbrand <[email protected]>
---

[...]

Quoted 7 lines

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

[...]

Quoted 10 lines

mm/hugetlb.c +2 -0

index 3a01a9dbf445..16513cd23d5d 100644
@@ -2925,13 +2925,16 @@ static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
 		 */
 		goto free_new;
 	} else if (folio_ref_count(old_folio)) {
+		bool isolated;
+

mm/hugetlb.c static int alloc_and_dissolve_hugetlb_folio(struct hstate *h, · line 2938

Quoted 10 lines

mm/hugetlb.c +2 -1

 		/*
 		 * Someone has grabbed the folio, try to isolate it here.
 		 * Fail with -EBUSY if not possible.
 		 */
 		spin_unlock_irq(&hugetlb_lock);
-		ret = isolate_hugetlb(old_folio, list);
+		isolated = isolate_hugetlb(old_folio, list);
 		spin_lock_irq(&hugetlb_lock);
+		ret = isolated ? 0 : -EBUSY;
 		goto free_new;

Nit. I'd personally prefer to set 'ret' before entering this critical section to keep the section short, but this would be just a mean comment that wouldn't worth request respin.

Thanks,
SJ

[...]

Baolin Wang

author reviewer

2023-02-16 02:04 UTC #9 lore raw

Re: [PATCH v3 3/4] mm: hugetlb: change to return bool for isolate_hugetlb()

Quoted 38 lines · SeongJae Park

Quoted 11 lines · On Wed, 15 Feb 2023 18:39:36 +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]>
Acked-by David Hildenbrand <[email protected]>
---
[...]
Quoted 7 lines
  include/linux/hugetlb.h |  6 +++---
  mm/hugetlb.c            | 13 ++++++++-----
  mm/memory-failure.c     |  2 +-
  mm/mempolicy.c          |  2 +-
  mm/migrate.c            |  7 +++----
  5 files changed, 16 insertions(+), 14 deletions(-)
[...]
Quoted 16 lines
mm/hugetlb.c +2 -1
index 3a01a9dbf445..16513cd23d5d 100644
@@ -2925,13 +2925,16 @@ static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
  		 */
  		goto free_new;
  	} else if (folio_ref_count(old_folio)) {
+		bool isolated;
+
  		/*
  		 * Someone has grabbed the folio, try to isolate it here.
  		 * Fail with -EBUSY if not possible.
  		 */
  		spin_unlock_irq(&hugetlb_lock);
-		ret = isolate_hugetlb(old_folio, list);

Quoted 8 lines

mm/hugetlb.c static int alloc_and_dissolve_hugetlb_folio(struct hstate *h, · line 2938
Quoted 4 lines
mm/hugetlb.c +2 -0
+		isolated = isolate_hugetlb(old_folio, list);
  		spin_lock_irq(&hugetlb_lock);
+		ret = isolated ? 0 : -EBUSY;
  		goto free_new;
Nit. I'd personally prefer to set 'ret' before entering this critical section to keep the section short, but this would be just a mean comment that wouldn't worth request respin.

Yes, good catch. And I see Andrew has helped to do this (Thanks Andrew).

Thanks for reviewing.

Baolin Wang

author reviewer

2023-02-15 10:39 UTC #10 lore raw

PATCH v3 4/4 mm: change to return bool for isolate_movable_page()

In reply to Baolin Wang

Now the isolate_movable_page() can only return 0 or -EBUSY, and no users will care about the negative return value, thus we can convert the isolate_movable_page() to return a boolean value to make the code more clear when checking the movable page isolation state.

No functional changes intended.

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

Acked-by David Hildenbrand <[email protected]>

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

include/linux/migrate.h +3 -3

index c88b96b48be7..6b252f519c86 100644
@@ -71,7 +71,7 @@ extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
 		unsigned long private, enum migrate_mode mode, int reason,
 		unsigned int *ret_succeeded);
 extern struct page *alloc_migration_target(struct page *page, unsigned long private);
-extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
+extern bool isolate_movable_page(struct page *page, isolate_mode_t mode);
 
 int migrate_huge_page_move_mapping(struct address_space *mapping,
 		struct folio *dst, struct folio *src);

@@ -92,8 +92,8 @@ static inline int migrate_pages(struct list_head *l, new_page_t new,
 static inline struct page *alloc_migration_target(struct page *page,
 		unsigned long private)
 	{ return NULL; }
-static inline int isolate_movable_page(struct page *page, isolate_mode_t mode)
-	{ return -EBUSY; }
+static inline bool isolate_movable_page(struct page *page, isolate_mode_t mode)
+	{ return false; }
 
 static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
 				  struct folio *dst, struct folio *src)

mm/compaction.c +1 -1

index d73578af44cc..ad7409f70519 100644
@@ -976,7 +976,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 					locked = NULL;
 				}
 
-				if (!isolate_movable_page(page, mode))
+				if (isolate_movable_page(page, mode))
 					goto isolate_success;
 			}

mm/memory-failure.c +2 -2

index 8604753bc644..a1ede7bdce95 100644
@@ -2515,8 +2515,8 @@ static bool isolate_page(struct page *page, struct list_head *pagelist)
 		if (lru)
 			isolated = isolate_lru_page(page);
 		else
-			isolated = !isolate_movable_page(page,
-							 ISOLATE_UNEVICTABLE);
+			isolated = isolate_movable_page(page,
+							ISOLATE_UNEVICTABLE);
 
 		if (isolated) {
 			list_add(&page->lru, pagelist);

mm/memory_hotplug.c +5 -5

index 5fc2dcf4e3ab..bcb0dc41c2f2 100644
@@ -1668,18 +1668,18 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		 * We can skip free pages. And we can deal with pages on
 		 * LRU and non-lru movable pages.
 		 */
-		if (PageLRU(page)) {
+		if (PageLRU(page))
 			isolated = isolate_lru_page(page);
-			ret = isolated ? 0 : -EBUSY;
-		} else
-			ret = isolate_movable_page(page, ISOLATE_UNEVICTABLE);
-		if (!ret) { /* Success */
+		else
+			isolated = isolate_movable_page(page, ISOLATE_UNEVICTABLE);
+		if (isolated) { /* Success */
 			list_add_tail(&page->lru, &source);
 			if (!__PageMovable(page))
 				inc_node_page_state(page, NR_ISOLATED_ANON +
 						    page_is_file_lru(page));
 
 		} else {
+			ret = -EBUSY;
 			if (__ratelimit(&migrate_rs)) {
 				pr_warn("failed to isolate pfn %lx\n", pfn);
 				dump_page(page, "isolation failed");

mm/migrate.c +3 -3

index 2db546a0618c..9a101c7bb8ff 100644
@@ -58,7 +58,7 @@
 
 #include "internal.h"
 
-int isolate_movable_page(struct page *page, isolate_mode_t mode)
+bool isolate_movable_page(struct page *page, isolate_mode_t mode)
 {
 	struct folio *folio = folio_get_nontail_page(page);
 	const struct movable_operations *mops;

@@ -119,14 +119,14 @@ int isolate_movable_page(struct page *page, isolate_mode_t mode)
 	folio_set_isolated(folio);
 	folio_unlock(folio);
 
-	return 0;
+	return true;
 
 out_no_isolated:
 	folio_unlock(folio);
 out_putfolio:
 	folio_put(folio);
 out:
-	return -EBUSY;
+	return false;
 }
 
 static void putback_movable_folio(struct folio *folio)

Signature · 1 line

2.27.0

Matthew Wilcox

Reviewed-by on v3 4/4

2023-02-15 15:44 UTC #11 lore raw

Re: [PATCH v3 4/4] mm: change to return bool for isolate_movable_page()

Quoted 9 lines · Baolin Wang

Now the isolate_movable_page() can only return 0 or -EBUSY, and no users will care about the negative return value, thus we can convert the isolate_movable_page() to return a boolean value to make the code more clear when checking the movable page isolation state.
No functional changes intended.
Signed-off-by Baolin Wang <baolin.wang-KPsoFbNs7GizrGE5bRqYAgC/[email protected]>
Acked-by David Hildenbrand <[email protected]>

Reviewed-by Matthew Wilcox (Oracle) <[email protected]>

A couple of nits below, not worth respinning the patch series for:

include/linux/migrate.h extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free, · line 74

Quoted 10 lines

include/linux/migrate.h +1 -1

index c88b96b48be7..6b252f519c86 100644
@@ -71,7 +71,7 @@ extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
 		unsigned long private, enum migrate_mode mode, int reason,
 		unsigned int *ret_succeeded);
 extern struct page *alloc_migration_target(struct page *page, unsigned long private);
-extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
+extern bool isolate_movable_page(struct page *page, isolate_mode_t mode);

You can drop the 'extern' here.

Quoted 5 lines

include/linux/migrate.h +1 -0

+++ b/mm/memory_hotplug.c

@@ -1668,18 +1668,18 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 		 * We can skip free pages. And we can deal with pages on
 		 * LRU and non-lru movable pages.
 		 */

include/linux/migrate.h do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) · line 1675

Quoted 10 lines

include/linux/migrate.h +4 -5

-		if (PageLRU(page)) {
+		if (PageLRU(page))
 			isolated = isolate_lru_page(page);
-			ret = isolated ? 0 : -EBUSY;
-		} else
-			ret = isolate_movable_page(page, ISOLATE_UNEVICTABLE);
-		if (!ret) { /* Success */
+		else
+			isolated = isolate_movable_page(page, ISOLATE_UNEVICTABLE);
+		if (isolated) { /* Success */

I would have dropped the "/* Success */" here. Before, commenting "!ret" is quite sensible, but "isolated" seems obviously success to me.

Thanks for doing all this.

Baolin Wang

author reviewer

2023-02-16 02:07 UTC #12 lore raw

Re: [PATCH v3 4/4] mm: change to return bool for isolate_movable_page()

Quoted 38 lines · Matthew Wilcox

Quoted 9 lines · Baolin Wang
Now the isolate_movable_page() can only return 0 or -EBUSY, and no users will care about the negative return value, thus we can convert the isolate_movable_page() to return a boolean value to make the code more clear when checking the movable page isolation state.
No functional changes intended.
Signed-off-by Baolin Wang <baolin.wang-KPsoFbNs7GizrGE5bRqYAgC/[email protected]>
Acked-by David Hildenbrand <[email protected]>
Reviewed-by Matthew Wilcox (Oracle) <[email protected]>
A couple of nits below, not worth respinning the patch series for:
include/linux/migrate.h extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free, · line 74
Quoted 10 lines
include/linux/migrate.h +1 -1
index c88b96b48be7..6b252f519c86 100644
@@ -71,7 +71,7 @@ extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
  		unsigned long private, enum migrate_mode mode, int reason,
  		unsigned int *ret_succeeded);
  extern struct page *alloc_migration_target(struct page *page, unsigned long private);
-extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
+extern bool isolate_movable_page(struct page *page, isolate_mode_t mode);
You can drop the 'extern' here.
Quoted 10 lines
include/linux/migrate.h +2 -3
+++ b/mm/memory_hotplug.c
@@ -1668,18 +1668,18 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
  		 * We can skip free pages. And we can deal with pages on
  		 * LRU and non-lru movable pages.
  		 */
-		if (PageLRU(page)) {
+		if (PageLRU(page))
  			isolated = isolate_lru_page(page);
-			ret = isolated ? 0 : -EBUSY;
-		} else

Quoted 8 lines

include/linux/migrate.h do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) · line 1675
Quoted 5 lines
include/linux/migrate.h +3 -2
-			ret = isolate_movable_page(page, ISOLATE_UNEVICTABLE);
-		if (!ret) { /* Success */
+		else
+			isolated = isolate_movable_page(page, ISOLATE_UNEVICTABLE);
+		if (isolated) { /* Success */
I would have dropped the "/* Success */" here. Before, commenting "!ret" is quite sensible, but "isolated" seems obviously success to me.

Right. Hope Andrew can help to drop this unnecessary comment:)

Thanks for reviewing.

Andrew Morton

maintainer

2023-02-16 22:46 UTC #13 lore raw

Re: [PATCH v3 4/4] mm: change to return bool for isolate_movable_page()

In reply to Matthew Wilcox

Quoted 5 lines · On Wed, 15 Feb 2023 15:44:22 +0000 Matthew Wilcox…

Review
Quoted 3 lines
extern struct page *alloc_migration_target(struct page *page, unsigned long private);
(quoted diff) +1 -1
-extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
+extern bool isolate_movable_page(struct page *page, isolate_mode_t mode);
You can drop the 'extern' here.

There are a bunch of them, so a separate patch would be better.

include/linux/migrate.h renamed from include/linux/migrate.h~a +8 -8

@@ -62,16 +62,16 @@ extern const char *migrate_reason_names[
 
 #ifdef CONFIG_MIGRATION
 
-extern void putback_movable_pages(struct list_head *l);
+void putback_movable_pages(struct list_head *l);
 int migrate_folio_extra(struct address_space *mapping, struct folio *dst,
 		struct folio *src, enum migrate_mode mode, int extra_count);
 int migrate_folio(struct address_space *mapping, struct folio *dst,
 		struct folio *src, enum migrate_mode mode);
-extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
-		unsigned long private, enum migrate_mode mode, int reason,
-		unsigned int *ret_succeeded);
-extern struct page *alloc_migration_target(struct page *page, unsigned long private);
-extern bool isolate_movable_page(struct page *page, isolate_mode_t mode);
+int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
+		  unsigned long private, enum migrate_mode mode, int reason,
+		  unsigned int *ret_succeeded);
+struct page *alloc_migration_target(struct page *page, unsigned long private);
+bool isolate_movable_page(struct page *page, isolate_mode_t mode);
 
 int migrate_huge_page_move_mapping(struct address_space *mapping,
 		struct folio *dst, struct folio *src);

@@ -142,8 +142,8 @@ const struct movable_operations *page_mo
 }
 
 #ifdef CONFIG_NUMA_BALANCING
-extern int migrate_misplaced_page(struct page *page,
-				  struct vm_area_struct *vma, int node);
+int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
+			   int node);
 #else
 static inline int migrate_misplaced_page(struct page *page,
 					 struct vm_area_struct *vma, int node)

Linus Torvalds

2023-02-15 20:14 UTC #14 lore raw

In reply to Baolin Wang

This v3 series looks like it's making things more readable, so ack as far as I'm concerned.

But it looks like it's firmly in the "Andrew's mm tree" category, so I'll leave it up to him to decide.

Linus

SeongJae Park

maintainer Reviewed-by on the v3 series

2023-02-15 20:26 UTC #15 lore raw

In reply to Baolin Wang

Hi Baolin,

Quoted 7 lines · On Wed, 15 Feb 2023 18:39:33 +0800 Baolin Wang <[email protected]>…

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))

Quoted 8 lines

        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.

For the series,

Reviewed-by SeongJae Park <[email protected]>

Thanks,
SJ

[...]