From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:07:14
Hi,
The cover letter is long, so the more important stuff is first:
* Jason, if you or someone could look at the the VFIO cleanup (patch 8)
and conversion to FOLL_PIN (patch 18), to make sure it's use of
remote and longterm gup matches what we discussed during the review
of v2, I'd appreciate it.
* Also for Jason and IB: as noted below, in patch 11, I am (too?) boldly
converting from put_user_pages() to release_pages().
* Jerome, I am going to take a look at doing your FOLL_GET change idea
(some callers should set FOLL_GET) separately, because it blew up "a
little bit" in my face. It's definitely a separate--tiny, but risky--project.
It also looks more reasonable when applied on top of this series here
(and it conflicts a lot), so I'm going to send it as a follow-up.
Changes since v2:
* Added a patch to convert IB/umem from normal gup, to gup_fast(). This
is also posted separately, in order to hopefully get some runtime
testing.
* Changed the page devmap code to be a little clearer,
thanks to Jerome for that.
* Split out the page devmap changes into a separate patch (and moved
Ira's Signed-off-by to that patch).
* Fixed my bug in IB: ODP code does not require pin_user_pages()
semantics. Therefore, revert the put_user_page() calls to put_page(),
and leave the get_user_pages() call as-is.
* As part of the revert, I am proposing here a change directly
from put_user_pages(), to release_pages(). I'd feel better if
someone agrees that this is the best way. It uses the more
efficient release_pages(), instead of put_page() in a loop,
and keep the change to just a few character on one line,
but OTOH it is not a pure revert.
* Loosened the FOLL_LONGTERM restrictions in the
__get_user_pages_locked() implementation, and used that in order
to fix up a VFIO bug. Thanks to Jason for that idea.
* Note the use of release_pages() in IB: is that OK?
* Added a few more WARN's and clarifying comments nearby.
* Many documentation improvements in various comments.
* Moved the new pin_user_pages.rst from Documentation/vm/ to
Documentation/core-api/ .
* Commit descriptions: added clarifying notes to the three patches
(drm/via, fs/io_uring, net/xdp) that already had put_user_page()
calls in place.
* Collected all pending Reviewed-by and Acked-by tags, from v1 and v2
email threads.
* Lot of churn from v2 --> v3, so it's possible that new bugs
sneaked in.
NOT DONE: separate patchset is required:
* __get_user_pages_locked(): stop compensating for
buggy callers who failed to set FOLL_GET. Instead, assert
that FOLL_GET is set (and fail if it's not).
======================================================================
Original cover letter (edited to fix up the patch description numbers)
This applies cleanly to linux-next and mmotm, and also to linux.git if
linux-next's commit 20cac10710c9 ("mm/gup_benchmark: fix MAP_HUGETLB
case") is first applied there.
This provides tracking of dma-pinned pages. This is a prerequisite to
solving the larger problem of proper interactions between file-backed
pages, and [R]DMA activities, as discussed in [1], [2], [3], and in
a remarkable number of email threads since about 2017. :)
A new internal gup flag, FOLL_PIN is introduced, and thoroughly
documented in the last patch's Documentation/vm/pin_user_pages.rst.
I believe that this will provide a good starting point for doing the
layout lease work that Ira Weiny has been working on. That's because
these new wrapper functions provide a clean, constrained, systematically
named set of functionality that, again, is required in order to even
know if a page is "dma-pinned".
In contrast to earlier approaches, the page tracking can be
incrementally applied to the kernel call sites that, until now, have
been simply calling get_user_pages() ("gup"). In other words, opt-in by
changing from this:
get_user_pages() (sets FOLL_GET)
put_page()
to this:
pin_user_pages() (sets FOLL_PIN)
put_user_page()
Because there are interdependencies with FOLL_LONGTERM, a similar
conversion as for FOLL_PIN, was applied. The change was from this:
get_user_pages(FOLL_LONGTERM) (also sets FOLL_GET)
put_page()
to this:
pin_longterm_pages() (sets FOLL_PIN | FOLL_LONGTERM)
put_user_page()
============================================================
Patch summary:
* Patches 1-8: refactoring and preparatory cleanup, independent fixes
* Patch 9: introduce pin_user_pages(), FOLL_PIN, but no functional
changes yet
* Patches 10-15: Convert existing put_user_page() callers, to use the
new pin*()
* Patch 16: Activate tracking of FOLL_PIN pages.
* Patches 17-19: convert FOLL_LONGTERM callers
* Patches: 20-22: gup_benchmark and run_vmtests support
* Patch 23: enforce FOLL_LONGTERM as a gup-internal (only) flag
============================================================
Testing:
* I've done some overall kernel testing (LTP, and a few other goodies),
and some directed testing to exercise some of the changes. And as you
can see, gup_benchmark is enhanced to exercise this. Basically, I've been
able to runtime test the core get_user_pages() and pin_user_pages() and
related routines, but not so much on several of the call sites--but those
are generally just a couple of lines changed, each.
Not much of the kernel is actually using this, which on one hand
reduces risk quite a lot. But on the other hand, testing coverage
is low. So I'd love it if, in particular, the Infiniband and PowerPC
folks could do a smoke test of this series for me.
Also, my runtime testing for the call sites so far is very weak:
* io_uring: Some directed tests from liburing exercise this, and they pass.
* process_vm_access.c: A small directed test passes.
* gup_benchmark: the enhanced version hits the new gup.c code, and passes.
* infiniband (still only have crude "IB pingpong" working, on a
good day: it's not exercising my conversions at runtime...)
* VFIO: compiles (I'm vowing to set up a run time test soon, but it's
not ready just yet)
* powerpc: it compiles...
* drm/via: compiles...
* goldfish: compiles...
* net/xdp: compiles...
* media/v4l2: compiles...
============================================================
Next:
* Get the block/bio_vec sites converted to use pin_user_pages().
* Work with Ira and Dave Chinner to weave this together with the
layout lease stuff.
============================================================
[1] Some slow progress on get_user_pages() (Apr 2, 2019): https://lwn.net/Articles/784574/
[2] DMA and get_user_pages() (LPC: Dec 12, 2018): https://lwn.net/Articles/774411/
[3] The trouble with get_user_pages() (Apr 30, 2018): https://lwn.net/Articles/753027/
John Hubbard (23):
mm/gup: pass flags arg to __gup_device_* functions
mm/gup: factor out duplicate code from four routines
mm/gup: move try_get_compound_head() to top, fix minor issues
mm: devmap: refactor 1-based refcounting for ZONE_DEVICE pages
goldish_pipe: rename local pin_user_pages() routine
IB/umem: use get_user_pages_fast() to pin DMA pages
media/v4l2-core: set pages dirty upon releasing DMA buffers
vfio, mm: fix get_user_pages_remote() and FOLL_LONGTERM
mm/gup: introduce pin_user_pages*() and FOLL_PIN
goldish_pipe: convert to pin_user_pages() and put_user_page()
IB/{core,hw,umem}: set FOLL_PIN, FOLL_LONGTERM via
pin_longterm_pages*()
mm/process_vm_access: set FOLL_PIN via pin_user_pages_remote()
drm/via: set FOLL_PIN via pin_user_pages_fast()
fs/io_uring: set FOLL_PIN via pin_user_pages()
net/xdp: set FOLL_PIN via pin_user_pages()
mm/gup: track FOLL_PIN pages
media/v4l2-core: pin_longterm_pages (FOLL_PIN) and put_user_page()
conversion
vfio, mm: pin_longterm_pages (FOLL_PIN) and put_user_page() conversion
powerpc: book3s64: convert to pin_longterm_pages() and put_user_page()
mm/gup_benchmark: use proper FOLL_WRITE flags instead of hard-coding
"1"
mm/gup_benchmark: support pin_user_pages() and related calls
selftests/vm: run_vmtests: invoke gup_benchmark with basic FOLL_PIN
coverage
mm/gup: remove support for gup(FOLL_LONGTERM)
Documentation/core-api/index.rst | 1 +
Documentation/core-api/pin_user_pages.rst | 218 +++++++
arch/powerpc/mm/book3s64/iommu_api.c | 15 +-
drivers/gpu/drm/via/via_dmablit.c | 2 +-
drivers/infiniband/core/umem.c | 17 +-
drivers/infiniband/core/umem_odp.c | 24 +-
drivers/infiniband/hw/hfi1/user_pages.c | 4 +-
drivers/infiniband/hw/mthca/mthca_memfree.c | 3 +-
drivers/infiniband/hw/qib/qib_user_pages.c | 8 +-
drivers/infiniband/hw/qib/qib_user_sdma.c | 2 +-
drivers/infiniband/hw/usnic/usnic_uiom.c | 9 +-
drivers/infiniband/sw/siw/siw_mem.c | 5 +-
drivers/media/v4l2-core/videobuf-dma-sg.c | 10 +-
drivers/platform/goldfish/goldfish_pipe.c | 35 +-
drivers/vfio/vfio_iommu_type1.c | 35 +-
fs/io_uring.c | 5 +-
include/linux/mm.h | 164 +++++-
include/linux/mmzone.h | 2 +
include/linux/page_ref.h | 10 +
mm/gup.c | 608 ++++++++++++++++----
mm/gup_benchmark.c | 87 ++-
mm/huge_memory.c | 54 +-
mm/hugetlb.c | 39 +-
mm/memremap.c | 67 +--
mm/process_vm_access.c | 28 +-
mm/vmstat.c | 2 +
net/xdp/xdp_umem.c | 4 +-
tools/testing/selftests/vm/gup_benchmark.c | 34 +-
tools/testing/selftests/vm/run_vmtests | 22 +
29 files changed, 1180 insertions(+), 334 deletions(-)
create mode 100644 Documentation/core-api/pin_user_pages.rst
--
2.24.0
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:07:33
Up until now, gup_benchmark supported testing of the
following kernel functions:
* get_user_pages(): via the '-U' command line option
* get_user_pages_longterm(): via the '-L' command line option
* get_user_pages_fast(): as the default (no options required)
Add test coverage for the new corresponding pin_*() functions:
* pin_user_pages(): via the '-c' command line option
* pin_longterm_pages(): via the '-b' command line option
* pin_user_pages_fast(): via the '-a' command line option
Also, add an option for clarity: '-u' for what is now (still) the
default choice: get_user_pages_fast().
Also, for the three commands that set FOLL_PIN, verify that the pages
really are dma-pinned, via the new is_dma_pinned() routine.
Those commands are:
PIN_FAST_BENCHMARK : calls pin_user_pages_fast()
PIN_LONGTERM_BENCHMARK : calls pin_longterm_pages()
PIN_BENCHMARK : calls pin_user_pages()
In between the calls to pin_*() and put_user_pages(),
check each page: if page_dma_pinned() returns false, then
WARN and return.
Do this outside of the benchmark timestamps, so that it doesn't
affect reported times.
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
mm/gup_benchmark.c | 73 ++++++++++++++++++++--
tools/testing/selftests/vm/gup_benchmark.c | 23 ++++++-
2 files changed, 90 insertions(+), 6 deletions(-)
@@ -19,6 +22,44 @@ struct gup_benchmark {__u64expansion[10];/* For future use */};+staticvoidput_back_pages(intcmd,structpage**pages,unsignedlongnr_pages)+{+inti;++switch(cmd){+caseGUP_FAST_BENCHMARK:+caseGUP_LONGTERM_BENCHMARK:+caseGUP_BENCHMARK:+for(i=0;i<nr_pages;i++)+put_page(pages[i]);+break;++casePIN_FAST_BENCHMARK:+casePIN_LONGTERM_BENCHMARK:+casePIN_BENCHMARK:+put_user_pages(pages,nr_pages);+break;+}+}++staticvoidverify_dma_pinned(intcmd,structpage**pages,+unsignedlongnr_pages)+{+inti;++switch(cmd){+casePIN_FAST_BENCHMARK:+casePIN_LONGTERM_BENCHMARK:+casePIN_BENCHMARK:+for(i=0;i<nr_pages;i++){+if(WARN(!page_dma_pinned(pages[i]),+"pages[%d] is NOT dma-pinned\n",i))+break;+}+break;+}+}+staticint__gup_benchmark_ioctl(unsignedintcmd,structgup_benchmark*gup){
@@ -65,6 +106,18 @@ static int __gup_benchmark_ioctl(unsigned int cmd,nr=get_user_pages(addr,nr,gup->flags,pages+i,NULL);break;+casePIN_FAST_BENCHMARK:+nr=pin_user_pages_fast(addr,nr,gup->flags,+pages+i);+break;+casePIN_LONGTERM_BENCHMARK:+nr=pin_longterm_pages(addr,nr,gup->flags,pages+i,+NULL);+break;+casePIN_BENCHMARK:+nr=pin_user_pages(addr,nr,gup->flags,pages+i,+NULL);+break;default:return-1;}
@@ -75,15 +128,22 @@ static int __gup_benchmark_ioctl(unsigned int cmd,}end_time=ktime_get();+/* Shifting the meaning of nr_pages: now it is actual number pinned: */+nr_pages=i;+gup->get_delta_usec=ktime_us_delta(end_time,start_time);gup->size=addr-gup->addr;+/*+*Takeanun-benchmark-timedmomenttoverifyDMApinned+*state:printawarningifanynon-dma-pinnedpagesarefound:+*/+verify_dma_pinned(cmd,pages,nr_pages);+start_time=ktime_get();-for(i=0;i<nr_pages;i++){-if(!pages[i])-break;-put_page(pages[i]);-}++put_back_pages(cmd,pages,nr_pages);+end_time=ktime_get();gup->put_delta_usec=ktime_us_delta(end_time,start_time);
@@ -101,6 +161,9 @@ static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,caseGUP_FAST_BENCHMARK:caseGUP_LONGTERM_BENCHMARK:caseGUP_BENCHMARK:+casePIN_FAST_BENCHMARK:+casePIN_LONGTERM_BENCHMARK:+casePIN_BENCHMARK:break;default:return-EINVAL;
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:07:42
1. Convert from get_user_pages(FOLL_LONGTERM) to pin_longterm_pages().
2. As required by pin_user_pages(), release these pages via
put_user_page(). In this case, do so via put_user_pages_dirty_lock().
That has the side effect of calling set_page_dirty_lock(), instead
of set_page_dirty(). This is probably more accurate.
As Christoph Hellwig put it, "set_page_dirty() is only safe if we are
dealing with a file backed page where we have reference on the inode it
hangs off." [1]
3. Release each page in mem->hpages[] (instead of mem->hpas[]), because
that is the array that pin_longterm_pages() filled in. This is more
accurate and should be a little safer from a maintenance point of
view.
[1] https://lore.kernel.org/r/20190723153640.GB720@lst.de
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
arch/powerpc/mm/book3s64/iommu_api.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
@@ -103,9 +103,8 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua,for(entry=0;entry<entries;entry+=chunk){unsignedlongn=min(entries-entry,chunk);-ret=get_user_pages(ua+(entry<<PAGE_SHIFT),n,-FOLL_WRITE|FOLL_LONGTERM,-mem->hpages+entry,NULL);+ret=pin_longterm_pages(ua+(entry<<PAGE_SHIFT),n,+FOLL_WRITE,mem->hpages+entry,NULL);if(ret==n){pinned+=n;continue;
@@ -167,9 +166,8 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua,return0;free_exit:-/* free the reference taken */-for(i=0;i<pinned;i++)-put_page(mem->hpages[i]);+/* free the references taken */+put_user_pages(mem->hpages,pinned);vfree(mem->hpas);kfree(mem);
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:07:58
It's good to have basic unit test coverage of the new FOLL_PIN
behavior. Fortunately, the gup_benchmark unit test is extremely
fast (a few milliseconds), so adding it the the run_vmtests suite
is going to cause no noticeable change in running time.
So, add two new invocations to run_vmtests:
1) Run gup_benchmark with normal get_user_pages().
2) Run gup_benchmark with pin_user_pages(). This is much like
the first call, except that it sets FOLL_PIN.
Running these two in quick succession also provide a visual
comparison of the running times, which is convenient.
The new invocations are fairly early in the run_vmtests script,
because with test suites, it's usually preferable to put the
shorter, faster tests first, all other things being equal.
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
tools/testing/selftests/vm/run_vmtests | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:08:19
Now that all other kernel callers of get_user_pages(FOLL_LONGTERM)
have been converted to pin_longterm_pages(), lock it down:
1) Add an assertion to get_user_pages(), preventing callers from
passing FOLL_LONGTERM (in addition to the existing assertion that
prevents FOLL_PIN).
2) Remove the associated GUP_LONGTERM_BENCHMARK test.
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
mm/gup.c | 8 ++++----
mm/gup_benchmark.c | 9 +--------
tools/testing/selftests/vm/gup_benchmark.c | 7 ++-----
3 files changed, 7 insertions(+), 17 deletions(-)
@@ -1743,11 +1743,11 @@ long get_user_pages(unsigned long start, unsigned long nr_pages,structvm_area_struct**vmas){/*-*FOLL_PINmustonlybesetinternallybythepin_user_page*()and-*pin_longterm_*()APIs,neverdirectlybythecaller,soenforcethat-*withanassertion:+*FOLL_PINandFOLL_LONGTERMmustonlybesetinternallybythe+*pin_user_page*()andpin_longterm_*()APIs,neverdirectlybythe+*caller,soenforcethatwithanassertion:*/-if(WARN_ON_ONCE(gup_flags&FOLL_PIN))+if(WARN_ON_ONCE(gup_flags&(FOLL_PIN|FOLL_LONGTERM)))return-EINVAL;return__gup_longterm_locked(current,current->mm,start,nr_pages,
@@ -97,11 +96,6 @@ static int __gup_benchmark_ioctl(unsigned int cmd,nr=get_user_pages_fast(addr,nr,gup->flags,pages+i);break;-caseGUP_LONGTERM_BENCHMARK:-nr=get_user_pages(addr,nr,-gup->flags|FOLL_LONGTERM,-pages+i,NULL);-break;caseGUP_BENCHMARK:nr=get_user_pages(addr,nr,gup->flags,pages+i,NULL);
@@ -159,7 +153,6 @@ static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,switch(cmd){caseGUP_FAST_BENCHMARK:-caseGUP_LONGTERM_BENCHMARK:caseGUP_BENCHMARK:casePIN_FAST_BENCHMARK:casePIN_LONGTERM_BENCHMARK:
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:08:31
Fix the gup benchmark flags to use the symbolic FOLL_WRITE,
instead of a hard-coded "1" value.
Also, clean up the filtering of gup flags a little, by just doing
it once before issuing any of the get_user_pages*() calls. This
makes it harder to overlook, instead of having little "gup_flags & 1"
phrases in the function calls.
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
mm/gup_benchmark.c | 9 ++++++---
tools/testing/selftests/vm/gup_benchmark.c | 6 +++++-
2 files changed, 11 insertions(+), 4 deletions(-)
@@ -48,18 +48,21 @@ static int __gup_benchmark_ioctl(unsigned int cmd,nr=(next-addr)/PAGE_SIZE;}+/* Filter out most gup flags: only allow a tiny subset here: */+gup->flags&=FOLL_WRITE;+switch(cmd){caseGUP_FAST_BENCHMARK:-nr=get_user_pages_fast(addr,nr,gup->flags&1,+nr=get_user_pages_fast(addr,nr,gup->flags,pages+i);break;caseGUP_LONGTERM_BENCHMARK:nr=get_user_pages(addr,nr,-(gup->flags&1)|FOLL_LONGTERM,+gup->flags|FOLL_LONGTERM,pages+i,NULL);break;caseGUP_BENCHMARK:-nr=get_user_pages(addr,nr,gup->flags&1,pages+i,+nr=get_user_pages(addr,nr,gup->flags,pages+i,NULL);break;default:
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:08:45
1. Change vfio from get_user_pages(FOLL_LONGTERM), to
pin_longterm_pages(), which sets both FOLL_LONGTERM and FOLL_PIN.
2. Because all FOLL_PIN-acquired pages must be released via
put_user_page(), also convert the put_page() call over to
put_user_pages().
Note that this effectively changes the code's behavior in
vfio_iommu_type1.c: put_pfn(): it now ultimately calls
set_page_dirty_lock(), instead of set_page_dirty(). This is
probably more accurate.
As Christoph Hellwig put it, "set_page_dirty() is only safe if we are
dealing with a file backed page where we have reference on the inode it
hangs off." [1]
[1] https://lore.kernel.org/r/20190723153640.GB720@lst.de
Cc: Alex Williamson <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
@@ -327,9 +327,8 @@ static int put_pfn(unsigned long pfn, int prot){if(!is_invalid_reserved_pfn(pfn)){structpage*page=pfn_to_page(pfn);-if(prot&IOMMU_WRITE)-SetPageDirty(page);-put_page(page);++put_user_pages_dirty_lock(&page,1,prot&IOMMU_WRITE);return1;}return0;
@@ -348,8 +347,8 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,flags|=FOLL_WRITE;down_read(&mm->mmap_sem);-ret=get_user_pages_remote(NULL,mm,vaddr,1,flags|FOLL_LONGTERM,-page,vmas,NULL);+ret=pin_longterm_pages_remote(NULL,mm,vaddr,1,flags,page,vmas,+NULL);/**Thelifetimeofavaddr_get_pfn()pagepinis*userspace-controlled.Inthefs-daxcasethiscould
@@ -359,7 +358,7 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,*/if(ret>0&&vma_is_fsdax(vmas[0])){ret=-EOPNOTSUPP;-put_page(page[0]);+put_user_page(page[0]);}up_read(&mm->mmap_sem);
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:09:00
1. Change v4l2 from get_user_pages(FOLL_LONGTERM), to
pin_longterm_pages(), which sets both FOLL_LONGTERM and FOLL_PIN.
2. Because all FOLL_PIN-acquired pages must be released via
put_user_page(), also convert the put_page() call over to
put_user_pages_dirty_lock().
Acked-by: Hans Verkuil <redacted>
Reviewed-by: Ira Weiny <redacted>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/media/v4l2-core/videobuf-dma-sg.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:09:02
Convert infiniband to use the new wrapper calls, and stop
explicitly setting FOLL_LONGTERM at the call sites.
The new pin_longterm_*() calls replace get_user_pages*()
calls, and set both FOLL_LONGTERM and a new FOLL_PIN
flag. The FOLL_PIN flag requires that the caller must
return the pages via put_user_page*() calls, but
infiniband was already doing that as part of an earlier
commit.
Reviewed-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/infiniband/core/umem.c | 10 ++++-----
drivers/infiniband/core/umem_odp.c | 24 ++++++++++-----------
drivers/infiniband/hw/hfi1/user_pages.c | 4 ++--
drivers/infiniband/hw/mthca/mthca_memfree.c | 3 +--
drivers/infiniband/hw/qib/qib_user_pages.c | 8 +++----
drivers/infiniband/hw/qib/qib_user_sdma.c | 2 +-
drivers/infiniband/hw/usnic/usnic_uiom.c | 9 ++++----
drivers/infiniband/sw/siw/siw_mem.c | 5 ++---
8 files changed, 31 insertions(+), 34 deletions(-)
@@ -108,10 +108,10 @@ int qib_get_user_pages(unsigned long start_page, size_t num_pages,down_read(¤t->mm->mmap_sem);for(got=0;got<num_pages;got+=ret){-ret=get_user_pages(start_page+got*PAGE_SIZE,-num_pages-got,-FOLL_LONGTERM|FOLL_WRITE|FOLL_FORCE,-p+got,NULL);+ret=pin_longterm_pages(start_page+got*PAGE_SIZE,+num_pages-got,+FOLL_WRITE|FOLL_FORCE,+p+got,NULL);if(ret<0){up_read(¤t->mm->mmap_sem);gotobail_release;
@@ -141,11 +141,10 @@ static int usnic_uiom_get_pages(unsigned long addr, size_t size, int writable,ret=0;while(npages){-ret=get_user_pages(cur_base,-min_t(unsignedlong,npages,-PAGE_SIZE/sizeof(structpage*)),-gup_flags|FOLL_LONGTERM,-page_list,NULL);+ret=pin_longterm_pages(cur_base,+min_t(unsignedlong,npages,+PAGE_SIZE/sizeof(structpage*)),+gup_flags,page_list,NULL);if(ret<0)gotoout;
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:09:04
Convert fs/io_uring to use the new pin_user_pages() call, which sets
FOLL_PIN. Setting FOLL_PIN is now required for code that requires
tracking of pinned pages, and therefore for any code that calls
put_user_page().
In partial anticipation of this work, the io_uring code was already
calling put_user_page() instead of put_page(). Therefore, in order to
convert from the get_user_pages()/put_page() model, to the
pin_user_pages()/put_user_page() model, the only change required
here is to change get_user_pages() to pin_longterm_pages().
Reviewed-by: Ira Weiny <redacted>
Reviewed-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
fs/io_uring.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:09:21
Convert net/xdp to use the new pin_longterm_pages() call, which sets
FOLL_PIN. Setting FOLL_PIN is now required for code that requires
tracking of pinned pages.
In partial anticipation of this work, the net/xdp code was already
calling put_user_page() instead of put_page(). Therefore, in order to
convert from the get_user_pages()/put_page() model, to the
pin_user_pages()/put_user_page() model, the only change required
here is to change get_user_pages() to pin_longterm_pages().
Reviewed-by: Ira Weiny <redacted>
Acked-by: Björn Töpel <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
net/xdp/xdp_umem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:09:24
Add tracking of pages that were pinned via FOLL_PIN.
As mentioned in the FOLL_PIN documentation, callers who effectively set
FOLL_PIN are required to ultimately free such pages via put_user_page().
The effect is similar to FOLL_GET, and may be thought of as "FOLL_GET
for DIO and/or RDMA use".
Pages that have been pinned via FOLL_PIN are identifiable via a
new function call:
bool page_dma_pinned(struct page *page);
What to do in response to encountering such a page, is left to later
patchsets. There is discussion about this in [1].
This also changes a BUG_ON(), to a WARN_ON(), in follow_page_mask().
Suggested-by: Jan Kara <jack@suse.cz>
Suggested-by: Jérôme Glisse <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
include/linux/mm.h | 75 ++++++++++++----
include/linux/mmzone.h | 2 +
include/linux/page_ref.h | 10 +++
mm/gup.c | 190 +++++++++++++++++++++++++++++++++------
mm/huge_memory.c | 54 ++++++++++-
mm/hugetlb.c | 39 +++++++-
mm/vmstat.c | 2 +
7 files changed, 322 insertions(+), 50 deletions(-)
@@ -1869,7 +1968,7 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,pgmap=get_dev_pagemap(pte_pfn(pte),pgmap);if(unlikely(!pgmap)){-undo_dev_pagemap(nr,nr_start,pages);+undo_dev_pagemap(nr,nr_start,flags,pages);gotopte_unmap;}}elseif(pte_special(pte))
@@ -1878,9 +1977,15 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,VM_BUG_ON(!pfn_valid(pte_pfn(pte)));page=pte_page(pte);-head=try_get_compound_head(page,1);-if(!head)-gotopte_unmap;+if(flags&FOLL_PIN){+head=page;+if(unlikely(!user_page_ref_inc(head)))+gotopte_unmap;+}else{+head=try_get_compound_head(page,1);+if(!head)+gotopte_unmap;+}if(unlikely(pte_val(pte)!=pte_val(*ptep))){put_page(head);
@@ -1934,12 +2039,20 @@ static int __gup_device_huge(unsigned long pfn, unsigned long addr,pgmap=get_dev_pagemap(pfn,pgmap);if(unlikely(!pgmap)){-undo_dev_pagemap(nr,nr_start,pages);+undo_dev_pagemap(nr,nr_start,flags,pages);return0;}SetPageReferenced(page);pages[*nr]=page;-get_page(page);++if(flags&FOLL_PIN){+if(unlikely(!user_page_ref_inc(page))){+undo_dev_pagemap(nr,nr_start,flags,pages);+return0;+}+}else+get_page(page);+(*nr)++;pfn++;}while(addr+=PAGE_SIZE,addr!=end);
@@ -1961,7 +2074,7 @@ static int __gup_device_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,return0;if(unlikely(pmd_val(orig)!=pmd_val(*pmdp))){-undo_dev_pagemap(nr,nr_start,pages);+undo_dev_pagemap(nr,nr_start,flags,pages);return0;}return1;
@@ -1979,7 +2092,7 @@ static int __gup_device_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,return0;if(unlikely(pud_val(orig)!=pud_val(*pudp))){-undo_dev_pagemap(nr,nr_start,pages);+undo_dev_pagemap(nr,nr_start,flags,pages);return0;}return1;
@@ -2063,9 +2176,16 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,page=head+((addr&(sz-1))>>PAGE_SHIFT);refs=__record_subpages(page,addr,end,pages,*nr);-head=try_get_compound_head(head,refs);-if(!head)-return0;+if(flags&FOLL_PIN){+head=page;+if(unlikely(!user_page_ref_inc(head)))+return0;+head=page;+}else{+head=try_get_compound_head(head,refs);+if(!head)+return0;+}if(unlikely(pte_val(pte)!=pte_val(*ptep))){put_compound_head(head,refs);
@@ -2122,9 +2242,15 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,page=pmd_page(orig)+((addr&~PMD_MASK)>>PAGE_SHIFT);refs=__record_subpages(page,addr,end,pages,*nr);-head=try_get_compound_head(pmd_page(orig),refs);-if(!head)-return0;+if(flags&FOLL_PIN){+head=page;+if(unlikely(!user_page_ref_inc(head)))+return0;+}else{+head=try_get_compound_head(pmd_page(orig),refs);+if(!head)+return0;+}if(unlikely(pmd_val(orig)!=pmd_val(*pmdp))){put_compound_head(head,refs);
@@ -2155,9 +2281,15 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,page=pud_page(orig)+((addr&~PUD_MASK)>>PAGE_SHIFT);refs=__record_subpages(page,addr,end,pages,*nr);-head=try_get_compound_head(pud_page(orig),refs);-if(!head)-return0;+if(flags&FOLL_PIN){+head=page;+if(unlikely(!user_page_ref_inc(head)))+return0;+}else{+head=try_get_compound_head(pud_page(orig),refs);+if(!head)+return0;+}if(unlikely(pud_val(orig)!=pud_val(*pudp))){put_compound_head(head,refs);
@@ -2183,9 +2315,15 @@ static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr,page=pgd_page(orig)+((addr&~PGDIR_MASK)>>PAGE_SHIFT);refs=__record_subpages(page,addr,end,pages,*nr);-head=try_get_compound_head(pgd_page(orig),refs);-if(!head)-return0;+if(flags&FOLL_PIN){+head=page;+if(unlikely(!user_page_ref_inc(head)))+return0;+}else{+head=try_get_compound_head(pgd_page(orig),refs);+if(!head)+return0;+}if(unlikely(pgd_val(orig)!=pgd_val(*pgdp))){put_compound_head(head,refs);
@@ -945,6 +945,11 @@ struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long addr,*/WARN_ONCE(flags&FOLL_COW,"mm: In follow_devmap_pmd with FOLL_COW set");+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE((flags&(FOLL_PIN|FOLL_GET))==+(FOLL_PIN|FOLL_GET)))+returnNULL;+if(flags&FOLL_WRITE&&!pmd_write(*pmd))returnNULL;
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:09:36
Introduce pin_user_pages*() variations of get_user_pages*() calls,
and also pin_longterm_pages*() variations.
These variants all set FOLL_PIN, which is also introduced, and
thoroughly documented.
The pin_longterm*() variants also set FOLL_LONGTERM, in addition
to FOLL_PIN:
pin_user_pages()
pin_user_pages_remote()
pin_user_pages_fast()
pin_longterm_pages()
pin_longterm_pages_remote()
pin_longterm_pages_fast()
All pages that are pinned via the above calls, must be unpinned via
put_user_page().
The underlying rules are:
* These are gup-internal flags, so the call sites should not directly
set FOLL_PIN nor FOLL_LONGTERM. That behavior is enforced with
assertions, for the new FOLL_PIN flag. However, for the pre-existing
FOLL_LONGTERM flag, which has some call sites that still directly
set FOLL_LONGTERM, there is no assertion yet.
* Call sites that want to indicate that they are going to do DirectIO
("DIO") or something with similar characteristics, should call a
get_user_pages()-like wrapper call that sets FOLL_PIN. These wrappers
will:
* Start with "pin_user_pages" instead of "get_user_pages". That
makes it easy to find and audit the call sites.
* Set FOLL_PIN
* For pages that are received via FOLL_PIN, those pages must be returned
via put_user_page().
Thanks to Jan Kara and Vlastimil Babka for explaining the 4 cases
in this documentation. (I've reworded it and expanded upon it.)
Reviewed-by: Jérôme Glisse <redacted>
Cc: Mike Rapoport <rppt@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
Documentation/core-api/index.rst | 1 +
Documentation/core-api/pin_user_pages.rst | 218 ++++++++++++++++++
include/linux/mm.h | 62 +++++-
mm/gup.c | 260 ++++++++++++++++++++--
4 files changed, 514 insertions(+), 27 deletions(-)
create mode 100644 Documentation/core-api/pin_user_pages.rst
@@ -0,0 +1,218 @@+.. SPDX-License-Identifier: GPL-2.0++====================================================+pin_user_pages() and related calls+====================================================++..contents:: :local:++Overview+========++This document describes the following functions: ::++ pin_user_pages+ pin_user_pages_fast+ pin_user_pages_remote++ pin_longterm_pages+ pin_longterm_pages_fast+ pin_longterm_pages_remote++Basic description of FOLL_PIN+=============================++FOLL_PIN and FOLL_LONGTERM are flags that can be passed to the get_user_pages*()+("gup") family of functions. FOLL_PIN has significant interactions and+interdependencies with FOLL_LONGTERM, so both are covered here.++Both FOLL_PIN and FOLL_LONGTERM are internal to gup, meaning that neither+FOLL_PIN nor FOLL_LONGTERM should not appear at the gup call sites. This allows+the associated wrapper functions (pin_user_pages() and others) to set the+correct combination of these flags, and to check for problems as well.++FOLL_PIN and FOLL_GET are mutually exclusive for a given gup call. However,+multiple threads and call sites are free to pin the same struct pages, via both+FOLL_PIN and FOLL_GET. It's just the call site that needs to choose one or the+other, not the struct page(s).++The FOLL_PIN implementation is nearly the same as FOLL_GET, except that FOLL_PIN+uses a different reference counting technique.++FOLL_PIN is a prerequisite to FOLL_LONGTGERM. Another way of saying that is,+FOLL_LONGTERM is a specific case, more restrictive case of FOLL_PIN.++Which flags are set by each wrapper+===================================++Only FOLL_PIN and FOLL_LONGTERM are covered here. These flags are added to+whatever flags the caller provides::++ Function gup flags (FOLL_PIN or FOLL_LONGTERM only)+ -------- ------------------------------------------+ pin_user_pages FOLL_PIN+ pin_user_pages_fast FOLL_PIN+ pin_user_pages_remote FOLL_PIN++ pin_longterm_pages FOLL_PIN | FOLL_LONGTERM+ pin_longterm_pages_fast FOLL_PIN | FOLL_LONGTERM+ pin_longterm_pages_remote FOLL_PIN | FOLL_LONGTERM++Tracking dma-pinned pages+=========================++Some of the key design constraints, and solutions, for tracking dma-pinned+pages:++* An actual reference count, per struct page, is required. This is because+ multiple processes may pin and unpin a page.++* False positives (reporting that a page is dma-pinned, when in fact it is not)+ are acceptable, but false negatives are not.++* struct page may not be increased in size for this, and all fields are already+ used.++* Given the above, we can overload the page->_refcount field by using, sort of,+ the upper bits in that field for a dma-pinned count. "Sort of", means that,+ rather than dividing page->_refcount into bit fields, we simple add a medium-+ large value (GUP_PIN_COUNTING_BIAS, initially chosen to be 1024: 10 bits) to+ page->_refcount. This provides fuzzy behavior: if a page has get_page() called+ on it 1024 times, then it will appear to have a single dma-pinned count.+ And again, that's acceptable.++This also leads to limitations: there are only 31-10==21 bits available for a+counter that increments 10 bits at a time.++TODO: for 1GB and larger huge pages, this is cutting it close. That's because+when pin_user_pages() follows such pages, it increments the head page by "1"+(where "1" used to mean "+1" for get_user_pages(), but now means "+1024" for+pin_user_pages()) for each tail page. So if you have a 1GB huge page:++* There are 256K (18 bits) worth of 4 KB tail pages.+* There are 21 bits available to count up via GUP_PIN_COUNTING_BIAS (that is,+ 10 bits at a time)+* There are 21 - 18 == 3 bits available to count. Except that there aren't,+ because you need to allow for a few normal get_page() calls on the head page,+ as well. Fortunately, the approach of using addition, rather than "hard"+ bitfields, within page->_refcount, allows for sharing these bits gracefully.+ But we're still looking at about 8 references.++This, however, is a missing feature more than anything else, because it's easily+solved by addressing an obvious inefficiency in the original get_user_pages()+approach of retrieving pages: stop treating all the pages as if they were+PAGE_SIZE. Retrieve huge pages as huge pages. The callers need to be aware of+this, so some work is required. Once that's in place, this limitation mostly+disappears from view, because there will be ample refcounting range available.++* Callers must specifically request "dma-pinned tracking of pages". In other+ words, just calling get_user_pages() will not suffice; a new set of functions,+ pin_user_page() and related, must be used.++FOLL_PIN, FOLL_GET, FOLL_LONGTERM: when to use which flags+==========================================================++Thanks to Jan Kara, Vlastimil Babka and several other -mm people, for describing+these categories:++CASE 1: Direct IO (DIO)+-----------------------+There are GUP references to pages that are serving+as DIO buffers. These buffers are needed for a relatively short time (so they+are not "long term"). No special synchronization with page_mkclean() or+munmap() is provided. Therefore, flags to set at the call site are: ::++ FOLL_PIN++...but rather than setting FOLL_PIN directly, call sites should use one of+the pin_user_pages*() routines that set FOLL_PIN.++CASE 2: RDMA+------------+There are GUP references to pages that are serving as DMA+buffers. These buffers are needed for a long time ("long term"). No special+synchronization with page_mkclean() or munmap() is provided. Therefore, flags+to set at the call site are: ::++ FOLL_PIN | FOLL_LONGTERM++NOTE: Some pages, such as DAX pages, cannot be pinned with longterm pins. That's+because DAX pages do not have a separate page cache, and so "pinning" implies+locking down file system blocks, which is not (yet) supported in that way.++CASE 3: Hardware with page faulting support+-------------------------------------------+Here, a well-written driver doesn't normally need to pin pages at all. However,+if the driver does choose to do so, it can register MMU notifiers for the range,+and will be called back upon invalidation. Either way (avoiding page pinning, or+using MMU notifiers to unpin upon request), there is proper synchronization with+both filesystem and mm (page_mkclean(), munmap(), etc).++Therefore, neither flag needs to be set.++In this case, ideally, neither get_user_pages() nor pin_user_pages() should be+called. Instead, the software should be written so that it does not pin pages.+This allows mm and filesystems to operate more efficiently and reliably.++CASE 4: Pinning for struct page manipulation only+-------------------------------------------------+Here, normal GUP calls are sufficient, so neither flag needs to be set.++page_dma_pinned(): the whole point of pinning+=============================================++The whole point of marking pages as "DMA-pinned" or "gup-pinned" is to be able+to query, "is this page DMA-pinned?" That allows code such as page_mkclean()+(and file system writeback code in general) to make informed decisions about+what to do when a page cannot be unmapped due to such pins.++What to do in those cases is the subject of a years-long series of discussions+and debates (see the References at the end of this document). It's a TODO item+here: fill in the details once that's worked out. Meanwhile, it's safe to say+that having this available: ::++ static inline bool page_dma_pinned(struct page *page)++...is a prerequisite to solving the long-running gup+DMA problem.++Another way of thinking about FOLL_GET, FOLL_PIN, and FOLL_LONGTERM+===================================================================++Another way of thinking about these flags is as a progression of restrictions:+FOLL_GET is for struct page manipulation, without affecting the data that the+struct page refers to. FOLL_PIN is a *replacement* for FOLL_GET, and is for+short term pins on pages whose data *will* get accessed. As such, FOLL_PIN is+a "more severe" form of pinning. And finally, FOLL_LONGTERM is an even more+restrictive case that has FOLL_PIN as a prerequisite: this is for pages that+will be pinned longterm, and whose data will be accessed.++Unit testing+============+This file::++ tools/testing/selftests/vm/gup_benchmark.c++has the following new calls to exercise the new pin*() wrapper functions:++* PIN_FAST_BENCHMARK (./gup_benchmark -a)+* PIN_LONGTERM_BENCHMARK (./gup_benchmark -a)+* PIN_BENCHMARK (./gup_benchmark -a)++You can monitor how many total dma-pinned pages have been acquired and released+since the system was booted, via two new /proc/vmstat entries: ::++ /proc/vmstat/nr_foll_pin_requested+ /proc/vmstat/nr_foll_pin_requested++Those are both going to show zero, unless CONFIG_DEBUG_VM is set. This is+because there is a noticeable performance drop in put_user_page(), when they+are activated.++References+==========++*`Some slow progress on get_user_pages() (Apr 2, 2019) <https://lwn.net/Articles/784574/>`_+*`DMA and get_user_pages() (LPC: Dec 12, 2018) <https://lwn.net/Articles/774411/>`_+*`The trouble with get_user_pages() (Apr 30, 2018) <https://lwn.net/Articles/753027/>`_++John Hubbard, October, 2019
@@ -1552,6 +1566,10 @@ long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,intget_user_pages_fast(unsignedlongstart,intnr_pages,unsignedintgup_flags,structpage**pages);+intpin_user_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,structpage**pages);+intpin_longterm_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,structpage**pages);intaccount_locked_vm(structmm_struct*mm,unsignedlongpages,boolinc);int__account_locked_vm(structmm_struct*mm,unsignedlongpages,boolinc,
@@ -2610,13 +2628,15 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,#define FOLL_ANON 0x8000 /* don't do file mappings */#define FOLL_LONGTERM 0x10000 /* mapping lifetime is indefinite: see below */#define FOLL_SPLIT_PMD 0x20000 /* split huge pmd before returning */+#define FOLL_PIN 0x40000 /* pages must be released via put_user_page() *//*-*NOTEonFOLL_LONGTERM:+*FOLL_PINandFOLL_LONGTERMmaybeusedinvariouscombinationswitheach+*other.Hereiswhattheymean,andhowtousethem:**FOLL_LONGTERMindicatesthatthepagewillbeheldforanindefinitetime-*period_often_underuserspacecontrol.Thisiscontrastedwith-*iov_iter_get_pages()whereusageswhicharetransient.+*period_often_underuserspacecontrol.Thisisincontrastto+*iov_iter_get_pages(),whereusageswhicharetransient.**FIXME:Forpageswhicharepartofafilesystem,mappingsaresubjecttothe*lifetimeenforcedbythefilesystemandweneedguaranteesthatlongterm
@@ -1626,6 +1647,14 @@ long get_user_pages(unsigned long start, unsigned long nr_pages,unsignedintgup_flags,structpage**pages,structvm_area_struct**vmas){+/*+*FOLL_PINmustonlybesetinternallybythepin_user_page*()and+*pin_longterm_*()APIs,neverdirectlybythecaller,soenforcethat+*withanassertion:+*/+if(WARN_ON_ONCE(gup_flags&FOLL_PIN))+return-EINVAL;+return__gup_longterm_locked(current,current->mm,start,nr_pages,pages,vmas,gup_flags|FOLL_TOUCH);}
@@ -2377,29 +2406,14 @@ static int __gup_longterm_unlocked(unsigned long start, int nr_pages,returnret;}-/**-*get_user_pages_fast()-pinuserpagesinmemory-*@start:startinguseraddress-*@nr_pages:numberofpagesfromstarttopin-*@gup_flags:flagsmodifyingpinbehaviour-*@pages:arraythatreceivespointerstothepagespinned.-*Shouldbeatleastnr_pageslong.-*-*Attempttopinuserpagesinmemorywithouttakingmm->mmap_sem.-*Ifnotsuccessful,itwillfallbacktotakingthelockand-*callingget_user_pages().-*-*Returnsnumberofpagespinned.Thismaybefewerthanthenumber-*requested.Ifnr_pagesis0ornegative,returns0.Ifnopages-*werepinned,returns-errno.-*/-intget_user_pages_fast(unsignedlongstart,intnr_pages,-unsignedintgup_flags,structpage**pages)+staticintinternal_get_user_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,+structpage**pages){unsignedlongaddr,len,end;intnr=0,ret=0;-if(WARN_ON_ONCE(gup_flags&~(FOLL_WRITE|FOLL_LONGTERM)))+if(WARN_ON_ONCE(gup_flags&~(FOLL_WRITE|FOLL_LONGTERM|FOLL_PIN)))return-EINVAL;start=untagged_addr(start)&PAGE_MASK;
@@ -2439,4 +2453,208 @@ int get_user_pages_fast(unsigned long start, int nr_pages,returnret;}++/**+*get_user_pages_fast()-pinuserpagesinmemory+*@start:startinguseraddress+*@nr_pages:numberofpagesfromstarttopin+*@gup_flags:flagsmodifyingpinbehaviour+*@pages:arraythatreceivespointerstothepagespinned.+*Shouldbeatleastnr_pageslong.+*+*Attempttopinuserpagesinmemorywithouttakingmm->mmap_sem.+*Ifnotsuccessful,itwillfallbacktotakingthelockand+*callingget_user_pages().+*+*Returnsnumberofpagespinned.Thismaybefewerthanthenumberrequested.+*Ifnr_pagesis0ornegative,returns0.Ifnopageswerepinned,returns+*-errno.+*/+intget_user_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,structpage**pages)+{+/*+*FOLL_PINmustonlybesetinternallybythepin_user_page*()and+*pin_longterm_*()APIs,neverdirectlybythecaller,soenforcethat:+*/+if(WARN_ON_ONCE(gup_flags&FOLL_PIN))+return-EINVAL;++returninternal_get_user_pages_fast(start,nr_pages,gup_flags,pages);+}EXPORT_SYMBOL_GPL(get_user_pages_fast);++/**+*pin_user_pages_fast()-pinuserpagesinmemorywithouttakinglocks+*+*Nearlythesameasget_user_pages_fast(),exceptthatFOLL_PINisset.See+*get_user_pages_fast()fordocumentationonthefunctionarguments,because+*theargumentshereareidentical.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaput_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstforfurtherdetails.+*+*ThisisintendedforCase1(DIO)inDocumentation/vm/pin_user_pages.rst.It+*isNOTintendedforCase2(RDMA:long-termpins).+*/+intpin_user_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,structpage**pages)+{+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=FOLL_PIN;+returninternal_get_user_pages_fast(start,nr_pages,gup_flags,pages);+}+EXPORT_SYMBOL_GPL(pin_user_pages_fast);++/**+*pin_longterm_pages_fast()-pinuserpagesinmemorywithouttakinglocks+*+*Nearlythesameasget_user_pages_fast(),exceptthatFOLL_PINand+*FOLL_LONGTERMareset.Seeget_user_pages_fast()fordocumentationonthe+*functionarguments,becausetheargumentshereareidentical.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaput_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstforfurtherdetails.+*+*FOLL_LONGTERMmeansthatthepagesarebeingpinnedfor"long term"use,+*typicallybyanon-CPUdevice,andwecannotbesurethatwaitingfora+*pinnedpagetobecomeunpinwillbeeffective.+*+*ThisisintendedforCase2(RDMA:long-termpins)oftheFOLL_PIN+*documentation.+*/+intpin_longterm_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,structpage**pages)+{+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=(FOLL_PIN|FOLL_LONGTERM);+returninternal_get_user_pages_fast(start,nr_pages,gup_flags,pages);+}+EXPORT_SYMBOL_GPL(pin_longterm_pages_fast);++/**+*pin_user_pages_remote()-pinpagesofaremoteprocess(task!=current)+*+*Nearlythesameasget_user_pages_remote(),exceptthatFOLL_PINisset.See+*get_user_pages_remote()fordocumentationonthefunctionarguments,because+*theargumentshereareidentical.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaput_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstfordetails.+*+*ThisisintendedforCase1(DIO)inDocumentation/vm/pin_user_pages.rst.It+*isNOTintendedforCase2(RDMA:long-termpins).+*/+longpin_user_pages_remote(structtask_struct*tsk,structmm_struct*mm,+unsignedlongstart,unsignedlongnr_pages,+unsignedintgup_flags,structpage**pages,+structvm_area_struct**vmas,int*locked)+{+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=FOLL_TOUCH|FOLL_REMOTE|FOLL_PIN;++return__get_user_pages_locked(tsk,mm,start,nr_pages,pages,vmas,+locked,gup_flags);+}+EXPORT_SYMBOL(pin_user_pages_remote);++/**+*pin_longterm_pages_remote()-pinpagesofaremoteprocess(task!=current)+*+*Nearlythesameasget_user_pages_remote(),butnotethatFOLL_TOUCHisnot+*set,andFOLL_PINandFOLL_LONGTERMareset.Seeget_user_pages_remote()for+*documentationonthefunctionarguments,becausetheargumentshereare+*identical.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaput_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstforfurtherdetails.+*+*FOLL_LONGTERMmeansthatthepagesarebeingpinnedfor"long term"use,+*typicallybyanon-CPUdevice,andwecannotbesurethatwaitingfora+*pinnedpagetobecomeunpinwillbeeffective.+*+*ThisisintendedforCase2(RDMA:long-termpins)in+*Documentation/vm/pin_user_pages.rst.+*/+longpin_longterm_pages_remote(structtask_struct*tsk,structmm_struct*mm,+unsignedlongstart,unsignedlongnr_pages,+unsignedintgup_flags,structpage**pages,+structvm_area_struct**vmas,int*locked)+{+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=FOLL_LONGTERM|FOLL_REMOTE|FOLL_PIN;++return__get_user_pages_locked(tsk,mm,start,nr_pages,pages,vmas,+locked,gup_flags);+}+EXPORT_SYMBOL(pin_longterm_pages_remote);++/**+*pin_user_pages()-pinuserpagesinmemoryforusebyotherdevices+*+*Nearlythesameasget_user_pages(),exceptthatFOLL_TOUCHisnotset,and+*FOLL_PINisset.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaput_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstfordetails.+*+*ThisisintendedforCase1(DIO)inDocumentation/vm/pin_user_pages.rst.It+*isNOTintendedforCase2(RDMA:long-termpins).+*/+longpin_user_pages(unsignedlongstart,unsignedlongnr_pages,+unsignedintgup_flags,structpage**pages,+structvm_area_struct**vmas)+{+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=FOLL_PIN;+return__gup_longterm_locked(current,current->mm,start,nr_pages,+pages,vmas,gup_flags);+}+EXPORT_SYMBOL(pin_user_pages);++/**+*pin_longterm_pages()-pinuserpagesinmemoryforlong-termuse(RDMA,+*typically)+*+*Nearlythesameasget_user_pages(),exceptthatFOLL_PINandFOLL_LONGTERM+*areset.Seeget_user_pages_fast()fordocumentationonthefunction+*arguments,becausetheargumentshereareidentical.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaput_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstforfurtherdetails.+*+*FOLL_LONGTERMmeansthatthepagesarebeingpinnedfor"long term"use,+*typicallybyanon-CPUdevice,andwecannotbesurethatwaitingfora+*pinnedpagetobecomeunpinwillbeeffective.+*+*ThisisintendedforCase2(RDMA:long-termpins)in+*Documentation/vm/pin_user_pages.rst.+*/+longpin_longterm_pages(unsignedlongstart,unsignedlongnr_pages,+unsignedintgup_flags,structpage**pages,+structvm_area_struct**vmas)+{+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=FOLL_PIN|FOLL_LONGTERM;+return__gup_longterm_locked(current,current->mm,start,nr_pages,+pages,vmas,gup_flags);+}+EXPORT_SYMBOL(pin_longterm_pages);
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:09:38
Convert drm/via to use the new pin_user_pages_fast() call, which sets
FOLL_PIN. Setting FOLL_PIN is now required for code that requires
tracking of pinned pages, and therefore for any code that calls
put_user_page().
In partial anticipation of this work, the drm/via driver was already
calling put_user_page() instead of put_page(). Therefore, in order to
convert from the get_user_pages()/put_page() model, to the
pin_user_pages()/put_user_page() model, the only change required
is to change get_user_pages() to pin_user_pages().
Acked-by: Daniel Vetter <redacted>
Reviewed-by: Jérôme Glisse <redacted>
Reviewed-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/gpu/drm/via/via_dmablit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:09:47
As it says in the updated comment in gup.c: current FOLL_LONGTERM
behavior is incompatible with FAULT_FLAG_ALLOW_RETRY because of the
FS DAX check requirement on vmas.
However, the corresponding restriction in get_user_pages_remote() was
slightly stricter than is actually required: it forbade all
FOLL_LONGTERM callers, but we can actually allow FOLL_LONGTERM callers
that do not set the "locked" arg.
Update the code and comments accordingly, and update the VFIO caller
to take advantage of this, fixing a bug as a result: the VFIO caller
is logically a FOLL_LONGTERM user.
Thanks to Jason Gunthorpe for pointing out a clean way to fix this.
Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Jerome Glisse <redacted>
Cc: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++-----------------
mm/gup.c | 13 ++++++++-----
2 files changed, 21 insertions(+), 22 deletions(-)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:10:03
Convert process_vm_access to use the new pin_user_pages_remote()
call, which sets FOLL_PIN. Setting FOLL_PIN is now required for
code that requires tracking of pinned pages.
Also, release the pages via put_user_page*().
Also, rename "pages" to "pinned_pages", as this makes for
easier reading of process_vm_rw_single_vec().
Reviewed-by: Jérôme Glisse <redacted>
Reviewed-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
mm/process_vm_access.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
@@ -42,12 +42,11 @@ static int process_vm_rw_pages(struct page **pages,if(copy>len)copy=len;-if(vm_write){+if(vm_write)copied=copy_page_from_iter(page,offset,copy,iter);-set_page_dirty_lock(page);-}else{+elsecopied=copy_page_to_iter(page,offset,copy,iter);-}+len-=copied;if(copied<copy&&iov_iter_count(iter))return-EFAULT;
@@ -96,7 +95,7 @@ static int process_vm_rw_single_vec(unsigned long addr,flags|=FOLL_WRITE;while(!rc&&nr_pages&&iov_iter_count(iter)){-intpages=min(nr_pages,max_pages_per_loop);+intpinned_pages=min(nr_pages,max_pages_per_loop);intlocked=1;size_tbytes;
@@ -106,14 +105,15 @@ static int process_vm_rw_single_vec(unsigned long addr,*current/current->mm*/down_read(&mm->mmap_sem);-pages=get_user_pages_remote(task,mm,pa,pages,flags,-process_pages,NULL,&locked);+pinned_pages=pin_user_pages_remote(task,mm,pa,pinned_pages,+flags,process_pages,+NULL,&locked);if(locked)up_read(&mm->mmap_sem);-if(pages<=0)+if(pinned_pages<=0)return-EFAULT;-bytes=pages*PAGE_SIZE-start_offset;+bytes=pinned_pages*PAGE_SIZE-start_offset;if(bytes>len)bytes=len;
@@ -122,10 +122,12 @@ static int process_vm_rw_single_vec(unsigned long addr,vm_write);len-=bytes;start_offset=0;-nr_pages-=pages;-pa+=pages*PAGE_SIZE;-while(pages)-put_page(process_pages[--pages]);+nr_pages-=pinned_pages;+pa+=pinned_pages*PAGE_SIZE;++/* If vm_write is set, the pages need to be made dirty: */+put_user_pages_dirty_lock(process_pages,pinned_pages,+vm_write);}returnrc;
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:10:14
After DMA is complete, and the device and CPU caches are synchronized,
it's still required to mark the CPU pages as dirty, if the data was
coming from the device. However, this driver was just issuing a
bare put_page() call, without any set_page_dirty*() call.
Fix the problem, by calling set_page_dirty_lock() if the CPU pages
were potentially receiving data from the device.
Acked-by: Hans Verkuil <redacted>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/media/v4l2-core/videobuf-dma-sg.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:10:24
A subsequent patch requires access to gup flags, so
pass the flags argument through to the __gup_device_*
functions.
Also placate checkpatch.pl by shortening a nearby line.
Reviewed-by: Jérôme Glisse <redacted>
Reviewed-by: Ira Weiny <redacted>
Cc: Kirill A. Shutemov <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
mm/gup.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
@@ -1890,7 +1890,8 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,#if defined(CONFIG_ARCH_HAS_PTE_DEVMAP) && defined(CONFIG_TRANSPARENT_HUGEPAGE)staticint__gup_device_huge(unsignedlongpfn,unsignedlongaddr,-unsignedlongend,structpage**pages,int*nr)+unsignedlongend,unsignedintflags,+structpage**pages,int*nr){intnr_start=*nr;structdev_pagemap*pgmap=NULL;
@@ -1916,13 +1917,14 @@ static int __gup_device_huge(unsigned long pfn, unsigned long addr,}staticint__gup_device_huge_pmd(pmd_torig,pmd_t*pmdp,unsignedlongaddr,-unsignedlongend,structpage**pages,int*nr)+unsignedlongend,unsignedintflags,+structpage**pages,int*nr){unsignedlongfault_pfn;intnr_start=*nr;fault_pfn=pmd_pfn(orig)+((addr&~PMD_MASK)>>PAGE_SHIFT);-if(!__gup_device_huge(fault_pfn,addr,end,pages,nr))+if(!__gup_device_huge(fault_pfn,addr,end,flags,pages,nr))return0;if(unlikely(pmd_val(orig)!=pmd_val(*pmdp))){
@@ -1933,13 +1935,14 @@ static int __gup_device_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,}staticint__gup_device_huge_pud(pud_torig,pud_t*pudp,unsignedlongaddr,-unsignedlongend,structpage**pages,int*nr)+unsignedlongend,unsignedintflags,+structpage**pages,int*nr){unsignedlongfault_pfn;intnr_start=*nr;fault_pfn=pud_pfn(orig)+((addr&~PUD_MASK)>>PAGE_SHIFT);-if(!__gup_device_huge(fault_pfn,addr,end,pages,nr))+if(!__gup_device_huge(fault_pfn,addr,end,flags,pages,nr))return0;if(unlikely(pud_val(orig)!=pud_val(*pudp))){
@@ -1950,14 +1953,16 @@ static int __gup_device_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,}#elsestaticint__gup_device_huge_pmd(pmd_torig,pmd_t*pmdp,unsignedlongaddr,-unsignedlongend,structpage**pages,int*nr)+unsignedlongend,unsignedintflags,+structpage**pages,int*nr){BUILD_BUG();return0;}staticint__gup_device_huge_pud(pud_tpud,pud_t*pudp,unsignedlongaddr,-unsignedlongend,structpage**pages,int*nr)+unsignedlongend,unsignedintflags,+structpage**pages,int*nr){BUILD_BUG();return0;
@@ -2062,7 +2067,8 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,if(pmd_devmap(orig)){if(unlikely(flags&FOLL_LONGTERM))return0;-return__gup_device_huge_pmd(orig,pmdp,addr,end,pages,nr);+return__gup_device_huge_pmd(orig,pmdp,addr,end,flags,+pages,nr);}refs=0;
@@ -2092,7 +2098,8 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,}staticintgup_huge_pud(pud_torig,pud_t*pudp,unsignedlongaddr,-unsignedlongend,unsignedintflags,structpage**pages,int*nr)+unsignedlongend,unsignedintflags,+structpage**pages,int*nr){structpage*head,*page;intrefs;
@@ -2103,7 +2110,8 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,if(pud_devmap(orig)){if(unlikely(flags&FOLL_LONGTERM))return0;-return__gup_device_huge_pud(orig,pudp,addr,end,pages,nr);+return__gup_device_huge_pud(orig,pudp,addr,end,flags,+pages,nr);}refs=0;
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:10:35
1. Call the new global pin_user_pages_fast(), from pin_goldfish_pages().
2. As required by pin_user_pages(), release these pages via
put_user_page(). In this case, do so via put_user_pages_dirty_lock().
That has the side effect of calling set_page_dirty_lock(), instead
of set_page_dirty(). This is probably more accurate.
As Christoph Hellwig put it, "set_page_dirty() is only safe if we are
dealing with a file backed page where we have reference on the inode it
hangs off." [1]
Another side effect is that the release code is simplified because
the page[] loop is now in gup.c instead of here, so just delete the
local release_user_pages() entirely, and call
put_user_pages_dirty_lock() directly, instead.
[1] https://lore.kernel.org/r/20190723153640.GB720@lst.de
Reviewed-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/platform/goldfish/goldfish_pipe.c | 17 +++--------------
1 file changed, 3 insertions(+), 14 deletions(-)
@@ -274,7 +274,7 @@ static int pin_goldfish_pages(unsigned long first_page,*iter_last_page_size=last_page_size;}-ret=get_user_pages_fast(first_page,requested_pages,+ret=pin_user_pages_fast(first_page,requested_pages,!is_write?FOLL_WRITE:0,pages);if(ret<=0)
@@ -285,18 +285,6 @@ static int pin_goldfish_pages(unsigned long first_page,returnret;}-staticvoidrelease_user_pages(structpage**pages,intpages_count,-intis_write,s32consumed_size)-{-inti;--for(i=0;i<pages_count;i++){-if(!is_write&&consumed_size>0)-set_page_dirty(pages[i]);-put_page(pages[i]);-}-}-/* Populate the call parameters, merging adjacent pages together */staticvoidpopulate_rw_params(structpage**pages,intpages_count,
@@ -372,7 +360,8 @@ static int transfer_max_buffers(struct goldfish_pipe *pipe,*consumed_size=pipe->command_buffer->rw_params.consumed_size;-release_user_pages(pipe->pages,pages_count,is_write,*consumed_size);+put_user_pages_dirty_lock(pipe->pages,pages_count,+!is_write&&*consumed_size>0);mutex_unlock(&pipe->lock);return0;
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:10:55
1. Avoid naming conflicts: rename local static function from
"pin_user_pages()" to "pin_goldfish_pages()".
An upcoming patch will introduce a global pin_user_pages()
function.
Reviewed-by: Jérôme Glisse <redacted>
Reviewed-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/platform/goldfish/goldfish_pipe.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:11:10
And get rid of the mmap_sem calls, as part of that. Note
that get_user_pages_fast() will, if necessary, fall back to
__gup_longterm_unlocked(), which takes the mmap_sem as needed.
Reviewed-by: Ira Weiny <redacted>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/infiniband/core/umem.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:11:22
There are four locations in gup.c that have a fair amount of code
duplication. This means that changing one requires making the same
changes in four places, not to mention reading the same code four
times, and wondering if there are subtle differences.
Factor out the common code into static functions, thus reducing the
overall line count and the code's complexity.
Also, take the opportunity to slightly improve the efficiency of the
error cases, by doing a mass subtraction of the refcount, surrounded
by get_page()/put_page().
Also, further simplify (slightly), by waiting until the the successful
end of each routine, to increment *nr.
Reviewed-by: Jérôme Glisse <redacted>
Cc: Ira Weiny <redacted>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Aneesh Kumar K.V <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
mm/gup.c | 104 ++++++++++++++++++++++++-------------------------------
1 file changed, 45 insertions(+), 59 deletions(-)
@@ -1969,6 +1969,34 @@ static int __gup_device_huge_pud(pud_t pud, pud_t *pudp, unsigned long addr,}#endif+staticint__record_subpages(structpage*page,unsignedlongaddr,+unsignedlongend,structpage**pages,intnr)+{+intnr_recorded_pages=0;++do{+pages[nr]=page;+nr++;+page++;+nr_recorded_pages++;+}while(addr+=PAGE_SIZE,addr!=end);+returnnr_recorded_pages;+}++staticvoidput_compound_head(structpage*page,intrefs)+{+/* Do a get_page() first, in case refs == page->_refcount */+get_page(page);+page_ref_sub(page,refs);+put_page(page);+}++staticvoid__huge_pt_done(structpage*head,intnr_recorded_pages,int*nr)+{+*nr+=nr_recorded_pages;+SetPageReferenced(head);+}+#ifdef CONFIG_ARCH_HAS_HUGEPDstaticunsignedlonghugepte_addr_end(unsignedlongaddr,unsignedlongend,unsignedlongsz)
@@ -1998,33 +2026,20 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,/* hugepages are never "special" */VM_BUG_ON(!pfn_valid(pte_pfn(pte)));-refs=0;head=pte_page(pte);-page=head+((addr&(sz-1))>>PAGE_SHIFT);-do{-VM_BUG_ON(compound_head(page)!=head);-pages[*nr]=page;-(*nr)++;-page++;-refs++;-}while(addr+=PAGE_SIZE,addr!=end);+refs=__record_subpages(page,addr,end,pages,*nr);head=try_get_compound_head(head,refs);-if(!head){-*nr-=refs;+if(!head)return0;-}if(unlikely(pte_val(pte)!=pte_val(*ptep))){-/* Could be optimized better */-*nr-=refs;-while(refs--)-put_page(head);+put_compound_head(head,refs);return0;}-SetPageReferenced(head);+__huge_pt_done(head,refs,nr);return1;}
@@ -2071,29 +2086,19 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,pages,nr);}-refs=0;page=pmd_page(orig)+((addr&~PMD_MASK)>>PAGE_SHIFT);-do{-pages[*nr]=page;-(*nr)++;-page++;-refs++;-}while(addr+=PAGE_SIZE,addr!=end);+refs=__record_subpages(page,addr,end,pages,*nr);head=try_get_compound_head(pmd_page(orig),refs);-if(!head){-*nr-=refs;+if(!head)return0;-}if(unlikely(pmd_val(orig)!=pmd_val(*pmdp))){-*nr-=refs;-while(refs--)-put_page(head);+put_compound_head(head,refs);return0;}-SetPageReferenced(head);+__huge_pt_done(head,refs,nr);return1;}
@@ -2114,29 +2119,19 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,pages,nr);}-refs=0;page=pud_page(orig)+((addr&~PUD_MASK)>>PAGE_SHIFT);-do{-pages[*nr]=page;-(*nr)++;-page++;-refs++;-}while(addr+=PAGE_SIZE,addr!=end);+refs=__record_subpages(page,addr,end,pages,*nr);head=try_get_compound_head(pud_page(orig),refs);-if(!head){-*nr-=refs;+if(!head)return0;-}if(unlikely(pud_val(orig)!=pud_val(*pudp))){-*nr-=refs;-while(refs--)-put_page(head);+put_compound_head(head,refs);return0;}-SetPageReferenced(head);+__huge_pt_done(head,refs,nr);return1;}
@@ -2151,29 +2146,20 @@ static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr,return0;BUILD_BUG_ON(pgd_devmap(orig));-refs=0;+page=pgd_page(orig)+((addr&~PGDIR_MASK)>>PAGE_SHIFT);-do{-pages[*nr]=page;-(*nr)++;-page++;-refs++;-}while(addr+=PAGE_SIZE,addr!=end);+refs=__record_subpages(page,addr,end,pages,*nr);head=try_get_compound_head(pgd_page(orig),refs);-if(!head){-*nr-=refs;+if(!head)return0;-}if(unlikely(pgd_val(orig)!=pgd_val(*pgdp))){-*nr-=refs;-while(refs--)-put_page(head);+put_compound_head(head,refs);return0;}-SetPageReferenced(head);+__huge_pt_done(head,refs,nr);return1;}
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:11:33
An upcoming patch changes and complicates the refcounting and
especially the "put page" aspects of it. In order to keep
everything clean, refactor the devmap page release routines:
* Rename put_devmap_managed_page() to page_is_devmap_managed(),
and limit the functionality to "read only": return a bool,
with no side effects.
* Add a new routine, put_devmap_managed_page(), to handle checking
what kind of page it is, and what kind of refcount handling it
requires.
* Rename __put_devmap_managed_page() to free_devmap_managed_page(),
and limit the functionality to unconditionally freeing a devmap
page.
This is originally based on a separate patch by Ira Weiny, which
applied to an early version of the put_user_page() experiments.
Since then, Jérôme Glisse suggested the refactoring described above.
Suggested-by: Jérôme Glisse <redacted>
Signed-off-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
include/linux/mm.h | 27 ++++++++++++++++---
mm/memremap.c | 67 ++++++++++++++++++++--------------------------
2 files changed, 53 insertions(+), 41 deletions(-)
@@ -410,48 +410,39 @@ struct dev_pagemap *get_dev_pagemap(unsigned long pfn,EXPORT_SYMBOL_GPL(get_dev_pagemap);#ifdef CONFIG_DEV_PAGEMAP_OPS-void__put_devmap_managed_page(structpage*page)+voidfree_devmap_managed_page(structpage*page){-intcount=page_ref_dec_return(page);+/* Clear Active bit in case of parallel mark_page_accessed */+__ClearPageActive(page);+__ClearPageWaiters(page);++mem_cgroup_uncharge(page);/*-*Ifrefcountis1thenpageisfreedandrefcountisstableasnobody-*holdsareferenceonthepage.+*Whenadevice_privatepageisfreed,thepage->mappingfield+*maystillcontaina(stale)mappingvalue.Forexample,the+*lowerbitsofpage->mappingmaystillidentifythepageas+*ananonymouspage.Ultimately,thisentirefieldisjust+*staleandwrong,anditwillcauseerrorsifnotcleared.+*Oneexampleis:+*+*migrate_vma_pages()+*migrate_vma_insert_page()+*page_add_new_anon_rmap()+*__page_set_anon_rmap()+*...checkspage->mapping,viaPageAnon(page)call,+*andincorrectlyconcludesthatthepageisan+*anonymouspage.Therefore,itincorrectly,+*silentlyfailstosetupthenewanonrmap.+*+*ForothertypesofZONE_DEVICEpages,migrationiseither+*handleddifferentlyornotdoneatall,sothereisnoneed+*toclearpage->mapping.*/-if(count==1){-/* Clear Active bit in case of parallel mark_page_accessed */-__ClearPageActive(page);-__ClearPageWaiters(page);--mem_cgroup_uncharge(page);--/*-*Whenadevice_privatepageisfreed,thepage->mappingfield-*maystillcontaina(stale)mappingvalue.Forexample,the-*lowerbitsofpage->mappingmaystillidentifythepageas-*ananonymouspage.Ultimately,thisentirefieldisjust-*staleandwrong,anditwillcauseerrorsifnotcleared.-*Oneexampleis:-*-*migrate_vma_pages()-*migrate_vma_insert_page()-*page_add_new_anon_rmap()-*__page_set_anon_rmap()-*...checkspage->mapping,viaPageAnon(page)call,-*andincorrectlyconcludesthatthepageisan-*anonymouspage.Therefore,itincorrectly,-*silentlyfailstosetupthenewanonrmap.-*-*ForothertypesofZONE_DEVICEpages,migrationiseither-*handleddifferentlyornotdoneatall,sothereisnoneed-*toclearpage->mapping.-*/-if(is_device_private_page(page))-page->mapping=NULL;+if(is_device_private_page(page))+page->mapping=NULL;-page->pgmap->ops->page_free(page);-}elseif(!count)-__put_page(page);+page->pgmap->ops->page_free(page);}-EXPORT_SYMBOL(__put_devmap_managed_page);+EXPORT_SYMBOL(free_devmap_managed_page);#endif /* CONFIG_DEV_PAGEMAP_OPS */
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 00:11:36
An upcoming patch uses try_get_compound_head() more widely,
so move it to the top of gup.c.
Also fix a tiny spelling error and a checkpatch.pl warning.
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
mm/gup.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
From: Mike Rapoport <rppt@kernel.org> Date: 2019-11-12 06:51:28
On Mon, Nov 11, 2019 at 04:06:46PM -0800, John Hubbard wrote:
Introduce pin_user_pages*() variations of get_user_pages*() calls,
and also pin_longterm_pages*() variations.
These variants all set FOLL_PIN, which is also introduced, and
thoroughly documented.
The pin_longterm*() variants also set FOLL_LONGTERM, in addition
to FOLL_PIN:
pin_user_pages()
pin_user_pages_remote()
pin_user_pages_fast()
pin_longterm_pages()
pin_longterm_pages_remote()
pin_longterm_pages_fast()
All pages that are pinned via the above calls, must be unpinned via
put_user_page().
The underlying rules are:
* These are gup-internal flags, so the call sites should not directly
set FOLL_PIN nor FOLL_LONGTERM. That behavior is enforced with
assertions, for the new FOLL_PIN flag. However, for the pre-existing
FOLL_LONGTERM flag, which has some call sites that still directly
set FOLL_LONGTERM, there is no assertion yet.
* Call sites that want to indicate that they are going to do DirectIO
("DIO") or something with similar characteristics, should call a
get_user_pages()-like wrapper call that sets FOLL_PIN. These wrappers
will:
* Start with "pin_user_pages" instead of "get_user_pages". That
makes it easy to find and audit the call sites.
* Set FOLL_PIN
* For pages that are received via FOLL_PIN, those pages must be returned
via put_user_page().
Thanks to Jan Kara and Vlastimil Babka for explaining the 4 cases
in this documentation. (I've reworded it and expanded upon it.)
Reviewed-by: Jérôme Glisse <redacted>
Cc: Mike Rapoport <rppt@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
Reviewed-by: Mike Rapoport <redacted> # Documentation
@@ -0,0 +1,218 @@+.. SPDX-License-Identifier: GPL-2.0++====================================================+pin_user_pages() and related calls+====================================================++..contents:: :local:++Overview+========++This document describes the following functions: ::++ pin_user_pages+ pin_user_pages_fast+ pin_user_pages_remote++ pin_longterm_pages+ pin_longterm_pages_fast+ pin_longterm_pages_remote++Basic description of FOLL_PIN+=============================++FOLL_PIN and FOLL_LONGTERM are flags that can be passed to the get_user_pages*()+("gup") family of functions. FOLL_PIN has significant interactions and+interdependencies with FOLL_LONGTERM, so both are covered here.++Both FOLL_PIN and FOLL_LONGTERM are internal to gup, meaning that neither+FOLL_PIN nor FOLL_LONGTERM should not appear at the gup call sites. This allows+the associated wrapper functions (pin_user_pages() and others) to set the+correct combination of these flags, and to check for problems as well.++FOLL_PIN and FOLL_GET are mutually exclusive for a given gup call. However,+multiple threads and call sites are free to pin the same struct pages, via both+FOLL_PIN and FOLL_GET. It's just the call site that needs to choose one or the+other, not the struct page(s).++The FOLL_PIN implementation is nearly the same as FOLL_GET, except that FOLL_PIN+uses a different reference counting technique.++FOLL_PIN is a prerequisite to FOLL_LONGTGERM. Another way of saying that is,+FOLL_LONGTERM is a specific case, more restrictive case of FOLL_PIN.++Which flags are set by each wrapper+===================================++Only FOLL_PIN and FOLL_LONGTERM are covered here. These flags are added to+whatever flags the caller provides::++ Function gup flags (FOLL_PIN or FOLL_LONGTERM only)+ -------- ------------------------------------------+ pin_user_pages FOLL_PIN+ pin_user_pages_fast FOLL_PIN+ pin_user_pages_remote FOLL_PIN++ pin_longterm_pages FOLL_PIN | FOLL_LONGTERM+ pin_longterm_pages_fast FOLL_PIN | FOLL_LONGTERM+ pin_longterm_pages_remote FOLL_PIN | FOLL_LONGTERM++Tracking dma-pinned pages+=========================++Some of the key design constraints, and solutions, for tracking dma-pinned+pages:++* An actual reference count, per struct page, is required. This is because+ multiple processes may pin and unpin a page.++* False positives (reporting that a page is dma-pinned, when in fact it is not)+ are acceptable, but false negatives are not.++* struct page may not be increased in size for this, and all fields are already+ used.++* Given the above, we can overload the page->_refcount field by using, sort of,+ the upper bits in that field for a dma-pinned count. "Sort of", means that,+ rather than dividing page->_refcount into bit fields, we simple add a medium-+ large value (GUP_PIN_COUNTING_BIAS, initially chosen to be 1024: 10 bits) to+ page->_refcount. This provides fuzzy behavior: if a page has get_page() called+ on it 1024 times, then it will appear to have a single dma-pinned count.+ And again, that's acceptable.++This also leads to limitations: there are only 31-10==21 bits available for a+counter that increments 10 bits at a time.++TODO: for 1GB and larger huge pages, this is cutting it close. That's because+when pin_user_pages() follows such pages, it increments the head page by "1"+(where "1" used to mean "+1" for get_user_pages(), but now means "+1024" for+pin_user_pages()) for each tail page. So if you have a 1GB huge page:++* There are 256K (18 bits) worth of 4 KB tail pages.+* There are 21 bits available to count up via GUP_PIN_COUNTING_BIAS (that is,+ 10 bits at a time)+* There are 21 - 18 == 3 bits available to count. Except that there aren't,+ because you need to allow for a few normal get_page() calls on the head page,+ as well. Fortunately, the approach of using addition, rather than "hard"+ bitfields, within page->_refcount, allows for sharing these bits gracefully.+ But we're still looking at about 8 references.++This, however, is a missing feature more than anything else, because it's easily+solved by addressing an obvious inefficiency in the original get_user_pages()+approach of retrieving pages: stop treating all the pages as if they were+PAGE_SIZE. Retrieve huge pages as huge pages. The callers need to be aware of+this, so some work is required. Once that's in place, this limitation mostly+disappears from view, because there will be ample refcounting range available.++* Callers must specifically request "dma-pinned tracking of pages". In other+ words, just calling get_user_pages() will not suffice; a new set of functions,+ pin_user_page() and related, must be used.++FOLL_PIN, FOLL_GET, FOLL_LONGTERM: when to use which flags+==========================================================++Thanks to Jan Kara, Vlastimil Babka and several other -mm people, for describing+these categories:++CASE 1: Direct IO (DIO)+-----------------------+There are GUP references to pages that are serving+as DIO buffers. These buffers are needed for a relatively short time (so they+are not "long term"). No special synchronization with page_mkclean() or+munmap() is provided. Therefore, flags to set at the call site are: ::++ FOLL_PIN++...but rather than setting FOLL_PIN directly, call sites should use one of+the pin_user_pages*() routines that set FOLL_PIN.++CASE 2: RDMA+------------+There are GUP references to pages that are serving as DMA+buffers. These buffers are needed for a long time ("long term"). No special+synchronization with page_mkclean() or munmap() is provided. Therefore, flags+to set at the call site are: ::++ FOLL_PIN | FOLL_LONGTERM++NOTE: Some pages, such as DAX pages, cannot be pinned with longterm pins. That's+because DAX pages do not have a separate page cache, and so "pinning" implies+locking down file system blocks, which is not (yet) supported in that way.++CASE 3: Hardware with page faulting support+-------------------------------------------+Here, a well-written driver doesn't normally need to pin pages at all. However,+if the driver does choose to do so, it can register MMU notifiers for the range,+and will be called back upon invalidation. Either way (avoiding page pinning, or+using MMU notifiers to unpin upon request), there is proper synchronization with+both filesystem and mm (page_mkclean(), munmap(), etc).++Therefore, neither flag needs to be set.++In this case, ideally, neither get_user_pages() nor pin_user_pages() should be+called. Instead, the software should be written so that it does not pin pages.+This allows mm and filesystems to operate more efficiently and reliably.++CASE 4: Pinning for struct page manipulation only+-------------------------------------------------+Here, normal GUP calls are sufficient, so neither flag needs to be set.++page_dma_pinned(): the whole point of pinning+=============================================++The whole point of marking pages as "DMA-pinned" or "gup-pinned" is to be able+to query, "is this page DMA-pinned?" That allows code such as page_mkclean()+(and file system writeback code in general) to make informed decisions about+what to do when a page cannot be unmapped due to such pins.++What to do in those cases is the subject of a years-long series of discussions+and debates (see the References at the end of this document). It's a TODO item+here: fill in the details once that's worked out. Meanwhile, it's safe to say+that having this available: ::++ static inline bool page_dma_pinned(struct page *page)++...is a prerequisite to solving the long-running gup+DMA problem.++Another way of thinking about FOLL_GET, FOLL_PIN, and FOLL_LONGTERM+===================================================================++Another way of thinking about these flags is as a progression of restrictions:+FOLL_GET is for struct page manipulation, without affecting the data that the+struct page refers to. FOLL_PIN is a *replacement* for FOLL_GET, and is for+short term pins on pages whose data *will* get accessed. As such, FOLL_PIN is+a "more severe" form of pinning. And finally, FOLL_LONGTERM is an even more+restrictive case that has FOLL_PIN as a prerequisite: this is for pages that+will be pinned longterm, and whose data will be accessed.++Unit testing+============+This file::++ tools/testing/selftests/vm/gup_benchmark.c++has the following new calls to exercise the new pin*() wrapper functions:++* PIN_FAST_BENCHMARK (./gup_benchmark -a)+* PIN_LONGTERM_BENCHMARK (./gup_benchmark -a)+* PIN_BENCHMARK (./gup_benchmark -a)++You can monitor how many total dma-pinned pages have been acquired and released+since the system was booted, via two new /proc/vmstat entries: ::++ /proc/vmstat/nr_foll_pin_requested+ /proc/vmstat/nr_foll_pin_requested++Those are both going to show zero, unless CONFIG_DEBUG_VM is set. This is+because there is a noticeable performance drop in put_user_page(), when they+are activated.++References+==========++*`Some slow progress on get_user_pages() (Apr 2, 2019) <https://lwn.net/Articles/784574/>`_+*`DMA and get_user_pages() (LPC: Dec 12, 2018) <https://lwn.net/Articles/774411/>`_+*`The trouble with get_user_pages() (Apr 30, 2018) <https://lwn.net/Articles/753027/>`_++John Hubbard, October, 2019
@@ -1552,6 +1566,10 @@ long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,intget_user_pages_fast(unsignedlongstart,intnr_pages,unsignedintgup_flags,structpage**pages);+intpin_user_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,structpage**pages);+intpin_longterm_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,structpage**pages);intaccount_locked_vm(structmm_struct*mm,unsignedlongpages,boolinc);int__account_locked_vm(structmm_struct*mm,unsignedlongpages,boolinc,
@@ -2610,13 +2628,15 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,#define FOLL_ANON 0x8000 /* don't do file mappings */#define FOLL_LONGTERM 0x10000 /* mapping lifetime is indefinite: see below */#define FOLL_SPLIT_PMD 0x20000 /* split huge pmd before returning */+#define FOLL_PIN 0x40000 /* pages must be released via put_user_page() *//*-*NOTEonFOLL_LONGTERM:+*FOLL_PINandFOLL_LONGTERMmaybeusedinvariouscombinationswitheach+*other.Hereiswhattheymean,andhowtousethem:**FOLL_LONGTERMindicatesthatthepagewillbeheldforanindefinitetime-*period_often_underuserspacecontrol.Thisiscontrastedwith-*iov_iter_get_pages()whereusageswhicharetransient.+*period_often_underuserspacecontrol.Thisisincontrastto+*iov_iter_get_pages(),whereusageswhicharetransient.**FIXME:Forpageswhicharepartofafilesystem,mappingsaresubjecttothe*lifetimeenforcedbythefilesystemandweneedguaranteesthatlongterm
@@ -1626,6 +1647,14 @@ long get_user_pages(unsigned long start, unsigned long nr_pages,unsignedintgup_flags,structpage**pages,structvm_area_struct**vmas){+/*+*FOLL_PINmustonlybesetinternallybythepin_user_page*()and+*pin_longterm_*()APIs,neverdirectlybythecaller,soenforcethat+*withanassertion:+*/+if(WARN_ON_ONCE(gup_flags&FOLL_PIN))+return-EINVAL;+return__gup_longterm_locked(current,current->mm,start,nr_pages,pages,vmas,gup_flags|FOLL_TOUCH);}
@@ -2377,29 +2406,14 @@ static int __gup_longterm_unlocked(unsigned long start, int nr_pages,returnret;}-/**-*get_user_pages_fast()-pinuserpagesinmemory-*@start:startinguseraddress-*@nr_pages:numberofpagesfromstarttopin-*@gup_flags:flagsmodifyingpinbehaviour-*@pages:arraythatreceivespointerstothepagespinned.-*Shouldbeatleastnr_pageslong.-*-*Attempttopinuserpagesinmemorywithouttakingmm->mmap_sem.-*Ifnotsuccessful,itwillfallbacktotakingthelockand-*callingget_user_pages().-*-*Returnsnumberofpagespinned.Thismaybefewerthanthenumber-*requested.Ifnr_pagesis0ornegative,returns0.Ifnopages-*werepinned,returns-errno.-*/-intget_user_pages_fast(unsignedlongstart,intnr_pages,-unsignedintgup_flags,structpage**pages)+staticintinternal_get_user_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,+structpage**pages){unsignedlongaddr,len,end;intnr=0,ret=0;-if(WARN_ON_ONCE(gup_flags&~(FOLL_WRITE|FOLL_LONGTERM)))+if(WARN_ON_ONCE(gup_flags&~(FOLL_WRITE|FOLL_LONGTERM|FOLL_PIN)))return-EINVAL;start=untagged_addr(start)&PAGE_MASK;
@@ -2439,4 +2453,208 @@ int get_user_pages_fast(unsigned long start, int nr_pages,returnret;}++/**+*get_user_pages_fast()-pinuserpagesinmemory+*@start:startinguseraddress+*@nr_pages:numberofpagesfromstarttopin+*@gup_flags:flagsmodifyingpinbehaviour+*@pages:arraythatreceivespointerstothepagespinned.+*Shouldbeatleastnr_pageslong.+*+*Attempttopinuserpagesinmemorywithouttakingmm->mmap_sem.+*Ifnotsuccessful,itwillfallbacktotakingthelockand+*callingget_user_pages().+*+*Returnsnumberofpagespinned.Thismaybefewerthanthenumberrequested.+*Ifnr_pagesis0ornegative,returns0.Ifnopageswerepinned,returns+*-errno.+*/+intget_user_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,structpage**pages)+{+/*+*FOLL_PINmustonlybesetinternallybythepin_user_page*()and+*pin_longterm_*()APIs,neverdirectlybythecaller,soenforcethat:+*/+if(WARN_ON_ONCE(gup_flags&FOLL_PIN))+return-EINVAL;++returninternal_get_user_pages_fast(start,nr_pages,gup_flags,pages);+}EXPORT_SYMBOL_GPL(get_user_pages_fast);++/**+*pin_user_pages_fast()-pinuserpagesinmemorywithouttakinglocks+*+*Nearlythesameasget_user_pages_fast(),exceptthatFOLL_PINisset.See+*get_user_pages_fast()fordocumentationonthefunctionarguments,because+*theargumentshereareidentical.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaput_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstforfurtherdetails.+*+*ThisisintendedforCase1(DIO)inDocumentation/vm/pin_user_pages.rst.It+*isNOTintendedforCase2(RDMA:long-termpins).+*/+intpin_user_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,structpage**pages)+{+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=FOLL_PIN;+returninternal_get_user_pages_fast(start,nr_pages,gup_flags,pages);+}+EXPORT_SYMBOL_GPL(pin_user_pages_fast);++/**+*pin_longterm_pages_fast()-pinuserpagesinmemorywithouttakinglocks+*+*Nearlythesameasget_user_pages_fast(),exceptthatFOLL_PINand+*FOLL_LONGTERMareset.Seeget_user_pages_fast()fordocumentationonthe+*functionarguments,becausetheargumentshereareidentical.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaput_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstforfurtherdetails.+*+*FOLL_LONGTERMmeansthatthepagesarebeingpinnedfor"long term"use,+*typicallybyanon-CPUdevice,andwecannotbesurethatwaitingfora+*pinnedpagetobecomeunpinwillbeeffective.+*+*ThisisintendedforCase2(RDMA:long-termpins)oftheFOLL_PIN+*documentation.+*/+intpin_longterm_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,structpage**pages)+{+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=(FOLL_PIN|FOLL_LONGTERM);+returninternal_get_user_pages_fast(start,nr_pages,gup_flags,pages);+}+EXPORT_SYMBOL_GPL(pin_longterm_pages_fast);++/**+*pin_user_pages_remote()-pinpagesofaremoteprocess(task!=current)+*+*Nearlythesameasget_user_pages_remote(),exceptthatFOLL_PINisset.See+*get_user_pages_remote()fordocumentationonthefunctionarguments,because+*theargumentshereareidentical.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaput_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstfordetails.+*+*ThisisintendedforCase1(DIO)inDocumentation/vm/pin_user_pages.rst.It+*isNOTintendedforCase2(RDMA:long-termpins).+*/+longpin_user_pages_remote(structtask_struct*tsk,structmm_struct*mm,+unsignedlongstart,unsignedlongnr_pages,+unsignedintgup_flags,structpage**pages,+structvm_area_struct**vmas,int*locked)+{+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=FOLL_TOUCH|FOLL_REMOTE|FOLL_PIN;++return__get_user_pages_locked(tsk,mm,start,nr_pages,pages,vmas,+locked,gup_flags);+}+EXPORT_SYMBOL(pin_user_pages_remote);++/**+*pin_longterm_pages_remote()-pinpagesofaremoteprocess(task!=current)+*+*Nearlythesameasget_user_pages_remote(),butnotethatFOLL_TOUCHisnot+*set,andFOLL_PINandFOLL_LONGTERMareset.Seeget_user_pages_remote()for+*documentationonthefunctionarguments,becausetheargumentshereare+*identical.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaput_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstforfurtherdetails.+*+*FOLL_LONGTERMmeansthatthepagesarebeingpinnedfor"long term"use,+*typicallybyanon-CPUdevice,andwecannotbesurethatwaitingfora+*pinnedpagetobecomeunpinwillbeeffective.+*+*ThisisintendedforCase2(RDMA:long-termpins)in+*Documentation/vm/pin_user_pages.rst.+*/+longpin_longterm_pages_remote(structtask_struct*tsk,structmm_struct*mm,+unsignedlongstart,unsignedlongnr_pages,+unsignedintgup_flags,structpage**pages,+structvm_area_struct**vmas,int*locked)+{+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=FOLL_LONGTERM|FOLL_REMOTE|FOLL_PIN;++return__get_user_pages_locked(tsk,mm,start,nr_pages,pages,vmas,+locked,gup_flags);+}+EXPORT_SYMBOL(pin_longterm_pages_remote);++/**+*pin_user_pages()-pinuserpagesinmemoryforusebyotherdevices+*+*Nearlythesameasget_user_pages(),exceptthatFOLL_TOUCHisnotset,and+*FOLL_PINisset.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaput_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstfordetails.+*+*ThisisintendedforCase1(DIO)inDocumentation/vm/pin_user_pages.rst.It+*isNOTintendedforCase2(RDMA:long-termpins).+*/+longpin_user_pages(unsignedlongstart,unsignedlongnr_pages,+unsignedintgup_flags,structpage**pages,+structvm_area_struct**vmas)+{+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=FOLL_PIN;+return__gup_longterm_locked(current,current->mm,start,nr_pages,+pages,vmas,gup_flags);+}+EXPORT_SYMBOL(pin_user_pages);++/**+*pin_longterm_pages()-pinuserpagesinmemoryforlong-termuse(RDMA,+*typically)+*+*Nearlythesameasget_user_pages(),exceptthatFOLL_PINandFOLL_LONGTERM+*areset.Seeget_user_pages_fast()fordocumentationonthefunction+*arguments,becausetheargumentshereareidentical.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaput_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstforfurtherdetails.+*+*FOLL_LONGTERMmeansthatthepagesarebeingpinnedfor"long term"use,+*typicallybyanon-CPUdevice,andwecannotbesurethatwaitingfora+*pinnedpagetobecomeunpinwillbeeffective.+*+*ThisisintendedforCase2(RDMA:long-termpins)in+*Documentation/vm/pin_user_pages.rst.+*/+longpin_longterm_pages(unsignedlongstart,unsignedlongnr_pages,+unsignedintgup_flags,structpage**pages,+structvm_area_struct**vmas)+{+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=FOLL_PIN|FOLL_LONGTERM;+return__gup_longterm_locked(current,current->mm,start,nr_pages,+pages,vmas,gup_flags);+}+EXPORT_SYMBOL(pin_longterm_pages);
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2019-11-12 20:38:07
On Mon, Nov 11, 2019 at 04:06:37PM -0800, John Hubbard wrote:
Hi,
The cover letter is long, so the more important stuff is first:
* Jason, if you or someone could look at the the VFIO cleanup (patch 8)
and conversion to FOLL_PIN (patch 18), to make sure it's use of
remote and longterm gup matches what we discussed during the review
of v2, I'd appreciate it.
* Also for Jason and IB: as noted below, in patch 11, I am (too?) boldly
converting from put_user_pages() to release_pages().
Why are we doing this? I think things got confused here someplace, as
the comment still says:
/**
* put_user_page() - release a gup-pinned page
* @page: pointer to page to be released
*
* Pages that were pinned via get_user_pages*() must be released via
* either put_user_page(), or one of the put_user_pages*() routines
* below.
I feel like if put_user_pages() is not the correct way to undo
get_user_pages() then it needs to be deleted.
Jason
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2019-11-12 20:43:45
On Mon, Nov 11, 2019 at 04:06:45PM -0800, John Hubbard wrote:
As it says in the updated comment in gup.c: current FOLL_LONGTERM
behavior is incompatible with FAULT_FLAG_ALLOW_RETRY because of the
FS DAX check requirement on vmas.
However, the corresponding restriction in get_user_pages_remote() was
slightly stricter than is actually required: it forbade all
FOLL_LONGTERM callers, but we can actually allow FOLL_LONGTERM callers
that do not set the "locked" arg.
Update the code and comments accordingly, and update the VFIO caller
to take advantage of this, fixing a bug as a result: the VFIO caller
is logically a FOLL_LONGTERM user.
Thanks to Jason Gunthorpe for pointing out a clean way to fix this.
Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Jerome Glisse <redacted>
Cc: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++-----------------
mm/gup.c | 13 ++++++++-----
2 files changed, 21 insertions(+), 22 deletions(-)
This matches what I thought, but I think DanW should check it too, and
the vfio users should test..
@@ -348,24 +348,20 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,flags|=FOLL_WRITE;down_read(&mm->mmap_sem);-if(mm==current->mm){-ret=get_user_pages(vaddr,1,flags|FOLL_LONGTERM,page,-vmas);-}else{-ret=get_user_pages_remote(NULL,mm,vaddr,1,flags,page,-vmas,NULL);-/*-*Thelifetimeofavaddr_get_pfn()pagepinis-*userspace-controlled.Inthefs-daxcasethiscould-*leadtoindefinitestallsinfilesystemoperations.-*Disallowattemptstopinfs-daxpagesviathis-*interface.-*/-if(ret>0&&vma_is_fsdax(vmas[0])){-ret=-EOPNOTSUPP;-put_page(page[0]);-}+ret=get_user_pages_remote(NULL,mm,vaddr,1,flags|FOLL_LONGTERM,+page,vmas,NULL);+/*+*Thelifetimeofavaddr_get_pfn()pagepinis+*userspace-controlled.Inthefs-daxcasethiscould+*leadtoindefinitestallsinfilesystemoperations.+*Disallowattemptstopinfs-daxpagesviathis+*interface.+*/+if(ret>0&&vma_is_fsdax(vmas[0])){+ret=-EOPNOTSUPP;+put_page(page[0]);}
AFAIK this chunk is redundant now as it is some hack to emulate
FOLL_LONGTERM? So vmas can be deleted too.
Also unclear why this function has this:
up_read(&mm->mmap_sem);
if (ret == 1) {
*pfn = page_to_pfn(page[0]);
return 0;
}
down_read(&mm->mmap_sem);
Jason
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2019-11-12 20:45:03
On Mon, Nov 11, 2019 at 04:06:48PM -0800, John Hubbard wrote:
quoted hunk
@@ -542,7 +541,7 @@ static int ib_umem_odp_map_dma_single_page( } out:- put_user_page(page);+ put_page(page); if (remove_existing_mapping) { ib_umem_notifier_start_account(umem_odp);
@@ -639,13 +638,14 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, /* * Note: this might result in redundent page getting. We can * avoid this by checking dma_list to be 0 before calling- * get_user_pages. However, this make the code much more- * complex (and doesn't gain us much performance in most use- * cases).+ * get_user_pages. However, this makes the code much+ * more complex (and doesn't gain us much performance in most+ * use cases). */ npages = get_user_pages_remote(owning_process, owning_mm,- user_virt, gup_num_pages,- flags, local_page_list, NULL, NULL);+ user_virt, gup_num_pages,+ flags, local_page_list, NULL,+ NULL); up_read(&owning_mm->mmap_sem);
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 21:11:00
On 11/12/19 12:38 PM, Jason Gunthorpe wrote:
On Mon, Nov 11, 2019 at 04:06:37PM -0800, John Hubbard wrote:
quoted
Hi,
The cover letter is long, so the more important stuff is first:
* Jason, if you or someone could look at the the VFIO cleanup (patch 8)
and conversion to FOLL_PIN (patch 18), to make sure it's use of
remote and longterm gup matches what we discussed during the review
of v2, I'd appreciate it.
* Also for Jason and IB: as noted below, in patch 11, I am (too?) boldly
converting from put_user_pages() to release_pages().
Why are we doing this? I think things got confused here someplace, as
Because:
a) These need put_page() calls, and
b) there is no put_pages() call, but there is a release_pages() call that
is, arguably, what put_pages() would be.
the comment still says:
/**
* put_user_page() - release a gup-pinned page
* @page: pointer to page to be released
*
* Pages that were pinned via get_user_pages*() must be released via
* either put_user_page(), or one of the put_user_pages*() routines
* below.
Ohhh, I missed those comments. They need to all be changed over to
say "pages that were pinned via pin_user_pages*() or
pin_longterm_pages*() must be released via put_user_page*()."
The get_user_pages*() pages must still be released via put_page.
The churn is due to a fairly significant change in strategy, whis
is: instead of changing all get_user_pages*() sites to call
put_user_page(), change selected sites to call pin_user_pages*() or
pin_longterm_pages*(), plus put_user_page().
That allows incrementally converting the kernel over to using the
new pin APIs, without taking on the huge risk of a big one-shot
conversion.
So, I've ended up with one place that actually needs to get reverted
back to get_user_pages(), and that's the IB ODP code.
I feel like if put_user_pages() is not the correct way to undo
get_user_pages() then it needs to be deleted.
Yes, you're right. I'll fix the put_user_page comments() as described.
thanks,
John Hubbard
NVIDIA
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 21:15:11
On 11/12/19 12:44 PM, Jason Gunthorpe wrote:
On Mon, Nov 11, 2019 at 04:06:48PM -0800, John Hubbard wrote:
quoted
@@ -542,7 +541,7 @@ static int ib_umem_odp_map_dma_single_page( } out:- put_user_page(page);+ put_page(page); if (remove_existing_mapping) { ib_umem_notifier_start_account(umem_odp);
@@ -639,13 +638,14 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, /* * Note: this might result in redundent page getting. We can * avoid this by checking dma_list to be 0 before calling- * get_user_pages. However, this make the code much more- * complex (and doesn't gain us much performance in most use- * cases).+ * get_user_pages. However, this makes the code much+ * more complex (and doesn't gain us much performance in most+ * use cases). */ npages = get_user_pages_remote(owning_process, owning_mm,- user_virt, gup_num_pages,- flags, local_page_list, NULL, NULL);+ user_virt, gup_num_pages,+ flags, local_page_list, NULL,+ NULL); up_read(&owning_mm->mmap_sem);
This is just whitespace churn? Drop it..
Whoops, yes. It got there because of going through the pin*() conversion
and then a revert, and now it's just whitespace. I'll drop it, thanks for
catching that.
thanks,
John Hubbard
NVIDIA
From: Dan Williams <hidden> Date: 2019-11-12 21:57:46
On Mon, Nov 11, 2019 at 4:07 PM John Hubbard [off-list ref] wrote:
quoted hunk
As it says in the updated comment in gup.c: current FOLL_LONGTERM
behavior is incompatible with FAULT_FLAG_ALLOW_RETRY because of the
FS DAX check requirement on vmas.
However, the corresponding restriction in get_user_pages_remote() was
slightly stricter than is actually required: it forbade all
FOLL_LONGTERM callers, but we can actually allow FOLL_LONGTERM callers
that do not set the "locked" arg.
Update the code and comments accordingly, and update the VFIO caller
to take advantage of this, fixing a bug as a result: the VFIO caller
is logically a FOLL_LONGTERM user.
Thanks to Jason Gunthorpe for pointing out a clean way to fix this.
Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Jerome Glisse <redacted>
Cc: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++-----------------
mm/gup.c | 13 ++++++++-----
2 files changed, 21 insertions(+), 22 deletions(-)
@@ -348,24 +348,20 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,flags|=FOLL_WRITE;down_read(&mm->mmap_sem);-if(mm==current->mm){-ret=get_user_pages(vaddr,1,flags|FOLL_LONGTERM,page,-vmas);-}else{-ret=get_user_pages_remote(NULL,mm,vaddr,1,flags,page,-vmas,NULL);-/*-*Thelifetimeofavaddr_get_pfn()pagepinis-*userspace-controlled.Inthefs-daxcasethiscould-*leadtoindefinitestallsinfilesystemoperations.-*Disallowattemptstopinfs-daxpagesviathis-*interface.-*/-if(ret>0&&vma_is_fsdax(vmas[0])){-ret=-EOPNOTSUPP;-put_page(page[0]);-}+ret=get_user_pages_remote(NULL,mm,vaddr,1,flags|FOLL_LONGTERM,+page,vmas,NULL);
Hmm, what's the point of passing FOLL_LONGTERM to
get_user_pages_remote() if get_user_pages_remote() is not going to
check the vma? I think we got to this code state because the
get_user_pages() vs get_user_pages_remote() split predated the
introduction of FOLL_LONGTERM.
I think check_vma_flags() should do the ((FOLL_LONGTERM | FOLL_GET) &&
vma_is_fsdax()) check and that would also remove the need for
__gup_longterm_locked.
@@ -348,24 +348,20 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,flags|=FOLL_WRITE;down_read(&mm->mmap_sem);-if(mm==current->mm){-ret=get_user_pages(vaddr,1,flags|FOLL_LONGTERM,page,-vmas);-}else{-ret=get_user_pages_remote(NULL,mm,vaddr,1,flags,page,-vmas,NULL);-/*-*Thelifetimeofavaddr_get_pfn()pagepinis-*userspace-controlled.Inthefs-daxcasethiscould-*leadtoindefinitestallsinfilesystemoperations.-*Disallowattemptstopinfs-daxpagesviathis-*interface.-*/-if(ret>0&&vma_is_fsdax(vmas[0])){-ret=-EOPNOTSUPP;-put_page(page[0]);-}+ret=get_user_pages_remote(NULL,mm,vaddr,1,flags|FOLL_LONGTERM,+page,vmas,NULL);
Hmm, what's the point of passing FOLL_LONGTERM to
get_user_pages_remote() if get_user_pages_remote() is not going to
check the vma? I think we got to this code state because the
FOLL_LONGTERM is short-lived in this location, because patch 23
("mm/gup: remove support for gup(FOLL_LONGTERM)") removes it, after
callers are changed over to pin_longterm_pages*().
So FOLL_LONGTERM is not doing much now, but it is basically a marker for
"change gup(FOLL_LONGTERM) to pin_longterm_pages()", and patch 18
actually makes that change.
And then pin_longterm_pages*() is, in turn, a way to mark all the
places that need file system and/or user space interactions (layout
leases, etc), as per "Case 2: RDMA" in the new
Documentation/vm/pin_user_pages.rst.
get_user_pages() vs get_user_pages_remote() split predated the
introduction of FOLL_LONGTERM.
Yes. And I do want clean this up as I go, so we don't end up with
stale concepts lingering in gup.c...
I think check_vma_flags() should do the ((FOLL_LONGTERM | FOLL_GET) &&
vma_is_fsdax()) check and that would also remove the need for
__gup_longterm_locked.
Good idea, but there is still the call to check_and_migrate_cma_pages(),
inside __gup_longterm_locked(). So it's a little more involved and
we can't trivially delete __gup_longterm_locked() yet, right?
thanks,
--
John Hubbard
NVIDIA
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 22:43:11
On 11/12/19 12:43 PM, Jason Gunthorpe wrote:
...
quoted
- }
+ ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags | FOLL_LONGTERM,
+ page, vmas, NULL);
+ /*
+ * The lifetime of a vaddr_get_pfn() page pin is
+ * userspace-controlled. In the fs-dax case this could
+ * lead to indefinite stalls in filesystem operations.
+ * Disallow attempts to pin fs-dax pages via this
+ * interface.
+ */
+ if (ret > 0 && vma_is_fsdax(vmas[0])) {
+ ret = -EOPNOTSUPP;
+ put_page(page[0]);
}
AFAIK this chunk is redundant now as it is some hack to emulate
FOLL_LONGTERM? So vmas can be deleted too.
Let me first make sure I understand what Dan has in mind for the vma
checking, in the other thread...
Also unclear why this function has this:
up_read(&mm->mmap_sem);
if (ret == 1) {
*pfn = page_to_pfn(page[0]);
return 0;
}
down_read(&mm->mmap_sem);
Yes, that's really odd. It's not good to release and retake the lock
anyway in general (without re-checking things), and certainly it is
not required to release mmap_sem in order to call page_to_pfn().
I've removed that up_read()/down_read() pair, for v4.
thanks,
--
John Hubbard
NVIDIA
@@ -348,24 +348,20 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,flags|=FOLL_WRITE;down_read(&mm->mmap_sem);-if(mm==current->mm){-ret=get_user_pages(vaddr,1,flags|FOLL_LONGTERM,page,-vmas);-}else{-ret=get_user_pages_remote(NULL,mm,vaddr,1,flags,page,-vmas,NULL);-/*-*Thelifetimeofavaddr_get_pfn()pagepinis-*userspace-controlled.Inthefs-daxcasethiscould-*leadtoindefinitestallsinfilesystemoperations.-*Disallowattemptstopinfs-daxpagesviathis-*interface.-*/-if(ret>0&&vma_is_fsdax(vmas[0])){-ret=-EOPNOTSUPP;-put_page(page[0]);-}+ret=get_user_pages_remote(NULL,mm,vaddr,1,flags|FOLL_LONGTERM,+page,vmas,NULL);
Hmm, what's the point of passing FOLL_LONGTERM to
get_user_pages_remote() if get_user_pages_remote() is not going to
check the vma? I think we got to this code state because the
FOLL_LONGTERM is short-lived in this location, because patch 23
("mm/gup: remove support for gup(FOLL_LONGTERM)") removes it, after
callers are changed over to pin_longterm_pages*().
So FOLL_LONGTERM is not doing much now, but it is basically a marker for
"change gup(FOLL_LONGTERM) to pin_longterm_pages()", and patch 18
actually makes that change.
And then pin_longterm_pages*() is, in turn, a way to mark all the
places that need file system and/or user space interactions (layout
leases, etc), as per "Case 2: RDMA" in the new
Documentation/vm/pin_user_pages.rst.
Ah, sorry. This was the first time I had looked at this series and
jumped in without reading the background.
Your patch as is looks ok, I assume you've removed the FOLL_LONGTERM
warning in get_user_pages_remote in another patch?
quoted
get_user_pages() vs get_user_pages_remote() split predated the
introduction of FOLL_LONGTERM.
Yes. And I do want clean this up as I go, so we don't end up with
stale concepts lingering in gup.c...
quoted
I think check_vma_flags() should do the ((FOLL_LONGTERM | FOLL_GET) &&
vma_is_fsdax()) check and that would also remove the need for
__gup_longterm_locked.
Good idea, but there is still the call to check_and_migrate_cma_pages(),
inside __gup_longterm_locked(). So it's a little more involved and
we can't trivially delete __gup_longterm_locked() yet, right?
[ add Aneesh ]
Yes, you're right. I had overlooked that had snuck in there. That to
me similarly needs to be pushed down into the core with its own FOLL
flag, or it needs to be an explicit fixup that each caller does after
get_user_pages. The fact that migration silently happens as a side
effect of gup is too magical for my taste.
From: Dan Williams <hidden> Date: 2019-11-12 22:46:11
On Tue, Nov 12, 2019 at 2:43 PM John Hubbard [off-list ref] wrote:
On 11/12/19 12:43 PM, Jason Gunthorpe wrote:
...
quoted
quoted
- }
+ ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags | FOLL_LONGTERM,
+ page, vmas, NULL);
+ /*
+ * The lifetime of a vaddr_get_pfn() page pin is
+ * userspace-controlled. In the fs-dax case this could
+ * lead to indefinite stalls in filesystem operations.
+ * Disallow attempts to pin fs-dax pages via this
+ * interface.
+ */
+ if (ret > 0 && vma_is_fsdax(vmas[0])) {
+ ret = -EOPNOTSUPP;
+ put_page(page[0]);
}
AFAIK this chunk is redundant now as it is some hack to emulate
FOLL_LONGTERM? So vmas can be deleted too.
Let me first make sure I understand what Dan has in mind for the vma
checking, in the other thread...
It's not redundant relative to upstream which does not do anything the
FOLL_LONGTERM in the gup-slow path... but I have not looked at patches
1-7 to see if something there made it redundant.
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 23:08:20
On 11/12/19 2:43 PM, Dan Williams wrote:
...
Ah, sorry. This was the first time I had looked at this series and
jumped in without reading the background.
Your patch as is looks ok, I assume you've removed the FOLL_LONGTERM
warning in get_user_pages_remote in another patch?
Actually, I haven't gone quite that far. Actually this patch is the last
change to that function. Therefore, at the end of this patchset,
get_user_pages_remote() ends up with this check in it which
is a less-restrictive version of the warning:
/*
* Current FOLL_LONGTERM behavior is incompatible with
* FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on
* vmas. However, this only comes up if locked is set, and there are
* callers that do request FOLL_LONGTERM, but do not set locked. So,
* allow what we can.
*/
if (gup_flags & FOLL_LONGTERM) {
if (WARN_ON_ONCE(locked))
return -EINVAL;
}
Is that OK, or did you want to go further (possibly in a follow-up
patchset, as I'm hoping to get this one in soon)?
...
quoted
quoted
I think check_vma_flags() should do the ((FOLL_LONGTERM | FOLL_GET) &&
vma_is_fsdax()) check and that would also remove the need for
__gup_longterm_locked.
Good idea, but there is still the call to check_and_migrate_cma_pages(),
inside __gup_longterm_locked(). So it's a little more involved and
we can't trivially delete __gup_longterm_locked() yet, right?
[ add Aneesh ]
Yes, you're right. I had overlooked that had snuck in there. That to
me similarly needs to be pushed down into the core with its own FOLL
flag, or it needs to be an explicit fixup that each caller does after
get_user_pages. The fact that migration silently happens as a side
effect of gup is too magical for my taste.
Yes. It's an intrusive side effect that is surprising, and not in a
"happy surprise" way. :) . Fixing up the CMA pages by splitting that
functionality into separate function calls sounds like an improvement
worth exploring.
thanks,
--
John Hubbard
NVIDIA
From: Dan Williams <hidden> Date: 2019-11-12 23:15:14
On Tue, Nov 12, 2019 at 3:08 PM John Hubbard [off-list ref] wrote:
On 11/12/19 2:43 PM, Dan Williams wrote:
...
quoted
Ah, sorry. This was the first time I had looked at this series and
jumped in without reading the background.
Your patch as is looks ok, I assume you've removed the FOLL_LONGTERM
warning in get_user_pages_remote in another patch?
Actually, I haven't gone quite that far. Actually this patch is the last
change to that function. Therefore, at the end of this patchset,
get_user_pages_remote() ends up with this check in it which
is a less-restrictive version of the warning:
/*
* Current FOLL_LONGTERM behavior is incompatible with
* FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on
* vmas. However, this only comes up if locked is set, and there are
* callers that do request FOLL_LONGTERM, but do not set locked. So,
* allow what we can.
*/
if (gup_flags & FOLL_LONGTERM) {
if (WARN_ON_ONCE(locked))
return -EINVAL;
}
Is that OK, or did you want to go further (possibly in a follow-up
patchset, as I'm hoping to get this one in soon)?
That looks ok. Something to maybe push down into the core in a future
cleanup, but not something that needs to be done now.
...
quoted
quoted
quoted
I think check_vma_flags() should do the ((FOLL_LONGTERM | FOLL_GET) &&
vma_is_fsdax()) check and that would also remove the need for
__gup_longterm_locked.
Good idea, but there is still the call to check_and_migrate_cma_pages(),
inside __gup_longterm_locked(). So it's a little more involved and
we can't trivially delete __gup_longterm_locked() yet, right?
[ add Aneesh ]
Yes, you're right. I had overlooked that had snuck in there. That to
me similarly needs to be pushed down into the core with its own FOLL
flag, or it needs to be an explicit fixup that each caller does after
get_user_pages. The fact that migration silently happens as a side
effect of gup is too magical for my taste.
Yes. It's an intrusive side effect that is surprising, and not in a
"happy surprise" way. :) . Fixing up the CMA pages by splitting that
functionality into separate function calls sounds like an improvement
worth exploring.
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 23:17:43
On 11/12/19 2:45 PM, Dan Williams wrote:
On Tue, Nov 12, 2019 at 2:43 PM John Hubbard [off-list ref] wrote:
quoted
On 11/12/19 12:43 PM, Jason Gunthorpe wrote:
...
quoted
quoted
- }
+ ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags | FOLL_LONGTERM,
+ page, vmas, NULL);
+ /*
+ * The lifetime of a vaddr_get_pfn() page pin is
+ * userspace-controlled. In the fs-dax case this could
+ * lead to indefinite stalls in filesystem operations.
+ * Disallow attempts to pin fs-dax pages via this
+ * interface.
+ */
+ if (ret > 0 && vma_is_fsdax(vmas[0])) {
+ ret = -EOPNOTSUPP;
+ put_page(page[0]);
}
AFAIK this chunk is redundant now as it is some hack to emulate
FOLL_LONGTERM? So vmas can be deleted too.
Let me first make sure I understand what Dan has in mind for the vma
checking, in the other thread...
It's not redundant relative to upstream which does not do anything the
FOLL_LONGTERM in the gup-slow path... but I have not looked at patches
1-7 to see if something there made it redundant.
There is nothing in patches 1-7 that would make it redundant.
About the only thing that you might find interesting in that subset is
patch 4 ("mm: devmap: refactor 1-based refcounting for ZONE_DEVICE pages"),
for devmap and ZONE_DEVICE interest. But it doesn't affect this
discussion directly.
thanks,
--
John Hubbard
NVIDIA
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-12 23:29:18
On 11/12/19 3:14 PM, Dan Williams wrote:
...
quoted
Is that OK, or did you want to go further (possibly in a follow-up
patchset, as I'm hoping to get this one in soon)?
That looks ok. Something to maybe push down into the core in a future
Great! I'll post a cleaned up v4 (with the extraneous up_read()/down_read()
removed), then.
cleanup, but not something that needs to be done now.
Yes. I've put the FOLL_LONGTERM cleanup items on my list now, in case
they don't get done as part of something else. There's a lot more
change coming in this area.
thanks,
--
John Hubbard
NVIDIA
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2019-11-12 23:42:57
On Tue, Nov 12, 2019 at 02:45:51PM -0800, Dan Williams wrote:
On Tue, Nov 12, 2019 at 2:43 PM John Hubbard [off-list ref] wrote:
quoted
On 11/12/19 12:43 PM, Jason Gunthorpe wrote:
...
quoted
quoted
- }
+ ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags | FOLL_LONGTERM,
+ page, vmas, NULL);
+ /*
+ * The lifetime of a vaddr_get_pfn() page pin is
+ * userspace-controlled. In the fs-dax case this could
+ * lead to indefinite stalls in filesystem operations.
+ * Disallow attempts to pin fs-dax pages via this
+ * interface.
+ */
+ if (ret > 0 && vma_is_fsdax(vmas[0])) {
+ ret = -EOPNOTSUPP;
+ put_page(page[0]);
}
AFAIK this chunk is redundant now as it is some hack to emulate
FOLL_LONGTERM? So vmas can be deleted too.
Let me first make sure I understand what Dan has in mind for the vma
checking, in the other thread...
It's not redundant relative to upstream which does not do anything the
FOLL_LONGTERM in the gup-slow path... but I have not looked at patches
1-7 to see if something there made it redundant.
Oh, the hunk John had below for get_user_pages_remote() also needs to
call __gup_longterm_locked() when FOLL_LONGTERM is specified, then
that calls check_dax_vmas() which duplicates the vma_is_fsdax() check
above.
Certainly no caller of FOLL_LONGTERM should have to do dax specific
VMA checking.
Jason
From: Dan Williams <hidden> Date: 2019-11-13 00:59:23
On Tue, Nov 12, 2019 at 3:43 PM Jason Gunthorpe [off-list ref] wrote:
On Tue, Nov 12, 2019 at 02:45:51PM -0800, Dan Williams wrote:
quoted
On Tue, Nov 12, 2019 at 2:43 PM John Hubbard [off-list ref] wrote:
quoted
On 11/12/19 12:43 PM, Jason Gunthorpe wrote:
...
quoted
quoted
- }
+ ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags | FOLL_LONGTERM,
+ page, vmas, NULL);
+ /*
+ * The lifetime of a vaddr_get_pfn() page pin is
+ * userspace-controlled. In the fs-dax case this could
+ * lead to indefinite stalls in filesystem operations.
+ * Disallow attempts to pin fs-dax pages via this
+ * interface.
+ */
+ if (ret > 0 && vma_is_fsdax(vmas[0])) {
+ ret = -EOPNOTSUPP;
+ put_page(page[0]);
}
AFAIK this chunk is redundant now as it is some hack to emulate
FOLL_LONGTERM? So vmas can be deleted too.
Let me first make sure I understand what Dan has in mind for the vma
checking, in the other thread...
It's not redundant relative to upstream which does not do anything the
FOLL_LONGTERM in the gup-slow path... but I have not looked at patches
1-7 to see if something there made it redundant.
Oh, the hunk John had below for get_user_pages_remote() also needs to
call __gup_longterm_locked() when FOLL_LONGTERM is specified, then
that calls check_dax_vmas() which duplicates the vma_is_fsdax() check
above.
Oh true, good eye. It is redundant if it does additionally call
__gup_longterm_locked(), and it needs to do that otherwises it undoes
the CMA migration magic that Aneesh added.
Certainly no caller of FOLL_LONGTERM should have to do dax specific
VMA checking.
Agree, that was my comment about cleaning up the vma_is_fsdax() check
to be internal to the gup core.
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-13 01:08:21
On 11/12/19 4:58 PM, Dan Williams wrote:
...
quoted
quoted
It's not redundant relative to upstream which does not do anything the
FOLL_LONGTERM in the gup-slow path... but I have not looked at patches
1-7 to see if something there made it redundant.
Oh, the hunk John had below for get_user_pages_remote() also needs to
call __gup_longterm_locked() when FOLL_LONGTERM is specified, then
that calls check_dax_vmas() which duplicates the vma_is_fsdax() check
above.
Oh true, good eye. It is redundant if it does additionally call
__gup_longterm_locked(), and it needs to do that otherwises it undoes
the CMA migration magic that Aneesh added.
OK. So just to be clear, I'll be removing this from the patch:
/*
* The lifetime of a vaddr_get_pfn() page pin is
* userspace-controlled. In the fs-dax case this could
* lead to indefinite stalls in filesystem operations.
* Disallow attempts to pin fs-dax pages via this
* interface.
*/
if (ret > 0 && vma_is_fsdax(vmas[0])) {
ret = -EOPNOTSUPP;
put_page(page[0]);
}
(and the declaration of "vmas", as well).
thanks,
--
John Hubbard
NVIDIA
From: Dan Williams <hidden> Date: 2019-11-13 01:36:04
On Tue, Nov 12, 2019 at 5:08 PM John Hubbard [off-list ref] wrote:
On 11/12/19 4:58 PM, Dan Williams wrote:
...
quoted
quoted
quoted
It's not redundant relative to upstream which does not do anything the
FOLL_LONGTERM in the gup-slow path... but I have not looked at patches
1-7 to see if something there made it redundant.
Oh, the hunk John had below for get_user_pages_remote() also needs to
call __gup_longterm_locked() when FOLL_LONGTERM is specified, then
that calls check_dax_vmas() which duplicates the vma_is_fsdax() check
above.
Oh true, good eye. It is redundant if it does additionally call
__gup_longterm_locked(), and it needs to do that otherwises it undoes
the CMA migration magic that Aneesh added.
OK. So just to be clear, I'll be removing this from the patch:
/*
* The lifetime of a vaddr_get_pfn() page pin is
* userspace-controlled. In the fs-dax case this could
* lead to indefinite stalls in filesystem operations.
* Disallow attempts to pin fs-dax pages via this
* interface.
*/
if (ret > 0 && vma_is_fsdax(vmas[0])) {
ret = -EOPNOTSUPP;
put_page(page[0]);
}
(and the declaration of "vmas", as well).
...and add a call to __gup_longterm_locked internal to
get_user_pages_remote(), right?
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-13 02:09:14
On 11/12/19 5:35 PM, Dan Williams wrote:
On Tue, Nov 12, 2019 at 5:08 PM John Hubbard [off-list ref] wrote:
quoted
On 11/12/19 4:58 PM, Dan Williams wrote:
...
quoted
quoted
quoted
It's not redundant relative to upstream which does not do anything the
FOLL_LONGTERM in the gup-slow path... but I have not looked at patches
1-7 to see if something there made it redundant.
Oh, the hunk John had below for get_user_pages_remote() also needs to
call __gup_longterm_locked() when FOLL_LONGTERM is specified, then
that calls check_dax_vmas() which duplicates the vma_is_fsdax() check
above.
Oh true, good eye. It is redundant if it does additionally call
__gup_longterm_locked(), and it needs to do that otherwises it undoes
the CMA migration magic that Aneesh added.
OK. So just to be clear, I'll be removing this from the patch:
/*
* The lifetime of a vaddr_get_pfn() page pin is
* userspace-controlled. In the fs-dax case this could
* lead to indefinite stalls in filesystem operations.
* Disallow attempts to pin fs-dax pages via this
* interface.
*/
if (ret > 0 && vma_is_fsdax(vmas[0])) {
ret = -EOPNOTSUPP;
put_page(page[0]);
}
(and the declaration of "vmas", as well).
...and add a call to __gup_longterm_locked internal to
get_user_pages_remote(), right?
Yes, and thanks for double-checking. I think I got a little dizzy following
the call stack there. :) And now I see that this also affects the
implementation of pin_longterm_pages_remote(), because that will need the
same logic that get_user_pages_remote() has.
thanks,
--
John Hubbard
NVIDIA
From: Daniel Vetter <hidden> Date: 2019-11-13 08:22:56
On Tue, Nov 12, 2019 at 10:10 PM John Hubbard [off-list ref] wrote:
On 11/12/19 12:38 PM, Jason Gunthorpe wrote:
quoted
On Mon, Nov 11, 2019 at 04:06:37PM -0800, John Hubbard wrote:
quoted
Hi,
The cover letter is long, so the more important stuff is first:
* Jason, if you or someone could look at the the VFIO cleanup (patch 8)
and conversion to FOLL_PIN (patch 18), to make sure it's use of
remote and longterm gup matches what we discussed during the review
of v2, I'd appreciate it.
* Also for Jason and IB: as noted below, in patch 11, I am (too?) boldly
converting from put_user_pages() to release_pages().
Why are we doing this? I think things got confused here someplace, as
Because:
a) These need put_page() calls, and
b) there is no put_pages() call, but there is a release_pages() call that
is, arguably, what put_pages() would be.
quoted
the comment still says:
/**
* put_user_page() - release a gup-pinned page
* @page: pointer to page to be released
*
* Pages that were pinned via get_user_pages*() must be released via
* either put_user_page(), or one of the put_user_pages*() routines
* below.
Ohhh, I missed those comments. They need to all be changed over to
say "pages that were pinned via pin_user_pages*() or
pin_longterm_pages*() must be released via put_user_page*()."
The get_user_pages*() pages must still be released via put_page.
The churn is due to a fairly significant change in strategy, whis
is: instead of changing all get_user_pages*() sites to call
put_user_page(), change selected sites to call pin_user_pages*() or
pin_longterm_pages*(), plus put_user_page().
Can't we call this unpin_user_page then, for some symmetry? Or is that
even more churn?
Looking from afar the naming here seems really confusing.
-Daniel
That allows incrementally converting the kernel over to using the
new pin APIs, without taking on the huge risk of a big one-shot
conversion.
So, I've ended up with one place that actually needs to get reverted
back to get_user_pages(), and that's the IB ODP code.
quoted
I feel like if put_user_pages() is not the correct way to undo
get_user_pages() then it needs to be deleted.
Yes, you're right. I'll fix the put_user_page comments() as described.
thanks,
John Hubbard
NVIDIA
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-13 09:04:59
On 11/13/19 12:22 AM, Daniel Vetter wrote:
...
quoted
quoted
Why are we doing this? I think things got confused here someplace, as
Because:
a) These need put_page() calls, and
b) there is no put_pages() call, but there is a release_pages() call that
is, arguably, what put_pages() would be.
quoted
the comment still says:
/**
* put_user_page() - release a gup-pinned page
* @page: pointer to page to be released
*
* Pages that were pinned via get_user_pages*() must be released via
* either put_user_page(), or one of the put_user_pages*() routines
* below.
Ohhh, I missed those comments. They need to all be changed over to
say "pages that were pinned via pin_user_pages*() or
pin_longterm_pages*() must be released via put_user_page*()."
The get_user_pages*() pages must still be released via put_page.
The churn is due to a fairly significant change in strategy, whis
is: instead of changing all get_user_pages*() sites to call
put_user_page(), change selected sites to call pin_user_pages*() or
pin_longterm_pages*(), plus put_user_page().
Can't we call this unpin_user_page then, for some symmetry? Or is that
even more churn?
Looking from afar the naming here seems really confusing.
That look from afar is valuable, because I'm too close to the problem to see
how the naming looks. :)
unpin_user_page() sounds symmetrical. It's true that it would cause more
churn (which is why I started off with a proposal that avoids changing the
names of put_user_page*() APIs). But OTOH, the amount of churn is proportional
to the change in direction here, and it's really only 10 or 20 lines changed,
in the end.
So I'm open to changing to that naming. It would be nice to hear what others
prefer, too...
thanks,
--
John Hubbard
NVIDIA
From: Jan Kara <jack@suse.cz> Date: 2019-11-13 10:12:21
On Wed 13-11-19 01:02:02, John Hubbard wrote:
On 11/13/19 12:22 AM, Daniel Vetter wrote:
...
quoted
quoted
quoted
Why are we doing this? I think things got confused here someplace, as
Because:
a) These need put_page() calls, and
b) there is no put_pages() call, but there is a release_pages() call that
is, arguably, what put_pages() would be.
quoted
the comment still says:
/**
* put_user_page() - release a gup-pinned page
* @page: pointer to page to be released
*
* Pages that were pinned via get_user_pages*() must be released via
* either put_user_page(), or one of the put_user_pages*() routines
* below.
Ohhh, I missed those comments. They need to all be changed over to
say "pages that were pinned via pin_user_pages*() or
pin_longterm_pages*() must be released via put_user_page*()."
The get_user_pages*() pages must still be released via put_page.
The churn is due to a fairly significant change in strategy, whis
is: instead of changing all get_user_pages*() sites to call
put_user_page(), change selected sites to call pin_user_pages*() or
pin_longterm_pages*(), plus put_user_page().
Can't we call this unpin_user_page then, for some symmetry? Or is that
even more churn?
Looking from afar the naming here seems really confusing.
That look from afar is valuable, because I'm too close to the problem to see
how the naming looks. :)
unpin_user_page() sounds symmetrical. It's true that it would cause more
churn (which is why I started off with a proposal that avoids changing the
names of put_user_page*() APIs). But OTOH, the amount of churn is proportional
to the change in direction here, and it's really only 10 or 20 lines changed,
in the end.
So I'm open to changing to that naming. It would be nice to hear what others
prefer, too...
FWIW I'd find unpin_user_page() also better than put_user_page() as a
counterpart to pin_user_pages().
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Daniel Vetter <hidden> Date: 2019-11-13 11:43:23
On Wed, Nov 13, 2019 at 11:12:10AM +0100, Jan Kara wrote:
On Wed 13-11-19 01:02:02, John Hubbard wrote:
quoted
On 11/13/19 12:22 AM, Daniel Vetter wrote:
...
quoted
quoted
quoted
Why are we doing this? I think things got confused here someplace, as
Because:
a) These need put_page() calls, and
b) there is no put_pages() call, but there is a release_pages() call that
is, arguably, what put_pages() would be.
quoted
the comment still says:
/**
* put_user_page() - release a gup-pinned page
* @page: pointer to page to be released
*
* Pages that were pinned via get_user_pages*() must be released via
* either put_user_page(), or one of the put_user_pages*() routines
* below.
Ohhh, I missed those comments. They need to all be changed over to
say "pages that were pinned via pin_user_pages*() or
pin_longterm_pages*() must be released via put_user_page*()."
The get_user_pages*() pages must still be released via put_page.
The churn is due to a fairly significant change in strategy, whis
is: instead of changing all get_user_pages*() sites to call
put_user_page(), change selected sites to call pin_user_pages*() or
pin_longterm_pages*(), plus put_user_page().
Can't we call this unpin_user_page then, for some symmetry? Or is that
even more churn?
Looking from afar the naming here seems really confusing.
That look from afar is valuable, because I'm too close to the problem to see
how the naming looks. :)
unpin_user_page() sounds symmetrical. It's true that it would cause more
churn (which is why I started off with a proposal that avoids changing the
names of put_user_page*() APIs). But OTOH, the amount of churn is proportional
to the change in direction here, and it's really only 10 or 20 lines changed,
in the end.
So I'm open to changing to that naming. It would be nice to hear what others
prefer, too...
FWIW I'd find unpin_user_page() also better than put_user_page() as a
counterpart to pin_user_pages().
One more point from afar on pin/unpin: We use that a lot in graphics for
permanently pinned graphics buffer objects. Which really only should be
used for scanout. So at least graphics folks should have an appropriate
mindset and try to make sure we don't overuse this stuff.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-13 20:31:06
On 11/13/19 3:43 AM, Daniel Vetter wrote:
...
quoted
quoted
quoted
Can't we call this unpin_user_page then, for some symmetry? Or is that
even more churn?
Looking from afar the naming here seems really confusing.
That look from afar is valuable, because I'm too close to the problem to see
how the naming looks. :)
unpin_user_page() sounds symmetrical. It's true that it would cause more
churn (which is why I started off with a proposal that avoids changing the
names of put_user_page*() APIs). But OTOH, the amount of churn is proportional
to the change in direction here, and it's really only 10 or 20 lines changed,
in the end.
So I'm open to changing to that naming. It would be nice to hear what others
prefer, too...
FWIW I'd find unpin_user_page() also better than put_user_page() as a
counterpart to pin_user_pages().
One more point from afar on pin/unpin: We use that a lot in graphics for
permanently pinned graphics buffer objects. Which really only should be
used for scanout. So at least graphics folks should have an appropriate
mindset and try to make sure we don't overuse this stuff.
-Daniel
OK, Ira also likes "unpin", and so far no one has said *anything* in favor
of the "put_user_page" names, so I think we have a winner! I'll change the
names to unpin_user_page*().
thanks,
--
John Hubbard
NVIDIA