Re: [PATCH] mm/hmm: Fix a hmm_range_fault() livelock / starvation problem

[John Hubbard <jhubbard@nvidia.com>]

On 2/1/26 11:24 AM, Matthew Brost wrote:
> On Sat, Jan 31, 2026 at 01:42:20PM -0800, John Hubbard wrote:
>> On 1/31/26 11:00 AM, Matthew Brost wrote:
>>> On Sat, Jan 31, 2026 at 01:57:21PM +0100, Thomas Hellström wrote:
>>>> On Fri, 2026-01-30 at 19:01 -0800, John Hubbard wrote:
>>>>> On 1/30/26 10:00 AM, Andrew Morton wrote:
>>>>>> On Fri, 30 Jan 2026 15:45:29 +0100 Thomas Hellström wrote:
>>>>> ...
>>> I’m not convinced the folio refcount has any bearing if we can take a
>>> sleeping lock in do_swap_page, but perhaps I’m missing something.
>>
>> So far, I am not able to find a problem with your proposal. So,
>> something like this I believe could actually work:
> 
> I did something slightly more defensive with a refcount protection, but
> this seems to work + fix the raised by Thomas and shows no noticeable
> performance difference. If we go this route, do_huge_pmd_device_private
> would need to be updated with the same pattern as well - I don't have
> large device pages enabled in current test branch but would have to test
> that part out too.
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index da360a6eb8a4..1e7ccc4a1a6c 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4652,6 +4652,8 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>                          vmf->page = softleaf_to_page(entry);
>                          ret = remove_device_exclusive_entry(vmf);
>                  } else if (softleaf_is_device_private(entry)) {
> +                       struct dev_pagemap *pgmap;
> +
>                          if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
>                                  /*
>                                   * migrate_to_ram is not yet ready to operate
> @@ -4670,21 +4672,15 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>                                                          vmf->orig_pte)))
>                                  goto unlock;
> 
> -                       /*
> -                        * Get a page reference while we know the page can't be
> -                        * freed.
> -                        */
> -                       if (trylock_page(vmf->page)) {
> -                               struct dev_pagemap *pgmap;
> -
> -                               get_page(vmf->page);
> -                               pte_unmap_unlock(vmf->pte, vmf->ptl);
> +                       pte_unmap_unlock(vmf->pte, vmf->ptl);
> +                       lock_page(vmf->page);
> +                       if (get_page_unless_zero(vmf->page)) {

I think this ordering has a problem, because it releases the PTL before
getting a refcount. That allows another thread to free the page, and
then the lock_page() call here will be doing a use-after-free.

That's why I reversed the order of those, and then as a result the
get_page_unless_zero() also becomes unnecessary.

>                                  pgmap = page_pgmap(vmf->page);
>                                  ret = pgmap->ops->migrate_to_ram(vmf);
>                                  unlock_page(vmf->page);
>                                  put_page(vmf->page);
>                          } else {
> -                               pte_unmap_unlock(vmf->pte, vmf->ptl);
> +                               unlock_page(vmf->page);
>                          }
>                  } else if (softleaf_is_hwpoison(entry)) {
>                          ret = VM_FAULT_HWPOISON;
> 
>> diff --git a/mm/memory.c b/mm/memory.c
>> index da360a6eb8a4..af73430e7888 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -4652,6 +4652,8 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>>   			vmf->page = softleaf_to_page(entry);
>>   			ret = remove_device_exclusive_entry(vmf);
>>   		} else if (softleaf_is_device_private(entry)) {
>> +			struct dev_pagemap *pgmap;
>> +
>>   			if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
>>   				/*
>>   				 * migrate_to_ram is not yet ready to operate
>> @@ -4674,18 +4676,13 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>>   			 * Get a page reference while we know the page can't be
>>   			 * freed.
>>   			 */
>> -			if (trylock_page(vmf->page)) {
>> -				struct dev_pagemap *pgmap;
>> -
>> -				get_page(vmf->page);
>> -				pte_unmap_unlock(vmf->pte, vmf->ptl);
>> -				pgmap = page_pgmap(vmf->page);
>> -				ret = pgmap->ops->migrate_to_ram(vmf);
>> -				unlock_page(vmf->page);
>> -				put_page(vmf->page);
>> -			} else {
>> -				pte_unmap_unlock(vmf->pte, vmf->ptl);
>> -			}
>> +			get_page(vmf->page);
>> +			pte_unmap_unlock(vmf->pte, vmf->ptl);
>> +			lock_page(vmf->page);
>> +			pgmap = page_pgmap(vmf->page);
>> +			ret = pgmap->ops->migrate_to_ram(vmf);
>> +			unlock_page(vmf->page);
>> +			put_page(vmf->page);
>>   		} else if (softleaf_is_hwpoison(entry)) {
>>   			ret = VM_FAULT_HWPOISON;
>>   		} else if (softleaf_is_marker(entry)) {
>>
>>>
>>>> But it looks like an AR for us to try to check how bad
>>>> lru_cache_disable() really is. And perhaps compare with an
>>>> unconditional lru_add_drain_all() at migration start.
>>>>
>>>> Does anybody know who would be able to tell whether a page refcount
>>>> still should block migration (like today) or whether that could
>>>> actually be relaxed to a page pincount?
>>
>> Yes, it really should block migration, see my response above: both
>> pincount and refcount literally mean "do not move this page".
>>
>> As an aside because it might help at some point, I'm just now testing a
>> tiny patchset that uses:
>>
>>      wait_var_event_killable(&folio->_refcount,
>>                              folio_ref_count(folio) <= expected)
>>
>> during migration, paired with:
>>
>>       wake_up_var(&folio->_refcount) in put_page().
>>
>> This waits for the expected refcount, instead of doing a blind, tight
>> retry loop during migration attempts. This lets migration succeed even
>> when waiting a long time for another caller to release a refcount.
>>
>> It works well, but of course, it also has a potentially serious
>> performance cost (which I need to quantify), because it adds cycles to
>> the put_page() hot path. Which is why I haven't posted it yet, even as
>> an RFC. It's still in the "is this even reasonable" stage, just food
>> for thought here.
>>
> 
> If you post an RFC we (Intel) can give it try as we have tests that
> really stress migration in odd ways and have fairly good metrics to
> catch perf issues too.
> 

That would be wonderful, thanks! Testing is hard.

thanks,
-- 
John Hubbard