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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

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
> > <thomas.hellstrom@linux.intel.com> wrote:
> ...
> > > This can happen, for example if the process holding the
> > > device-private folio lock is stuck in
> > >    migrate_device_unmap()->lru_add_drain_all()
> > > The lru_add_drain_all() function requires a short work-item
> > > to be run on all online cpus to complete.
> > 
> > This is pretty bad behavior from lru_add_drain_all().
> 
> Yes. And also, by code inspection, it seems like other folio_batch
> items (I was going to say pagevecs, heh) can leak in after calling
> lru_add_drain_all(), making things even worse.
> 
> Maybe we really should be calling lru_cache_disable/enable()
> pairs for migration, even though it looks heavier weight.
> 
> This diff would address both points, and maybe fix Matthew's issue,
> although I haven't done much real testing on it other than a quick
> run of run_vmtests.sh:

It looks like lru_cache_disable() is using synchronize_rcu_expedited(),
which whould be a huge performance killer?

From the migrate code it looks like it's calling lru_add_drain_all()
once only, because migration is still best effort, so it's accepting
failures if someone adds pages to the per-cpu lru_add structures,
rather than wanting to take the heavy performance loss of
lru_cache_disable().

The problem at hand is also solved if we move the lru_add_drain_all()
out of the page-locked region in migrate_vma_setup(), like if we hit a
system folio not on the LRU, we'd unlock all folios, call
lru_add_drain_all() and retry from start.

But the root cause, even though lru_add_drain_all() is bad-behaving, is
IMHO the trylock spin in hmm_range_fault(). This is relatively recently
introduced to avoid another livelock problem, but there were other
fixes associated with that as well, so might not be strictly necessary.

IIRC he original non-trylocking code in do_swap_page() first took a
reference to the folio, released the page-table lock and then performed
a sleeping folio lock. Problem was that if the folio was already locked
for migration, that additional folio refcount would block migration
(which might not be a big problem considering do_swap_page() might want
to migrate to system ram anyway). @Matt Brost what's your take on this?

I'm also not sure a folio refcount should block migration after the
introduction of pinned (like in pin_user_pages) pages. Rather perhaps a
folio pin-count should block migration and in that case do_swap_page()
can definitely do a sleeping folio lock and the problem is gone.

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?

Thanks,
Thomas

> 
> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index 23379663b1e1..3c55a766dd33 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -570,7 +570,6 @@ static unsigned long
> migrate_device_unmap(unsigned long *src_pfns,
>  	struct folio *fault_folio = fault_page ?
>  		page_folio(fault_page) : NULL;
>  	unsigned long i, restore = 0;
> -	bool allow_drain = true;
>  	unsigned long unmapped = 0;
>  
>  	lru_add_drain();
> @@ -595,12 +594,6 @@ static unsigned long
> migrate_device_unmap(unsigned long *src_pfns,
>  
>  		/* ZONE_DEVICE folios are not on LRU */
>  		if (!folio_is_zone_device(folio)) {
> -			if (!folio_test_lru(folio) && allow_drain) {
> -				/* Drain CPU's lru cache */
> -				lru_add_drain_all();
> -				allow_drain = false;
> -			}
> -
>  			if (!folio_isolate_lru(folio)) {
>  				src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
>  				restore++;
> @@ -759,11 +752,15 @@ int migrate_vma_setup(struct migrate_vma *args)
>  	args->cpages = 0;
>  	args->npages = 0;
>  
> +	lru_cache_disable();
> +
>  	migrate_vma_collect(args);
>  
>  	if (args->cpages)
>  		migrate_vma_unmap(args);
>  
> +	lru_cache_enable();
> +
>  	/*
>  	 * At this point pages are locked and unmapped, and thus
> they have
>  	 * stable content and can safely be copied to destination
> memory that
> @@ -1395,6 +1392,8 @@ int migrate_device_range(unsigned long
> *src_pfns, unsigned long start,
>  {
>  	unsigned long i, j, pfn;
>  
> +	lru_cache_disable();
> +
>  	for (pfn = start, i = 0; i < npages; pfn++, i++) {
>  		struct page *page = pfn_to_page(pfn);
>  		struct folio *folio = page_folio(page);
> @@ -1413,6 +1412,8 @@ int migrate_device_range(unsigned long
> *src_pfns, unsigned long start,
>  
>  	migrate_device_unmap(src_pfns, npages, NULL);
>  
> +	lru_cache_enable();
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(migrate_device_range);
> @@ -1429,6 +1430,8 @@ int migrate_device_pfns(unsigned long
> *src_pfns, unsigned long npages)
>  {
>  	unsigned long i, j;
>  
> +	lru_cache_disable();
> +
>  	for (i = 0; i < npages; i++) {
>  		struct page *page = pfn_to_page(src_pfns[i]);
>  		struct folio *folio = page_folio(page);
> @@ -1446,6 +1449,8 @@ int migrate_device_pfns(unsigned long
> *src_pfns, unsigned long npages)
>  
>  	migrate_device_unmap(src_pfns, npages, NULL);
>  
> +	lru_cache_enable();
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(migrate_device_pfns);
> 
> 
> 
> 
> thanks,