Re: reply: [RFC] pin_user_pages_fast failure count increased

[David Hildenbrand <david@redhat.com>]

On 22.05.25 16:44, 김재원 wrote:
>> 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.

-- 
Cheers,

David / dhildenb