Re: [PATCH 03/10] mm/hmm: improve and rename hmm_vma_get_pfns() to hmm_range_snapshot()
From: Jerome Glisse
Date: Wed Feb 20 2019 - 19:28:14 EST
On Wed, Feb 20, 2019 at 04:25:07PM -0800, John Hubbard wrote:
> On 1/29/19 8:54 AM, jglisse@xxxxxxxxxx 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.
> >
> > Signed-off-by: Jérôme Glisse <jglisse@xxxxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Ralph Campbell <rcampbell@xxxxxxxxxx>
> > Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> > ---
> > 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