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

[David Hildenbrand]

On 17.10.24 23:57, John Hubbard wrote:
> On 10/17/24 2:47 PM, David Hildenbrand wrote:
> > On 17.10.24 23:28, Alistair Popple wrote:
> > > David Hildenbrand <david@xxxxxxxxxx> writes:
> > > > On 16.10.24 22:22, John Hubbard wrote:
> ...
> > > > > +        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.
> > >
> > > I initially thought the same thing but I'm not sure it is
> > > correct. Consider what happens when __get_user_pages_locked() fails
> > > earlier in the loop. In this case it will have bailed out of the loop
> > > with rc <= 0 but we shouldn't call unpin_user_pages().
>
> doh. yes. Thanks for catching that, Alistair! I actually considered
> it during the first draft, too, but got tunnel vision during the
> review, sigh.
>
> > Ah, I see what you mean, I primarily only stared at the diff.
> >
> > We should likely avoid using nr_pinned_pages as a temporary variable that
> > can hold an error value.
>
> OK, I still want to defer all the pretty refactoring ideas into some
> future effort, so for now, let's just leave v1 alone except for fixing
> the typo in the comment, yes?

Fine with me!

Acked-by: David Hildenbrand <david@xxxxxxxxxx>

--
Cheers,

David / dhildenb