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