On Tue, Aug 02, 2022 at 10:30:12AM +1000, 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. This is indicated by returning zero. When all pages
> are in the correct zone the number of pinned pages is returned.
>
> However migration can also fail, in which case pages are unpinned and
> -ENOMEM is returned. However if the failure was due to not being unable
> to isolate a page zero is returned. This leads to indefinite looping in
> __gup_longterm_locked().
>
> Fix this by simplifying the return codes such that zero indicates all
> pages were successfully pinned in the correct zone while errors indicate
> either pages were migrated and pinning should be retried or that
> migration has failed and therefore the pinning operation should fail.
>
> This fixes the indefinite looping on page isolation failure by failing
> the pin operation instead of retrying indefinitely.
>
> Signed-off-by: Alistair Popple <apopple@xxxxxxxxxx>
>
> ---
>
> Changes for v2:
> - Changed error handling to be move conventional using goto as
> suggested by Jason.
> - Removed coherent_pages check as it isn't necessary.
> ---
> mm/gup.c | 81 ++++++++++++++++++++++++++++-----------------------------
> 1 file changed, 41 insertions(+), 40 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 364b274..5707c56 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1901,20 +1901,24 @@ struct page *get_dump_page(unsigned long addr)
>
> #ifdef CONFIG_MIGRATION
> /*
> - * Check whether all pages are pinnable, if so return number of pages. If some
> - * pages are not pinnable, migrate them, and unpin all pages. Return zero if
> - * pages were migrated, or if some pages were not successfully isolated.
> - * Return negative error if migration fails.
> + * 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.
> */
> static long check_and_migrate_movable_pages(unsigned long nr_pages,
> struct page **pages,
> unsigned int gup_flags)
> {
> - unsigned long isolation_error_count = 0, i;
> + unsigned long i;
> struct folio *prev_folio = NULL;
> LIST_HEAD(movable_page_list);
> - bool drain_allow = true, coherent_pages = false;
> - int ret = 0;
> + bool drain_allow = true;
> + int ret = -EAGAIN;
It looked like every goto error set this? Why initialize it?
It looks OK to me, a lot clearer
Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Thanks,
Jason
