Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] mm/hmm: hmm_vma_walk_pmd() skips remaining PTEs on folio split failure --
commit 467255657c2d81a197c74dccb6095cef3e48051a Author: Mika Penttilä <[email protected]> mm/hmm: do the plumbing for HMM to participate in migration This commit prepares hmm_range_fault() and the HMM pagewalk callbacks for migration collection by adding PMD and PTE locking during the walk and wiring in the migration preparation hooks. > diff --git a/mm/hmm.c b/mm/hmm.c > index 9220e6648636d..854d2c090b20c 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c [ ... ] > @@ -532,24 +739,65 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp, > * recover. > */ > if (pmd_bad(pmd)) { > - if (hmm_range_need_fault(hmm_vma_walk, hmm_pfns, npages, 0)) > + if (hmm_range_need_fault(hmm_vma_walk, hmm_pfns_start, npages, > 0)) > return -EFAULT; > - return hmm_pfns_fill(start, end, range, HMM_PFN_ERROR); > + return hmm_pfns_fill(start, end, hmm_vma_walk, HMM_PFN_ERROR); > } > > - ptep = pte_offset_map(pmdp, addr); > - if (!ptep) > + if (minfo) { > + ptep = pte_offset_map_lock(mm, pmdp, addr, &hmm_vma_walk->ptl); > + if (ptep) > + hmm_vma_walk->ptelocked = true; > + } else { > + ptep = pte_offset_map(pmdp, addr); > + } > + if (!ptep) { > + addr = start; > goto again; > + } > + > for (; addr < end; addr += PAGE_SIZE, ptep++, hmm_pfns++) { > - int r; > > r = hmm_vma_handle_pte(walk, addr, end, pmdp, ptep, hmm_pfns); > if (r) { > - /* hmm_vma_handle_pte() did pte_unmap() */ > + /* hmm_vma_handle_pte() did pte_unmap() / > pte_unmap_unlock */ > + if (unmapped) > + flush_tlb_range(walk->vma, start, addr); > return r; > } > + > + r = hmm_vma_handle_migrate_prepare(walk, pmdp, ptep, addr, > hmm_pfns, &unmapped); > + if (r == -EAGAIN) { > + HMM_ASSERT_UNLOCKED(hmm_vma_walk); > + if (unmapped) { > + flush_tlb_range(walk->vma, start, addr); > + unmapped = false; > + } > + goto again; > + } > + if (r) { > + /* > + * Splitting the folio failed; migrate_vma_split_folio() > + * already dropped the PTE lock. Don't migrate the rest > + * of the pmd range; addr keeps its valid pfn. > + */ > + HMM_ASSERT_UNLOCKED(hmm_vma_walk); > + if (unmapped) > + flush_tlb_range(walk->vma, start, addr); > + return 0; > + } [Severity: Medium] As reported by Sashiko on v16 [1], when hmm_vma_handle_migrate_prepare() returns a non-zero error other than -EAGAIN in hmm_vma_walk_pmd() (such as when migrate_vma_split_folio() fails to split a PTE-mapped large folio in the subsequent patch "mm/hmm: migrate collection in HMM pagewalk - pte level"), does returning 0 here leave the remaining entries in [addr + PAGE_SIZE, end) completely unprocessed? At the start of a migrating walk, hmm_range_fault_locked() clears range->hmm_pfns[]: mm/hmm.c:hmm_range_fault_locked() { ... if (hmm_select_migrate(range)) { WARN_ON_ONCE(range->pfn_flags_mask); WARN_ON_ONCE(range->dev_private_owner != range->migrate->pgmap_owner); memset(range->hmm_pfns, 0, ((range->end - range->start) >> PAGE_SHIFT) * sizeof(*range->hmm_pfns)); } ... } While the current address addr already ran hmm_vma_handle_pte() and keeps its valid PFN, returning 0 directly from the PTE loop terminates processing for the rest of the PMD range [addr + PAGE_SIZE, end) while reporting success to walk_page_range(). Consequently, for any addresses in [addr + PAGE_SIZE, end), neither hmm_vma_handle_pte() nor hmm_vma_handle_migrate_prepare() is called. Independent migratable pages in that remainder are not collected, unmapped or read-only PTEs are not faulted in even when HMM_PFN_REQ_FAULT is requested, and hmm_range_fault() can return 0 with those range->hmm_pfns[] entries still zeroed and lacking HMM_PFN_VALID. [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
