reply: [RFC] pin_user_pages_fast failure count increased

[김재원 <jaewon31.kim@samsung.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.

I did not understand. Do you mean we need to call pofs_unpin in case of any_unpinnable?
I think your following code seems to be good to me.
  return any_unpinnable ? -EAGAIN : 0;

>
>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.

Oh, I didn't see that pofs_unpin within migrate_longterm_unpinnable_folios, I missed it.
Then I think your previous change is good enough.

>
>Do you have the capacity to send a proper fix?

If you meant to allow us to use your code and make a patch, we will do.

BR

>
>-- 
>Cheers,
>
>David / dhildenb