Messages in this thread | | | Subject | Re: [PATCH 03/10] mm/hmm: improve and rename hmm_vma_get_pfns() to hmm_range_snapshot() | From | John Hubbard <> | Date | Wed, 20 Feb 2019 16:25:07 -0800 |
| |
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
| |