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

[John Hubbard <jhubbard@nvidia.com>]

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