Re: [PATCH v16 10/12] lib/test_hmm: add a new testcase for the migrate on fault

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mm: test_hmm: unmapped device pages on retry in do_fault_and_migrate()
- [Medium] selftests/mm: pre-faulting buffer in migrate_on_fault skips hmm_do_fault()
--

commit 43a1dd324a1ecfe103c0cf9ba76ea8935506e50c
Author: Mika Penttilä <mpenttil@redhat.com>

lib/test_hmm: add a new testcase for the migrate on fault

This commit adds the HMM_DMIRROR_MIGRATE_ON_FAULT_TO_DEV ioctl to the
test_hmm driver to test faulting and migrating memory to device private
memory via hmm_range_fault(), along with a corresponding kselftest.

> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> index f05f801b8404b..2045896493ae3 100644
> --- a/lib/test_hmm.c
> +++ b/lib/test_hmm.c

[ ... ]

> @@ -1377,6 +1398,127 @@ static int dmirror_migrate_to_device(struct dmirror *dmirror,
>  	return ret;
>  }
>  
> +static int do_fault_and_migrate(struct dmirror *dmirror, struct hmm_range *range)
> +{
> +	struct migrate_vma *migrate = range->migrate;
> +	unsigned long timeout =
> +		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> +	int ret;
> +
> +	do {
> +		mmap_read_lock(dmirror->notifier.mm);
> +
> +		/* Fault-in pages for migration */
> +		ret = dmirror_range_fault(dmirror, range);
> +		/*
> +		 * Set this up even on error: hmm_range_fault() may have collected
> +		 * part of the range before failing.
> +		 */
> +		migrate_hmm_range_setup(range);
> +		if (ret) {
> +			/*
> +			 * dst[] entries are empty, so this marks every collected
> +			 * migration as failed. finalize then restores the source
> +			 * mappings and drops the associated locks/references.
> +			 */
> +			migrate_vma_pages(migrate);
> +			migrate_vma_finalize(migrate);
> +			goto unlock;
> +		}
> +
> +		pr_debug("Migrating from sys mem to device mem\n");
> +
> +		dmirror_migrate_alloc_and_copy(migrate, dmirror);
> +		migrate_vma_pages(migrate);
> +
> +		mutex_lock(&dmirror->mutex);
> +		if (mmu_interval_read_retry(&dmirror->notifier,
> +					    range->notifier_seq))
> +			ret = -EBUSY;
> +		else
> +			ret = __dmirror_migrate_map_locked(migrate, dmirror);
> +		mutex_unlock(&dmirror->mutex);
> +
> +		migrate_vma_finalize(migrate);
> +unlock:
> +		mmap_read_unlock(dmirror->notifier.mm);
> +	} while (ret == -EBUSY && !time_after(jiffies, timeout));
> +
> +	return ret;
> +}

[Severity: Medium]
What happens in do_fault_and_migrate() if mmu_interval_read_retry() returns
true after migrate_vma_pages() has already migrated the folios?

When mmu_interval_read_retry() is true (for example, if an invalidation bumps
dmirror->notifier.invalidate_seq after dmirror_range_fault() drops
dmirror->mutex, such as during migrate_vma_unmap() on a shared page or
split_huge_pmd() in migrate_vma_insert_page()), __dmirror_migrate_map_locked()
is skipped and ret is set to -EBUSY, but migrate_vma_finalize() still commits
the device private entries into the CPU page tables.

On the next iteration of the retry loop, dmirror_fault_and_migrate_to_device()
has configured migrate.flags with only MIGRATE_VMA_SELECT_SYSTEM:

    migrate.pgmap_owner = dmirror->mdevice;
    migrate.src = src_pfns;
    migrate.dst = dst_pfns;
    migrate.flags = MIGRATE_VMA_SELECT_SYSTEM;

Because the pages are now in device private memory, the retry walk in
hmm_range_fault() will not set HMM_PFN_MIGRATE on them, and
dmirror_range_fault() also skips dmirror_do_fault() when HMM_PFN_REQ_MIGRATE
is set:

    /* update device page table after migration */
    if (!migrate)
            ret = dmirror_do_fault(dmirror, range);

Won't this leave those migrated pages missing from dmirror->pt, causing the
subsequent dmirror_do_read() call in dmirror_fault_and_migrate_to_device() to
fail with -ENOENT?

[ ... ]

> diff --git a/tools/testing/selftests/mm/hmm-tests.c b/tools/testing/selftests/mm/hmm-tests.c
> index e5f273ca84c10..897ff2eb34061 100644
> --- a/tools/testing/selftests/mm/hmm-tests.c
> +++ b/tools/testing/selftests/mm/hmm-tests.c

[ ... ]

> @@ -937,6 +944,53 @@ TEST_F(hmm, migrate)
>  	hmm_buffer_free(buffer);
>  }
>  
> +
> +/*
> + * Fault and migrate anonymous memory to device private memory.
> + */
> +TEST_F(hmm, migrate_on_fault)
> +{
> +	struct hmm_buffer *buffer;
> +	unsigned long npages;
> +	unsigned long size;
> +	unsigned long i;
> +	int *ptr;
> +	int ret;
> +
> +	npages = ALIGN(HMM_BUFFER_SIZE, self->page_size) >> self->page_shift;
> +	ASSERT_NE(npages, 0);
> +	size = npages << self->page_shift;
> +
> +	buffer = malloc(sizeof(*buffer));
> +	ASSERT_NE(buffer, NULL);
> +
> +	buffer->fd = -1;
> +	buffer->size = size;
> +	buffer->mirror = malloc(size);
> +	ASSERT_NE(buffer->mirror, NULL);
> +
> +	buffer->ptr = mmap(NULL, size,
> +			   PROT_READ | PROT_WRITE,
> +			   MAP_PRIVATE | MAP_ANONYMOUS,
> +			   buffer->fd, 0);
> +	ASSERT_NE(buffer->ptr, MAP_FAILED);
> +
> +	/* Initialize buffer in system memory. */
> +	for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
> +		ptr[i] = i;
> +
> +	/* Fault and migrate memory to device. */
> +	ret = hmm_migrate_on_fault_sys_to_dev(self->fd, buffer, npages);

[Severity: Medium]
Does TEST_F(hmm, migrate_on_fault) actually trigger page faults during
hmm_range_fault()?

Writing to every element of buffer->ptr in the initialization loop populates
all PTEs in the range as present and writable before
hmm_migrate_on_fault_sys_to_dev() is called. When hmm_range_fault() then walks
the range with HMM_PFN_REQ_FAULT | HMM_PFN_REQ_MIGRATE, hmm_pte_need_fault()
in mm/hmm.c sees HMM_PFN_VALID on every PTE and returns 0:

    /* If CPU page table is not valid then we need to fault */
    if (!(cpu_flags & HMM_PFN_VALID))
            return HMM_NEED_FAULT;
    return 0;

As a result, hmm_record_fault() and hmm_do_fault() are never invoked during
the migration walk. Should part or all of the buffer be left unpopulated
before calling hmm_migrate_on_fault_sys_to_dev() (similar to TEST_F(hmm,
anon_read)), and could this also use the existing hmm_buffer_alloc() helper
instead of open-coding the buffer and mmap allocation?

> +	ASSERT_EQ(ret, 0);
> +	ASSERT_EQ(buffer->cpages, npages);
> +
> +	/* Check what the device read. */
> +	for (i = 0, ptr = buffer->mirror; i < size / sizeof(*ptr); ++i)
> +		ASSERT_EQ(ptr[i], i);
> +
> +	hmm_buffer_free(buffer);
> +}

[ ... ]

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