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

[Alistair Popple <apopple@nvidia.com>]

On 2026-02-10 at 01:47 +1100, Thomas Hellström <thomas.hellstrom@linux.intel.com> wrote...
> @Alistair, any chance of an R-B for the below version?

For sure. Sorry I've been getting back to this but caught up with internal
stuff.

> > +static inline void softleaf_entry_wait_on_locked(softleaf_t entry,
> > spinlock_t *ptl)
> > +	__releases(ptl)
> > +{
> > +	spin_unlock(ptl);
> > +}
> > +

I noticed this just because we didn't have it previously, but I assume it's to
avoid compilation failures in do_swap_page(). This is definitely the better way
of dealing with the conditional compilation, though if I were to add a nit it
would be that a WARN_ON_ONCE() would be nice here.

But this is fine, and thanks for doing the rename. Feel free to add:

Reviewed-by: Alistair Popple <apopple@nvidia.com>

> >  #endif /* CONFIG_MIGRATION */
> >  
> >  #ifdef CONFIG_NUMA_BALANCING
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index ebd75684cb0a..d98e4883f13d 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -1379,14 +1379,16 @@ static inline int
> > folio_wait_bit_common(struct folio *folio, int bit_nr,
> >  
> >  #ifdef CONFIG_MIGRATION
> >  /**
> > - * migration_entry_wait_on_locked - Wait for a migration entry to be
> > removed
> > - * @entry: migration swap entry.
> > + * softleaf_entry_wait_on_locked - Wait for a migration entry or
> > + * device_private entry to be removed.
> > + * @entry: migration or device_private swap entry.
> >   * @ptl: already locked ptl. This function will drop the lock.
> >   *
> > - * Wait for a migration entry referencing the given page to be
> > removed. This is
> > + * Wait for a migration entry referencing the given page, or
> > device_private
> > + * entry referencing a dvice_private page to be unlocked. This is
> >   * equivalent to folio_put_wait_locked(folio, TASK_UNINTERRUPTIBLE)
> > except
> >   * this can be called without taking a reference on the page.
> > Instead this
> > - * should be called while holding the ptl for the migration entry
> > referencing
> > + * should be called while holding the ptl for @entry referencing
> >   * the page.
> >   *
> >   * Returns after unlocking the ptl.
> > @@ -1394,7 +1396,7 @@ static inline int folio_wait_bit_common(struct
> > folio *folio, int bit_nr,
> >   * This follows the same logic as folio_wait_bit_common() so see the
> > comments
> >   * there.
> >   */
> > -void migration_entry_wait_on_locked(softleaf_t entry, spinlock_t
> > *ptl)
> > +void softleaf_entry_wait_on_locked(softleaf_t entry, spinlock_t
> > *ptl)
> >  	__releases(ptl)
> >  {
> >  	struct wait_page_queue wait_page;
> > @@ -1428,6 +1430,9 @@ void migration_entry_wait_on_locked(softleaf_t
> > entry, spinlock_t *ptl)
> >  	 * If a migration entry exists for the page the migration
> > path must hold
> >  	 * a valid reference to the page, and it must take the ptl
> > to remove the
> >  	 * migration entry. So the page is valid until the ptl is
> > dropped.
> > +	 * Similarly any path attempting to drop the last reference
> > to a
> > +	 * device-private page needs to grab the ptl to remove the
> > device-private
> > +	 * entry.
> >  	 */
> >  	spin_unlock(ptl);
> >  
> > diff --git a/mm/memory.c b/mm/memory.c
> > index da360a6eb8a4..20172476a57f 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -4684,7 +4684,8 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> >  				unlock_page(vmf->page);
> >  				put_page(vmf->page);
> >  			} else {
> > -				pte_unmap_unlock(vmf->pte, vmf-
> > >ptl);
> > +				pte_unmap(vmf->pte);
> > +				softleaf_entry_wait_on_locked(entry,
> > vmf->ptl);
> >  			}
> >  		} else if (softleaf_is_hwpoison(entry)) {
> >  			ret = VM_FAULT_HWPOISON;
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 4688b9e38cd2..cf6449b4202e 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -499,7 +499,7 @@ void migration_entry_wait(struct mm_struct *mm,
> > pmd_t *pmd,
> >  	if (!softleaf_is_migration(entry))
> >  		goto out;
> >  
> > -	migration_entry_wait_on_locked(entry, ptl);
> > +	softleaf_entry_wait_on_locked(entry, ptl);
> >  	return;
> >  out:
> >  	spin_unlock(ptl);
> > @@ -531,10 +531,10 @@ void migration_entry_wait_huge(struct
> > vm_area_struct *vma, unsigned long addr, p
> >  		 * If migration entry existed, safe to release vma
> > lock
> >  		 * here because the pgtable page won't be freed
> > without the
> >  		 * pgtable lock released.  See comment right above
> > pgtable
> > -		 * lock release in migration_entry_wait_on_locked().
> > +		 * lock release in softleaf_entry_wait_on_locked().
> >  		 */
> >  		hugetlb_vma_unlock_read(vma);
> > -		migration_entry_wait_on_locked(entry, ptl);
> > +		softleaf_entry_wait_on_locked(entry, ptl);
> >  		return;
> >  	}
> >  
> > @@ -552,7 +552,7 @@ void pmd_migration_entry_wait(struct mm_struct
> > *mm, pmd_t *pmd)
> >  	ptl = pmd_lock(mm, pmd);
> >  	if (!pmd_is_migration_entry(*pmd))
> >  		goto unlock;
> > -	migration_entry_wait_on_locked(softleaf_from_pmd(*pmd),
> > ptl);
> > +	softleaf_entry_wait_on_locked(softleaf_from_pmd(*pmd), ptl);
> >  	return;
> >  unlock:
> >  	spin_unlock(ptl);
> > diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> > index 23379663b1e1..deab89fd4541 100644
> > --- a/mm/migrate_device.c
> > +++ b/mm/migrate_device.c
> > @@ -176,7 +176,7 @@ static int migrate_vma_collect_huge_pmd(pmd_t
> > *pmdp, unsigned long start,
> >  		}
> >  
> >  		if (softleaf_is_migration(entry)) {
> > -			migration_entry_wait_on_locked(entry, ptl);
> > +			softleaf_entry_wait_on_locked(entry, ptl);
> >  			spin_unlock(ptl);
> >  			return -EAGAIN;
> >  		}