* [PATCH] mm/gup.c: fix the wrong comments
@ 2019-04-04 7:23 Huang Shijie
2019-04-04 12:32 ` William Kucharski
2019-04-04 16:50 ` Ira Weiny
0 siblings, 2 replies; 5+ messages in thread
From: Huang Shijie @ 2019-04-04 7:23 UTC (permalink / raw)
To: akpm; +Cc: ira.weiny, sfr, linux-mm, linux-kernel, Huang Shijie
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.
So change the comments, and make it more clear for the driver
users.
Signed-off-by: Huang Shijie <sjhuang@iluvatar.ai>
---
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().
+ *
* 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/gup.c: fix the wrong comments
2019-04-04 7:23 [PATCH] mm/gup.c: fix the wrong comments Huang Shijie
@ 2019-04-04 12:32 ` William Kucharski
2019-04-04 16:53 ` Weiny, Ira
2019-04-04 16:50 ` Ira Weiny
1 sibling, 1 reply; 5+ messages in thread
From: William Kucharski @ 2019-04-04 12:32 UTC (permalink / raw)
To: Huang Shijie; +Cc: akpm, ira.weiny, sfr, linux-mm, linux-kernel
> On Apr 4, 2019, at 1:23 AM, Huang Shijie <sjhuang@iluvatar.ai> wrote:
>
>
> + * 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 suggest a slight rewrite of the comment, something like:
* Note this routine may fill the pages array with entries in a
* different order than get_user_pages_unlocked(), which may cause
* issues for callers expecting the routines to be equivalent.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/gup.c: fix the wrong comments
2019-04-04 7:23 [PATCH] mm/gup.c: fix the wrong comments Huang Shijie
2019-04-04 12:32 ` William Kucharski
@ 2019-04-04 16:50 ` Ira Weiny
2019-04-08 1:23 ` Huang Shijie
1 sibling, 1 reply; 5+ messages in thread
From: Ira Weiny @ 2019-04-04 16:50 UTC (permalink / raw)
To: Huang Shijie; +Cc: akpm, sfr, linux-mm, linux-kernel
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@iluvatar.ai>
> ---
> 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] mm/gup.c: fix the wrong comments
2019-04-04 12:32 ` William Kucharski
@ 2019-04-04 16:53 ` Weiny, Ira
0 siblings, 0 replies; 5+ messages in thread
From: Weiny, Ira @ 2019-04-04 16:53 UTC (permalink / raw)
To: William Kucharski, Huang Shijie; +Cc: akpm, sfr, linux-mm, linux-kernel
> > On Apr 4, 2019, at 1:23 AM, Huang Shijie <sjhuang@iluvatar.ai> wrote:
> >
> >
> > + * 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 suggest a slight rewrite of the comment, something like:
>
> * Note this routine may fill the pages array with entries in a
> * different order than get_user_pages_unlocked(), which may cause
> * issues for callers expecting the routines to be equivalent.
This is good too. :-D
Ira
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/gup.c: fix the wrong comments
2019-04-04 16:50 ` Ira Weiny
@ 2019-04-08 1:23 ` Huang Shijie
0 siblings, 0 replies; 5+ messages in thread
From: Huang Shijie @ 2019-04-08 1:23 UTC (permalink / raw)
To: Ira Weiny; +Cc: akpm, sfr, linux-mm, linux-kernel
On Thu, Apr 04, 2019 at 09:50:47AM -0700, Ira Weiny wrote:
> 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.
The sg_alloc_table_from_pages() cannot work if the page order is wrong...
> So should we update that comment as well?
Okay.
I will create a DMA patch to add more comment for sg_alloc_table_from_pages().
Thanks
Huang Shijie
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-04-08 1:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-04 7:23 [PATCH] mm/gup.c: fix the wrong comments Huang Shijie
2019-04-04 12:32 ` William Kucharski
2019-04-04 16:53 ` Weiny, Ira
2019-04-04 16:50 ` Ira Weiny
2019-04-08 1:23 ` Huang Shijie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).