Re: [PATCH v2] mm/gup.c: Simplify and fix check_and_migrate_movable_pages() return codes

[Jason Gunthorpe]

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