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

[David Hildenbrand <david@redhat.com>]

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