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

[Matthew Brost <matthew.brost@intel.com>]

On Sun, Feb 01, 2026 at 12:48:33PM -0800, John Hubbard wrote:
> 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

Yes, I reasoned that this could be a problem too after thinking about it
a bit more. The issue with taking a refcount without the lock is that
we’re back to the livelock problem that was solved here:

git format-patch -1 1afaeb8293c9a

> then the lock_page() call here will be doing a use-after-free.
> 

I don’t think it’s a use-after-free per se; rather, the page could have
moved and been reallocated. If the same_pte check were moved under the
page lock, I think it would largely solve that, but if the page were
reallocated as a larger folio, the page lock might collide with the
folio-order bit encoding and hang forever. This is likely extremely hard
to hit, as you’d need multiple threads faulting the same page plus the
driver reallocating the page as a folio at the same time, but
nonetheless it could be a problem.

So maybe back to drawing board...

Matt

> 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
>