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
