* [PATCH v4 0/2] vfio/type1: Synchronous locked page accounting
@ 2017-04-17 1:42 Alex Williamson
2017-04-17 1:42 ` [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue Alex Williamson
2017-04-17 1:42 ` [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external() Alex Williamson
0 siblings, 2 replies; 14+ messages in thread
From: Alex Williamson @ 2017-04-17 1:42 UTC (permalink / raw)
To: alex.williamson, kvm; +Cc: eric.auger, kwankhede, peterx, linux-kernel, slp
v4: vfio_lock_acct() should not fail due to RLIMIT_MEMLOCK if task
has CAP_IPC_LOCK capability. Introduced 2nd patch to remove
redundancy from vfio_pin_page_external() and fix return value.
Please re-review. Thanks!
Alex
---
Alex Williamson (2):
vfio/type1: Remove locked page accounting workqueue
vfio/type1: Prune vfio_pin_page_external()
drivers/vfio/vfio_iommu_type1.c | 127 ++++++++++++++-------------------------
1 file changed, 46 insertions(+), 81 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue 2017-04-17 1:42 [PATCH v4 0/2] vfio/type1: Synchronous locked page accounting Alex Williamson @ 2017-04-17 1:42 ` Alex Williamson 2017-04-17 6:47 ` Peter Xu 2017-04-17 1:42 ` [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external() Alex Williamson 1 sibling, 1 reply; 14+ messages in thread From: Alex Williamson @ 2017-04-17 1:42 UTC (permalink / raw) To: alex.williamson, kvm; +Cc: eric.auger, kwankhede, peterx, linux-kernel, slp If the mmap_sem is contented then the vfio type1 IOMMU backend will defer locked page accounting updates to a workqueue task. This has a few problems and depending on which side the user tries to play, they might be over-penalized for unmaps that haven't yet been accounted or race the workqueue to enter more mappings than they're allowed. The original intent of this workqueue mechanism seems to be focused on reducing latency through the ioctl, but we cannot do so at the cost of correctness. Remove this workqueue mechanism and update the callers to allow for failure. We can also now recheck the limit under write lock to make sure we don't exceed it. vfio_pin_pages_remote() also now necessarily includes an unwind path which we can jump to directly if the consecutive page pinning finds that we're exceeding the user's memory limits. This avoids the current lazy approach which does accounting and mapping up to the fault, only to return an error on the next iteration to unwind the entire vfio_dma. Cc: stable@vger.kernel.org Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- drivers/vfio/vfio_iommu_type1.c | 107 +++++++++++++++++---------------------- 1 file changed, 48 insertions(+), 59 deletions(-) diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 32d2633092a3..fb18e4a5df62 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -246,69 +246,43 @@ static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma, struct vfio_pfn *vpfn) return ret; } -struct vwork { - struct mm_struct *mm; - long npage; - struct work_struct work; -}; - -/* delayed decrement/increment for locked_vm */ -static void vfio_lock_acct_bg(struct work_struct *work) -{ - struct vwork *vwork = container_of(work, struct vwork, work); - struct mm_struct *mm; - - mm = vwork->mm; - down_write(&mm->mmap_sem); - mm->locked_vm += vwork->npage; - up_write(&mm->mmap_sem); - mmput(mm); - kfree(vwork); -} - -static void vfio_lock_acct(struct task_struct *task, long npage) +static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) { - struct vwork *vwork; struct mm_struct *mm; bool is_current; + int ret; if (!npage) - return; + return 0; is_current = (task->mm == current->mm); mm = is_current ? task->mm : get_task_mm(task); if (!mm) - return; /* process exited */ + return -ESRCH; /* process exited */ - if (down_write_trylock(&mm->mmap_sem)) { - mm->locked_vm += npage; - up_write(&mm->mmap_sem); - if (!is_current) - mmput(mm); - return; - } + ret = down_write_killable(&mm->mmap_sem); + if (!ret) { + if (npage < 0 || lock_cap) { + mm->locked_vm += npage; + } else { + unsigned long limit; - if (is_current) { - mm = get_task_mm(task); - if (!mm) - return; + limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT; + + if (mm->locked_vm + npage <= limit) + mm->locked_vm += npage; + else + ret = -ENOMEM; + } + + up_write(&mm->mmap_sem); } - /* - * Couldn't get mmap_sem lock, so must setup to update - * mm->locked_vm later. If locked_vm were atomic, we - * wouldn't need this silliness - */ - vwork = kmalloc(sizeof(struct vwork), GFP_KERNEL); - if (WARN_ON(!vwork)) { + if (!is_current) mmput(mm); - return; - } - INIT_WORK(&vwork->work, vfio_lock_acct_bg); - vwork->mm = mm; - vwork->npage = npage; - schedule_work(&vwork->work); + + return ret; } /* @@ -405,7 +379,7 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, long npage, unsigned long *pfn_base) { - unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; + unsigned long pfn = 0, limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; bool lock_cap = capable(CAP_IPC_LOCK); long ret, pinned = 0, lock_acct = 0; bool rsvd; @@ -442,8 +416,6 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, /* Lock all the consecutive pages from pfn_base */ for (vaddr += PAGE_SIZE, iova += PAGE_SIZE; pinned < npage; pinned++, vaddr += PAGE_SIZE, iova += PAGE_SIZE) { - unsigned long pfn = 0; - ret = vaddr_get_pfn(current->mm, vaddr, dma->prot, &pfn); if (ret) break; @@ -460,14 +432,25 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, put_pfn(pfn, dma->prot); pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", __func__, limit << PAGE_SHIFT); - break; + ret = -ENOMEM; + goto unpin_out; } lock_acct++; } } out: - vfio_lock_acct(current, lock_acct); + ret = vfio_lock_acct(current, lock_acct, lock_cap); + +unpin_out: + if (ret) { + if (!rsvd) { + for (pfn = *pfn_base ; pinned ; pfn++, pinned--) + put_pfn(pfn, dma->prot); + } + + return ret; + } return pinned; } @@ -488,7 +471,7 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova, } if (do_accounting) - vfio_lock_acct(dma->task, locked - unlocked); + vfio_lock_acct(dma->task, locked - unlocked, false); return unlocked; } @@ -522,8 +505,14 @@ static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr, goto pin_page_exit; } - if (!rsvd && do_accounting) - vfio_lock_acct(dma->task, 1); + if (!rsvd && do_accounting) { + ret = vfio_lock_acct(dma->task, 1, lock_cap); + if (ret) { + put_pfn(*pfn_base, dma->prot); + goto pin_page_exit; + } + } + ret = 1; pin_page_exit: @@ -543,7 +532,7 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova, unlocked = vfio_iova_put_vfio_pfn(dma, vpfn); if (do_accounting) - vfio_lock_acct(dma->task, -unlocked); + vfio_lock_acct(dma->task, -unlocked, false); return unlocked; } @@ -740,7 +729,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, dma->iommu_mapped = false; if (do_accounting) { - vfio_lock_acct(dma->task, -unlocked); + vfio_lock_acct(dma->task, -unlocked, false); return 0; } return unlocked; @@ -1382,7 +1371,7 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) if (!is_invalid_reserved_pfn(vpfn->pfn)) locked++; } - vfio_lock_acct(dma->task, locked - unlocked); + vfio_lock_acct(dma->task, locked - unlocked, false); } } ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue 2017-04-17 1:42 ` [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue Alex Williamson @ 2017-04-17 6:47 ` Peter Xu 2017-04-17 14:32 ` Alex Williamson 0 siblings, 1 reply; 14+ messages in thread From: Peter Xu @ 2017-04-17 6:47 UTC (permalink / raw) To: Alex Williamson; +Cc: kvm, eric.auger, kwankhede, linux-kernel, slp On Sun, Apr 16, 2017 at 07:42:27PM -0600, Alex Williamson wrote: [...] > -static void vfio_lock_acct(struct task_struct *task, long npage) > +static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) > { > - struct vwork *vwork; > struct mm_struct *mm; > bool is_current; > + int ret; > > if (!npage) > - return; > + return 0; > > is_current = (task->mm == current->mm); > > mm = is_current ? task->mm : get_task_mm(task); > if (!mm) > - return; /* process exited */ > + return -ESRCH; /* process exited */ > > - if (down_write_trylock(&mm->mmap_sem)) { > - mm->locked_vm += npage; > - up_write(&mm->mmap_sem); > - if (!is_current) > - mmput(mm); > - return; > - } > + ret = down_write_killable(&mm->mmap_sem); > + if (!ret) { > + if (npage < 0 || lock_cap) { Nit: maybe we can avoid passing in lock_cap in all the callers of vfio_lock_acct() and fetch it via has_capability() only if npage < 0? IMHO that'll keep the vfio_lock_acct() interface cleaner, and we won't need to pass in "false" any time when doing unpins. [...] > @@ -405,7 +379,7 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, > static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > long npage, unsigned long *pfn_base) > { > - unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > + unsigned long pfn = 0, limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > bool lock_cap = capable(CAP_IPC_LOCK); > long ret, pinned = 0, lock_acct = 0; > bool rsvd; > @@ -442,8 +416,6 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > /* Lock all the consecutive pages from pfn_base */ > for (vaddr += PAGE_SIZE, iova += PAGE_SIZE; pinned < npage; > pinned++, vaddr += PAGE_SIZE, iova += PAGE_SIZE) { > - unsigned long pfn = 0; > - > ret = vaddr_get_pfn(current->mm, vaddr, dma->prot, &pfn); > if (ret) > break; > @@ -460,14 +432,25 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > put_pfn(pfn, dma->prot); > pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", > __func__, limit << PAGE_SHIFT); > - break; > + ret = -ENOMEM; > + goto unpin_out; > } > lock_acct++; > } > } > > out: > - vfio_lock_acct(current, lock_acct); > + ret = vfio_lock_acct(current, lock_acct, lock_cap); I just didn't notice this in previous review, but... do we need to check against !rsvd as well here before doing the accounting? Thanks! > + > +unpin_out: > + if (ret) { > + if (!rsvd) { > + for (pfn = *pfn_base ; pinned ; pfn++, pinned--) > + put_pfn(pfn, dma->prot); > + } > + > + return ret; > + } > > return pinned; > } -- Peter Xu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue 2017-04-17 6:47 ` Peter Xu @ 2017-04-17 14:32 ` Alex Williamson 2017-04-17 19:05 ` Kirti Wankhede 0 siblings, 1 reply; 14+ messages in thread From: Alex Williamson @ 2017-04-17 14:32 UTC (permalink / raw) To: Peter Xu; +Cc: kvm, eric.auger, kwankhede, linux-kernel, slp On Mon, 17 Apr 2017 14:47:54 +0800 Peter Xu <peterx@redhat.com> wrote: > On Sun, Apr 16, 2017 at 07:42:27PM -0600, Alex Williamson wrote: > > [...] > > > -static void vfio_lock_acct(struct task_struct *task, long npage) > > +static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) > > { > > - struct vwork *vwork; > > struct mm_struct *mm; > > bool is_current; > > + int ret; > > > > if (!npage) > > - return; > > + return 0; > > > > is_current = (task->mm == current->mm); > > > > mm = is_current ? task->mm : get_task_mm(task); > > if (!mm) > > - return; /* process exited */ > > + return -ESRCH; /* process exited */ > > > > - if (down_write_trylock(&mm->mmap_sem)) { > > - mm->locked_vm += npage; > > - up_write(&mm->mmap_sem); > > - if (!is_current) > > - mmput(mm); > > - return; > > - } > > + ret = down_write_killable(&mm->mmap_sem); > > + if (!ret) { > > + if (npage < 0 || lock_cap) { > > Nit: maybe we can avoid passing in lock_cap in all the callers of > vfio_lock_acct() and fetch it via has_capability() only if npage < 0? > IMHO that'll keep the vfio_lock_acct() interface cleaner, and we won't > need to pass in "false" any time when doing unpins. Unfortunately vfio_pin_pages_remote() needs to know about lock_cap since it tests whether the user is exceeding their locked memory limit. The other callers could certainly get away with vfio_lock_acct() testing the capability itself but that would add a redundant call for the most common user. I'm not a big fan of passing a lock_cap bool either, but it seemed the best fix for now. The cleanest alternative I can up with is this (untested): diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 07e0e58f22e9..0dbcf950fef9 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -246,7 +246,7 @@ static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma, struct vfio_pfn *vpfn) return ret; } -static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) +static int vfio_lock_acct(struct task_struct *task, long npage, bool *lock_cap) { struct mm_struct *mm; bool is_current; @@ -263,19 +263,24 @@ static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) ret = down_write_killable(&mm->mmap_sem); if (!ret) { - if (npage < 0 || lock_cap) { + if (npage < 0 || (lock_cap && *lock_cap)) { mm->locked_vm += npage; } else { - unsigned long limit; + if (lock_cap || !has_capability(task, CAP_IPC_LOCK)) { + unsigned long limit; - limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT; + limit = task_rlimit(task, RLIMIT_MEMLOCK) + >> PAGE_SHIFT; - if (mm->locked_vm + npage <= limit) - mm->locked_vm += npage; - else - ret = -ENOMEM; - } + if (mm->locked_vm + npage > limit) { + ret = -ENOMEM; + goto upwrite; + } + } + mm->locked_vm += npage; + } +upwrite: up_write(&mm->mmap_sem); } @@ -440,7 +445,7 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, } out: - ret = vfio_lock_acct(current, lock_acct, lock_cap); + ret = vfio_lock_acct(current, lock_acct, &lock_cap); unpin_out: if (ret) { @@ -471,7 +476,7 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova, } if (do_accounting) - vfio_lock_acct(dma->task, locked - unlocked, false); + vfio_lock_acct(dma->task, locked - unlocked, NULL); return unlocked; } @@ -488,8 +493,7 @@ static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr, ret = vaddr_get_pfn(mm, vaddr, dma->prot, pfn_base); if (!ret && do_accounting && !is_invalid_reserved_pfn(*pfn_base)) { - ret = vfio_lock_acct(dma->task, 1, - has_capability(dma->task, CAP_IPC_LOCK)); + ret = vfio_lock_acct(dma->task, 1, NULL); if (ret) put_pfn(*pfn_base, dma->prot); } @@ -510,7 +514,7 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova, unlocked = vfio_iova_put_vfio_pfn(dma, vpfn); if (do_accounting) - vfio_lock_acct(dma->task, -unlocked, false); + vfio_lock_acct(dma->task, -unlocked, NULL); return unlocked; } @@ -705,7 +709,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, dma->iommu_mapped = false; if (do_accounting) { - vfio_lock_acct(dma->task, -unlocked, false); + vfio_lock_acct(dma->task, -unlocked, NULL); return 0; } return unlocked; @@ -1347,7 +1351,7 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) if (!is_invalid_reserved_pfn(vpfn->pfn)) locked++; } - vfio_lock_acct(dma->task, locked - unlocked, false); + vfio_lock_acct(dma->task, locked - unlocked, NULL); } } ie. we keep that third arg to vfio_lock_acct(), but it's effectively optional. Thoughts? > [...] > > > @@ -405,7 +379,7 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, > > static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > > long npage, unsigned long *pfn_base) > > { > > - unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > > + unsigned long pfn = 0, limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > > bool lock_cap = capable(CAP_IPC_LOCK); > > long ret, pinned = 0, lock_acct = 0; > > bool rsvd; > > @@ -442,8 +416,6 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > > /* Lock all the consecutive pages from pfn_base */ > > for (vaddr += PAGE_SIZE, iova += PAGE_SIZE; pinned < npage; > > pinned++, vaddr += PAGE_SIZE, iova += PAGE_SIZE) { > > - unsigned long pfn = 0; > > - > > ret = vaddr_get_pfn(current->mm, vaddr, dma->prot, &pfn); > > if (ret) > > break; > > @@ -460,14 +432,25 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > > put_pfn(pfn, dma->prot); > > pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", > > __func__, limit << PAGE_SHIFT); > > - break; > > + ret = -ENOMEM; > > + goto unpin_out; > > } > > lock_acct++; > > } > > } > > > > out: > > - vfio_lock_acct(current, lock_acct); > > + ret = vfio_lock_acct(current, lock_acct, lock_cap); > > I just didn't notice this in previous review, but... do we need to > check against !rsvd as well here before doing the accounting? rsvd is taken care of above, lock_acct is only incremented for non-reserved pages, so a block of rsvd pages would call vfio_lock_acct with 0 pages, which will immediately return. Thanks, Alex > > + > > +unpin_out: > > + if (ret) { > > + if (!rsvd) { > > + for (pfn = *pfn_base ; pinned ; pfn++, pinned--) > > + put_pfn(pfn, dma->prot); > > + } > > + > > + return ret; > > + } > > > > return pinned; > > } > ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue 2017-04-17 14:32 ` Alex Williamson @ 2017-04-17 19:05 ` Kirti Wankhede 2017-04-17 19:19 ` Alex Williamson 0 siblings, 1 reply; 14+ messages in thread From: Kirti Wankhede @ 2017-04-17 19:05 UTC (permalink / raw) To: Alex Williamson, Peter Xu; +Cc: kvm, eric.auger, linux-kernel, slp On 4/17/2017 8:02 PM, Alex Williamson wrote: > On Mon, 17 Apr 2017 14:47:54 +0800 > Peter Xu <peterx@redhat.com> wrote: > >> On Sun, Apr 16, 2017 at 07:42:27PM -0600, Alex Williamson wrote: >> >> [...] >> >>> -static void vfio_lock_acct(struct task_struct *task, long npage) >>> +static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) >>> { >>> - struct vwork *vwork; >>> struct mm_struct *mm; >>> bool is_current; >>> + int ret; >>> >>> if (!npage) >>> - return; >>> + return 0; >>> >>> is_current = (task->mm == current->mm); >>> >>> mm = is_current ? task->mm : get_task_mm(task); >>> if (!mm) >>> - return; /* process exited */ >>> + return -ESRCH; /* process exited */ >>> >>> - if (down_write_trylock(&mm->mmap_sem)) { >>> - mm->locked_vm += npage; >>> - up_write(&mm->mmap_sem); >>> - if (!is_current) >>> - mmput(mm); >>> - return; >>> - } >>> + ret = down_write_killable(&mm->mmap_sem); >>> + if (!ret) { >>> + if (npage < 0 || lock_cap) { >> >> Nit: maybe we can avoid passing in lock_cap in all the callers of >> vfio_lock_acct() and fetch it via has_capability() only if npage < 0? >> IMHO that'll keep the vfio_lock_acct() interface cleaner, and we won't >> need to pass in "false" any time when doing unpins. > > Unfortunately vfio_pin_pages_remote() needs to know about lock_cap > since it tests whether the user is exceeding their locked memory > limit. The other callers could certainly get away with > vfio_lock_acct() testing the capability itself but that would add a > redundant call for the most common user. I'm not a big fan of passing > a lock_cap bool either, but it seemed the best fix for now. The > cleanest alternative I can up with is this (untested): > In my opinion, passing 'bool lock_cap' looks much clean and simple. Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com> Thanks, Kirti. > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 07e0e58f22e9..0dbcf950fef9 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -246,7 +246,7 @@ static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma, struct vfio_pfn *vpfn) > return ret; > } > > -static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) > +static int vfio_lock_acct(struct task_struct *task, long npage, bool *lock_cap) > { > struct mm_struct *mm; > bool is_current; > @@ -263,19 +263,24 @@ static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) > > ret = down_write_killable(&mm->mmap_sem); > if (!ret) { > - if (npage < 0 || lock_cap) { > + if (npage < 0 || (lock_cap && *lock_cap)) { > mm->locked_vm += npage; > } else { > - unsigned long limit; > + if (lock_cap || !has_capability(task, CAP_IPC_LOCK)) { > + unsigned long limit; > > - limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT; > + limit = task_rlimit(task, RLIMIT_MEMLOCK) > + >> PAGE_SHIFT; > > - if (mm->locked_vm + npage <= limit) > - mm->locked_vm += npage; > - else > - ret = -ENOMEM; > - } > + if (mm->locked_vm + npage > limit) { > + ret = -ENOMEM; > + goto upwrite; > + } > + } > > + mm->locked_vm += npage; > + } > +upwrite: > up_write(&mm->mmap_sem); > } > > @@ -440,7 +445,7 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > } > > out: > - ret = vfio_lock_acct(current, lock_acct, lock_cap); > + ret = vfio_lock_acct(current, lock_acct, &lock_cap); > > unpin_out: > if (ret) { > @@ -471,7 +476,7 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova, > } > > if (do_accounting) > - vfio_lock_acct(dma->task, locked - unlocked, false); > + vfio_lock_acct(dma->task, locked - unlocked, NULL); > > return unlocked; > } > @@ -488,8 +493,7 @@ static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr, > > ret = vaddr_get_pfn(mm, vaddr, dma->prot, pfn_base); > if (!ret && do_accounting && !is_invalid_reserved_pfn(*pfn_base)) { > - ret = vfio_lock_acct(dma->task, 1, > - has_capability(dma->task, CAP_IPC_LOCK)); > + ret = vfio_lock_acct(dma->task, 1, NULL); > if (ret) > put_pfn(*pfn_base, dma->prot); > } > @@ -510,7 +514,7 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova, > unlocked = vfio_iova_put_vfio_pfn(dma, vpfn); > > if (do_accounting) > - vfio_lock_acct(dma->task, -unlocked, false); > + vfio_lock_acct(dma->task, -unlocked, NULL); > > return unlocked; > } > @@ -705,7 +709,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, > > dma->iommu_mapped = false; > if (do_accounting) { > - vfio_lock_acct(dma->task, -unlocked, false); > + vfio_lock_acct(dma->task, -unlocked, NULL); > return 0; > } > return unlocked; > @@ -1347,7 +1351,7 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) > if (!is_invalid_reserved_pfn(vpfn->pfn)) > locked++; > } > - vfio_lock_acct(dma->task, locked - unlocked, false); > + vfio_lock_acct(dma->task, locked - unlocked, NULL); > } > } > > ie. we keep that third arg to vfio_lock_acct(), but it's effectively > optional. Thoughts? > > >> [...] >> >>> @@ -405,7 +379,7 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, >>> static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, >>> long npage, unsigned long *pfn_base) >>> { >>> - unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; >>> + unsigned long pfn = 0, limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; >>> bool lock_cap = capable(CAP_IPC_LOCK); >>> long ret, pinned = 0, lock_acct = 0; >>> bool rsvd; >>> @@ -442,8 +416,6 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, >>> /* Lock all the consecutive pages from pfn_base */ >>> for (vaddr += PAGE_SIZE, iova += PAGE_SIZE; pinned < npage; >>> pinned++, vaddr += PAGE_SIZE, iova += PAGE_SIZE) { >>> - unsigned long pfn = 0; >>> - >>> ret = vaddr_get_pfn(current->mm, vaddr, dma->prot, &pfn); >>> if (ret) >>> break; >>> @@ -460,14 +432,25 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, >>> put_pfn(pfn, dma->prot); >>> pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", >>> __func__, limit << PAGE_SHIFT); >>> - break; >>> + ret = -ENOMEM; >>> + goto unpin_out; >>> } >>> lock_acct++; >>> } >>> } >>> >>> out: >>> - vfio_lock_acct(current, lock_acct); >>> + ret = vfio_lock_acct(current, lock_acct, lock_cap); >> >> I just didn't notice this in previous review, but... do we need to >> check against !rsvd as well here before doing the accounting? > > rsvd is taken care of above, lock_acct is only incremented for > non-reserved pages, so a block of rsvd pages would call vfio_lock_acct > with 0 pages, which will immediately return. Thanks, > > Alex > >>> + >>> +unpin_out: >>> + if (ret) { >>> + if (!rsvd) { >>> + for (pfn = *pfn_base ; pinned ; pfn++, pinned--) >>> + put_pfn(pfn, dma->prot); >>> + } >>> + >>> + return ret; >>> + } >>> >>> return pinned; >>> } >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue 2017-04-17 19:05 ` Kirti Wankhede @ 2017-04-17 19:19 ` Alex Williamson 2017-04-17 19:32 ` Kirti Wankhede 0 siblings, 1 reply; 14+ messages in thread From: Alex Williamson @ 2017-04-17 19:19 UTC (permalink / raw) To: Kirti Wankhede; +Cc: Peter Xu, kvm, eric.auger, linux-kernel, slp On Tue, 18 Apr 2017 00:35:06 +0530 Kirti Wankhede <kwankhede@nvidia.com> wrote: > On 4/17/2017 8:02 PM, Alex Williamson wrote: > > On Mon, 17 Apr 2017 14:47:54 +0800 > > Peter Xu <peterx@redhat.com> wrote: > > > >> On Sun, Apr 16, 2017 at 07:42:27PM -0600, Alex Williamson wrote: > >> > >> [...] > >> > >>> -static void vfio_lock_acct(struct task_struct *task, long npage) > >>> +static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) > >>> { > >>> - struct vwork *vwork; > >>> struct mm_struct *mm; > >>> bool is_current; > >>> + int ret; > >>> > >>> if (!npage) > >>> - return; > >>> + return 0; > >>> > >>> is_current = (task->mm == current->mm); > >>> > >>> mm = is_current ? task->mm : get_task_mm(task); > >>> if (!mm) > >>> - return; /* process exited */ > >>> + return -ESRCH; /* process exited */ > >>> > >>> - if (down_write_trylock(&mm->mmap_sem)) { > >>> - mm->locked_vm += npage; > >>> - up_write(&mm->mmap_sem); > >>> - if (!is_current) > >>> - mmput(mm); > >>> - return; > >>> - } > >>> + ret = down_write_killable(&mm->mmap_sem); > >>> + if (!ret) { > >>> + if (npage < 0 || lock_cap) { > >> > >> Nit: maybe we can avoid passing in lock_cap in all the callers of > >> vfio_lock_acct() and fetch it via has_capability() only if npage < 0? > >> IMHO that'll keep the vfio_lock_acct() interface cleaner, and we won't > >> need to pass in "false" any time when doing unpins. > > > > Unfortunately vfio_pin_pages_remote() needs to know about lock_cap > > since it tests whether the user is exceeding their locked memory > > limit. The other callers could certainly get away with > > vfio_lock_acct() testing the capability itself but that would add a > > redundant call for the most common user. I'm not a big fan of passing > > a lock_cap bool either, but it seemed the best fix for now. The > > cleanest alternative I can up with is this (untested): > > > > In my opinion, passing 'bool lock_cap' looks much clean and simple. > > Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com> Well shoot, I was just starting to warm up to the bool*. I like that we're not presuming the polarity for the callers we expect to be removing pages and I generally just dislike passing fixed bool parameters to change the function behavior. I've cleaned it up a bit further and was starting to do some testing on this which I'd propose for v5. Does it change your opinion? commit cd61c5f507d614ac14b75b0a548c8738deff88ea Author: Alex Williamson <alex.williamson@redhat.com> Date: Thu Apr 13 14:10:15 2017 -0600 vfio/type1: Remove locked page accounting workqueue If the mmap_sem is contented then the vfio type1 IOMMU backend will defer locked page accounting updates to a workqueue task. This has a few problems and depending on which side the user tries to play, they might be over-penalized for unmaps that haven't yet been accounted or race the workqueue to enter more mappings than they're allowed. The original intent of this workqueue mechanism seems to be focused on reducing latency through the ioctl, but we cannot do so at the cost of correctness. Remove this workqueue mechanism and update the callers to allow for failure. We can also now recheck the limit under write lock to make sure we don't exceed it. vfio_pin_pages_remote() also now necessarily includes an unwind path which we can jump to directly if the consecutive page pinning finds that we're exceeding the user's memory limits. This avoids the current lazy approach which does accounting and mapping up to the fault, only to return an error on the next iteration to unwind the entire vfio_dma. Cc: stable@vger.kernel.org Signed-off-by: Alex Williamson <alex.williamson@redhat.com> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 32d2633092a3..a8a079ba9477 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -246,69 +246,46 @@ static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma, struct vfio_pfn *vpfn) return ret; } -struct vwork { - struct mm_struct *mm; - long npage; - struct work_struct work; -}; - -/* delayed decrement/increment for locked_vm */ -static void vfio_lock_acct_bg(struct work_struct *work) -{ - struct vwork *vwork = container_of(work, struct vwork, work); - struct mm_struct *mm; - - mm = vwork->mm; - down_write(&mm->mmap_sem); - mm->locked_vm += vwork->npage; - up_write(&mm->mmap_sem); - mmput(mm); - kfree(vwork); -} - -static void vfio_lock_acct(struct task_struct *task, long npage) +static int vfio_lock_acct(struct task_struct *task, long npage, bool *lock_cap) { - struct vwork *vwork; struct mm_struct *mm; bool is_current; + int ret; if (!npage) - return; + return 0; is_current = (task->mm == current->mm); mm = is_current ? task->mm : get_task_mm(task); if (!mm) - return; /* process exited */ + return -ESRCH; /* process exited */ - if (down_write_trylock(&mm->mmap_sem)) { - mm->locked_vm += npage; - up_write(&mm->mmap_sem); - if (!is_current) - mmput(mm); - return; - } + ret = down_write_killable(&mm->mmap_sem); + if (!ret) { + if (npage > 0) { + if (lock_cap ? !*lock_cap : + !has_capability(task, CAP_IPC_LOCK)) { + unsigned long limit; + + limit = task_rlimit(task, + RLIMIT_MEMLOCK) >> PAGE_SHIFT; + + if (mm->locked_vm + npage > limit) + ret = -ENOMEM; + } + } + + if (!ret) + mm->locked_vm += npage; - if (is_current) { - mm = get_task_mm(task); - if (!mm) - return; + up_write(&mm->mmap_sem); } - /* - * Couldn't get mmap_sem lock, so must setup to update - * mm->locked_vm later. If locked_vm were atomic, we - * wouldn't need this silliness - */ - vwork = kmalloc(sizeof(struct vwork), GFP_KERNEL); - if (WARN_ON(!vwork)) { + if (!is_current) mmput(mm); - return; - } - INIT_WORK(&vwork->work, vfio_lock_acct_bg); - vwork->mm = mm; - vwork->npage = npage; - schedule_work(&vwork->work); + + return ret; } /* @@ -405,7 +382,7 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, long npage, unsigned long *pfn_base) { - unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; + unsigned long pfn = 0, limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; bool lock_cap = capable(CAP_IPC_LOCK); long ret, pinned = 0, lock_acct = 0; bool rsvd; @@ -442,8 +419,6 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, /* Lock all the consecutive pages from pfn_base */ for (vaddr += PAGE_SIZE, iova += PAGE_SIZE; pinned < npage; pinned++, vaddr += PAGE_SIZE, iova += PAGE_SIZE) { - unsigned long pfn = 0; - ret = vaddr_get_pfn(current->mm, vaddr, dma->prot, &pfn); if (ret) break; @@ -460,14 +435,25 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, put_pfn(pfn, dma->prot); pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", __func__, limit << PAGE_SHIFT); - break; + ret = -ENOMEM; + goto unpin_out; } lock_acct++; } } out: - vfio_lock_acct(current, lock_acct); + ret = vfio_lock_acct(current, lock_acct, &lock_cap); + +unpin_out: + if (ret) { + if (!rsvd) { + for (pfn = *pfn_base ; pinned ; pfn++, pinned--) + put_pfn(pfn, dma->prot); + } + + return ret; + } return pinned; } @@ -488,7 +474,7 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova, } if (do_accounting) - vfio_lock_acct(dma->task, locked - unlocked); + vfio_lock_acct(dma->task, locked - unlocked, NULL); return unlocked; } @@ -522,8 +508,14 @@ static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr, goto pin_page_exit; } - if (!rsvd && do_accounting) - vfio_lock_acct(dma->task, 1); + if (!rsvd && do_accounting) { + ret = vfio_lock_acct(dma->task, 1, &lock_cap); + if (ret) { + put_pfn(*pfn_base, dma->prot); + goto pin_page_exit; + } + } + ret = 1; pin_page_exit: @@ -543,7 +535,7 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova, unlocked = vfio_iova_put_vfio_pfn(dma, vpfn); if (do_accounting) - vfio_lock_acct(dma->task, -unlocked); + vfio_lock_acct(dma->task, -unlocked, NULL); return unlocked; } @@ -740,7 +732,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, dma->iommu_mapped = false; if (do_accounting) { - vfio_lock_acct(dma->task, -unlocked); + vfio_lock_acct(dma->task, -unlocked, NULL); return 0; } return unlocked; @@ -1382,7 +1374,7 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) if (!is_invalid_reserved_pfn(vpfn->pfn)) locked++; } - vfio_lock_acct(dma->task, locked - unlocked); + vfio_lock_acct(dma->task, locked - unlocked, NULL); } } Patch 2/2 would clearly change the &lock_cap in vfio_pin_page_external() to a NULL, so only _remote passes a pointer there. Thanks, Alex ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue 2017-04-17 19:19 ` Alex Williamson @ 2017-04-17 19:32 ` Kirti Wankhede 2017-04-17 21:32 ` Alex Williamson 0 siblings, 1 reply; 14+ messages in thread From: Kirti Wankhede @ 2017-04-17 19:32 UTC (permalink / raw) To: Alex Williamson; +Cc: Peter Xu, kvm, eric.auger, linux-kernel, slp On 4/18/2017 12:49 AM, Alex Williamson wrote: > On Tue, 18 Apr 2017 00:35:06 +0530 > Kirti Wankhede <kwankhede@nvidia.com> wrote: > >> On 4/17/2017 8:02 PM, Alex Williamson wrote: >>> On Mon, 17 Apr 2017 14:47:54 +0800 >>> Peter Xu <peterx@redhat.com> wrote: >>> >>>> On Sun, Apr 16, 2017 at 07:42:27PM -0600, Alex Williamson wrote: >>>> >>>> [...] >>>> >>>>> -static void vfio_lock_acct(struct task_struct *task, long npage) >>>>> +static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) >>>>> { >>>>> - struct vwork *vwork; >>>>> struct mm_struct *mm; >>>>> bool is_current; >>>>> + int ret; >>>>> >>>>> if (!npage) >>>>> - return; >>>>> + return 0; >>>>> >>>>> is_current = (task->mm == current->mm); >>>>> >>>>> mm = is_current ? task->mm : get_task_mm(task); >>>>> if (!mm) >>>>> - return; /* process exited */ >>>>> + return -ESRCH; /* process exited */ >>>>> >>>>> - if (down_write_trylock(&mm->mmap_sem)) { >>>>> - mm->locked_vm += npage; >>>>> - up_write(&mm->mmap_sem); >>>>> - if (!is_current) >>>>> - mmput(mm); >>>>> - return; >>>>> - } >>>>> + ret = down_write_killable(&mm->mmap_sem); >>>>> + if (!ret) { >>>>> + if (npage < 0 || lock_cap) { >>>> >>>> Nit: maybe we can avoid passing in lock_cap in all the callers of >>>> vfio_lock_acct() and fetch it via has_capability() only if npage < 0? >>>> IMHO that'll keep the vfio_lock_acct() interface cleaner, and we won't >>>> need to pass in "false" any time when doing unpins. >>> >>> Unfortunately vfio_pin_pages_remote() needs to know about lock_cap >>> since it tests whether the user is exceeding their locked memory >>> limit. The other callers could certainly get away with >>> vfio_lock_acct() testing the capability itself but that would add a >>> redundant call for the most common user. I'm not a big fan of passing >>> a lock_cap bool either, but it seemed the best fix for now. The >>> cleanest alternative I can up with is this (untested): >>> >> >> In my opinion, passing 'bool lock_cap' looks much clean and simple. >> >> Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com> > > Well shoot, I was just starting to warm up to the bool*. I like that > we're not presuming the polarity for the callers we expect to be > removing pages and I generally just dislike passing fixed bool > parameters to change the function behavior. I've cleaned it up a bit > further and was starting to do some testing on this which I'd propose > for v5. Does it change your opinion? If passing fixed bool parameter is the concern then I would lean towards Peter's suggestion. vfio_pin_pages_remote() will check lock capability outside vfio_lock_acct() and again in vfio_lock_acct(). At other places, it will be takes care within vfio_lock_acct() Thanks, Kirti > > commit cd61c5f507d614ac14b75b0a548c8738deff88ea > Author: Alex Williamson <alex.williamson@redhat.com> > Date: Thu Apr 13 14:10:15 2017 -0600 > > vfio/type1: Remove locked page accounting workqueue > > If the mmap_sem is contented then the vfio type1 IOMMU backend will > defer locked page accounting updates to a workqueue task. This has a > few problems and depending on which side the user tries to play, they > might be over-penalized for unmaps that haven't yet been accounted or > race the workqueue to enter more mappings than they're allowed. The > original intent of this workqueue mechanism seems to be focused on > reducing latency through the ioctl, but we cannot do so at the cost > of correctness. Remove this workqueue mechanism and update the > callers to allow for failure. We can also now recheck the limit under > write lock to make sure we don't exceed it. > > vfio_pin_pages_remote() also now necessarily includes an unwind path > which we can jump to directly if the consecutive page pinning finds > that we're exceeding the user's memory limits. This avoids the > current lazy approach which does accounting and mapping up to the > fault, only to return an error on the next iteration to unwind the > entire vfio_dma. > > Cc: stable@vger.kernel.org > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 32d2633092a3..a8a079ba9477 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -246,69 +246,46 @@ static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma, struct vfio_pfn *vpfn) > return ret; > } > > -struct vwork { > - struct mm_struct *mm; > - long npage; > - struct work_struct work; > -}; > - > -/* delayed decrement/increment for locked_vm */ > -static void vfio_lock_acct_bg(struct work_struct *work) > -{ > - struct vwork *vwork = container_of(work, struct vwork, work); > - struct mm_struct *mm; > - > - mm = vwork->mm; > - down_write(&mm->mmap_sem); > - mm->locked_vm += vwork->npage; > - up_write(&mm->mmap_sem); > - mmput(mm); > - kfree(vwork); > -} > - > -static void vfio_lock_acct(struct task_struct *task, long npage) > +static int vfio_lock_acct(struct task_struct *task, long npage, bool *lock_cap) > { > - struct vwork *vwork; > struct mm_struct *mm; > bool is_current; > + int ret; > > if (!npage) > - return; > + return 0; > > is_current = (task->mm == current->mm); > > mm = is_current ? task->mm : get_task_mm(task); > if (!mm) > - return; /* process exited */ > + return -ESRCH; /* process exited */ > > - if (down_write_trylock(&mm->mmap_sem)) { > - mm->locked_vm += npage; > - up_write(&mm->mmap_sem); > - if (!is_current) > - mmput(mm); > - return; > - } > + ret = down_write_killable(&mm->mmap_sem); > + if (!ret) { > + if (npage > 0) { > + if (lock_cap ? !*lock_cap : > + !has_capability(task, CAP_IPC_LOCK)) { > + unsigned long limit; > + > + limit = task_rlimit(task, > + RLIMIT_MEMLOCK) >> PAGE_SHIFT; > + > + if (mm->locked_vm + npage > limit) > + ret = -ENOMEM; > + } > + } > + > + if (!ret) > + mm->locked_vm += npage; > > - if (is_current) { > - mm = get_task_mm(task); > - if (!mm) > - return; > + up_write(&mm->mmap_sem); > } > > - /* > - * Couldn't get mmap_sem lock, so must setup to update > - * mm->locked_vm later. If locked_vm were atomic, we > - * wouldn't need this silliness > - */ > - vwork = kmalloc(sizeof(struct vwork), GFP_KERNEL); > - if (WARN_ON(!vwork)) { > + if (!is_current) > mmput(mm); > - return; > - } > - INIT_WORK(&vwork->work, vfio_lock_acct_bg); > - vwork->mm = mm; > - vwork->npage = npage; > - schedule_work(&vwork->work); > + > + return ret; > } > > /* > @@ -405,7 +382,7 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, > static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > long npage, unsigned long *pfn_base) > { > - unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > + unsigned long pfn = 0, limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > bool lock_cap = capable(CAP_IPC_LOCK); > long ret, pinned = 0, lock_acct = 0; > bool rsvd; > @@ -442,8 +419,6 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > /* Lock all the consecutive pages from pfn_base */ > for (vaddr += PAGE_SIZE, iova += PAGE_SIZE; pinned < npage; > pinned++, vaddr += PAGE_SIZE, iova += PAGE_SIZE) { > - unsigned long pfn = 0; > - > ret = vaddr_get_pfn(current->mm, vaddr, dma->prot, &pfn); > if (ret) > break; > @@ -460,14 +435,25 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > put_pfn(pfn, dma->prot); > pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", > __func__, limit << PAGE_SHIFT); > - break; > + ret = -ENOMEM; > + goto unpin_out; > } > lock_acct++; > } > } > > out: > - vfio_lock_acct(current, lock_acct); > + ret = vfio_lock_acct(current, lock_acct, &lock_cap); > + > +unpin_out: > + if (ret) { > + if (!rsvd) { > + for (pfn = *pfn_base ; pinned ; pfn++, pinned--) > + put_pfn(pfn, dma->prot); > + } > + > + return ret; > + } > > return pinned; > } > @@ -488,7 +474,7 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova, > } > > if (do_accounting) > - vfio_lock_acct(dma->task, locked - unlocked); > + vfio_lock_acct(dma->task, locked - unlocked, NULL); > > return unlocked; > } > @@ -522,8 +508,14 @@ static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr, > goto pin_page_exit; > } > > - if (!rsvd && do_accounting) > - vfio_lock_acct(dma->task, 1); > + if (!rsvd && do_accounting) { > + ret = vfio_lock_acct(dma->task, 1, &lock_cap); > + if (ret) { > + put_pfn(*pfn_base, dma->prot); > + goto pin_page_exit; > + } > + } > + > ret = 1; > > pin_page_exit: > @@ -543,7 +535,7 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova, > unlocked = vfio_iova_put_vfio_pfn(dma, vpfn); > > if (do_accounting) > - vfio_lock_acct(dma->task, -unlocked); > + vfio_lock_acct(dma->task, -unlocked, NULL); > > return unlocked; > } > @@ -740,7 +732,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, > > dma->iommu_mapped = false; > if (do_accounting) { > - vfio_lock_acct(dma->task, -unlocked); > + vfio_lock_acct(dma->task, -unlocked, NULL); > return 0; > } > return unlocked; > @@ -1382,7 +1374,7 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) > if (!is_invalid_reserved_pfn(vpfn->pfn)) > locked++; > } > - vfio_lock_acct(dma->task, locked - unlocked); > + vfio_lock_acct(dma->task, locked - unlocked, NULL); > } > } > > > Patch 2/2 would clearly change the &lock_cap in > vfio_pin_page_external() to a NULL, so only _remote passes a pointer > there. Thanks, > > Alex > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue 2017-04-17 19:32 ` Kirti Wankhede @ 2017-04-17 21:32 ` Alex Williamson 2017-04-18 2:54 ` Peter Xu 0 siblings, 1 reply; 14+ messages in thread From: Alex Williamson @ 2017-04-17 21:32 UTC (permalink / raw) To: Kirti Wankhede; +Cc: Peter Xu, kvm, eric.auger, linux-kernel, slp On Tue, 18 Apr 2017 01:02:12 +0530 Kirti Wankhede <kwankhede@nvidia.com> wrote: > On 4/18/2017 12:49 AM, Alex Williamson wrote: > > On Tue, 18 Apr 2017 00:35:06 +0530 > > Kirti Wankhede <kwankhede@nvidia.com> wrote: > > > >> On 4/17/2017 8:02 PM, Alex Williamson wrote: > >>> On Mon, 17 Apr 2017 14:47:54 +0800 > >>> Peter Xu <peterx@redhat.com> wrote: > >>> > >>>> On Sun, Apr 16, 2017 at 07:42:27PM -0600, Alex Williamson wrote: > >>>> > >>>> [...] > >>>> > >>>>> -static void vfio_lock_acct(struct task_struct *task, long npage) > >>>>> +static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) > >>>>> { > >>>>> - struct vwork *vwork; > >>>>> struct mm_struct *mm; > >>>>> bool is_current; > >>>>> + int ret; > >>>>> > >>>>> if (!npage) > >>>>> - return; > >>>>> + return 0; > >>>>> > >>>>> is_current = (task->mm == current->mm); > >>>>> > >>>>> mm = is_current ? task->mm : get_task_mm(task); > >>>>> if (!mm) > >>>>> - return; /* process exited */ > >>>>> + return -ESRCH; /* process exited */ > >>>>> > >>>>> - if (down_write_trylock(&mm->mmap_sem)) { > >>>>> - mm->locked_vm += npage; > >>>>> - up_write(&mm->mmap_sem); > >>>>> - if (!is_current) > >>>>> - mmput(mm); > >>>>> - return; > >>>>> - } > >>>>> + ret = down_write_killable(&mm->mmap_sem); > >>>>> + if (!ret) { > >>>>> + if (npage < 0 || lock_cap) { > >>>> > >>>> Nit: maybe we can avoid passing in lock_cap in all the callers of > >>>> vfio_lock_acct() and fetch it via has_capability() only if npage < 0? > >>>> IMHO that'll keep the vfio_lock_acct() interface cleaner, and we won't > >>>> need to pass in "false" any time when doing unpins. > >>> > >>> Unfortunately vfio_pin_pages_remote() needs to know about lock_cap > >>> since it tests whether the user is exceeding their locked memory > >>> limit. The other callers could certainly get away with > >>> vfio_lock_acct() testing the capability itself but that would add a > >>> redundant call for the most common user. I'm not a big fan of passing > >>> a lock_cap bool either, but it seemed the best fix for now. The > >>> cleanest alternative I can up with is this (untested): > >>> > >> > >> In my opinion, passing 'bool lock_cap' looks much clean and simple. > >> > >> Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com> > > > > Well shoot, I was just starting to warm up to the bool*. I like that > > we're not presuming the polarity for the callers we expect to be > > removing pages and I generally just dislike passing fixed bool > > parameters to change the function behavior. I've cleaned it up a bit > > further and was starting to do some testing on this which I'd propose > > for v5. Does it change your opinion? > > If passing fixed bool parameter is the concern then I would lean towards > Peter's suggestion. vfio_pin_pages_remote() will check lock capability > outside vfio_lock_acct() and again in vfio_lock_acct(). At other places, > it will be takes care within vfio_lock_acct() Sorry, I don't see that as a viable option. Testing for CAP_IPC_LOCK in both vfio_pin_pages_remote() and vfio_lock_acct() results in over a 10% performance hit on the mapping path with a custom micro-benchmark. In fact, it suggests we should probably pass that from even higher in the call stack. Thanks, Alex > > > > commit cd61c5f507d614ac14b75b0a548c8738deff88ea > > Author: Alex Williamson <alex.williamson@redhat.com> > > Date: Thu Apr 13 14:10:15 2017 -0600 > > > > vfio/type1: Remove locked page accounting workqueue > > > > If the mmap_sem is contented then the vfio type1 IOMMU backend will > > defer locked page accounting updates to a workqueue task. This has a > > few problems and depending on which side the user tries to play, they > > might be over-penalized for unmaps that haven't yet been accounted or > > race the workqueue to enter more mappings than they're allowed. The > > original intent of this workqueue mechanism seems to be focused on > > reducing latency through the ioctl, but we cannot do so at the cost > > of correctness. Remove this workqueue mechanism and update the > > callers to allow for failure. We can also now recheck the limit under > > write lock to make sure we don't exceed it. > > > > vfio_pin_pages_remote() also now necessarily includes an unwind path > > which we can jump to directly if the consecutive page pinning finds > > that we're exceeding the user's memory limits. This avoids the > > current lazy approach which does accounting and mapping up to the > > fault, only to return an error on the next iteration to unwind the > > entire vfio_dma. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > > index 32d2633092a3..a8a079ba9477 100644 > > --- a/drivers/vfio/vfio_iommu_type1.c > > +++ b/drivers/vfio/vfio_iommu_type1.c > > @@ -246,69 +246,46 @@ static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma, struct vfio_pfn *vpfn) > > return ret; > > } > > > > -struct vwork { > > - struct mm_struct *mm; > > - long npage; > > - struct work_struct work; > > -}; > > - > > -/* delayed decrement/increment for locked_vm */ > > -static void vfio_lock_acct_bg(struct work_struct *work) > > -{ > > - struct vwork *vwork = container_of(work, struct vwork, work); > > - struct mm_struct *mm; > > - > > - mm = vwork->mm; > > - down_write(&mm->mmap_sem); > > - mm->locked_vm += vwork->npage; > > - up_write(&mm->mmap_sem); > > - mmput(mm); > > - kfree(vwork); > > -} > > - > > -static void vfio_lock_acct(struct task_struct *task, long npage) > > +static int vfio_lock_acct(struct task_struct *task, long npage, bool *lock_cap) > > { > > - struct vwork *vwork; > > struct mm_struct *mm; > > bool is_current; > > + int ret; > > > > if (!npage) > > - return; > > + return 0; > > > > is_current = (task->mm == current->mm); > > > > mm = is_current ? task->mm : get_task_mm(task); > > if (!mm) > > - return; /* process exited */ > > + return -ESRCH; /* process exited */ > > > > - if (down_write_trylock(&mm->mmap_sem)) { > > - mm->locked_vm += npage; > > - up_write(&mm->mmap_sem); > > - if (!is_current) > > - mmput(mm); > > - return; > > - } > > + ret = down_write_killable(&mm->mmap_sem); > > + if (!ret) { > > + if (npage > 0) { > > + if (lock_cap ? !*lock_cap : > > + !has_capability(task, CAP_IPC_LOCK)) { > > + unsigned long limit; > > + > > + limit = task_rlimit(task, > > + RLIMIT_MEMLOCK) >> PAGE_SHIFT; > > + > > + if (mm->locked_vm + npage > limit) > > + ret = -ENOMEM; > > + } > > + } > > + > > + if (!ret) > > + mm->locked_vm += npage; > > > > - if (is_current) { > > - mm = get_task_mm(task); > > - if (!mm) > > - return; > > + up_write(&mm->mmap_sem); > > } > > > > - /* > > - * Couldn't get mmap_sem lock, so must setup to update > > - * mm->locked_vm later. If locked_vm were atomic, we > > - * wouldn't need this silliness > > - */ > > - vwork = kmalloc(sizeof(struct vwork), GFP_KERNEL); > > - if (WARN_ON(!vwork)) { > > + if (!is_current) > > mmput(mm); > > - return; > > - } > > - INIT_WORK(&vwork->work, vfio_lock_acct_bg); > > - vwork->mm = mm; > > - vwork->npage = npage; > > - schedule_work(&vwork->work); > > + > > + return ret; > > } > > > > /* > > @@ -405,7 +382,7 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, > > static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > > long npage, unsigned long *pfn_base) > > { > > - unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > > + unsigned long pfn = 0, limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > > bool lock_cap = capable(CAP_IPC_LOCK); > > long ret, pinned = 0, lock_acct = 0; > > bool rsvd; > > @@ -442,8 +419,6 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > > /* Lock all the consecutive pages from pfn_base */ > > for (vaddr += PAGE_SIZE, iova += PAGE_SIZE; pinned < npage; > > pinned++, vaddr += PAGE_SIZE, iova += PAGE_SIZE) { > > - unsigned long pfn = 0; > > - > > ret = vaddr_get_pfn(current->mm, vaddr, dma->prot, &pfn); > > if (ret) > > break; > > @@ -460,14 +435,25 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr, > > put_pfn(pfn, dma->prot); > > pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", > > __func__, limit << PAGE_SHIFT); > > - break; > > + ret = -ENOMEM; > > + goto unpin_out; > > } > > lock_acct++; > > } > > } > > > > out: > > - vfio_lock_acct(current, lock_acct); > > + ret = vfio_lock_acct(current, lock_acct, &lock_cap); > > + > > +unpin_out: > > + if (ret) { > > + if (!rsvd) { > > + for (pfn = *pfn_base ; pinned ; pfn++, pinned--) > > + put_pfn(pfn, dma->prot); > > + } > > + > > + return ret; > > + } > > > > return pinned; > > } > > @@ -488,7 +474,7 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova, > > } > > > > if (do_accounting) > > - vfio_lock_acct(dma->task, locked - unlocked); > > + vfio_lock_acct(dma->task, locked - unlocked, NULL); > > > > return unlocked; > > } > > @@ -522,8 +508,14 @@ static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr, > > goto pin_page_exit; > > } > > > > - if (!rsvd && do_accounting) > > - vfio_lock_acct(dma->task, 1); > > + if (!rsvd && do_accounting) { > > + ret = vfio_lock_acct(dma->task, 1, &lock_cap); > > + if (ret) { > > + put_pfn(*pfn_base, dma->prot); > > + goto pin_page_exit; > > + } > > + } > > + > > ret = 1; > > > > pin_page_exit: > > @@ -543,7 +535,7 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova, > > unlocked = vfio_iova_put_vfio_pfn(dma, vpfn); > > > > if (do_accounting) > > - vfio_lock_acct(dma->task, -unlocked); > > + vfio_lock_acct(dma->task, -unlocked, NULL); > > > > return unlocked; > > } > > @@ -740,7 +732,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, > > > > dma->iommu_mapped = false; > > if (do_accounting) { > > - vfio_lock_acct(dma->task, -unlocked); > > + vfio_lock_acct(dma->task, -unlocked, NULL); > > return 0; > > } > > return unlocked; > > @@ -1382,7 +1374,7 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) > > if (!is_invalid_reserved_pfn(vpfn->pfn)) > > locked++; > > } > > - vfio_lock_acct(dma->task, locked - unlocked); > > + vfio_lock_acct(dma->task, locked - unlocked, NULL); > > } > > } > > > > > > Patch 2/2 would clearly change the &lock_cap in > > vfio_pin_page_external() to a NULL, so only _remote passes a pointer > > there. Thanks, > > > > Alex > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue 2017-04-17 21:32 ` Alex Williamson @ 2017-04-18 2:54 ` Peter Xu 2017-04-18 18:21 ` Kirti Wankhede 0 siblings, 1 reply; 14+ messages in thread From: Peter Xu @ 2017-04-18 2:54 UTC (permalink / raw) To: Alex Williamson; +Cc: Kirti Wankhede, kvm, eric.auger, linux-kernel, slp On Mon, Apr 17, 2017 at 03:32:20PM -0600, Alex Williamson wrote: > On Tue, 18 Apr 2017 01:02:12 +0530 > Kirti Wankhede <kwankhede@nvidia.com> wrote: > > > On 4/18/2017 12:49 AM, Alex Williamson wrote: > > > On Tue, 18 Apr 2017 00:35:06 +0530 > > > Kirti Wankhede <kwankhede@nvidia.com> wrote: > > > > > >> On 4/17/2017 8:02 PM, Alex Williamson wrote: > > >>> On Mon, 17 Apr 2017 14:47:54 +0800 > > >>> Peter Xu <peterx@redhat.com> wrote: > > >>> > > >>>> On Sun, Apr 16, 2017 at 07:42:27PM -0600, Alex Williamson wrote: > > >>>> > > >>>> [...] > > >>>> > > >>>>> -static void vfio_lock_acct(struct task_struct *task, long npage) > > >>>>> +static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) > > >>>>> { > > >>>>> - struct vwork *vwork; > > >>>>> struct mm_struct *mm; > > >>>>> bool is_current; > > >>>>> + int ret; > > >>>>> > > >>>>> if (!npage) > > >>>>> - return; > > >>>>> + return 0; > > >>>>> > > >>>>> is_current = (task->mm == current->mm); > > >>>>> > > >>>>> mm = is_current ? task->mm : get_task_mm(task); > > >>>>> if (!mm) > > >>>>> - return; /* process exited */ > > >>>>> + return -ESRCH; /* process exited */ > > >>>>> > > >>>>> - if (down_write_trylock(&mm->mmap_sem)) { > > >>>>> - mm->locked_vm += npage; > > >>>>> - up_write(&mm->mmap_sem); > > >>>>> - if (!is_current) > > >>>>> - mmput(mm); > > >>>>> - return; > > >>>>> - } > > >>>>> + ret = down_write_killable(&mm->mmap_sem); > > >>>>> + if (!ret) { > > >>>>> + if (npage < 0 || lock_cap) { > > >>>> > > >>>> Nit: maybe we can avoid passing in lock_cap in all the callers of > > >>>> vfio_lock_acct() and fetch it via has_capability() only if npage < 0? > > >>>> IMHO that'll keep the vfio_lock_acct() interface cleaner, and we won't > > >>>> need to pass in "false" any time when doing unpins. > > >>> > > >>> Unfortunately vfio_pin_pages_remote() needs to know about lock_cap > > >>> since it tests whether the user is exceeding their locked memory > > >>> limit. The other callers could certainly get away with > > >>> vfio_lock_acct() testing the capability itself but that would add a > > >>> redundant call for the most common user. I'm not a big fan of passing > > >>> a lock_cap bool either, but it seemed the best fix for now. The > > >>> cleanest alternative I can up with is this (untested): > > >>> > > >> > > >> In my opinion, passing 'bool lock_cap' looks much clean and simple. > > >> > > >> Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com> > > > > > > Well shoot, I was just starting to warm up to the bool*. I like that > > > we're not presuming the polarity for the callers we expect to be > > > removing pages and I generally just dislike passing fixed bool > > > parameters to change the function behavior. I've cleaned it up a bit > > > further and was starting to do some testing on this which I'd propose > > > for v5. Does it change your opinion? > > > > If passing fixed bool parameter is the concern then I would lean towards > > Peter's suggestion. vfio_pin_pages_remote() will check lock capability > > outside vfio_lock_acct() and again in vfio_lock_acct(). At other places, > > it will be takes care within vfio_lock_acct() > > Sorry, I don't see that as a viable option. Testing for CAP_IPC_LOCK in > both vfio_pin_pages_remote() and vfio_lock_acct() results in over a > 10% performance hit on the mapping path with a custom micro-benchmark. > In fact, it suggests we should probably pass that from even higher in > the call stack. Thanks, Sorry I wasn't aware of such a performance degradation with such a change. Then I would be perfectly fine with either current patch, or the new one you proposed (with bool *). Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue 2017-04-18 2:54 ` Peter Xu @ 2017-04-18 18:21 ` Kirti Wankhede 0 siblings, 0 replies; 14+ messages in thread From: Kirti Wankhede @ 2017-04-18 18:21 UTC (permalink / raw) To: Peter Xu, Alex Williamson; +Cc: kvm, eric.auger, linux-kernel, slp On 4/18/2017 8:24 AM, Peter Xu wrote: > On Mon, Apr 17, 2017 at 03:32:20PM -0600, Alex Williamson wrote: >> On Tue, 18 Apr 2017 01:02:12 +0530 >> Kirti Wankhede <kwankhede@nvidia.com> wrote: >> >>> On 4/18/2017 12:49 AM, Alex Williamson wrote: >>>> On Tue, 18 Apr 2017 00:35:06 +0530 >>>> Kirti Wankhede <kwankhede@nvidia.com> wrote: >>>> >>>>> On 4/17/2017 8:02 PM, Alex Williamson wrote: >>>>>> On Mon, 17 Apr 2017 14:47:54 +0800 >>>>>> Peter Xu <peterx@redhat.com> wrote: >>>>>> >>>>>>> On Sun, Apr 16, 2017 at 07:42:27PM -0600, Alex Williamson wrote: >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>> -static void vfio_lock_acct(struct task_struct *task, long npage) >>>>>>>> +static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap) >>>>>>>> { >>>>>>>> - struct vwork *vwork; >>>>>>>> struct mm_struct *mm; >>>>>>>> bool is_current; >>>>>>>> + int ret; >>>>>>>> >>>>>>>> if (!npage) >>>>>>>> - return; >>>>>>>> + return 0; >>>>>>>> >>>>>>>> is_current = (task->mm == current->mm); >>>>>>>> >>>>>>>> mm = is_current ? task->mm : get_task_mm(task); >>>>>>>> if (!mm) >>>>>>>> - return; /* process exited */ >>>>>>>> + return -ESRCH; /* process exited */ >>>>>>>> >>>>>>>> - if (down_write_trylock(&mm->mmap_sem)) { >>>>>>>> - mm->locked_vm += npage; >>>>>>>> - up_write(&mm->mmap_sem); >>>>>>>> - if (!is_current) >>>>>>>> - mmput(mm); >>>>>>>> - return; >>>>>>>> - } >>>>>>>> + ret = down_write_killable(&mm->mmap_sem); >>>>>>>> + if (!ret) { >>>>>>>> + if (npage < 0 || lock_cap) { >>>>>>> >>>>>>> Nit: maybe we can avoid passing in lock_cap in all the callers of >>>>>>> vfio_lock_acct() and fetch it via has_capability() only if npage < 0? >>>>>>> IMHO that'll keep the vfio_lock_acct() interface cleaner, and we won't >>>>>>> need to pass in "false" any time when doing unpins. >>>>>> >>>>>> Unfortunately vfio_pin_pages_remote() needs to know about lock_cap >>>>>> since it tests whether the user is exceeding their locked memory >>>>>> limit. The other callers could certainly get away with >>>>>> vfio_lock_acct() testing the capability itself but that would add a >>>>>> redundant call for the most common user. I'm not a big fan of passing >>>>>> a lock_cap bool either, but it seemed the best fix for now. The >>>>>> cleanest alternative I can up with is this (untested): >>>>>> >>>>> >>>>> In my opinion, passing 'bool lock_cap' looks much clean and simple. >>>>> >>>>> Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com> >>>> >>>> Well shoot, I was just starting to warm up to the bool*. I like that >>>> we're not presuming the polarity for the callers we expect to be >>>> removing pages and I generally just dislike passing fixed bool >>>> parameters to change the function behavior. I've cleaned it up a bit >>>> further and was starting to do some testing on this which I'd propose >>>> for v5. Does it change your opinion? >>> >>> If passing fixed bool parameter is the concern then I would lean towards >>> Peter's suggestion. vfio_pin_pages_remote() will check lock capability >>> outside vfio_lock_acct() and again in vfio_lock_acct(). At other places, >>> it will be takes care within vfio_lock_acct() >> >> Sorry, I don't see that as a viable option. Testing for CAP_IPC_LOCK in >> both vfio_pin_pages_remote() and vfio_lock_acct() results in over a >> 10% performance hit on the mapping path with a custom micro-benchmark. >> In fact, it suggests we should probably pass that from even higher in >> the call stack. Thanks, > > Sorry I wasn't aware of such a performance degradation with such a > change. Then I would be perfectly fine with either current patch, or > the new one you proposed (with bool *). Thanks, > Sorry, even I wasn't aware of. Looking at v5 version now. Thanks, Kirti ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external() 2017-04-17 1:42 [PATCH v4 0/2] vfio/type1: Synchronous locked page accounting Alex Williamson 2017-04-17 1:42 ` [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue Alex Williamson @ 2017-04-17 1:42 ` Alex Williamson 2017-04-17 6:54 ` Peter Xu 2017-04-17 19:16 ` Kirti Wankhede 1 sibling, 2 replies; 14+ messages in thread From: Alex Williamson @ 2017-04-17 1:42 UTC (permalink / raw) To: alex.williamson, kvm; +Cc: eric.auger, kwankhede, peterx, linux-kernel, slp With vfio_lock_acct() testing the locked memory limit under mmap_sem, it's redundant to do it here for a single page. We can also reorder our tests such that we can avoid testing for reserved pages if we're not doing accounting, and test the process CAP_IPC_LOCK only if we are doing accounting. Finally, this function oddly returns 1 on success. Update to return zero on success, -errno on error. Since the function only pins a single page, there's no need to return the number of pages pinned. N.B. vfio_pin_pages_remote() can pin a large contiguous range of pages before calling vfio_lock_acct(). If we were to similarly remove the extra test there, a user could temporarily pin far more pages than they're allowed. Suggested-by: Kirti Wankhede <kwankhede@nvidia.com> Suggested-by: Peter Xu <peterx@redhat.com> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- drivers/vfio/vfio_iommu_type1.c | 34 +++++----------------------------- 1 file changed, 5 insertions(+), 29 deletions(-) diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index fb18e4a5df62..07e0e58f22e9 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -479,43 +479,21 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova, static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr, unsigned long *pfn_base, bool do_accounting) { - unsigned long limit; - bool lock_cap = has_capability(dma->task, CAP_IPC_LOCK); struct mm_struct *mm; int ret; - bool rsvd; mm = get_task_mm(dma->task); if (!mm) return -ENODEV; ret = vaddr_get_pfn(mm, vaddr, dma->prot, pfn_base); - if (ret) - goto pin_page_exit; - - rsvd = is_invalid_reserved_pfn(*pfn_base); - limit = task_rlimit(dma->task, RLIMIT_MEMLOCK) >> PAGE_SHIFT; - - if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) { - put_pfn(*pfn_base, dma->prot); - pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n", - __func__, dma->task->comm, task_pid_nr(dma->task), - limit << PAGE_SHIFT); - ret = -ENOMEM; - goto pin_page_exit; - } - - if (!rsvd && do_accounting) { - ret = vfio_lock_acct(dma->task, 1, lock_cap); - if (ret) { + if (!ret && do_accounting && !is_invalid_reserved_pfn(*pfn_base)) { + ret = vfio_lock_acct(dma->task, 1, + has_capability(dma->task, CAP_IPC_LOCK)); + if (ret) put_pfn(*pfn_base, dma->prot); - goto pin_page_exit; - } } - ret = 1; - -pin_page_exit: mmput(mm); return ret; } @@ -595,10 +573,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data, remote_vaddr = dma->vaddr + iova - dma->iova; ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i], do_accounting); - if (ret <= 0) { - WARN_ON(!ret); + if (ret) goto pin_unwind; - } ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]); if (ret) { ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external() 2017-04-17 1:42 ` [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external() Alex Williamson @ 2017-04-17 6:54 ` Peter Xu 2017-04-17 17:20 ` Alex Williamson 2017-04-17 19:16 ` Kirti Wankhede 1 sibling, 1 reply; 14+ messages in thread From: Peter Xu @ 2017-04-17 6:54 UTC (permalink / raw) To: Alex Williamson; +Cc: kvm, eric.auger, kwankhede, linux-kernel, slp On Sun, Apr 16, 2017 at 07:42:39PM -0600, Alex Williamson wrote: > With vfio_lock_acct() testing the locked memory limit under mmap_sem, > it's redundant to do it here for a single page. We can also reorder > our tests such that we can avoid testing for reserved pages if we're > not doing accounting, and test the process CAP_IPC_LOCK only if we > are doing accounting. Finally, this function oddly returns 1 on > success. Update to return zero on success, -errno on error. Since > the function only pins a single page, there's no need to return the > number of pages pinned. > > N.B. vfio_pin_pages_remote() can pin a large contiguous range of pages > before calling vfio_lock_acct(). If we were to similarly remove the > extra test there, a user could temporarily pin far more pages than > they're allowed. > > Suggested-by: Kirti Wankhede <kwankhede@nvidia.com> > Suggested-by: Peter Xu <peterx@redhat.com> Maybe this suggested-by honor should be for Kirti only? :) For the patch, I think it's good to me as long as we have the accounting check in vfio_lock_acct() which is just introduced in previous patch, so: Reviewed-by: Peter Xu <peterx@redhat.com> Thanks! -- Peter Xu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external() 2017-04-17 6:54 ` Peter Xu @ 2017-04-17 17:20 ` Alex Williamson 0 siblings, 0 replies; 14+ messages in thread From: Alex Williamson @ 2017-04-17 17:20 UTC (permalink / raw) To: Peter Xu; +Cc: kvm, eric.auger, kwankhede, linux-kernel, slp On Mon, 17 Apr 2017 14:54:21 +0800 Peter Xu <peterx@redhat.com> wrote: > On Sun, Apr 16, 2017 at 07:42:39PM -0600, Alex Williamson wrote: > > With vfio_lock_acct() testing the locked memory limit under mmap_sem, > > it's redundant to do it here for a single page. We can also reorder > > our tests such that we can avoid testing for reserved pages if we're > > not doing accounting, and test the process CAP_IPC_LOCK only if we > > are doing accounting. Finally, this function oddly returns 1 on > > success. Update to return zero on success, -errno on error. Since > > the function only pins a single page, there's no need to return the > > number of pages pinned. > > > > N.B. vfio_pin_pages_remote() can pin a large contiguous range of pages > > before calling vfio_lock_acct(). If we were to similarly remove the > > extra test there, a user could temporarily pin far more pages than > > they're allowed. > > > > Suggested-by: Kirti Wankhede <kwankhede@nvidia.com> > > Suggested-by: Peter Xu <peterx@redhat.com> > > Maybe this suggested-by honor should be for Kirti only? :) Sorry, I mis-attributed this, Eric suggested that vfio_pin_page_external() should have a standard return value. I'll change the Suggested-by. > For the patch, I think it's good to me as long as we have the > accounting check in vfio_lock_acct() which is just introduced in > previous patch, so: > > Reviewed-by: Peter Xu <peterx@redhat.com> Thanks! ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external() 2017-04-17 1:42 ` [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external() Alex Williamson 2017-04-17 6:54 ` Peter Xu @ 2017-04-17 19:16 ` Kirti Wankhede 1 sibling, 0 replies; 14+ messages in thread From: Kirti Wankhede @ 2017-04-17 19:16 UTC (permalink / raw) To: Alex Williamson, kvm; +Cc: eric.auger, peterx, linux-kernel, slp On 4/17/2017 7:12 AM, Alex Williamson wrote: > With vfio_lock_acct() testing the locked memory limit under mmap_sem, > it's redundant to do it here for a single page. We can also reorder > our tests such that we can avoid testing for reserved pages if we're > not doing accounting, and test the process CAP_IPC_LOCK only if we > are doing accounting. Finally, this function oddly returns 1 on > success. Update to return zero on success, -errno on error. Since > the function only pins a single page, there's no need to return the > number of pages pinned. > > N.B. vfio_pin_pages_remote() can pin a large contiguous range of pages > before calling vfio_lock_acct(). If we were to similarly remove the > extra test there, a user could temporarily pin far more pages than > they're allowed. > > Suggested-by: Kirti Wankhede <kwankhede@nvidia.com> > Suggested-by: Peter Xu <peterx@redhat.com> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > drivers/vfio/vfio_iommu_type1.c | 34 +++++----------------------------- > 1 file changed, 5 insertions(+), 29 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index fb18e4a5df62..07e0e58f22e9 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -479,43 +479,21 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova, > static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr, > unsigned long *pfn_base, bool do_accounting) > { > - unsigned long limit; > - bool lock_cap = has_capability(dma->task, CAP_IPC_LOCK); > struct mm_struct *mm; > int ret; > - bool rsvd; > > mm = get_task_mm(dma->task); > if (!mm) > return -ENODEV; > > ret = vaddr_get_pfn(mm, vaddr, dma->prot, pfn_base); > - if (ret) > - goto pin_page_exit; > - > - rsvd = is_invalid_reserved_pfn(*pfn_base); > - limit = task_rlimit(dma->task, RLIMIT_MEMLOCK) >> PAGE_SHIFT; > - > - if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) { > - put_pfn(*pfn_base, dma->prot); > - pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n", > - __func__, dma->task->comm, task_pid_nr(dma->task), > - limit << PAGE_SHIFT); > - ret = -ENOMEM; > - goto pin_page_exit; > - } > - > - if (!rsvd && do_accounting) { > - ret = vfio_lock_acct(dma->task, 1, lock_cap); > - if (ret) { > + if (!ret && do_accounting && !is_invalid_reserved_pfn(*pfn_base)) { > + ret = vfio_lock_acct(dma->task, 1, > + has_capability(dma->task, CAP_IPC_LOCK)); > + if (ret) > put_pfn(*pfn_base, dma->prot); > - goto pin_page_exit; > - } > } > > - ret = 1; > - > -pin_page_exit: > mmput(mm); > return ret; > } Thanks. This looks clean. Just a nit pick, if vfio_lock_acct() returns -ENOMEM, its better to have warning about task's mlock limit exceeded, which got removed in the cleanup. No need to review again. Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com> Thanks, Kirti > @@ -595,10 +573,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data, > remote_vaddr = dma->vaddr + iova - dma->iova; > ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i], > do_accounting); > - if (ret <= 0) { > - WARN_ON(!ret); > + if (ret) > goto pin_unwind; > - } > > ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]); > if (ret) { > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-04-18 18:21 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-17 1:42 [PATCH v4 0/2] vfio/type1: Synchronous locked page accounting Alex Williamson 2017-04-17 1:42 ` [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue Alex Williamson 2017-04-17 6:47 ` Peter Xu 2017-04-17 14:32 ` Alex Williamson 2017-04-17 19:05 ` Kirti Wankhede 2017-04-17 19:19 ` Alex Williamson 2017-04-17 19:32 ` Kirti Wankhede 2017-04-17 21:32 ` Alex Williamson 2017-04-18 2:54 ` Peter Xu 2017-04-18 18:21 ` Kirti Wankhede 2017-04-17 1:42 ` [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external() Alex Williamson 2017-04-17 6:54 ` Peter Xu 2017-04-17 17:20 ` Alex Williamson 2017-04-17 19:16 ` Kirti Wankhede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).