Re: [PATCH rdma-next v2 2/2] RDMA/umem: Refactor exit paths in ib_umem_get

[Leon Romanovsky <leon@xxxxxxxxxx>]

On Mon, Jul 09, 2018 at 07:50:03PM +0000, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma-
> >owner@xxxxxxxxxxxxxxx] On Behalf Of Leon Romanovsky
> >Sent: Sunday, July 8, 2018 4:59 AM
> >To: Doug Ledford <dledford@xxxxxxxxxx>; Jason Gunthorpe
> ><jgg@xxxxxxxxxxxx>
> >Cc: Leon Romanovsky <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux-
> >rdma@xxxxxxxxxxxxxxx>; Huy Nguyen <huyn@xxxxxxxxxxxx>
> >Subject: [PATCH rdma-next v2 2/2] RDMA/umem: Refactor exit paths in
> >ib_umem_get
> >
> >From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >
> >Simplify exit paths in ib_umem_get.
> >
> >Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >---
> > drivers/infiniband/core/umem.c | 44 +++++++++++++++++++------------------
> >-----
> > 1 file changed, 20 insertions(+), 24 deletions(-)
> >
> >diff --git a/drivers/infiniband/core/umem.c
> >b/drivers/infiniband/core/umem.c
> >index abe9924baf7c..2ef5f69aaef0 100644
> >--- a/drivers/infiniband/core/umem.c
> >+++ b/drivers/infiniband/core/umem.c
> >@@ -91,7 +91,6 @@ struct ib_umem *ib_umem_get(struct ib_ucontext
> >*context, unsigned long addr,
> > 	int i;
> > 	unsigned long dma_attrs = 0;
> > 	struct scatterlist *sg, *sg_list_start;
> >-	int need_release = 0;
> > 	unsigned int gup_flags = FOLL_WRITE;
> >
> > 	if (dmasync)
> >@@ -120,10 +119,8 @@ struct ib_umem *ib_umem_get(struct ib_ucontext
> >*context, unsigned long addr,
> >
> > 	if (access & IB_ACCESS_ON_DEMAND) {
> > 		ret = ib_umem_odp_get(context, umem, access);
> >-		if (ret) {
> >-			kfree(umem);
> >-			return ERR_PTR(ret);
> >-		}
> >+		if (ret)
> >+			goto umem;
> > 		return umem;
>
> Having a variable and a target name be the same is a little disconcerting.
>
> Perhaps name the target 'err' or 'fail', etc.?

Oh, thanks.
I'll fix.

>
> Mike
>
>
> > 	}
> >
> >@@ -134,8 +131,8 @@ struct ib_umem *ib_umem_get(struct ib_ucontext
> >*context, unsigned long addr,
> >
> > 	page_list = (struct page **) __get_free_page(GFP_KERNEL);
> > 	if (!page_list) {
> >-		kfree(umem);
> >-		return ERR_PTR(-ENOMEM);
> >+		ret = -ENOMEM;
> >+		goto umem;
> > 	}
> >
> > 	/*
> >@@ -155,7 +152,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext
> >*context, unsigned long addr,
> > 	if ((current->mm->pinned_vm > lock_limit) &&
> >!capable(CAP_IPC_LOCK)) {
> > 		up_write(&current->mm->mmap_sem);
> > 		ret = -ENOMEM;
> >-		goto out;
> >+		goto vma;
> > 	}
> > 	up_write(&current->mm->mmap_sem);
> >
> >@@ -163,17 +160,16 @@ struct ib_umem *ib_umem_get(struct ib_ucontext
> >*context, unsigned long addr,
> >
> > 	if (npages == 0 || npages > UINT_MAX) {
> > 		ret = -EINVAL;
> >-		goto out;
> >+		goto vma;
> > 	}
> >
> > 	ret = sg_alloc_table(&umem->sg_head, npages, GFP_KERNEL);
> > 	if (ret)
> >-		goto out;
> >+		goto vma;
> >
> > 	if (!umem->writable)
> > 		gup_flags |= FOLL_FORCE;
> >
> >-	need_release = 1;
> > 	sg_list_start = umem->sg_head.sgl;
> >
> > 	down_read(&current->mm->mmap_sem);
> >@@ -184,7 +180,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext
> >*context, unsigned long addr,
> > 				     gup_flags, page_list, vma_list);
> > 		if (ret < 0) {
> > 			up_read(&current->mm->mmap_sem);
> >-			goto out;
> >+			goto umem_release;
> > 		}
> >
> > 		umem->npages += ret;
> >@@ -211,26 +207,26 @@ struct ib_umem *ib_umem_get(struct ib_ucontext
> >*context, unsigned long addr,
> >
> > 	if (!umem->nmap) {
> > 		ret = -ENOMEM;
> >-		goto out;
> >+		goto umem_release;
> > 	}
> >
> > 	ret = 0;
> >+	goto out;
> >
> >+umem_release:
> >+	__ib_umem_release(context->device, umem, 0);
> >+vma:
> >+	down_write(&current->mm->mmap_sem);
> >+	current->mm->pinned_vm -= ib_umem_num_pages(umem);
> >+	up_write(&current->mm->mmap_sem);
> > out:
> >-	if (ret < 0) {
> >-		down_write(&current->mm->mmap_sem);
> >-		current->mm->pinned_vm -= ib_umem_num_pages(umem);
> >-		up_write(&current->mm->mmap_sem);
> >-		if (need_release)
> >-			__ib_umem_release(context->device, umem, 0);
> >-		kfree(umem);
> >-	}
> >-
> > 	if (vma_list)
> > 		free_page((unsigned long) vma_list);
> > 	free_page((unsigned long) page_list);
> >-
> >-	return ret < 0 ? ERR_PTR(ret) : umem;
> >+umem:
> >+	if (ret)
> >+		kfree(umem);
> >+	return ret ? ERR_PTR(ret) : umem;
> > }
> > EXPORT_SYMBOL(ib_umem_get);
> >
> >--
> >2.14.4
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> >the body of a message to majordomo@xxxxxxxxxxxxxxx
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html

Attachment: signature.asc
Description: PGP signature