Re: [PATCH v2 2/3] mm/gup.c: Migrate device coherent pages when pinning instead of failing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/11/22 18:51, Alistair Popple wrote:
...
@@ -1888,15 +1942,40 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
   			continue;
   		prev_head = head;
   		/*
-		 * If we get a movable page, since we are going to be pinning
-		 * these entries, try to move them out if possible.
+		 * 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
+		 * migrating page out of device memory.
   		 */
   		if (is_dev_private_or_coherent_page(head)) {
+			/*
+			 * device private pages will get faulted in during gup
+			 * so it shouldn't be possible to see one here.
+			 */
   			WARN_ON_ONCE(is_device_private_page(head));
-			ret = -EFAULT;
-			goto unpin_pages;
+			WARN_ON_ONCE(PageCompound(head));
+
+			/*
+			 * 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(head);
+				unpin_user_page(head);

OK...but now gup_flags can no longer be used as a guide for how to
release these pages, right? In other words, up until this point,
FOLL_PIN meant "call unpin_user_page() in order to release". However,
now this page must be released via put_page().

This is the source page (head). We are unpinning it because we can't migrate a
pinned page, however we still need a reference on it for migrate_vma hence the
get_page followed by unpin. In the non-FOLL_PIN case we already have a
reference from gup.

See below...

+			}
+
+			pages[i] = migrate_device_page(head, gup_flags);

migrate_device_page() will return a new page that has been correctly pinned
with gup_flags by try_grab_page(). Therefore this page can still be released
with unpin_user_page() or put_page() as appropriate for the given gup_flags.

The reference we had on the source page (head) always gets dropped in
migrate_vma_finalize().

OK. Good.

The above would be good to have in a comment, right around here, imho.
Because we have this marvelous mix of references for migration (get_page())
and other, and it's a bit hard to see that it's all correct without a
hint or two.

...

Which unless I've missed something is still the correct thing to do.

This reminds me: out of the many things to monitor, the FOLL_PIN counts
in /proc/vmstat are especially helpful, whenever making changes to code
that deals with this:

	nr_foll_pin_acquired
	nr_foll_pin_released

...and those should normally be equal to each other when "at rest".


I hope this is/was run, just to be sure?


thanks,
--
John Hubbard
NVIDIA



[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux