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

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

[Liu Xiang]

Because nr_pages is unsigned long, it can not be negative.

Signed-off-by: Liu Xiang <liuxiang_1999@126.com>
---
 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.
  *
-- 
1.9.1

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

[David Hildenbrand]

On 23.10.19 15:51, Liu Xiang wrote:
> Because nr_pages is unsigned long, it can not be negative.
> 
> Signed-off-by: Liu Xiang <liuxiang_1999@126.com>
> ---
>  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.

Can we fix the "when it is finished with" part right away, too? At least
it sounds wrong to me, but I am not a native speaker.

Simply "Each page returned must be released with put_page()." should be
good enough

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

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@126.com>
> ---
>  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

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

[Liu Xiang]

At 2019-10-24 03:28:14, "John Hubbard" <jhubbard@nvidia.com> wrote:
>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@126.com>
>> ---
>>  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


Hi John,

Thanks for your suggestion. I will send a new patch.

Regards

Liu Xiang