Re: reply: [RFC] pin_user_pages_fast failure count increased

[David Hildenbrand <david@redhat.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