| Message ID | 20190404072347.3440-1-sjhuang@iluvatar.ai (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gup.c: fix the wrong comments | expand |
> 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.
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 >
> > 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
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
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.
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(-)