Re: [PATCH] mm/gup: Drain batched mlock folio processing before attempting migration

[David Hildenbrand]

On 09.09.25 13:39, Will Deacon wrote:
> On Fri, Aug 29, 2025 at 08:46:52AM -0700, Hugh Dickins wrote:
> > On Fri, 29 Aug 2025, Will Deacon wrote:
> > > On Thu, Aug 28, 2025 at 01:47:14AM -0700, Hugh Dickins wrote:
> > > > diff --git a/mm/gup.c b/mm/gup.c
> > > > index adffe663594d..9f7c87f504a9 100644
> > > > --- a/mm/gup.c
> > > > +++ b/mm/gup.c
> > > > @@ -2291,6 +2291,8 @@ static unsigned long collect_longterm_unpinnable_folios(
> > > >   	struct folio *folio;
> > > >   	long i = 0;
> > > >   
> > > > +	lru_add_drain();
> > > > +
> > > >   	for (folio = pofs_get_folio(pofs, i); folio;
> > > >   	     folio = pofs_next_folio(folio, pofs, &i)) {
> > > >   
> > > > @@ -2307,7 +2309,8 @@ static unsigned long collect_longterm_unpinnable_folios(
> > > >   			continue;
> > > >   		}
> > > >   
> > > > -		if (!folio_test_lru(folio) && drain_allow) {
> > > > +		if (drain_allow && folio_ref_count(folio) !=
> > > > +				   folio_expected_ref_count(folio) + 1) {
> > > >   			lru_add_drain_all();
> > >
> > > How does this synchronise with the folio being added to the mlock batch
> > > on another CPU?
> > >
> > > need_mlock_drain(), which is what I think lru_add_drain_all() ends up
> > > using to figure out which CPU batches to process, just looks at the
> > > 'nr' field in the batch and I can't see anything in mlock_folio() to
> > > ensure any ordering between adding the folio to the batch and
> > > incrementing its refcount.
> > >
> > > Then again, my hack to use folio_test_mlocked() would have a similar
> > > issue because the flag is set (albeit with barrier semantics) before
> > > adding the folio to the batch, meaning the drain could miss the folio.
> > >
> > > I guess there's some higher-level synchronisation making this all work,
> > > but it would be good to understand that as I can't see that
> > > collect_longterm_unpinnable_folios() can rely on much other than the pin.
> >
> > No such strict synchronization: you've been misled if people have told
> > you that this pinning migration stuff is deterministically successful:
> > it's best effort - or will others on the Cc disagree?
> >
> > Just as there's no synchronization between the calculation inside
> > folio_expected_ref_count() and the reading of folio's refcount.
> >
> > It wouldn't make sense for this unpinnable collection to anguish over
> > such synchronization, when a moment later the migration is liable to
> > fail (on occasion) for other transient reasons.  All ending up reported
> > as -ENOMEM apparently? that looks unhelpful.
>
> I see this was tangentially discussed with David on the patches you sent
> and I agree that it's a distinct issue from what we're solving here,
> however, -ENOMEM is a particularly problematic way to report transient
> errors with migration due to a race. For KVM, the -ENOMEM will bubble
> back up to userspace and the VMM is likely to destroy the VM altogether
> whereas -EAGAIN would return back to the guest and retry the faulting
> instruction.

Migration code itself will retry multiple times, which usually takes 
care of most races.

Not all of course.

Now, I recall John was working on that at some point (I recall an RFC 
patch, but I might be daydreaming), and I recall discussions at LSF/MM 
around improving handling when we are mixing a flood of short-therm gup 
with a single long-term gup that wants to migrate these (short-term 
pinned) pages.

Essentially, we would have to temporarily prevent new short-term GUP 
pins in order to make the long-term GUP-pin succeed in migrating the folio.

--
Cheers

David / dhildenb