Re: [PATCH v17 06/12] mm/hmm: migrate collection in HMM pagewalk - pmd level

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] mm/hmm: missing folio_test_anon() check in hmm_vma_handle_migrate_prepare_pmd()
- [High] mm/hmm: missing device-coherent checks in hmm_vma_handle_migrate_prepare_pmd()
- [High] mm/hmm: ABBA folio lock deadlock in migrate_vma_split_folio()
- [Medium] mm/hmm: PMD split in hmm_vma_handle_migrate_prepare_pmd() aborts notifier walk
--

commit 2ed090a278a29feb7fac7d83140b748506dedc9e
Author: Mika Penttilä <mpenttil@redhat.com>

mm/hmm: migrate collection in HMM pagewalk - pmd level

This commit implements hmm_vma_handle_migrate_prepare_pmd() to collect
PMD-mapped THPs or split them into PTEs during HMM pagewalk migration, and
factors out __migrate_vma_split_folio() for splitting folios.

> diff --git a/mm/hmm.c b/mm/hmm.c
> index ec9280ef26822..cbde004fd778a 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -511,9 +537,7 @@ static int migrate_vma_split_folio(struct folio *folio,
>  				   struct hmm_vma_walk *hmm_vma_walk,
>  				   pte_t *ptep)
>  {
> -	int ret;
>  	struct folio *fault_folio = fault_page ? page_folio(fault_page) : NULL;
> -	struct folio *new_fault_folio = NULL;
>  
>  	/*
>  	 * Take the reference while still holding the PTE lock, so the folio
> @@ -530,44 +554,136 @@ static int migrate_vma_split_folio(struct folio *folio,
>  	if (folio != fault_folio)
>  		folio_lock(folio);
>  
> -	ret = split_folio(folio);
> -	if (ret) {
> -		if (folio != fault_folio) {
> +	return __migrate_vma_split_folio(folio, fault_page);
> +}

[Severity: High]
Can calling folio_lock(folio) in migrate_vma_split_folio() cause an ABBA
deadlock when another folio is already held locked in the walk?

During a migrating hmm_range_fault() walk, the calling thread already holds
folio locks on fault_folio (when called from do_swap_page() ->
migrate_to_ram()) as well as on any folios collected at earlier addresses
in the range via hmm_vma_handle_migrate_prepare_pmd() or
hmm_vma_handle_migrate_prepare().

While normal collection in hmm_vma_handle_migrate_prepare() and
hmm_vma_handle_migrate_prepare_pmd() uses folio_trylock() to avoid deadlocking
against concurrent migrations or faults, migrate_vma_split_folio() drops the
PTE lock and sleeps unconditionally in folio_lock(folio):

  Thread 1                              Thread 2
  holds folio A locked
    (fault_folio or earlier entry)      holds folio B locked
  hmm_vma_handle_migrate_prepare()
    migrate_vma_split_folio(folio B)
      folio_lock(folio B)               migrate_vma_split_folio(folio A)
                                          folio_lock(folio A)
                                          (or migration_entry_wait(folio A))

Should migrate_vma_split_folio() also use folio_trylock(folio) and skip
migrating the folio if it cannot be locked?

> +
> +static int hmm_vma_handle_migrate_prepare_pmd(const struct mm_walk *walk,
> +					      pmd_t *pmdp,
> +					      unsigned long start,
> +					      unsigned long end,
> +					      unsigned long *hmm_pfn)
> +{
> +	struct hmm_vma_walk *hmm_vma_walk = walk->private;
> +	struct hmm_range *range = hmm_vma_walk->range;
> +	struct migrate_vma *migrate = range->migrate;
> +	struct folio *fault_folio = NULL;
> +	enum migrate_vma_info minfo;
> +	struct folio *folio;
> +	unsigned long i;
> +	int r = 0;
> +
> +	// Do we want to migrate at all?
> +	minfo = hmm_select_migrate(range);
> +	if (!minfo)
> +		return r;
> +
> +	WARN_ON_ONCE(!migrate);
> +	HMM_ASSERT_PMD_LOCKED(hmm_vma_walk, true);
> +
> +	fault_folio = migrate->fault_page ?
> +		page_folio(migrate->fault_page) : NULL;
> +
> +	if (pmd_none(*pmdp))
> +		return hmm_pfns_fill(start, end, hmm_vma_walk, 0);
> +
> +	/* Like at the pte level, NUMA hinting (protnone) pmds are migrated */
> +	if (!(hmm_pfn[0] & HMM_PFN_VALID) &&
> +	    !(pmd_trans_huge(*pmdp) && pmd_protnone(*pmdp)))
> +		goto out;
> +
> +	if (pmd_trans_huge(*pmdp)) {
> +		if (!(minfo & MIGRATE_VMA_SELECT_SYSTEM))
> +			goto out;

[Severity: High]
How are PMD-mapped MEMORY_DEVICE_COHERENT folios handled here?

Compound device-coherent folios are mapped as present huge PMDs where
pmd_trans_huge(*pmdp) is true (via migrate_vma_insert_huge_pmd_page() and
remove_migration_pmd()). At the PTE level, hmm_vma_handle_migrate_prepare()
distinguishes system memory from device-coherent pages:

mm/hmm.c:hmm_vma_handle_migrate_prepare() {
    ...
		page = vm_normal_page(walk->vma, addr, pte);
		if (page && !is_zone_device_page(page) &&
		    !(minfo & MIGRATE_VMA_SELECT_SYSTEM)) {
			goto out;
		} else if (page && is_device_coherent_page(page)) {
			pgmap = page_pgmap(page);

			if (!(minfo &
			      MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
			    pgmap->owner != migrate->pgmap_owner)
				goto out;
		}
    ...
}

In hmm_vma_handle_migrate_prepare_pmd(), requiring MIGRATE_VMA_SELECT_SYSTEM
for all pmd_trans_huge(*pmdp) entries without checking
folio_is_zone_device(folio), folio_is_device_coherent(folio),
MIGRATE_VMA_SELECT_DEVICE_COHERENT, or folio->pgmap->owner means that
migrating device-coherent memory to system RAM with
MIGRATE_VMA_SELECT_DEVICE_COHERENT skips present device-coherent huge PMDs,
while migrating with MIGRATE_VMA_SELECT_SYSTEM collects or splits
device-coherent huge PMDs regardless of pgmap->owner.

Does the pmd_trans_huge(*pmdp) branch need the same zone_device and
device_coherent checks as hmm_vma_handle_migrate_prepare()?

> +
> +		folio = pmd_folio(*pmdp);
> +		if (is_huge_zero_folio(folio))
> +			return hmm_pfns_fill(start, end, hmm_vma_walk, 0);
> +
> +	} else if (!pmd_present(*pmdp)) {

[Severity: High]
Should hmm_vma_handle_migrate_prepare_pmd() check folio_test_anon(folio)
before collecting or splitting a PMD-mapped folio?

At the PTE level, hmm_vma_handle_migrate_prepare() rejects non-anonymous
folios:

mm/hmm.c:hmm_vma_handle_migrate_prepare() {
    ...
	/* Only anonymous memory can be migrated to device memory */
	if (!page || !folio_test_anon(page_folio(page)))
		goto out;
    ...
}

Because hmm_vma_capture_migrate_range() and migrate_vma_setup() allow file and
shmem VMAs, a PMD in such a VMA can map a non-anonymous THP. Without a
folio_test_anon(folio) check in hmm_vma_handle_migrate_prepare_pmd():

1. If compound migration is selected, the page-cache folio is locked and a PMD
   migration entry is installed with HMM_PFN_MIGRATE | HMM_PFN_COMPOUND.
   Holding a page-cache folio locked while hmm_do_fault() -> handle_mm_fault()
   faults subsequent pages in the VMA inverts the mapping->invalidate_lock ->
   folio_lock ordering. If the caller later splits the unmapped compound
   folio in __migrate_device_pages() -> migrate_vma_split_unmapped_folio(),
   split_huge_pmd_address() zaps the non-anonymous PMD without dropping the
   extra folio reference, and folio_split_unmapped() triggers
   VM_WARN_ON_ONCE_FOLIO(!folio_test_anon(folio), folio) before running
   __folio_freeze_split_anon() on a page-cache folio:

mm/huge_memory.c:folio_split_unmapped() {
	VM_WARN_ON_ONCE_FOLIO(folio_mapped(folio), folio);
	VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
	VM_WARN_ON_ONCE_FOLIO(!folio_test_large(folio), folio);
	VM_WARN_ON_ONCE_FOLIO(!folio_test_anon(folio), folio);

	return __folio_freeze_split_anon(folio, new_order, &folio->page,
					 false, NULL, SPLIT_TYPE_UNIFORM);
}

2. If the split fallback is taken, __migrate_vma_split_folio() ->
   split_folio() -> __folio_freeze_split_file() acquires
   i_mmap_lock_read(mapping) while holding locks on previously collected
   folios and unmaps the file folio completely via unmap_folio(). If the
   split fails after unmapping, hmm_vma_handle_migrate_prepare_pmd() returns 0
   with stale HMM_PFN_VALID entries for a zapped PMD; if it succeeds, the
   re-walk finds pte_none entries and loses the valid PFNs when
   HMM_PFN_REQ_FAULT is not set.

[ ... ]

> +split:
> +	spin_unlock(hmm_vma_walk->ptl);
> +	hmm_vma_walk->pmdlocked = false;
>  
>  	/*
> -	 * Ensure the lock is held on the correct
> -	 * folio after the split
> +	 * folio_get() above took an extra reference. For the fault folio the
> +	 * caller still holds a reference and the lock, which is the
> +	 * precondition of __migrate_vma_split_folio(), so drop the extra one.
>  	 */
> -	if (!new_fault_folio) {
> -		folio_unlock(folio);
> +	if (folio == fault_folio)
>  		folio_put(folio);
> -	} else if (folio != new_fault_folio) {
> -		if (new_fault_folio != fault_folio) {
> -			folio_get(new_fault_folio);
> -			folio_lock(new_fault_folio);
> -		}
> -		folio_unlock(folio);
> -		folio_put(folio);
> -	}
>  
> -	return 0;
> -}
> +	/* Split failed: not migrated, the valid pfns stay as reported */
> +	if (__migrate_vma_split_folio(folio, migrate->fault_page))
> +		return 0;
>  
> -static int hmm_vma_handle_migrate_prepare_pmd(const struct mm_walk *walk,
> -					      pmd_t *pmdp,
> -					      unsigned long start,
> -					      unsigned long end,
> -					      unsigned long *hmm_pfn)
> -{
> -	// TODO: implement migration entry insertion
> -	return 0;
> +	/* Re-walked as PTEs from start: drop the pmd-level output */
> +	for (i = 0; i < (end - start) >> PAGE_SHIFT; i++)
> +		hmm_pfn[i] &= HMM_PFN_INOUT_FLAGS;
> +
> +	hmm_vma_walk->last = start;
> +	return -EBUSY;
>  }
>  
>  /*
> @@ -1016,6 +1132,11 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
>  			hmm_vma_walk->pmdlocked = false;
>  		}
>  
> +		/*
> +		 * hmm_vma_handle_migrate_prepare_pmd() splits the huge pmd in
> +		 * place when needed and returns -EBUSY to re-walk the range as
> +		 * PTEs.
> +		 */
>  		return r;
>  	}

[Severity: Medium]
When range->notifier is set during a migrating hmm_range_fault() call, does
returning -EBUSY out of walk_page_range() after splitting a PMD cause
hmm_range_fault_locked() to abort the walk instead of re-walking the split
PMD as PTEs?

In hmm_vma_handle_migrate_prepare_pmd(), __migrate_vma_split_folio() calls
split_folio() or split_huge_page() -> __folio_split() ->
__folio_freeze_split_anon() -> unmap_folio() -> try_to_migrate_one(), which
invokes mmu_notifier_invalidate_range_start() with MMU_NOTIFY_CLEAR and
range.owner == NULL. Because the event is MMU_NOTIFY_CLEAR rather than
MMU_NOTIFY_MIGRATE with range->dev_private_owner, the registered
mmu_interval_notifier updates range->notifier->invalidate_seq.

When hmm_vma_handle_migrate_prepare_pmd() and hmm_vma_walk_pmd() return
-EBUSY back to hmm_range_fault_locked(), the next iteration of the loop checks
mmu_interval_check_retry():

mm/hmm.c:hmm_range_fault_locked() {
    ...
	do {
		/* If range is no longer valid force retry. */
		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);
    ...
}

Because range->notifier was invalidated by the split,
mmu_interval_check_retry() returns true and hmm_range_fault_locked() breaks
out with -EBUSY, forcing a full rollback and retry of the range. By contrast,
the PTE-level split in hmm_vma_handle_migrate_prepare() returns -EAGAIN and
restarts locally at the again label inside hmm_vma_walk_pmd() without
returning to hmm_range_fault_locked().

Could hmm_vma_walk_pmd() similarly loop locally to the again label when
hmm_vma_handle_migrate_prepare_pmd() splits the PMD?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103061629.972669-1-mpenttil@redhat.com?part=6