lkml.org 
[lkml]   [2019]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 03/10] mm/hmm: improve and rename hmm_vma_get_pfns() to hmm_range_snapshot()
From
Date
On 1/29/19 8:54 AM, jglisse@redhat.com wrote:
> From: Jérôme Glisse <jglisse@redhat.com>
>
> 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.
>
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Ralph Campbell <rcampbell@nvidia.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> ---
> include/linux/hmm.h | 4 ++--
> mm/hmm.c | 23 ++++++++++-------------
> 2 files changed, 12 insertions(+), 15 deletions(-)
>

Hi Jerome,

After applying the entire patchset, I still see a few hits of the old name,
in Documentation:

$ git grep -n hmm_vma_get_pfns
Documentation/vm/hmm.rst:192: int hmm_vma_get_pfns(struct vm_area_struct *vma,
Documentation/vm/hmm.rst:205:The first one (hmm_vma_get_pfns()) will only fetch
present CPU page table
Documentation/vm/hmm.rst:224: ret = hmm_vma_get_pfns(vma, &range, start,
end, pfns);
include/linux/hmm.h:145: * HMM pfn value returned by hmm_vma_get_pfns() or
hmm_vma_fault() will be:


> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index bd6e058597a6..ddf49c1b1f5e 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 74d69812d6be..0d9ecd3337e5 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -706,23 +706,19 @@ 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
> + * hmm_range_snapshot() - snapshot CPU page table for a range
> + * @range: range
> * Returns: -EINVAL if invalid argument, -ENOMEM out of memory, -EPERM invalid

Channeling Mike Rapoport, that should be @Return: instead of Returns: , but...


> - * vma permission, 0 success
> + * permission (for instance asking for write and range is read only),
> + * -EAGAIN if you need to retry, -EFAULT invalid (ie either no valid
> + * vma or it is illegal to access that range), number of valid pages
> + * in range->pfns[] (from range start address).

...actually, that's a little hard to spot that we're returning number of valid
pages. How about:

* @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 range->invalid is set, or range->start or range->end
* are not valid.
* -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

(caution: my white space might be wrong with respect to tabs)

> *
> * 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;
> @@ -776,6 +772,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;
> @@ -792,9 +789,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
>

Otherwise, looks good.

thanks,
--
John Hubbard
NVIDIA

\
 
 \ /
  Last update: 2019-02-21 01:26    [W:2.489 / U:0.744 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site