[PATCH 0/2] mm/gup: introduce pin_user_pages_locked(), use it in frame_vector.c

[PATCH 0/2] mm/gup: introduce pin_user_pages_locked(), use it in frame_vector.c

[John Hubbard]

Hi,

This applies on top of the pin_user_pages_unlocked() patch that I sent
earlier [1].

This adds yet one more pin_user_pages*() variant, and uses that to
convert mm/frame_vector.c.

With this, along with maybe 20 or 30 other recent patches in various
trees, we are close to having the relevant gup call sites
converted--with the notable exception of the bio/block layer.


[1] https://lore.kernel.org/r/20200518012157.1178336-2-jhubbard@nvidia.com

John Hubbard (2):
  mm/gup: introduce pin_user_pages_locked()
  mm/gup: frame_vector: convert get_user_pages() --> pin_user_pages()

 include/linux/mm.h |  2 ++
 mm/frame_vector.c  |  7 +++----
 mm/gup.c           | 30 ++++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 4 deletions(-)


base-commit: 9cb1fd0efd195590b828b9b865421ad345a4a145
-- 
2.26.2

[PATCH 1/2] mm/gup: introduce pin_user_pages_locked()

[John Hubbard]

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

Re: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()

[David Hildenbrand]

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>

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()

[Souptick Joarder]

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
>
>

Re: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()

[Souptick Joarder]

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
>
>

Re: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()

[Souptick Joarder]

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
> >
> >

Re: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()

[Souptick Joarder]

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
> >
> >

Re: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()

[Pankaj Gupta]

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
>
>

Re: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()

[Pankaj Gupta]

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
>
>

[PATCH 2/2] mm/gup: frame_vector: convert get_user_pages() --> pin_user_pages()

[John Hubbard]

This code was using get_user_pages*(), and all of the callers so far
were in a "Case 2" scenario (DMA/RDMA), using the categorization
from [1]. That means that it's time to convert the get_user_pages*() +
put_page() calls to pin_user_pages*() + unpin_user_pages() calls.

There is some helpful background in [2]: basically, this is a small
part of fixing a long-standing disconnect between pinning pages, and
file systems' use of those pages.

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
    https://lwn.net/Articles/807108/

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 mm/frame_vector.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/mm/frame_vector.c b/mm/frame_vector.c
index c431ca81dad5..4107dbca0056 100644
--- a/mm/frame_vector.c
+++ b/mm/frame_vector.c
@@ -72,7 +72,7 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
 	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) {
 		vec->got_ref = true;
 		vec->is_pfns = false;
-		ret = get_user_pages_locked(start, nr_frames,
+		ret = pin_user_pages_locked(start, nr_frames,
 			gup_flags, (struct page **)(vec->ptrs), &locked);
 		goto out;
 	}
@@ -122,7 +122,6 @@ EXPORT_SYMBOL(get_vaddr_frames);
  */
 void put_vaddr_frames(struct frame_vector *vec)
 {
-	int i;
 	struct page **pages;
 
 	if (!vec->got_ref)
@@ -135,8 +134,8 @@ void put_vaddr_frames(struct frame_vector *vec)
 	 */
 	if (WARN_ON(IS_ERR(pages)))
 		goto out;
-	for (i = 0; i < vec->nr_frames; i++)
-		put_page(pages[i]);
+
+	unpin_user_pages(pages, vec->nr_frames);
 	vec->got_ref = false;
 out:
 	vec->nr_frames = 0;
-- 
2.26.2

Re: [PATCH 2/2] mm/gup: frame_vector: convert get_user_pages() --> pin_user_pages()

[David Hildenbrand]

On 28.05.20 00:32, John Hubbard wrote:
> This code was using get_user_pages*(), and all of the callers so far
> were in a "Case 2" scenario (DMA/RDMA), using the categorization
> from [1]. That means that it's time to convert the get_user_pages*() +
> put_page() calls to pin_user_pages*() + unpin_user_pages() calls.
> 
> There is some helpful background in [2]: basically, this is a small
> part of fixing a long-standing disconnect between pinning pages, and
> file systems' use of those pages.
> 
> [1] Documentation/core-api/pin_user_pages.rst
> 
> [2] "Explicit pinning of user-space pages":
>     https://lwn.net/Articles/807108/
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  mm/frame_vector.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/frame_vector.c b/mm/frame_vector.c
> index c431ca81dad5..4107dbca0056 100644
> --- a/mm/frame_vector.c
> +++ b/mm/frame_vector.c
> @@ -72,7 +72,7 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
>  	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) {
>  		vec->got_ref = true;
>  		vec->is_pfns = false;
> -		ret = get_user_pages_locked(start, nr_frames,
> +		ret = pin_user_pages_locked(start, nr_frames,
>  			gup_flags, (struct page **)(vec->ptrs), &locked);
>  		goto out;
>  	}
> @@ -122,7 +122,6 @@ EXPORT_SYMBOL(get_vaddr_frames);
>   */
>  void put_vaddr_frames(struct frame_vector *vec)
>  {
> -	int i;
>  	struct page **pages;
>  
>  	if (!vec->got_ref)
> @@ -135,8 +134,8 @@ void put_vaddr_frames(struct frame_vector *vec)
>  	 */
>  	if (WARN_ON(IS_ERR(pages)))
>  		goto out;
> -	for (i = 0; i < vec->nr_frames; i++)
> -		put_page(pages[i]);
> +
> +	unpin_user_pages(pages, vec->nr_frames);
>  	vec->got_ref = false;
>  out:
>  	vec->nr_frames = 0;
> 

Not familiar with this code, but from what I can tell this LGTM.

-- 
Thanks,

David / dhildenb