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

[Alistair Popple <apopple@nvidia.com>]

Jason Gunthorpe <jgg@nvidia.com> writes:

> On Fri, Jul 29, 2022 at 12:46:45PM +1000, Alistair Popple wrote:

[...]

> I have to say I prefer the usual style where all the places that error
> exit do 'goto error' instead of trying to keep track in 'ret'

Ok. Part of the complexity was my understanding from the documentation
for migrate_pages() is that putback_movable_pages() should only be
called if migrate_pages() != 0:

 * It is caller's responsibility to call putback_movable_pages() to return pages
 * to the LRU or free list only if ret != 0.

But I think it should be fine to do regardless, because on success the
pages will be deleted from movable_page_list. Eg. From unmap_and_move():

	if (rc != -EAGAIN) {
		/*
		 * A page that has been migrated has all references
		 * removed and will be freed. A page that has not been
		 * migrated will have kept its references and be restored.
		 */
		list_del(&page->lru);
	}

So will post a v2 doing this.

> AFAICT there is no reason to 'continue' in most of these paths since
> we intend to return to userspace with an error anyhow? Why try to
> isolate more pages?

The main reason would be if callers want to retry the operation. AFAIK
isolate_folio_lru() can have transient failures, so if callers want to
retry it makes sense to isolate and migrate as many pages as possible
rather than one page at a time as subsequent retries may find different
pages that can't be isolated.

Actually I should have called this out more clearly - the previous
behaviour on isolation failure was to retry indefinitely which is what
lead to looping in the kernel. This patch turns isolation failure into
an error and doesn't retry. I wonder though if we need to maintain a
retry count similar to what migrate_pages() does if there are unexpected
page refs?

>> @@ -1980,19 +1980,18 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>>  				    folio_nr_pages(folio));
>>  	}
>>
>> -	if (!list_empty(&movable_page_list) || isolation_error_count
>> -		|| coherent_pages)
>> -		goto unpin_pages;
>> -
>>  	/*
>>  	 * If list is empty, and no isolation errors, means that all pages are
>> -	 * in the correct zone.
>> +	 * in the correct zone. If there were device coherent pages some pages
>> +	 * have been unpinned.
>>  	 */
>
> That comment is a bit confusing.. I guess it is trying to explain why
> coherent_pages is doing?
>
> Maybe just:
>
> All the given pages are fine, nothing was done

Ok.

>> +	if (list_empty(&movable_page_list) && !ret && !coherent_pages)

Actually I think we can drop the coherent_pages variable too. At this
point coherent_pages will either be in the correct zone or we will have
jumped to the error label.

>> +		return 0;
>>
>> -unpin_pages:
>
> Now that this label is removed this if following it
>
> 	if (!list_empty(&movable_page_list)) {
>
> is also now unneeded because the above 'return 0' already checked it
>
> I came up with this ontop:

Thanks for the suggestions.

 - Alistair

> diff --git a/mm/gup.c b/mm/gup.c
> index 9e7c76d1e4ee3c..eddcf3c0eba727 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1912,11 +1912,15 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  					    struct page **pages,
>  					    unsigned int gup_flags)
>  {
> +	struct migration_target_control mtc = {
> +		.nid = NUMA_NO_NODE,
> +		.gfp_mask = GFP_USER | __GFP_NOWARN,
> +	};
>  	unsigned long i;
>  	struct folio *prev_folio = NULL;
>  	LIST_HEAD(movable_page_list);
>  	bool drain_allow = true, coherent_pages = false;
> -	int ret = 0;
> +	int ret = -EBUSY;
>
>  	for (i = 0; i < nr_pages; i++) {
>  		struct folio *folio = page_folio(pages[i]);
> @@ -1948,10 +1952,8 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  				unpin_user_page(&folio->page);
>  			}
>
> -			if (migrate_device_coherent_page(&folio->page)) {
> -				ret = -EBUSY;
> -				break;
> -			}
> +			if (migrate_device_coherent_page(&folio->page))
> +				goto error;
>  			continue;
>  		}
>
> @@ -1963,7 +1965,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  		if (folio_test_hugetlb(folio)) {
>  			if (isolate_hugetlb(&folio->page,
>  						&movable_page_list))
> -				ret = -EBUSY;
> +				goto error;
>  			continue;
>  		}
>
> @@ -1972,10 +1974,8 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  			drain_allow = false;
>  		}
>
> -		if (folio_isolate_lru(folio)) {
> -			ret = -EBUSY;
> -			continue;
> -		}
> +		if (folio_isolate_lru(folio))
> +			goto error;
>  		list_add_tail(&folio->lru, &movable_page_list);
>  		node_stat_mod_folio(folio,
>  				    NR_ISOLATED_ANON + folio_is_file_lru(folio),
> @@ -1987,7 +1987,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  	 * in the correct zone. If there were device coherent pages some pages
>  	 * have been unpinned.
>  	 */
> -	if (list_empty(&movable_page_list) && !ret && !coherent_pages)
> +	if (list_empty(&movable_page_list) && !coherent_pages)
>  		return 0;
>
>  	/*
> @@ -2005,23 +2005,19 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>  			put_page(pages[i]);
>  	}
>
> -	if (!list_empty(&movable_page_list)) {
> -		struct migration_target_control mtc = {
> -			.nid = NUMA_NO_NODE,
> -			.gfp_mask = GFP_USER | __GFP_NOWARN,
> -		};
> -
> -		ret = migrate_pages(&movable_page_list, alloc_migration_target,
> -				    NULL, (unsigned long)&mtc, MIGRATE_SYNC,
> -				    MR_LONGTERM_PIN, NULL);
> -		if (ret > 0) /* number of pages not migrated */
> -			ret = -ENOMEM;
> +	not_migrated = migrate_pages(&movable_page_list, alloc_migration_target,
> +				     NULL, (unsigned long)&mtc, MIGRATE_SYNC,
> +				     MR_LONGTERM_PIN, NULL);
> +	if (not_migrated > 0) {
> +		ret = -ENOMEM;
> +		goto error;
>  	}
> +	return -EAGAIN;
>
> -	if (ret && !list_empty(&movable_page_list))
> +error:
> +	if (!list_empty(&movable_page_list))
>  		putback_movable_pages(&movable_page_list);
> -
> -	return ret ? ret : -EAGAIN;
> +	return ret;
>  }
>  #else
>  static long check_and_migrate_movable_pages(unsigned long nr_pages,