[RFC] mm/gup.c: Use gup_flags as parameter instead of passing write flag

[RFC] mm/gup.c: Use gup_flags as parameter instead of passing write flag

[Souptick Joarder]

The idea is to get rid of write parameter. Instead caller will pass
FOLL_WRITE to __get_user_pages_fast(). This will not change any
functionality of the API. Once it is upstream all the callers will
be changed to pass FOLL_WRITE.

Updated the documentation of the API.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
---
 include/linux/mm.h |  4 ++--
 mm/gup.c           | 18 +++++++++++-------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5a32342..15dd594 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1816,8 +1816,8 @@ extern int mprotect_fixup(struct vm_area_struct *vma,
 /*
  * doesn't attempt to fault and will return short.
  */
-int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
-			  struct page **pages);
+int __get_user_pages_fast(unsigned long start, int nr_pages,
+			unsigned int gup_flags, struct page **pages);
 /*
  * per-process(per-mm_struct) statistics.
  */
diff --git a/mm/gup.c b/mm/gup.c
index 87a6a59..a8f869e 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2666,7 +2666,14 @@ static bool gup_fast_permitted(unsigned long start, unsigned long end)
 }
 #endif
 
-/*
+/**
+ * __get_user_pages_fast() - pin user pages in memory
+ * @start:      starting user address
+ * @nr_pages:   number of pages from start to pin
+ * @gup_flags:  flags modifying pin behaviour
+ * @pages:      array that receives pointers to the pages pinned.
+ *              Should be at least nr_pages long.
+ *
  * Like get_user_pages_fast() except it's IRQ-safe in that it won't fall back to
  * the regular GUP.
  * Note a difference with get_user_pages_fast: this always returns the
@@ -2675,8 +2682,8 @@ static bool gup_fast_permitted(unsigned long start, unsigned long end)
  * If the architecture does not support this function, simply return with no
  * pages pinned.
  */
-int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
-			  struct page **pages)
+int __get_user_pages_fast(unsigned long start, int nr_pages,
+			unsigned int gup_flags, struct page **pages)
 {
 	unsigned long len, end;
 	unsigned long flags;
@@ -2685,10 +2692,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 	 * Internally (within mm/gup.c), gup fast variants must set FOLL_GET,
 	 * because gup fast is always a "pin with a +1 page refcount" request.
 	 */
-	unsigned int gup_flags = FOLL_GET;
-
-	if (write)
-		gup_flags |= FOLL_WRITE;
+	gup_flags |= FOLL_GET;
 
 	start = untagged_addr(start) & PAGE_MASK;
 	len = (unsigned long) nr_pages << PAGE_SHIFT;
-- 
1.9.1

Re: [RFC] mm/gup.c: Use gup_flags as parameter instead of passing write flag

[Matthew Wilcox]

On Tue, May 19, 2020 at 01:28:23AM +0530, Souptick Joarder wrote:
> The idea is to get rid of write parameter. Instead caller will pass
> FOLL_WRITE to __get_user_pages_fast(). This will not change any
> functionality of the API. Once it is upstream all the callers will
> be changed to pass FOLL_WRITE.

Uhh ... until you change all the callers, haven't you just broken all
the callers?

> -int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> -			  struct page **pages)
> +int __get_user_pages_fast(unsigned long start, int nr_pages,
> +			unsigned int gup_flags, struct page **pages)
>  {
>  	unsigned long len, end;
>  	unsigned long flags;
> @@ -2685,10 +2692,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
>  	 * Internally (within mm/gup.c), gup fast variants must set FOLL_GET,
>  	 * because gup fast is always a "pin with a +1 page refcount" request.
>  	 */
> -	unsigned int gup_flags = FOLL_GET;
> -
> -	if (write)
> -		gup_flags |= FOLL_WRITE;
> +	gup_flags |= FOLL_GET;

Re: [RFC] mm/gup.c: Use gup_flags as parameter instead of passing write flag

[Souptick Joarder]

On Tue 19 May, 2020, 1:47 AM Matthew Wilcox, <willy@infradead.org> wrote:
>
> On Tue, May 19, 2020 at 01:28:23AM +0530, Souptick Joarder wrote:
> > The idea is to get rid of write parameter. Instead caller will pass
> > FOLL_WRITE to __get_user_pages_fast(). This will not change any
> > functionality of the API. Once it is upstream all the callers will
> > be changed to pass FOLL_WRITE.
>
> Uhh ... until you change all the callers, haven't you just broken all
> the callers?

All the callers have called the API with either 1 or 0.  I think, it's
not going to break
any of the callers.

>
> > -int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> > -                       struct page **pages)
> > +int __get_user_pages_fast(unsigned long start, int nr_pages,
> > +                     unsigned int gup_flags, struct page **pages)
> >  {
> >       unsigned long len, end;
> >       unsigned long flags;
> > @@ -2685,10 +2692,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> >        * Internally (within mm/gup.c), gup fast variants must set FOLL_GET,
> >        * because gup fast is always a "pin with a +1 page refcount" request.
> >        */
> > -     unsigned int gup_flags = FOLL_GET;
> > -
> > -     if (write)
> > -             gup_flags |= FOLL_WRITE;
> > +     gup_flags |= FOLL_GET;

Re: [RFC] mm/gup.c: Use gup_flags as parameter instead of passing write flag

[Souptick Joarder]

On Tue 19 May, 2020, 1:47 AM Matthew Wilcox, <willy@infradead.org> wrote:
>
> On Tue, May 19, 2020 at 01:28:23AM +0530, Souptick Joarder wrote:
> > The idea is to get rid of write parameter. Instead caller will pass
> > FOLL_WRITE to __get_user_pages_fast(). This will not change any
> > functionality of the API. Once it is upstream all the callers will
> > be changed to pass FOLL_WRITE.
>
> Uhh ... until you change all the callers, haven't you just broken all
> the callers?

All the callers have called the API with either 1 or 0.  I think, it's
not going to break
any of the callers.

>
> > -int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> > -                       struct page **pages)
> > +int __get_user_pages_fast(unsigned long start, int nr_pages,
> > +                     unsigned int gup_flags, struct page **pages)
> >  {
> >       unsigned long len, end;
> >       unsigned long flags;
> > @@ -2685,10 +2692,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> >        * Internally (within mm/gup.c), gup fast variants must set FOLL_GET,
> >        * because gup fast is always a "pin with a +1 page refcount" request.
> >        */
> > -     unsigned int gup_flags = FOLL_GET;
> > -
> > -     if (write)
> > -             gup_flags |= FOLL_WRITE;
> > +     gup_flags |= FOLL_GET;

Re: [RFC] mm/gup.c: Use gup_flags as parameter instead of passing write flag

[John Hubbard]

On 2020-05-18 13:44, Souptick Joarder wrote:
> On Tue 19 May, 2020, 1:47 AM Matthew Wilcox, <willy@infradead.org> wrote:
>>
>> On Tue, May 19, 2020 at 01:28:23AM +0530, Souptick Joarder wrote:
>>> The idea is to get rid of write parameter. Instead caller will pass
>>> FOLL_WRITE to __get_user_pages_fast(). This will not change any
>>> functionality of the API. Once it is upstream all the callers will
>>> be changed to pass FOLL_WRITE.
>>
>> Uhh ... until you change all the callers, haven't you just broken all
>> the callers?
> 
> All the callers have called the API with either 1 or 0.  I think, it's
> not going to break
> any of the callers.

Maybe so, but it's still "wrong" to do that, because it only works more
or less accidentally. That is, it works in spite of a type safety
violation. So we don't want to do that sort of thing unless there is
a compelling reason.

In addition to that, I am at the exact moment putting together a minor
refactoring of this function, because I need a FOLL_PIN variant:
__pin_user_pages_fast(), as part of my work to change over a few dozen
gup call sites to pin_user_pages*().

(In fact, I was wondering whether to stick with the "write" parameter, for
the new __pin_user_pages_fast(), or go with gup_flags. That could go either
way: gup_flags provides a nicer API, but "write" matches the existing
callers.)

So in other words, if you do go out and change all the call sites (there only
seem to be about 7, outside of gup.c, actually), that's going to conflict
a little bit with what I'm doing here.

So, how would you like proceed? If you want to do the full conversion
(which really should include the call sites), it would be easier for me
if you based it on my upcoming small patchset, which I expect to post
shortly (later today).

thanks,
-- 
John Hubbard
NVIDIA

> 
>>
>>> -int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
>>> -                       struct page **pages)
>>> +int __get_user_pages_fast(unsigned long start, int nr_pages,
>>> +                     unsigned int gup_flags, struct page **pages)
>>>   {
>>>        unsigned long len, end;
>>>        unsigned long flags;
>>> @@ -2685,10 +2692,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
>>>         * Internally (within mm/gup.c), gup fast variants must set FOLL_GET,
>>>         * because gup fast is always a "pin with a +1 page refcount" request.
>>>         */
>>> -     unsigned int gup_flags = FOLL_GET;
>>> -
>>> -     if (write)
>>> -             gup_flags |= FOLL_WRITE;
>>> +     gup_flags |= FOLL_GET;
> 

Re: [RFC] mm/gup.c: Use gup_flags as parameter instead of passing write flag

[Souptick Joarder]

On Tue, May 19, 2020 at 2:15 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 2020-05-18 13:44, Souptick Joarder wrote:
> > On Tue 19 May, 2020, 1:47 AM Matthew Wilcox, <willy@infradead.org> wrote:
> >>
> >> On Tue, May 19, 2020 at 01:28:23AM +0530, Souptick Joarder wrote:
> >>> The idea is to get rid of write parameter. Instead caller will pass
> >>> FOLL_WRITE to __get_user_pages_fast(). This will not change any
> >>> functionality of the API. Once it is upstream all the callers will
> >>> be changed to pass FOLL_WRITE.
> >>
> >> Uhh ... until you change all the callers, haven't you just broken all
> >> the callers?
> >
> > All the callers have called the API with either 1 or 0.  I think, it's
> > not going to break
> > any of the callers.
>
> Maybe so, but it's still "wrong" to do that, because it only works more
> or less accidentally. That is, it works in spite of a type safety
> violation. So we don't want to do that sort of thing unless there is
> a compelling reason.
>
> In addition to that, I am at the exact moment putting together a minor
> refactoring of this function, because I need a FOLL_PIN variant:
> __pin_user_pages_fast(), as part of my work to change over a few dozen
> gup call sites to pin_user_pages*().
>
> (In fact, I was wondering whether to stick with the "write" parameter, for
> the new __pin_user_pages_fast(), or go with gup_flags. That could go either
> way: gup_flags provides a nicer API, but "write" matches the existing
> callers.)
>
> So in other words, if you do go out and change all the call sites (there only
> seem to be about 7, outside of gup.c, actually), that's going to conflict
> a little bit with what I'm doing here.
>
> So, how would you like proceed? If you want to do the full conversion
> (which really should include the call sites), it would be easier for me
> if you based it on my upcoming small patchset, which I expect to post
> shortly (later today).

Sure, I will wait for your patchset :)

As there are only 7 callers of the __get_user_pages_fast(), I prefer to do
full conversion in a single commit. But if it is not preferred way, I would go
as per feedback.

> >>
> >>> -int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> >>> -                       struct page **pages)
> >>> +int __get_user_pages_fast(unsigned long start, int nr_pages,
> >>> +                     unsigned int gup_flags, struct page **pages)
> >>>   {
> >>>        unsigned long len, end;
> >>>        unsigned long flags;
> >>> @@ -2685,10 +2692,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> >>>         * Internally (within mm/gup.c), gup fast variants must set FOLL_GET,
> >>>         * because gup fast is always a "pin with a +1 page refcount" request.
> >>>         */
> >>> -     unsigned int gup_flags = FOLL_GET;
> >>> -
> >>> -     if (write)
> >>> -             gup_flags |= FOLL_WRITE;
> >>> +     gup_flags |= FOLL_GET;
> >
>

Re: [RFC] mm/gup.c: Use gup_flags as parameter instead of passing write flag

[Souptick Joarder]

On Tue, May 19, 2020 at 2:15 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 2020-05-18 13:44, Souptick Joarder wrote:
> > On Tue 19 May, 2020, 1:47 AM Matthew Wilcox, <willy@infradead.org> wrote:
> >>
> >> On Tue, May 19, 2020 at 01:28:23AM +0530, Souptick Joarder wrote:
> >>> The idea is to get rid of write parameter. Instead caller will pass
> >>> FOLL_WRITE to __get_user_pages_fast(). This will not change any
> >>> functionality of the API. Once it is upstream all the callers will
> >>> be changed to pass FOLL_WRITE.
> >>
> >> Uhh ... until you change all the callers, haven't you just broken all
> >> the callers?
> >
> > All the callers have called the API with either 1 or 0.  I think, it's
> > not going to break
> > any of the callers.
>
> Maybe so, but it's still "wrong" to do that, because it only works more
> or less accidentally. That is, it works in spite of a type safety
> violation. So we don't want to do that sort of thing unless there is
> a compelling reason.
>
> In addition to that, I am at the exact moment putting together a minor
> refactoring of this function, because I need a FOLL_PIN variant:
> __pin_user_pages_fast(), as part of my work to change over a few dozen
> gup call sites to pin_user_pages*().
>
> (In fact, I was wondering whether to stick with the "write" parameter, for
> the new __pin_user_pages_fast(), or go with gup_flags. That could go either
> way: gup_flags provides a nicer API, but "write" matches the existing
> callers.)
>
> So in other words, if you do go out and change all the call sites (there only
> seem to be about 7, outside of gup.c, actually), that's going to conflict
> a little bit with what I'm doing here.
>
> So, how would you like proceed? If you want to do the full conversion
> (which really should include the call sites), it would be easier for me
> if you based it on my upcoming small patchset, which I expect to post
> shortly (later today).

Sure, I will wait for your patchset :)

As there are only 7 callers of the __get_user_pages_fast(), I prefer to do
full conversion in a single commit. But if it is not preferred way, I would go
as per feedback.

> >>
> >>> -int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> >>> -                       struct page **pages)
> >>> +int __get_user_pages_fast(unsigned long start, int nr_pages,
> >>> +                     unsigned int gup_flags, struct page **pages)
> >>>   {
> >>>        unsigned long len, end;
> >>>        unsigned long flags;
> >>> @@ -2685,10 +2692,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> >>>         * Internally (within mm/gup.c), gup fast variants must set FOLL_GET,
> >>>         * because gup fast is always a "pin with a +1 page refcount" request.
> >>>         */
> >>> -     unsigned int gup_flags = FOLL_GET;
> >>> -
> >>> -     if (write)
> >>> -             gup_flags |= FOLL_WRITE;
> >>> +     gup_flags |= FOLL_GET;
> >
>

Re: [RFC] mm/gup.c: Use gup_flags as parameter instead of passing write flag

[John Hubbard]

On 2020-05-18 14:06, Souptick Joarder wrote:
...
>> So in other words, if you do go out and change all the call sites (there only
>> seem to be about 7, outside of gup.c, actually), that's going to conflict
>> a little bit with what I'm doing here.
>>
>> So, how would you like proceed? If you want to do the full conversion
>> (which really should include the call sites), it would be easier for me
>> if you based it on my upcoming small patchset, which I expect to post
>> shortly (later today).
> 
> Sure, I will wait for your patchset :)
> 
> As there are only 7 callers of the __get_user_pages_fast(), I prefer to do
> full conversion in a single commit. But if it is not preferred way, I would go
> as per feedback.
> 

Thanks, I appreciate it! And yes, you'll want to do the "write" to "gup_flags"
conversion in a single patch, definitely.


thanks,
-- 
John Hubbard
NVIDIA