Re: [PATCH] mm/gup: fix always draining LRU caches in collect_longterm_unpinnable_folios()

On 8/3/26 11:17, Barry Song wrote:
> On Sat, Aug 1, 2026 at 4:28 AM David Hildenbrand (Arm) <[email protected]> wrote:
>>
>> folio_may_be_lru_cached() is currently only true for small folios, and
>> for small folios FOLL_PIN adds GUP_PIN_COUNTING_BIAS references instead
>> of 1 in try_grab_folio()/try_grab_folio_fast().
>>
>> Consequently, our
>>
>>         folio_ref_count(folio) != folio_expected_ref_count(folio) + 1
>>
>> check in collect_longterm_unpinnable_folios() will currently always
>> identify "reference mismatch" and first drain the local LRU cache to then
>> drain the LRU cache on all CPUs, as collect_longterm_unpinnable_folios()
>> is really called after pinning the folios with FOLL_PIN.
>>
>> Add a comment because the current code is not quite intuitive: we used to
>> drain only to make sure the folio_isolate_lru() would succeed. But then we
>> also started draining to make later migration more reliable.
>>
>> We'll refactor that code soon a bit, to also make it usable in other
>> context where we really want to remove any references from LRU caches.
>>
>> Let's add CC stable, because having an easy way for excessive LRU cache
>> draining on all CPUs does not sound right. In common scenarios we
>> don't expect to every have to drain.
>>
>> Fixes: 98c6d259319e ("mm/gup: check ref_count instead of lru before migration")
>> Fixes: a09a8a1fbb37 ("mm/gup: local lru_add_drain() to avoid lru_add_drain_all()")
>> Cc: [email protected]
>> Signed-off-by: David Hildenbrand (Arm) <[email protected]>
>> ---
>> Found by code inspection. If someone has a testcase that can easily
>> trigger this and result in migration problems, please test! But this
>> change seems to be "obvious the right thing to do".
>> ---
>>  mm/gup.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 99902c15703b0..41c3317e0f0f4 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -2273,6 +2273,7 @@ static unsigned long collect_longterm_unpinnable_folios(
>>
>>         for (folio = pofs_get_folio(pofs, i); folio;
>>              folio = pofs_next_folio(folio, pofs, &i)) {
>> +               const int pin_refs = folio_has_pincount(folio) ? 1 : GUP_PIN_COUNTING_BIAS;
>>
>>                 if (folio_is_longterm_pinnable(folio))
>>                         continue;
>> @@ -2287,15 +2288,20 @@ static unsigned long collect_longterm_unpinnable_folios(
>>                         continue;
>>                 }
>>
>> +               /*
>> +                * We drain not only to make the folio_isolate_lru() succeed,
>> +                * but also to remove any other folio references from LRU
>> +                * caches.
>> +                */
>>                 if (drained == 0 && folio_may_be_lru_cached(folio) &&
>>                                 folio_ref_count(folio) !=
>> -                               folio_expected_ref_count(folio) + 1) {
>> +                               folio_expected_ref_count(folio) + pin_refs) {
>>                         lru_add_drain();
>>                         drained = 1;
>>                 }
>>                 if (drained == 1 && folio_may_be_lru_cached(folio) &&
>>                                 folio_ref_count(folio) !=
>> -                               folio_expected_ref_count(folio) + 1) {
>> +                               folio_expected_ref_count(folio) + pin_refs) {
>>                         lru_add_drain_all();
>>                         drained = 2;
>>                 }
> 
> Not regarding the fix itself, I agree that the fix is correct. I am
> just a bit curious why we cannot simply do:
> 
>                 if (folio_may_be_lru_cached(folio) && !folio_test_lru(folio)) {
>                         lru_add_drain();
>                         if (!folio_test_lru(folio))
>                                 lru_add_drain_all();
>                 }
> 
> Or at least the following diff?

Best to read 98c6d259319e, which documents why this is required.

-- 
Cheers,

David