mm/gup: track dma-pinned pages: FOLL_PIN
Subsystems (29)
John Hubbard
reviewerv9 cover mm/gup: track dma-pinned pages: FOLL_PIN
Hi,
This implements an API naming change (put_user_page*() --> unpin_user_page*()), and also implements tracking of FOLL_PIN pages. It extends that tracking to a few select subsystems. More subsystems will be added in follow up work.
Christoph Hellwig, a point of interest:
a) I've moved the bulk of the code out of the inline functions, as requested, for the devmap changes (patch 4: "mm: devmap: refactor 1-based refcounting for ZONE_DEVICE pages").
Changes since v8:
* Merged the "mm/gup: pass flags arg to __gup_device_* functions" patch into the "mm/gup: track FOLL_PIN pages" patch, as requested by Christoph and Jan.
* Changed void grab_page() to bool try_grab_page(), and handled errors at the call sites. (From Jan's review comments.) try_grab_page() attempts to avoid page refcount overflows, even when counting up with GUP_PIN_COUNTING_BIAS increments.
* Fixed a bug that I'd introduced, when changing a BUG() to a WARN().
* Added Jan's reviewed-by tag to the " mm/gup: allow FOLL_FORCE for get_user_pages_fast()" patch.
* Documentation: pin_user_pages.rst: fixed an incorrect gup_benchmark invocation, left over from the pin_longterm days, spotted while preparing this version.
* Rebased onto today's linux.git (-rc1), and re-tested.
Changes since v7:
* Rebased onto Linux 5.5-rc1
* Reworked the grab_page() and try_grab_compound_head(), for API consistency and less diffs (thanks to Jan Kara's reviews).
* Added Leon Romanovsky's reviewed-by tags for two of the IB-related patches.
* patch 4 refactoring changes, as mentioned above.
There is a git repo and branch, for convenience:
[email protected]:johnhubbard/linux.git pin_user_pages_tracking_v8
For the remaining list of "changes since version N", those are all in v7, which is here:
https://lore.kernel.org/r/[email protected]
============================================================
Overview:
This is a prerequisite to solving the 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)
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.
Runtime testing for the call sites so far is pretty light:
* 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: ran "ib_write_bw", which exercises the umem.c changes, but not the other changes.
* 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...
[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 (24):
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
mm: fix get_user_pages_remote()'s handling of FOLL_LONGTERM
vfio: fix FOLL_LONGTERM use, simplify get_user_pages_remote() call mm/gup: allow FOLL_FORCE for get_user_pages_fast() IB/umem: use get_user_pages_fast() to pin DMA pages 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() media/v4l2-core: set pages dirty upon releasing DMA buffers 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, tree-wide: rename put_user_page*() to unpin_user_page*() mm/gup: track FOLL_PIN pages
mm/gup_benchmark: support pin_user_pages() and related calls selftests/vm: run_vmtests: invoke gup_benchmark with basic FOLL_PIN
coverage
Documentation/core-api/index.rst | 1 + Documentation/core-api/pin_user_pages.rst | 232 ++++++++ arch/powerpc/mm/book3s64/iommu_api.c | 10 +- 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 | 149 ++++- include/linux/mmzone.h | 2 + include/linux/page_ref.h | 10 + mm/gup.c | 598 +++++++++++++++----- mm/gup_benchmark.c | 74 ++- mm/huge_memory.c | 26 +- mm/hugetlb.c | 25 +- mm/memremap.c | 76 ++- mm/process_vm_access.c | 28 +- mm/swap.c | 24 + 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 + 31 files changed, 1109 insertions(+), 355 deletions(-) create mode 100644 Documentation/core-api/pin_user_pages.rst
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 1/25 mm/gup: factor out duplicate code from four routines
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.
--- mm/gup.c | 91 ++++++++++++++++++++++---------------------------------- 1 file changed, 36 insertions(+), 55 deletions(-)
mm/gup.c +36 -55
@@ -1978,6 +1978,25 @@ static int __gup_device_huge_pud(pud_t pud, pud_t *pudp, unsigned long addr, }#endif +static int record_subpages (struct page *page,unsigned long addr, +unsigned long end,struct page **pages) +{ +int nr; + +for (nr =0 ; addr != end; addr +=PAGE_SIZE ) + pages[nr++] = page++; + +return nr; +} + +static void put_compound_head (struct page *page,int refs) +{ +/* 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_HUGEPD static unsigned long hugepte_addr_end (unsigned long addr,unsigned long end,unsigned long sz)
@@ -2007,32 +2026,20 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr,/* hugepages are never "special" */ VM_BUG_ON (!pfn_valid (pte_pfn (pte))); - refs =0 ; head =pte_page (pte); - page = head + ((addr & (sz-1 )) >>PAGE_SHIFT ); -do { -VM_BUG_ON (compound_head (page) != head); - pages[*nr] = page; - (*nr)++; - page++; - refs++; - }while (addr +=PAGE_SIZE , addr != end); + refs =record_subpages (page, addr, end, pages + *nr); head =try_get_compound_head (head, refs); -if (!head) { - *nr -= refs; +if (!head)return 0 ; - }if (unlikely (pte_val (pte) !=pte_val (*ptep))) { -/* Could be optimized better */ - *nr -= refs; -while (refs--) -put_page (head); +put_compound_head (head, refs);return 0 ; } + *nr += refs;SetPageReferenced (head);return 1 ; }
@@ -2079,28 +2086,19 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,return __gup_device_huge_pmd (orig, pmdp, addr, end, 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)return 0 ; - }if (unlikely (pmd_val (orig) !=pmd_val (*pmdp))) { - *nr -= refs; -while (refs--) -put_page (head); +put_compound_head (head, refs);return 0 ; } + *nr += refs;SetPageReferenced (head);return 1 ; }
@@ -2120,28 +2118,19 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,return __gup_device_huge_pud (orig, pudp, addr, end, 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)return 0 ; - }if (unlikely (pud_val (orig) !=pud_val (*pudp))) { - *nr -= refs; -while (refs--) -put_page (head); +put_compound_head (head, refs);return 0 ; } + *nr += refs;SetPageReferenced (head);return 1 ; }
@@ -2157,28 +2146,20 @@ static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr,return 0 ;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)return 0 ; - }if (unlikely (pgd_val (orig) !=pgd_val (*pgdp))) { - *nr -= refs; -while (refs--) -put_page (head); +put_compound_head (head, refs);return 0 ; } + *nr += refs;SetPageReferenced (head);return 1 ; }
Signature · 6 lines
2.24.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
John Hubbard
reviewerPATCH v9 2/25 mm/gup: move try_get_compound_head() to top, fix minor issues
In reply to John HubbardAn 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.
--- mm/gup.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)
mm/gup.c +15 -14
@@ -29,6 +29,21 @@ struct follow_page_context {unsigned int page_mask; }; +/* +* Return the compound head page with ref appropriately incremented, +* or NULL if that failed. +*/ +static inline struct page *try_get_compound_head (struct page *page,int refs) +{ +struct page *head =compound_head (page); + +if (WARN_ON_ONCE (page_ref_count (head) <0 )) +return NULL ; +if (unlikely (!page_cache_add_speculative (head, refs))) +return NULL ; +return head; +} +/** * put_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages * @pages: array of pages to be maybe marked dirty, and definitely released.
@@ -1807,20 +1822,6 @@ static void __maybe_unused undo_dev_pagemap(int *nr, int nr_start, } } -/* -* Return the compund head page with ref appropriately incremented, -* or NULL if that failed. -*/ -static inline struct page *try_get_compound_head (struct page *page,int refs) -{ -struct page *head =compound_head (page); -if (WARN_ON_ONCE (page_ref_count (head) <0 )) -return NULL ; -if (unlikely (!page_cache_add_speculative (head, refs))) -return NULL ; -return head; -} -#ifdef CONFIG_ARCH_HAS_PTE_SPECIAL static int gup_pte_range (pmd_t pmd,unsigned long addr,unsigned long end,unsigned int flags,struct page **pages,int *nr)
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 3/25 mm: Cleanup __put_devmap_managed_page() vs ->page_free()
In reply to John HubbardFrom: Dan Williams <[email protected]>
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.
--- drivers/nvdimm/pmem.c | 6 ---- mm/memremap.c | 80 ++++++++++++++++++++++++------------------- 2 files changed, 44 insertions(+), 42 deletions(-)
drivers/nvdimm/pmem.c +0 -6
@@ -337,13 +337,7 @@ static void pmem_release_disk(void *__pmem)put_disk (pmem->disk); } -static void pmem_pagemap_page_free (struct page *page) -{ -wake_up_var (&page->_refcount); -} -static const struct dev_pagemap_ops fsdax_pagemap_ops = { - .page_free = pmem_pagemap_page_free, .kill = pmem_pagemap_kill, .cleanup = pmem_pagemap_cleanup, };
mm/memremap.c +44 -36
95 lines not on this page · raw message
Signature · 6 lines
2.24.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
John Hubbard
reviewerPATCH v9 4/25 mm: devmap: refactor 1-based refcounting for ZONE_DEVICE pages
In reply to John HubbardAn 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.
--- include/linux/mm.h | 17 +++++++++++++---- mm/memremap.c | 16 ++-------------- mm/swap.c | 24 ++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 18 deletions(-)
include/linux/mm.h +13 -4
@@ -952,9 +952,10 @@ static inline bool is_zone_device_page(const struct page *page)#endif #ifdef CONFIG_DEV_PAGEMAP_OPS -void __put_devmap_managed_page (struct page *page); +void free_devmap_managed_page (struct page *page);DECLARE_STATIC_KEY_FALSE (devmap_managed_key); -static inline bool put_devmap_managed_page (struct page *page) + +static inline bool page_is_devmap_managed (struct page *page) {if (!static_branch_unlikely (&devmap_managed_key))return false;
@@ -963,7 +964,6 @@ static inline bool put_devmap_managed_page(struct page *page)switch (page->pgmap->type) {case MEMORY_DEVICE_PRIVATE :case MEMORY_DEVICE_FS_DAX : -__put_devmap_managed_page (page);return true;default :break ;
@@ -971,7 +971,14 @@ static inline bool put_devmap_managed_page(struct page *page) return false; } +bool put_devmap_managed_page (struct page *page); +#else /* CONFIG_DEV_PAGEMAP_OPS */ +static inline bool page_is_devmap_managed (struct page *page) +{ +return false; +} +static inline bool put_devmap_managed_page (struct page *page) {return false;
@@ -1028,8 +1035,10 @@ static inline void put_page(struct page *page)* need to inform the device driver through callback. See * include/linux/memremap.h and HMM for details. */ -if (put_devmap_managed_page (page)) +if (page_is_devmap_managed (page)) { +put_devmap_managed_page (page);return ; + }if (put_page_testzero (page))__put_page (page);
mm/memremap.c +2 -14
@@ -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 (struct page *page) +void free_devmap_managed_page (struct page *page) { -int count =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);
@@ -461,5 +449,5 @@ void __put_devmap_managed_page(struct page *page) page->mapping =NULL ; page->pgmap->ops->page_free(page); } -EXPORT_SYMBOL (__put_devmap_managed_page); +EXPORT_SYMBOL (free_devmap_managed_page);#endif /* CONFIG_DEV_PAGEMAP_OPS */
mm/swap.c +24 -0
@@ -1102,3 +1102,27 @@ void __init swap_setup(void)* _really_ don't want to cluster much more */ } + +#ifdef CONFIG_DEV_PAGEMAP_OPS +bool put_devmap_managed_page (struct page *page) +{ +bool is_devmap =page_is_devmap_managed (page); + +if (is_devmap) { +int count =page_ref_dec_return (page); + +/* +* 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; +} +EXPORT_SYMBOL (put_devmap_managed_page); +#endif
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 5/25 goldish_pipe: rename local pin_user_pages() routine
In reply to John Hubbard1. Avoid naming conflicts: rename local static function from "pin_user_pages()" to "goldfish_pin_pages()".
An upcoming patch will introduce a global pin_user_pages() function.
--- drivers/platform/goldfish/goldfish_pipe.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
drivers/platform/goldfish/goldfish_pipe.c +9 -9
@@ -257,12 +257,12 @@ static int goldfish_pipe_error_convert(int status) } } -static int pin_user_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) +static int goldfish_pin_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) {int ret;int requested_pages = ((last_page - first_page) >>PAGE_SHIFT ) +1 ;
@@ -354,9 +354,9 @@ static int transfer_max_buffers(struct goldfish_pipe *pipe,if (mutex_lock_interruptible (&pipe->lock))return -ERESTARTSYS ; - pages_count =pin_user_pages (first_page, last_page, - last_page_size, is_write, - pipe->pages, &iter_last_page_size); + pages_count =goldfish_pin_pages (first_page, last_page, + last_page_size, is_write, + pipe->pages, &iter_last_page_size);if (pages_count <0 ) {mutex_unlock (&pipe->lock);return pages_count;
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 6/25 mm: fix get_user_pages_remote()'s handling of FOLL_LONGTERM
In reply to John HubbardAs 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 to loosen the restriction, allowing FOLL_LONGTERM in some cases.
Also, copy 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(). This check will then be removed from the VFIO call site in a subsequent patch.
Thanks to Jason Gunthorpe for pointing out a clean way to fix this, and to Dan Williams for helping clarify the DAX refactoring.
--- mm/gup.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
mm/gup.c +22 -5
@@ -29,6 +29,13 @@ struct follow_page_context {unsigned int page_mask; }; +static __always_inline long __gup_longterm_locked (struct task_struct *tsk, +struct mm_struct *mm, +unsigned long start, +unsigned long nr_pages, +struct page **pages, +struct vm_area_struct **vmas, +unsigned int flags);/* * Return the compound head page with ref appropriately incremented, * or NULL if that failed.
@@ -1179,13 +1186,23 @@ long get_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm, struct vm_area_struct **vmas, int *locked) {/* -* FIXME: Current FOLL_LONGTERM behavior is incompatible with +* Parts of FOLL_LONGTERM behavior are incompatible with * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on -* vmas. As there are no users of this flag in this call we simply -* disallow this option for now. +* vmas. However, this only comes up if locked is set, and there are +* callers that do request FOLL_LONGTERM, but do not set locked. So, +* allow what we can. */ -if (WARN_ON_ONCE (gup_flags &FOLL_LONGTERM )) -return -EINVAL ; +if (gup_flags &FOLL_LONGTERM ) { +if (WARN_ON_ONCE (locked)) +return -EINVAL ; +/* +* This will check the vmas (even if our vmas arg is NULL) +* and return -ENOTSUPP if DAX isn't allowed in this case: +*/ +return __gup_longterm_locked (tsk, mm, start, nr_pages, pages, + vmas, gup_flags |FOLL_TOUCH | +FOLL_REMOTE ); + } return __get_user_pages_locked(tsk, mm, start, nr_pages, pages, vmas, locked,
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 7/25 vfio: fix FOLL_LONGTERM use, simplify get_user_pages_remote() call
In reply to John HubbardUpdate VFIO to take advantage of the recently loosened restriction on FOLL_LONGTERM with get_user_pages_remote(). Also, now it is possible to fix a bug: the VFIO caller is logically a FOLL_LONGTERM user, but it wasn't setting FOLL_LONGTERM.
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, now that the the DAX check ("if a VMA is DAX, don't allow long term pinning") is in the internals of get_user_pages_remote() and __gup_longterm_locked(), there's no need for it at the VFIO call site. So remove it.
--- drivers/vfio/vfio_iommu_type1.c | 30 +++++------------------------- 1 file changed, 5 insertions(+), 25 deletions(-)
drivers/vfio/vfio_iommu_type1.c +5 -25
@@ -322,7 +322,6 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, {struct page *page[1 ];struct vm_area_struct *vma; -struct vm_area_struct *vmas[1 ];unsigned int flags =0 ;int ret;
@@ -330,33 +329,14 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, flags |=FOLL_WRITE ;down_read (&mm->mmap_sem); -if (mm == current->mm) { - ret =get_user_pages (vaddr,1 , flags |FOLL_LONGTERM , page, - vmas); - }else { - ret =get_user_pages_remote (NULL , mm, vaddr,1 , flags, page, - vmas,NULL ); -/* -* The lifetime of a vaddr_get_pfn() page pin is -* userspace-controlled. In the fs-dax case this could -* lead to indefinite stalls in filesystem operations. -* Disallow attempts to pin fs-dax pages via this -* interface. -*/ -if (ret >0 &&vma_is_fsdax (vmas[0 ])) { - ret = -EOPNOTSUPP ; -put_page (page[0 ]); - } - } -up_read (&mm->mmap_sem); - + ret =get_user_pages_remote (NULL , mm, vaddr,1 , flags |FOLL_LONGTERM , + page,NULL ,NULL );if (ret ==1 ) { *pfn =page_to_pfn (page[0 ]); -return 0 ; + ret =0 ; + goto done; } -down_read (&mm->mmap_sem); - vaddr =untagged_addr (vaddr); vma =find_vma_intersection (mm, vaddr, vaddr +1 );
@@ -366,7 +346,7 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,if (is_invalid_reserved_pfn (*pfn)) ret =0 ; } - +done:up_read (&mm->mmap_sem);return ret; }
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 8/25 mm/gup: allow FOLL_FORCE for get_user_pages_fast()
In reply to John HubbardCommit 817be129e6f2 ("mm: validate get_user_pages_fast flags") allowed only FOLL_WRITE and FOLL_LONGTERM to be passed to get_user_pages_fast(). This, combined with the fact that get_user_pages_fast() falls back to "slow gup", which *does* accept FOLL_FORCE, leads to an odd situation:
if you need FOLL_FORCE, you cannot call get_user_pages_fast().
There does not appear to be any reason for filtering out FOLL_FORCE. There is nothing in the _fast() implementation that requires that we avoid writing to the pages. So it appears to have been an oversight.
Fix by allowing FOLL_FORCE to be set for get_user_pages_fast().
--- mm/gup.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
mm/gup.c +2 -1
@@ -2414,7 +2414,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages,unsigned long addr, len, end;int nr =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 ;
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 9/25 IB/umem: use get_user_pages_fast() to pin DMA pages
In reply to John HubbardAnd 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.
--- drivers/infiniband/core/umem.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
drivers/infiniband/core/umem.c +6 -11
@@ -266,16 +266,13 @@ struct ib_umem *ib_umem_get(struct ib_udata *udata, unsigned long addr, sg = umem->sg_head.sgl;while (npages) { -down_read (&mm->mmap_sem); - ret =get_user_pages (cur_base, - min_t(unsigned long, npages, -PAGE_SIZE /sizeof (struct page *)), - gup_flags |FOLL_LONGTERM , - page_list,NULL ); -if (ret <0 ) { -up_read (&mm->mmap_sem); + ret =get_user_pages_fast (cur_base, + min_t(unsigned long, npages, +PAGE_SIZE / +sizeof (struct page *)), + gup_flags |FOLL_LONGTERM , page_list); +if (ret <0 ) goto umem_release; - } cur_base += ret *PAGE_SIZE ; npages -= ret;
@@ -283,8 +280,6 @@ struct ib_umem *ib_umem_get(struct ib_udata *udata, unsigned long addr, sg =ib_umem_add_sg_table (sg, page_list, ret,dma_get_max_seg_size (context->device->dma_device), &umem->sg_nents); - -up_read (&mm->mmap_sem); }sg_mark_end (sg);
Signature · 6 lines
2.24.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
John Hubbard
reviewerPATCH v9 10/25 mm/gup: introduce pin_user_pages*() and FOLL_PIN
In reply to John HubbardIntroduce 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.)
--- Documentation/core-api/index.rst | 1 + Documentation/core-api/pin_user_pages.rst | 232 ++++++++++++++++++++++ include/linux/mm.h | 63 ++++-- mm/gup.c | 161 +++++++++++++-- 4 files changed, 423 insertions(+), 34 deletions(-) create mode 100644 Documentation/core-api/pin_user_pages.rst
Documentation/core-api/index.rst +1 -0
@@ -31,6 +31,7 @@ Core utilities generic-radix-tree memory-allocation mm-api + pin_user_pages gfp_mask-from-fs-io timekeeping boot-time-mm
Documentation/core-api/pin_user_pages.rst new file +232 -0
232 lines not on this page · raw message
include/linux/mm.h +50 -13
105 lines not on this page · raw message
mm/gup.c +140 -21
212 lines not on this page · raw message
Signature · 1 line
2.24.0
Jonathan Corbet
maintainerRe: [PATCH v9 10/25] mm/gup: introduce pin_user_pages*() and FOLL_PIN
Quoted 2 lines · On Tue, 10 Dec 2019 18:53:03 -0800 John Hubbard <[email protected]> wrote:
Introduce pin_user_pages*() variations of get_user_pages*() calls, and also pin_longterm_pages*() variations.
Just a couple of nits on the documentation patch
Quoted 8 lines
+++
b/Documentation/core-api/pin_user_pages.rstDocumentation/core-api/pin_user_pages.rst new file +6
-0@@ -0,0 +1,232 @@ +.. SPDX-License-Identifier: GPL-2.0 + +==================================================== +pin_user_pages() and related calls +==================================================== +
Documentation/core-api/pin_user_pages.rst line 16
Quoted 10 lines
Documentation/core-api/pin_user_pages.rst new file +10
-0+.. contents:: :local: + +Overview +======== + +This document describes the following functions: :: + + pin_user_pages + pin_user_pages_fast + pin_user_pages_remote
You could just say "the following functions::" and get the result you're after with a slightly less alien plain-text reading experience.
Of course, you could also just say "This document describes pin_user_pages(), pin_user_pages_fast(), and pin_user_pages_remote()." But that's a matter of personal taste, I guess. Using the function() notation will cause the docs system to automatically link to the kerneldoc info, though.
Quoted 20 lines
Documentation/core-api/pin_user_pages.rst new file +20
-0+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.
Documentation/core-api/pin_user_pages.rst line 47
Quoted 10 lines
Documentation/core-api/pin_user_pages.rst new file +10
-0+ +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 typoed there.
Thanks,
jon
John Hubbard
reviewerRe: [PATCH v9 10/25] mm/gup: introduce pin_user_pages*() and FOLL_PIN
Quoted 21 lines · Jonathan Corbet
Quoted 2 lines · On Tue, 10 Dec 2019 18:53:03 -0800 John Hubbard <[email protected]> wrote:
Introduce pin_user_pages*() variations of get_user_pages*() calls, and also pin_longterm_pages*() variations.
Just a couple of nits on the documentation patch
Quoted 13 lines
+++
b/Documentation/core-api/pin_user_pages.rstDocumentation/core-api/pin_user_pages.rst new file +11
-0@@ -0,0 +1,232 @@ +.. SPDX-License-Identifier: GPL-2.0 + +==================================================== +pin_user_pages() and related calls +==================================================== + +.. contents:: :local: + +Overview +======== +
Quoted 8 lines
Documentation/core-api/pin_user_pages.rst line 16
Quoted 5 lines
Documentation/core-api/pin_user_pages.rst new file +5
-0+This document describes the following functions: :: + + pin_user_pages + pin_user_pages_fast + pin_user_pages_remoteYou could just say "the following functions::" and get the result you're after with a slightly less alien plain-text reading experience.
I see. That works nicely: same result with fewer :'s.
Quoted 6 lines
Of course, you could also just say "This document describes
pin_user_pages(),pin_user_pages_fast(), andpin_user_pages_remote()." But that's a matter of personal taste, I guess. Using thefunction()notation will cause the docs system to automatically link to the kerneldoc info, though.
OK. I did try the single-sentence approach just now, but to me the one-per-line seems to make both the text and the generated HTML slightly easier to look at. Of course, like you say, different people will have different preferences. So in the end I've combined the tips, like this:
+Overview
+========
+
+This document describes the following functions::
+
+ pin_user_pages()
+ pin_user_pages_fast()
+ pin_user_pages_remote()
Quoted 25 lines
Quoted 25 lines
Documentation/core-api/pin_user_pages.rst new file +24
-0+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
Quoted 9 lines
Documentation/core-api/pin_user_pages.rst line 47
Quoted 6 lines
Documentation/core-api/pin_user_pages.rst new file +6
-0+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_LONGTERMtypoed there.
Good catch. Fixed.
thanks,
Signature · 8 lines
John Hubbard NVIDIA _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
John Hubbard
reviewerPATCH v9 11/25 goldish_pipe: convert to pin_user_pages() and put_user_page()
In reply to John Hubbard1. 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/[email protected]
--- drivers/platform/goldfish/goldfish_pipe.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
drivers/platform/goldfish/goldfish_pipe.c +3 -14
@@ -274,7 +274,7 @@ static int goldfish_pin_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 goldfish_pin_pages(unsigned long first_page,return ret; } -static void release_user_pages (struct page **pages,int pages_count, -int is_write,s32 consumed_size) -{ -int i; - -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 */ static void populate_rw_params(struct page **pages, int pages_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);return 0 ;
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 12/25 IB/{core,hw,umem}: set FOLL_PIN via pin_user_pages*(), fix up ODP
In reply to John HubbardConvert 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.
--- 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(-)
drivers/infiniband/core/umem.c +1 -1
@@ -266,7 +266,7 @@ struct ib_umem *ib_umem_get(struct ib_udata *udata, unsigned long addr, sg = umem->sg_head.sgl;while (npages) { - ret = get_user_pages_fast(cur_base, + ret = pin_user_pages_fast(cur_base, min_t(unsigned long, npages, PAGE_SIZE / sizeof(struct page *)),
drivers/infiniband/core/umem_odp.c +6 -7
@@ -308,9 +308,8 @@ EXPORT_SYMBOL(ib_umem_odp_release);* The function returns -EFAULT if the DMA mapping operation fails. It returns * -EAGAIN if a concurrent invalidation prevents us from updating the page. * -* The page is released via put_user_page even if the operation failed. For -* on-demand pinning, the page is released whenever it isn't stored in the -* umem. +* The page is released via put_page even if the operation failed. For on-demand +* pinning, the page is released whenever it isn't stored in the umem. */ static int ib_umem_odp_map_dma_single_page( struct ib_umem_odp *umem_odp,
@@ -363,7 +362,7 @@ static int ib_umem_odp_map_dma_single_page( } out: -put_user_page (page); +put_page (page);return ret; }
@@ -473,7 +472,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, ret = -EFAULT ;break ; } -put_user_page (local_page_list[j]); +put_page (local_page_list[j]);continue ; }
@@ -500,8 +499,8 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,* ib_umem_odp_map_dma_single_page(). */ if (npages - (j +1 ) >0 ) -put_user_pages (&local_page_list[j+1 ], - npages - (j +1 )); +release_pages (&local_page_list[j+1 ], + npages - (j +1 ));break ; } }
drivers/infiniband/hw/hfi1/user_pages.c +1 -1
@@ -106,7 +106,7 @@ int hfi1_acquire_user_pages(struct mm_struct *mm, unsigned long vaddr, size_t npint ret;unsigned int gup_flags =FOLL_LONGTERM | (writable ?FOLL_WRITE :0 ); - ret =get_user_pages_fast (vaddr, npages, gup_flags, pages); + ret =pin_user_pages_fast (vaddr, npages, gup_flags, pages);if (ret <0 )return ret;
drivers/infiniband/hw/mthca/mthca_memfree.c +1 -1
@@ -472,7 +472,7 @@ int mthca_map_user_db(struct mthca_dev *dev, struct mthca_uar *uar, goto out; } - ret =get_user_pages_fast (uaddr &PAGE_MASK ,1 , + ret =pin_user_pages_fast (uaddr &PAGE_MASK ,1 ,FOLL_WRITE |FOLL_LONGTERM , pages);if (ret <0 ) goto out;
drivers/infiniband/hw/qib/qib_user_pages.c +1 -1
@@ -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 );
drivers/infiniband/hw/qib/qib_user_sdma.c +1 -1
@@ -670,7 +670,7 @@ static int qib_user_sdma_pin_pages(const struct qib_devdata *dd,else j = npages; - ret =get_user_pages_fast (addr, j,FOLL_LONGTERM , pages); + ret =pin_user_pages_fast (addr, j,FOLL_LONGTERM , pages);if (ret != j) { i =0 ; j = ret;
drivers/infiniband/hw/usnic/usnic_uiom.c +1 -1
@@ -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(unsigned long, npages, PAGE_SIZE / sizeof(struct page *)), gup_flags | FOLL_LONGTERM,
drivers/infiniband/sw/siw/siw_mem.c +1 -1
@@ -426,7 +426,7 @@ struct siw_umem *siw_umem_get(u64 start, u64 len, bool writable)while (nents) {struct page **plist = &umem->page_chunk[i].plist[got]; - rv =get_user_pages (first_page_va, nents, + rv =pin_user_pages (first_page_va, nents, foll_flags |FOLL_LONGTERM , plist,NULL );if (rv <0 )
Signature · 6 lines
2.24.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
John Hubbard
reviewerPATCH v9 13/25 mm/process_vm_access: set FOLL_PIN via pin_user_pages_remote()
In reply to John HubbardConvert 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().
--- mm/process_vm_access.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)
mm/process_vm_access.c +15 -13
@@ -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 { +else copied =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)) { -int pages =min (nr_pages, max_pages_per_loop); +int pinned_pages =min (nr_pages, max_pages_per_loop);int locked =1 ;size_t bytes;
@@ -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); }return rc;
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 14/25 drm/via: set FOLL_PIN via pin_user_pages_fast()
In reply to John HubbardConvert 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().
--- drivers/gpu/drm/via/via_dmablit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/via/via_dmablit.c +1 -1
@@ -239,7 +239,7 @@ via_lock_all_dma_pages(drm_via_sg_info_t *vsg, drm_via_dmablit_t *xfer) vsg->pages =vzalloc (array_size (sizeof (struct page *), vsg->num_pages));if (NULL == vsg->pages)return -ENOMEM ; - ret =get_user_pages_fast ((unsigned long )xfer->mem_addr, + ret =pin_user_pages_fast ((unsigned long )xfer->mem_addr, vsg->num_pages, vsg->direction ==DMA_FROM_DEVICE ?FOLL_WRITE :0 , vsg->pages);
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 15/25 fs/io_uring: set FOLL_PIN via pin_user_pages()
In reply to John HubbardConvert 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().
--- fs/io_uring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/io_uring.c +1 -1
@@ -4521,7 +4521,7 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, void __user *arg, ret =0 ;down_read (¤t->mm->mmap_sem); - pret =get_user_pages (ubuf, nr_pages, + pret =pin_user_pages (ubuf, nr_pages,FOLL_WRITE |FOLL_LONGTERM , pages, vmas);if (pret == nr_pages) {
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 16/25 net/xdp: set FOLL_PIN via pin_user_pages()
In reply to John HubbardConvert 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().
--- net/xdp/xdp_umem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/xdp/xdp_umem.c +1 -1
@@ -291,7 +291,7 @@ static int xdp_umem_pin_pages(struct xdp_umem *umem)return -ENOMEM ;down_read (¤t->mm->mmap_sem); - npgs =get_user_pages (umem->address, umem->npgs, + npgs =pin_user_pages (umem->address, umem->npgs, gup_flags |FOLL_LONGTERM , &umem->pgs[0 ],NULL );up_read (¤t->mm->mmap_sem);
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 17/25 media/v4l2-core: set pages dirty upon releasing DMA buffers
In reply to John HubbardAfter 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.
--- drivers/media/v4l2-core/videobuf-dma-sg.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/media/v4l2-core/videobuf-dma-sg.c +4 -1
@@ -349,8 +349,11 @@ int videobuf_dma_free(struct videobuf_dmabuf *dma)BUG_ON (dma->sglen);if (dma->pages) { -for (i =0 ; i < dma->nr_pages; i++) +for (i =0 ; i < dma->nr_pages; i++) { +if (dma->direction ==DMA_FROM_DEVICE ) +set_page_dirty_lock (dma->pages[i]);put_page (dma->pages[i]); + }kfree (dma->pages); dma->pages =NULL ; }
Signature · 6 lines
2.24.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
John Hubbard
reviewerPATCH v9 18/25 media/v4l2-core: pin_user_pages (FOLL_PIN) and put_user_page() conversion
In reply to John Hubbard1. 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().
--- drivers/media/v4l2-core/videobuf-dma-sg.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
drivers/media/v4l2-core/videobuf-dma-sg.c +4 -7
@@ -183,12 +183,12 @@ static int videobuf_dma_init_user_locked(struct videobuf_dmabuf *dma,dprintk (1 ,"init user [0x%lx+0x%lx => %d pages]\n" , data, size, dma->nr_pages); - err =get_user_pages (data &PAGE_MASK , dma->nr_pages, + err =pin_user_pages (data &PAGE_MASK , dma->nr_pages, flags |FOLL_LONGTERM , dma->pages,NULL );if (err != dma->nr_pages) { dma->nr_pages = (err >=0 ) ? err :0 ; -dprintk (1 ,"get_user_pages: err=%d [%d]\n" , err, +dprintk (1 ,"pin_user_pages: err=%d [%d]\n" , err, dma->nr_pages);return err <0 ? err : -EINVAL ; }
@@ -349,11 +349,8 @@ int videobuf_dma_free(struct videobuf_dmabuf *dma)BUG_ON (dma->sglen);if (dma->pages) { -for (i =0 ; i < dma->nr_pages; i++) { -if (dma->direction ==DMA_FROM_DEVICE ) -set_page_dirty_lock (dma->pages[i]); -put_page (dma->pages[i]); - } +put_user_pages_dirty_lock (dma->pages, dma->nr_pages, + dma->direction ==DMA_FROM_DEVICE );kfree (dma->pages); dma->pages =NULL ; }
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 19/25 vfio, mm: pin_user_pages (FOLL_PIN) and put_user_page() conversion
In reply to John Hubbard1. Change vfio from get_user_pages_remote(), topin_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/[email protected]
--- drivers/vfio/vfio_iommu_type1.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/vfio/vfio_iommu_type1.c +3 -4
@@ -309,9 +309,8 @@ static int put_pfn(unsigned long pfn, int prot) {if (!is_invalid_reserved_pfn (pfn)) {struct page *page =pfn_to_page (pfn); -if (prot &IOMMU_WRITE ) -SetPageDirty (page); -put_page (page); + +put_user_pages_dirty_lock (&page,1 , prot &IOMMU_WRITE );return 1 ; }return 0 ;
@@ -329,7 +328,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 ]);
Signature · 1 line
2.24.0
John Hubbard
reviewerPATCH v9 20/25 powerpc: book3s64: convert to pin_user_pages() and put_user_page()
In reply to John Hubbard1. Convert from get_user_pages() to pin_user_pages().
2. As required by pin_user_pages(), release these pages via put_user_page().
--- arch/powerpc/mm/book3s64/iommu_api.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
arch/powerpc/mm/book3s64/iommu_api.c +5 -5
@@ -103,7 +103,7 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua,for (entry =0 ; entry < entries; entry += chunk) {unsigned long n =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,return 0 ; 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);
@@ -215,7 +214,8 @@ static void mm_iommu_unpin(struct mm_iommu_table_group_mem_t *mem)if (mem->hpas[i] &MM_IOMMU_TABLE_GROUP_PAGE_DIRTY )SetPageDirty (page); -put_page (page); +put_user_page (page); + mem->hpas[i] =0 ; } }
Signature · 6 lines
2.24.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v9 20/25] powerpc: book3s64: convert to pin_user_pages() and put_user_page()
Quoted 7 lines · John Hubbard
1. Convert from
get_user_pages()topin_user_pages().2. As required by
pin_user_pages(), release these pages viaput_user_page().Cc Jan Kara <[email protected]>Signed-off-by John Hubbard <[email protected]>
The patch looks good to me. You can add:
I'd just note that mm_iommu_do_alloc() has a pre-existing bug that the last jump to 'free_exit' (at line 157) happens already after converting page pointers to physical addresses so put_page() calls there will just crash. But that's completely unrelated to your change. I'll send a fix separately.
Honza
Quoted 42 lines
--- arch/powerpc/mm/book3s64/iommu_api.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)arch/powerpc/mm/book3s64/iommu_api.c +5
-5@@ -103,7 +103,7 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua,for (entry =0 ; entry < entries; entry += chunk) {unsigned long n =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,return 0 ; 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);@@ -215,7 +214,8 @@ static void mm_iommu_unpin(struct mm_iommu_table_group_mem_t *mem)if (mem->hpas[i] &MM_IOMMU_TABLE_GROUP_PAGE_DIRTY )SetPageDirty (page); -put_page (page); +put_user_page (page); + mem->hpas[i] =0 ; } }Signature · 1 line
2.24.0
Signature · 6 lines
Jan Kara <[email protected]> SUSE Labs, CR _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
John Hubbard
reviewerPATCH v9 21/25 mm/gup_benchmark: use proper FOLL_WRITE flags instead of hard-coding "1"
In reply to John HubbardFix 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.
--- mm/gup_benchmark.c | 9 ++++++--- tools/testing/selftests/vm/gup_benchmark.c | 6 +++++- 2 files changed, 11 insertions(+), 4 deletions(-)
mm/gup_benchmark.c +6 -3
@@ -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) {case GUP_FAST_BENCHMARK : - nr =get_user_pages_fast (addr, nr, gup->flags &1 , + nr =get_user_pages_fast (addr, nr, gup->flags, pages + i);break ;case GUP_LONGTERM_BENCHMARK : nr =get_user_pages (addr, nr, - (gup->flags &1 ) |FOLL_LONGTERM , + gup->flags |FOLL_LONGTERM , pages + i,NULL );break ;case GUP_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 :
tools/testing/selftests/vm/gup_benchmark.c +5 -1
@@ -18,6 +18,9 @@#define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)#define GUP_BENCHMARK _IOWR('g', 3, struct gup_benchmark) +/* Just the flags we need, copied from mm.h: */ +#define FOLL_WRITE 0x01/* check pte is writable */ +struct gup_benchmark {__u64 get_delta_usec;__u64 put_delta_usec;
@@ -85,7 +88,8 @@ int main(int argc, char **argv) } gup.nr_pages_per_call = nr_pages; - gup.flags = write; +if (write) + gup.flags |=FOLL_WRITE ; fd =open ("/sys/kernel/debug/gup_benchmark" ,O_RDWR );if (fd ==-1 )
Signature · 6 lines
2.24.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
John Hubbard
reviewerPATCH v9 22/25 mm, tree-wide: rename put_user_page*() to unpin_user_page*()
In reply to John HubbardIn order to provide a clearer, more symmetric API for pinning and unpinning DMA pages. This way, pin_user_pages*() calls match up with unpin_user_pages*() calls, and the API is a lot closer to being self-explanatory.
--- Documentation/core-api/pin_user_pages.rst | 2 +- arch/powerpc/mm/book3s64/iommu_api.c | 4 +-- drivers/gpu/drm/via/via_dmablit.c | 4 +-- drivers/infiniband/core/umem.c | 2 +- drivers/infiniband/hw/hfi1/user_pages.c | 2 +- drivers/infiniband/hw/mthca/mthca_memfree.c | 6 ++-- drivers/infiniband/hw/qib/qib_user_pages.c | 2 +- drivers/infiniband/hw/qib/qib_user_sdma.c | 6 ++-- drivers/infiniband/hw/usnic/usnic_uiom.c | 2 +- drivers/infiniband/sw/siw/siw_mem.c | 2 +- drivers/media/v4l2-core/videobuf-dma-sg.c | 4 +-- drivers/platform/goldfish/goldfish_pipe.c | 4 +-- drivers/vfio/vfio_iommu_type1.c | 2 +- fs/io_uring.c | 4 +-- include/linux/mm.h | 26 ++++++++--------- mm/gup.c | 32 ++++++++++----------- mm/process_vm_access.c | 4 +-- net/xdp/xdp_umem.c | 2 +- 18 files changed, 55 insertions(+), 55 deletions(-)
Documentation/core-api/pin_user_pages.rst +1 -1
@@ -219,7 +219,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
arch/powerpc/mm/book3s64/iommu_api.c +2 -2
@@ -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);
@@ -214,7 +214,7 @@ static void mm_iommu_unpin(struct mm_iommu_table_group_mem_t *mem)if (mem->hpas[i] &MM_IOMMU_TABLE_GROUP_PAGE_DIRTY )SetPageDirty (page); -put_user_page (page); +unpin_user_page (page); mem->hpas[i] =0 ; }
drivers/gpu/drm/via/via_dmablit.c +2 -2
@@ -188,8 +188,8 @@ via_free_sg_info(struct pci_dev *pdev, drm_via_sg_info_t *vsg)kfree (vsg->desc_pages);/* fall through */ case dr_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 */ case dr_via_pages_alloc:vfree (vsg->pages);
drivers/infiniband/core/umem.c +1 -1
@@ -54,7 +54,7 @@ static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int dfor_each_sg_page (umem->sg_head.sgl, &sg_iter, umem->sg_nents,0 ) { page =sg_page_iter_page (&sg_iter); -put_user_pages_dirty_lock (&page,1 , umem->writable && dirty); +unpin_user_pages_dirty_lock (&page,1 , umem->writable && dirty); }sg_free_table (&umem->sg_head);
drivers/infiniband/hw/hfi1/user_pages.c +1 -1
@@ -118,7 +118,7 @@ int hfi1_acquire_user_pages(struct mm_struct *mm, unsigned long vaddr, size_t npvoid hfi1_release_user_pages (struct mm_struct *mm,struct page **p,size_t npages,bool dirty) { -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);
drivers/infiniband/hw/mthca/mthca_memfree.c +3 -3
@@ -482,7 +482,7 @@ int mthca_map_user_db(struct mthca_dev *dev, struct mthca_uar *uar, ret =pci_map_sg (dev->pdev, &db_tab->page[i].mem,1 ,PCI_DMA_TODEVICE );if (ret <0 ) { -put_user_page (pages[0 ]); +unpin_user_page (pages[0 ]); goto out; }
@@ -490,7 +490,7 @@ int mthca_map_user_db(struct mthca_dev *dev, struct mthca_uar *uar, mthca_uarc_virt(dev, uar, i));if (ret) {pci_unmap_sg (dev->pdev, &db_tab->page[i].mem,1 ,PCI_DMA_TODEVICE ); -put_user_page (sg_page (&db_tab->page[i].mem)); +unpin_user_page (sg_page (&db_tab->page[i].mem)); goto out; }
@@ -556,7 +556,7 @@ void mthca_cleanup_user_db_tab(struct mthca_dev *dev, struct mthca_uar *uar,if (db_tab->page[i].uvirt) {mthca_UNMAP_ICM (dev,mthca_uarc_virt (dev, uar, i),1 );pci_unmap_sg (dev->pdev, &db_tab->page[i].mem,1 ,PCI_DMA_TODEVICE ); -put_user_page (sg_page (&db_tab->page[i].mem)); +unpin_user_page (sg_page (&db_tab->page[i].mem)); } }
drivers/infiniband/hw/qib/qib_user_pages.c +1 -1
@@ -40,7 +40,7 @@static void __qib_release_user_pages (struct page **p,size_t num_pages,int dirty) { -put_user_pages_dirty_lock (p, num_pages, dirty); +unpin_user_pages_dirty_lock (p, num_pages, dirty); }/**
drivers/infiniband/hw/qib/qib_user_sdma.c +3 -3
@@ -317,7 +317,7 @@ static int qib_user_sdma_page_to_frags(const struct qib_devdata *dd,* the caller can ignore this page. */ if (put) { -put_user_page (page); +unpin_user_page (page); }else {/* coalesce case */ kunmap (page);
@@ -631,7 +631,7 @@ static void qib_user_sdma_free_pkt_frag(struct device *dev,kunmap (pkt->addr[i].page);if (pkt->addr[i].put_page) -put_user_page (pkt->addr[i].page); +unpin_user_page (pkt->addr[i].page);else __free_page (pkt->addr[i].page); }else if (pkt->addr[i].kvaddr) {
@@ -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:return ret;
drivers/infiniband/hw/usnic/usnic_uiom.c +1 -1
@@ -75,7 +75,7 @@ static void usnic_uiom_put_pages(struct list_head *chunk_list, int dirty)for_each_sg (chunk->page_list, sg, chunk->nents, i) { page =sg_page (sg); pa =sg_phys (sg); -put_user_pages_dirty_lock (&page,1 , dirty); +unpin_user_pages_dirty_lock (&page,1 , dirty);usnic_dbg ("pa: %pa\n" , &pa); }kfree (chunk);
drivers/infiniband/sw/siw/siw_mem.c +1 -1
@@ -63,7 +63,7 @@ struct siw_mem *siw_mem_id2obj(struct siw_device *sdev, int stag_index)static void siw_free_plist (struct siw_page_chunk *chunk,int num_pages,bool dirty) { -put_user_pages_dirty_lock (chunk->plist, num_pages, dirty); +unpin_user_pages_dirty_lock (chunk->plist, num_pages, dirty); }void siw_umem_release (struct siw_umem *umem,bool dirty)
drivers/media/v4l2-core/videobuf-dma-sg.c +2 -2
@@ -349,8 +349,8 @@ int videobuf_dma_free(struct videobuf_dmabuf *dma)BUG_ON (dma->sglen);if (dma->pages) { -put_user_pages_dirty_lock (dma->pages, dma->nr_pages, - dma->direction ==DMA_FROM_DEVICE ); +unpin_user_pages_dirty_lock (dma->pages, dma->nr_pages, + dma->direction ==DMA_FROM_DEVICE );kfree (dma->pages); dma->pages =NULL ; }
drivers/platform/goldfish/goldfish_pipe.c +2 -2
@@ -360,8 +360,8 @@ static int transfer_max_buffers(struct goldfish_pipe *pipe, *consumed_size = pipe->command_buffer->rw_params.consumed_size; -put_user_pages_dirty_lock (pipe->pages, pages_count, - !is_write && *consumed_size >0 ); +unpin_user_pages_dirty_lock (pipe->pages, pages_count, + !is_write && *consumed_size >0 );mutex_unlock (&pipe->lock);return 0 ;
drivers/vfio/vfio_iommu_type1.c +1 -1
@@ -310,7 +310,7 @@ static int put_pfn(unsigned long pfn, int prot)if (!is_invalid_reserved_pfn (pfn)) {struct page *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 );return 1 ; }return 0 ;
fs/io_uring.c +2 -2
@@ -4400,7 +4400,7 @@ static int io_sqe_buffer_unregister(struct io_ring_ctx *ctx)struct io_mapped_ubuf *imu = &ctx->user_bufs[i];for (j =0 ; j < imu->nr_bvecs; j++) -put_user_page (imu->bvec[j].bv_page); +unpin_user_page (imu->bvec[j].bv_page);if (ctx->account_mem)io_unaccount_mem (ctx->user, imu->nr_bvecs);
@@ -4545,7 +4545,7 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, void __user *arg,* release any pages we did get */ if (pret >0 ) -put_user_pages (pages, pret); +unpin_user_pages (pages, pret);if (ctx->account_mem)io_unaccount_mem (ctx->user, nr_pages);kvfree (imu->bvec);
include/linux/mm.h +13 -13
@@ -1045,27 +1045,27 @@ static inline void put_page(struct page *page) }/** -* put_user_page() - release a gup-pinned page +* unpin_user_page() - release a gup-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 -* eventually such pages can be separately tracked and uniquely handled. In +* unpin_user_page(), or one of the unpin_user_pages*() routines. This is so +* that eventually such pages can be separately tracked and uniquely handled. In * particular, interactions with RDMA and filesystems need special handling. * -* put_user_page() and put_page() are not interchangeable, despite this early -* implementation that makes them look the same. put_user_page() calls must +* unpin_user_page() and put_page() are not interchangeable, despite this early +* implementation that makes them look the same. unpin_user_page() calls must * be perfectly matched up with pin*() calls. */ -static inline void put_user_page (struct page *page) +static inline void unpin_user_page (struct page *page) {put_page (page); } -void put_user_pages_dirty_lock (struct page **pages,unsigned long npages, -bool make_dirty); +void unpin_user_pages_dirty_lock (struct page **pages,unsigned long npages, +bool make_dirty); -void put_user_pages (struct page **pages,unsigned long npages); +void unpin_user_pages (struct page **pages,unsigned long npages);#if defined(CONFIG_SPARSEMEM ) && !defined(CONFIG_SPARSEMEM_VMEMMAP )#define SECTION_IN_PAGE_FLAGS
@@ -2595,7 +2595,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_PIN and FOLL_LONGTERM may be used in various combinations with each
@@ -2630,7 +2630,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,* Direct IO). This lets the filesystem know that some non-file-system entity is * potentially changing the pages' data. In contrast to FOLL_GET (whose pages * are released via put_page()), FOLL_PIN pages must be released, ultimately, by -* a call to put_user_page(). +* a call to unpin_user_page(). * * FOLL_PIN is similar to FOLL_GET: both of these pin pages. They use different * and separate refcounting mechanisms, however, and that means that each has
@@ -2638,7 +2638,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,* * FOLL_GET: get_user_pages*() to acquire, and put_page() to release. * -* FOLL_PIN: pin_user_pages*() to acquire, and put_user_pages to release. +* FOLL_PIN: pin_user_pages*() to acquire, and unpin_user_pages to release. * * FOLL_PIN and FOLL_GET are mutually exclusive for a given function call. * (The underlying pages may experience both FOLL_GET-based and FOLL_PIN-based
@@ -2648,7 +2648,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,* FOLL_PIN should be set internally by the pin_user_pages*() APIs, never * directly by the caller. That's in order to help avoid mismatches when * releasing pages: get_user_pages*() pages must be released via put_page(), -* while pin_user_pages*() pages must be released via put_user_page(). +* while pin_user_pages*() pages must be released via unpin_user_page(). * * Please see Documentation/vm/pin_user_pages.rst for more information. */
mm/gup.c +16 -16
@@ -52,7 +52,7 @@ static inline struct page *try_get_compound_head(struct page *page, int refs) }/** -* put_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages +* unpin_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages * @pages: array of pages to be maybe marked dirty, and definitely released. * @npages: number of pages in the @pages array. * @make_dirty: whether to mark the pages dirty
@@ -62,19 +62,19 @@ static inline struct page *try_get_compound_head(struct page *page, int refs)* * For each page in the @pages array, make that page (or its head page, if a * compound page) dirty, if @make_dirty is true, and if the page was previously -* listed as clean. In any case, releases all pages using put_user_page(), -* possibly via put_user_pages(), for the non-dirty case. +* listed as clean. In any case, releases all pages using unpin_user_page(), +* possibly via unpin_user_pages(), for the non-dirty case. * -* Please see the put_user_page() documentation for details. +* Please see the unpin_user_page() documentation for details. * * set_page_dirty_lock() is used internally. If instead, set_page_dirty() is * required, then the caller should a) verify that this is really correct, * because _lock() is usually required, and b) hand code it: -* set_page_dirty_lock(), put_user_page(). +* set_page_dirty_lock(), unpin_user_page(). * */ -void put_user_pages_dirty_lock (struct page **pages,unsigned long npages, -bool make_dirty) +void unpin_user_pages_dirty_lock (struct page **pages,unsigned long npages, +bool make_dirty) {unsigned long index;
@@ -85,7 +85,7 @@ void put_user_pages_dirty_lock(struct page **pages, unsigned long npages,*/ if (!make_dirty) { -put_user_pages (pages, npages); +unpin_user_pages (pages, npages);return ; }
@@ -113,21 +113,21 @@ void put_user_pages_dirty_lock(struct page **pages, unsigned long npages,*/ if (!PageDirty (page))set_page_dirty_lock (page); -put_user_page (page); +unpin_user_page (page); } } -EXPORT_SYMBOL (put_user_pages_dirty_lock); +EXPORT_SYMBOL (unpin_user_pages_dirty_lock);/** -* put_user_pages() - release an array of gup-pinned pages. +* unpin_user_pages() - release an array of gup-pinned pages. * @pages: array of pages to be marked dirty and released. * @npages: number of pages in the @pages array. * -* For each page in the @pages array, release the page using put_user_page(). +* For each page in the @pages array, release the page using unpin_user_page(). * -* Please see the put_user_page() documentation for details. +* Please see the unpin_user_page() documentation for details. */ -void put_user_pages (struct page **pages,unsigned long npages) +void unpin_user_pages (struct page **pages,unsigned long npages) {unsigned long index;
@@ -137,9 +137,9 @@ void put_user_pages(struct page **pages, unsigned long npages)* single operation to the head page should suffice. */ for (index =0 ; index < npages; index++) -put_user_page (pages[index]); +unpin_user_page (pages[index]); } -EXPORT_SYMBOL (put_user_pages); +EXPORT_SYMBOL (unpin_user_pages);#ifdef CONFIG_MMU static struct page *no_page_table(struct vm_area_struct *vma,
mm/process_vm_access.c +2 -2
@@ -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); }return rc;
net/xdp/xdp_umem.c +1 -1
@@ -212,7 +212,7 @@ static int xdp_umem_map_pages(struct xdp_umem *umem)static void xdp_umem_unpin_pages (struct xdp_umem *umem) { -put_user_pages_dirty_lock (umem->pgs, umem->npgs, true); +unpin_user_pages_dirty_lock (umem->pgs, umem->npgs, true);kfree (umem->pgs); umem->pgs =NULL ;
Signature · 6 lines
2.24.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
John Hubbard
reviewerPATCH v9 23/25 mm/gup: track FOLL_PIN pages
In reply to John HubbardAdd 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 unpin_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/
--- Documentation/core-api/pin_user_pages.rst | 2 +- include/linux/mm.h | 77 ++++- include/linux/mmzone.h | 2 + include/linux/page_ref.h | 10 + mm/gup.c | 369 ++++++++++++++++------ mm/huge_memory.c | 26 +- mm/hugetlb.c | 25 +- mm/vmstat.c | 2 + 8 files changed, 397 insertions(+), 116 deletions(-)
Documentation/core-api/pin_user_pages.rst +1 -1
@@ -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 --------
include/linux/mm.h +62 -15
103 lines not on this page · raw message
include/linux/mmzone.h +2 -0
@@ -244,6 +244,8 @@ enum node_stat_item {NR_DIRTIED ,/* page dirtyings since bootup */ NR_WRITTEN ,/* page writings since bootup */ NR_KERNEL_MISC_RECLAIMABLE ,/* reclaimable non-slab kernel pages */ +NR_FOLL_PIN_REQUESTED ,/* via: pin_user_page(), gup flag: FOLL_PIN */ +NR_FOLL_PIN_RETURNED ,/* pages returned via unpin_user_page() */ NR_VM_NODE_STAT_ITEMS };
include/linux/page_ref.h +10 -0
@@ -102,6 +102,16 @@ static inline void page_ref_sub(struct page *page, int nr)__page_ref_mod (page, -nr); } +static inline int page_ref_sub_return (struct page *page,int nr) +{ +int ret =atomic_sub_return (nr, &page->_refcount); + +if (page_ref_tracepoint_active (__tracepoint_page_ref_mod)) +__page_ref_mod (page, -nr); + +return ret; +} +static inline void page_ref_inc (struct page *page) {atomic_inc (&page->_refcount);
mm/gup.c +280 -89
596 lines not on this page · raw message
mm/huge_memory.c +20 -6
@@ -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 ))) +return NULL ; +if (flags &FOLL_WRITE && !pmd_write (*pmd))return NULL ;
@@ -960,7 +965,7 @@ struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long addr,* device mapped pages can only be returned if the * caller will manage the page reference count. */ -if (!(flags &FOLL_GET )) +if (!(flags & (FOLL_GET |FOLL_PIN )))return ERR_PTR (-EEXIST ); pfn += (addr & ~PMD_MASK ) >>PAGE_SHIFT ;
@@ -968,7 +973,8 @@ struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long addr,if (!*pgmap)return ERR_PTR (-EFAULT ); page =pfn_to_page (pfn); -get_page (page); +if (!try_grab_page (page, flags)) + page =ERR_PTR (-EFAULT );return page; }
@@ -1088,6 +1094,11 @@ struct page *follow_devmap_pud(struct vm_area_struct *vma, unsigned long addr,if (flags &FOLL_WRITE && !pud_write (*pud))return NULL ; +/* FOLL_GET and FOLL_PIN are mutually exclusive. */ +if (WARN_ON_ONCE ((flags & (FOLL_PIN |FOLL_GET )) == + (FOLL_PIN |FOLL_GET ))) +return NULL ; +if (pud_present (*pud) &&pud_devmap (*pud))/* pass */ ;else
@@ -1099,8 +1110,10 @@ struct page *follow_devmap_pud(struct vm_area_struct *vma, unsigned long addr,/* * device mapped pages can only be returned if the * caller will manage the page reference count. +* +* At least one of FOLL_GET | FOLL_PIN must be set, so assert that here: */ -if (!(flags &FOLL_GET )) +if (!(flags & (FOLL_GET |FOLL_PIN )))return ERR_PTR (-EEXIST ); pfn += (addr & ~PUD_MASK ) >>PAGE_SHIFT ;
@@ -1108,7 +1121,8 @@ struct page *follow_devmap_pud(struct vm_area_struct *vma, unsigned long addr,if (!*pgmap)return ERR_PTR (-EFAULT ); page =pfn_to_page (pfn); -get_page (page); +if (!try_grab_page (page, flags)) + page =ERR_PTR (-EFAULT );return page; }
@@ -1522,8 +1536,8 @@ struct page *follow_trans_huge_pmd(struct vm_area_struct *vma, skip_mlock: page += (addr & ~HPAGE_PMD_MASK ) >>PAGE_SHIFT ;VM_BUG_ON_PAGE (!PageCompound (page) && !is_zone_device_page (page), page); -if (flags &FOLL_GET ) -get_page (page); +if (!try_grab_page (page, flags)) + page =ERR_PTR (-EFAULT ); out:return page;
mm/hugetlb.c +20 -5
@@ -4356,7 +4356,13 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma, same_page:if (pages) { pages[i] =mem_map_offset (page, pfn_offset); -get_page (pages[i]); +if (!try_grab_page (pages[i], flags)) { +spin_unlock (ptl); + remainder =0 ; + err = -ENOMEM ; +WARN_ON_ONCE (1 ); +break ; + } }if (vmas)
@@ -4916,6 +4922,12 @@ follow_huge_pmd(struct mm_struct *mm, unsigned long address,struct page *page =NULL ;spinlock_t *ptl;pte_t pte; + +/* FOLL_GET and FOLL_PIN are mutually exclusive. */ +if (WARN_ON_ONCE ((flags & (FOLL_PIN |FOLL_GET )) == + (FOLL_PIN |FOLL_GET ))) +return NULL ; + retry: ptl =pmd_lockptr (mm, pmd);spin_lock (ptl);
@@ -4928,8 +4940,11 @@ 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); +if (unlikely (!try_grab_page (page, flags))) { +WARN_ON_ONCE (1 ); + page =NULL ; + goto out; + } }else {if (is_hugetlb_entry_migration (pte)) {spin_unlock (ptl);
@@ -4950,7 +4965,7 @@ struct page * __weak follow_huge_pud(struct mm_struct *mm, unsigned long address, pud_t *pud, int flags) { - if (flags & FOLL_GET) + if (flags & (FOLL_GET | FOLL_PIN)) return NULL;return pte_page (*(pte_t *)pud) + ((address & ~PUD_MASK ) >>PAGE_SHIFT );
@@ -4959,7 +4974,7 @@ follow_huge_pud(struct mm_struct *mm, unsigned long address,struct page *__weak follow_huge_pgd (struct mm_struct *mm,unsigned long address,pgd_t *pgd,int flags) { -if (flags &FOLL_GET ) +if (flags & (FOLL_GET |FOLL_PIN ))return NULL ;return pte_page (*(pte_t *)pgd) + ((address & ~PGDIR_MASK ) >>PAGE_SHIFT );
mm/vmstat.c +2 -0
@@ -1168,6 +1168,8 @@ const char * const vmstat_text[] = {"nr_dirtied" ,"nr_written" ,"nr_kernel_misc_reclaimable" , +"nr_foll_pin_requested" , +"nr_foll_pin_returned" ,/* enum writeback_stat_item counters */ "nr_dirty_threshold",
Signature · 6 lines
2.24.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Jan Kara
reviewerRe: [PATCH v9 23/25] mm/gup: track FOLL_PIN pages
Quoted 15 lines · John Hubbard
Add tracking of pages that were pinned via
FOLL_PIN.As mentioned in the
FOLL_PINdocumentation, callers who effectively setFOLL_PINare required to ultimately free such pages viaunpin_user_page(). The effect is similar toFOLL_GET, and may be thought of as "FOLL_GETfor DIO and/or RDMA use".Pages that have been pinned via
FOLL_PINare 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].
Quoted 8 lines
This also changes a
BUG_ON(), to aWARN_ON(), infollow_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/
The patch looks mostly good to me now. Just a few smaller comments below.
Quoted 5 lines
Suggested-by Jan Kara <[email protected]>Suggested-by Jérôme Glisse <[email protected]>Reviewed-by Jan Kara <[email protected]>Reviewed-by Jérôme Glisse <[email protected]>Reviewed-by Ira Weiny <[email protected]>
I think you inherited here the Reviewed-by tags from the "add flags" patch you've merged into this one but that's not really fair since this patch does much more... In particular I didn't give my Reviewed-by tag for this patch yet.
Quoted 9 lines
mm/gup.c +9
-0+/* +* try_grab_compound_head() - attempt to elevate a page's refcount, by a +* flags-dependent amount. +* +* This has a default assumption of "use FOLL_GET behavior, if FOLL_PIN is not +* set". +* +* "grab" names in this file mean, "look at flags to decide whether to use +* FOLL_PIN or FOLL_GET behavior, when incrementing the page's refcount.
mm/gup.c static inline struct page *try_get_compound_head(struct page *page, int refs) · line 107
Quoted 10 lines
mm/gup.c +10
-0+*/ +static __maybe_unused struct page *try_grab_compound_head (struct page *page, +int refs, +unsigned int flags) +{ +if (flags &FOLL_PIN ) +return try_pin_compound_head (page, refs); + +return try_get_compound_head (page, refs); +}
I somewhat wonder about the asymmetry of try_grab_compound_head() vs try_grab_page() in the treatment of 'flags'. How costly would it be to make them symmetric (i.e., either set FOLL_GET for try_grab_compound_head() callers or make sure one of FOLL_GET, FOLL_PIN is set for try_grab_page())?
Because this difference looks like a subtle catch in the long run...
Quoted 40 lines
mm/gup.c +40
-0+ +/** +* try_grab_page() - elevate a page's refcount by a flag-dependent amount +* +* This might not do anything at all, depending on the flags argument. +* +* "grab" names in this file mean, "look at flags to decide whether to use +* FOLL_PIN or FOLL_GET behavior, when incrementing the page's refcount. +* +* @page: pointer to page to be grabbed +* @flags: gup flags: these are the FOLL_* flag values. +* +* Either FOLL_PIN or FOLL_GET (or neither) may be set, but not both at the same +* time. (That's true throughout the get_user_pages*() and pin_user_pages*() +* APIs.) Cases: +* +* FOLL_GET: page's refcount will be incremented by 1. +* FOLL_PIN: page's refcount will be incremented by GUP_PIN_COUNTING_BIAS. +* +* Return: true for success, or if no action was required (if neither FOLL_PIN +* nor FOLL_GET was set, nothing is done). False for failure: FOLL_GET or +* FOLL_PIN was set, but the page could not be grabbed. +*/ +bool __must_check try_grab_page (struct page *page,unsigned int flags) +{ +if (flags &FOLL_GET ) +return try_get_page (page); +else if (flags &FOLL_PIN ) { + page =compound_head (page); +WARN_ON_ONCE (flags &FOLL_GET ); + +if (WARN_ON_ONCE (page_ref_zero_or_close_to_bias_overflow (page))) +return false; + +page_ref_add (page,GUP_PIN_COUNTING_BIAS ); +__update_proc_vmstat (page,NR_FOLL_PIN_REQUESTED ,1 ); + } + +return true; +}
...
mm/huge_memory.c struct page *follow_trans_huge_pmd(struct vm_area_struct *vma, · line 1540
Quoted 8 lines
mm/huge_memory.c +2
-2@@ -1522,8 +1536,8 @@ struct page *follow_trans_huge_pmd(struct vm_area_struct *vma, skip_mlock: page += (addr & ~HPAGE_PMD_MASK ) >>PAGE_SHIFT ;VM_BUG_ON_PAGE (!PageCompound (page) && !is_zone_device_page (page), page); -if (flags &FOLL_GET ) -get_page (page); +if (!try_grab_page (page, flags)) + page =ERR_PTR (-EFAULT );
I think you need to also move the try_grab_page() earlier in the function. At this point the page may be marked as mlocked and you'd need to undo that in case try_grab_page() fails.
Quoted 7 lines
mm/hugetlb.c +0
-0@@ -4356,7 +4356,13 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma, same_page:if (pages) {
mm/hugetlb.c long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma, · line 4366
Quoted 10 lines
mm/hugetlb.c +7
-1pages[i] =mem_map_offset (page, pfn_offset); -get_page (pages[i]); +if (!try_grab_page (pages[i], flags)) { +spin_unlock (ptl); + remainder =0 ; + err = -ENOMEM ; +WARN_ON_ONCE (1 ); +break ; + } }
This function does a refcount overflow check early so that it doesn't have to do try_get_page() here. So that check can be now removed when you do try_grab_page() here anyway since that early check seems to be just a tiny optimization AFAICT.
Honza
Signature · 6 lines
Jan Kara <[email protected]> SUSE Labs, CR _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
John Hubbard
reviewerRe: [PATCH v9 23/25] mm/gup: track FOLL_PIN pages
On 12/11/19 3:28 AM, Jan Kara wrote:
...
Quoted 9 lines
The patch looks mostly good to me now. Just a few smaller comments below.
Quoted 5 lines
Suggested-by Jan Kara <[email protected]>Suggested-by Jérôme Glisse <[email protected]>Reviewed-by Jan Kara <[email protected]>Reviewed-by Jérôme Glisse <[email protected]>Reviewed-by Ira Weiny <[email protected]>
Quoted 4 lines
I think you inherited here the Reviewed-by tags from the "add flags" patch you've merged into this one but that's not really fair since this patch does much more... In particular I didn't give my Reviewed-by tag for this patch yet.
OK, I've removed those reviewed-by's. (I felt bad about dropping them, after people had devoted time to reviewing, but I do see that it's wrong to imply that they've reviewed this much much larger thing.)
...
Quoted 7 lines
I somewhat wonder about the asymmetry of
try_grab_compound_head()vstry_grab_page()in the treatment of 'flags'. How costly would it be to make them symmetric (i.e., either setFOLL_GETfortry_grab_compound_head()callers or make sure one ofFOLL_GET,FOLL_PINis set fortry_grab_page())?Because this difference looks like a subtle catch in the long run...
Done. It is only a modest code-level change, at least the way I've done it, which is setting FOLL_GET for try_grab_compound_head(). In order to do that, I set it at the top of the internal gup fast calling stacks, which is actually a good design anyway: gup fast is logically doing FOLL_GET in all cases. So setting the flag internally is accurate and consistent with the overall design.
Quoted 14 lines
...
mm/huge_memory.c struct page *follow_trans_huge_pmd(struct vm_area_struct *vma, · line 1540
Quoted 8 lines
mm/huge_memory.c +2
-2@@ -1522,8 +1536,8 @@ struct page *follow_trans_huge_pmd(struct vm_area_struct *vma, skip_mlock: page += (addr & ~HPAGE_PMD_MASK ) >>PAGE_SHIFT ;VM_BUG_ON_PAGE (!PageCompound (page) && !is_zone_device_page (page), page); -if (flags &FOLL_GET ) -get_page (page); +if (!try_grab_page (page, flags)) + page =ERR_PTR (-EFAULT );I think you need to also move the
try_grab_page()earlier in the function. At this point the page may be marked as mlocked and you'd need to undo that in casetry_grab_page()fails.
OK, I've moved it up, adding a "subpage" variable in order to make that work.
Quoted 24 lines
Quoted 8 lines
mm/hugetlb.c +0
-0@@ -4356,7 +4356,13 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma, same_page:if (pages) {mm/hugetlb.c long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma, · line 4366
Quoted 10 lines
mm/hugetlb.c +7
-1pages[i] =mem_map_offset (page, pfn_offset); -get_page (pages[i]); +if (!try_grab_page (pages[i], flags)) { +spin_unlock (ptl); + remainder =0 ; + err = -ENOMEM ; +WARN_ON_ONCE (1 ); +break ; + } }This function does a refcount overflow check early so that it doesn't have to do
try_get_page()here. So that check can be now removed when you dotry_grab_page()here anyway since that early check seems to be just a tiny optimization AFAICT.
Quoted 2 lines
Honza
Yes. I've removed it, good spot.
thanks,
Signature · 2 lines
John Hubbard NVIDIA
kbuild test robot
build botRe: [PATCH v9 23/25] mm/gup: track FOLL_PIN pages
In reply to John HubbardThank you for the patch! Perhaps something to improve:
Hi John,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on rdma/for-next]
[also build test WARNING on linus/master v5.5-rc2 next-20191216] [cannot apply to mmotm/master vfio/next]
[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/20191212-013238 base: https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next reproduce: # apt-get install sparse # sparse version: v0.6.1-104-gf934193-dirty make ARCH=x86_64 allmodconfig make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
sparse warnings: (new ones prefixed by >>)
>> mm/gup.c:78:26: sparse: sparse: symbol 'try_pin_compound_head' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation
kbuild test robot
build bot[RFC PATCH] mm/gup: try_pin_compound_head() can be static
In reply to John HubbardFixes: 8086d1c61970 ("mm/gup: track FOLL_PIN pages")
--- gup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/gup.c +1 -1
@@ -75,7 +75,7 @@ static inline struct page *try_get_compound_head(struct page *page, int refs)* @Return: the compound head page, with ref appropriately incremented, * or NULL upon failure. */ -__must_check struct page *try_pin_compound_head (struct page *page,int refs) +static __must_check struct page *try_pin_compound_head (struct page *page,int refs) {struct page *head =try_get_compound_head (page,GUP_PIN_COUNTING_BIAS * refs);
John Hubbard
reviewerRe: [RFC PATCH] mm/gup: try_pin_compound_head() can be static
Quoted 13 lines · kbuild test robot
Fixes 8086d1c61970 ("mm/gup: track FOLL_PIN pages")Signed-off-by kbuild test robot <[email protected]>--- gup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/gup.c +0
-0
mm/gup.c static inline struct page *try_get_compound_head(struct page *page, int refs) · line 81
Quoted 8 lines
mm/gup.c +1
-1@@ -75,7 +75,7 @@ static inline struct page *try_get_compound_head(struct page *page, int refs)* @Return: the compound head page, with ref appropriately incremented, * or NULL upon failure. */ -__must_check struct page *try_pin_compound_head (struct page *page,int refs) +static __must_check struct page *try_pin_compound_head (struct page *page,int refs) {struct page *head =try_get_compound_head (page,GUP_PIN_COUNTING_BIAS * refs);
Yes, it should have been declared static. And this also applies to the latest version (v11). The preferred fix would stay within 80 columns, like this:
mm/gup.c +2 -1
@@ -75,7 +75,8 @@ static inline struct page *try_get_compound_head(struct page *page, int refs)* @Return: the compound head page, with ref appropriately incremented, * or NULL upon failure. */ -__must_check struct page *try_pin_compound_head (struct page *page,int refs) +static __must_check struct page *try_pin_compound_head (struct page *page, +int refs) {struct page *head =try_get_compound_head (page,GUP_PIN_COUNTING_BIAS * refs);
thanks,
Signature · 2 lines
John Hubbard NVIDIA
John Hubbard
reviewerPATCH v9 24/25 mm/gup_benchmark: support pin_user_pages() and related calls
In reply to John HubbardUp 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_fast(): via the '-a' command line option
* pin_user_pages(): 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 unpin_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.
--- mm/gup_benchmark.c | 65 ++++++++++++++++++++-- tools/testing/selftests/vm/gup_benchmark.c | 15 ++++- 2 files changed, 74 insertions(+), 6 deletions(-)
mm/gup_benchmark.c +60 -5
@@ -8,6 +8,8 @@#define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_benchmark)#define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)#define GUP_BENCHMARK _IOWR('g', 3, struct gup_benchmark) +#define PIN_FAST_BENCHMARK _IOWR('g', 4, struct gup_benchmark) +#define PIN_BENCHMARK _IOWR('g', 5, struct gup_benchmark)struct gup_benchmark {__u64 get_delta_usec;
@@ -19,6 +21,42 @@ struct gup_benchmark {__u64 expansion[10 ];/* For future use */ }; +static void put_back_pages (int cmd,struct page **pages,unsigned long nr_pages) +{ +int i; + +switch (cmd) { +case GUP_FAST_BENCHMARK : +case GUP_LONGTERM_BENCHMARK : +case GUP_BENCHMARK : +for (i =0 ; i < nr_pages; i++) +put_page (pages[i]); +break ; + +case PIN_FAST_BENCHMARK : +case PIN_BENCHMARK : +unpin_user_pages (pages, nr_pages); +break ; + } +} + +static void verify_dma_pinned (int cmd,struct page **pages, +unsigned long nr_pages) +{ +int i; + +switch (cmd) { +case PIN_FAST_BENCHMARK : +case PIN_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 ; + } +} +static int __gup_benchmark_ioctl (unsigned int cmd,struct gup_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 ; +case PIN_FAST_BENCHMARK : + nr =pin_user_pages_fast (addr, nr, gup->flags, + pages + i); +break ; +case PIN_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; +/* +* Take an un-benchmark-timed moment to verify DMA pinned +* state: print a warning if any non-dma-pinned pages are found: +*/ +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,case GUP_FAST_BENCHMARK :case GUP_LONGTERM_BENCHMARK :case GUP_BENCHMARK : +case PIN_FAST_BENCHMARK : +case PIN_BENCHMARK :break ;default :return -EINVAL ;
tools/testing/selftests/vm/gup_benchmark.c +14 -1
@@ -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 ;
Signature · 6 lines
2.24.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
John Hubbard
reviewerPATCH v9 25/25 selftests/vm: run_vmtests: invoke gup_benchmark with basic FOLL_PIN coverage
In reply to John HubbardIt'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.
--- tools/testing/selftests/vm/run_vmtests | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
tools/testing/selftests/vm/run_vmtests +22 -0
@@ -112,6 +112,28 @@ echo "NOTE: The above hugetlb tests provide minimal coverage. Use" echo " https://github.com/libhugetlbfs/libhugetlbfs.git for" echo " hugetlb regression testing." +echo "--------------------------------------------" +echo "running 'gup_benchmark -U' (normal/slow gup)" +echo "--------------------------------------------" +./gup_benchmark -U +if [ $? -ne 0 ]; then + echo "[FAIL]" + exitcode=1 +else + echo "[PASS]" +fi + +echo "------------------------------------------" +echo "running gup_benchmark -b (pin_user_pages)" +echo "------------------------------------------" +./gup_benchmark -b +if [ $? -ne 0 ]; then + echo "[FAIL]" + exitcode=1 +else + echo "[PASS]" +fi + echo "-------------------" echo "running userfaultfd" echo "-------------------"
Signature · 6 lines
2.24.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel