From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753032AbdDQOcQ (ORCPT ); Mon, 17 Apr 2017 10:32:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47954 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750776AbdDQOcM (ORCPT ); Mon, 17 Apr 2017 10:32:12 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com DEA60C07F671 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=alex.williamson@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com DEA60C07F671 Date: Mon, 17 Apr 2017 08:32:01 -0600 From: Alex Williamson To: Peter Xu Cc: kvm@vger.kernel.org, eric.auger@redhat.com, kwankhede@nvidia.com, linux-kernel@vger.kernel.org, slp@redhat.com Subject: Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue Message-ID: <20170417083201.043cedbf@t450s.home> In-Reply-To: <20170417064754.GC16703@pxdev.xzpeter.org> References: <20170417014142.25866.16852.stgit@gimli.home> <20170417014227.25866.59899.stgit@gimli.home> <20170417064754.GC16703@pxdev.xzpeter.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Mon, 17 Apr 2017 14:32:12 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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): 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; > > } >