Jason Gunthorpe <jgg@xxxxxxxxxx> writes:
> On Fri, Aug 12, 2022 at 12:13:09PM +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. Migration can also fail, in which case the pages
>> will also have been unpinned but the operation should not be retried. If
>> all pages are in the correct zone nothing will be unpinned and no retry
>> is required.
>>
>> The logic in check_and_migrate_movable_pages() tracks unnecessary state
>> and the return codes for each case are difficult to follow. Refactor the
>> code to clean this up. No behaviour change is intended.
>>
>> Signed-off-by: Alistair Popple <apopple@xxxxxxxxxx>
>>
>> ---
>
> This seems like the cleanest version yet!
Thanks, the feedback from John and yourself has been very useful!
>> +static long check_and_migrate_movable_pages(unsigned long nr_pages,
>> + struct page **pages)
>> +{
>> + int ret;
>> + unsigned long collected;
>> + LIST_HEAD(movable_page_list);
>> +
>> + collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
>> + if (!collected)
>> + return 0;
>> +
>> + ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);
>> + if (!ret)
>> + return -EAGAIN;
>> + else
>> + return ret;
>
> I would drop the else path and just return zero
Unless I've misunderstood you I don't think that's correct though.
check_and_migrate_movable_pages() needs to return one of 3 conditions:
- 0: All pages are in the correct zone and are still pinned (ie. "success").
- -EAGAIN: Some pages were migrated, all pages need re-pinning.
- errno: Migration failed, pins were dropped and PUP should fail.
John's suggested comment update spells this out more clearly.
> Arguably migrate_longterm_unpinnable_pages() should do the same?
migrate_longterm_unpinnable_pages() returns 0 for success, errno for
some kind of "permanent" failure that needs propagating.
>> @@ -2051,10 +2079,10 @@ static long __gup_longterm_locked(struct mm_struct *mm,
>> break;
>>
>> rc = check_and_migrate_movable_pages(rc, pages);
>> - } while (!rc);
>> + } while (rc == -EAGAIN);
>
> Since the only reader only cares about errno or not errno..
See above. Errors other than -EAGAIN should not be retried.
> But no biggie either way
So will leave this as for now at least.
> Thanks,
> Jason
