Skip to contentSkip to conversation

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

RFC In review Souptick Joarder 3 participants 6 messages May 18, 2020

Subsystems (3)
Memory management MM core MM GUP
lore
Share & tools
lore

Souptick Joarder

author
#0 lore raw

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

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 <[email protected]>
---
 include/linux/mm.h |  4 ++--
 mm/gup.c           | 18 +++++++++++-------
 2 files changed, 13 insertions(+), 9 deletions(-)
include/linux/mm.h +2 -2
index 5a32342..15dd594 100644
@@ -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.
  */
mm/gup.c +11 -7
index 87a6a59..a8f869e 100644
@@ -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;
Signature · 1 line
1.9.1
#1 lore raw
Quoted 4 lines · 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.

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

Quoted 16 lines
mm/gup.c +3 -6
-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;
author
#2 lore raw
Quoted 9 lines · Matthew Wilcox,
Quoted 4 lines · 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.

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.

Quoted 17 lines
Quoted 17 lines
mm/gup.c +3 -6
-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;
reviewer
#3 lore raw
Quoted 8 lines · Souptick Joarder
Quoted 7 lines · Matthew Wilcox,
Quoted 4 lines · 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.

Quoted 6 lines
Quoted 2 lines

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,

Signature · 2 lines
John Hubbard
NVIDIA
Quoted 19 lines
Quoted 18 lines
Quoted 18 lines
mm/gup.c +3 -6
-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;
author
#4 lore raw
Quoted 32 lines · John Hubbard
Quoted 8 lines · Souptick Joarder
Quoted 7 lines · Matthew Wilcox,
Quoted 4 lines · 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.

Quoted 6 lines
Quoted 2 lines

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

Quoted 8 lines

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.

Quoted 19 lines
Quoted 18 lines
Quoted 17 lines
Quoted 17 lines
mm/gup.c +3 -6
-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;
reviewer
#5 lore raw

On 2020-05-18 14:06, Souptick Joarder wrote:
...

Quoted 9 lines
Quoted 8 lines

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

Quoted 6 lines

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,

Signature · 2 lines
John Hubbard
NVIDIA