| Message ID | 20190129165428.3931-4-jglisse@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | HMM updates for 5.1 | expand |
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,
On Wed, Feb 20, 2019 at 04:25:07PM -0800, John Hubbard wrote: > 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) Will do a documentation patch to improve things and remove leftover. Cheers, Jérôme
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 - * 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). * * 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