[PATCH v4 1/2] mm/gup.c: Don't pass gup_flags to check_and_migrate_movable_pages()

[PATCH v4 1/2] mm/gup.c: Don't pass gup_flags to check_and_migrate_movable_pages()

[Alistair Popple]

gup_flags is passed to check_and_migrate_movable_pages() so that it can
call either put_page() or unpin_user_page() to drop the page reference.
However check_and_migrate_movable_pages() is only called for
FOLL_LONGTERM, which implies FOLL_PIN so there is no need to pass
gup_flags.

Signed-off-by: Alistair Popple <apopple@nvidia.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>

---

Changes for v3:

 - Move WARN_ON() out of loop
---
 mm/gup.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index c6d060d..a2baa8b 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1907,8 +1907,7 @@ struct page *get_dump_page(unsigned long addr)
  * Return negative error if migration fails.
  */
 static long check_and_migrate_movable_pages(unsigned long nr_pages,
-					    struct page **pages,
-					    unsigned int gup_flags)
+					    struct page **pages)
 {
 	unsigned long isolation_error_count = 0, i;
 	struct folio *prev_folio = NULL;
@@ -1941,10 +1940,8 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 			 * Migration will fail if the page is pinned, so convert
 			 * the pin on the source page to a normal reference.
 			 */
-			if (gup_flags & FOLL_PIN) {
-				get_page(&folio->page);
-				unpin_user_page(&folio->page);
-			}
+			get_page(&folio->page);
+			unpin_user_page(&folio->page);
 
 			ret = migrate_device_coherent_page(&folio->page);
 			if (ret)
@@ -1998,10 +1995,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 		if (!pages[i])
 			continue;
 
-		if (gup_flags & FOLL_PIN)
-			unpin_user_page(pages[i]);
-		else
-			put_page(pages[i]);
+		unpin_user_page(pages[i]);
 	}
 
 	if (!list_empty(&movable_page_list)) {
@@ -2023,8 +2017,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 }
 #else
 static long check_and_migrate_movable_pages(unsigned long nr_pages,
-					    struct page **pages,
-					    unsigned int gup_flags)
+					    struct page **pages)
 {
 	return nr_pages;
 }
@@ -2047,13 +2040,17 @@ static long __gup_longterm_locked(struct mm_struct *mm,
 	if (!(gup_flags & FOLL_LONGTERM))
 		return __get_user_pages_locked(mm, start, nr_pages, pages, vmas,
 					       NULL, gup_flags);
+	/* check_and_migrate_movable_pages() assumes pages have been pinned. */
+	if (WARN_ON(!(gup_flags & FOLL_PIN)))
+		return -EINVAL;
 	flags = memalloc_pin_save();
 	do {
 		rc = __get_user_pages_locked(mm, start, nr_pages, pages, vmas,
 					     NULL, gup_flags);
 		if (rc <= 0)
 			break;
-		rc = check_and_migrate_movable_pages(rc, pages, gup_flags);
+
+		rc = check_and_migrate_movable_pages(rc, pages);
 	} while (!rc);
 	memalloc_pin_restore(flags);
 

base-commit: 360614c01f81f48a89d8b13f8fa69c3ae0a1f5c7
-- 
git-series 0.9.1

[PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[Alistair Popple]

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. Migration can also fail, in which case the pages
will also have been unpinned but the operation should not be retried. If
all pages are in the correct zone nothing will be unpinned and no retry
is required.

The logic in check_and_migrate_movable_pages() tracks unnecessary state
and the return codes for each case are difficult to follow. Refactor the
code to clean this up. No behaviour change is intended.

Signed-off-by: Alistair Popple <apopple@nvidia.com>

---

Changes for v4:

 - Use folio directly instead of page based functions and folio->page.

Changes for v3:

 - Improved comments (thanks John).
 - Fix up inconsistent int/long/unsigned long.
 - Rename (migrate|collect)_unpinnable_pages
   to (migrate|collect)_longterm_unpinnable_pages() as suggested by David.

Changes for v2:

 - Split into different functions as suggested by John.
 - Made error handling more conventional as requested by Jason.

Originally posted as "mm/gup.c: Simplify and fix
check_and_migrate_movable_pages() return codes"[1].

Changes from that version:

 - Restore the original isolation failure behaviour and don't fail the
   pup. Instead retry indefinitely.
 - Unpin all pages on retry or failure rather than just failure.

[1] https://lore.kernel.org/linux-mm/814dee5d3aadd38c3370eaaf438ba7eee9bf9d2b.1659399696.git-series.apopple@nvidia.com/
---
 mm/gup.c | 166 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 97 insertions(+), 69 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index a2baa8b..b181d97 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1901,19 +1901,16 @@ struct page *get_dump_page(unsigned long addr)
 
 #ifdef CONFIG_MIGRATION
 /*
- * Check whether all pages are pinnable, if so return number of pages.  If some
- * pages are not pinnable, migrate them, and unpin all pages. Return zero if
- * pages were migrated, or if some pages were not successfully isolated.
- * Return negative error if migration fails.
+ * Returns the number of collected pages. Return value is always >= 0.
  */
-static long check_and_migrate_movable_pages(unsigned long nr_pages,
-					    struct page **pages)
+static unsigned long collect_longterm_unpinnable_pages(
+					struct list_head *movable_page_list,
+					unsigned long nr_pages,
+					struct page **pages)
 {
-	unsigned long isolation_error_count = 0, i;
+	unsigned long i, collected = 0;
 	struct folio *prev_folio = NULL;
-	LIST_HEAD(movable_page_list);
-	bool drain_allow = true, coherent_pages = false;
-	int ret = 0;
+	bool drain_allow = true;
 
 	for (i = 0; i < nr_pages; i++) {
 		struct folio *folio = page_folio(pages[i]);
@@ -1922,43 +1919,16 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 			continue;
 		prev_folio = folio;
 
-		/*
-		 * Device coherent pages are managed by a driver and should not
-		 * be pinned indefinitely as it prevents the driver moving the
-		 * page. So when trying to pin with FOLL_LONGTERM instead try
-		 * to migrate the page out of device memory.
-		 */
-		if (folio_is_device_coherent(folio)) {
-			/*
-			 * We always want a new GUP lookup with device coherent
-			 * pages.
-			 */
-			pages[i] = 0;
-			coherent_pages = true;
-
-			/*
-			 * Migration will fail if the page is pinned, so convert
-			 * the pin on the source page to a normal reference.
-			 */
-			get_page(&folio->page);
-			unpin_user_page(&folio->page);
+		if (folio_is_longterm_pinnable(folio))
+			continue;
 
-			ret = migrate_device_coherent_page(&folio->page);
-			if (ret)
-				goto unpin_pages;
+		collected++;
 
+		if (folio_is_device_coherent(folio))
 			continue;
-		}
 
-		if (folio_is_longterm_pinnable(folio))
-			continue;
-		/*
-		 * Try to move out any movable page before pinning the range.
-		 */
 		if (folio_test_hugetlb(folio)) {
-			if (isolate_hugetlb(&folio->page,
-						&movable_page_list))
-				isolation_error_count++;
+			isolate_hugetlb(&folio->page, movable_page_list);
 			continue;
 		}
 
@@ -1967,59 +1937,117 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 			drain_allow = false;
 		}
 
-		if (folio_isolate_lru(folio)) {
-			isolation_error_count++;
+		if (!folio_isolate_lru(folio))
 			continue;
-		}
-		list_add_tail(&folio->lru, &movable_page_list);
+
+		list_add_tail(&folio->lru, movable_page_list);
 		node_stat_mod_folio(folio,
 				    NR_ISOLATED_ANON + folio_is_file_lru(folio),
 				    folio_nr_pages(folio));
 	}
 
-	if (!list_empty(&movable_page_list) || isolation_error_count ||
-	    coherent_pages)
-		goto unpin_pages;
+	return collected;
+}
 
-	/*
-	 * If list is empty, and no isolation errors, means that all pages are
-	 * in the correct zone.
-	 */
-	return nr_pages;
+/*
+ * Unpins all pages and migrates device coherent pages and movable_page_list.
+ * Returns zero if all pages were successfully migrated or -errno for failure
+ * (or partial success).
+ */
+static int migrate_longterm_unpinnable_pages(
+					struct list_head *movable_page_list,
+					unsigned long nr_pages,
+					struct page **pages)
+{
+	int ret;
+	unsigned long i;
 
-unpin_pages:
-	/*
-	 * pages[i] might be NULL if any device coherent pages were found.
-	 */
 	for (i = 0; i < nr_pages; i++) {
-		if (!pages[i])
+		struct folio *folio = page_folio(pages[i]);
+
+		if (folio_is_device_coherent(folio)) {
+			/*
+			 * Migration will fail if the page is pinned, so convert
+			 * the pin on the source page to a normal reference.
+			 */
+			pages[i] = NULL;
+			folio_get(folio);
+			gup_put_folio(folio, 1, FOLL_PIN);
+
+			if (migrate_device_coherent_page(&folio->page)) {
+				ret = -EBUSY;
+				goto err;
+			}
+
 			continue;
+		}
 
+		/*
+		 * We can't migrate pages with unexpected references, so drop
+		 * the reference obtained by __get_user_pages_locked().
+		 * Migrating pages have been added to movable_page_list after
+		 * calling folio_isolate_lru() which takes a reference so the
+		 * page won't be freed if it's migrating.
+		 */
 		unpin_user_page(pages[i]);
+		pages[i] = NULL;
 	}
 
-	if (!list_empty(&movable_page_list)) {
+	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 */
+		if (migrate_pages(movable_page_list, alloc_migration_target,
+				  NULL, (unsigned long)&mtc, MIGRATE_SYNC,
+				  MR_LONGTERM_PIN, NULL)) {
 			ret = -ENOMEM;
+			goto err;
+		}
 	}
 
-	if (ret && !list_empty(&movable_page_list))
-		putback_movable_pages(&movable_page_list);
+	putback_movable_pages(movable_page_list);
+
+	return 0;
+
+err:
+	for (i = 0; i < nr_pages; i++)
+		if (pages[i])
+			unpin_user_page(pages[i]);
+	putback_movable_pages(movable_page_list);
+
 	return ret;
 }
+
+/*
+ * 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.
+ */
+static long check_and_migrate_movable_pages(unsigned long nr_pages,
+					    struct page **pages)
+{
+	int ret;
+	unsigned long collected;
+	LIST_HEAD(movable_page_list);
+
+	collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
+	if (!collected)
+		return 0;
+
+	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
+	if (!ret)
+		return -EAGAIN;
+	else
+		return ret;
+}
 #else
 static long check_and_migrate_movable_pages(unsigned long nr_pages,
 					    struct page **pages)
 {
-	return nr_pages;
+	return 0;
 }
 #endif /* CONFIG_MIGRATION */
 
@@ -2051,10 +2079,10 @@ static long __gup_longterm_locked(struct mm_struct *mm,
 			break;
 
 		rc = check_and_migrate_movable_pages(rc, pages);
-	} while (!rc);
+	} while (rc == -EAGAIN);
 	memalloc_pin_restore(flags);
 
-	return rc;
+	return rc ? rc : nr_pages;
 }
 
 static bool is_valid_gup_flags(unsigned int gup_flags)
-- 
git-series 0.9.1

Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[John Hubbard]

On 8/11/22 19:13, Alistair Popple wrote:
> 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. Migration can also fail, in which case the pages
> will also have been unpinned but the operation should not be retried. If
> all pages are in the correct zone nothing will be unpinned and no retry
> is required.
> 
> The logic in check_and_migrate_movable_pages() tracks unnecessary state
> and the return codes for each case are difficult to follow. Refactor the
> code to clean this up. No behaviour change is intended.
> 
> Signed-off-by: Alistair Popple <apopple@nvidia.com>

OK, I've finally convinced myself that this is a correct transformation.
This cleanup does help clarify things, definitely.

I've got two documentation additions (and changes) to suggest, below, and a
couple of too-long lines, but the code itself looks good, so with those
tweaks or something approximating them, please feel free to add:

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

...

> +/*
> + * 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.
> + */

Instead of the above, I'd like to suggest this:

/*
  * Check whether all pages are *allowed* to be pinned. Rather confusingly, all
  * pages in the range are required to be pinned via FOLL_PIN, before calling
  * this routine.
  *
  * If any pages in the range are not allowed to be pinned, then this routine
  * will migrate those pages away, unpin all the pages in the range and return
  * -EAGAIN. The caller should re-pin the entire range with FOLL_PIN and then
  * call this routine again.
  *
  * If an error other than -EAGAIN occurs, this indicates a migration failure.
  * The caller should give up, and propagate the error back up the call stack.
  *
  * If everything is OK and all pages in the range are allowed to be pinned, then
  * this routine leaves all pages pinned and returns zero for success.
  */

> +static long check_and_migrate_movable_pages(unsigned long nr_pages,
> +					    struct page **pages)
> +{
> +	int ret;
> +	unsigned long collected;
> +	LIST_HEAD(movable_page_list);
> +
> +	collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);

There is no reason to exceed 80 cols here.

> +	if (!collected)
> +		return 0;
> +
> +	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);

Nor here.

...

> @@ -2051,10 +2079,10 @@ static long __gup_longterm_locked(struct mm_struct *mm,
>   			break;

...and in this routine, let's fortify the comment like so:

@@ -2068,7 +2078,15 @@ static long __gup_longterm_locked(struct mm_struct *mm,
  	if (!(gup_flags & FOLL_LONGTERM))
  		return __get_user_pages_locked(mm, start, nr_pages, pages, vmas,
  					       NULL, gup_flags);
-	/* check_and_migrate_movable_pages() assumes pages have been pinned. */
+	/*
+	 * If we get to this point then FOLL_LONGTERM is set. And FOLL_LONGTERM
+	 * implies FOLL_PIN (although the reverse is not true). And that, in
+	 * turn, makes it correct to unconditionally call
+	 * check_and_migrate_movable_pages(), which assumes pages have been
+	 * pinned via FOLL_PIN.
+	 *
+	 * Enforce the above reasoning, by asserting that FOLL_PIN is set:
+	 */
  	if (WARN_ON(!(gup_flags & FOLL_PIN)))
  		return -EINVAL;
  	flags = memalloc_pin_save();


...and with that, it's actually possible for the reader to work their way
through this story, I think.


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[Jason Gunthorpe]

On Fri, Aug 12, 2022 at 12:13:09PM +1000, Alistair Popple wrote:
> 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. Migration can also fail, in which case the pages
> will also have been unpinned but the operation should not be retried. If
> all pages are in the correct zone nothing will be unpinned and no retry
> is required.
> 
> The logic in check_and_migrate_movable_pages() tracks unnecessary state
> and the return codes for each case are difficult to follow. Refactor the
> code to clean this up. No behaviour change is intended.
> 
> Signed-off-by: Alistair Popple <apopple@nvidia.com>
> 
> ---

This seems like the cleanest version yet!

> +static long check_and_migrate_movable_pages(unsigned long nr_pages,
> +					    struct page **pages)
> +{
> +	int ret;
> +	unsigned long collected;
> +	LIST_HEAD(movable_page_list);
> +
> +	collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
> +	if (!collected)
> +		return 0;
> +
> +	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
> +	if (!ret)
> +		return -EAGAIN;
> +	else
> +		return ret;

I would drop the else path and just return zero

Arguably migrate_longterm_unpinnable_pages() should do the same?

> @@ -2051,10 +2079,10 @@ static long __gup_longterm_locked(struct mm_struct *mm,
>  			break;
>  
>  		rc = check_and_migrate_movable_pages(rc, pages);
> -	} while (!rc);
> +	} while (rc == -EAGAIN);

Since the only reader only cares about errno or not errno..

But no biggie either way

Thanks,
Jason

Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[Alistair Popple]

Jason Gunthorpe <jgg@nvidia.com> writes:

> On Fri, Aug 12, 2022 at 12:13:09PM +1000, Alistair Popple wrote:
>> 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. Migration can also fail, in which case the pages
>> will also have been unpinned but the operation should not be retried. If
>> all pages are in the correct zone nothing will be unpinned and no retry
>> is required.
>>
>> The logic in check_and_migrate_movable_pages() tracks unnecessary state
>> and the return codes for each case are difficult to follow. Refactor the
>> code to clean this up. No behaviour change is intended.
>>
>> Signed-off-by: Alistair Popple <apopple@nvidia.com>
>>
>> ---
>
> This seems like the cleanest version yet!

Thanks, the feedback from John and yourself has been very useful!

>> +static long check_and_migrate_movable_pages(unsigned long nr_pages,
>> +					    struct page **pages)
>> +{
>> +	int ret;
>> +	unsigned long collected;
>> +	LIST_HEAD(movable_page_list);
>> +
>> +	collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
>> +	if (!collected)
>> +		return 0;
>> +
>> +	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
>> +	if (!ret)
>> +		return -EAGAIN;
>> +	else
>> +		return ret;
>
> I would drop the else path and just return zero

Unless I've misunderstood you I don't think that's correct though.
check_and_migrate_movable_pages() needs to return one of 3 conditions:

 - 0: All pages are in the correct zone and are still pinned (ie. "success").
 - -EAGAIN: Some pages were migrated, all pages need re-pinning.
 - errno: Migration failed, pins were dropped and PUP should fail.

John's suggested comment update spells this out more clearly.

> Arguably migrate_longterm_unpinnable_pages() should do the same?

migrate_longterm_unpinnable_pages() returns 0 for success, errno for
some kind of "permanent" failure that needs propagating.

>> @@ -2051,10 +2079,10 @@ static long __gup_longterm_locked(struct mm_struct *mm,
>>  			break;
>>
>>  		rc = check_and_migrate_movable_pages(rc, pages);
>> -	} while (!rc);
>> +	} while (rc == -EAGAIN);
>
> Since the only reader only cares about errno or not errno..

See above. Errors other than -EAGAIN should not be retried.

> But no biggie either way

So will leave this as for now at least.

> Thanks,
> Jason

Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[Jason Gunthorpe]

On Tue, Aug 16, 2022 at 03:29:20PM +1000, Alistair Popple wrote:

> >> +static long check_and_migrate_movable_pages(unsigned long nr_pages,
> >> +					    struct page **pages)
> >> +{
> >> +	int ret;
> >> +	unsigned long collected;
> >> +	LIST_HEAD(movable_page_list);
> >> +
> >> +	collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
> >> +	if (!collected)
> >> +		return 0;
> >> +
> >> +	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
> >> +	if (!ret)
> >> +		return -EAGAIN;
> >> +	else
> >> +		return ret;
> >
> > I would drop the else path and just return zero
> 
> Unless I've misunderstood you I don't think that's correct though.
> check_and_migrate_movable_pages() needs to return one of 3 conditions:
> 
>  - 0: All pages are in the correct zone and are still pinned (ie. "success").
>  - -EAGAIN: Some pages were migrated, all pages need re-pinning.
>  - errno: Migration failed, pins were dropped and PUP should fail.
> 
> John's suggested comment update spells this out more clearly.

But that is not what it does..

if (!ret) == if (ret != 0)
        return -EAGAIN
thus ret == 0
return ret == return 0

Jason

Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[Alistair Popple]

Jason Gunthorpe <jgg@nvidia.com> writes:

> On Tue, Aug 16, 2022 at 03:29:20PM +1000, Alistair Popple wrote:
>
>> >> +static long check_and_migrate_movable_pages(unsigned long nr_pages,
>> >> +					    struct page **pages)
>> >> +{
>> >> +	int ret;
>> >> +	unsigned long collected;
>> >> +	LIST_HEAD(movable_page_list);
>> >> +
>> >> +	collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
>> >> +	if (!collected)
>> >> +		return 0;
>> >> +
>> >> +	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
>> >> +	if (!ret)
>> >> +		return -EAGAIN;
>> >> +	else
>> >> +		return ret;
>> >
>> > I would drop the else path and just return zero
>>
>> Unless I've misunderstood you I don't think that's correct though.
>> check_and_migrate_movable_pages() needs to return one of 3 conditions:
>>
>>  - 0: All pages are in the correct zone and are still pinned (ie. "success").
>>  - -EAGAIN: Some pages were migrated, all pages need re-pinning.
>>  - errno: Migration failed, pins were dropped and PUP should fail.
>>
>> John's suggested comment update spells this out more clearly.
>
> But that is not what it does..
>
> if (!ret) == if (ret != 0)

Huh? Unless I'm misinterpretting you or am really behind on coffee
if (!ret) is equivalent to if (ret == 0), not if (ret != 0).

Ie:

if (ret == 0)
    return -EAGAIN;
else
    return !0;

Thus we will never return 0 from the above snippet.

>         return -EAGAIN
> thus ret == 0
> return ret == return 0
>
> Jason

Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[Jason Gunthorpe]

On Wed, Aug 17, 2022 at 12:01:58PM +1000, Alistair Popple wrote:
> 
> Jason Gunthorpe <jgg@nvidia.com> writes:
> 
> > On Tue, Aug 16, 2022 at 03:29:20PM +1000, Alistair Popple wrote:
> >
> >> >> +static long check_and_migrate_movable_pages(unsigned long nr_pages,
> >> >> +					    struct page **pages)
> >> >> +{
> >> >> +	int ret;
> >> >> +	unsigned long collected;
> >> >> +	LIST_HEAD(movable_page_list);
> >> >> +
> >> >> +	collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
> >> >> +	if (!collected)
> >> >> +		return 0;
> >> >> +
> >> >> +	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
> >> >> +	if (!ret)
> >> >> +		return -EAGAIN;
> >> >> +	else
> >> >> +		return ret;
> >> >
> >> > I would drop the else path and just return zero
> >>
> >> Unless I've misunderstood you I don't think that's correct though.
> >> check_and_migrate_movable_pages() needs to return one of 3 conditions:
> >>
> >>  - 0: All pages are in the correct zone and are still pinned (ie. "success").
> >>  - -EAGAIN: Some pages were migrated, all pages need re-pinning.
> >>  - errno: Migration failed, pins were dropped and PUP should fail.
> >>
> >> John's suggested comment update spells this out more clearly.
> >
> > But that is not what it does..
> >
> > if (!ret) == if (ret != 0)
> 
> Huh? Unless I'm misinterpretting you or am really behind on coffee
> if (!ret) is equivalent to if (ret == 0), not if (ret != 0).

Oh Dear, maybe I am still a bit jetlagged

Regardless, it is confusingly written :)

Sorry,
Jason

Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[John Hubbard]

On 8/17/22 05:09, Jason Gunthorpe wrote:
>>> if (!ret) == if (ret != 0)
>>
>> Huh? Unless I'm misinterpretting you or am really behind on coffee
>> if (!ret) is equivalent to if (ret == 0), not if (ret != 0).

Although the !ret idiom is a huge, huge part of kernel coding practice,
it also adds a slight bit of unnecessary mental translation (go ahead,
flame me hard, I know I'm asking for it...but...see above, after all.
Pixels have been spilt working through it).

And also...

> 
> Oh Dear, maybe I am still a bit jetlagged
> 
> Regardless, it is confusingly written :)
> 

...this is a slightly tricky and unusual error case, so it helps to be
extra clear by comparing against zero, and even adding a few short comments.

How's this look to you:

	collected = collect_longterm_unpinnable_pages(&movable_page_list,
						      nr_pages, pages);
	if (collected == 0)
		return 0;

	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages,
						pages);

	/* If we got here, we have some unpinnable pages... */

	if (ret == 0) {
		/*
		 * ...and we successfully migrated those pages. Which means that
		 * the caller should retry the operation now.
		 */
		ret = -EAGAIN;
	}

	return ret;


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[Jason Gunthorpe]

On Wed, Aug 17, 2022 at 01:35:12PM -0700, John Hubbard wrote:
> How's this look to you:
> 
> 	collected = collect_longterm_unpinnable_pages(&movable_page_list,
> 						      nr_pages, pages);
> 	if (collected == 0)
> 		return 0;
> 
> 	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages,
> 						pages);
> 
> 	/* If we got here, we have some unpinnable pages... */
> 
> 	if (ret == 0) {
> 		/*
> 		 * ...and we successfully migrated those pages. Which means that
> 		 * the caller should retry the operation now.
> 		 */
> 		ret = -EAGAIN;

return -EAGAIN

> 	}
> 
> 	return ret;

But why return 0 from the helper function in the first place?

Jason

Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[John Hubbard]

On 8/17/22 15:50, Jason Gunthorpe wrote:
> 
> But why return 0 from the helper function in the first place?
> 

The caller is looking for three distinct return code cases: 0, -EAGAIN,
or some other -errno.  Unless you restructure more, you need all three
of those cases. The 0 case leads to the caller, __gup_longterm_locked,
returning nr_pages.

Here is the comment block that I recommended earlier in this thread, that
documents those cases:

    https://lore.kernel.org/r/dc8fd102-ba30-d980-bdbb-11f39326103d@nvidia.com




thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[Alistair Popple]

Jason Gunthorpe <jgg@nvidia.com> writes:

> On Wed, Aug 17, 2022 at 01:35:12PM -0700, John Hubbard wrote:
>> How's this look to you:

I agree, I think all the refactoring left this written in a weird way. I
was going to suggest this though:

	collected = collect_longterm_unpinnable_pages(&movable_page_list,
						      nr_pages, pages);
	if (collected == 0)
		return 0;

	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages,
						pages);
	if (ret)
		return ret;

	return -EAGAIN;

Which IMHO looks at lot more normal and sane than what I had.

>> 	collected = collect_longterm_unpinnable_pages(&movable_page_list,
>> 						      nr_pages, pages);
>> 	if (collected == 0)
>> 		return 0;
>>
>> 	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages,
>> 						pages);
>>
>> 	/* If we got here, we have some unpinnable pages... */
>>
>> 	if (ret == 0) {
>> 		/*
>> 		 * ...and we successfully migrated those pages. Which means that
>> 		 * the caller should retry the operation now.
>> 		 */
>> 		ret = -EAGAIN;
>
> return -EAGAIN
>
>> 	}
>>
>> 	return ret;
>
> But why return 0 from the helper function in the first place?

To stick with the paradigm of 0 == success. Ie.
migrate_longterm_unpinnable_pages() successfully migrated everything
requested. I don't feel particularly strongly about this though - happy
to return -EAGAIN directly from migrate_longterm_unpinnable_pages() and
just pass that return code up the stack if others think it's clearer.

> Jason

Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[Jason Gunthorpe]

On Thu, Aug 18, 2022 at 09:24:28AM +1000, Alistair Popple wrote:
> 	collected = collect_longterm_unpinnable_pages(&movable_page_list,
> 						      nr_pages, pages);
> 	if (collected == 0)
> 		return 0;
> 
> 	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages,
> 						pages);
> 	if (ret)
> 		return ret;
> 
> 	return -EAGAIN;
> 
> Which IMHO looks at lot more normal and sane than what I had.

That isn't "success oriented flow" :)

> > But why return 0 from the helper function in the first place?
> 
> To stick with the paradigm of 0 == success. Ie.

But it doesn't mean success if we squashed it to EAGAIN here.

-EAGAIN == retry
      0 == success, real success
everything else == -ERRNO, failure

Stick with one convection in all the functions in this file in this grouping.

Jason

Re: [PATCH v4 1/2] mm/gup.c: Don't pass gup_flags to check_and_migrate_movable_pages()

[Matthew Wilcox]

On Fri, Aug 12, 2022 at 12:13:08PM +1000, Alistair Popple wrote:
> +			get_page(&folio->page);
> +			unpin_user_page(&folio->page);

https://lore.kernel.org/linux-mm/YvJddHPZsh7Lwzps@casper.infradead.org/

Re: [PATCH v4 1/2] mm/gup.c: Don't pass gup_flags to check_and_migrate_movable_pages()

[John Hubbard]

On 8/12/22 05:57, Matthew Wilcox wrote:
> On Fri, Aug 12, 2022 at 12:13:08PM +1000, Alistair Popple wrote:
>> +			get_page(&folio->page);
>> +			unpin_user_page(&folio->page);
> 
> https://lore.kernel.org/linux-mm/YvJddHPZsh7Lwzps@casper.infradead.org/

The above fix shows up in patch 2/2. I noticed during review that
it was applied to a different patch than the one you replied to,
but figured it didn't matter which patch picked up this fix, since
the problem precedes either patch.


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v4 1/2] mm/gup.c: Don't pass gup_flags to check_and_migrate_movable_pages()

[Matthew Wilcox]

On Fri, Aug 12, 2022 at 11:02:42AM -0700, John Hubbard wrote:
> On 8/12/22 05:57, Matthew Wilcox wrote:
> > On Fri, Aug 12, 2022 at 12:13:08PM +1000, Alistair Popple wrote:
> > > +			get_page(&folio->page);
> > > +			unpin_user_page(&folio->page);
> > 
> > https://lore.kernel.org/linux-mm/YvJddHPZsh7Lwzps@casper.infradead.org/
> 
> The above fix shows up in patch 2/2. I noticed during review that
> it was applied to a different patch than the one you replied to,
> but figured it didn't matter which patch picked up this fix, since
> the problem precedes either patch.

Oh!  I didn't realise patch 2/2 changed the same lines.  let me go
and read 2/2.

Re: [PATCH v4 1/2] mm/gup.c: Don't pass gup_flags to check_and_migrate_movable_pages()

[Alistair Popple]

Matthew Wilcox <willy@infradead.org> writes:

> On Fri, Aug 12, 2022 at 11:02:42AM -0700, John Hubbard wrote:
>> On 8/12/22 05:57, Matthew Wilcox wrote:
>> > On Fri, Aug 12, 2022 at 12:13:08PM +1000, Alistair Popple wrote:
>> > > +			get_page(&folio->page);
>> > > +			unpin_user_page(&folio->page);
>> >
>> > https://lore.kernel.org/linux-mm/YvJddHPZsh7Lwzps@casper.infradead.org/
>>
>> The above fix shows up in patch 2/2. I noticed during review that
>> it was applied to a different patch than the one you replied to,
>> but figured it didn't matter which patch picked up this fix, since
>> the problem precedes either patch.
>
> Oh!  I didn't realise patch 2/2 changed the same lines.  let me go
> and read 2/2.

Oh, and I missed that your original comment was on patch 1/2 not 2/2.
Anyway let me know if you think I should move it to the first patch, I
don't mind either way.