Re: [PATCH] mm/gup.c: fix the wrong comments

[Ira Weiny]

On Thu, Apr 04, 2019 at 03:23:47PM +0800, Huang Shijie wrote:
> When CONFIG_HAVE_GENERIC_GUP is defined, the kernel will use its own
> get_user_pages_fast().
> 
> In the following scenario, we will may meet the bug in the DMA case:
> 	    .....................
> 	    get_user_pages_fast(start,,, pages);
> 	        ......
> 	    sg_alloc_table_from_pages(, pages, ...);
> 	    .....................
> 
> The root cause is that sg_alloc_table_from_pages() requires the
> page order to keep the same as it used in the user space, but
> get_user_pages_fast() will mess it up.

I wonder if there is something we can do to change sg_alloc_table_from_pages()
to work?  Reading the comment for it there is no indication of this limitation.
So should we update that comment as well?

> 
> So change the comments, and make it more clear for the driver
> users.
> 
> Signed-off-by: Huang Shijie <sjhuang@xxxxxxxxxxx>
> ---
>  mm/gup.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 22acdd0f79ff..b810d15d4db9 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1129,10 +1129,6 @@ EXPORT_SYMBOL(get_user_pages_locked);
>   *  with:
>   *
>   *      get_user_pages_unlocked(tsk, mm, ..., pages);
> - *
> - * It is functionally equivalent to get_user_pages_fast so
> - * get_user_pages_fast should be used instead if specific gup_flags
> - * (e.g. FOLL_FORCE) are not required.
>   */
>  long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
>  			     struct page **pages, unsigned int gup_flags)
> @@ -2147,6 +2143,10 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
>   * If not successful, it will fall back to taking the lock and
>   * calling get_user_pages().
>   *
> + * This function is different from the get_user_pages_unlocked():
> + *      The @pages may has different page order with the result
> + *      got by get_user_pages_unlocked().
> + *

I think I would word this a bit more generally.  Say:

<quote>
NOTE: Because get_user_pages_fast() walks the page tables to find the pages,
the order of pages returned may be different from those returned by other
get_user_pages_*() calls.
</quote>

Ira

>   * 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.
> -- 
> 2.17.1
>