Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755327AbdDQTFa (ORCPT ); Mon, 17 Apr 2017 15:05:30 -0400 Received: from nat-hk.nvidia.com ([203.18.50.4]:14006 "EHLO nat-hk.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750883AbdDQTFY (ORCPT ); Mon, 17 Apr 2017 15:05:24 -0400 X-PGP-Universal: processed; by hkpgpgate101.nvidia.com on Mon, 17 Apr 2017 12:05:22 -0700 Subject: Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue To: Alex Williamson , Peter Xu References: <20170417014142.25866.16852.stgit@gimli.home> <20170417014227.25866.59899.stgit@gimli.home> <20170417064754.GC16703@pxdev.xzpeter.org> <20170417083201.043cedbf@t450s.home> CC: , , , X-Nvconfidentiality: public From: Kirti Wankhede Message-ID: <71c0bcf5-9d73-c875-aa3f-482472027e7a@nvidia.com> Date: Tue, 18 Apr 2017 00:35:06 +0530 MIME-Version: 1.0 In-Reply-To: <20170417083201.043cedbf@t450s.home> X-Originating-IP: [10.24.71.218] X-ClientProxiedBy: DRBGMAIL104.nvidia.com (10.18.16.23) To DRBGMAIL102.nvidia.com (10.18.16.21) Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7225 Lines: 226 On 4/17/2017 8:02 PM, Alex Williamson wrote: > On Mon, 17 Apr 2017 14:47:54 +0800 > Peter Xu 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 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; >>> } >> >