Re: [PATCH 03/10] mm/hmm: improve and rename hmm_vma_get_pfns() to hmm_range_snapshot()

From: John Hubbard <jhubbard@nvidia.com>
To: <jglisse@redhat.com>, <linux-mm@kvack.org>
Cc: <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Ralph Campbell <rcampbell@nvidia.com>
Subject: Re: [PATCH 03/10] mm/hmm: improve and rename hmm_vma_get_pfns() to hmm_range_snapshot()
Date: Wed, 20 Feb 2019 16:25:07 -0800	[thread overview]
Message-ID: <cc2d909c-37c6-4239-7755-4383a8eca0df@nvidia.com> (raw)
In-Reply-To: <20190129165428.3931-4-jglisse@redhat.com>

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