Alistair Popple <apopple@xxxxxxxxxx> writes:
> Jason Gunthorpe <jgg@xxxxxxxxxx> 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,
