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

[Alistair Popple <apopple@nvidia.com>]

Alistair Popple <apopple@nvidia.com> writes:

> 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.

Actually sorry, ignore the above. I'd missed the return -EAGAIN after
the migrate_pages() call in your proposed patch.

>> 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,