On Tue, Jan 15, 2019 at 10:12:56AM -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.
>
> Cc: sudeep.dutt@xxxxxxxxx
> Cc: ashutosh.dixit@xxxxxxxxx
> Signed-off-by: Davidlohr Bueso <dbueso@xxxxxxx>
Reviewed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> ---
> drivers/misc/mic/scif/scif_rma.c | 36 +++++++++++-------------------------
> 1 file changed, 11 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/misc/mic/scif/scif_rma.c b/drivers/misc/mic/scif/scif_rma.c
> index a92b4d6f099c..445529ce2ad7 100644
> --- a/drivers/misc/mic/scif/scif_rma.c
> +++ b/drivers/misc/mic/scif/scif_rma.c
> @@ -272,21 +272,12 @@ static inline void __scif_release_mm(struct mm_struct *mm)
>
> static inline int
> __scif_dec_pinned_vm_lock(struct mm_struct *mm,
> - int nr_pages, bool try_lock)
> + int nr_pages)
> {
> if (!mm || !nr_pages || !scif_ulimit_check)
> return 0;
> - if (try_lock) {
> - if (!down_write_trylock(&mm->mmap_sem)) {
> - dev_err(scif_info.mdev.this_device,
> - "%s %d err\n", __func__, __LINE__);
> - return -1;
> - }
> - } else {
> - down_write(&mm->mmap_sem);
> - }
> +
> atomic_long_sub(nr_pages, &mm->pinned_vm);
> - up_write(&mm->mmap_sem);
> return 0;
> }
>
> @@ -298,16 +289,16 @@ static inline int __scif_check_inc_pinned_vm(struct mm_struct *mm,
> if (!mm || !nr_pages || !scif_ulimit_check)
> return 0;
>
> - locked = nr_pages;
> - locked += atomic_long_read(&mm->pinned_vm);
> lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> + locked = atomic_long_add_return(nr_pages, &mm->pinned_vm);
> +
> if ((locked > lock_limit) && !capable(CAP_IPC_LOCK)) {
> + atomic_long_sub(nr_pages, &mm->pinned_vm);
> dev_err(scif_info.mdev.this_device,
> "locked(%lu) > lock_limit(%lu)\n",
> locked, lock_limit);
> return -ENOMEM;
> }
> - atomic_long_set(&mm->pinned_vm, locked);
> return 0;
> }
>
> @@ -326,7 +317,7 @@ int scif_destroy_window(struct scif_endpt *ep, struct scif_window *window)
>
> might_sleep();
> if (!window->temp && window->mm) {
> - __scif_dec_pinned_vm_lock(window->mm, window->nr_pages, 0);
> + __scif_dec_pinned_vm_lock(window->mm, window->nr_pages);
> __scif_release_mm(window->mm);
> window->mm = NULL;
> }
> @@ -737,7 +728,7 @@ int scif_unregister_window(struct scif_window *window)
> ep->rma_info.dma_chan);
> } else {
> if (!__scif_dec_pinned_vm_lock(window->mm,
> - window->nr_pages, 1)) {
> + window->nr_pages)) {
> __scif_release_mm(window->mm);
> window->mm = NULL;
> }
> @@ -1385,28 +1376,23 @@ int __scif_pin_pages(void *addr, size_t len, int *out_prot,
> prot |= SCIF_PROT_WRITE;
> retry:
> mm = current->mm;
> - down_write(&mm->mmap_sem);
> if (ulimit) {
> err = __scif_check_inc_pinned_vm(mm, nr_pages);
> if (err) {
> - up_write(&mm->mmap_sem);
> pinned_pages->nr_pages = 0;
> goto error_unmap;
> }
> }
>
> - pinned_pages->nr_pages = get_user_pages(
> + pinned_pages->nr_pages = get_user_pages_fast(
> (u64)addr,
> nr_pages,
> (prot & SCIF_PROT_WRITE) ? FOLL_WRITE : 0,
> - pinned_pages->pages,
> - NULL);
> - up_write(&mm->mmap_sem);
> + pinned_pages->pages);
> if (nr_pages != pinned_pages->nr_pages) {
> if (try_upgrade) {
> if (ulimit)
> - __scif_dec_pinned_vm_lock(mm,
> - nr_pages, 0);
> + __scif_dec_pinned_vm_lock(mm, nr_pages);
> /* Roll back any pinned pages */
> for (i = 0; i < pinned_pages->nr_pages; i++) {
> if (pinned_pages->pages[i])
> @@ -1433,7 +1419,7 @@ int __scif_pin_pages(void *addr, size_t len, int *out_prot,
> return err;
> dec_pinned:
> if (ulimit)
> - __scif_dec_pinned_vm_lock(mm, nr_pages, 0);
> + __scif_dec_pinned_vm_lock(mm, nr_pages);
> /* Something went wrong! Rollback */
> error_unmap:
> pinned_pages->nr_pages = nr_pages;
> --
> 2.16.4
>
