mm/gup.c: Simplify and fix check_and_migrate_movable_pages() return codes

When pinning pages with FOLL_LONGTERM check_and_migrate_movable_pages() is called to migrate pages out of zones which should not contain any longterm pinned pages.

When migration succeeds all pages will have been unpinned so pinning needs to be retried. This is indicated by returning zero. When all pages are in the correct zone the number of pinned pages is returned.

However migration can also fail, in which case pages are unpinned and -ENOMEM is returned. However if the failure was due to not being unable to isolate a page zero is returned. This leads to indefinite looping in __gup_longterm_locked().

Fix this by simplifying the return codes such that zero indicates all pages were successfully pinned in the correct zone while errors indicate either pages were migrated and pinning should be retried or that migration has failed and therefore the pinning operation should fail.

Signed-off-by Alistair Popple <[email protected]>

---
 mm/gup.c | 46 +++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

(quoted diff) +0 -0

@@ -1980,19 +1980,18 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 				    folio_nr_pages(folio));
 	}
 

I came up with this ontop:

Quoted 2 lines · Jason Gunthorpe

I came up with this ontop:

That cleans it up even more, looks nice. I'd go just a touch further, and also (unless there is some odd reason?) stay with -EAGAIN rather than -EBUSY, because otherwise both the function's comment header, and the caller, should change from -EBUSY to -EAGAIN just for consistency.

And also because the way it's used: the caller is literally "trying again".

So on top of the ontop:

mm/gup.c +9 -7

index 43c1fc532842..5f04033ee0ed 100644
@@ -1901,10 +1901,12 @@ struct page *get_dump_page(unsigned long addr)
 
 #ifdef CONFIG_MIGRATION
 /*
- * Check whether all pages are pinnable. If some pages are not pinnable migrate
- * them and unpin all the pages. Returns -EAGAIN if pages were unpinned or zero
- * if all pages are pinnable and in the right zone. Other errors indicate
- * migration failure.
+ * Check whether all pages are pinnable. If some pages are not pinnable, migrate
+ * them and unpin all the pages.
+ * Return values:
+ *       0:      all pages are already pinnable and in the right zone
+ *      -EAGAIN: some pages were unpinned or were zero
+ *      -ENOMEM: migration of some pages failed
  */
 static long check_and_migrate_movable_pages(unsigned long nr_pages,
 					    struct page **pages,

@@ -1914,11 +1916,11 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 		.nid = NUMA_NO_NODE,
 		.gfp_mask = GFP_USER | __GFP_NOWARN,
 	};
-	unsigned long i;
+	unsigned long i, not_migrated;
 	struct folio *prev_folio = NULL;
 	LIST_HEAD(movable_page_list);
 	bool drain_allow = true, coherent_pages = false;
-	int ret = -EBUSY;
+	int ret = -EAGAIN;
 
 	for (i = 0; i < nr_pages; i++) {
 		struct folio *folio = page_folio(pages[i]);

@@ -1990,7 +1992,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 
 	/*
 	 * Unpin all pages. If device coherent pages were found
-	 * migrate_deivce_coherent_page() will have dropped the pin and set
+	 * migrate_device_coherent_page() will have dropped the pin and set
 	 * pages[i] == NULL.
 	 */
 	for (i = 0; i < nr_pages; i++) {

thanks,

On Fri, Jul 29, 2022 at 12:46:45PM +1000, Alistair Popple wrote:

I have to say I prefer the usual style where all the places that error exit do 'goto error' instead of trying to keep track in 'ret'

AFAICT there is no reason to 'continue' in most of these paths since we intend to return to userspace with an error anyhow? Why try to isolate more pages?

Quoted 4 lines

(quoted diff) +0 -0

@@ -1980,19 +1980,18 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 				    folio_nr_pages(folio));
 	}
 

Review static long check_and_migrate_movable_pages(unsigned long nr_pages, · line 1987

Quoted 10 lines

(quoted diff) +2 -5

-	if (!list_empty(&movable_page_list) || isolation_error_count
-		|| coherent_pages)
-		goto unpin_pages;
-
 	/*
 	 * If list is empty, and no isolation errors, means that all pages are
-	 * in the correct zone.
+	 * in the correct zone. If there were device coherent pages some pages
+	 * have been unpinned.
 	 */

That comment is a bit confusing.. I guess it is trying to explain why coherent_pages is doing?

Maybe just:

All the given pages are fine, nothing was done

Review static long check_and_migrate_movable_pages(unsigned long nr_pages, · line 1988

Quoted 1 line

(quoted diff) +1 -0

+	if (list_empty(&movable_page_list) && !ret && !coherent_pages)

Review static long check_and_migrate_movable_pages(unsigned long nr_pages, · line 1992

Quoted 3 lines

(quoted diff) +1 -1

+		return 0;
 
-unpin_pages:

Now that this label is removed this if following it

	if (!list_empty(&movable_page_list)) {

is also now unneeded because the above 'return 0' already checked it

I came up with this ontop:

mm/gup.c +21 -25

index 9e7c76d1e4ee3c..eddcf3c0eba727 100644
@@ -1912,11 +1912,15 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 					    struct page **pages,
 					    unsigned int gup_flags)
 {
+	struct migration_target_control mtc = {
+		.nid = NUMA_NO_NODE,
+		.gfp_mask = GFP_USER | __GFP_NOWARN,
+	};
 	unsigned long i;
 	struct folio *prev_folio = NULL;
 	LIST_HEAD(movable_page_list);
 	bool drain_allow = true, coherent_pages = false;
-	int ret = 0;
+	int ret = -EBUSY;
 
 	for (i = 0; i < nr_pages; i++) {
 		struct folio *folio = page_folio(pages[i]);

@@ -1948,10 +1952,8 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 				unpin_user_page(&folio->page);
 			}
 
-			if (migrate_device_coherent_page(&folio->page)) {
-				ret = -EBUSY;
-				break;
-			}
+			if (migrate_device_coherent_page(&folio->page))
+				goto error;
 			continue;
 		}
 

@@ -1963,7 +1965,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 		if (folio_test_hugetlb(folio)) {
 			if (isolate_hugetlb(&folio->page,
 						&movable_page_list))
-				ret = -EBUSY;
+				goto error;
 			continue;
 		}
 

@@ -1972,10 +1974,8 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 			drain_allow = false;
 		}
 
-		if (folio_isolate_lru(folio)) {
-			ret = -EBUSY;
-			continue;
-		}
+		if (folio_isolate_lru(folio))
+			goto error;
 		list_add_tail(&folio->lru, &movable_page_list);
 		node_stat_mod_folio(folio,
 				    NR_ISOLATED_ANON + folio_is_file_lru(folio),

@@ -1987,7 +1987,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 	 * in the correct zone. If there were device coherent pages some pages
 	 * have been unpinned.
 	 */
-	if (list_empty(&movable_page_list) && !ret && !coherent_pages)
+	if (list_empty(&movable_page_list) && !coherent_pages)
 		return 0;
 
 	/*

@@ -2005,23 +2005,19 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 			put_page(pages[i]);
 	}
 
-	if (!list_empty(&movable_page_list)) {
-		struct migration_target_control mtc = {
-			.nid = NUMA_NO_NODE,
-			.gfp_mask = GFP_USER | __GFP_NOWARN,
-		};
-
-		ret = migrate_pages(&movable_page_list, alloc_migration_target,
-				    NULL, (unsigned long)&mtc, MIGRATE_SYNC,
-				    MR_LONGTERM_PIN, NULL);
-		if (ret > 0) /* number of pages not migrated */
-			ret = -ENOMEM;
+	not_migrated = migrate_pages(&movable_page_list, alloc_migration_target,
+				     NULL, (unsigned long)&mtc, MIGRATE_SYNC,
+				     MR_LONGTERM_PIN, NULL);
+	if (not_migrated > 0) {
+		ret = -ENOMEM;
+		goto error;
 	}
+	return -EAGAIN;
 
-	if (ret && !list_empty(&movable_page_list))
+error:
+	if (!list_empty(&movable_page_list))
 		putback_movable_pages(&movable_page_list);
-
-	return ret ? ret : -EAGAIN;
+	return ret;
 }
 #else
 static long check_and_migrate_movable_pages(unsigned long nr_pages,

Quoted 1 line · Jason Gunthorpe

On Fri, Jul 29, 2022 at 12:46:45PM +1000, Alistair Popple wrote:

[...]

Quoted 2 lines

I have to say I prefer the usual style where all the places that error exit do 'goto error' instead of trying to keep track in 'ret'

Ok. Part of the complexity was my understanding from the documentation for migrate_pages() is that putback_movable_pages() should only be called if migrate_pages() != 0:

* It is caller's responsibility to call putback_movable_pages() to return pages
* to the LRU or free list only if ret != 0.

But I think it should be fine to do regardless, because on success the pages will be deleted from movable_page_list. Eg. From unmap_and_move():

	if (rc != -EAGAIN) {
		/*
		 * A page that has been migrated has all references
		 * removed and will be freed. A page that has not been
		 * migrated will have kept its references and be restored.
		 */
		list_del(&page->lru);
	}

So will post a v2 doing this.

Quoted 3 lines

AFAICT there is no reason to 'continue' in most of these paths since we intend to return to userspace with an error anyhow? Why try to isolate more pages?

The main reason would be if callers want to retry the operation. AFAIK isolate_folio_lru() can have transient failures, so if callers want to retry it makes sense to isolate and migrate as many pages as possible rather than one page at a time as subsequent retries may find different pages that can't be isolated.

Actually I should have called this out more clearly - the previous behaviour on isolation failure was to retry indefinitely which is what lead to looping in the kernel. This patch turns isolation failure into an error and doesn't retry. I wonder though if we need to maintain a retry count similar to what migrate_pages() does if there are unexpected page refs?

Quoted 18 lines

Quoted 4 lines

(quoted diff) +0 -0

@@ -1980,19 +1980,18 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 				    folio_nr_pages(folio));
 	}
 

Review static long check_and_migrate_movable_pages(unsigned long nr_pages, · line 1987

Quoted 10 lines

(quoted diff) +2 -5

-	if (!list_empty(&movable_page_list) || isolation_error_count
-		|| coherent_pages)
-		goto unpin_pages;
-
 	/*
 	 * If list is empty, and no isolation errors, means that all pages are
-	 * in the correct zone.
+	 * in the correct zone. If there were device coherent pages some pages
+	 * have been unpinned.
 	 */

That comment is a bit confusing.. I guess it is trying to explain why coherent_pages is doing?

Quoted 3 lines

Maybe just:

All the given pages are fine, nothing was done

Ok.

Quoted 1 line

Review static long check_and_migrate_movable_pages(unsigned long nr_pages, · line 1988

Quoted 1 line

(quoted diff) +1 -0

+	if (list_empty(&movable_page_list) && !ret && !coherent_pages)

Actually I think we can drop the coherent_pages variable too. At this point coherent_pages will either be in the correct zone or we will have jumped to the error label.

Quoted 11 lines

Review static long check_and_migrate_movable_pages(unsigned long nr_pages, · line 1992

Quoted 3 lines

(quoted diff) +1 -1

+		return 0;
 
-unpin_pages:

Now that this label is removed this if following it

	if (!list_empty(&movable_page_list)) {

is also now unneeded because the above 'return 0' already checked it

I came up with this ontop:

Thanks for the suggestions.

- Alistair

Quoted 99 lines

mm/gup.c +21 -25

index 9e7c76d1e4ee3c..eddcf3c0eba727 100644
@@ -1912,11 +1912,15 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 					    struct page **pages,
 					    unsigned int gup_flags)
 {
+	struct migration_target_control mtc = {
+		.nid = NUMA_NO_NODE,
+		.gfp_mask = GFP_USER | __GFP_NOWARN,
+	};
 	unsigned long i;
 	struct folio *prev_folio = NULL;
 	LIST_HEAD(movable_page_list);
 	bool drain_allow = true, coherent_pages = false;
-	int ret = 0;
+	int ret = -EBUSY;
 
 	for (i = 0; i < nr_pages; i++) {
 		struct folio *folio = page_folio(pages[i]);

@@ -1948,10 +1952,8 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 				unpin_user_page(&folio->page);
 			}
 
-			if (migrate_device_coherent_page(&folio->page)) {
-				ret = -EBUSY;
-				break;
-			}
+			if (migrate_device_coherent_page(&folio->page))
+				goto error;
 			continue;
 		}
 

@@ -1963,7 +1965,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 		if (folio_test_hugetlb(folio)) {
 			if (isolate_hugetlb(&folio->page,
 						&movable_page_list))
-				ret = -EBUSY;
+				goto error;
 			continue;
 		}
 

@@ -1972,10 +1974,8 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 			drain_allow = false;
 		}
 
-		if (folio_isolate_lru(folio)) {
-			ret = -EBUSY;
-			continue;
-		}
+		if (folio_isolate_lru(folio))
+			goto error;
 		list_add_tail(&folio->lru, &movable_page_list);
 		node_stat_mod_folio(folio,
 				    NR_ISOLATED_ANON + folio_is_file_lru(folio),

@@ -1987,7 +1987,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 	 * in the correct zone. If there were device coherent pages some pages
 	 * have been unpinned.
 	 */
-	if (list_empty(&movable_page_list) && !ret && !coherent_pages)
+	if (list_empty(&movable_page_list) && !coherent_pages)
 		return 0;
 
 	/*

@@ -2005,23 +2005,19 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 			put_page(pages[i]);
 	}
 
-	if (!list_empty(&movable_page_list)) {
-		struct migration_target_control mtc = {
-			.nid = NUMA_NO_NODE,
-			.gfp_mask = GFP_USER | __GFP_NOWARN,
-		};
-
-		ret = migrate_pages(&movable_page_list, alloc_migration_target,
-				    NULL, (unsigned long)&mtc, MIGRATE_SYNC,
-				    MR_LONGTERM_PIN, NULL);
-		if (ret > 0) /* number of pages not migrated */
-			ret = -ENOMEM;
+	not_migrated = migrate_pages(&movable_page_list, alloc_migration_target,
+				     NULL, (unsigned long)&mtc, MIGRATE_SYNC,
+				     MR_LONGTERM_PIN, NULL);
+	if (not_migrated > 0) {
+		ret = -ENOMEM;
+		goto error;
 	}
+	return -EAGAIN;
 
-	if (ret && !list_empty(&movable_page_list))
+error:
+	if (!list_empty(&movable_page_list))
 		putback_movable_pages(&movable_page_list);
-
-	return ret ? ret : -EAGAIN;
+	return ret;
 }
 #else
 static long check_and_migrate_movable_pages(unsigned long nr_pages,

Quoted 3 lines

AFAICT there is no reason to 'continue' in most of these paths since we intend to return to userspace with an error anyhow? Why try to isolate more pages?

The main reason would be if callers want to retry the operation. AFAIK isolate_folio_lru() can have transient failures, so if callers want to retry it makes sense to isolate and migrate as many pages as possible rather than one page at a time as subsequent retries may find different pages that can't be isolated.

Actually I should have called this out more clearly - the previous behaviour on isolation failure was to retry indefinitely which is what lead to looping in the kernel. This patch turns isolation failure into an error and doesn't retry. I wonder though if we need to maintain a retry count similar to what migrate_pages() does if there are unexpected page refs?

Quoted 4 lines · Alistair Popple

Quoted 3 lines

AFAICT there is no reason to 'continue' in most of these paths since we intend to return to userspace with an error anyhow? Why try to isolate more pages?

Quoted 5 lines

The main reason would be if callers want to retry the operation. AFAIK isolate_folio_lru() can have transient failures, so if callers want to retry it makes sense to isolate and migrate as many pages as possible rather than one page at a time as subsequent retries may find different pages that can't be isolated.

Except we don't try to do the migrate, we just isolate and then unisolate and return failure.

Quoted 6 lines

Actually I should have called this out more clearly - the previous behaviour on isolation failure was to retry indefinitely which is what lead to looping in the kernel. This patch turns isolation failure into an error and doesn't retry. I wonder though if we need to maintain a retry count similar to what migrate_pages() does if there are unexpected page refs?

This makes more sense, exporting this mess to the caller and hoping they retry (they won't) doesn't make sense..

Jason