Re: [PATCH 3/6] drivers/IB,qib: do not use mmap_sem

[Ira Weiny <ira.weiny@xxxxxxxxx>]

On Tue, Jan 15, 2019 at 10:12:57AM -0800, Davidlohr Bueso wrote:
> The driver uses mmap_sem for both pinned_vm accounting and
> get_user_pages(). By using gup_fast() and letting the mm handle
> the lock if needed, we can no longer rely on the semaphore and
> simplify the whole thing as the pinning is decoupled from the lock.
> 
> This also fixes a bug that __qib_get_user_pages was not taking into
> account the current value of pinned_vm.
> 
> Cc: dennis.dalessandro@xxxxxxxxx
> Cc: mike.marciniszyn@xxxxxxxxx
> Signed-off-by: Davidlohr Bueso <dbueso@xxxxxxx>

Reviewed-by: Ira Weiny <ira.weiny@xxxxxxxxx>

> ---
>  drivers/infiniband/hw/qib/qib_user_pages.c | 67 ++++++++++--------------------
>  1 file changed, 22 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/qib/qib_user_pages.c b/drivers/infiniband/hw/qib/qib_user_pages.c
> index 981795b23b73..4b7a5be782e6 100644
> --- a/drivers/infiniband/hw/qib/qib_user_pages.c
> +++ b/drivers/infiniband/hw/qib/qib_user_pages.c
> @@ -49,43 +49,6 @@ static void __qib_release_user_pages(struct page **p, size_t num_pages,
>  	}
>  }
>  
> -/*
> - * Call with current->mm->mmap_sem held.
> - */
> -static int __qib_get_user_pages(unsigned long start_page, size_t num_pages,
> -				struct page **p)
> -{
> -	unsigned long lock_limit;
> -	size_t got;
> -	int ret;
> -
> -	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> -
> -	if (num_pages > lock_limit && !capable(CAP_IPC_LOCK)) {
> -		ret = -ENOMEM;
> -		goto bail;
> -	}
> -
> -	for (got = 0; got < num_pages; got += ret) {
> -		ret = get_user_pages(start_page + got * PAGE_SIZE,
> -				     num_pages - got,
> -				     FOLL_WRITE | FOLL_FORCE,
> -				     p + got, NULL);
> -		if (ret < 0)
> -			goto bail_release;
> -	}
> -
> -	atomic_long_add(num_pages, &current->mm->pinned_vm);
> -
> -	ret = 0;
> -	goto bail;
> -
> -bail_release:
> -	__qib_release_user_pages(p, got, 0);
> -bail:
> -	return ret;
> -}
> -
>  /**
>   * qib_map_page - a safety wrapper around pci_map_page()
>   *
> @@ -137,26 +100,40 @@ int qib_map_page(struct pci_dev *hwdev, struct page *page, dma_addr_t *daddr)
>  int qib_get_user_pages(unsigned long start_page, size_t num_pages,
>  		       struct page **p)
>  {
> +	unsigned long locked, lock_limit;
> +	size_t got;
>  	int ret;
>  
> -	down_write(&current->mm->mmap_sem);
> +	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> +	locked = atomic_long_add_return(num_pages, &current->mm->pinned_vm);
>  
> -	ret = __qib_get_user_pages(start_page, num_pages, p);
> +	if (locked > lock_limit && !capable(CAP_IPC_LOCK)) {
> +		ret = -ENOMEM;
> +		goto bail;
> +	}
>  
> -	up_write(&current->mm->mmap_sem);
> +	for (got = 0; got < num_pages; got += ret) {
> +		ret = get_user_pages_fast(start_page + got * PAGE_SIZE,
> +				     num_pages - got,
> +				     FOLL_WRITE | FOLL_FORCE,
> +				     p + got);
> +		if (ret < 0)
> +			goto bail_release;
> +	}
>  
> +	return 0;
> +bail_release:
> +	__qib_release_user_pages(p, got, 0);
> +bail:
> +	atomic_long_sub(num_pages, &current->mm->pinned_vm);
>  	return ret;
>  }
>  
>  void qib_release_user_pages(struct page **p, size_t num_pages)
>  {
> -	if (current->mm) /* during close after signal, mm can be NULL */
> -		down_write(&current->mm->mmap_sem);
> -
>  	__qib_release_user_pages(p, num_pages, 1);
>  
> -	if (current->mm) {
> +	if (current->mm) { /* during close after signal, mm can be NULL */
>  		atomic_long_sub(num_pages, &current->mm->pinned_vm);
> -		up_write(&current->mm->mmap_sem);
>  	}
>  }
> -- 
> 2.16.4
>