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?
>
>At least that's what we did before 1aaf8c122 when calling 
>collect_longterm_unpinnable_folios() I think.
>
>> I think your following code seems to be good to me.
>>    return any_unpinnable ? -EAGAIN : 0;
>
>I think we have to call pofs_unpin() here.
>
>Assuming we always call migrate_longterm_unpinnable_folios() with a list 
>now, we could drop the list_empty() check in there.
>

Thank for your kind explanation.

I think this is what you meant, please let me know if you have an idea to make this nicer.
We may be to able to prepare the patch next week.

 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);
-       if (list_empty(&movable_folio_list))
-               return 0;
+       any_unpinnable = collect_longterm_unpinnable_folios(&movable_folio_list, pofs);
+       if (list_empty(&movable_folio_list)) {
+               if (any_unpinnable)
+                       pofs_unpin(pofs);
+               return any_unpinnable ? -EAGAIN : 0;
+       }
 
        return migrate_longterm_unpinnable_folios(&movable_folio_list, pofs);
 }


>-- 
>Cheers,
>
>David / dhildenb