Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755908AbcLTShi (ORCPT ); Tue, 20 Dec 2016 13:37:38 -0500 Received: from hqemgate15.nvidia.com ([216.228.121.64]:13350 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754523AbcLTShf (ORCPT ); Tue, 20 Dec 2016 13:37:35 -0500 X-PGP-Universal: processed; by hqnvupgp08.nvidia.com on Tue, 20 Dec 2016 10:37:27 -0800 Subject: Re: [PATCH v2] vfio/type1: Restore mapping performance with mdev support To: Alex Williamson References: <20161219221046.4719.17454.stgit@gimli.home> CC: , , X-Nvconfidentiality: public From: Kirti Wankhede Message-ID: Date: Wed, 21 Dec 2016 00:07:18 +0530 MIME-Version: 1.0 In-Reply-To: <20161219221046.4719.17454.stgit@gimli.home> X-Originating-IP: [10.25.72.251] X-ClientProxiedBy: BGMAIL104.nvidia.com (10.25.59.13) To bgmail102.nvidia.com (10.25.59.11) Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6113 Lines: 204 On 12/20/2016 3:43 AM, Alex Williamson wrote: > As part of the mdev support, type1 now gets a task reference per > vfio_dma and uses that to get an mm reference for the task while > working on accounting. That's correct, but it's not fast. For some > paths, like vfio_pin_pages_remote(), we know we're only called from > user context, so we can restore the lighter weight calls. In other > cases, we're effectively already testing whether we're in the stored > task context elsewhere, extend this vfio_lock_acct() as well. > > Signed-off-by: Alex Williamson > --- > > v2: Use (mm == current->mm) test in vfio_lock_acct() as well rather > than passing around is_current. It doesn't make sense to keep it > in vaddr_get_pfn() and not use it elsewhere. > Thanks Alex. This change looks good to me. Reviewed by: Kirti Wankhede Thanks, Kirti > drivers/vfio/vfio_iommu_type1.c | 98 ++++++++++++++++++++------------------- > 1 file changed, 51 insertions(+), 47 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 9815e45..7154987 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -268,28 +268,38 @@ static void vfio_lock_acct(struct task_struct *task, long npage) > { > struct vwork *vwork; > struct mm_struct *mm; > + bool is_current; > > if (!npage) > return; > > - mm = get_task_mm(task); > + is_current = (task->mm == current->mm); > + > + mm = is_current ? task->mm : get_task_mm(task); > if (!mm) > - return; /* process exited or nothing to do */ > + return; /* process exited */ > > if (down_write_trylock(&mm->mmap_sem)) { > mm->locked_vm += npage; > up_write(&mm->mmap_sem); > - mmput(mm); > + if (!is_current) > + mmput(mm); > return; > } > > + if (is_current) { > + mm = get_task_mm(task); > + if (!mm) > + return; > + } > + > /* > * 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 (!vwork) { > + if (WARN_ON(!vwork)) { > mmput(mm); > return; > } > @@ -393,77 +403,71 @@ 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; > - bool lock_cap = ns_capable(task_active_pid_ns(dma->task)->user_ns, > - CAP_IPC_LOCK); > - struct mm_struct *mm; > - long ret, i = 0, lock_acct = 0; > + unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > + bool lock_cap = capable(CAP_IPC_LOCK); > + long ret, pinned = 0, lock_acct = 0; > bool rsvd; > dma_addr_t iova = vaddr - dma->vaddr + dma->iova; > > - mm = get_task_mm(dma->task); > - if (!mm) > + /* This code path is only user initiated */ > + if (!current->mm) > return -ENODEV; > > - ret = vaddr_get_pfn(mm, vaddr, dma->prot, pfn_base); > + ret = vaddr_get_pfn(current->mm, vaddr, dma->prot, pfn_base); > if (ret) > - goto pin_pg_remote_exit; > + return ret; > > + pinned++; > rsvd = is_invalid_reserved_pfn(*pfn_base); > - limit = task_rlimit(dma->task, RLIMIT_MEMLOCK) >> PAGE_SHIFT; > > /* > * Reserved pages aren't counted against the user, externally pinned > * pages are already counted against the user. > */ > if (!rsvd && !vfio_find_vpfn(dma, iova)) { > - if (!lock_cap && mm->locked_vm + 1 > limit) { > + if (!lock_cap && current->mm->locked_vm + 1 > limit) { > put_pfn(*pfn_base, dma->prot); > pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", __func__, > limit << PAGE_SHIFT); > - ret = -ENOMEM; > - goto pin_pg_remote_exit; > + return -ENOMEM; > } > lock_acct++; > } > > - i++; > - if (likely(!disable_hugepages)) { > - /* Lock all the consecutive pages from pfn_base */ > - for (vaddr += PAGE_SIZE, iova += PAGE_SIZE; i < npage; > - i++, vaddr += PAGE_SIZE, iova += PAGE_SIZE) { > - unsigned long pfn = 0; > + if (unlikely(disable_hugepages)) > + goto out; > > - ret = vaddr_get_pfn(mm, vaddr, dma->prot, &pfn); > - if (ret) > - break; > + /* 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; > > - if (pfn != *pfn_base + i || > - rsvd != is_invalid_reserved_pfn(pfn)) { > + ret = vaddr_get_pfn(current->mm, vaddr, dma->prot, &pfn); > + if (ret) > + break; > + > + if (pfn != *pfn_base + pinned || > + rsvd != is_invalid_reserved_pfn(pfn)) { > + put_pfn(pfn, dma->prot); > + break; > + } > + > + if (!rsvd && !vfio_find_vpfn(dma, iova)) { > + if (!lock_cap && > + current->mm->locked_vm + lock_acct + 1 > limit) { > put_pfn(pfn, dma->prot); > + pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", > + __func__, limit << PAGE_SHIFT); > break; > } > - > - if (!rsvd && !vfio_find_vpfn(dma, iova)) { > - if (!lock_cap && > - mm->locked_vm + lock_acct + 1 > limit) { > - put_pfn(pfn, dma->prot); > - pr_warn("%s: RLIMIT_MEMLOCK (%ld) " > - "exceeded\n", __func__, > - limit << PAGE_SHIFT); > - break; > - } > - lock_acct++; > - } > + lock_acct++; > } > } > > - vfio_lock_acct(dma->task, lock_acct); > - ret = i; > +out: > + vfio_lock_acct(current, lock_acct); > > -pin_pg_remote_exit: > - mmput(mm); > - return ret; > + return pinned; > } > > static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova, > @@ -473,10 +477,10 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova, > long unlocked = 0, locked = 0; > long i; > > - for (i = 0; i < npage; i++) { > + for (i = 0; i < npage; i++, iova += PAGE_SIZE) { > if (put_pfn(pfn++, dma->prot)) { > unlocked++; > - if (vfio_find_vpfn(dma, iova + (i << PAGE_SHIFT))) > + if (vfio_find_vpfn(dma, iova)) > locked++; > } > } >