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ä <[email protected]> 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/[email protected]?part=10
