Re: reply: [RFC] pin_user_pages_fast failure count increased

[David Hildenbrand <david@redhat.com>]

On 22.05.25 15:09, Jaewon Kim wrote:
>> On 22.05.25 12:18, ?朝? (Zhaoyang Huang) wrote:
>>>> On Mon, Apr 28, 2025 at 02:12:57PM -0700, John Hubbard wrote:
>>>>> On 4/28/25 1:56 PM, David Hildenbrand wrote:
>>>>>> On 28.04.25 22:14, John Hubbard wrote:
>>>>>>> On 4/28/25 8:17 AM, Jaewon Kim wrote:
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> If pin_user_pages_fast does not pin all the requested number of
>>>>>>>> pages, then drivers calling to pin_user_pages_fast should retry
>>>>>>>> until the gup pins all?
>>>>>>>>
>>>>>>>
>>>>>>> Approaches vary, for handling partial success of pin_user_pages().
>>>>>>>
>>>>>>> * Many drivers unpin everything and either bail out entirely, or
>>>>>>> retry pinning the entire original range.
>>>>>>
>>>>>> Hm, unpinning + trying to repin the entire range can easily result
>>>>>> in an endless loop on persistent errors IIRC?
>>>>>>
>>>>>
>>>>> I vaguely recall a limited number of retries, yes.
>>>>>
>>>>> thanks,
>>>>> --
>>>>> John Hubbard
>>>>>
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> I'd like to report a potential issue introduced by a recent change in
>>>> 1aaf8c122918 mm: gup: fix infinite loop within __get_longterm_locked
>>>>
>>>> Previously, the call to migrate_longterm_unpinnable_folio() was guarded by the
>>>> collected variable. This meant that if a CMA page was temporarily held in the
>>>> pagevec and failed LRU isolation, it wouldn't be added to the
>>>> movable_page_list, but the collected counter would still be incremented.
>>
>> Okay, so we'd also express that way "any longterm_pinnable page found".
>>
>>> There is lru_add_drain_all for dealing with this scenario, so this won't be the case, right?
>>
>> Good point. Only concurrent isolation might be problematic (concurrent reclaim?).
>>
>>>>
>>>> As a result, migrate_longterm_unpinnable_folio() would return -EAGAIN, and
>>>> the process would be retried until migration of the CMA page succeeded.
>>>>
>>>> However, in the recent patch merged into mainline, the logic now only checks
>>>> whether movable_page_list is empty, and no longer relies on the collected
>>>> count.
>>>> This can cause CMA pages that fail isolation to bypass retry logic and remain
>>>> pinned.
>>>>
>>>> Effectively,long-term pinning is now possible for CMA pages ? something that
>>>> previously would have been avoided through repeated attempts.
>>
>> Calling migrate_longterm_unpinnable_folios() when there is nothing to migrate is stupid.
>>
>> Maybe something like:
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 329c5f7acc7a0..58b8e40fc19ed 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -2301,14 +2301,15 @@ static void pofs_unpin(struct pages_or_folios *pofs)
>>   }
>>
>>   /*
>> - * Returns the number of collected folios. Return value is always >= 0.
>> + * Returns whether any longterm unpinnable folio was found (if isolation
>> + * fails, not all can be added to the movable_folio_list).
>>    */
>> -static void collect_longterm_unpinnable_folios(
>> +static bool collect_longterm_unpinnable_folios(
>>                  struct list_head *movable_folio_list,
>>                  struct pages_or_folios *pofs)
>>   {
>> +       bool drain_allow = true, any_unpinnable = false;
>>          struct folio *prev_folio = NULL;
>> -       bool drain_allow = true;
>>          unsigned long i;
>>
>>          for (i = 0; i < pofs->nr_entries; i++) {
>> @@ -2320,6 +2321,7 @@ static void collect_longterm_unpinnable_folios(
>>
>>                  if (folio_is_longterm_pinnable(folio))
>>                          continue;
>> +               any_unpinnable = true;
>>
>>                  if (folio_is_device_coherent(folio))
>>                          continue;
>> @@ -2342,6 +2344,8 @@ static void collect_longterm_unpinnable_folios(
>>                                      NR_ISOLATED_ANON + folio_is_file_lru(folio),
>>                                      folio_nr_pages(folio));
>>          }
>> +
>> +       return any_unpinnable;
>>   }
>>
>>   /*
>> @@ -2417,11 +2421,12 @@ migrate_longterm_unpinnable_folios(struct list_head *movable_folio_list,
>>   static long
>>   check_and_migrate_movable_pages_or_folios(struct pages_or_folios *pofs)
>>   {
>> +       bool any_unpinnable;
>>          LIST_HEAD(movable_folio_list);
>>
>> -       collect_longterm_unpinnable_folios(&movable_folio_list, pofs);
>> +       any_unpinnable = collect_longterm_unpinnable_folios(&movable_folio_list, pofs);
>>          if (list_empty(&movable_folio_list))
>> -               return 0;
>> +               return any_unpinnable ? -EAGAIN : 0;
>>
>>          return migrate_longterm_unpinnable_folios(&movable_folio_list, pofs);
>>   }
>>
>>
>> --
>> Cheers,
>>
>> David / dhildenb
> 
> Hi
> 
> Thank you for your comment and patch.
> By the way, what if there are any_unpinnable pages and also pages in the movable_folio_list,
> but migrate_longterm_unpinnable_folios failed to migrate and return other erros instead of -EAGAIN?
> In that case, I think the CMA or other unpinnable pages would be pinned.

Oh, I think what we have to do is call pofs_unpin(pofs)() in case we 
return with -EAGAIN early. That's what 
migrate_longterm_unpinnable_folios() would do.

If we happen to call migrate_longterm_unpinnable_folios(), it would 
already unpin all folios by calling pofs_unpin(pofs). So we can just 
return whatever error we actually saw.

Do you have the capacity to send a proper fix?

-- 
Cheers,

David / dhildenb