Re: [PATCH v16 03/12] mm/hmm: preparations for HMM to participate in migration

["Mika Penttilä" <mpenttil@redhat.com>]

On 10/21/26 10:17, Balbir Singh wrote:

> On 10/21/26 5:04 PM, mpenttil@redhat.com wrote:
>> From: Mika Penttilä <mpenttil@redhat.com>
>>
>> For migration to happen after hmm_range_fault(), the vma has to
>> retrieved. Luckily, pagewalk already resolves that for us, so
>> just have to save it for further use. For migration, mmap_lock
>> has to be hold for the whole operation, so vma stays stable.
>> In case lock-drop behavior is requested, return an error while
>> migrating. Also, rename some struct hmm_vma_walk fields to solve
>> namespace collisions, and we are later introducing more
>> xxlocked fields so be unambiguous.
>>
>> Take care also of firing the mmu_notifier when migrating.
>>
>> Also, prepare for differencies of how mm_struct * is retrieved
>> in fault and migrate_vma paths.
>>
>> Cc: David Hildenbrand <david@kernel.org>
>> Cc: Jason Gunthorpe <jgg@nvidia.com>
>> Cc: Leon Romanovsky <leonro@nvidia.com>
>> Cc: Alistair Popple <apopple@nvidia.com>
>> Cc: Balbir Singh <balbirs@nvidia.com>
>> Cc: Zi Yan <ziy@nvidia.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Suggested-by: Alistair Popple <apopple@nvidia.com>
>> Signed-off-by: Mika Penttilä <mpenttil@redhat.com>
>> ---
>>  include/linux/migrate.h |  50 +++++++++++++++++-
>>  lib/test_hmm.c          |   2 +-
>>  mm/hmm.c                | 110 ++++++++++++++++++++++++++++++++++------
>>  3 files changed, 145 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
>> index 280ec9a866c7..dd7dd7ec6995 100644
>> --- a/include/linux/migrate.h
>> +++ b/include/linux/migrate.h
>> @@ -5,6 +5,7 @@
>>  #include <linux/mm.h>
>>  #include <linux/hmm.h>
>>  #include <linux/mempolicy.h>
>> +#include <linux/mmu_notifier.h>
>>  #include <linux/migrate_mode.h>
>>  #include <linux/hugetlb.h>
>>  
>> @@ -108,6 +109,29 @@ static inline void softleaf_entry_wait_on_locked(softleaf_t entry, spinlock_t *p
>>  	spin_unlock(ptl);
>>  }
>>  
>> +enum migrate_vma_info {
>> +	MIGRATE_VMA_SELECT_NONE = 0,
>> +	MIGRATE_VMA_SELECT_COMPOUND = MIGRATE_VMA_SELECT_NONE,
>> +};
>> +
>> +static inline enum migrate_vma_info hmm_select_migrate(struct hmm_range *range)
>> +{
>> +	return MIGRATE_VMA_SELECT_NONE;
>> +}
>> +
>> +static inline void hmm_fill_migrate_vma(struct hmm_range *range,
>> +					struct vm_area_struct *vma,
>> +					unsigned long start,
>> +					unsigned long end)
>> +{
>> +}
>> +
>> +#ifdef CONFIG_MMU_NOTIFIER
>> +static inline struct mm_struct *hmm_range_fault_mm(struct hmm_range *range)
>> +{
>> +	return range->notifier->mm;
>> +}
>> +#endif
>>  #endif /* CONFIG_MIGRATION */
>>  
>>  #ifdef CONFIG_NUMA_BALANCING
>> @@ -151,7 +175,8 @@ static inline unsigned long migrate_pfn(unsigned long pfn)
>>  	return (pfn << MIGRATE_PFN_SHIFT) | MIGRATE_PFN_VALID;
>>  }
>>  
>> -enum migrate_vma_direction {
>> +enum migrate_vma_info {
>> +	MIGRATE_VMA_SELECT_NONE = 0,
>>  	MIGRATE_VMA_SELECT_SYSTEM = 1 << 0,
>>  	MIGRATE_VMA_SELECT_DEVICE_PRIVATE = 1 << 1,
>>  	MIGRATE_VMA_SELECT_DEVICE_COHERENT = 1 << 2,
>> @@ -193,6 +218,29 @@ struct migrate_vma {
>>  	struct page		*fault_page;
>>  };
>>  
>> +// TODO: enable migration
>> +static inline enum migrate_vma_info hmm_select_migrate(struct hmm_range *range)
>> +{
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_MMU_NOTIFIER
>> +static inline struct mm_struct *hmm_range_fault_mm(struct hmm_range *range)
>> +{
>> +	return range->notifier ? range->notifier->mm : range->migrate->vma->vm_mm;
>> +}
>> +#endif
>> +
>> +static inline void hmm_fill_migrate_vma(struct hmm_range *range,
>> +					struct vm_area_struct *vma,
>> +					unsigned long start,
>> +					unsigned long end)
>> +{
>> +	range->migrate->vma   = vma;
>> +	range->migrate->start = start;
>> +	range->migrate->end   = end;
>> +}
>> +
>>  int migrate_vma_setup(struct migrate_vma *args);
>>  void migrate_vma_pages(struct migrate_vma *migrate);
>>  void migrate_vma_finalize(struct migrate_vma *migrate);
>> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
>> index 6911daa9f854..cd88e8177d0a 100644
>> --- a/lib/test_hmm.c
>> +++ b/lib/test_hmm.c
>> @@ -145,7 +145,7 @@ static bool dmirror_is_private_zone(struct dmirror_device *mdevice)
>>  		HMM_DMIRROR_MEMORY_DEVICE_PRIVATE);
>>  }
>>  
>> -static enum migrate_vma_direction
>> +static enum migrate_vma_info
>>  dmirror_select_device(struct dmirror *dmirror)
>>  {
>>  	return (dmirror->mdevice->zone_device_type ==
>> diff --git a/mm/hmm.c b/mm/hmm.c
>> index 2f1e98c6b644..4e9542aedc25 100644
>> --- a/mm/hmm.c
>> +++ b/mm/hmm.c
>> @@ -21,6 +21,7 @@
>>  #include <linux/pagemap.h>
>>  #include <linux/leafops.h>
>>  #include <linux/hugetlb.h>
>> +#include <linux/migrate.h>
>>  #include <linux/memremap.h>
>>  #include <linux/sched/mm.h>
>>  #include <linux/jump_label.h>
>> @@ -28,15 +29,23 @@
>>  #include <linux/pci-p2pdma.h>
>>  #include <linux/mmu_notifier.h>
>>  #include <linux/memory_hotplug.h>
>> +#include <asm/tlbflush.h>
>>  
>>  #include "internal.h"
>>  
>>  struct hmm_vma_walk {
>> -	struct hmm_range	*range;
>> -	bool			*locked;
>> -	unsigned long		last;
>> -	unsigned long		end;
>> -	unsigned int		required_fault;
>> +	struct mmu_notifier_range	mmu_range;
>> +	/* Set once mmu_range is armed with mmu_notifier_invalidate_range_start() */
>> +	bool				mmu_armed;
>> +	struct vm_area_struct		*vma;
>> +	struct hmm_range		*range;
>> +	unsigned long			start;
>> +	unsigned long			end;
>> +	unsigned long			last;
>> +	/* fault and lock drop related fields */
>> +	bool				*mmlocked;
>> +	unsigned long			fault_end;
>> +	unsigned int			required_fault;
>>  };
>>  
>>  /*
>> @@ -95,7 +104,7 @@ static int hmm_record_fault(unsigned long addr, unsigned long end,
>>  
>>  	WARN_ON_ONCE(!required_fault);
>>  	hmm_vma_walk->last = addr;
>> -	hmm_vma_walk->end = end;
>> +	hmm_vma_walk->fault_end = end;
>>  	hmm_vma_walk->required_fault = required_fault;
>>  	return HMM_FAULT_PENDING;
>>  }
>> @@ -400,6 +409,58 @@ static int hmm_vma_handle_absent_pmd(struct mm_walk *walk, unsigned long start,
>>  }
>>  #endif  /* CONFIG_ARCH_HAS_PMD_SOFTLEAVES */
>>  
>> +static int hmm_vma_capture_migrate_range(unsigned long start,
>> +					 unsigned long end,
>> +					 struct mm_walk *walk)
>> +{
>> +	struct hmm_vma_walk *hmm_vma_walk = walk->private;
>> +	struct hmm_range *range = hmm_vma_walk->range;
>> +
>> +	if (!hmm_select_migrate(range))
>> +		return 0;
>> +
>> +	if (hmm_vma_walk->vma && (hmm_vma_walk->vma != walk->vma))
>> +		return -ERANGE;
>> +
>> +	hmm_vma_walk->vma = walk->vma;
> I don't quite follow the check for -ERANGE, it checks for hmm_vma_walk->vma !=
> walk->vma, but then set's it later to be the same?

The intention is to catch a range that crosses a vma boundary. hmm_vma_walk lives across the restart loop in hmm_range_fault_locked(),
so test_walk can run again for the same vma after an -EBUSY re-walk. The first call records the vma, a re-walk of the same vma passes,
and a different vma means the range spans more than one vma, which migration doesn't support, so -ERANGE. 
The assignment only takes effect on the first call. I'll restructure it as "record if unset, else -ERANGE if different" and add a comment.

>
>> +
>> +	if (end - start > range->end - range->start)
>> +		return -ERANGE;
>> +
> Have we established that end > start and same for range->end > range->start?

Yes. walk_page_range() rejects start >= end before calling any callback, and hmm_range_fault_locked() walks from hmm_vma_walk.last 
(initially range->start) to range->end, so range->start < range->end holds whenever test_walk runs. 
The test_walk start/end are a sub-range of that walk clamped to the vma, so start < end too. 
That also means end - start can never exceed range->end - range->start, so the check is redundant. I'll drop it in v17.

>
>> +	if (!hmm_vma_walk->mmu_armed) {
>> +		hmm_vma_walk->start = start;
>> +		hmm_vma_walk->end = end;
> Does the walk range matter here?

The walk range is the caller's range clamped to the vma: with the caller's range inside a single vma the two are the same,
and the clamp keeps the walk range's end within the vma (migrate_vma needs that) if the caller's range extends past
the vma into a hole. The walk range is used as both the MMU_NOTIFY_MIGRATE invalidation range and the migrate_vma start/end,
and is set once on the first test_walk call. I'll also add a check that the walk range starts at range->start,
since migrate_vma indexes src[] from there.

>> +		mmu_notifier_range_init_owner(&hmm_vma_walk->mmu_range, MMU_NOTIFY_MIGRATE, 0,
>> +					      walk->vma->vm_mm, start, end,
>> +					      range->dev_private_owner);
>> +		mmu_notifier_invalidate_range_start(&hmm_vma_walk->mmu_range);
>> +		hmm_vma_walk->mmu_armed = true;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void hmm_vma_post_range_fault(struct hmm_vma_walk *hmm_vma_walk)
>> +{
>> +
>> +	struct hmm_range *range = hmm_vma_walk->range;
>> +
>> +	if (hmm_select_migrate(range) &&
>> +	    hmm_vma_walk->mmu_armed) {
>> +		/*
>> +		 *  The migrate_vma path has the following initialized,
>> +		 *  so take care of fault path below.
>> +		 */
>> +		if (range->notifier) {
>> +			hmm_fill_migrate_vma(range,
>> +					     hmm_vma_walk->vma,
>> +					     hmm_vma_walk->start,
>> +					     hmm_vma_walk->end);
>> +		}
>> +		mmu_notifier_invalidate_range_end(&hmm_vma_walk->mmu_range);
>> +	}
>> +}
>> +
>>  static int hmm_vma_walk_pmd(pmd_t *pmdp,
>>  			    unsigned long start,
>>  			    unsigned long end,
>> @@ -594,6 +655,11 @@ static int hmm_vma_walk_test(unsigned long start, unsigned long end,
>>  	struct hmm_vma_walk *hmm_vma_walk = walk->private;
>>  	struct hmm_range *range = hmm_vma_walk->range;
>>  	struct vm_area_struct *vma = walk->vma;
>> +	int r;
>> +
>> +	r = hmm_vma_capture_migrate_range(start, end, walk);
>> +	if (r)
>> +		return r;
>>  
>>  	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)) &&
>>  	    vma->vm_flags & VM_READ)
>> @@ -646,12 +712,12 @@ static int hmm_do_fault(struct mm_struct *mm,
>>  			struct hmm_vma_walk *hmm_vma_walk)
>>  {
>>  	unsigned long addr = hmm_vma_walk->last;
>> -	unsigned long end = hmm_vma_walk->end;
>> +	unsigned long end = hmm_vma_walk->fault_end;
>>  	unsigned int required_fault = hmm_vma_walk->required_fault;
>>  	unsigned int fault_flags = FAULT_FLAG_REMOTE;
>>  	struct vm_area_struct *vma;
>>  
>> -	if (hmm_vma_walk->locked)
>> +	if (hmm_vma_walk->mmlocked)
>>  		fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
>>  
>>  	vma = vma_lookup(mm, addr);
>> @@ -670,8 +736,8 @@ static int hmm_do_fault(struct mm_struct *mm,
>>  		ret = handle_mm_fault(vma, addr, fault_flags, NULL);
>>  
>>  		if (ret & (VM_FAULT_COMPLETED | VM_FAULT_RETRY)) {
>> -			if (hmm_vma_walk->locked)    /* needed by sparse */
>> -				*hmm_vma_walk->locked = false;
>> +			if (hmm_vma_walk->mmlocked)    /* needed by sparse */
>> +				*hmm_vma_walk->mmlocked = false;
>>  			else
>>  				WARN_ON_ONCE(1);    /* broken fault handler */
>>  			return HMM_FAULT_UNLOCKED;
>> @@ -694,19 +760,31 @@ static int hmm_range_fault_locked(struct hmm_range *range, bool *locked)
>>  {
>>  	struct hmm_vma_walk hmm_vma_walk = {
>>  		.range = range,
>> -		.locked = locked,
>> +		.mmlocked = locked,
>>  		.last = range->start,
>>  	};
>> -	struct mm_struct *mm = range->notifier->mm;
>> +	/*
>> +	 *  Could be serving a device fault or come from migrate
>> +	 *  entry point. For the former we have not resolved the vma
>> +	 *  yet, and the latter we don't have a notifier (but have a vma).
>> +	 *
>> +	 */
>> +	struct mm_struct *mm = hmm_range_fault_mm(range);
>>  	int ret;
>>  
>> +	/* Migration is incompatible with mmap lock drop */
>> +	if (locked && hmm_select_migrate(range))
>> +		return -EINVAL;
>> +
>>  	mmap_assert_locked(mm);
>>  
>>  	do {
>>  		/* If range is no longer valid force retry. */
>> -		if (mmu_interval_check_retry(range->notifier,
>> -					     range->notifier_seq))
>> -			return -EBUSY;
>> +		if (range->notifier && mmu_interval_check_retry(range->notifier,
>> +								range->notifier_seq)) {
>> +			ret =  -EBUSY;
>> +			break;
>> +		}
>>  		ret = walk_page_range(mm, hmm_vma_walk.last, range->end,
>>  				      &hmm_walk_ops, &hmm_vma_walk);
>>  		/*
>> @@ -732,6 +810,8 @@ static int hmm_range_fault_locked(struct hmm_range *range, bool *locked)
>>  		 * output, and all >= are still at their input values.
>>  		 */
>>  	} while (ret == -EBUSY);
>> +
>> +	hmm_vma_post_range_fault(&hmm_vma_walk);
> Have you checked to see if we could end up with notifier start/end inbalance?
> For example if hmm_select_migrate returns false?

Yes. invalidate_range_start() is only called from hmm_vma_capture_migrate_range(), after the hmm_select_migrate()
check and only once (mmu_armed). invalidate_range_end() is only called from hmm_vma_post_range_fault() when mmu_armed
is set. hmm_select_migrate() doesn't change during the call, so if it returns false neither is called.
Every exit from the loop goes through hmm_vma_post_range_fault(); except after test for lock drop which happens before walks.

>
>>  	return ret;
>>  }
>>  

Thanks,
--Mika