[PATCH for-rc 0/6] vmw_pvrdma fixes for 4.15

[PATCH for-rc 0/6] vmw_pvrdma fixes for 4.15

[Bryan Tan]

Do some cleanup and fixes related to the comments received during SRQ
support upstreaming that applied to existing code. Also add in two
missed macros used by the userlevel library for SRQs, a missed
ib_umem_release, and an incorrect usage of the new refcount_t type.

Bryan Tan (6):
  RDMA/vmw_pvrdma: Clarify QP is_kernel logic
  RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path
  RDMA/vmw_pvrdma: Use more specific sizeof in kcalloc
  RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning
  RDMA/vmw_pvrdma: Use refcount_t instead of atomic_t
  RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file

 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  4 ++--
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c   |  6 +++---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 19 ++++++++-----------
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c   | 20 +++++++++++++-------
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c  |  4 ++--
 include/uapi/rdma/vmw_pvrdma-abi.h             |  2 ++
 6 files changed, 30 insertions(+), 25 deletions(-)

-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic

[Bryan Tan]

Be more consistent in checking is_kernel flag for QPs.

Testing Done: ibv_rc_pingpong, rping, perftests.

Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
index 10420a1..b932b7e 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
@@ -249,8 +249,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 		init_waitqueue_head(&qp->wait);
 
 		qp->state = IB_QPS_RESET;
+		qp->is_kernel = !(pd->uobject && udata);
 
-		if (pd->uobject && udata) {
+		if (!qp->is_kernel) {
 			dev_dbg(&dev->pdev->dev,
 				"create queuepair from user space\n");
 
@@ -291,8 +292,6 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 				qp->npages_recv = 0;
 			qp->npages = qp->npages_send + qp->npages_recv;
 		} else {
-			qp->is_kernel = true;
-
 			ret = pvrdma_set_sq_size(to_vdev(pd->device),
 						 &init_attr->cap, qp);
 			if (ret)
@@ -394,7 +393,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 err_pdir:
 	pvrdma_page_dir_cleanup(dev, &qp->pdir);
 err_umem:
-	if (pd->uobject && udata) {
+	if (!qp->is_kernel) {
 		if (qp->rumem)
 			ib_umem_release(qp->rumem);
 		if (qp->sumem)
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path

[Bryan Tan]

The QP cleanup did not previously call ib_umem_release. Fix this.

Testing Done: ibv_rc_pingpong, rping, perftests.

Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
index b932b7e..77e7e57 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
@@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
 	atomic_dec(&qp->refcnt);
 	wait_event(qp->wait, !atomic_read(&qp->refcnt));
 
+	if (!qp->is_kernel) {
+		if (qp->rumem)
+			ib_umem_release(qp->rumem);
+		if (qp->sumem)
+			ib_umem_release(qp->sumem);
+	}
+
 	pvrdma_page_dir_cleanup(dev, &qp->pdir);
 
 	kfree(qp);
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH for-rc 3/6] RDMA/vmw_pvrdma: Use more specific sizeof in kcalloc

[Bryan Tan]

Convert the sizeof(void *) in two kcalloc calls to be more
specific for the arrays that are being allocated.

Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
index 1f4e187..941e324 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
@@ -243,13 +243,13 @@ static int pvrdma_register_device(struct pvrdma_dev *dev)
 	mutex_init(&dev->port_mutex);
 	spin_lock_init(&dev->desc_lock);
 
-	dev->cq_tbl = kcalloc(dev->dsr->caps.max_cq, sizeof(void *),
+	dev->cq_tbl = kcalloc(dev->dsr->caps.max_cq, sizeof(struct pvrdma_cq *),
 			      GFP_KERNEL);
 	if (!dev->cq_tbl)
 		return ret;
 	spin_lock_init(&dev->cq_tbl_lock);
 
-	dev->qp_tbl = kcalloc(dev->dsr->caps.max_qp, sizeof(void *),
+	dev->qp_tbl = kcalloc(dev->dsr->caps.max_qp, sizeof(struct pvrdma_qp *),
 			      GFP_KERNEL);
 	if (!dev->qp_tbl)
 		goto err_cq_free;
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Bryan Tan]

refcount_dec generates a warning when the operation
causes the refcount to hit zero. Avoid this by using
refcount_dec_and_test.

Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
index 826ccb8..a2b1a3c 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
@@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq)
 	dev->srq_tbl[srq->srq_handle] = NULL;
 	spin_unlock_irqrestore(&dev->srq_tbl_lock, flags);
 
-	refcount_dec(&srq->refcnt);
-	wait_event(srq->wait, !refcount_read(&srq->refcnt));
+	if (!refcount_dec_and_test(&srq->refcnt))
+		wait_event(srq->wait, !refcount_read(&srq->refcnt));
 
 	/* There is no support for kernel clients, so this is safe. */
 	ib_umem_release(srq->umem);
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH for-rc 5/6] RDMA/vmw_pvrdma: Use refcount_t instead of atomic_t

[Bryan Tan]

refcount_t is the preferred type for refcounts. Change the
QP and CQ refcnt fields to use refcount_t.

Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      |  4 ++--
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c   |  6 +++---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 15 ++++++---------
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c   |  6 +++---
 4 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
index 63bc2ef..07d287e 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
@@ -93,7 +93,7 @@ struct pvrdma_cq {
 	struct pvrdma_page_dir pdir;
 	u32 cq_handle;
 	bool is_kernel;
-	atomic_t refcnt;
+	refcount_t refcnt;
 	wait_queue_head_t wait;
 };
 
@@ -196,7 +196,7 @@ struct pvrdma_qp {
 	u8 state;
 	bool is_kernel;
 	struct mutex mutex; /* QP state mutex. */
-	atomic_t refcnt;
+	refcount_t refcnt;
 	wait_queue_head_t wait;
 };
 
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
index 3562c0c..11cda60 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c
@@ -178,7 +178,7 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
 	else
 		pvrdma_page_dir_insert_umem(&cq->pdir, cq->umem, 0);
 
-	atomic_set(&cq->refcnt, 1);
+	refcount_set(&cq->refcnt, 1);
 	init_waitqueue_head(&cq->wait);
 	spin_lock_init(&cq->cq_lock);
 
@@ -230,8 +230,8 @@ struct ib_cq *pvrdma_create_cq(struct ib_device *ibdev,
 
 static void pvrdma_free_cq(struct pvrdma_dev *dev, struct pvrdma_cq *cq)
 {
-	atomic_dec(&cq->refcnt);
-	wait_event(cq->wait, !atomic_read(&cq->refcnt));
+	if (!refcount_dec_and_test(&cq->refcnt))
+		wait_event(cq->wait, !refcount_read(&cq->refcnt));
 
 	if (!cq->is_kernel)
 		ib_umem_release(cq->umem);
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
index 941e324..5cff9fa 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
@@ -333,7 +333,7 @@ static void pvrdma_qp_event(struct pvrdma_dev *dev, u32 qpn, int type)
 	spin_lock_irqsave(&dev->qp_tbl_lock, flags);
 	qp = dev->qp_tbl[qpn % dev->dsr->caps.max_qp];
 	if (qp)
-		atomic_inc(&qp->refcnt);
+		refcount_inc(&qp->refcnt);
 	spin_unlock_irqrestore(&dev->qp_tbl_lock, flags);
 
 	if (qp && qp->ibqp.event_handler) {
@@ -346,8 +346,7 @@ static void pvrdma_qp_event(struct pvrdma_dev *dev, u32 qpn, int type)
 		ibqp->event_handler(&e, ibqp->qp_context);
 	}
 	if (qp) {
-		atomic_dec(&qp->refcnt);
-		if (atomic_read(&qp->refcnt) == 0)
+		if (refcount_dec_and_test(&qp->refcnt))
 			wake_up(&qp->wait);
 	}
 }
@@ -360,7 +359,7 @@ static void pvrdma_cq_event(struct pvrdma_dev *dev, u32 cqn, int type)
 	spin_lock_irqsave(&dev->cq_tbl_lock, flags);
 	cq = dev->cq_tbl[cqn % dev->dsr->caps.max_cq];
 	if (cq)
-		atomic_inc(&cq->refcnt);
+		refcount_inc(&cq->refcnt);
 	spin_unlock_irqrestore(&dev->cq_tbl_lock, flags);
 
 	if (cq && cq->ibcq.event_handler) {
@@ -373,8 +372,7 @@ static void pvrdma_cq_event(struct pvrdma_dev *dev, u32 cqn, int type)
 		ibcq->event_handler(&e, ibcq->cq_context);
 	}
 	if (cq) {
-		atomic_dec(&cq->refcnt);
-		if (atomic_read(&cq->refcnt) == 0)
+		if (refcount_dec_and_test(&cq->refcnt))
 			wake_up(&cq->wait);
 	}
 }
@@ -533,14 +531,13 @@ static irqreturn_t pvrdma_intrx_handler(int irq, void *dev_id)
 		spin_lock_irqsave(&dev->cq_tbl_lock, flags);
 		cq = dev->cq_tbl[cqne->info % dev->dsr->caps.max_cq];
 		if (cq)
-			atomic_inc(&cq->refcnt);
+			refcount_inc(&cq->refcnt);
 		spin_unlock_irqrestore(&dev->cq_tbl_lock, flags);
 
 		if (cq && cq->ibcq.comp_handler)
 			cq->ibcq.comp_handler(&cq->ibcq, cq->ibcq.cq_context);
 		if (cq) {
-			atomic_dec(&cq->refcnt);
-			if (atomic_read(&cq->refcnt))
+			if (refcount_dec_and_test(&cq->refcnt))
 				wake_up(&cq->wait);
 		}
 		pvrdma_idx_ring_inc(&ring->cons_head, ring_slots);
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
index 77e7e57..9745cb1 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
@@ -245,7 +245,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 		spin_lock_init(&qp->sq.lock);
 		spin_lock_init(&qp->rq.lock);
 		mutex_init(&qp->mutex);
-		atomic_set(&qp->refcnt, 1);
+		refcount_set(&qp->refcnt, 1);
 		init_waitqueue_head(&qp->wait);
 
 		qp->state = IB_QPS_RESET;
@@ -427,8 +427,8 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
 
 	pvrdma_unlock_cqs(scq, rcq, &scq_flags, &rcq_flags);
 
-	atomic_dec(&qp->refcnt);
-	wait_event(qp->wait, !atomic_read(&qp->refcnt));
+	if (!refcount_dec_and_test(&qp->refcnt))
+		wait_event(qp->wait, !refcount_read(&qp->refcnt));
 
 	if (!qp->is_kernel) {
 		if (qp->rumem)
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH for-rc 6/6] RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file

[Bryan Tan]

Add the two UAR SRQ macros that are required by the userlevel
library.

Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
---
 include/uapi/rdma/vmw_pvrdma-abi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/rdma/vmw_pvrdma-abi.h b/include/uapi/rdma/vmw_pvrdma-abi.h
index aaa352f..4007cac 100644
--- a/include/uapi/rdma/vmw_pvrdma-abi.h
+++ b/include/uapi/rdma/vmw_pvrdma-abi.h
@@ -58,6 +58,8 @@
 #define PVRDMA_UAR_CQ_ARM_SOL		BIT(29)		/* Arm solicited bit. */
 #define PVRDMA_UAR_CQ_ARM		BIT(30)		/* Arm bit. */
 #define PVRDMA_UAR_CQ_POLL		BIT(31)		/* Poll bit. */
+#define PVRDMA_UAR_SRQ_OFFSET		8		/* SRQ doorbell. */
+#define PVRDMA_UAR_SRQ_RECV		BIT(30)		/* Recv bit. */
 
 enum pvrdma_wr_opcode {
 	PVRDMA_WR_RDMA_WRITE,
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 6/6] RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file

[Jason Gunthorpe]

On Fri, Dec 08, 2017 at 11:03:26AM -0800, Bryan Tan wrote:
> Add the two UAR SRQ macros that are required by the userlevel
> library.
> 
> Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
> Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
>  include/uapi/rdma/vmw_pvrdma-abi.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/uapi/rdma/vmw_pvrdma-abi.h b/include/uapi/rdma/vmw_pvrdma-abi.h
> index aaa352f..4007cac 100644
> +++ b/include/uapi/rdma/vmw_pvrdma-abi.h
> @@ -58,6 +58,8 @@
>  #define PVRDMA_UAR_CQ_ARM_SOL		BIT(29)		/* Arm solicited bit. */
>  #define PVRDMA_UAR_CQ_ARM		BIT(30)		/* Arm bit. */
>  #define PVRDMA_UAR_CQ_POLL		BIT(31)		/* Poll bit. */
> +#define PVRDMA_UAR_SRQ_OFFSET		8		/* SRQ doorbell. */
> +#define PVRDMA_UAR_SRQ_RECV		BIT(30)		/* Recv bit. */

I didn't think BIT() was allowed in uapi headers? Where does it get
defined in user space?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Jason Gunthorpe]

On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote:
> refcount_dec generates a warning when the operation
> causes the refcount to hit zero. Avoid this by using
> refcount_dec_and_test.
> 
> Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
> Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> index 826ccb8..a2b1a3c 100644
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq)
>  	dev->srq_tbl[srq->srq_handle] = NULL;
>  	spin_unlock_irqrestore(&dev->srq_tbl_lock, flags);
>  
> -	refcount_dec(&srq->refcnt);
> -	wait_event(srq->wait, !refcount_read(&srq->refcnt));
> +	if (!refcount_dec_and_test(&srq->refcnt))
> +		wait_event(srq->wait, !refcount_read(&srq->refcnt));

I really don't like this idiom for using refcount, refcount should
not be dec'd below 0 even under a dec_and_test..

Why not just simplify:

	wait_event(srq->wait, refcount_read(&srq->refcnt) == 1);

???

Same comment on the other patch switching from atomic to refcount

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Bryan Tan]

On Fri, Dec 08, 2017 at 04:30:49PM -0700, Jason Gunthorpe wrote:
> On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote:
> > refcount_dec generates a warning when the operation
> > causes the refcount to hit zero. Avoid this by using
> > refcount_dec_and_test.
> > 
> > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
> > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> > index 826ccb8..a2b1a3c 100644
> > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> > @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq)
> >  	dev->srq_tbl[srq->srq_handle] = NULL;
> >  	spin_unlock_irqrestore(&dev->srq_tbl_lock, flags);
> >  
> > -	refcount_dec(&srq->refcnt);
> > -	wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > +	if (!refcount_dec_and_test(&srq->refcnt))
> > +		wait_event(srq->wait, !refcount_read(&srq->refcnt));
> 
> I really don't like this idiom for using refcount, refcount should
> not be dec'd below 0 even under a dec_and_test..
> Why not just simplify:
> 
> 	wait_event(srq->wait, refcount_read(&srq->refcnt) == 1);
> 
> ???
> 
> Same comment on the other patch switching from atomic to refcount
> 
> Jason

The refcount doesn't go below 0 in either case--the problem is that
I didn't realise refcount_dec complains about decrements resulting
in a value of 0 [1]. There are no problems with refcount_dec_and_test
resulting in a value of 0.

About checking for refcnt == 1, I do not know of a safe way to only
wake up when the refcount hits 1. Right now we do that by checking
for 0 and doing a wake_up if the result of refcount_dec_and_test is
0 (see the SRQ event handler in pvrdma_main.c). If there's a way to
accomplish this without another dec_and_test in the destroy path, I
can do so.

Bryan

[1] https://patchwork.kernel.org/patch/9863573/ 3rd paragraph
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 6/6] RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file

[Bryan Tan]

On Fri, Dec 08, 2017 at 04:28:09PM -0700, Jason Gunthorpe wrote:
> On Fri, Dec 08, 2017 at 11:03:26AM -0800, Bryan Tan wrote:
> > Add the two UAR SRQ macros that are required by the userlevel
> > library.
> > 
> > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
> > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> >  include/uapi/rdma/vmw_pvrdma-abi.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/include/uapi/rdma/vmw_pvrdma-abi.h b/include/uapi/rdma/vmw_pvrdma-abi.h
> > index aaa352f..4007cac 100644
> > +++ b/include/uapi/rdma/vmw_pvrdma-abi.h
> > @@ -58,6 +58,8 @@
> >  #define PVRDMA_UAR_CQ_ARM_SOL		BIT(29)		/* Arm solicited bit. */
> >  #define PVRDMA_UAR_CQ_ARM		BIT(30)		/* Arm bit. */
> >  #define PVRDMA_UAR_CQ_POLL		BIT(31)		/* Poll bit. */
> > +#define PVRDMA_UAR_SRQ_OFFSET		8		/* SRQ doorbell. */
> > +#define PVRDMA_UAR_SRQ_RECV		BIT(30)		/* Recv bit. */
> 
> I didn't think BIT() was allowed in uapi headers? Where does it get
> defined in user space?
> 
> Jason

Looks like we had BIT() defined in userspace. Seems like the rest of
the headers just use a bit shift? If that's fine I can send out a V2
with the BIT()s removed.

Bryan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 6/6] RDMA/vmw_pvrdma: Add UAR SRQ macros in ABI header file

[Jason Gunthorpe]

On Mon, Dec 11, 2017 at 10:35:55AM -0800, Bryan Tan wrote:
> On Fri, Dec 08, 2017 at 04:28:09PM -0700, Jason Gunthorpe wrote:
> > On Fri, Dec 08, 2017 at 11:03:26AM -0800, Bryan Tan wrote:
> > > Add the two UAR SRQ macros that are required by the userlevel
> > > library.
> > > 
> > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
> > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > >  include/uapi/rdma/vmw_pvrdma-abi.h | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/include/uapi/rdma/vmw_pvrdma-abi.h b/include/uapi/rdma/vmw_pvrdma-abi.h
> > > index aaa352f..4007cac 100644
> > > +++ b/include/uapi/rdma/vmw_pvrdma-abi.h
> > > @@ -58,6 +58,8 @@
> > >  #define PVRDMA_UAR_CQ_ARM_SOL		BIT(29)		/* Arm solicited bit. */
> > >  #define PVRDMA_UAR_CQ_ARM		BIT(30)		/* Arm bit. */
> > >  #define PVRDMA_UAR_CQ_POLL		BIT(31)		/* Poll bit. */
> > > +#define PVRDMA_UAR_SRQ_OFFSET		8		/* SRQ doorbell. */
> > > +#define PVRDMA_UAR_SRQ_RECV		BIT(30)		/* Recv bit. */
> > 
> > I didn't think BIT() was allowed in uapi headers? Where does it get
> > defined in user space?
> > 
> > Jason
> 
> Looks like we had BIT() defined in userspace.

In rdma-core? Please remove that too :)

> Seems like the rest of the headers just use a bit shift? If that's
> fine I can send out a V2 with the BIT()s removed.

Yes please remove them all

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Jason Gunthorpe]

On Mon, Dec 11, 2017 at 10:33:15AM -0800, Bryan Tan wrote:
> On Fri, Dec 08, 2017 at 04:30:49PM -0700, Jason Gunthorpe wrote:
> > On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote:
> > > refcount_dec generates a warning when the operation
> > > causes the refcount to hit zero. Avoid this by using
> > > refcount_dec_and_test.
> > > 
> > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
> > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> > > index 826ccb8..a2b1a3c 100644
> > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> > > @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq)
> > >  	dev->srq_tbl[srq->srq_handle] = NULL;
> > >  	spin_unlock_irqrestore(&dev->srq_tbl_lock, flags);
> > >  
> > > -	refcount_dec(&srq->refcnt);
> > > -	wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > > +	if (!refcount_dec_and_test(&srq->refcnt))
> > > +		wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > 
> > I really don't like this idiom for using refcount, refcount should
> > not be dec'd below 0 even under a dec_and_test..
> > Why not just simplify:
> >
> > 	wait_event(srq->wait, refcount_read(&srq->refcnt) == 1);
> >
> > ???
> >
> > Same comment on the other patch switching from atomic to refcount
> > 
> > Jason
> 
> The refcount doesn't go below 0 in either case--the problem is that
> I didn't realise refcount_dec complains about decrements resulting
> in a value of 0 [1]. There are no problems with refcount_dec_and_test
> resulting in a value of 0.

Thats what I ment with my remark.. Sorry for the confusion

> About checking for refcnt == 1, I do not know of a safe way to only
> wake up when the refcount hits 1. Right now we do that by checking
> for 0 and doing a wake_up if the result of refcount_dec_and_test is
> 0 (see the SRQ event handler in pvrdma_main.c). If there's a way to
> accomplish this without another dec_and_test in the destroy path, I
> can do so.

What is wrong with this:

 	wait_event(srq->wait, refcount_read(&srq->refcnt) == 1);
?

refcount == 1 means the caller is the last owner of the refcount,
presumably you have already taken care to ensure it cannot be inc'd
again (or the code is already not locked right)

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Bryan Tan]

On Mon, Dec 11, 2017 at 01:14:21PM -0700, Jason Gunthorpe wrote:
> On Mon, Dec 11, 2017 at 10:33:15AM -0800, Bryan Tan wrote:
> > On Fri, Dec 08, 2017 at 04:30:49PM -0700, Jason Gunthorpe wrote:
> > > On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote:
> > > > refcount_dec generates a warning when the operation
> > > > causes the refcount to hit zero. Avoid this by using
> > > > refcount_dec_and_test.
> > > > 
> > > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
> > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> > > > index 826ccb8..a2b1a3c 100644
> > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> > > > @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq)
> > > >  	dev->srq_tbl[srq->srq_handle] = NULL;
> > > >  	spin_unlock_irqrestore(&dev->srq_tbl_lock, flags);
> > > >  
> > > > -	refcount_dec(&srq->refcnt);
> > > > -	wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > > > +	if (!refcount_dec_and_test(&srq->refcnt))
> > > > +		wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > > 
> > > I really don't like this idiom for using refcount, refcount should
> > > not be dec'd below 0 even under a dec_and_test..
> > > Why not just simplify:
> > >
> > > 	wait_event(srq->wait, refcount_read(&srq->refcnt) == 1);
> > >
> > > ???
> > >
> > > Same comment on the other patch switching from atomic to refcount
> > > 
> > > Jason
> > 
> > The refcount doesn't go below 0 in either case--the problem is that
> > I didn't realise refcount_dec complains about decrements resulting
> > in a value of 0 [1]. There are no problems with refcount_dec_and_test
> > resulting in a value of 0.
> 
> Thats what I ment with my remark.. Sorry for the confusion
> 
> > About checking for refcnt == 1, I do not know of a safe way to only
> > wake up when the refcount hits 1. Right now we do that by checking
> > for 0 and doing a wake_up if the result of refcount_dec_and_test is
> > 0 (see the SRQ event handler in pvrdma_main.c). If there's a way to
> > accomplish this without another dec_and_test in the destroy path, I
> > can do so.
> 
> What is wrong with this:
> 
>  	wait_event(srq->wait, refcount_read(&srq->refcnt) == 1);
> ?
> 
> refcount == 1 means the caller is the last owner of the refcount,
> presumably you have already taken care to ensure it cannot be inc'd
> again (or the code is already not locked right)

In the SRQ event handler, we do this at the end of handling the event:

> if (refcount_dec_and_test(&srq->refcnt))
>         wake_up(&srq->wait);

If we wait on refcnt == 1, there isn't an atomic way to decrement and
check if the result is 1. If it's fine to wake up the process while
the condition hasn't been met, this would work, but that doesn't seem
to be the right solution here, unless I'm missing something.

Bryan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Jason Gunthorpe]

On Mon, Dec 11, 2017 at 01:59:37PM -0800, Bryan Tan wrote:
> On Mon, Dec 11, 2017 at 01:14:21PM -0700, Jason Gunthorpe wrote:
> > On Mon, Dec 11, 2017 at 10:33:15AM -0800, Bryan Tan wrote:
> > > On Fri, Dec 08, 2017 at 04:30:49PM -0700, Jason Gunthorpe wrote:
> > > > On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote:
> > > > > refcount_dec generates a warning when the operation
> > > > > causes the refcount to hit zero. Avoid this by using
> > > > > refcount_dec_and_test.
> > > > > 
> > > > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
> > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> > > > > index 826ccb8..a2b1a3c 100644
> > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> > > > > @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq)
> > > > >  	dev->srq_tbl[srq->srq_handle] = NULL;
> > > > >  	spin_unlock_irqrestore(&dev->srq_tbl_lock, flags);
> > > > >  
> > > > > -	refcount_dec(&srq->refcnt);
> > > > > -	wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > > > > +	if (!refcount_dec_and_test(&srq->refcnt))
> > > > > +		wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > > > 
> > > > I really don't like this idiom for using refcount, refcount should
> > > > not be dec'd below 0 even under a dec_and_test..
> > > > Why not just simplify:
> > > >
> > > > 	wait_event(srq->wait, refcount_read(&srq->refcnt) == 1);
> > > >
> > > > ???
> > > >
> > > > Same comment on the other patch switching from atomic to refcount
> > > > 
> > > > Jason
> > > 
> > > The refcount doesn't go below 0 in either case--the problem is that
> > > I didn't realise refcount_dec complains about decrements resulting
> > > in a value of 0 [1]. There are no problems with refcount_dec_and_test
> > > resulting in a value of 0.
> > 
> > Thats what I ment with my remark.. Sorry for the confusion
> > 
> > > About checking for refcnt == 1, I do not know of a safe way to only
> > > wake up when the refcount hits 1. Right now we do that by checking
> > > for 0 and doing a wake_up if the result of refcount_dec_and_test is
> > > 0 (see the SRQ event handler in pvrdma_main.c). If there's a way to
> > > accomplish this without another dec_and_test in the destroy path, I
> > > can do so.
> > 
> > What is wrong with this:
> > 
> >  	wait_event(srq->wait, refcount_read(&srq->refcnt) == 1);
> > ?
> > 
> > refcount == 1 means the caller is the last owner of the refcount,
> > presumably you have already taken care to ensure it cannot be inc'd
> > again (or the code is already not locked right)
> 
> In the SRQ event handler, we do this at the end of handling the event:
> 
> > if (refcount_dec_and_test(&srq->refcnt))
> >         wake_up(&srq->wait);

What? You can't combine one thread doing

 if (refcount_dec_and_test(&srq->refcnt))
         wake_up(&srq->wait);

With
  if (!refcount_dec_and_test(&srq->refcnt))
	wait_event(srq->wait, !refcount_read(&srq->refcnt));

It makes no sense, how is that a refcount???

They can't *BOTH* refcount_dec_and_test!

I can understand doing

  if (refcount_dec_and_test(&srq->refcnt))
         wake_up(&srq->wait);

and then

   wait_event(srq->wait, !refcount_read(&srq->refcnt));

That makes perfect sense.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Bryan Tan]

On Mon, Dec 11, 2017 at 03:14:25PM -0700, Jason Gunthorpe wrote:
> On Mon, Dec 11, 2017 at 01:59:37PM -0800, Bryan Tan wrote:
> > On Mon, Dec 11, 2017 at 01:14:21PM -0700, Jason Gunthorpe wrote:
> > > On Mon, Dec 11, 2017 at 10:33:15AM -0800, Bryan Tan wrote:
> > > > On Fri, Dec 08, 2017 at 04:30:49PM -0700, Jason Gunthorpe wrote:
> > > > > On Fri, Dec 08, 2017 at 11:02:24AM -0800, Bryan Tan wrote:
> > > > > > refcount_dec generates a warning when the operation
> > > > > > causes the refcount to hit zero. Avoid this by using
> > > > > > refcount_dec_and_test.
> > > > > > 
> > > > > > Fixes: 8b10ba783c9d ("RDMA/vmw_pvrdma: Add shared receive queue support")
> > > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> > > > > > index 826ccb8..a2b1a3c 100644
> > > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c
> > > > > > @@ -236,8 +236,8 @@ static void pvrdma_free_srq(struct pvrdma_dev *dev, struct pvrdma_srq *srq)
> > > > > >  	dev->srq_tbl[srq->srq_handle] = NULL;
> > > > > >  	spin_unlock_irqrestore(&dev->srq_tbl_lock, flags);
> > > > > >  
> > > > > > -	refcount_dec(&srq->refcnt);
> > > > > > -	wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > > > > > +	if (!refcount_dec_and_test(&srq->refcnt))
> > > > > > +		wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > > > > 
> > > > > I really don't like this idiom for using refcount, refcount should
> > > > > not be dec'd below 0 even under a dec_and_test..
> > > > > Why not just simplify:
> > > > >
> > > > > 	wait_event(srq->wait, refcount_read(&srq->refcnt) == 1);
> > > > >
> > > > > ???
> > > > >
> > > > > Same comment on the other patch switching from atomic to refcount
> > > > > 
> > > > > Jason
> > > > 
> > > > The refcount doesn't go below 0 in either case--the problem is that
> > > > I didn't realise refcount_dec complains about decrements resulting
> > > > in a value of 0 [1]. There are no problems with refcount_dec_and_test
> > > > resulting in a value of 0.
> > > 
> > > Thats what I ment with my remark.. Sorry for the confusion
> > > 
> > > > About checking for refcnt == 1, I do not know of a safe way to only
> > > > wake up when the refcount hits 1. Right now we do that by checking
> > > > for 0 and doing a wake_up if the result of refcount_dec_and_test is
> > > > 0 (see the SRQ event handler in pvrdma_main.c). If there's a way to
> > > > accomplish this without another dec_and_test in the destroy path, I
> > > > can do so.
> > > 
> > > What is wrong with this:
> > > 
> > >  	wait_event(srq->wait, refcount_read(&srq->refcnt) == 1);
> > > ?
> > > 
> > > refcount == 1 means the caller is the last owner of the refcount,
> > > presumably you have already taken care to ensure it cannot be inc'd
> > > again (or the code is already not locked right)
> > 
> > In the SRQ event handler, we do this at the end of handling the event:
> > 
> > > if (refcount_dec_and_test(&srq->refcnt))
> > >         wake_up(&srq->wait);
> 
> What? You can't combine one thread doing
> 
>  if (refcount_dec_and_test(&srq->refcnt))
>          wake_up(&srq->wait);
> 
> With
>   if (!refcount_dec_and_test(&srq->refcnt))
> 	wait_event(srq->wait, !refcount_read(&srq->refcnt));
> 
> It makes no sense, how is that a refcount???
> 
> They can't *BOTH* refcount_dec_and_test!
>
> I can understand doing
> 
>   if (refcount_dec_and_test(&srq->refcnt))
>          wake_up(&srq->wait);
> 
> and then
> 
>    wait_event(srq->wait, !refcount_read(&srq->refcnt));
> 
> That makes perfect sense.

Perhaps I am mistaken in my understanding of refcounts here, but
what is wrong with this? If it's not that the refcount is set to
1 on resource creation (which, by your earlier suggestion of
checking refcnt == 1, seems to be fine), and both callers (the
resource event handler and resource destroy call) need to make
sure the destroy sequence only happens once (when the refcount
reaches 0), then using refcount_dec_and_test seems right to me.

Your last suggestion would mean setting the refcount to 0 on
resource creation, as we wait on refcount hitting 0, but that
would mean the resource event handler would often be calling
wake_up unnecessarily.

What we're doing is effectively the same as how mlx5 handles
this (see drivers/net/ethernet/mellanox/mlx5/core/srq.c) I
suppose using refcount_t requires one to follow a particular
model of usage but I'm not sure how this isn't right.

Thanks,
Bryan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Jason Gunthorpe]

> > What? You can't combine one thread doing
> > 
> >  if (refcount_dec_and_test(&srq->refcnt))
> >          wake_up(&srq->wait);
> > 
> > With
> >   if (!refcount_dec_and_test(&srq->refcnt))
> > 	wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > 
> > It makes no sense, how is that a refcount???
> > 
> > They can't *BOTH* refcount_dec_and_test!
> >
> > I can understand doing
> >
> >   if (refcount_dec_and_test(&srq->refcnt))
> >          wake_up(&srq->wait);
> >
> > and then
> > 
> >    wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > 
> > That makes perfect sense.
> 
> Perhaps I am mistaken in my understanding of refcounts here, but
> what is wrong with this? If it's not that the refcount is set to
> 1 on resource creation (which, by your earlier suggestion of
> checking refcnt == 1, seems to be fine), and both callers (the
> resource event handler and resource destroy call) need to make
> sure the destroy sequence only happens once (when the refcount
> reaches 0), then using refcount_dec_and_test seems right to me.

I think the issue here is trying to fit an optimized approach that was
using atomics into a refcount.

I have certain expectations when I see something called 'refcount'
that this scheme really doesn't follow anymore, and those expectations
are pretty much matched by the protections inside refcount against
going to 0 and so on.

The algorithm works OK, but I'm not sure it is a 'refcount', more of a
'usecnt'..

On the other hand, having the refcount overflow protections in this
path does seem useful.

I did a random audit of refcount_dec_and_test existing users and
didn't find any that use this pattern.

What you've actually built here is a rwsem out of an atomic and a wait
queue..

So, what is wrong with a rwsem?

> What we're doing is effectively the same as how mlx5 handles
> this (see drivers/net/ethernet/mellanox/mlx5/core/srq.c) I
> suppose using refcount_t requires one to follow a particular
> model of usage but I'm not sure how this isn't right.

I didn't say it was wrong, I asked 'how is that a refcount??'

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Bryan Tan]

On Tue, Dec 12, 2017 at 11:55:30AM -0700, Jason Gunthorpe wrote:
> > > What? You can't combine one thread doing
> > > 
> > >  if (refcount_dec_and_test(&srq->refcnt))
> > >          wake_up(&srq->wait);
> > > 
> > > With
> > >   if (!refcount_dec_and_test(&srq->refcnt))
> > > 	wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > > 
> > > It makes no sense, how is that a refcount???
> > > 
> > > They can't *BOTH* refcount_dec_and_test!
> > >
> > > I can understand doing
> > >
> > >   if (refcount_dec_and_test(&srq->refcnt))
> > >          wake_up(&srq->wait);
> > >
> > > and then
> > > 
> > >    wait_event(srq->wait, !refcount_read(&srq->refcnt));
> > > 
> > > That makes perfect sense.
> > 
> > Perhaps I am mistaken in my understanding of refcounts here, but
> > what is wrong with this? If it's not that the refcount is set to
> > 1 on resource creation (which, by your earlier suggestion of
> > checking refcnt == 1, seems to be fine), and both callers (the
> > resource event handler and resource destroy call) need to make
> > sure the destroy sequence only happens once (when the refcount
> > reaches 0), then using refcount_dec_and_test seems right to me.
> 
> I think the issue here is trying to fit an optimized approach that was
> using atomics into a refcount.
> 
> I have certain expectations when I see something called 'refcount'
> that this scheme really doesn't follow anymore, and those expectations
> are pretty much matched by the protections inside refcount against
> going to 0 and so on.
> 
> The algorithm works OK, but I'm not sure it is a 'refcount', more of a
> 'usecnt'..

Hm, it's still not clear to me what expectations are not followed here,
and how you're differentiating 'refcount' from 'usecnt'. And not that
it makes it right, but I did notice that mlx4 uses the same pattern as
what I've done here [1].

If it makes the most sense to revert to atomic operations or use a
rwsem, I can do that. Just trying to understand what the expectations
are for using refcount_t.

Bryan

[1] Commit ff61b5e3f041c2f1aa8d7c700af3007889973889
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Jason Gunthorpe]

On Tue, Dec 12, 2017 at 04:08:16PM -0800, Bryan Tan wrote:
> On Tue, Dec 12, 2017 at 11:55:30AM -0700, Jason Gunthorpe wrote:

> Hm, it's still not clear to me what expectations are not followed here,
> and how you're differentiating 'refcount' from 'usecnt'.

A refcount is something that has exactly one 'refcount_dec_and_test'
that then goes on to 'free' the thing being refcountedt. It never has
a wait queue.

A usecnt is something that has an async path that waits for all users
to stop using it then 'frees' it. It always includes a wait
queue.

> And not that it makes it right, but I did notice that mlx4 uses the
> same pattern as what I've done here [1].

Ah, this is good, I was trying to find something like it. Kees
approves so I have no problem taking your patch.

Though you might want to reflect on if using a completion is better
than a wait_event.. Not immediately clear to me.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path

[Yuval Shaia]

On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote:
> The QP cleanup did not previously call ib_umem_release. Fix this.
> 
> Testing Done: ibv_rc_pingpong, rping, perftests.
> 
> Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> index b932b7e..77e7e57 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
>  	atomic_dec(&qp->refcnt);
>  	wait_event(qp->wait, !atomic_read(&qp->refcnt));
>  
> +	if (!qp->is_kernel) {
> +		if (qp->rumem)

IS_ERR_OR_NULL?

> +			ib_umem_release(qp->rumem);
> +		if (qp->sumem)
> +			ib_umem_release(qp->sumem);
> +	}
> +
>  	pvrdma_page_dir_cleanup(dev, &qp->pdir);
>  
>  	kfree(qp);
> -- 
> 1.8.5.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic

[Yuval Shaia]

On Fri, Dec 08, 2017 at 11:00:17AM -0800, Bryan Tan wrote:
> Be more consistent in checking is_kernel flag for QPs.

Consist with what?
(asking because expecting also pvrdma_create_cq to be fixed).

> 
> Testing Done: ibv_rc_pingpong, rping, perftests.

These are all userspace tests, right?

> 
> Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> index 10420a1..b932b7e 100644
> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> @@ -249,8 +249,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
>  		init_waitqueue_head(&qp->wait);
>  
>  		qp->state = IB_QPS_RESET;
> +		qp->is_kernel = !(pd->uobject && udata);
>  
> -		if (pd->uobject && udata) {
> +		if (!qp->is_kernel) {
>  			dev_dbg(&dev->pdev->dev,
>  				"create queuepair from user space\n");
>  
> @@ -291,8 +292,6 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
>  				qp->npages_recv = 0;
>  			qp->npages = qp->npages_send + qp->npages_recv;
>  		} else {
> -			qp->is_kernel = true;
> -
>  			ret = pvrdma_set_sq_size(to_vdev(pd->device),
>  						 &init_attr->cap, qp);
>  			if (ret)
> @@ -394,7 +393,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
>  err_pdir:
>  	pvrdma_page_dir_cleanup(dev, &qp->pdir);
>  err_umem:
> -	if (pd->uobject && udata) {
> +	if (!qp->is_kernel) {
>  		if (qp->rumem)
>  			ib_umem_release(qp->rumem);
>  		if (qp->sumem)

Regardless of the comments:

Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

> -- 
> 1.8.5.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Bryan Tan]

On Tue, Dec 12, 2017 at 07:20:31PM -0700, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2017 at 04:08:16PM -0800, Bryan Tan wrote:
> > On Tue, Dec 12, 2017 at 11:55:30AM -0700, Jason Gunthorpe wrote:
> 
> > Hm, it's still not clear to me what expectations are not followed here,
> > and how you're differentiating 'refcount' from 'usecnt'.
> 
> A refcount is something that has exactly one 'refcount_dec_and_test'
> that then goes on to 'free' the thing being refcountedt. It never has
> a wait queue.
> 
> A usecnt is something that has an async path that waits for all users
> to stop using it then 'frees' it. It always includes a wait
> queue.

I see, thanks for the explanation.

> > And not that it makes it right, but I did notice that mlx4 uses the
> > same pattern as what I've done here [1].
> 
> Ah, this is good, I was trying to find something like it. Kees
> approves so I have no problem taking your patch.
> 
> Though you might want to reflect on if using a completion is better
> than a wait_event.. Not immediately clear to me.

Oof, thanks for pointing that out. There's a problem here it seems:
0. refcount for a resource is 2 after entering an event handler
1. refcount_dec_and_test gets called by the resource destroy
2. refcount_dec_and_test gets called by the resource event handler
3. resource destroy call checks the condition in wait_event and
   proceeds to free the resource
4. resource event handler calls wake_up() on free'd resource

I'll use a completion here instead.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 4/6] RDMA/vmw_pvrdma: Use refcount_dec_and_test to avoid warning

[Jason Gunthorpe]

On Wed, Dec 13, 2017 at 01:07:02AM -0800, Bryan Tan wrote:

> > > And not that it makes it right, but I did notice that mlx4 uses the
> > > same pattern as what I've done here [1].
> > 
> > Ah, this is good, I was trying to find something like it. Kees
> > approves so I have no problem taking your patch.
> > 
> > Though you might want to reflect on if using a completion is better
> > than a wait_event.. Not immediately clear to me.
> 
> Oof, thanks for pointing that out. There's a problem here it seems:
> 0. refcount for a resource is 2 after entering an event handler
> 1. refcount_dec_and_test gets called by the resource destroy
> 2. refcount_dec_and_test gets called by the resource event handler
> 3. resource destroy call checks the condition in wait_event and
>    proceeds to free the resource
> 4. resource event handler calls wake_up() on free'd resource

Yes I agree. I knew it looked funny :)

Please resend

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path

[Jason Gunthorpe]

On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote:
> On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote:
> > The QP cleanup did not previously call ib_umem_release. Fix this.
> > 
> > Testing Done: ibv_rc_pingpong, rping, perftests.
> > 
> > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > index b932b7e..77e7e57 100644
> > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
> >  	atomic_dec(&qp->refcnt);
> >  	wait_event(qp->wait, !atomic_read(&qp->refcnt));
> >  
> > +	if (!qp->is_kernel) {
> > +		if (qp->rumem)
> 
> IS_ERR_OR_NULL?

How can we get an ERR_PTR in rumem?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path

[Yuval Shaia]

On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote:
> On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote:
> > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote:
> > > The QP cleanup did not previously call ib_umem_release. Fix this.
> > > 
> > > Testing Done: ibv_rc_pingpong, rping, perftests.
> > > 
> > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > index b932b7e..77e7e57 100644
> > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
> > >  	atomic_dec(&qp->refcnt);
> > >  	wait_event(qp->wait, !atomic_read(&qp->refcnt));
> > >  
> > > +	if (!qp->is_kernel) {
> > > +		if (qp->rumem)
> > 
> > IS_ERR_OR_NULL?
> 
> How can we get an ERR_PTR in rumem?

qp->rumem is set with ib_umem_get which can either return ib_umem pointer
or ERR_PTR.
So i looked at other places which calls ib_umem_release. The first one that
pops up is bnxt_re which do this check with IS_ERR_OR_NULL?.

So assuming it can be used here.

> 
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path

[Bryan Tan]

On Wed, Dec 13, 2017 at 08:40:13PM +0200, Yuval Shaia wrote:
> On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote:
> > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote:
> > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote:
> > > > The QP cleanup did not previously call ib_umem_release. Fix this.
> > > > 
> > > > Testing Done: ibv_rc_pingpong, rping, perftests.
> > > > 
> > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > > index b932b7e..77e7e57 100644
> > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
> > > >  	atomic_dec(&qp->refcnt);
> > > >  	wait_event(qp->wait, !atomic_read(&qp->refcnt));
> > > >  
> > > > +	if (!qp->is_kernel) {
> > > > +		if (qp->rumem)
> > > 
> > > IS_ERR_OR_NULL?
> > 
> > How can we get an ERR_PTR in rumem?
> 
> qp->rumem is set with ib_umem_get which can either return ib_umem pointer
> or ERR_PTR.
> So i looked at other places which calls ib_umem_release. The first one that
> pops up is bnxt_re which do this check with IS_ERR_OR_NULL?.
> 
> So assuming it can be used here.

We check in pvrdma_qp.c pvrdma_create_qp the result of ib_umem_get.
If it's ERR_PTR we fail the QP creation:

> qp->rumem = ib_umem_get(pd->uobject->context,
> 			ucmd.rbuf_addr,
> 			ucmd.rbuf_size, 0, 0);
> if (IS_ERR(qp->rumem)) {
> 	ret = PTR_ERR(qp->rumem);
> 	goto err_qp;
> }

so checking IS_ERR_OR_NULL isn't necessary here (and same for sumem).

Thanks,
Bryan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path

[Jason Gunthorpe]

On Wed, Dec 13, 2017 at 08:40:13PM +0200, Yuval Shaia wrote:
> On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote:
> > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote:
> > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote:
> > > > The QP cleanup did not previously call ib_umem_release. Fix this.
> > > > 
> > > > Testing Done: ibv_rc_pingpong, rping, perftests.
> > > > 
> > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > > index b932b7e..77e7e57 100644
> > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
> > > >  	atomic_dec(&qp->refcnt);
> > > >  	wait_event(qp->wait, !atomic_read(&qp->refcnt));
> > > >  
> > > > +	if (!qp->is_kernel) {
> > > > +		if (qp->rumem)
> > > 
> > > IS_ERR_OR_NULL?
> > 
> > How can we get an ERR_PTR in rumem?
> 
> qp->rumem is set with ib_umem_get which can either return ib_umem pointer
> or ERR_PTR.

In generall ERR_PTR's should not be permanently stored in structs and
I do not expect to see IS_ERR_OR_NULL used. Several kernel devs
consider is an abomination, and it certainly should *NEVER* be used
on a ptr which cannot, by design, have an ERR_PTR in it.

At least vmw_pvrdma appears to be OK:

                                qp->rumem = ib_umem_get(pd->uobject->context,
                                                        ucmd.rbuf_addr,
                                                        ucmd.rbuf_size, 0, 0);

                                if (IS_ERR(qp->rumem)) {
                                        ret = PTR_ERR(qp->rumem);
                                        goto err_qp;
[..]
err_qp:
        kfree(qp);


So we cannot have a 'qp' struct with an ERR_PTR in rumem.

> So i looked at other places which calls ib_umem_release. The first one that
> pops up is bnxt_re which do this check with IS_ERR_OR_NULL?.

The test in bnxt looks wrong to me, and should be removed:

                umem = ib_umem_get(context, ureq.qprva, bytes,
                                   IB_ACCESS_LOCAL_WRITE, 1);
                if (IS_ERR(umem))
                        goto rqfail;
                qp->rumem = umem;

So rumem can never be assigned to PTR_ERR, and this is garbage:

        if (!IS_ERR_OR_NULL(qp->rumem))
                ib_umem_release(qp->rumem);


Similar remark for other tests in that driver. Care to send a patch?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path

[Yuval Shaia]

On Wed, Dec 13, 2017 at 10:55:18AM -0800, Bryan Tan wrote:
> On Wed, Dec 13, 2017 at 08:40:13PM +0200, Yuval Shaia wrote:
> > On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote:
> > > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote:
> > > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote:
> > > > > The QP cleanup did not previously call ib_umem_release. Fix this.
> > > > > 
> > > > > Testing Done: ibv_rc_pingpong, rping, perftests.
> > > > > 
> > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > > > index b932b7e..77e7e57 100644
> > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
> > > > >  	atomic_dec(&qp->refcnt);
> > > > >  	wait_event(qp->wait, !atomic_read(&qp->refcnt));
> > > > >  
> > > > > +	if (!qp->is_kernel) {
> > > > > +		if (qp->rumem)
> > > > 
> > > > IS_ERR_OR_NULL?
> > > 
> > > How can we get an ERR_PTR in rumem?
> > 
> > qp->rumem is set with ib_umem_get which can either return ib_umem pointer
> > or ERR_PTR.
> > So i looked at other places which calls ib_umem_release. The first one that
> > pops up is bnxt_re which do this check with IS_ERR_OR_NULL?.
> > 
> > So assuming it can be used here.
> 
> We check in pvrdma_qp.c pvrdma_create_qp the result of ib_umem_get.
> If it's ERR_PTR we fail the QP creation:
> 
> > qp->rumem = ib_umem_get(pd->uobject->context,
> > 			ucmd.rbuf_addr,
> > 			ucmd.rbuf_size, 0, 0);
> > if (IS_ERR(qp->rumem)) {
> > 	ret = PTR_ERR(qp->rumem);
> > 	goto err_qp;
> > }
> 
> so checking IS_ERR_OR_NULL isn't necessary here (and same for sumem).

I see.
Missed that.

> 
> Thanks,
> Bryan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic

[Bryan Tan]

On Wed, Dec 13, 2017 at 11:02:46AM +0200, Yuval Shaia wrote:
> On Fri, Dec 08, 2017 at 11:00:17AM -0800, Bryan Tan wrote:
> > Be more consistent in checking is_kernel flag for QPs.
> 
> Consist with what?
> (asking because expecting also pvrdma_create_cq to be fixed).

I had updated create SRQ's is_kernel in the same way. Thanks for
pointing out that I had missed it in create CQ.

> > 
> > Testing Done: ibv_rc_pingpong, rping, perftests.
> 
> These are all userspace tests, right?

Yes. That was accidentally carried over from our how we
format our commits internally. I'll remove the "Testing Done"
line here and in the next commit message.

> > 
> > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > index 10420a1..b932b7e 100644
> > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > @@ -249,8 +249,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
> >  		init_waitqueue_head(&qp->wait);
> >  
> >  		qp->state = IB_QPS_RESET;
> > +		qp->is_kernel = !(pd->uobject && udata);
> >  
> > -		if (pd->uobject && udata) {
> > +		if (!qp->is_kernel) {
> >  			dev_dbg(&dev->pdev->dev,
> >  				"create queuepair from user space\n");
> >  
> > @@ -291,8 +292,6 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
> >  				qp->npages_recv = 0;
> >  			qp->npages = qp->npages_send + qp->npages_recv;
> >  		} else {
> > -			qp->is_kernel = true;
> > -
> >  			ret = pvrdma_set_sq_size(to_vdev(pd->device),
> >  						 &init_attr->cap, qp);
> >  			if (ret)
> > @@ -394,7 +393,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
> >  err_pdir:
> >  	pvrdma_page_dir_cleanup(dev, &qp->pdir);
> >  err_umem:
> > -	if (pd->uobject && udata) {
> > +	if (!qp->is_kernel) {
> >  		if (qp->rumem)
> >  			ib_umem_release(qp->rumem);
> >  		if (qp->sumem)
> 
> Regardless of the comments:
> 
> Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> > -- 
> > 1.8.5.6
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=MjCVwEZlX7H-yXFOZ8b3IdkUDz9tjnFu-2RVvpFiKHw&m=M9b99UdgNMHlndgR9oY0KSFjeYht9EpUHM4iqxKTcUE&s=d8rmC5ZUNj2gLH8KOcrCgMmMg6484BYoXivYSjKEha0&e=
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 2/6] RDMA/vmw_pvrdma: Call ib_umem_release on destroy QP path

[Yuval Shaia]

On Wed, Dec 13, 2017 at 09:04:22PM +0200, Yuval Shaia wrote:
> On Wed, Dec 13, 2017 at 10:55:18AM -0800, Bryan Tan wrote:
> > On Wed, Dec 13, 2017 at 08:40:13PM +0200, Yuval Shaia wrote:
> > > On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote:
> > > > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote:
> > > > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote:
> > > > > > The QP cleanup did not previously call ib_umem_release. Fix this.
> > > > > > 
> > > > > > Testing Done: ibv_rc_pingpong, rping, perftests.
> > > > > > 
> > > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver")
> > > > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > > >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++
> > > > > >  1 file changed, 7 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > > > > index b932b7e..77e7e57 100644
> > > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp)
> > > > > >  	atomic_dec(&qp->refcnt);
> > > > > >  	wait_event(qp->wait, !atomic_read(&qp->refcnt));
> > > > > >  
> > > > > > +	if (!qp->is_kernel) {
> > > > > > +		if (qp->rumem)
> > > > > 
> > > > > IS_ERR_OR_NULL?
> > > > 
> > > > How can we get an ERR_PTR in rumem?
> > > 
> > > qp->rumem is set with ib_umem_get which can either return ib_umem pointer
> > > or ERR_PTR.
> > > So i looked at other places which calls ib_umem_release. The first one that
> > > pops up is bnxt_re which do this check with IS_ERR_OR_NULL?.
> > > 
> > > So assuming it can be used here.
> > 
> > We check in pvrdma_qp.c pvrdma_create_qp the result of ib_umem_get.
> > If it's ERR_PTR we fail the QP creation:
> > 
> > > qp->rumem = ib_umem_get(pd->uobject->context,
> > > 			ucmd.rbuf_addr,
> > > 			ucmd.rbuf_size, 0, 0);
> > > if (IS_ERR(qp->rumem)) {
> > > 	ret = PTR_ERR(qp->rumem);
> > > 	goto err_qp;
> > > }
> > 
> > so checking IS_ERR_OR_NULL isn't necessary here (and same for sumem).
> 
> I see.
> Missed that.

That being said i have no other questions.

Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

> 
> > 
> > Thanks,
> > Bryan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic

[Yuval Shaia]

On Wed, Dec 13, 2017 at 11:08:26AM -0800, Bryan Tan wrote:
> On Wed, Dec 13, 2017 at 11:02:46AM +0200, Yuval Shaia wrote:
> > On Fri, Dec 08, 2017 at 11:00:17AM -0800, Bryan Tan wrote:
> > > Be more consistent in checking is_kernel flag for QPs.
> > 
> > Consist with what?
> > (asking because expecting also pvrdma_create_cq to be fixed).
> 
> I had updated create SRQ's is_kernel in the same way. Thanks for
> pointing out that I had missed it in create CQ.
> 
> > > 
> > > Testing Done: ibv_rc_pingpong, rping, perftests.
> > 
> > These are all userspace tests, right?
> 
> Yes. That was accidentally carried over from our how we
> format our commits internally. I'll remove the "Testing Done"
> line here and in the next commit message.

But...since you exposed this info my question remains, you changed a flow
of a branch that checks if verb was requested by kernel or userspace but
you tested it only with userspace tools.

> 
> > > 
> > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > index 10420a1..b932b7e 100644
> > > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > @@ -249,8 +249,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
> > >  		init_waitqueue_head(&qp->wait);
> > >  
> > >  		qp->state = IB_QPS_RESET;
> > > +		qp->is_kernel = !(pd->uobject && udata);
> > >  
> > > -		if (pd->uobject && udata) {
> > > +		if (!qp->is_kernel) {
> > >  			dev_dbg(&dev->pdev->dev,
> > >  				"create queuepair from user space\n");
> > >  
> > > @@ -291,8 +292,6 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
> > >  				qp->npages_recv = 0;
> > >  			qp->npages = qp->npages_send + qp->npages_recv;
> > >  		} else {
> > > -			qp->is_kernel = true;
> > > -
> > >  			ret = pvrdma_set_sq_size(to_vdev(pd->device),
> > >  						 &init_attr->cap, qp);
> > >  			if (ret)
> > > @@ -394,7 +393,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
> > >  err_pdir:
> > >  	pvrdma_page_dir_cleanup(dev, &qp->pdir);
> > >  err_umem:
> > > -	if (pd->uobject && udata) {
> > > +	if (!qp->is_kernel) {
> > >  		if (qp->rumem)
> > >  			ib_umem_release(qp->rumem);
> > >  		if (qp->sumem)
> > 
> > Regardless of the comments:
> > 
> > Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > 
> > > -- 
> > > 1.8.5.6
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=MjCVwEZlX7H-yXFOZ8b3IdkUDz9tjnFu-2RVvpFiKHw&m=M9b99UdgNMHlndgR9oY0KSFjeYht9EpUHM4iqxKTcUE&s=d8rmC5ZUNj2gLH8KOcrCgMmMg6484BYoXivYSjKEha0&e=
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc 1/6] RDMA/vmw_pvrdma: Clarify QP is_kernel logic

[Bryan Tan]

On Wed, Dec 13, 2017 at 09:20:09PM +0200, Yuval Shaia wrote:
> On Wed, Dec 13, 2017 at 11:08:26AM -0800, Bryan Tan wrote:
> > On Wed, Dec 13, 2017 at 11:02:46AM +0200, Yuval Shaia wrote:
> > > On Fri, Dec 08, 2017 at 11:00:17AM -0800, Bryan Tan wrote:
> > > > Be more consistent in checking is_kernel flag for QPs.
> > > 
> > > Consist with what?
> > > (asking because expecting also pvrdma_create_cq to be fixed).
> > 
> > I had updated create SRQ's is_kernel in the same way. Thanks for
> > pointing out that I had missed it in create CQ.
> > 
> > > > 
> > > > Testing Done: ibv_rc_pingpong, rping, perftests.
> > > 
> > > These are all userspace tests, right?
> > 
> > Yes. That was accidentally carried over from our how we
> > format our commits internally. I'll remove the "Testing Done"
> > line here and in the next commit message.
> 
> But...since you exposed this info my question remains, you changed a flow
> of a branch that checks if verb was requested by kernel or userspace but
> you tested it only with userspace tools.

Given that it was an if-else and the nature of the change
I felt it sufficient to test with userspace applications.
A kernel QP is also created for CM, which is used in rping.
I just tested the patch series with krping though, and no
issues there.

> > 
> > > > 
> > > > Reviewed-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > Signed-off-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > >  drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++----
> > > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > > index 10420a1..b932b7e 100644
> > > > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c
> > > > @@ -249,8 +249,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
> > > >  		init_waitqueue_head(&qp->wait);
> > > >  
> > > >  		qp->state = IB_QPS_RESET;
> > > > +		qp->is_kernel = !(pd->uobject && udata);
> > > >  
> > > > -		if (pd->uobject && udata) {
> > > > +		if (!qp->is_kernel) {
> > > >  			dev_dbg(&dev->pdev->dev,
> > > >  				"create queuepair from user space\n");
> > > >  
> > > > @@ -291,8 +292,6 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
> > > >  				qp->npages_recv = 0;
> > > >  			qp->npages = qp->npages_send + qp->npages_recv;
> > > >  		} else {
> > > > -			qp->is_kernel = true;
> > > > -
> > > >  			ret = pvrdma_set_sq_size(to_vdev(pd->device),
> > > >  						 &init_attr->cap, qp);
> > > >  			if (ret)
> > > > @@ -394,7 +393,7 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
> > > >  err_pdir:
> > > >  	pvrdma_page_dir_cleanup(dev, &qp->pdir);
> > > >  err_umem:
> > > > -	if (pd->uobject && udata) {
> > > > +	if (!qp->is_kernel) {
> > > >  		if (qp->rumem)
> > > >  			ib_umem_release(qp->rumem);
> > > >  		if (qp->sumem)
> > > 
> > > Regardless of the comments:
> > > 
> > > Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > > 
> > > > -- 
> > > > 1.8.5.6
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=MjCVwEZlX7H-yXFOZ8b3IdkUDz9tjnFu-2RVvpFiKHw&m=M9b99UdgNMHlndgR9oY0KSFjeYht9EpUHM4iqxKTcUE&s=d8rmC5ZUNj2gLH8KOcrCgMmMg6484BYoXivYSjKEha0&e=
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html