Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1618093 > unrolled thread

[PATCH v2] vfio/type1: Remove locked page accounting workqueue

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] vfio/type1: Remove locked page accounting workqueue Alex Williamson <alex.williamson@redhat.com> - 2017-04-06 17:00 +0200
    Re: [PATCH v2] vfio/type1: Remove locked page accounting workqueue Auger Eric <eric.auger@redhat.com> - 2017-04-06 19:00 +0200
      Re: [PATCH v2] vfio/type1: Remove locked page accounting workqueue Alex Williamson <alex.williamson@redhat.com> - 2017-04-06 19:10 +0200
        Re: [PATCH v2] vfio/type1: Remove locked page accounting workqueue Alex Williamson <alex.williamson@redhat.com> - 2017-04-06 21:40 +0200

#1618093 — [PATCH v2] 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 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 <alex.williamson@redhat.com>
---

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:

[toc] | [next] | [standalone]

#1618191

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 <alex.williamson@redhat.com>
Looks good to me.

Reviewed-by: Eric Auger <eric.auger@redhat.com>

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:
> 

[toc] | [prev] | [next] | [standalone]

#1618199

On Thu, 6 Apr 2017 18:53:04 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> 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 <alex.williamson@redhat.com>  
> Looks good to me.
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 

One possible enhancement below.

> 
> > ---
> > 
> > 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);


Hmm, since we're already testing current, I wonder if it makes sense to
have this bimodal, killable if current, straight down_write()
otherwise.  I'm not too sure how important it is to use killable
regardless, but mlock does, which seemed like a good model to follow.
Thanks,

Alex

> > +	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:
> >   

[toc] | [prev] | [next] | [standalone]

#1618301

On Thu, 6 Apr 2017 11:05:31 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Thu, 6 Apr 2017 18:53:04 +0200
> Auger Eric <eric.auger@redhat.com> wrote:
> 
> > 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 <alex.williamson@redhat.com>    
> > Looks good to me.
> > 
> > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> >   
> 
> One possible enhancement below.
> 
> >   
> > > ---
> > > 
> > > 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);  
> 
> 
> Hmm, since we're already testing current, I wonder if it makes sense to
> have this bimodal, killable if current, straight down_write()
> otherwise.  I'm not too sure how important it is to use killable
> regardless, but mlock does, which seemed like a good model to follow.

I see other examples in the kernel where killable is used even for
remote mm, which would match the external page pinning usage, so I think
I'll keep this as is unless there are other comments for v2.  Thanks
for the review.

Alex

> > > +	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:
> > >     
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web