lkml.org 
[lkml]   [2022]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()
From
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

\
 
 \ /
  Last update: 2022-08-12 09:09    [W:0.111 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site