mm/gup: introduce pin_user_pages_locked(), use it in frame_vector.c
Subsystems (3)
John Hubbard
reviewercover mm/gup: introduce pin_user_pages_locked(), use it in frame_vector.c
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/[email protected]
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
Signature · 1 line
2.26.2
John Hubbard
reviewerPATCH 1/2 mm/gup: introduce 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.
--- include/linux/mm.h | 2 ++ mm/gup.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)
include/linux/mm.h +2 -0
@@ -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,
mm/gup.c +30 -0
@@ -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); +
Signature · 1 line
2.26.2
Re: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()
Quoted 52 lines · John Hubbard
Introduce
pin_user_pages_locked(), which is nearly identical toget_user_pages_locked()except that it setsFOLL_PINand rejectsFOLL_GET.Signed-off-by John Hubbard <[email protected]>--- include/linux/mm.h | 2 ++ mm/gup.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)include/linux/mm.h +2
-0@@ -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,mm/gup.c +20
-0@@ -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. */
mm/gup.c long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, · line 3024
Quoted 10 lines
mm/gup.c +10
-0+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
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()
In reply to John HubbardQuoted 46 lines · John Hubbard
Introduce
pin_user_pages_locked(), which is nearly identical toget_user_pages_locked()except that it setsFOLL_PINand rejectsFOLL_GET.Signed-off-by John Hubbard <[email protected]>--- include/linux/mm.h | 2 ++ mm/gup.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)include/linux/mm.h +2
-0@@ -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,mm/gup.c +14
-0@@ -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
mm/gup.c long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, · line 3018
Quoted 10 lines
mm/gup.c +10
-0+* 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() ?
Quoted 10 lines
mm/gup.c +6
-1+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()
Quoted 6 lines · Souptick Joarder
Quoted 4 lines · John Hubbard
Introduce
pin_user_pages_locked(), which is nearly identical toget_user_pages_locked()except that it setsFOLL_PINand rejectsFOLL_GET.
Forget to ask, is it fine to add this new pin_user_pages_locked() in Documentation/core-api/pin_user_pages.rst ?
Quoted 68 lines
Quoted 42 lines
Signed-off-by John Hubbard <[email protected]>--- include/linux/mm.h | 2 ++ mm/gup.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)include/linux/mm.h +2
-0@@ -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,mm/gup.c +14
-0@@ -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 mm/gup.c long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, · line 3018
Quoted 10 lines
mm/gup.c +10
-0+* 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 forFOLL_PINand also not settingFOLL_GETinternally irrespective ofgup_flagspassed by user.
Do we need to add the same inget_user_pages_locked()?Quoted 10 lines
mm/gup.c +6
-1+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
John Hubbard
reviewerRe: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()
Quoted 11 lines · Souptick Joarder
Quoted 6 lines · Souptick Joarder
Quoted 4 lines · John Hubbard
Introduce
pin_user_pages_locked(), which is nearly identical toget_user_pages_locked()except that it setsFOLL_PINand rejectsFOLL_GET.Forget to ask, is it fine to add this new
pin_user_pages_locked()inDocumentation/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,
Signature · 2 lines
John Hubbard NVIDIA
John Hubbard
reviewerRe: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()
In reply to Souptick JoarderOn 2020-05-31 00:04, Souptick Joarder wrote:
...
Quoted 17 lines
Quoted 17 lines
mm/gup.c +17
-0+/* +* 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 ;
Quoted 8 lines
mm/gup.c long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, · line 3018
Quoted 6 lines
mm/gup.c +6
-0+ +/* 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 forFOLL_PIN
Yes, that should be added...
Quoted 3 lines
and also not setting
FOLL_GETinternally irrespective ofgup_flagspassed by user.
Do we need to add the same inget_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,
Signature · 2 lines
John Hubbard NVIDIA
Re: [PATCH 1/2] mm/gup: introduce pin_user_pages_locked()
In reply to John HubbardQuoted 66 lines · John Hubbard
Introduce
pin_user_pages_locked(), which is nearly identical toget_user_pages_locked()except that it setsFOLL_PINand rejectsFOLL_GET.Signed-off-by John Hubbard <[email protected]>--- include/linux/mm.h | 2 ++ mm/gup.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)include/linux/mm.h +2
-0@@ -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,mm/gup.c +30
-1@@ -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
John Hubbard
reviewerPATCH 2/2 mm/gup: frame_vector: convert get_user_pages() --> pin_user_pages()
In reply to John HubbardThis 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/
--- mm/frame_vector.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
mm/frame_vector.c +3 -4
@@ -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 ;
Signature · 1 line
2.26.2
Re: [PATCH 2/2] mm/gup: frame_vector: convert get_user_pages() --> pin_user_pages()
Quoted 42 lines · 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 <[email protected]>--- mm/frame_vector.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)mm/frame_vector.c +1
-2@@ -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)
mm/frame_vector.c void put_vaddr_frames(struct frame_vector *vec) · line 141
Quoted 10 lines
mm/frame_vector.c +2
-2@@ -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.
Signature · 3 lines
Thanks, David / dhildenb