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

[Hugh Dickins <hughd@google.com>]

On Fri, 31 Jul 2026, David Hildenbrand (Arm) 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@vger.kernel.org
> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>

Ughh! And I was so proud of it. Now I'm ashamed. It just never
occurred to me, that the unpinnable pages were pinned there.
Thanks for the fix (but please don't tell anyone ;)

Acked-by: Hugh Dickins <hughd@google.com>

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

Sorry, I don't (as perhaps is too obvious). And I'm a wee bit afraid
that some of the problems which those changes happened to succeed in
fixing, might now reappear once the drains are finely targeted.

Am I arguing for a delay before advancing to stable?  Perhaps, but
I think not: it's probably best to get that working as intended,
then deal with any fallout if it arises: just be on guard.

> ---
>  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;
>  		}
> 
> ---
> 
> base-commit: e5492213654050379e78ec6f9acfd6c9fe00f334
> 
> change-id: 20260731-check_and_migrate_movable_folios-43500556943d
> 
> --
> 
> Cheers,
> 
> David