Skip to contentSkip to conversation

RDMA/siw: publish QP after initialization

PATCH Applied Ruoyu Wang 3 participants 3 messages Jun 30 – Jul 2

Subsystems (2)
lore
Share & tools
lore

Ruoyu Wang

author
#0 lore raw

PATCH v4 1/1 RDMA/siw: publish QP after initialization

siw_create_qp() currently calls siw_qp_add() before the queues, CQ pointers, state, completion, and device list entry are ready. A QPN lookup can therefore reach a QP that is still being constructed.

Move siw_qp_add() to the end of siw_create_qp(), after QP initialization and before adding the QP to the siw device list.

Fixes f29dd55b0236 ("rdma/siw: queue pair methods")

---
Changes in v4:
- Move if (udata->outlen < sizeof(uresp)) into the second if (udata) clause, right before ib_copy_to_udata() (Bernard Metzler).
- Move INIT_LIST_HEAD(&qp->devq) to just before spin_lock_irqsave(), close to list_add_tail() where it logically belongs (Bernard Metzler).

Changes in v3:
- Move siw_qp_add()/xa_alloc() to the end of siw_create_qp().
- Drop the QPN reservation helper from v2.

 drivers/infiniband/sw/siw/siw_verbs.c | 57 ++++++++++++++++----------
 1 file changed, 35 insertions(+), 22 deletions(-)
drivers/infiniband/sw/siw/siw_verbs.c +24 -20
@@ -316,6 +316,7 @@
 	struct siw_ucontext *uctx =
 		rdma_udata_to_drv_context(udata, struct siw_ucontext,
 					  base_ucontext);
+	struct siw_uresp_create_qp uresp = {};
 	unsigned long flags;
 	int num_sqe, num_rqe, rv = 0;
 	size_t length;
@@ -369,11 +370,6 @@
 	spin_lock_init(&qp->rq_lock);
 	spin_lock_init(&qp->orq_lock);
 
-	rv = siw_qp_add(sdev, qp);
-	if (rv)
-		goto err_atomic;
-
-
 	/* All queue indices are derived from modulo operations
 	 * on a free running 'get' (consumer) and 'put' (producer)
 	 * unsigned counter. Having queue sizes at power of two
@@ -391,14 +387,14 @@
 
 	if (qp->sendq == NULL) {
 		rv = -ENOMEM;
-		goto err_out_xa;
+		goto err_out;
 	}
 	if (attrs->sq_sig_type != IB_SIGNAL_REQ_WR) {
 		if (attrs->sq_sig_type == IB_SIGNAL_ALL_WR)
 			qp->attrs.flags |= SIW_SIGNAL_ALL_WR;
 		else {
 			rv = -EINVAL;
-			goto err_out_xa;
+			goto err_out;
 		}
 	}
 	qp->pd = pd;
@@ -424,7 +420,7 @@
 
 		if (qp->recvq == NULL) {
 			rv = -ENOMEM;
-			goto err_out_xa;
+			goto err_out;
 		}
 		qp->attrs.rq_size = num_rqe;
 	}
@@ -439,11 +435,8 @@
 	qp->attrs.state = SIW_QP_STATE_IDLE;
 
 	if (udata) {
-		struct siw_uresp_create_qp uresp = {};
-
 		uresp.num_sqe = num_sqe;
 		uresp.num_rqe = num_rqe;
-		uresp.qp_id = qp_id(qp);
 
 		if (qp->sendq) {
 			length = num_sqe * sizeof(struct siw_sqe);
@@ -452,7 +445,7 @@
 						      length, &uresp.sq_key);
 			if (!qp->sq_entry) {
 				rv = -ENOMEM;
-				goto err_out_xa;
+				goto err_out;
 			}
 		}
 
@@ -464,9 +457,23 @@
 			if (!qp->rq_entry) {
 				uresp.sq_key = SIW_INVAL_UOBJ_KEY;
 				rv = -ENOMEM;
-				goto err_out_xa;
+				goto err_out;
 			}
 		}
+	}
+	qp->tx_cpu = siw_get_tx_cpu(sdev);
+	if (qp->tx_cpu < 0) {
+		rv = -EINVAL;
+		goto err_out;
+	}
+	init_completion(&qp->qp_free);
+
+	rv = siw_qp_add(sdev, qp);
+	if (rv)
+		goto err_out_tx;
+
+	if (udata) {
+		uresp.qp_id = qp_id(qp);
 
 		if (udata->outlen < sizeof(uresp)) {
 			rv = -EINVAL;
@@ -476,22 +483,19 @@
 		if (rv)
 			goto err_out_xa;
 	}
-	qp->tx_cpu = siw_get_tx_cpu(sdev);
-	if (qp->tx_cpu < 0) {
-		rv = -EINVAL;
-		goto err_out_xa;
-	}
+
 	INIT_LIST_HEAD(&qp->devq);
 	spin_lock_irqsave(&sdev->lock, flags);
 	list_add_tail(&qp->devq, &sdev->qp_list);
 	spin_unlock_irqrestore(&sdev->lock, flags);
 
-	init_completion(&qp->qp_free);
-
 	return 0;
 
 err_out_xa:
 	xa_erase(&sdev->qp_xa, qp_id(qp));
+err_out_tx:
+	siw_put_tx_cpu(qp->tx_cpu);
+err_out:
 	if (uctx) {
 		rdma_user_mmap_entry_remove(qp->sq_entry);
 		rdma_user_mmap_entry_remove(qp->rq_entry);
Signature · 1 line
2.51.0
maintainer Acked-by on v4 1/1
#1 lore raw
Quoted 136 lines · Ruoyu Wang

siw_create_qp() currently calls siw_qp_add() before the queues, CQ pointers, state, completion, and device list entry are ready. A QPN lookup can therefore reach a QP that is still being constructed.

Move siw_qp_add() to the end of siw_create_qp(), after QP initialization and before adding the QP to the siw device list.

Fixes f29dd55b0236 ("rdma/siw: queue pair methods")

---
Changes in v4:
- Move if (udata->outlen < sizeof(uresp)) into the second if (udata) clause, right before ib_copy_to_udata() (Bernard Metzler).
- Move INIT_LIST_HEAD(&qp->devq) to just before spin_lock_irqsave(), close to list_add_tail() where it logically belongs (Bernard Metzler).

Changes in v3:
- Move siw_qp_add()/xa_alloc() to the end of siw_create_qp().
- Drop the QPN reservation helper from v2.

  drivers/infiniband/sw/siw/siw_verbs.c | 57 ++++++++++++++++----------
  1 file changed, 35 insertions(+), 22 deletions(-)
drivers/infiniband/sw/siw/siw_verbs.c +21 -20
@@ -316,6 +316,7 @@
  	struct siw_ucontext *uctx =
  		rdma_udata_to_drv_context(udata, struct siw_ucontext,
  					  base_ucontext);
+	struct siw_uresp_create_qp uresp = {};
  	unsigned long flags;
  	int num_sqe, num_rqe, rv = 0;
  	size_t length;
@@ -369,11 +370,6 @@
  	spin_lock_init(&qp->rq_lock);
  	spin_lock_init(&qp->orq_lock);
 
-	rv = siw_qp_add(sdev, qp);
-	if (rv)
-		goto err_atomic;
-
-
  	/* All queue indices are derived from modulo operations
  	 * on a free running 'get' (consumer) and 'put' (producer)
  	 * unsigned counter. Having queue sizes at power of two
@@ -391,14 +387,14 @@
 
  	if (qp->sendq == NULL) {
  		rv = -ENOMEM;
-		goto err_out_xa;
+		goto err_out;
  	}
  	if (attrs->sq_sig_type != IB_SIGNAL_REQ_WR) {
  		if (attrs->sq_sig_type == IB_SIGNAL_ALL_WR)
  			qp->attrs.flags |= SIW_SIGNAL_ALL_WR;
  		else {
  			rv = -EINVAL;
-			goto err_out_xa;
+			goto err_out;
  		}
  	}
  	qp->pd = pd;
@@ -424,7 +420,7 @@
 
  		if (qp->recvq == NULL) {
  			rv = -ENOMEM;
-			goto err_out_xa;
+			goto err_out;
  		}
  		qp->attrs.rq_size = num_rqe;
  	}
@@ -439,11 +435,8 @@
  	qp->attrs.state = SIW_QP_STATE_IDLE;
 
  	if (udata) {
-		struct siw_uresp_create_qp uresp = {};
-
  		uresp.num_sqe = num_sqe;
  		uresp.num_rqe = num_rqe;
-		uresp.qp_id = qp_id(qp);
 
  		if (qp->sendq) {
  			length = num_sqe * sizeof(struct siw_sqe);
@@ -452,7 +445,7 @@
  						      length, &uresp.sq_key);
  			if (!qp->sq_entry) {
  				rv = -ENOMEM;
-				goto err_out_xa;
+				goto err_out;
  			}
  		}
 
@@ -464,9 +457,23 @@
  			if (!qp->rq_entry) {
  				uresp.sq_key = SIW_INVAL_UOBJ_KEY;
  				rv = -ENOMEM;
-				goto err_out_xa;
+				goto err_out;
  			}
  		}
+	}
+	qp->tx_cpu = siw_get_tx_cpu(sdev);
+	if (qp->tx_cpu < 0) {
+		rv = -EINVAL;
+		goto err_out;
+	}
+	init_completion(&qp->qp_free);
+
+	rv = siw_qp_add(sdev, qp);
+	if (rv)
+		goto err_out_tx;
+
+	if (udata) {
+		uresp.qp_id = qp_id(qp);
 
  		if (udata->outlen < sizeof(uresp)) {
  			rv = -EINVAL;
@@ -476,22 +483,19 @@
  		if (rv)
  			goto err_out_xa;
  	}
-	qp->tx_cpu = siw_get_tx_cpu(sdev);
-	if (qp->tx_cpu < 0) {
-		rv = -EINVAL;
-		goto err_out_xa;
-	}
+
  	INIT_LIST_HEAD(&qp->devq);
  	spin_lock_irqsave(&sdev->lock, flags);
  	list_add_tail(&qp->devq, &sdev->qp_list);
  	spin_unlock_irqrestore(&sdev->lock, flags);
 
-	init_completion(&qp->qp_free);
-

drivers/infiniband/sw/siw/siw_verbs.c line 501

Quoted 10 lines
drivers/infiniband/sw/siw/siw_verbs.c +3 -0
  	return 0;
 
  err_out_xa:
  	xa_erase(&sdev->qp_xa, qp_id(qp));
+err_out_tx:
+	siw_put_tx_cpu(qp->tx_cpu);
+err_out:
  	if (uctx) {
  		rdma_user_mmap_entry_remove(qp->sq_entry);
  		rdma_user_mmap_entry_remove(qp->rq_entry);

Thanks Ruoyu, that looks good.

maintainer Applied
#2 lore raw
In reply to Ruoyu Wang
Quoted 18 lines · Ruoyu Wang

siw_create_qp() currently calls siw_qp_add() before the queues, CQ pointers, state, completion, and device list entry are ready. A QPN lookup can therefore reach a QP that is still being constructed.

Move siw_qp_add() to the end of siw_create_qp(), after QP initialization and before adding the QP to the siw device list.

Fixes f29dd55b0236 ("rdma/siw: queue pair methods")

---
Changes in v4:
- Move if (udata->outlen < sizeof(uresp)) into the second if (udata) clause, right before ib_copy_to_udata() (Bernard Metzler).
- Move INIT_LIST_HEAD(&qp->devq) to just before spin_lock_irqsave(), close to list_add_tail() where it logically belongs (Bernard Metzler).

Quoted 6 lines

Changes in v3:
- Move siw_qp_add()/xa_alloc() to the end of siw_create_qp().
- Drop the QPN reservation helper from v2.

 drivers/infiniband/sw/siw/siw_verbs.c | 57 ++++++++++++++++----------
 1 file changed, 35 insertions(+), 22 deletions(-)

Applied to for-rc, thanks

Jason