From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:16:23
Hi,
OK, here is v7, maybe this is the last one. The corresponding git repo
and branch is:
git@github.com:johnhubbard/linux.git pin_user_pages_tracking_v7
Ira, you reviewed the gup_benchmark patches a bit earlier, but I
removed one or two of those review-by tags, due to invasive changes
I made after your review (in response to further reviews).
So could you please reply to any patches you'd like to have
reviewed-by's restoredto, if any? Mainly I'm thinking of
"mm/gup_benchmark: support pin_user_pages() and related calls". Also
various FOLL_LONGTERM vs pin_longterm*() patches.
The following blurb from the v6 cover letter is still applicable, and
I'll repeat it here so it doesn't get lost in the patch blizzard:
Christoph Hellwig has a preference to do things a little differently,
for the devmap cleanup in patch 5 ("mm: devmap: refactor 1-based
refcounting for ZONE_DEVICE pages"). That came up in a different
review thread, because the patch is out for review in two locations.
Here's that review thread:
https://lore.kernel.org/r/20191118070826.GB3099@infradead.org
...and I'm hoping that we can defer that request, because otherwise
it derails this series, which is starting to otherwise look like
it could be ready for 5.5.
Changes since v6:
* Renamed a couple of routines, to get rid of unnecessary leading
underscores:
__pin_compound_head() --> grab_compound_head()
__record_subpages() --> record_subpages()
* Fixed the error fallback (put_compound_head()) so as to match the fix
in the previous version: need to put back N * GUP_PIN_COUNTING_BIAS
pages, for FOLL_PIN cases.
* Factored out yet another common chunk of code, into a new grab_page()
routine.
* Added a missing compound_head() call to put_compound_head().
* [Re-]added Jens Axboe's reviewed-by tag to the fs/io_uring patch.
* Added more reviewed-by's from Jan Kara.
Changes since v5:
* Fixed the refcounting for huge pages: in most cases, it was
only taking one GUP_PIN_COUNTING_BIAS's worth of refs, when it
should have been taking one GUP_PIN_COUNTING_BIAS for each subpage.
(Much thanks to Jan Kara for spotting that one!)
* Renamed user_page_ref_inc() to try_pin_page(), and added a new
try_pin_compound_head(). This definitely improves readability.
* Factored out some more duplication in the FOLL_PIN and FOLL_GET
cases, in gup.c.
* Fixed up some straggling "get_" --> "pin_" references in the comments.
* Added reviewed-by tags.
Changes since v4:
* Renamed put_user_page*() --> unpin_user_page().
* Removed all pin_longterm_pages*() calls. We will use FOLL_LONGTERM
at the call sites. (FOLL_PIN, however, remains an internal gup flag).
This is very nice: many patches just change three characters now:
get_user_pages --> pin_user_pages. I think we've found the right
balance of wrapper calls and gup flags, for the call sites.
* Updated a lot of documentation and commit logs to match the above
two large changes.
* Changed gup_benchmark tests and run_vmtests, to adapt to one less
use case: there is no pin_longterm_pages() call anymore.
* This includes a new devmap cleanup patch from Dan Williams, along
with a rebased follow-up: patches 4 and 5, already mentioned above.
* Fixed patch 10 ("mm/gup: introduce pin_user_pages*() and FOLL_PIN"),
so as to make pin_user_pages*() calls act as placeholders for the
corresponding get_user_pages*() calls, until a later patch fully
implements the DMA-pinning functionality.
Thanks to Jan Kara for noticing that.
* Fixed the implementation of pin_user_pages_remote().
* Further tweaked patch 2 ("mm/gup: factor out duplicate code from four
routines"), in response to Jan Kara's feedback.
* Dropped a few reviewed-by tags due to changes that invalidated
them.
Changes since v3:
* VFIO fix (patch 8): applied further cleanup: removed a pre-existing,
unnecessary release and reacquire of mmap_sem. Moved the DAX vma
checks from the vfio call site, to gup internals, and added comments
(and commit log) to clarify.
* Due to the above, made a corresponding fix to the
pin_longterm_pages_remote(), which was actually calling the wrong
gup internal function.
* Changed put_user_page() comments, to refer to pin*() APIs, rather than
get_user_pages*() APIs.
* Reverted an accidental whitespace-only change in the IB ODP code.
* Added a few more reviewed-by tags.
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-9: refactoring and preparatory cleanup, independent fixes
* Patch 10: introduce pin_user_pages(), FOLL_PIN, but no functional
changes yet
* Patches 11-16: Convert existing put_user_page() callers, to use the
new pin*()
* Patch 17: Activate tracking of FOLL_PIN pages.
* Patches 18-20: convert various callers
* Patches: 21-23: gup_benchmark and run_vmtests support
* Patch 24: rename put_user_page*() --> unpin_user_page*()
============================================================
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/
Dan Williams (1):
mm: Cleanup __put_devmap_managed_page() vs ->page_free()
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 via pin_user_pages*(), fix up ODP
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_user_pages (FOLL_PIN) and put_user_page()
conversion
vfio, mm: pin_user_pages (FOLL_PIN) and put_user_page() conversion
powerpc: book3s64: convert to pin_user_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, tree-wide: rename put_user_page*() to unpin_user_page*()
Documentation/core-api/index.rst | 1 +
Documentation/core-api/pin_user_pages.rst | 233 +++++++++
arch/powerpc/mm/book3s64/iommu_api.c | 12 +-
drivers/gpu/drm/via/via_dmablit.c | 6 +-
drivers/infiniband/core/umem.c | 19 +-
drivers/infiniband/core/umem_odp.c | 13 +-
drivers/infiniband/hw/hfi1/user_pages.c | 4 +-
drivers/infiniband/hw/mthca/mthca_memfree.c | 8 +-
drivers/infiniband/hw/qib/qib_user_pages.c | 4 +-
drivers/infiniband/hw/qib/qib_user_sdma.c | 8 +-
drivers/infiniband/hw/usnic/usnic_uiom.c | 4 +-
drivers/infiniband/sw/siw/siw_mem.c | 4 +-
drivers/media/v4l2-core/videobuf-dma-sg.c | 8 +-
drivers/nvdimm/pmem.c | 6 -
drivers/platform/goldfish/goldfish_pipe.c | 35 +-
drivers/vfio/vfio_iommu_type1.c | 35 +-
fs/io_uring.c | 6 +-
include/linux/mm.h | 195 ++++++-
include/linux/mmzone.h | 2 +
include/linux/page_ref.h | 10 +
mm/gup.c | 553 +++++++++++++++-----
mm/gup_benchmark.c | 74 ++-
mm/huge_memory.c | 44 +-
mm/hugetlb.c | 36 +-
mm/memremap.c | 76 ++-
mm/process_vm_access.c | 28 +-
mm/vmstat.c | 2 +
net/xdp/xdp_umem.c | 4 +-
tools/testing/selftests/vm/gup_benchmark.c | 21 +-
tools/testing/selftests/vm/run_vmtests | 22 +
30 files changed, 1121 insertions(+), 352 deletions(-)
create mode 100644 Documentation/core-api/pin_user_pages.rst
--
2.24.0
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:14:26
1. Change v4l2 from get_user_pages() to pin_user_pages().
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>
Cc: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/media/v4l2-core/videobuf-dma-sg.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:14:38
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: Jan Kara <jack@suse.cz>
Reviewed-by: Jason Gunthorpe <redacted>
Reviewed-by: Ira Weiny <redacted>
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-21 07:14:55
Convert infiniband to use the new pin_user_pages*() calls.
Also, revert earlier changes to Infiniband ODP that had it using
put_user_page(). ODP is "Case 3" in
Documentation/core-api/pin_user_pages.rst, which is to say, normal
get_user_pages() and put_page() is the API to use there.
The new pin_user_pages*() calls replace corresponding get_user_pages*()
calls, and set the 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: Jason Gunthorpe <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/infiniband/core/umem.c | 2 +-
drivers/infiniband/core/umem_odp.c | 13 ++++++-------
drivers/infiniband/hw/hfi1/user_pages.c | 2 +-
drivers/infiniband/hw/mthca/mthca_memfree.c | 2 +-
drivers/infiniband/hw/qib/qib_user_pages.c | 2 +-
drivers/infiniband/hw/qib/qib_user_sdma.c | 2 +-
drivers/infiniband/hw/usnic/usnic_uiom.c | 2 +-
drivers/infiniband/sw/siw/siw_mem.c | 2 +-
8 files changed, 13 insertions(+), 14 deletions(-)
@@ -108,7 +108,7 @@ 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,+ret=pin_user_pages(start_page+got*PAGE_SIZE,num_pages-got,FOLL_LONGTERM|FOLL_WRITE|FOLL_FORCE,p+got,NULL);
@@ -141,7 +141,7 @@ static int usnic_uiom_get_pages(unsigned long addr, size_t size, int writable,ret=0;while(npages){-ret=get_user_pages(cur_base,+ret=pin_user_pages(cur_base,min_t(unsignedlong,npages,PAGE_SIZE/sizeof(structpage*)),gup_flags|FOLL_LONGTERM,
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:15:04
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: Jan Kara <jack@suse.cz>
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-21 07:15:07
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], [2], and [3].
This also changes a BUG_ON(), to a WARN_ON(), in follow_page_mask().
[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/
Suggested-by: Jan Kara <jack@suse.cz>
Suggested-by: Jérôme Glisse <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
Documentation/core-api/pin_user_pages.rst | 2 +-
include/linux/mm.h | 113 +++++++-
include/linux/mmzone.h | 2 +
include/linux/page_ref.h | 10 +
mm/gup.c | 323 ++++++++++++++++------
mm/huge_memory.c | 44 ++-
mm/hugetlb.c | 36 ++-
mm/vmstat.c | 2 +
8 files changed, 421 insertions(+), 111 deletions(-)
@@ -53,7 +53,7 @@ Which flags are set by each wrapper For these pin_user_pages*() functions, FOLL_PIN is OR'd in with whatever gup flags the caller provides. The caller is required to pass in a non-null struct pages* array, and the function then pin pages by incrementing each by a special-value. For now, that value is +1, just like get_user_pages*().::+value: GUP_PIN_COUNTING_BIAS.:: Function --------
@@ -1881,7 +1991,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))
@@ -1890,9 +2000,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(!try_pin_page(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);
@@ -1946,12 +2062,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(!try_pin_page(page))){+undo_dev_pagemap(nr,nr_start,flags,pages);+return0;+}+}else+get_page(page);+(*nr)++;pfn++;}while(addr+=PAGE_SIZE,addr!=end);
@@ -1973,7 +2097,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;
@@ -1991,7 +2115,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;
@@ -2014,8 +2138,8 @@ 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)+staticintrecord_subpages(structpage*page,unsignedlongaddr,+unsignedlongend,structpage**pages){intnr;
@@ -2025,12 +2149,31 @@ static int __record_subpages(struct page *page, unsigned long addr,returnnr;}-staticvoidput_compound_head(structpage*page,intrefs)+staticboolgrab_compound_head(structpage*head,intrefs,unsignedintflags){+if(flags&FOLL_PIN){+if(unlikely(!try_pin_compound_head(head,refs)))+returnfalse;+}else{+head=try_get_compound_head(head,refs);+if(!head)+returnfalse;+}++returntrue;+}++staticvoidput_compound_head(structpage*page,intrefs,unsignedintflags)+{+structpage*head=compound_head(page);++if(flags&FOLL_PIN)+refs*=GUP_PIN_COUNTING_BIAS;+/* Do a get_page() first, in case refs == page->_refcount */-get_page(page);-page_ref_sub(page,refs);-put_page(page);+get_page(head);+page_ref_sub(head,refs);+put_page(head);}#ifdef CONFIG_ARCH_HAS_HUGEPD
@@ -2064,14 +2207,13 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,head=pte_page(pte);page=head+((addr&(sz-1))>>PAGE_SHIFT);-refs=__record_subpages(page,addr,end,pages+*nr);+refs=record_subpages(page,addr,end,pages+*nr);-head=try_get_compound_head(head,refs);-if(!head)+if(!grab_compound_head(head,refs,flags))return0;if(unlikely(pte_val(pte)!=pte_val(*ptep))){-put_compound_head(head,refs);+put_compound_head(head,refs,flags);return0;}
@@ -2124,14 +2266,14 @@ 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);+refs=record_subpages(page,addr,end,pages+*nr);-head=try_get_compound_head(pmd_page(orig),refs);-if(!head)+head=pmd_page(orig);+if(!grab_compound_head(head,refs,flags))return0;if(unlikely(pmd_val(orig)!=pmd_val(*pmdp))){-put_compound_head(head,refs);+put_compound_head(head,refs,flags);return0;}
@@ -2158,14 +2300,14 @@ 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);+refs=record_subpages(page,addr,end,pages+*nr);-head=try_get_compound_head(pud_page(orig),refs);-if(!head)+head=pud_page(orig);+if(!grab_compound_head(head,refs,flags))return0;if(unlikely(pud_val(orig)!=pud_val(*pudp))){-put_compound_head(head,refs);+put_compound_head(head,refs,flags);return0;}
@@ -2187,14 +2329,14 @@ static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr,BUILD_BUG_ON(pgd_devmap(orig));page=pgd_page(orig)+((addr&~PGDIR_MASK)>>PAGE_SHIFT);-refs=__record_subpages(page,addr,end,pages+*nr);+refs=record_subpages(page,addr,end,pages+*nr);-head=try_get_compound_head(pgd_page(orig),refs);-if(!head)+head=pgd_page(orig);+if(!grab_compound_head(head,refs,flags))return0;if(unlikely(pgd_val(orig)!=pgd_val(*pgdp))){-put_compound_head(head,refs);+put_compound_head(head,refs,flags);return0;}
@@ -2504,21 +2649,24 @@ EXPORT_SYMBOL_GPL(get_user_pages_fast);intpin_user_pages_fast(unsignedlongstart,intnr_pages,unsignedintgup_flags,structpage**pages){-/*-*Thisisaplaceholder,untilthepinfunctionalityisactivated.-*Untilthen,justbehavelikethecorrespondingget_user_pages*()-*routine.-*/-returnget_user_pages_fast(start,nr_pages,gup_flags,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_user_pages_remote()-pinpagesofaremoteprocess(task!=current)*-*Fornow,thisisaplaceholderfunction,untilvariouscallsitesare-*convertedtousethecorrectget_user_pages*()orpin_user_pages*()API.So,-*thisisidenticaltoget_user_pages_remote().+*Nearlythesameasget_user_pages_remote(),exceptthatFOLL_PINisset.See+*get_user_pages_remote()fordocumentationonthefunctionarguments,because+*theargumentshereareidentical.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaunpin_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstfordetails.**ThisisintendedforCase1(DIO)inDocumentation/vm/pin_user_pages.rst.It*isNOTintendedforCase2(RDMA:long-termpins).
@@ -2528,22 +2676,24 @@ long pin_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm,unsignedintgup_flags,structpage**pages,structvm_area_struct**vmas,int*locked){-/*-*Thisisaplaceholder,untilthepinfunctionalityisactivated.-*Untilthen,justbehavelikethecorrespondingget_user_pages*()-*routine.-*/-returnget_user_pages_remote(tsk,mm,start,nr_pages,gup_flags,pages,-vmas,locked);+/* FOLL_GET and FOLL_PIN are mutually exclusive. */+if(WARN_ON_ONCE(gup_flags&FOLL_GET))+return-EINVAL;++gup_flags|=FOLL_PIN;+return__get_user_pages_remote(tsk,mm,start,nr_pages,gup_flags,+pages,vmas,locked);}EXPORT_SYMBOL(pin_user_pages_remote);/***pin_user_pages()-pinuserpagesinmemoryforusebyotherdevices*-*Fornow,thisisaplaceholderfunction,untilvariouscallsitesare-*convertedtousethecorrectget_user_pages*()orpin_user_pages*()API.So,-*thisisidenticaltoget_user_pages().+*Nearlythesameasget_user_pages(),exceptthatFOLL_TOUCHisnotset,and+*FOLL_PINisset.+*+*FOLL_PINmeansthatthepagesmustbereleasedviaunpin_user_page().Please+*seeDocumentation/vm/pin_user_pages.rstfordetails.**ThisisintendedforCase1(DIO)inDocumentation/vm/pin_user_pages.rst.It*isNOTintendedforCase2(RDMA:long-termpins).
@@ -2552,11 +2702,12 @@ long pin_user_pages(unsigned long start, unsigned long nr_pages,unsignedintgup_flags,structpage**pages,structvm_area_struct**vmas){-/*-*Thisisaplaceholder,untilthepinfunctionalityisactivated.-*Untilthen,justbehavelikethecorrespondingget_user_pages*()-*routine.-*/-returnget_user_pages(start,nr_pages,gup_flags,pages,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);
@@ -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-21 07:15:09
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-21 07:15:10
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_user_pages().
Reviewed-by: Jens Axboe <axboe@kernel.dk>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
fs/io_uring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:15:25
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_user_pages_fast(): via the '-b' 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 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_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.
Cc: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
mm/gup_benchmark.c | 65 ++++++++++++++++++++--
tools/testing/selftests/vm/gup_benchmark.c | 15 ++++-
2 files changed, 74 insertions(+), 6 deletions(-)
@@ -19,6 +21,42 @@ 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_BENCHMARK:+put_user_pages(pages,nr_pages);+break;+}+}++staticvoidverify_dma_pinned(intcmd,structpage**pages,+unsignedlongnr_pages)+{+inti;++switch(cmd){+casePIN_FAST_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 +103,14 @@ 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_BENCHMARK:+nr=pin_user_pages(addr,nr,gup->flags,pages+i,+NULL);+break;default:return-1;}
@@ -75,15 +121,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 +154,8 @@ static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,caseGUP_FAST_BENCHMARK:caseGUP_LONGTERM_BENCHMARK:caseGUP_BENCHMARK:+casePIN_FAST_BENCHMARK:+casePIN_BENCHMARK:break;default:return-EINVAL;
@@ -18,6 +18,10 @@#define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)#define GUP_BENCHMARK _IOWR('g', 3, struct gup_benchmark)+/* Similar to above, but use FOLL_PIN instead of FOLL_GET. */+#define PIN_FAST_BENCHMARK _IOWR('g', 4, struct gup_benchmark)+#define PIN_BENCHMARK _IOWR('g', 5, struct gup_benchmark)+/* Just the flags we need, copied from mm.h: */#define FOLL_WRITE 0x01 /* check pte is writable */
@@ -40,8 +44,14 @@ int main(int argc, char **argv)char*file="/dev/zero";char*p;-while((opt=getopt(argc,argv,"m:r:n:f:tTLUwSH"))!=-1){+while((opt=getopt(argc,argv,"m:r:n:f:abtTLUuwSH"))!=-1){switch(opt){+case'a':+cmd=PIN_FAST_BENCHMARK;+break;+case'b':+cmd=PIN_BENCHMARK;+break;case'm':size=atoi(optarg)*MB;break;
@@ -63,6 +73,9 @@ int main(int argc, char **argv)case'U':cmd=GUP_BENCHMARK;break;+case'u':+cmd=GUP_FAST_BENCHMARK;+break;case'w':write=1;break;
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:15:31
From: Dan Williams <redacted>
After the removal of the device-public infrastructure there are only 2
->page_free() call backs in the kernel. One of those is a device-private
callback in the nouveau driver, the other is a generic wakeup needed in
the DAX case. In the hopes that all ->page_free() callbacks can be
migrated to common core kernel functionality, move the device-private
specific actions in __put_devmap_managed_page() under the
is_device_private_page() conditional, including the ->page_free()
callback. For the other page types just open-code the generic wakeup.
Yes, the wakeup is only needed in the MEMORY_DEVICE_FSDAX case, but it
does no harm in the MEMORY_DEVICE_DEVDAX and MEMORY_DEVICE_PCI_P2PDMA
case.
Cc: Jan Kara <jack@suse.cz>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ira Weiny <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jérôme Glisse <redacted>
Signed-off-by: Dan Williams <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/nvdimm/pmem.c | 6 ----
mm/memremap.c | 80 ++++++++++++++++++++++++-------------------
2 files changed, 44 insertions(+), 42 deletions(-)
@@ -414,44 +415,51 @@ void __put_devmap_managed_page(struct page *page){intcount=page_ref_dec_return(page);-/*-*Ifrefcountis1thenpageisfreedandrefcountisstableasnobody-*holdsareferenceonthepage.-*/-if(count==1){-/* Clear Active bit in case of parallel mark_page_accessed */-__ClearPageActive(page);-__ClearPageWaiters(page);+/* still busy */+if(count>1)+return;-mem_cgroup_uncharge(page);+/* only triggered by the dev_pagemap shutdown path */+if(count==0){+__put_page(page);+return;+}-/*-*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;+/* notify page idle for dax */+if(!is_device_private_page(page)){+wake_up_var(&page->_refcount);+return;+}-page->pgmap->ops->page_free(page);-}elseif(!count)-__put_page(page);+/* 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->mappingmaystillidentifythepageasan+*anonymouspage.Ultimately,thisentirefieldisjuststale+*andwrong,anditwillcauseerrorsifnotcleared.One+*exampleis:+*+*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.+*/+page->mapping=NULL;+page->pgmap->ops->page_free(page);}EXPORT_SYMBOL(__put_devmap_managed_page);#endif /* CONFIG_DEV_PAGEMAP_OPS */
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:15:59
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.
Reviewed-by: Ira Weiny <redacted>
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-21 07:16:03
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: Jan Kara <jack@suse.cz>
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;
@@ -220,7 +220,7 @@ since the system was booted, via two new /proc/vmstat entries: :: /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+because there is a noticeable performance drop in unpin_user_page(), when they are activated. References
@@ -168,7 +168,7 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua,free_exit:/* free the references taken */-put_user_pages(mem->hpages,pinned);+unpin_user_pages(mem->hpages,pinned);vfree(mem->hpas);kfree(mem);
@@ -188,8 +188,8 @@ via_free_sg_info(struct pci_dev *pdev, drm_via_sg_info_t *vsg)kfree(vsg->desc_pages);/* fall through */casedr_via_pages_locked:-put_user_pages_dirty_lock(vsg->pages,vsg->num_pages,-(vsg->direction==DMA_FROM_DEVICE));+unpin_user_pages_dirty_lock(vsg->pages,vsg->num_pages,+(vsg->direction==DMA_FROM_DEVICE));/* fall through */casedr_via_pages_alloc:vfree(vsg->pages);
@@ -118,7 +118,7 @@ int hfi1_acquire_user_pages(struct mm_struct *mm, unsigned long vaddr, size_t npvoidhfi1_release_user_pages(structmm_struct*mm,structpage**p,size_tnpages,booldirty){-put_user_pages_dirty_lock(p,npages,dirty);+unpin_user_pages_dirty_lock(p,npages,dirty);if(mm){/* during close after signal, mm can be NULL */atomic64_sub(npages,&mm->pinned_vm);
@@ -706,7 +706,7 @@ static int qib_user_sdma_pin_pages(const struct qib_devdata *dd,/* if error, return all pages not managed by pkt */free_pages:while(i<j)-put_user_page(pages[i++]);+unpin_user_page(pages[i++]);done:returnret;
@@ -328,7 +328,7 @@ static int put_pfn(unsigned long pfn, int prot)if(!is_invalid_reserved_pfn(pfn)){structpage*page=pfn_to_page(pfn);-put_user_pages_dirty_lock(&page,1,prot&IOMMU_WRITE);+unpin_user_pages_dirty_lock(&page,1,prot&IOMMU_WRITE);return1;}return0;
@@ -2673,7 +2673,7 @@ 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() */+#define FOLL_PIN 0x40000 /* pages must be released via unpin_user_page *//**FOLL_PINandFOLL_LONGTERMmaybeusedinvariouscombinationswitheach
@@ -126,8 +126,8 @@ static int process_vm_rw_single_vec(unsigned long addr,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);+unpin_user_pages_dirty_lock(process_pages,pinned_pages,+vm_write);}returnrc;
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:16:42
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: Jan Kara <jack@suse.cz>
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-21 07:16:56
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.
Reviewed-by: Ira Weiny <redacted>
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-21 07:17:08
1. Convert from get_user_pages() to pin_user_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 | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
@@ -103,7 +103,7 @@ 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,+ret=pin_user_pages(ua+(entry<<PAGE_SHIFT),n,FOLL_WRITE|FOLL_LONGTERM,mem->hpages+entry,NULL);if(ret==n){
@@ -167,9 +167,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-21 07:17:19
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>
Reviewed-by: Jan Kara <jack@suse.cz>
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 | 91 ++++++++++++++++++++++----------------------------------
1 file changed, 36 insertions(+), 55 deletions(-)
@@ -1969,6 +1969,25 @@ 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;++for(nr=0;addr!=end;addr+=PAGE_SIZE)+pages[nr++]=page++;++returnnr;+}++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);+}+#ifdef CONFIG_ARCH_HAS_HUGEPDstaticunsignedlonghugepte_addr_end(unsignedlongaddr,unsignedlongend,unsignedlongsz)
@@ -1998,32 +2017,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;}+*nr+=refs;SetPageReferenced(head);return1;}
@@ -2071,28 +2078,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;}+*nr+=refs;SetPageReferenced(head);return1;}
@@ -2114,28 +2112,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;}+*nr+=refs;SetPageReferenced(head);return1;}
@@ -2151,28 +2140,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;}+*nr+=refs;SetPageReferenced(head);return1;}
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:17:26
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-21 07:17:37
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: Jan Kara <jack@suse.cz>
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-21 07:18:05
1. Change vfio from get_user_pages_remote(), to
pin_user_pages_remote().
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().
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 | 7 +++----
1 file changed, 3 insertions(+), 4 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;
@@ -347,7 +346,7 @@ 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,+ret=pin_user_pages_remote(NULL,mm,vaddr,1,flags|FOLL_LONGTERM,page,NULL,NULL);if(ret==1){*pfn=page_to_pfn(page[0]);
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:18:08
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.
Also, remove an unnessary pair of calls that were releasing and
reacquiring the mmap_sem. There is no need to avoid holding mmap_sem
just in order to call page_to_pfn().
Also, move the DAX check ("if a VMA is DAX, don't allow long term
pinning") from the VFIO call site, all the way into the internals
of get_user_pages_remote() and __gup_longterm_locked(). That is:
get_user_pages_remote() calls __gup_longterm_locked(), which in turn
calls check_dax_vmas(). It's lightly explained in the comments as well.
Thanks to Jason Gunthorpe for pointing out a clean way to fix this,
and to Dan Williams for helping clarify the DAX refactoring.
Reviewed-by: Jason Gunthorpe <redacted>
Reviewed-by: Ira Weiny <redacted>
Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Dan Williams <redacted>
Cc: Jerome Glisse <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 30 +++++-------------------------
mm/gup.c | 27 ++++++++++++++++++++++-----
2 files changed, 27 insertions(+), 30 deletions(-)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:18:13
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_user_pages().
Acked-by: Björn Töpel <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
net/xdp/xdp_umem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:18:17
Introduce pin_user_pages*() variations of get_user_pages*() calls,
and also pin_longterm_pages*() variations.
For now, these are placeholder calls, until the various call sites
are converted to use the correct get_user_pages*() or
pin_user_pages*() API.
These variants will eventually all set FOLL_PIN, which is also
introduced, and thoroughly documented.
pin_user_pages()
pin_user_pages_remote()
pin_user_pages_fast()
All pages that are pinned via the above calls, must be unpinned via
put_user_page().
The underlying rules are:
* FOLL_PIN is a gup-internal flag, so the call sites should not directly
set it. That behavior is enforced with assertions.
* 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: Jan Kara <jack@suse.cz>
Reviewed-by: Mike Rapoport <redacted> # Documentation
Reviewed-by: Jérôme Glisse <redacted>
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 | 233 ++++++++++++++++++++++
include/linux/mm.h | 63 ++++--
mm/gup.c | 153 ++++++++++++--
4 files changed, 416 insertions(+), 34 deletions(-)
create mode 100644 Documentation/core-api/pin_user_pages.rst
@@ -0,0 +1,233 @@+.. 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++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.++FOLL_PIN is internal to gup, meaning that it 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_LONGTERM, on the other hand, *is* allowed to be set at the gup call sites.+This is in order to avoid creating a large number of wrapper functions to cover+all combinations of get*(), pin*(), FOLL_LONGTERM, and more. Also, the+pin_user_pages*() APIs are clearly distinct from the get_user_pages*() APIs, so+that's a natural dividing line, and a good point to make separate wrapper calls.+In other words, use pin_user_pages*() for DMA-pinned pages, and+get_user_pages*() for other cases. There are four cases described later on in+this document, to further clarify that concept.++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+===================================++For these pin_user_pages*() functions, FOLL_PIN is OR'd in with whatever gup+flags the caller provides. The caller is required to pass in a non-null struct+pages* array, and the function then pin pages by incrementing each by a special+value. For now, that value is +1, just like get_user_pages*().::++ Function+ --------+ pin_user_pages FOLL_PIN is always set internally by this function.+ pin_user_pages_fast FOLL_PIN is always set internally by this function.+ pin_user_pages_remote FOLL_PIN is always set internally by this function.++For these get_user_pages*() functions, FOLL_GET might not even be specified.+Behavior is a little more complex than above. If FOLL_GET was *not* specified,+but the caller passed in a non-null struct pages* array, then the function+sets FOLL_GET for you, and proceeds to pin pages by incrementing the refcount+of each page by +1.::++ Function+ --------+ get_user_pages FOLL_GET is sometimes set internally by this function.+ get_user_pages_fast FOLL_GET is sometimes set internally by this function.+ get_user_pages_remote FOLL_GET is sometimes set internally by this function.++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 +1557,8 @@ 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);intaccount_locked_vm(structmm_struct*mm,unsignedlongpages,boolinc);int__account_locked_vm(structmm_struct*mm,unsignedlongpages,boolinc,
@@ -2610,13 +2617,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(),whoseusagesaretransient.**FIXME:Forpageswhicharepartofafilesystem,mappingsaresubjecttothe*lifetimeenforcedbythefilesystemandweneedguaranteesthatlongterm
@@ -1640,6 +1660,13 @@ long get_user_pages(unsigned long start, unsigned long nr_pages,unsignedintgup_flags,structpage**pages,structvm_area_struct**vmas){+/*+*FOLL_PINmustonlybesetinternallybythepin_user_pages*()APIs,+*neverdirectlybythecaller,soenforcethatwithanassertion:+*/+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);}
@@ -2386,29 +2413,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;
@@ -2448,4 +2460,103 @@ 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_pages*()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+*+*Fornow,thisisaplaceholderfunction,untilvariouscallsitesare+*convertedtousethecorrectget_user_pages*()orpin_user_pages*()API.So,+*thisisidenticaltoget_user_pages_fast().+*+*ThisisintendedforCase1(DIO)inDocumentation/vm/pin_user_pages.rst.It+*isNOTintendedforCase2(RDMA:long-termpins).+*/+intpin_user_pages_fast(unsignedlongstart,intnr_pages,+unsignedintgup_flags,structpage**pages)+{+/*+*Thisisaplaceholder,untilthepinfunctionalityisactivated.+*Untilthen,justbehavelikethecorrespondingget_user_pages*()+*routine.+*/+returnget_user_pages_fast(start,nr_pages,gup_flags,pages);+}+EXPORT_SYMBOL_GPL(pin_user_pages_fast);++/**+*pin_user_pages_remote()-pinpagesofaremoteprocess(task!=current)+*+*Fornow,thisisaplaceholderfunction,untilvariouscallsitesare+*convertedtousethecorrectget_user_pages*()orpin_user_pages*()API.So,+*thisisidenticaltoget_user_pages_remote().+*+*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)+{+/*+*Thisisaplaceholder,untilthepinfunctionalityisactivated.+*Untilthen,justbehavelikethecorrespondingget_user_pages*()+*routine.+*/+returnget_user_pages_remote(tsk,mm,start,nr_pages,gup_flags,pages,+vmas,locked);+}+EXPORT_SYMBOL(pin_user_pages_remote);++/**+*pin_user_pages()-pinuserpagesinmemoryforusebyotherdevices+*+*Fornow,thisisaplaceholderfunction,untilvariouscallsitesare+*convertedtousethecorrectget_user_pages*()orpin_user_pages*()API.So,+*thisisidenticaltoget_user_pages().+*+*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)+{+/*+*Thisisaplaceholder,untilthepinfunctionalityisactivated.+*Untilthen,justbehavelikethecorrespondingget_user_pages*()+*routine.+*/+returnget_user_pages(start,nr_pages,gup_flags,pages,vmas);+}+EXPORT_SYMBOL(pin_user_pages);
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:18:32
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.
Cc: Christoph Hellwig <hch@lst.de>
Suggested-by: Jérôme Glisse <redacted>
Reviewed-by: Dan Williams <redacted>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
include/linux/mm.h | 27 ++++++++++++++++++++++++---
mm/memremap.c | 16 ++--------------
2 files changed, 26 insertions(+), 17 deletions(-)
@@ -411,20 +411,8 @@ 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);--/* still busy */-if(count>1)-return;--/* only triggered by the dev_pagemap shutdown path */-if(count==0){-__put_page(page);-return;-}-/* notify page idle for dax */if(!is_device_private_page(page)){wake_up_var(&page->_refcount);
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 07:18:37
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.
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
mm/gup.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2019-11-21 08:04:10
On Wed, Nov 20, 2019 at 11:13:32PM -0800, John Hubbard wrote:
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.
Any reason for the spurious underscore in the function name?
Otherwise this looks fine and might be a worthwhile cleanup to feed
Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig <hch@lst.de>
From: Christoph Hellwig <hch@lst.de> Date: 2019-11-21 08:06:07
So while this looks correct and I still really don't see the major
benefit of the new code organization, especially as it bloats all
put_page callers.
I'd love to see code size change stats for an allyesconfig on this
commit.
From: Christoph Hellwig <hch@infradead.org> Date: 2019-11-21 08:09:57
On Wed, Nov 20, 2019 at 11:13:37PM -0800, John Hubbard wrote:
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: Jan Kara <jack@suse.cz>
Reviewed-by: Jason Gunthorpe <redacted>
Reviewed-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Jason, can you queue this up for 5.5 to reduce this patch stack a bit?
From: Christoph Hellwig <hch@infradead.org> Date: 2019-11-21 08:10:40
On Wed, Nov 20, 2019 at 11:13:36PM -0800, John Hubbard wrote:
+static int pin_goldfish_pages(unsigned long first_page,
+ unsigned long last_page,
+ unsigned int last_page_size,
+ int is_write,
+ struct page *pages[MAX_BUFFERS_PER_COMMAND],
+ unsigned int *iter_last_page_size)
Why not goldfish_pin_pages? Normally we put the module / subsystem
in front.
Also can we get this queued up for 5.5 to get some trivial changes
out of the way?
From: Christoph Hellwig <hch@infradead.org> Date: 2019-11-21 08:11:10
On Wed, Nov 20, 2019 at 11:13:38PM -0800, John Hubbard wrote:
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.
Looks good, and like a fix that should be queued up through the media
tree for 5.5 and maybe even added to -stable.
Reviewed-by: Christoph Hellwig <hch@lst.de>
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 08:32:56
On 11/21/19 12:03 AM, Christoph Hellwig wrote:
On Wed, Nov 20, 2019 at 11:13:32PM -0800, John Hubbard wrote:
quoted
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.
Any reason for the spurious underscore in the function name?
argghh, I just fixed that, but applied the fix to the wrong patch! So now
patch 17 ("mm/gup: track FOLL_PIN pages") is improperly renaming it, instead
of this patch naming it correctly in the first place. Will fix.
Otherwise this looks fine and might be a worthwhile cleanup to feed
Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Thanks for the reviews! Say, it sounds like your view here is that this
series should be targeted at 5.6 (not 5.5), is that what you have in mind?
And get the preparatory patches (1-9, and maybe even 10-16) into 5.5?
thanks,
--
John Hubbard
NVIDIA
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 08:39:36
On 11/21/19 12:08 AM, Christoph Hellwig wrote:
On Wed, Nov 20, 2019 at 11:13:36PM -0800, John Hubbard wrote:
quoted
+static int pin_goldfish_pages(unsigned long first_page,
+ unsigned long last_page,
+ unsigned int last_page_size,
+ int is_write,
+ struct page *pages[MAX_BUFFERS_PER_COMMAND],
+ unsigned int *iter_last_page_size)
Why not goldfish_pin_pages? Normally we put the module / subsystem
in front.
Heh, is that how it's supposed to go? Sure, I'll change it. :)
Also can we get this queued up for 5.5 to get some trivial changes
out of the way?
Is that a question to Andrew, or a request for me to send this as a
separate patch email (or both)?
thanks,
--
John Hubbard
NVIDIA
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 08:57:55
On 11/21/19 12:05 AM, Christoph Hellwig wrote:
So while this looks correct and I still really don't see the major
benefit of the new code organization, especially as it bloats all
put_page callers.
I'd love to see code size change stats for an allyesconfig on this
commit.
Right, I'm running that now, will post the results. (btw, if there is
a script and/or standard format I should use, I'm all ears. I'll dig
through lwn...)
thanks,
--
John Hubbard
NVIDIA
From: Jan Kara <jack@suse.cz> Date: 2019-11-21 09:39:55
On Wed 20-11-19 23:13:47, John Hubbard wrote:
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], [2], and [3].
This also changes a BUG_ON(), to a WARN_ON(), in follow_page_mask().
[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/
Suggested-by: Jan Kara <jack@suse.cz>
Suggested-by: Jérôme Glisse <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
Thanks for the patch! We are mostly getting there. Some smaller comments
below.
+/**
+ * try_pin_compound_head() - mark a compound page as being used by
+ * pin_user_pages*().
+ *
+ * This is the FOLL_PIN counterpart to try_get_compound_head().
+ *
+ * @page: pointer to page to be marked
+ * @Return: true for success, false for failure
+ */
+__must_check bool try_pin_compound_head(struct page *page, int refs)
+{
+ page = try_get_compound_head(page, GUP_PIN_COUNTING_BIAS * refs);
+ if (!page)
+ return false;
+
+ __update_proc_vmstat(page, NR_FOLL_PIN_REQUESTED, refs);
+ return true;
+}
+
+#ifdef CONFIG_DEV_PAGEMAP_OPS
+static bool __put_devmap_managed_user_page(struct page *page)
Probably call this __unpin_devmap_managed_user_page()? To match the later
conversion of put_user_page() to unpin_user_page()?
quoted hunk
+{
+ bool is_devmap = page_is_devmap_managed(page);
+
+ if (is_devmap) {
+ int count = page_ref_sub_return(page, GUP_PIN_COUNTING_BIAS);
+
+ __update_proc_vmstat(page, NR_FOLL_PIN_RETURNED, 1);
+ /*
+ * devmap page refcounts are 1-based, rather than 0-based: if
+ * refcount is 1, then the page is free and the refcount is
+ * stable because nobody holds a reference on the page.
+ */
+ if (count == 1)
+ free_devmap_managed_page(page);
+ else if (!count)
+ __put_page(page);
+ }
+
+ return is_devmap;
+}
+#else
+static bool __put_devmap_managed_user_page(struct page *page)
+{
+ return false;
+}
+#endif /* CONFIG_DEV_PAGEMAP_OPS */
+
+/**
+ * put_user_page() - release a dma-pinned page
+ * @page: pointer to page to be released
+ *
+ * Pages that were pinned via pin_user_pages*() must be released via either
+ * put_user_page(), or one of the put_user_pages*() routines. This is so that
+ * such pages can be separately tracked and uniquely handled. In particular,
+ * interactions with RDMA and filesystems need special handling.
+ */
+void put_user_page(struct page *page)
+{
+ page = compound_head(page);
+
+ /*
+ * For devmap managed pages we need to catch refcount transition from
+ * GUP_PIN_COUNTING_BIAS to 1, when refcount reach one it means the
+ * page is free and we need to inform the device driver through
+ * callback. See include/linux/memremap.h and HMM for details.
+ */
+ if (__put_devmap_managed_user_page(page))
+ return;
+
+ if (page_ref_sub_and_test(page, GUP_PIN_COUNTING_BIAS))
+ __put_page(page);
+
+ __update_proc_vmstat(page, NR_FOLL_PIN_RETURNED, 1);
+}
+EXPORT_SYMBOL(put_user_page);
+
/**
* put_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages
* @pages: array of pages to be maybe marked dirty, and definitely released.
@@ -237,10 +327,11 @@ static struct page *follow_page_pte(struct vm_area_struct *vma, } page = vm_normal_page(vma, address, pte);- if (!page && pte_devmap(pte) && (flags & FOLL_GET)) {+ if (!page && pte_devmap(pte) && (flags & (FOLL_GET | FOLL_PIN))) { /*- * Only return device mapping pages in the FOLL_GET case since- * they are only valid while holding the pgmap reference.+ * Only return device mapping pages in the FOLL_GET or FOLL_PIN+ * case since they are only valid while holding the pgmap+ * reference. */ *pgmap = get_dev_pagemap(pte_pfn(pte), *pgmap); if (*pgmap)
}
if (flags & FOLL_TOUCH) {
if ((flags & FOLL_WRITE) &&
@@ -1890,9 +2000,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)- goto pte_unmap;+ if (flags & FOLL_PIN) {+ head = page;+ if (unlikely(!try_pin_page(head)))+ goto pte_unmap;+ } else {+ head = try_get_compound_head(page, 1);+ if (!head)+ goto pte_unmap;+ }
Why don't you use grab_page() here? Also you seem to loose the head =
compound_head(page) indirection here for the FOLL_PIN case?
quoted hunk
if (unlikely(pte_val(pte) != pte_val(*ptep))) {
put_page(head);
@@ -1946,12 +2062,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); return 0; } SetPageReferenced(page); pages[*nr] = page;- get_page(page);++ if (flags & FOLL_PIN) {+ if (unlikely(!try_pin_page(page))) {+ undo_dev_pagemap(nr, nr_start, flags, pages);+ return 0;+ }+ } else+ get_page(page);+
Use grab_page() here?
(*nr)++;
pfn++;
} while (addr += PAGE_SIZE, addr != end);
...
quoted hunk
@@ -2025,12 +2149,31 @@ static int __record_subpages(struct page *page, unsigned long addr, return nr; }-static void put_compound_head(struct page *page, int refs)+static bool grab_compound_head(struct page *head, int refs, unsigned int flags) {+ if (flags & FOLL_PIN) {+ if (unlikely(!try_pin_compound_head(head, refs)))+ return false;+ } else {+ head = try_get_compound_head(head, refs);+ if (!head)+ return false;+ }++ return true;+}++static void put_compound_head(struct page *page, int refs, unsigned int flags)+{+ struct page *head = compound_head(page);++ if (flags & FOLL_PIN)+ refs *= GUP_PIN_COUNTING_BIAS;+ /* Do a get_page() first, in case refs == page->_refcount */- get_page(page);- page_ref_sub(page, refs);- put_page(page);+ get_page(head);+ page_ref_sub(head, refs);+ put_page(head); } #ifdef CONFIG_ARCH_HAS_HUGEPD
@@ -2064,14 +2207,13 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr, head = pte_page(pte); page = head + ((addr & (sz-1)) >> PAGE_SHIFT);- refs = __record_subpages(page, addr, end, pages + *nr);+ refs = record_subpages(page, addr, end, pages + *nr);- head = try_get_compound_head(head, refs);- if (!head)+ if (!grab_compound_head(head, refs, flags))
Are you sure this is correct? Historically we seem to have always had logic
like:
head = compound_head(pte_page / pmd_page / ... (orig))
in this code. And you removed this now. Looking at the code I'm not sure
whether the compound_head() indirection is really needed or not. We seem to
have already huge page head in the page table but maybe there's some subtle
case I'm missing. So I'd be calmer if we left the head=compound_head(...)
in the code but if you really want to remove it, I'd like to see Ack from
someone actually familiar with huge pages - e.g. Kirill Shutemov...
And even if we find out that compound_head() indirection isn't really
needed, that is big enough change in the logic that it would deserve to be
done in a separate patch (if only for debugging by bisection purposes).
@@ -5034,8 +5052,20 @@ follow_huge_pmd(struct mm_struct *mm, unsigned long address, pte = huge_ptep_get((pte_t *)pmd); if (pte_present(pte)) { page = pmd_page(*pmd) + ((address & ~PMD_MASK) >> PAGE_SHIFT);+ if (flags & FOLL_GET) get_page(page);+ else if (flags & FOLL_PIN) {+ /*+ * try_pin_page() is not actually expected to fail+ * here because we hold the ptl.+ */+ if (unlikely(!try_pin_page(page))) {+ WARN_ON_ONCE(1);+ page = NULL;+ goto out;+ }+ }
Use grab_page() here?
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2019-11-21 09:49:21
On Thu 21-11-19 00:29:59, John Hubbard wrote:
On 11/21/19 12:03 AM, Christoph Hellwig wrote:
quoted
Otherwise this looks fine and might be a worthwhile cleanup to feed
Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Thanks for the reviews! Say, it sounds like your view here is that this
series should be targeted at 5.6 (not 5.5), is that what you have in mind?
And get the preparatory patches (1-9, and maybe even 10-16) into 5.5?
Yeah, actually I feel the same. The merge window is going to open on Sunday
and the series isn't still fully baked and happily sitting in linux-next
(and larger changes should really sit in linux-next for at least a week,
preferably two, before the merge window opens to get some reasonable test
coverage). So I'd take out the independent easy patches that are already
reviewed, get them merged into Andrew's (or whatever other appropriate
tree) now so that they get at least a week of testing in linux-next before
going upstream. And the more involved bits will have to wait for 5.6 -
which means let's just continue working on them as we do now because
ideally in 4 weeks we should have them ready with all the reviews so that
they can be picked up and integrated into linux-next.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2019-11-21 09:54:27
On Thu 21-11-19 00:29:59, John Hubbard wrote:
quoted
Otherwise this looks fine and might be a worthwhile cleanup to feed
Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Thanks for the reviews! Say, it sounds like your view here is that this
series should be targeted at 5.6 (not 5.5), is that what you have in mind?
And get the preparatory patches (1-9, and maybe even 10-16) into 5.5?
One more note :) If you are going to push pin_user_pages() interfaces
(which I'm fine with), it would probably make sense to push also the
put_user_pages() -> unpin_user_pages() renaming so that that inconsistency
in naming does not exist in the released upstream kernel.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2019-11-21 14:36:50
On Thu, Nov 21, 2019 at 12:07:46AM -0800, Christoph Hellwig wrote:
On Wed, Nov 20, 2019 at 11:13:37PM -0800, John Hubbard wrote:
quoted
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: Jan Kara <jack@suse.cz>
Reviewed-by: Jason Gunthorpe <redacted>
Reviewed-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Jason, can you queue this up for 5.5 to reduce this patch stack a bit?
Yes, I said I'd do this in an earlier revision. Now that it is clear this
won't go through Andrew's tree, applied to rdma for-next
Thanks,
Jason
From: Dan Williams <hidden> Date: 2019-11-21 17:00:19
On Thu, Nov 21, 2019 at 12:57 AM John Hubbard [off-list ref] wrote:
On 11/21/19 12:05 AM, Christoph Hellwig wrote:
quoted
So while this looks correct and I still really don't see the major
benefit of the new code organization, especially as it bloats all
put_page callers.
I'd love to see code size change stats for an allyesconfig on this
commit.
Right, I'm running that now, will post the results. (btw, if there is
a script and/or standard format I should use, I'm all ears. I'll dig
through lwn...)
From: Alex Williamson <hidden> Date: 2019-11-21 21:35:48
On Wed, 20 Nov 2019 23:13:39 -0800
John Hubbard [off-list ref] 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.
Also, remove an unnessary pair of calls that were releasing and
reacquiring the mmap_sem. There is no need to avoid holding mmap_sem
just in order to call page_to_pfn().
Also, move the DAX check ("if a VMA is DAX, don't allow long term
pinning") from the VFIO call site, all the way into the internals
of get_user_pages_remote() and __gup_longterm_locked(). That is:
get_user_pages_remote() calls __gup_longterm_locked(), which in turn
calls check_dax_vmas(). It's lightly explained in the comments as well.
Thanks to Jason Gunthorpe for pointing out a clean way to fix this,
and to Dan Williams for helping clarify the DAX refactoring.
Reviewed-by: Jason Gunthorpe <redacted>
Reviewed-by: Ira Weiny <redacted>
Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Dan Williams <redacted>
Cc: Jerome Glisse <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 30 +++++-------------------------
mm/gup.c | 27 ++++++++++++++++++++++-----
2 files changed, 27 insertions(+), 30 deletions(-)
Tested with device assignment and Intel mdev vGPU assignment with QEMU
userspace:
Tested-by: Alex Williamson <redacted>
Acked-by: Alex Williamson <redacted>
Feel free to include for 19/24 as well. Thanks,
Alex
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 21:50:44
On 11/21/19 1:49 AM, Jan Kara wrote:
On Thu 21-11-19 00:29:59, John Hubbard wrote:
quoted
On 11/21/19 12:03 AM, Christoph Hellwig wrote:
quoted
Otherwise this looks fine and might be a worthwhile cleanup to feed
Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Thanks for the reviews! Say, it sounds like your view here is that this
series should be targeted at 5.6 (not 5.5), is that what you have in mind?
And get the preparatory patches (1-9, and maybe even 10-16) into 5.5?
Yeah, actually I feel the same. The merge window is going to open on Sunday
and the series isn't still fully baked and happily sitting in linux-next
(and larger changes should really sit in linux-next for at least a week,
preferably two, before the merge window opens to get some reasonable test
coverage). So I'd take out the independent easy patches that are already
reviewed, get them merged into Andrew's (or whatever other appropriate
tree) now so that they get at least a week of testing in linux-next before
going upstream. And the more involved bits will have to wait for 5.6 -
which means let's just continue working on them as we do now because
ideally in 4 weeks we should have them ready with all the reviews so that
they can be picked up and integrated into linux-next.
Honza
OK, thanks for spelling it out. I'll shift over to getting the easy patches
prepared for 5.5, for now.
thanks,
--
John Hubbard
NVIDIA
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 21:52:38
On 11/21/19 1:35 PM, Alex Williamson wrote:
On Wed, 20 Nov 2019 23:13:39 -0800
John Hubbard [off-list ref] wrote:
quoted
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.
Also, remove an unnessary pair of calls that were releasing and
reacquiring the mmap_sem. There is no need to avoid holding mmap_sem
just in order to call page_to_pfn().
Also, move the DAX check ("if a VMA is DAX, don't allow long term
pinning") from the VFIO call site, all the way into the internals
of get_user_pages_remote() and __gup_longterm_locked(). That is:
get_user_pages_remote() calls __gup_longterm_locked(), which in turn
calls check_dax_vmas(). It's lightly explained in the comments as well.
Thanks to Jason Gunthorpe for pointing out a clean way to fix this,
and to Dan Williams for helping clarify the DAX refactoring.
Reviewed-by: Jason Gunthorpe <redacted>
Reviewed-by: Ira Weiny <redacted>
Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Dan Williams <redacted>
Cc: Jerome Glisse <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 30 +++++-------------------------
mm/gup.c | 27 ++++++++++++++++++++++-----
2 files changed, 27 insertions(+), 30 deletions(-)
Tested with device assignment and Intel mdev vGPU assignment with QEMU
userspace:
Tested-by: Alex Williamson <redacted>
Acked-by: Alex Williamson <redacted>
Feel free to include for 19/24 as well. Thanks,
Alex
Great! Thanks for the testing and ack on those. I'm about to repackage
(and split up as CH requested) for 5.5, and will keep you on CC, of course.
thanks,
--
John Hubbard
NVIDIA
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-21 22:25:22
On 11/21/19 8:59 AM, Dan Williams wrote:
On Thu, Nov 21, 2019 at 12:57 AM John Hubbard [off-list ref] wrote:
quoted
On 11/21/19 12:05 AM, Christoph Hellwig wrote:
quoted
So while this looks correct and I still really don't see the major
benefit of the new code organization, especially as it bloats all
put_page callers.
I'd love to see code size change stats for an allyesconfig on this
commit.
Right, I'm running that now, will post the results. (btw, if there is
a script and/or standard format I should use, I'm all ears. I'll dig
through lwn...)
Just run:
size vmlinux
Beautiful. I thought it would involve a lot more. Here's results:
linux.git (Linux 5.4-rc8+):
==============================================
text data bss dec hex filename
227578032 213267935 76877984 517723951 1edbd72f vmlinux
With patches 4 and 5 applied to linux.git:
==========================================
text data bss dec hex filename
229698560 213288379 76853408 519840347 1efc225b vmlinux
Analysis:
=========
This increased the size of text by 0.93%. Which is a measurable bloat, so
the inlining really is undesirable here, yes. I'll do it differently.
thanks,
--
John Hubbard
NVIDIA
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-22 02:57:07
On 11/21/19 1:54 AM, Jan Kara wrote:
On Thu 21-11-19 00:29:59, John Hubbard wrote:
quoted
quoted
Otherwise this looks fine and might be a worthwhile cleanup to feed
Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Thanks for the reviews! Say, it sounds like your view here is that this
series should be targeted at 5.6 (not 5.5), is that what you have in mind?
And get the preparatory patches (1-9, and maybe even 10-16) into 5.5?
One more note :) If you are going to push pin_user_pages() interfaces
(which I'm fine with), it would probably make sense to push also the
put_user_pages() -> unpin_user_pages() renaming so that that inconsistency
in naming does not exist in the released upstream kernel.
Honza
Yes, that's what this patch series does. But I'm not sure if "push" here
means, "push out: defer to 5.6", "push (now) into 5.5", or "advocate for"?
I will note that it's not going to be easy to rename in one step, now
that this is being split up. Because various put_user_pages()-based items
are going into 5.5 via different maintainer trees now. Probably I'd need
to introduce unpin_user_page() alongside put_user_page()...thoughts?
thanks,
--
John Hubbard
NVIDIA
From: Jan Kara <jack@suse.cz> Date: 2019-11-22 11:15:17
On Thu 21-11-19 18:54:02, John Hubbard wrote:
On 11/21/19 1:54 AM, Jan Kara wrote:
quoted
On Thu 21-11-19 00:29:59, John Hubbard wrote:
quoted
quoted
Otherwise this looks fine and might be a worthwhile cleanup to feed
Andrew for 5.5 independent of the gut of the changes.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Thanks for the reviews! Say, it sounds like your view here is that this
series should be targeted at 5.6 (not 5.5), is that what you have in mind?
And get the preparatory patches (1-9, and maybe even 10-16) into 5.5?
One more note :) If you are going to push pin_user_pages() interfaces
(which I'm fine with), it would probably make sense to push also the
put_user_pages() -> unpin_user_pages() renaming so that that inconsistency
in naming does not exist in the released upstream kernel.
Honza
Yes, that's what this patch series does. But I'm not sure if "push" here
means, "push out: defer to 5.6", "push (now) into 5.5", or "advocate for"?
I meant to include the patch in the "for 5.5" batch.
I will note that it's not going to be easy to rename in one step, now
that this is being split up. Because various put_user_pages()-based items
are going into 5.5 via different maintainer trees now. Probably I'd need
to introduce unpin_user_page() alongside put_user_page()...thoughts?
Yes, I understand that moving that patch from the end of the series would
cause fair amount of conflicts. I was hoping that you could generate the
patch with sed/Coccinelle and then rebasing what remains for 5.6 on top of
that patch should not be that painful so overall it should not be that much
work. But I may be wrong so if it proves to be too tedious, let's just
postpone the renaming to 5.6. I don't find having both unpin_user_page()
and put_user_page() a better alternative to current state. Thanks!
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-24 06:15:15
On 11/21/19 6:36 AM, Jason Gunthorpe wrote:
On Thu, Nov 21, 2019 at 12:07:46AM -0800, Christoph Hellwig wrote:
quoted
On Wed, Nov 20, 2019 at 11:13:37PM -0800, John Hubbard wrote:
quoted
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: Jan Kara <jack@suse.cz>
Reviewed-by: Jason Gunthorpe <redacted>
Reviewed-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Jason, can you queue this up for 5.5 to reduce this patch stack a bit?
Yes, I said I'd do this in an earlier revision. Now that it is clear this
won't go through Andrew's tree, applied to rdma for-next
Great, I'll plan on it going up through that tree. To be clear, is it headed
for:
git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
?
thanks,
--
John Hubbard
NVIDIA
From: Leon Romanovsky <leon@kernel.org> Date: 2019-11-24 10:07:30
On Thu, Nov 21, 2019 at 10:36:43AM -0400, Jason Gunthorpe wrote:
On Thu, Nov 21, 2019 at 12:07:46AM -0800, Christoph Hellwig wrote:
quoted
On Wed, Nov 20, 2019 at 11:13:37PM -0800, John Hubbard wrote:
quoted
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: Jan Kara <jack@suse.cz>
Reviewed-by: Jason Gunthorpe <redacted>
Reviewed-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Jason, can you queue this up for 5.5 to reduce this patch stack a bit?
Yes, I said I'd do this in an earlier revision. Now that it is clear this
won't go through Andrew's tree, applied to rdma for-next
From: John Hubbard <jhubbard@nvidia.com> Date: 2019-11-25 00:05:28
On 11/24/19 2:07 AM, Leon Romanovsky wrote:
On Thu, Nov 21, 2019 at 10:36:43AM -0400, Jason Gunthorpe wrote:
quoted
On Thu, Nov 21, 2019 at 12:07:46AM -0800, Christoph Hellwig wrote:
quoted
On Wed, Nov 20, 2019 at 11:13:37PM -0800, John Hubbard wrote:
quoted
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: Jan Kara <jack@suse.cz>
Reviewed-by: Jason Gunthorpe <redacted>
Reviewed-by: Ira Weiny <redacted>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Jason, can you queue this up for 5.5 to reduce this patch stack a bit?
Yes, I said I'd do this in an earlier revision. Now that it is clear this
won't go through Andrew's tree, applied to rdma for-next
Jason,
This patch broke RDMA completely.
Change from get_user_pages() to get_user_pages_fast() causes to endless
amount of splats due to combination of the following code:
189 struct ib_umem *ib_umem_get(struct ib_udata *udata, unsigned long addr,
190 size_t size, int access)
...
263 if (!umem->writable)
264 gup_flags |= FOLL_FORCE;
265
and
2398 int get_user_pages_fast(unsigned long start, int nr_pages,
2399 unsigned int gup_flags, struct page **pages)
2400 {
2401 unsigned long addr, len, end;
2402 int nr = 0, ret = 0;
2403
2404 if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM)))
2405 return -EINVAL;
Hi Leon,
I looked into this, and I believe that the problem is in gup.c. There appears to
have been an oversight, in commit 817be129e6f2 ("mm: validate get_user_pages_fast
flags"), in filtering out FOLL_FORCE. There is nothing in the _fast() implementation
that requires that we avoid writing to the pages.
So I intend, to post a two-patch series that includes this fix, first, but maybe this
means that it should go in via -mm. I'm not sure what's the best tree.
@@ -2401,7 +2401,8 @@ int get_user_pages_fast(unsigned long start, int nr_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_FORCE)))return-EINVAL;start=untagged_addr(start)&PAGE_MASK;
From: Jason Gunthorpe <jgg@ziepe.ca> Date: 2019-11-25 00:53:49
On Sun, Nov 24, 2019 at 04:05:16PM -0800, John Hubbard wrote:
I looked into this, and I believe that the problem is in gup.c. There appears to
have been an oversight, in commit 817be129e6f2 ("mm: validate get_user_pages_fast
flags"), in filtering out FOLL_FORCE. There is nothing in the _fast() implementation
that requires that we avoid writing to the pages.
I think it is too late to be doing these kinds of changes, I will
revert the patch and this will miss this merge window.
Jason
From ec6cb45292d21d1af9b9d95997b8cf204bbe854c Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <redacted>
Date: Sun, 24 Nov 2019 20:47:59 -0400
Subject: [PATCH] Revert "IB/umem: use get_user_pages_fast() to pin DMA pages"
This reverts commit c9a7a2ed837c563f9f89743a6db732591cb4035b.
This was merged before enough testing was done, and it triggers a WARN_ON()
in get_user_pages_fast():
WARNING: CPU: 1 PID: 2557 at mm/gup.c:2404 get_user_pages_fast+0x115/0x180
Call Trace:
ib_umem_get+0x298/0x550 [ib_uverbs]
mlx5_ib_db_map_user+0xad/0x130 [mlx5_ib]
mlx5_ib_create_cq+0x1e8/0xaa0 [mlx5_ib]
create_cq+0x1c8/0x2d0 [ib_uverbs]
ib_uverbs_create_cq+0x70/0xa0 [ib_uverbs]
ib_uverbs_handler_UVERBS_METHOD_INVOKE_WRITE+0xc2/0xf0 [ib_uverbs]
ib_uverbs_cmd_verbs.isra.6+0x5be/0xbe0 [ib_uverbs]
? uverbs_disassociate_api+0xd0/0xd0 [ib_uverbs]
? kvm_clock_get_cycles+0xd/0x10
? kmem_cache_alloc+0x176/0x1c0
? filemap_map_pages+0x18c/0x350
ib_uverbs_ioctl+0xc0/0x120 [ib_uverbs]
do_vfs_ioctl+0xa1/0x610
ksys_ioctl+0x70/0x80
__x64_sys_ioctl+0x16/0x20
do_syscall_64+0x42/0x110
entry_SYSCALL_64_after_hwframe+0x44/0xa9
2404 if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM)))
2405 return -EINVAL;
While we think this WARN_ON is probably bogus, resolving this will have to
wait.
Signed-off-by: Jason Gunthorpe <redacted>
---
drivers/infiniband/core/umem.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
From: kbuild test robot <hidden> Date: 2019-11-30 19:00:07
Hi John,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.4]
[cannot apply to mmotm/master rdma/for-next linuxtv-media/master next-20191129]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/John-Hubbard/mm-gup-track-dma-pinned-pages-FOLL_PIN/20191122-092349
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git c74386d50fbaf4a54fd3fe560f1abc709c0cff4b
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <redacted>
All errors (new ones prefixed by >>):
mm/gup.c: In function 'pin_user_pages_remote':
quoted
mm/gup.c:2684:9: error: implicit declaration of function '__get_user_pages_remote'; did you mean 'get_user_pages_remote'? [-Werror=implicit-function-declaration]
return __get_user_pages_remote(tsk, mm, start, nr_pages, gup_flags,
^~~~~~~~~~~~~~~~~~~~~~~
get_user_pages_remote
cc1: some warnings being treated as errors
vim +2684 mm/gup.c
2660
2661 /**
2662 * pin_user_pages_remote() - pin pages of a remote process (task != current)
2663 *
2664 * Nearly the same as get_user_pages_remote(), except that FOLL_PIN is set. See
2665 * get_user_pages_remote() for documentation on the function arguments, because
2666 * the arguments here are identical.
2667 *
2668 * FOLL_PIN means that the pages must be released via unpin_user_page(). Please
2669 * see Documentation/vm/pin_user_pages.rst for details.
2670 *
2671 * This is intended for Case 1 (DIO) in Documentation/vm/pin_user_pages.rst. It
2672 * is NOT intended for Case 2 (RDMA: long-term pins).
2673 */
2674 long pin_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm,
2675 unsigned long start, unsigned long nr_pages,
2676 unsigned int gup_flags, struct page **pages,
2677 struct vm_area_struct **vmas, int *locked)
2678 {
2679 /* FOLL_GET and FOLL_PIN are mutually exclusive. */
2680 if (WARN_ON_ONCE(gup_flags & FOLL_GET))
2681 return -EINVAL;
2682
2683 gup_flags |= FOLL_PIN;
2684 return __get_user_pages_remote(tsk, mm, start, nr_pages, gup_flags,