From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932793AbdDFQxV (ORCPT ); Thu, 6 Apr 2017 12:53:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44190 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752828AbdDFQxK (ORCPT ); Thu, 6 Apr 2017 12:53:10 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7135780F98 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=eric.auger@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 7135780F98 Subject: Re: [PATCH v2] vfio/type1: Remove locked page accounting workqueue To: Alex Williamson , kvm@vger.kernel.org References: <20170406145250.16956.95264.stgit@gimli.home> Cc: kwankhede@nvidia.com, linux-kernel@vger.kernel.org, slp@redhat.com From: Auger Eric Message-ID: <6927e4ef-2589-1c3c-3055-93abe9df2eb5@redhat.com> Date: Thu, 6 Apr 2017 18:53:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170406145250.16956.95264.stgit@gimli.home> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 06 Apr 2017 16:53:10 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alex, On 06/04/2017 16:53, Alex Williamson wrote: > 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 able to race the workqueue to enter more mappings > than they're allowed. It's not entirely clear what motivated this > workqueue mechanism in the original vfio design, but it seems to > introduce more problems than it solves, so remove it 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. > > Cc: stable@vger.kernel.org > Signed-off-by: Alex Williamson Looks good to me. Reviewed-by: Eric Auger Thanks Eric > --- > > v2: Fixed missed mmput on failure to acquire mmap_sem as noted by Eric > > drivers/vfio/vfio_iommu_type1.c | 101 ++++++++++++++++++--------------------- > 1 file changed, 46 insertions(+), 55 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 32d2633092a3..b799edbb8c4f 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -246,69 +246,45 @@ 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) > +static int vfio_lock_acct(struct task_struct *task, long npage) > { > - 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) > -{ > - 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) { > + mm->locked_vm += npage; > + } else { > + unsigned long limit; > + > + limit = is_current ? > + rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT : > + task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT; > + > + if (mm->locked_vm + npage <= limit) > + mm->locked_vm += npage; > + else > + ret = -ENOMEM; > + } > > - 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 +381,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 +418,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 +434,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); > + > +unpin_out: > + if (ret) { > + if (!rsvd) { > + for (pfn = *pfn_base ; pinned ; pfn++, pinned--) > + put_pfn(pfn, dma->prot); > + } > + > + return ret; > + } > > return pinned; > } > @@ -522,8 +507,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); > + if (ret) { > + put_pfn(*pfn_base, dma->prot); > + goto pin_page_exit; > + } > + } > + > ret = 1; > > pin_page_exit: >