Re: reply: [RFC] pin_user_pages_fast failure count increased

[Jaewon Kim <jaewon31.kim@samsung.com>]

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

I think we need to return -EGAIN if we have any_unpinnable.

BR
Jaewon Kim