Re: [PATCH v2 04/11] mm/hmm: improve and rename hmm_vma_get_pfns() to hmm_range_snapshot() v2
From: Ira Weiny
Date: Thu Mar 28 2019 - 17:31:22 EST
On Mon, Mar 25, 2019 at 10:40:04AM -0400, Jerome Glisse wrote:
> From: Jérôme Glisse <jglisse@xxxxxxxxxx>
>
> Rename for consistency between code, comments and documentation. Also
> improves the comments on all the possible returns values. Improve the
> function by returning the number of populated entries in pfns array.
>
> Changes since v1:
> - updated documentation
> - reformated some comments
>
> Signed-off-by: Jérôme Glisse <jglisse@xxxxxxxxxx>
> Reviewed-by: Ralph Campbell <rcampbell@xxxxxxxxxx>
> Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>
Reviewed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
> Documentation/vm/hmm.rst | 26 ++++++++++++++++++--------
> include/linux/hmm.h | 4 ++--
> mm/hmm.c | 31 +++++++++++++++++--------------
> 3 files changed, 37 insertions(+), 24 deletions(-)
>
> diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst
> index 44205f0b671f..d9b27bdadd1b 100644
> --- a/Documentation/vm/hmm.rst
> +++ b/Documentation/vm/hmm.rst
> @@ -189,11 +189,7 @@ the driver callback returns.
> When the device driver wants to populate a range of virtual addresses, it can
> use either::
>
> - int hmm_vma_get_pfns(struct vm_area_struct *vma,
> - struct hmm_range *range,
> - unsigned long start,
> - unsigned long end,
> - hmm_pfn_t *pfns);
> + long hmm_range_snapshot(struct hmm_range *range);
> int hmm_vma_fault(struct vm_area_struct *vma,
> struct hmm_range *range,
> unsigned long start,
> @@ -202,7 +198,7 @@ When the device driver wants to populate a range of virtual addresses, it can
> bool write,
> bool block);
>
> -The first one (hmm_vma_get_pfns()) will only fetch present CPU page table
> +The first one (hmm_range_snapshot()) will only fetch present CPU page table
> entries and will not trigger a page fault on missing or non-present entries.
> The second one does trigger a page fault on missing or read-only entry if the
> write parameter is true. Page faults use the generic mm page fault code path
> @@ -220,19 +216,33 @@ Locking with the update() callback is the most important aspect the driver must
> {
> struct hmm_range range;
> ...
> +
> + range.start = ...;
> + range.end = ...;
> + range.pfns = ...;
> + range.flags = ...;
> + range.values = ...;
> + range.pfn_shift = ...;
> +
> again:
> - ret = hmm_vma_get_pfns(vma, &range, start, end, pfns);
> - if (ret)
> + down_read(&mm->mmap_sem);
> + range.vma = ...;
> + ret = hmm_range_snapshot(&range);
> + if (ret) {
> + up_read(&mm->mmap_sem);
> return ret;
> + }
> take_lock(driver->update);
> if (!hmm_vma_range_done(vma, &range)) {
> release_lock(driver->update);
> + up_read(&mm->mmap_sem);
> goto again;
> }
>
> // Use pfns array content to update device page table
>
> release_lock(driver->update);
> + up_read(&mm->mmap_sem);
> return 0;
> }
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 716fc61fa6d4..32206b0b1bfd 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -365,11 +365,11 @@ void hmm_mirror_unregister(struct hmm_mirror *mirror);
> * table invalidation serializes on it.
> *
> * YOU MUST CALL hmm_vma_range_done() ONCE AND ONLY ONCE EACH TIME YOU CALL
> - * hmm_vma_get_pfns() WITHOUT ERROR !
> + * hmm_range_snapshot() WITHOUT ERROR !
> *
> * IF YOU DO NOT FOLLOW THE ABOVE RULE THE SNAPSHOT CONTENT MIGHT BE INVALID !
> */
> -int hmm_vma_get_pfns(struct hmm_range *range);
> +long hmm_range_snapshot(struct hmm_range *range);
> bool hmm_vma_range_done(struct hmm_range *range);
>
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 213b0beee8d3..91361aa74b8b 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -698,23 +698,25 @@ static void hmm_pfns_special(struct hmm_range *range)
> }
>
> /*
> - * hmm_vma_get_pfns() - snapshot CPU page table for a range of virtual addresses
> - * @range: range being snapshotted
> - * Returns: -EINVAL if invalid argument, -ENOMEM out of memory, -EPERM invalid
> - * vma permission, 0 success
> + * hmm_range_snapshot() - snapshot CPU page table for a range
> + * @range: range
> + * Returns: number of valid pages in range->pfns[] (from range start
> + * address). This may be zero. If the return value is negative,
> + * then one of the following values may be returned:
> + *
> + * -EINVAL invalid arguments or mm or virtual address are in an
> + * invalid vma (ie either hugetlbfs or device file vma).
> + * -EPERM For example, asking for write, when the range is
> + * read-only
> + * -EAGAIN Caller needs to retry
> + * -EFAULT Either no valid vma exists for this range, or it is
> + * illegal to access the range
> *
> * This snapshots the CPU page table for a range of virtual addresses. Snapshot
> * validity is tracked by range struct. See hmm_vma_range_done() for further
> * information.
> - *
> - * The range struct is initialized here. It tracks the CPU page table, but only
> - * if the function returns success (0), in which case the caller must then call
> - * hmm_vma_range_done() to stop CPU page table update tracking on this range.
> - *
> - * NOT CALLING hmm_vma_range_done() IF FUNCTION RETURNS 0 WILL LEAD TO SERIOUS
> - * MEMORY CORRUPTION ! YOU HAVE BEEN WARNED !
> */
> -int hmm_vma_get_pfns(struct hmm_range *range)
> +long hmm_range_snapshot(struct hmm_range *range)
> {
> struct vm_area_struct *vma = range->vma;
> struct hmm_vma_walk hmm_vma_walk;
> @@ -768,6 +770,7 @@ int hmm_vma_get_pfns(struct hmm_range *range)
> hmm_vma_walk.fault = false;
> hmm_vma_walk.range = range;
> mm_walk.private = &hmm_vma_walk;
> + hmm_vma_walk.last = range->start;
>
> mm_walk.vma = vma;
> mm_walk.mm = vma->vm_mm;
> @@ -784,9 +787,9 @@ int hmm_vma_get_pfns(struct hmm_range *range)
> * function return 0).
> */
> range->hmm = hmm;
> - return 0;
> + return (hmm_vma_walk.last - range->start) >> PAGE_SHIFT;
> }
> -EXPORT_SYMBOL(hmm_vma_get_pfns);
> +EXPORT_SYMBOL(hmm_range_snapshot);
>
> /*
> * hmm_vma_range_done() - stop tracking change to CPU page table over a range
> --
> 2.17.2
>