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

Commit Message

Alistair Popple Aug. 12, 2022, 2:13 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]>
Reviewed-by: David Hildenbrand <[e-mail]>
Reviewed-by: John Hubbard <[e-mail]>

---

Changes for v3:

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


base-commit: 360614c01f81f48a89d8b13f8fa69c3ae0a1f5c7

Comments

Matthew Wilcox Aug. 12, 2022, 12:57 p.m. UTC | #1

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/[e-mail]/

John Hubbard Aug. 12, 2022, 6:02 p.m. UTC | #2

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/[e-mail]/

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,

Matthew Wilcox Aug. 12, 2022, 6:11 p.m. UTC | #3

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/[e-mail]/
> 
> 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.

Alistair Popple Aug. 15, 2022, 5:52 a.m. UTC | #4

Matthew Wilcox <[e-mail]> 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/[e-mail]/
>>
>> 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.

Patch

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