Messages in this thread Patch in this message | | | Date | Fri, 12 Aug 2022 00:05:18 -0700 | Subject | Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages() | From | John Hubbard <> |
| |
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
| |