Re: [linux-next RFC v2] mm/gup.c: Convert to use get_user_{page|pages}_fast_only()
On Mon, May 25, 2020 at 6:36 AM John Hubbard wrote: > > On 2020-05-23 21:27, Souptick Joarder wrote: > > API __get_user_pages_fast() renamed to get_user_pages_fast_only() > > to align with pin_user_pages_fast_only(). > > > > As part of this we will get rid of write parameter. Instead caller > > will pass FOLL_WRITE to get_user_pages_fast_only(). This will not > > change any existing functionality of the API. > > > > All the callers are changed to pass FOLL_WRITE. > > This looks good. A few nits below, but with those fixed, feel free to > add: > > Reviewed-by: John Hubbard > > > > > There are few places where 1 is passed to 2nd parameter of > > __get_user_pages_fast() and return value is checked for 1 > > like [1]. Those are replaced with new inline > > get_user_page_fast_only(). > > > > [1] if (__get_user_pages_fast(hva, 1, 1, ) == 1) > > > > We try to avoid talking *too* much about the previous version of > the code. Just enough. So, instead of the above two paragraphs, > I'd compress it down to: > > Also: introduce get_user_page_fast_only(), and use it in a few > places that hard-code nr_pages to 1. > > ... > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 93d93bd..8d4597f 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -1817,10 +1817,16 @@ 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_only(unsigned long start, int nr_pages, > > + unsigned int gup_flags, struct page **pages); > > Silly nit: > > Can you please leave the original indentation in place? I don't normally > comment about this, but I like the original indentation better, and it matches > the pin_user_pages_fast() below, too. > > ... > > @@ -2786,8 +2792,8 @@ static int internal_get_user_pages_fast(unsigned long > > start, int nr_pages, > >* 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_only(unsigned long start, int nr_pages, > > + unsigned int gup_flags, struct page **pages) > > > Same thing here: you've changed the original indentation, which was > (arguably, but > to my mind anyway) more readable, and for no reason. It still would have fit > within > 80 cols. > > I'm sure it's a perfect 50/50 mix of people who prefer either indentation > style, and > so for brand new code, I'll remain silent, as long as it is consistent with > either > itself and/or the surrounding code. But changing it back and forth is a bit > aggravating, and best avoided. :) Ok, along with these changes I will remove the *RFC* tag and repost it.
Re: [linux-next RFC v2] mm/gup.c: Convert to use get_user_{page|pages}_fast_only()
On 2020-05-23 21:27, Souptick Joarder wrote: API __get_user_pages_fast() renamed to get_user_pages_fast_only() to align with pin_user_pages_fast_only(). As part of this we will get rid of write parameter. Instead caller will pass FOLL_WRITE to get_user_pages_fast_only(). This will not change any existing functionality of the API. All the callers are changed to pass FOLL_WRITE. This looks good. A few nits below, but with those fixed, feel free to add: Reviewed-by: John Hubbard There are few places where 1 is passed to 2nd parameter of __get_user_pages_fast() and return value is checked for 1 like [1]. Those are replaced with new inline get_user_page_fast_only(). [1] if (__get_user_pages_fast(hva, 1, 1, ) == 1) We try to avoid talking *too* much about the previous version of the code. Just enough. So, instead of the above two paragraphs, I'd compress it down to: Also: introduce get_user_page_fast_only(), and use it in a few places that hard-code nr_pages to 1. ... diff --git a/include/linux/mm.h b/include/linux/mm.h index 93d93bd..8d4597f 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1817,10 +1817,16 @@ 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_only(unsigned long start, int nr_pages, + unsigned int gup_flags, struct page **pages); Silly nit: Can you please leave the original indentation in place? I don't normally comment about this, but I like the original indentation better, and it matches the pin_user_pages_fast() below, too. ... @@ -2786,8 +2792,8 @@ static int internal_get_user_pages_fast(unsigned long start, int nr_pages, * 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_only(unsigned long start, int nr_pages, + unsigned int gup_flags, struct page **pages) Same thing here: you've changed the original indentation, which was (arguably, but to my mind anyway) more readable, and for no reason. It still would have fit within 80 cols. I'm sure it's a perfect 50/50 mix of people who prefer either indentation style, and so for brand new code, I'll remain silent, as long as it is consistent with either itself and/or the surrounding code. But changing it back and forth is a bit aggravating, and best avoided. :) thanks, -- John Hubbard NVIDIA
[linux-next RFC v2] mm/gup.c: Convert to use get_user_{page|pages}_fast_only()
API __get_user_pages_fast() renamed to get_user_pages_fast_only()
to align with pin_user_pages_fast_only().
As part of this we will get rid of write parameter. Instead caller
will pass FOLL_WRITE to get_user_pages_fast_only(). This will not
change any existing functionality of the API.
All the callers are changed to pass FOLL_WRITE.
There are few places where 1 is passed to 2nd parameter of
__get_user_pages_fast() and return value is checked for 1
like [1]. Those are replaced with new inline
get_user_page_fast_only().
[1] if (__get_user_pages_fast(hva, 1, 1, ) == 1)
Updated the documentation of the API.
Signed-off-by: Souptick Joarder
Cc: John Hubbard
Cc: Matthew Wilcox
---
v2:
Updated the subject line and change log.
Address Matthew's comment to fix a bug and added
new inline get_user_page_fast_only().
arch/powerpc/kvm/book3s_64_mmu_hv.c| 2 +-
arch/powerpc/kvm/book3s_64_mmu_radix.c | 2 +-
arch/powerpc/perf/callchain_64.c | 4 +---
include/linux/mm.h | 10 --
kernel/events/core.c | 4 ++--
mm/gup.c | 29 -
virt/kvm/kvm_main.c| 8 +++-
7 files changed, 32 insertions(+), 27 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 18aed97..ddfc4c9 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -581,7 +581,7 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct
kvm_vcpu *vcpu,
* We always ask for write permission since the common case
* is that the page is writable.
*/
- if (__get_user_pages_fast(hva, 1, 1, ) == 1) {
+ if (get_user_page_fast_only(hva, FOLL_WRITE, )) {
write_ok = true;
} else {
/* Call KVM generic code to do the slow-path check */
diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c
b/arch/powerpc/kvm/book3s_64_mmu_radix.c
index 3248f78..5d4c087 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
@@ -795,7 +795,7 @@ int kvmppc_book3s_instantiate_page(struct kvm_vcpu *vcpu,
* is that the page is writable.
*/
hva = gfn_to_hva_memslot(memslot, gfn);
- if (!kvm_ro && __get_user_pages_fast(hva, 1, 1, ) == 1) {
+ if (!kvm_ro && get_user_page_fast_only(hva, FOLL_WRITE, )) {
upgrade_write = true;
} else {
unsigned long pfn;
diff --git a/arch/powerpc/perf/callchain_64.c b/arch/powerpc/perf/callchain_64.c
index 1bff896d..814d1c2 100644
--- a/arch/powerpc/perf/callchain_64.c
+++ b/arch/powerpc/perf/callchain_64.c
@@ -29,11 +29,9 @@ int read_user_stack_slow(void __user *ptr, void *buf, int nb)
unsigned long addr = (unsigned long) ptr;
unsigned long offset;
struct page *page;
- int nrpages;
void *kaddr;
- nrpages = __get_user_pages_fast(addr, 1, 1, );
- if (nrpages == 1) {
+ if (get_user_page_fast_only(addr, FOLL_WRITE, )) {
kaddr = page_address(page);
/* align address to page boundary */
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 93d93bd..8d4597f 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1817,10 +1817,16 @@ 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_only(unsigned long start, int nr_pages,
+ unsigned int gup_flags, struct page **pages);
int pin_user_pages_fast_only(unsigned long start, int nr_pages,
unsigned int gup_flags, struct page **pages);
+
+static inline bool get_user_page_fast_only(unsigned long addr,
+ unsigned int gup_flags, struct page **pagep)
+{
+ return get_user_pages_fast_only(addr, 1, gup_flags, pagep) == 1;
+}
/*
* per-process(per-mm_struct) statistics.
*/
diff --git a/kernel/events/core.c b/kernel/events/core.c
index c94eb27..856d98c 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6934,12 +6934,12 @@ static u64 perf_virt_to_phys(u64 virt)
* Walking the pages tables for user address.
* Interrupts are disabled, so it prevents any tear down
* of the page tables.
-* Try IRQ-safe __get_user_pages_fast first.
+* Try IRQ-safe get_user_page_fast_only first.
* If failed, leave phys_addr as 0.
*/
if (current->mm != NULL) {
pagefault_disable();
- if (__get_user_pages_fast(virt, 1, 0, ) == 1)
+ if (get_user_page_fast_only(virt, 0, ))
phys_addr =
