Message-ID: <ef85aa74-180c-4fbc-8af6-e6cca45eed43@nvidia.com>
Date: Fri, 15 Aug 2025 18:03:17 -0700
From: John Hubbard <jhubbard@...dia.com>
To: Will Deacon <will@...nel.org>, linux-mm@...ck.org
Cc: linux-kernel@...r.kernel.org, Hugh Dickins <hughd@...gle.com>,
 Keir Fraser <keirf@...gle.com>, Jason Gunthorpe <jgg@...pe.ca>,
 David Hildenbrand <david@...hat.com>, Frederick Mayle <fmayle@...gle.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Peter Xu <peterx@...hat.com>
Subject: Re: [PATCH] mm/gup: Drain batched mlock folio processing before
 attempting migration

On 8/15/25 3:18 AM, Will Deacon wrote:
> When taking a longterm GUP pin via pin_user_pages(),
> __gup_longterm_locked() tries to migrate target folios that should not
> be longterm pinned, for example because they reside in a CMA region or
> movable zone. This is done by first pinning all of the target folios
> anyway, collecting all of the longterm-unpinnable target folios into a
> list, dropping the pins that were just taken and finally handing the
> list off to migrate_pages() for the actual migration.
> 
> It is critically important that no unexpected references are held on the
> folios being migrated, otherwise the migration will fail and
> pin_user_pages() will return -ENOMEM to its caller. Unfortunately, it is
> relatively easy to observe migration failures when running pKVM (which
> uses pin_user_pages() on crosvm's virtual address space to resolve
> stage-2 page faults from the guest) on a 6.15-based Pixel 6 device and
> this results in the VM terminating prematurely.
> 
> In the failure case, 'crosvm' has called mlock(MLOCK_ONFAULT) on its
> mapping of guest memory prior to the pinning. Subsequently, when
> pin_user_pages() walks the page-table, the relevant 'pte' is not
> present and so the faulting logic allocates a new folio, mlocks it
> with mlock_folio() and maps it in the page-table.
> 
> Since commit 2fbb0c10d1e8 ("mm/munlock: mlock_page() munlock_page()
> batch by pagevec"), mlock/munlock operations on a folio (formerly page),
> are deferred. For example, mlock_folio() takes an additional reference
> on the target folio before placing it into a per-cpu 'folio_batch' for
> later processing by mlock_folio_batch(), which drops the refcount once
> the operation is complete. Processing of the batches is coupled with
> the LRU batch logic and can be forcefully drained with
> lru_add_drain_all() but as long as a folio remains unprocessed on the
> batch, its refcount will be elevated.
> 
> This deferred batching therefore interacts poorly with the pKVM pinning

I would go even a little broader (more general), and claim that this
deferred batching interacts poorly with gup FOLL_LONGTERM when trying
to pin folios in CMA or ZONE_MOVABLE, in fact. 

More on this below.

> scenario as we can find ourselves in a situation where the migration
> code fails to migrate a folio due to the elevated refcount from the
> pending mlock operation.
> 
> Extend the existing LRU draining logic in
> collect_longterm_unpinnable_folios() so that unpinnable mlocked folios
> on the LRU also trigger a drain.
> 
> Cc: Hugh Dickins <hughd@...gle.com>
> Cc: Keir Fraser <keirf@...gle.com>
> Cc: Jason Gunthorpe <jgg@...pe.ca>
> Cc: David Hildenbrand <david@...hat.com>
> Cc: John Hubbard <jhubbard@...dia.com>
> Cc: Frederick Mayle <fmayle@...gle.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Peter Xu <peterx@...hat.com>
> Fixes: 2fbb0c10d1e8 ("mm/munlock: mlock_page() munlock_page() batch by pagevec")
> Signed-off-by: Will Deacon <will@...nel.org>
> ---
> 
> This has been quite unpleasant to debug and, as I'm not intimately

I'll bet! It's not even pleasant to *read* about it! haha. Sorry you had
to suffer through this.

> familiar with the mm internals, I've tried to include all the relevant
> details in the commit message in case there's a preferred alternative
> way of solving the problem or there's a flaw in my logic.
> 
>  mm/gup.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index adffe663594d..656835890f05 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2307,7 +2307,8 @@ static unsigned long collect_longterm_unpinnable_folios(
>  			continue;
>  		}
>  
> -		if (!folio_test_lru(folio) && drain_allow) {
> +		if (drain_allow &&
> +		   (!folio_test_lru(folio) || folio_test_mlocked(folio))) {

That should work, yes.

Alternatively, after thinking about this a bit today, it seems to me that the
mlock batching is a little too bold, given the presence of gup/pup. And so I'm
tempted to fix the problem closer to the root cause, like this (below).

But maybe this is actually *less* wise than what you have proposed...

I'd like to hear other mm folks' opinion on this approach:

diff --git a/mm/mlock.c b/mm/mlock.c
index a1d93ad33c6d..edecdd32996e 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -278,7 +278,15 @@ void mlock_new_folio(struct folio *folio)
 
 	folio_get(folio);
 	if (!folio_batch_add(fbatch, mlock_new(folio)) ||
-	    folio_test_large(folio) || lru_cache_disabled())
+	    folio_test_large(folio) || lru_cache_disabled() ||
+	/*
+	 * If this is being called as part of a gup FOLL_LONGTERM operation in
+	 * CMA/MOVABLE zones with MLOCK_ONFAULT active, then the newly faulted
+	 * in folio will need to immediately migrate to a pinnable zone.
+	 * Allowing the mlock operation to batch would break the ability to
+	 * migrate the folio. Instead, force immediate processing.
+	 */
+	 (current->flags & PF_MEMALLOC_PIN))
 		mlock_folio_batch(fbatch);
 	local_unlock(&mlock_fbatch.lock);
 }



>  			lru_add_drain_all();
>  			drain_allow = false;
>  		}

thanks,
-- 
John Hubbard

Powered by blists - more mailing lists