Re: [PATCH v4 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[Jason Gunthorpe]

On Wed, Aug 17, 2022 at 12:01:58PM +1000, Alistair Popple wrote:
> 
> Jason Gunthorpe <jgg@xxxxxxxxxx> writes:
> 
> > On Tue, Aug 16, 2022 at 03:29:20PM +1000, Alistair Popple wrote:
> >
> >> >> +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.
> >
> > But that is not what it does..
> >
> > if (!ret) == if (ret != 0)
> 
> Huh? Unless I'm misinterpretting you or am really behind on coffee
> if (!ret) is equivalent to if (ret == 0), not if (ret != 0).

Oh Dear, maybe I am still a bit jetlagged

Regardless, it is confusingly written :)

Sorry,
Jason