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