diff mbox series

[5/6] drivers/IB,usnic: reduce scope of mmap_sem

Message ID 20190115181300.27547-6-dave@stgolabs.net (mailing list archive)
State New, archived
Headers show
Series mm: make pinned_vm atomic and simplify users | expand

Commit Message

Davidlohr Bueso Jan. 15, 2019, 6:12 p.m. UTC
usnic_uiom_get_pages() uses gup_longterm() so we cannot really
get rid of mmap_sem altogether in the driver, but we can get
rid of some complexity that mmap_sem brings with only pinned_vm.
We can get rid of the wq altogether as we no longer need to
defer work to unpin pages as the counter is now atomic.

Cc: benve@cisco.com
Cc: neescoba@cisco.com
Cc: pkaustub@cisco.com
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 drivers/infiniband/hw/usnic/usnic_ib_main.c |  2 --
 drivers/infiniband/hw/usnic/usnic_uiom.c    | 54 +++--------------------------
 drivers/infiniband/hw/usnic/usnic_uiom.h    |  1 -
 3 files changed, 4 insertions(+), 53 deletions(-)

Comments

Ira Weiny Jan. 15, 2019, 8:30 p.m. UTC | #1
On Tue, Jan 15, 2019 at 10:12:59AM -0800, Davidlohr Bueso wrote:
> usnic_uiom_get_pages() uses gup_longterm() so we cannot really
> get rid of mmap_sem altogether in the driver, but we can get
> rid of some complexity that mmap_sem brings with only pinned_vm.
> We can get rid of the wq altogether as we no longer need to
> defer work to unpin pages as the counter is now atomic.
> 
> Cc: benve@cisco.com
> Cc: neescoba@cisco.com
> Cc: pkaustub@cisco.com
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>

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

> ---
>  drivers/infiniband/hw/usnic/usnic_ib_main.c |  2 --
>  drivers/infiniband/hw/usnic/usnic_uiom.c    | 54 +++--------------------------
>  drivers/infiniband/hw/usnic/usnic_uiom.h    |  1 -
>  3 files changed, 4 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/usnic/usnic_ib_main.c b/drivers/infiniband/hw/usnic/usnic_ib_main.c
> index b2323a52a0dd..64bc4fda36bf 100644
> --- a/drivers/infiniband/hw/usnic/usnic_ib_main.c
> +++ b/drivers/infiniband/hw/usnic/usnic_ib_main.c
> @@ -691,7 +691,6 @@ static int __init usnic_ib_init(void)
>  out_pci_unreg:
>  	pci_unregister_driver(&usnic_ib_pci_driver);
>  out_umem_fini:
> -	usnic_uiom_fini();
>  
>  	return err;
>  }
> @@ -704,7 +703,6 @@ static void __exit usnic_ib_destroy(void)
>  	unregister_inetaddr_notifier(&usnic_ib_inetaddr_notifier);
>  	unregister_netdevice_notifier(&usnic_ib_netdevice_notifier);
>  	pci_unregister_driver(&usnic_ib_pci_driver);
> -	usnic_uiom_fini();
>  }
>  
>  MODULE_DESCRIPTION("Cisco VIC (usNIC) Verbs Driver");
> diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.c b/drivers/infiniband/hw/usnic/usnic_uiom.c
> index 22c40c432b9e..555d7bc93e72 100644
> --- a/drivers/infiniband/hw/usnic/usnic_uiom.c
> +++ b/drivers/infiniband/hw/usnic/usnic_uiom.c
> @@ -47,8 +47,6 @@
>  #include "usnic_uiom.h"
>  #include "usnic_uiom_interval_tree.h"
>  
> -static struct workqueue_struct *usnic_uiom_wq;
> -
>  #define USNIC_UIOM_PAGE_CHUNK						\
>  	((PAGE_SIZE - offsetof(struct usnic_uiom_chunk, page_list))	/\
>  	((void *) &((struct usnic_uiom_chunk *) 0)->page_list[1] -	\
> @@ -129,7 +127,7 @@ static int usnic_uiom_get_pages(unsigned long addr, size_t size, int writable,
>  	uiomr->owning_mm = mm = current->mm;
>  	down_write(&mm->mmap_sem);
>  
> -	locked = npages + atomic_long_read(&current->mm->pinned_vm);
> +	locked = atomic_long_add_return(npages, &current->mm->pinned_vm);
>  	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>  
>  	if ((locked > lock_limit) && !capable(CAP_IPC_LOCK)) {
> @@ -185,12 +183,11 @@ static int usnic_uiom_get_pages(unsigned long addr, size_t size, int writable,
>  	}
>  
>  out:
> -	if (ret < 0)
> +	if (ret < 0) {
>  		usnic_uiom_put_pages(chunk_list, 0);
> -	else {
> -		atomic_long_set(&mm->pinned_vm, locked);
> +		atomic_long_sub(npages, &current->mm->pinned_vm);
> +	} else
>  		mmgrab(uiomr->owning_mm);
> -	}
>  
>  	up_write(&mm->mmap_sem);
>  	free_page((unsigned long) page_list);
> @@ -436,43 +433,12 @@ static inline size_t usnic_uiom_num_pages(struct usnic_uiom_reg *uiomr)
>  	return PAGE_ALIGN(uiomr->length + uiomr->offset) >> PAGE_SHIFT;
>  }
>  
> -static void usnic_uiom_release_defer(struct work_struct *work)
> -{
> -	struct usnic_uiom_reg *uiomr =
> -		container_of(work, struct usnic_uiom_reg, work);
> -
> -	down_write(&uiomr->owning_mm->mmap_sem);
> -	atomic_long_sub(usnic_uiom_num_pages(uiomr), &uiomr->owning_mm->pinned_vm);
> -	up_write(&uiomr->owning_mm->mmap_sem);
> -
> -	__usnic_uiom_release_tail(uiomr);
> -}
> -
>  void usnic_uiom_reg_release(struct usnic_uiom_reg *uiomr,
>  			    struct ib_ucontext *context)
>  {
>  	__usnic_uiom_reg_release(uiomr->pd, uiomr, 1);
>  
> -	/*
> -	 * We may be called with the mm's mmap_sem already held.  This
> -	 * can happen when a userspace munmap() is the call that drops
> -	 * the last reference to our file and calls our release
> -	 * method.  If there are memory regions to destroy, we'll end
> -	 * up here and not be able to take the mmap_sem.  In that case
> -	 * we defer the vm_locked accounting to a workqueue.
> -	 */
> -	if (context->closing) {
> -		if (!down_write_trylock(&uiomr->owning_mm->mmap_sem)) {
> -			INIT_WORK(&uiomr->work, usnic_uiom_release_defer);
> -			queue_work(usnic_uiom_wq, &uiomr->work);
> -			return;
> -		}
> -	} else {
> -		down_write(&uiomr->owning_mm->mmap_sem);
> -	}
>  	atomic_long_sub(usnic_uiom_num_pages(uiomr), &uiomr->owning_mm->pinned_vm);
> -	up_write(&uiomr->owning_mm->mmap_sem);
> -
>  	__usnic_uiom_release_tail(uiomr);
>  }
>  
> @@ -601,17 +567,5 @@ int usnic_uiom_init(char *drv_name)
>  		return -EPERM;
>  	}
>  
> -	usnic_uiom_wq = create_workqueue(drv_name);
> -	if (!usnic_uiom_wq) {
> -		usnic_err("Unable to alloc wq for drv %s\n", drv_name);
> -		return -ENOMEM;
> -	}
> -
>  	return 0;
>  }
> -
> -void usnic_uiom_fini(void)
> -{
> -	flush_workqueue(usnic_uiom_wq);
> -	destroy_workqueue(usnic_uiom_wq);
> -}
> diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.h b/drivers/infiniband/hw/usnic/usnic_uiom.h
> index b86a9731071b..c88cfa087e3a 100644
> --- a/drivers/infiniband/hw/usnic/usnic_uiom.h
> +++ b/drivers/infiniband/hw/usnic/usnic_uiom.h
> @@ -93,5 +93,4 @@ struct usnic_uiom_reg *usnic_uiom_reg_get(struct usnic_uiom_pd *pd,
>  void usnic_uiom_reg_release(struct usnic_uiom_reg *uiomr,
>  			    struct ib_ucontext *ucontext);
>  int usnic_uiom_init(char *drv_name);
> -void usnic_uiom_fini(void);
>  #endif /* USNIC_UIOM_H_ */
> -- 
> 2.16.4
>
Parvi Kaustubhi (pkaustub) Jan. 17, 2019, 11:41 p.m. UTC | #2
usnic driver was tested with this.

Acked-by: Parvi Kaustubhi <pkaustub@cisco.com>


> On Jan 15, 2019, at 10:12 AM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> 
> usnic_uiom_get_pages() uses gup_longterm() so we cannot really
> get rid of mmap_sem altogether in the driver, but we can get
> rid of some complexity that mmap_sem brings with only pinned_vm.
> We can get rid of the wq altogether as we no longer need to
> defer work to unpin pages as the counter is now atomic.
> 
> Cc: benve@cisco.com
> Cc: neescoba@cisco.com
> Cc: pkaustub@cisco.com
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
> drivers/infiniband/hw/usnic/usnic_ib_main.c |  2 --
> drivers/infiniband/hw/usnic/usnic_uiom.c    | 54 +++--------------------------
> drivers/infiniband/hw/usnic/usnic_uiom.h    |  1 -
> 3 files changed, 4 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/usnic/usnic_ib_main.c b/drivers/infiniband/hw/usnic/usnic_ib_main.c
> index b2323a52a0dd..64bc4fda36bf 100644
> --- a/drivers/infiniband/hw/usnic/usnic_ib_main.c
> +++ b/drivers/infiniband/hw/usnic/usnic_ib_main.c
> @@ -691,7 +691,6 @@ static int __init usnic_ib_init(void)
> out_pci_unreg:
> 	pci_unregister_driver(&usnic_ib_pci_driver);
> out_umem_fini:
> -	usnic_uiom_fini();
> 
> 	return err;
> }
> @@ -704,7 +703,6 @@ static void __exit usnic_ib_destroy(void)
> 	unregister_inetaddr_notifier(&usnic_ib_inetaddr_notifier);
> 	unregister_netdevice_notifier(&usnic_ib_netdevice_notifier);
> 	pci_unregister_driver(&usnic_ib_pci_driver);
> -	usnic_uiom_fini();
> }
> 
> MODULE_DESCRIPTION("Cisco VIC (usNIC) Verbs Driver");
> diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.c b/drivers/infiniband/hw/usnic/usnic_uiom.c
> index 22c40c432b9e..555d7bc93e72 100644
> --- a/drivers/infiniband/hw/usnic/usnic_uiom.c
> +++ b/drivers/infiniband/hw/usnic/usnic_uiom.c
> @@ -47,8 +47,6 @@
> #include "usnic_uiom.h"
> #include "usnic_uiom_interval_tree.h"
> 
> -static struct workqueue_struct *usnic_uiom_wq;
> -
> #define USNIC_UIOM_PAGE_CHUNK						\
> 	((PAGE_SIZE - offsetof(struct usnic_uiom_chunk, page_list))	/\
> 	((void *) &((struct usnic_uiom_chunk *) 0)->page_list[1] -	\
> @@ -129,7 +127,7 @@ static int usnic_uiom_get_pages(unsigned long addr, size_t size, int writable,
> 	uiomr->owning_mm = mm = current->mm;
> 	down_write(&mm->mmap_sem);
> 
> -	locked = npages + atomic_long_read(&current->mm->pinned_vm);
> +	locked = atomic_long_add_return(npages, &current->mm->pinned_vm);
> 	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> 
> 	if ((locked > lock_limit) && !capable(CAP_IPC_LOCK)) {
> @@ -185,12 +183,11 @@ static int usnic_uiom_get_pages(unsigned long addr, size_t size, int writable,
> 	}
> 
> out:
> -	if (ret < 0)
> +	if (ret < 0) {
> 		usnic_uiom_put_pages(chunk_list, 0);
> -	else {
> -		atomic_long_set(&mm->pinned_vm, locked);
> +		atomic_long_sub(npages, &current->mm->pinned_vm);
> +	} else
> 		mmgrab(uiomr->owning_mm);
> -	}
> 
> 	up_write(&mm->mmap_sem);
> 	free_page((unsigned long) page_list);
> @@ -436,43 +433,12 @@ static inline size_t usnic_uiom_num_pages(struct usnic_uiom_reg *uiomr)
> 	return PAGE_ALIGN(uiomr->length + uiomr->offset) >> PAGE_SHIFT;
> }
> 
> -static void usnic_uiom_release_defer(struct work_struct *work)
> -{
> -	struct usnic_uiom_reg *uiomr =
> -		container_of(work, struct usnic_uiom_reg, work);
> -
> -	down_write(&uiomr->owning_mm->mmap_sem);
> -	atomic_long_sub(usnic_uiom_num_pages(uiomr), &uiomr->owning_mm->pinned_vm);
> -	up_write(&uiomr->owning_mm->mmap_sem);
> -
> -	__usnic_uiom_release_tail(uiomr);
> -}
> -
> void usnic_uiom_reg_release(struct usnic_uiom_reg *uiomr,
> 			    struct ib_ucontext *context)
> {
> 	__usnic_uiom_reg_release(uiomr->pd, uiomr, 1);
> 
> -	/*
> -	 * We may be called with the mm's mmap_sem already held.  This
> -	 * can happen when a userspace munmap() is the call that drops
> -	 * the last reference to our file and calls our release
> -	 * method.  If there are memory regions to destroy, we'll end
> -	 * up here and not be able to take the mmap_sem.  In that case
> -	 * we defer the vm_locked accounting to a workqueue.
> -	 */
> -	if (context->closing) {
> -		if (!down_write_trylock(&uiomr->owning_mm->mmap_sem)) {
> -			INIT_WORK(&uiomr->work, usnic_uiom_release_defer);
> -			queue_work(usnic_uiom_wq, &uiomr->work);
> -			return;
> -		}
> -	} else {
> -		down_write(&uiomr->owning_mm->mmap_sem);
> -	}
> 	atomic_long_sub(usnic_uiom_num_pages(uiomr), &uiomr->owning_mm->pinned_vm);
> -	up_write(&uiomr->owning_mm->mmap_sem);
> -
> 	__usnic_uiom_release_tail(uiomr);
> }
> 
> @@ -601,17 +567,5 @@ int usnic_uiom_init(char *drv_name)
> 		return -EPERM;
> 	}
> 
> -	usnic_uiom_wq = create_workqueue(drv_name);
> -	if (!usnic_uiom_wq) {
> -		usnic_err("Unable to alloc wq for drv %s\n", drv_name);
> -		return -ENOMEM;
> -	}
> -
> 	return 0;
> }
> -
> -void usnic_uiom_fini(void)
> -{
> -	flush_workqueue(usnic_uiom_wq);
> -	destroy_workqueue(usnic_uiom_wq);
> -}
> diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.h b/drivers/infiniband/hw/usnic/usnic_uiom.h
> index b86a9731071b..c88cfa087e3a 100644
> --- a/drivers/infiniband/hw/usnic/usnic_uiom.h
> +++ b/drivers/infiniband/hw/usnic/usnic_uiom.h
> @@ -93,5 +93,4 @@ struct usnic_uiom_reg *usnic_uiom_reg_get(struct usnic_uiom_pd *pd,
> void usnic_uiom_reg_release(struct usnic_uiom_reg *uiomr,
> 			    struct ib_ucontext *ucontext);
> int usnic_uiom_init(char *drv_name);
> -void usnic_uiom_fini(void);
> #endif /* USNIC_UIOM_H_ */
> -- 
> 2.16.4
>
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/usnic/usnic_ib_main.c b/drivers/infiniband/hw/usnic/usnic_ib_main.c
index b2323a52a0dd..64bc4fda36bf 100644
--- a/drivers/infiniband/hw/usnic/usnic_ib_main.c
+++ b/drivers/infiniband/hw/usnic/usnic_ib_main.c
@@ -691,7 +691,6 @@  static int __init usnic_ib_init(void)
 out_pci_unreg:
 	pci_unregister_driver(&usnic_ib_pci_driver);
 out_umem_fini:
-	usnic_uiom_fini();
 
 	return err;
 }
@@ -704,7 +703,6 @@  static void __exit usnic_ib_destroy(void)
 	unregister_inetaddr_notifier(&usnic_ib_inetaddr_notifier);
 	unregister_netdevice_notifier(&usnic_ib_netdevice_notifier);
 	pci_unregister_driver(&usnic_ib_pci_driver);
-	usnic_uiom_fini();
 }
 
 MODULE_DESCRIPTION("Cisco VIC (usNIC) Verbs Driver");
diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.c b/drivers/infiniband/hw/usnic/usnic_uiom.c
index 22c40c432b9e..555d7bc93e72 100644
--- a/drivers/infiniband/hw/usnic/usnic_uiom.c
+++ b/drivers/infiniband/hw/usnic/usnic_uiom.c
@@ -47,8 +47,6 @@ 
 #include "usnic_uiom.h"
 #include "usnic_uiom_interval_tree.h"
 
-static struct workqueue_struct *usnic_uiom_wq;
-
 #define USNIC_UIOM_PAGE_CHUNK						\
 	((PAGE_SIZE - offsetof(struct usnic_uiom_chunk, page_list))	/\
 	((void *) &((struct usnic_uiom_chunk *) 0)->page_list[1] -	\
@@ -129,7 +127,7 @@  static int usnic_uiom_get_pages(unsigned long addr, size_t size, int writable,
 	uiomr->owning_mm = mm = current->mm;
 	down_write(&mm->mmap_sem);
 
-	locked = npages + atomic_long_read(&current->mm->pinned_vm);
+	locked = atomic_long_add_return(npages, &current->mm->pinned_vm);
 	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
 
 	if ((locked > lock_limit) && !capable(CAP_IPC_LOCK)) {
@@ -185,12 +183,11 @@  static int usnic_uiom_get_pages(unsigned long addr, size_t size, int writable,
 	}
 
 out:
-	if (ret < 0)
+	if (ret < 0) {
 		usnic_uiom_put_pages(chunk_list, 0);
-	else {
-		atomic_long_set(&mm->pinned_vm, locked);
+		atomic_long_sub(npages, &current->mm->pinned_vm);
+	} else
 		mmgrab(uiomr->owning_mm);
-	}
 
 	up_write(&mm->mmap_sem);
 	free_page((unsigned long) page_list);
@@ -436,43 +433,12 @@  static inline size_t usnic_uiom_num_pages(struct usnic_uiom_reg *uiomr)
 	return PAGE_ALIGN(uiomr->length + uiomr->offset) >> PAGE_SHIFT;
 }
 
-static void usnic_uiom_release_defer(struct work_struct *work)
-{
-	struct usnic_uiom_reg *uiomr =
-		container_of(work, struct usnic_uiom_reg, work);
-
-	down_write(&uiomr->owning_mm->mmap_sem);
-	atomic_long_sub(usnic_uiom_num_pages(uiomr), &uiomr->owning_mm->pinned_vm);
-	up_write(&uiomr->owning_mm->mmap_sem);
-
-	__usnic_uiom_release_tail(uiomr);
-}
-
 void usnic_uiom_reg_release(struct usnic_uiom_reg *uiomr,
 			    struct ib_ucontext *context)
 {
 	__usnic_uiom_reg_release(uiomr->pd, uiomr, 1);
 
-	/*
-	 * We may be called with the mm's mmap_sem already held.  This
-	 * can happen when a userspace munmap() is the call that drops
-	 * the last reference to our file and calls our release
-	 * method.  If there are memory regions to destroy, we'll end
-	 * up here and not be able to take the mmap_sem.  In that case
-	 * we defer the vm_locked accounting to a workqueue.
-	 */
-	if (context->closing) {
-		if (!down_write_trylock(&uiomr->owning_mm->mmap_sem)) {
-			INIT_WORK(&uiomr->work, usnic_uiom_release_defer);
-			queue_work(usnic_uiom_wq, &uiomr->work);
-			return;
-		}
-	} else {
-		down_write(&uiomr->owning_mm->mmap_sem);
-	}
 	atomic_long_sub(usnic_uiom_num_pages(uiomr), &uiomr->owning_mm->pinned_vm);
-	up_write(&uiomr->owning_mm->mmap_sem);
-
 	__usnic_uiom_release_tail(uiomr);
 }
 
@@ -601,17 +567,5 @@  int usnic_uiom_init(char *drv_name)
 		return -EPERM;
 	}
 
-	usnic_uiom_wq = create_workqueue(drv_name);
-	if (!usnic_uiom_wq) {
-		usnic_err("Unable to alloc wq for drv %s\n", drv_name);
-		return -ENOMEM;
-	}
-
 	return 0;
 }
-
-void usnic_uiom_fini(void)
-{
-	flush_workqueue(usnic_uiom_wq);
-	destroy_workqueue(usnic_uiom_wq);
-}
diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.h b/drivers/infiniband/hw/usnic/usnic_uiom.h
index b86a9731071b..c88cfa087e3a 100644
--- a/drivers/infiniband/hw/usnic/usnic_uiom.h
+++ b/drivers/infiniband/hw/usnic/usnic_uiom.h
@@ -93,5 +93,4 @@  struct usnic_uiom_reg *usnic_uiom_reg_get(struct usnic_uiom_pd *pd,
 void usnic_uiom_reg_release(struct usnic_uiom_reg *uiomr,
 			    struct ib_ucontext *ucontext);
 int usnic_uiom_init(char *drv_name);
-void usnic_uiom_fini(void);
 #endif /* USNIC_UIOM_H_ */