Messages in this thread | | | From | Alistair Popple <> | Subject | Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages() | Date | Thu, 18 Aug 2022 09:24:28 +1000 |
| |
Jason Gunthorpe <jgg@nvidia.com> writes:
> On Wed, Aug 17, 2022 at 01:35:12PM -0700, John Hubbard wrote: >> How's this look to you:
I agree, I think all the refactoring left this written in a weird way. I was going to suggest this though:
collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); if (collected == 0) return 0;
ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages); if (ret) return ret;
return -EAGAIN;
Which IMHO looks at lot more normal and sane than what I had.
>> collected = collect_longterm_unpinnable_pages(&movable_page_list, >> nr_pages, pages); >> if (collected == 0) >> return 0; >> >> ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, >> pages); >> >> /* If we got here, we have some unpinnable pages... */ >> >> if (ret == 0) { >> /* >> * ...and we successfully migrated those pages. Which means that >> * the caller should retry the operation now. >> */ >> ret = -EAGAIN; > > return -EAGAIN > >> } >> >> return ret; > > But why return 0 from the helper function in the first place?
To stick with the paradigm of 0 == success. Ie. migrate_longterm_unpinnable_pages() successfully migrated everything requested. I don't feel particularly strongly about this though - happy to return -EAGAIN directly from migrate_longterm_unpinnable_pages() and just pass that return code up the stack if others think it's clearer.
> Jason
| |