| Message ID | 20200527223243.884385-2-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gup: introduce pin_user_pages_locked(), use it in frame_vector.c | expand |
On 28.05.20 00:32, John Hubbard wrote: > Introduce pin_user_pages_locked(), which is nearly identical to > get_user_pages_locked() except that it sets FOLL_PIN and rejects > FOLL_GET. > > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- > include/linux/mm.h | 2 ++ > mm/gup.c | 30 ++++++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 98be7289d7e9..d16951087c93 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1700,6 +1700,8 @@ long pin_user_pages(unsigned long start, unsigned long nr_pages, > struct vm_area_struct **vmas); > long get_user_pages_locked(unsigned long start, unsigned long nr_pages, > unsigned int gup_flags, struct page **pages, int *locked); > +long pin_user_pages_locked(unsigned long start, unsigned long nr_pages, > + unsigned int gup_flags, struct page **pages, int *locked); > long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > struct page **pages, unsigned int gup_flags); > long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > diff --git a/mm/gup.c b/mm/gup.c > index 2f0a0b497c23..17418a949067 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2992,3 +2992,33 @@ long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > return get_user_pages_unlocked(start, nr_pages, pages, gup_flags); > } > EXPORT_SYMBOL(pin_user_pages_unlocked); > + > +/* > + * pin_user_pages_locked() is the FOLL_PIN variant of get_user_pages_locked(). > + * Behavior is the same, except that this one sets FOLL_PIN and rejects > + * FOLL_GET. > + */ > +long pin_user_pages_locked(unsigned long start, unsigned long nr_pages, > + unsigned int gup_flags, struct page **pages, > + int *locked) > +{ > + /* > + * FIXME: Current FOLL_LONGTERM behavior is incompatible with > + * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on > + * vmas. As there are no users of this flag in this call we simply > + * disallow this option for now. > + */ > + if (WARN_ON_ONCE(gup_flags & FOLL_LONGTERM)) > + return -EINVAL; > + > + /* FOLL_GET and FOLL_PIN are mutually exclusive. */ > + if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > + return -EINVAL; > + > + gup_flags |= FOLL_PIN; > + return __get_user_pages_locked(current, current->mm, start, nr_pages, > + pages, NULL, locked, > + gup_flags | FOLL_TOUCH); > +} > +EXPORT_SYMBOL(pin_user_pages_locked); > + > LGTM Reviewed-by: David Hildenbrand <david@redhat.com>
On Thu, May 28, 2020 at 4:02 AM John Hubbard <jhubbard@nvidia.com> wrote: > > Introduce pin_user_pages_locked(), which is nearly identical to > get_user_pages_locked() except that it sets FOLL_PIN and rejects > FOLL_GET. > > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- > include/linux/mm.h | 2 ++ > mm/gup.c | 30 ++++++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 98be7289d7e9..d16951087c93 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1700,6 +1700,8 @@ long pin_user_pages(unsigned long start, unsigned long nr_pages, > struct vm_area_struct **vmas); > long get_user_pages_locked(unsigned long start, unsigned long nr_pages, > unsigned int gup_flags, struct page **pages, int *locked); > +long pin_user_pages_locked(unsigned long start, unsigned long nr_pages, > + unsigned int gup_flags, struct page **pages, int *locked); > long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > struct page **pages, unsigned int gup_flags); > long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > diff --git a/mm/gup.c b/mm/gup.c > index 2f0a0b497c23..17418a949067 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2992,3 +2992,33 @@ long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > return get_user_pages_unlocked(start, nr_pages, pages, gup_flags); > } > EXPORT_SYMBOL(pin_user_pages_unlocked); > + > +/* > + * pin_user_pages_locked() is the FOLL_PIN variant of get_user_pages_locked(). > + * Behavior is the same, except that this one sets FOLL_PIN and rejects > + * FOLL_GET. > + */ > +long pin_user_pages_locked(unsigned long start, unsigned long nr_pages, > + unsigned int gup_flags, struct page **pages, > + int *locked) > +{ > + /* > + * FIXME: Current FOLL_LONGTERM behavior is incompatible with > + * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on > + * vmas. As there are no users of this flag in this call we simply > + * disallow this option for now. > + */ > + if (WARN_ON_ONCE(gup_flags & FOLL_LONGTERM)) > + return -EINVAL; > + > + /* FOLL_GET and FOLL_PIN are mutually exclusive. */ > + if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > + return -EINVAL; > + > + gup_flags |= FOLL_PIN; Right now get_user_pages_locked() doesn't have similar check for FOLL_PIN and also not setting FOLL_GET internally irrespective of gup_flags passed by user. Do we need to add the same in get_user_pages_locked() ? > + return __get_user_pages_locked(current, current->mm, start, nr_pages, > + pages, NULL, locked, > + gup_flags | FOLL_TOUCH); > +} > +EXPORT_SYMBOL(pin_user_pages_locked); > + > -- > 2.26.2 > >
On Sun, May 31, 2020 at 12:34 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: > > On Thu, May 28, 2020 at 4:02 AM John Hubbard <jhubbard@nvidia.com> wrote: > > > > Introduce pin_user_pages_locked(), which is nearly identical to > > get_user_pages_locked() except that it sets FOLL_PIN and rejects > > FOLL_GET. Forget to ask, is it fine to add this new pin_user_pages_locked() in Documentation/core-api/pin_user_pages.rst ? > > > > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > > --- > > include/linux/mm.h | 2 ++ > > mm/gup.c | 30 ++++++++++++++++++++++++++++++ > > 2 files changed, 32 insertions(+) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 98be7289d7e9..d16951087c93 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -1700,6 +1700,8 @@ long pin_user_pages(unsigned long start, unsigned long nr_pages, > > struct vm_area_struct **vmas); > > long get_user_pages_locked(unsigned long start, unsigned long nr_pages, > > unsigned int gup_flags, struct page **pages, int *locked); > > +long pin_user_pages_locked(unsigned long start, unsigned long nr_pages, > > + unsigned int gup_flags, struct page **pages, int *locked); > > long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > > struct page **pages, unsigned int gup_flags); > > long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > > diff --git a/mm/gup.c b/mm/gup.c > > index 2f0a0b497c23..17418a949067 100644 > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -2992,3 +2992,33 @@ long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > > return get_user_pages_unlocked(start, nr_pages, pages, gup_flags); > > } > > EXPORT_SYMBOL(pin_user_pages_unlocked); > > + > > +/* > > + * pin_user_pages_locked() is the FOLL_PIN variant of get_user_pages_locked(). > > + * Behavior is the same, except that this one sets FOLL_PIN and rejects > > + * FOLL_GET. > > + */ > > +long pin_user_pages_locked(unsigned long start, unsigned long nr_pages, > > + unsigned int gup_flags, struct page **pages, > > + int *locked) > > +{ > > + /* > > + * FIXME: Current FOLL_LONGTERM behavior is incompatible with > > + * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on > > + * vmas. As there are no users of this flag in this call we simply > > + * disallow this option for now. > > + */ > > + if (WARN_ON_ONCE(gup_flags & FOLL_LONGTERM)) > > + return -EINVAL; > > + > > + /* FOLL_GET and FOLL_PIN are mutually exclusive. */ > > + if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > > + return -EINVAL; > > + > > + gup_flags |= FOLL_PIN; > > Right now get_user_pages_locked() doesn't have similar check for FOLL_PIN > and also not setting FOLL_GET internally irrespective of gup_flags > passed by user. > Do we need to add the same in get_user_pages_locked() ? > > > + return __get_user_pages_locked(current, current->mm, start, nr_pages, > > + pages, NULL, locked, > > + gup_flags | FOLL_TOUCH); > > +} > > +EXPORT_SYMBOL(pin_user_pages_locked); > > + > > -- > > 2.26.2 > > > >
Acked-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com> On Thu, 28 May 2020 at 00:32, John Hubbard <jhubbard@nvidia.com> wrote: > > Introduce pin_user_pages_locked(), which is nearly identical to > get_user_pages_locked() except that it sets FOLL_PIN and rejects > FOLL_GET. > > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- > include/linux/mm.h | 2 ++ > mm/gup.c | 30 ++++++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 98be7289d7e9..d16951087c93 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1700,6 +1700,8 @@ long pin_user_pages(unsigned long start, unsigned long nr_pages, > struct vm_area_struct **vmas); > long get_user_pages_locked(unsigned long start, unsigned long nr_pages, > unsigned int gup_flags, struct page **pages, int *locked); > +long pin_user_pages_locked(unsigned long start, unsigned long nr_pages, > + unsigned int gup_flags, struct page **pages, int *locked); > long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > struct page **pages, unsigned int gup_flags); > long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > diff --git a/mm/gup.c b/mm/gup.c > index 2f0a0b497c23..17418a949067 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2992,3 +2992,33 @@ long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > return get_user_pages_unlocked(start, nr_pages, pages, gup_flags); > } > EXPORT_SYMBOL(pin_user_pages_unlocked); > + > +/* > + * pin_user_pages_locked() is the FOLL_PIN variant of get_user_pages_locked(). > + * Behavior is the same, except that this one sets FOLL_PIN and rejects > + * FOLL_GET. > + */ > +long pin_user_pages_locked(unsigned long start, unsigned long nr_pages, > + unsigned int gup_flags, struct page **pages, > + int *locked) > +{ > + /* > + * FIXME: Current FOLL_LONGTERM behavior is incompatible with > + * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on > + * vmas. As there are no users of this flag in this call we simply > + * disallow this option for now. > + */ > + if (WARN_ON_ONCE(gup_flags & FOLL_LONGTERM)) > + return -EINVAL; > + > + /* FOLL_GET and FOLL_PIN are mutually exclusive. */ > + if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > + return -EINVAL; > + > + gup_flags |= FOLL_PIN; > + return __get_user_pages_locked(current, current->mm, start, nr_pages, > + pages, NULL, locked, > + gup_flags | FOLL_TOUCH); > +} > +EXPORT_SYMBOL(pin_user_pages_locked); > + > -- > 2.26.2 > >
On 2020-05-31 00:04, Souptick Joarder wrote: ... >> +/* >> + * pin_user_pages_locked() is the FOLL_PIN variant of get_user_pages_locked(). >> + * Behavior is the same, except that this one sets FOLL_PIN and rejects >> + * FOLL_GET. >> + */ >> +long pin_user_pages_locked(unsigned long start, unsigned long nr_pages, >> + unsigned int gup_flags, struct page **pages, >> + int *locked) >> +{ >> + /* >> + * FIXME: Current FOLL_LONGTERM behavior is incompatible with >> + * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on >> + * vmas. As there are no users of this flag in this call we simply >> + * disallow this option for now. >> + */ >> + if (WARN_ON_ONCE(gup_flags & FOLL_LONGTERM)) >> + return -EINVAL; >> + >> + /* FOLL_GET and FOLL_PIN are mutually exclusive. */ >> + if (WARN_ON_ONCE(gup_flags & FOLL_GET)) >> + return -EINVAL; >> + >> + gup_flags |= FOLL_PIN; > > Right now get_user_pages_locked() doesn't have similar check for FOLL_PIN Yes, that should be added... > and also not setting FOLL_GET internally irrespective of gup_flags > passed by user. > Do we need to add the same in get_user_pages_locked() ? ...and no, that should not. Yes, it's prudent to assert that FOLL_PIN is *not* set, at all the get_user_pages*() API calls, thanks for spotting that. I'll add that to this patch and send out a v2. The same check should also be added to get_user_pages_unlocked(). I'll send out a correction (I think just a v3 of that patchset) to add that. The setting of FOLL_GET, on the other hand, is something best left as-is so far. Some call sites set FOLL_GET, some want it *not* set, and some expect that FOLL_GET is implied, and at the moment, the delicate balance is correct. :) thanks,
On 2020-05-31 00:13, Souptick Joarder wrote: > On Sun, May 31, 2020 at 12:34 PM Souptick Joarder <jrdr.linux@gmail.com> wrote: >> >> On Thu, May 28, 2020 at 4:02 AM John Hubbard <jhubbard@nvidia.com> wrote: >>> >>> Introduce pin_user_pages_locked(), which is nearly identical to >>> get_user_pages_locked() except that it sets FOLL_PIN and rejects >>> FOLL_GET. > > Forget to ask, is it fine to add this new pin_user_pages_locked() in > Documentation/core-api/pin_user_pages.rst ? > I wasn't planning on maintaining a rigorous list of all of the pin_user_page*() API calls, in pin_user_pages.rst. Each call is documented in gup.c, for that. The .rst is there to help explain things at a slighly higher abstraction level. thanks,
diff --git a/include/linux/mm.h b/include/linux/mm.h index 98be7289d7e9..d16951087c93 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1700,6 +1700,8 @@ long pin_user_pages(unsigned long start, unsigned long nr_pages, struct vm_area_struct **vmas); long get_user_pages_locked(unsigned long start, unsigned long nr_pages, unsigned int gup_flags, struct page **pages, int *locked); +long pin_user_pages_locked(unsigned long start, unsigned long nr_pages, + unsigned int gup_flags, struct page **pages, int *locked); long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, struct page **pages, unsigned int gup_flags); long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, diff --git a/mm/gup.c b/mm/gup.c index 2f0a0b497c23..17418a949067 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -2992,3 +2992,33 @@ long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, return get_user_pages_unlocked(start, nr_pages, pages, gup_flags); } EXPORT_SYMBOL(pin_user_pages_unlocked); + +/* + * pin_user_pages_locked() is the FOLL_PIN variant of get_user_pages_locked(). + * Behavior is the same, except that this one sets FOLL_PIN and rejects + * FOLL_GET. + */ +long pin_user_pages_locked(unsigned long start, unsigned long nr_pages, + unsigned int gup_flags, struct page **pages, + int *locked) +{ + /* + * FIXME: Current FOLL_LONGTERM behavior is incompatible with + * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on + * vmas. As there are no users of this flag in this call we simply + * disallow this option for now. + */ + if (WARN_ON_ONCE(gup_flags & FOLL_LONGTERM)) + return -EINVAL; + + /* FOLL_GET and FOLL_PIN are mutually exclusive. */ + if (WARN_ON_ONCE(gup_flags & FOLL_GET)) + return -EINVAL; + + gup_flags |= FOLL_PIN; + return __get_user_pages_locked(current, current->mm, start, nr_pages, + pages, NULL, locked, + gup_flags | FOLL_TOUCH); +} +EXPORT_SYMBOL(pin_user_pages_locked); +
Introduce pin_user_pages_locked(), which is nearly identical to get_user_pages_locked() except that it sets FOLL_PIN and rejects FOLL_GET. Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- include/linux/mm.h | 2 ++ mm/gup.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)