Re: [PATCH v17 11/12] Documentation/mm/hmm: document migration through hmm_range_fault()

[sashiko-bot@kernel.org]

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ä <mpenttil@redhat.com>

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/20261103061629.972669-1-mpenttil@redhat.com?part=11