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

Commit Message

Alistair Popple Aug. 5, 2022, 6:29 a.m. UTC

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 <[e-mail]>
---
 mm/gup.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)


base-commit: 360614c01f81f48a89d8b13f8fa69c3ae0a1f5c7

Comments

John Hubbard Aug. 5, 2022, 7:35 a.m. UTC | #1

On 8/4/22 23:29, Alistair Popple wrote:
> 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

heh, while verifying the above claim, I noticed that
__gup_longterm_locked() has been misnamed for some time now. That
function handles both FOLL_LONGTERM and !FOLL_LONGTERM. There's
always something...

Anyway, though, this patch is a good cleanup step, so:

Reviewed-by: John Hubbard <[e-mail]>


thanks,

David Hildenbrand Aug. 5, 2022, 8:10 a.m. UTC | #2

On 05.08.22 08:29, Alistair Popple wrote:
> 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 <[e-mail]>
> ---
>  mm/gup.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)

Reviewed-by: David Hildenbrand <[e-mail]>

Jason Gunthorpe Aug. 5, 2022, 12:06 p.m. UTC | #3

On Fri, Aug 05, 2022 at 04:29:52PM +1000, Alistair Popple wrote:
> @@ -2053,7 +2046,9 @@ static long __gup_longterm_locked(struct mm_struct *mm,
>  					     NULL, gup_flags);
>  		if (rc <= 0)
>  			break;
> -		rc = check_and_migrate_movable_pages(rc, pages, gup_flags);
> +
> +		WARN_ON_ONCE(!(gup_flags & FOLL_PIN));
> +		rc = check_and_migrate_movable_pages(rc, pages);

This should be moved up:

	if (!(gup_flags & FOLL_LONGTERM))
		return __get_user_pages_locked(mm, start, nr_pages, pages, vmas,
					       NULL, gup_flags);
	if (WARN_ON(!(gup_flags & FOLL_PIN)))
	   return  -EINVAL;
	flags = memalloc_pin_save();

Jason

Alistair Popple Aug. 8, 2022, 1:55 a.m. UTC | #4

Jason Gunthorpe <[e-mail]> writes:

> On Fri, Aug 05, 2022 at 04:29:52PM +1000, Alistair Popple wrote:
>> @@ -2053,7 +2046,9 @@ static long __gup_longterm_locked(struct mm_struct *mm,
>>  					     NULL, gup_flags);
>>  		if (rc <= 0)
>>  			break;
>> -		rc = check_and_migrate_movable_pages(rc, pages, gup_flags);
>> +
>> +		WARN_ON_ONCE(!(gup_flags & FOLL_PIN));
>> +		rc = check_and_migrate_movable_pages(rc, pages);
>
> This should be moved up:

Ok. I debated doing that originally but wanted to keep the WARN_ON
close to the code that makes that assumption. On the other hand a
comment would achieve that so will move it up. Thanks.

> 	if (!(gup_flags & FOLL_LONGTERM))
> 		return __get_user_pages_locked(mm, start, nr_pages, pages, vmas,
> 					       NULL, gup_flags);
> 	if (WARN_ON(!(gup_flags & FOLL_PIN)))
> 	   return  -EINVAL;
> 	flags = memalloc_pin_save();
>
> Jason

Patch

diff --git a/mm/gup.c b/mm/gup.c
index c6d060d..e26ccc0 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;
 }
@@ -2053,7 +2046,9 @@  static long __gup_longterm_locked(struct mm_struct *mm,
 					     NULL, gup_flags);
 		if (rc <= 0)
 			break;
-		rc = check_and_migrate_movable_pages(rc, pages, gup_flags);
+
+		WARN_ON_ONCE(!(gup_flags & FOLL_PIN));
+		rc = check_and_migrate_movable_pages(rc, pages);
 	} while (!rc);
 	memalloc_pin_restore(flags);