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

[Barry Song]

On Sat, Aug 1, 2026 at 4:28 AM David Hildenbrand (Arm) <david@xxxxxxxxxx> 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: stable@xxxxxxxxxxxxxxx
> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> ---
> 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?

diff --git a/mm/gup.c b/mm/gup.c
index d110f30d9bf4..3aa1d498e9ea 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2300,13 +2300,13 @@ static unsigned long collect_longterm_unpinnable_folios(
                 * but also to remove any other folio references from LRU
                 * caches.
                 */
-               if (drained == 0 && folio_may_be_lru_cached(folio) &&
+               if (drained == 0 && folio_may_be_lru_cached(folio) &&
!folio_test_lru(folio) &&
                                folio_ref_count(folio) !=
                                folio_expected_ref_count(folio) + pin_refs) {
                        lru_add_drain();
                        drained = 1;
                }
-               if (drained == 1 && folio_may_be_lru_cached(folio) &&
+               if (drained == 1 && folio_may_be_lru_cached(folio) &&
!folio_test_lru(folio) &&
                                folio_ref_count(folio) !=
                                folio_expected_ref_count(folio) + pin_refs) {
                        lru_add_drain_all();

If the folio is on the LRU, why do we need to drain? Is it
because we already know that the folio is not on the LRU before
calling collect_longterm_unpinnable_folios()?

Thanks
Barry