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:6.902 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site