Re: [PATCH] mm/gup: stop leaking pinned pages in low memory conditions

[John Hubbard]

On 10/17/24 1:51 AM, David Hildenbrand wrote:
> On 16.10.24 22:22, John Hubbard wrote:
...
> > diff --git a/mm/gup.c b/mm/gup.c
> > index a82890b46a36..24acf53c8294 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -2492,6 +2492,17 @@ static long __gup_longterm_locked(struct 
> > mm_struct *mm,
> >           /* FOLL_LONGTERM implies FOLL_PIN */
> >           rc = check_and_migrate_movable_pages(nr_pinned_pages, pages);
> > +
> > +        /*
> > +         * The __get_user_pages_locked() call happens before we know
> > +         * that whether it's possible to successfully complete the whole

oops, s/that whether/that/

> > +         * operation. To compensate for this, if we get an unexpected
> > +         * error (such as -ENOMEM) then we must unpin everything, before
> > +         * erroring out.
> > +         */
> > +        if (rc != -EAGAIN && rc != 0)
> > +            unpin_user_pages(pages, nr_pinned_pages);
> > +
> >       } while (rc == -EAGAIN);
>
> Wouldn't it be cleaner to simply have here after the loop (possibly even 
> after the memalloc_pin_restore())
>
> if (rc)
>      unpin_user_pages(pages, nr_pinned_pages);
>
> But maybe I am missing something.

Yes, I think you are correct. That is cleaner. Let me retest real quick just
in case, and then send a v2 that also picks up the typo and moves the 
comment
to the new location.

thanks,
--
John Hubbard