Re: [PATCH] mm: gup: fix comment of __get_user_pages()

[John Hubbard]

On 10/23/19 6:51 AM, Liu Xiang wrote:
> Because nr_pages is unsigned long, it can not be negative.
> 
> Signed-off-by: Liu Xiang <liuxiang_1999@xxxxxxx>
> ---
>  mm/gup.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 8f236a3..0236954 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -735,10 +735,10 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
>   * @nonblocking: whether waiting for disk IO or mmap_sem contention
>   *
>   * Returns number of pages pinned. This may be fewer than the number
> - * requested. If nr_pages is 0 or negative, returns 0. If no pages
> - * were pinned, returns -errno. Each page returned must be released
> - * with a put_page() call when it is finished with. vmas will only
> - * remain valid while mmap_sem is held.
> + * requested. If nr_pages is 0, returns 0. If no pages were pinned,
> + * returns -errno. Each page returned must be released with a
> + * put_page() call when it is finished with. vmas will only remain
> + * valid while mmap_sem is held.
>   *
>   * Must be called with mmap_sem held.  It may be released.  See below.
>   *
> 

Hi Liu,

Thanks for fixing the documentation! As long as you're there...for the actual 
wording, can we please do it as shown below? This also addresses David's 
feedback, and it makes this read a lot better overall:

diff --git a/mm/gup.c b/mm/gup.c
index 8f236a335ae9..5816876fee51 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -734,11 +734,17 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
  *             Or NULL if the caller does not require them.
  * @nonblocking: whether waiting for disk IO or mmap_sem contention
  *
- * Returns number of pages pinned. This may be fewer than the number
- * requested. If nr_pages is 0 or negative, returns 0. If no pages
- * were pinned, returns -errno. Each page returned must be released
- * with a put_page() call when it is finished with. vmas will only
- * remain valid while mmap_sem is held.
+ * Returns either number of pages pinned (which may be less than the number
+ * requested), or an error. Details about the return value:
+ *
+ *      -- If nr_pages is 0, returns 0.
+ *      -- If nr_pages is >0, but no pages were pinned, returns -errno.
+ *      -- If nr_pages is >0, and some pages were pinned, returns the number of
+ *         pages pinned. Again, this may be less than nr_pages.
+ *
+ * The caller is responsible for releasing returned @pages, via put_page().
+ *
+ * @vmas are valid only as long as mmap_sem is held.
  *
  * Must be called with mmap_sem held.  It may be released.  See below.
  *


Patch ordering: I'll have to change the above as part of my upcoming series, to
make it refer to "put_page() or put_user_page(), depending on FOLL_PIN", 
approximately. (Just more of a note to self than anything else, here.)

thanks,

John Hubbard
NVIDIA