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

[Will Deacon]

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.

Will