* Re: [PATCH] mm/gup: fix always draining LRU caches in collect_longterm_unpinnable_folios()
2026-07-31 20:27 [PATCH] mm/gup: fix always draining LRU caches in collect_longterm_unpinnable_folios() David Hildenbrand (Arm)
@ 2026-08-01 18:49 ` Hugh Dickins
2026-08-01 19:20 ` Andrew Morton
2026-08-03 9:01 ` David Hildenbrand (Arm)
2026-08-03 9:17 ` Barry Song
2 siblings, 1 reply; 6+ messages in thread
From: Hugh Dickins @ 2026-08-01 18:49 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Andrew Morton, Jason Gunthorpe, John Hubbard, Peter Xu,
Kiryl Shutsemau, Hugh Dickins, Ackerley Tng, Frank van der Linden,
linux-mm, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mm/gup: fix always draining LRU caches in collect_longterm_unpinnable_folios()
2026-08-01 18:49 ` Hugh Dickins
@ 2026-08-01 19:20 ` Andrew Morton
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2026-08-01 19:20 UTC (permalink / raw)
To: Hugh Dickins
Cc: David Hildenbrand (Arm), Jason Gunthorpe, John Hubbard, Peter Xu,
Kiryl Shutsemau, Ackerley Tng, Frank van der Linden, linux-mm,
linux-kernel, stable
On Sat, 1 Aug 2026 11:49:39 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> > 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.
This patch is in mm-new, so it's still some months away from -stable
eligibility.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/gup: fix always draining LRU caches in collect_longterm_unpinnable_folios()
2026-07-31 20:27 [PATCH] mm/gup: fix always draining LRU caches in collect_longterm_unpinnable_folios() David Hildenbrand (Arm)
2026-08-01 18:49 ` Hugh Dickins
@ 2026-08-03 9:01 ` David Hildenbrand (Arm)
2026-08-03 9:17 ` Barry Song
2 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand (Arm) @ 2026-08-03 9:01 UTC (permalink / raw)
To: Andrew Morton, Jason Gunthorpe, John Hubbard, Peter Xu,
Kiryl Shutsemau, Hugh Dickins, Ackerley Tng
Cc: linux-mm, linux-kernel, stable
On 7/31/26 22:27, 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.
Stumbling over this:
s/every/ever/
Thanks for the review Hugh!
--
Cheers,
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/gup: fix always draining LRU caches in collect_longterm_unpinnable_folios()
2026-07-31 20:27 [PATCH] mm/gup: fix always draining LRU caches in collect_longterm_unpinnable_folios() David Hildenbrand (Arm)
2026-08-01 18:49 ` Hugh Dickins
2026-08-03 9:01 ` David Hildenbrand (Arm)
@ 2026-08-03 9:17 ` Barry Song
2026-08-03 9:19 ` David Hildenbrand (Arm)
2 siblings, 1 reply; 6+ messages in thread
From: Barry Song @ 2026-08-03 9:17 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Andrew Morton, Jason Gunthorpe, John Hubbard, Peter Xu,
Kiryl Shutsemau, Hugh Dickins, Ackerley Tng, linux-mm,
linux-kernel, stable
On Sat, Aug 1, 2026 at 4:28 AM David Hildenbrand (Arm) <david@kernel.org> 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>
> ---
> 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] mm/gup: fix always draining LRU caches in collect_longterm_unpinnable_folios()
2026-08-03 9:17 ` Barry Song
@ 2026-08-03 9:19 ` David Hildenbrand (Arm)
0 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand (Arm) @ 2026-08-03 9:19 UTC (permalink / raw)
To: Barry Song
Cc: Andrew Morton, Jason Gunthorpe, John Hubbard, Peter Xu,
Kiryl Shutsemau, Hugh Dickins, Ackerley Tng, linux-mm,
linux-kernel, stable
On 8/3/26 11:17, Barry Song wrote:
> On Sat, Aug 1, 2026 at 4:28 AM David Hildenbrand (Arm) <david@kernel.org> 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>
>> ---
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread