Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] mm/hmm: incomplete error rollback steps for migrate_hmm_range_setup() in docs --
commit 908a5d19d6ca6398929f82b830d122f054aa64a3 Author: Mika Penttilä <[email protected]> Documentation/mm/hmm: document migration through hmm_range_fault() This commit documents the MIGRATE_VMA_FAULT and MIGRATE_VMA_WRITE flags for migrate_vma_setup(), as well as driving the migration collection phase directly from hmm_range_fault() via HMM_PFN_REQ_MIGRATE and migrate_hmm_range_setup(). > diff --git a/Documentation/mm/hmm.rst b/Documentation/mm/hmm.rst > index fc1b8dc198250..914ef6bfe8486 100644 > --- a/Documentation/mm/hmm.rst > +++ b/Documentation/mm/hmm.rst [ ... ] > @@ -427,6 +434,44 @@ between device driver specific code and shared common > code: > > The lock can now be released. > > +Migration collection through hmm_range_fault() > +============================================== [ ... ] > +After hmm_range_fault() returns, the driver calls:: > + > + void migrate_hmm_range_setup(struct hmm_range *range); > + > +to translate ``range->hmm_pfns`` into ``migrate->src[]`` (``migrate->dst[]`` > is > +zeroed) and initialize ``migrate->cpages`` and ``migrate->npages``. From > here on > +the ``range->migrate`` struct is ready for the rest of the flow (steps 3 > +onwards), i.e. migrate_vma_pages() and migrate_vma_finalize(). This should be > +called even on error, since hmm_range_fault() may have collected part of the > +range before failing. [Severity: Low] Could this wording be clarified regarding what needs to be called when hmm_range_fault() returns an error? In mm/migrate_device.c, migrate_hmm_range_setup() only translates range->hmm_pfns into migrate->src[], zeroes migrate->dst[], initializes migrate->cpages and migrate->npages, and calls migrate_vma_unmap(). Calling migrate_hmm_range_setup() alone on error would leave migration PTEs installed and leave collected folios isolated from the LRU, locked, and with elevated reference counts. On the other hand, if "This" is read as referring to the rest of the flow (steps 3 onwards), step 3 in Documentation/mm/hmm.rst instructs the driver to allocate destination pages, copy source pages, and populate migrate->dst[], which would commit the partial migration rather than aborting it. As described in the kernel-doc for hmm_range_fault() in mm/hmm.c: * On any other error, or when abandoning a retry, the caller must call * migrate_hmm_range_setup(), migrate_vma_pages() with no valid destination * entries, and migrate_vma_finalize() to abort the partial migration. Should this paragraph explicitly state that on error the driver must call migrate_hmm_range_setup(), skip step 3 so migrate->dst[] remains zeroed, and still call migrate_vma_pages() and migrate_vma_finalize() to restore the original PTEs and release the folio locks and references? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
