RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp

PATCH In review Krishnamraju Eraparaju 2 participants 10 messages Sep 27, 2019 – Oct 4, 2019

Subsystems (2)

InfiniBand Soft-iWARP driver (siw)

lore

Share & tools

lore

Krishnamraju Eraparaju

author

2019-09-27 22:16 UTC #0 lore raw

PATCH 1/1 RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp

The storage ULPs(iSER & NVMeOF) uses ib_drain_qp() to drain QP/CQ properly. But SIW is currently using it's own routines to drain SQ & RQ, which can't help ULPs to determine the last CQE. Failing to wait until last CQE causes touch after free issues:

[  +0.001831] general protection fault: 0000 [#1] SMP PTI
[  +0.000002] Call Trace:
[  +0.000026]  ? __ib_process_cq+0x7a/0xc0 [ib_core]
[  +0.000008]  ? ib_poll_handler+0x2c/0x80 [ib_core]
[  +0.000005]  ? irq_poll_softirq+0xae/0x110
[  +0.000005]  ? __do_softirq+0xda/0x2a8
[  +0.000004]  ? run_ksoftirqd+0x26/0x40
[  +0.000005]  ? smpboot_thread_fn+0x10e/0x160
[  +0.000004]  ? kthread+0xf8/0x130
[  +0.000003]  ? sort_range+0x20/0x20
[  +0.000003]  ? kthread_bind+0x10/0x10
[  +0.000003]  ? ret_from_fork+0x35/0x40

Hence, changing the SQ & RQ drain logic to support current IB/core drain semantics, though this drain method does not naturally aligns to iWARP spec(where post_send/recv calls are not allowed in QP ERROR state). More on this was described in below commit:
commit 4fe7c2962e11 ("iw_cxgb4: refactor sq/rq drain logic")

Signed-off-by Krishnamraju Eraparaju <[email protected]>

---
 drivers/infiniband/sw/siw/siw.h       |  3 +-
 drivers/infiniband/sw/siw/siw_cm.c    |  4 +-
 drivers/infiniband/sw/siw/siw_main.c  | 20 ---------
 drivers/infiniband/sw/siw/siw_verbs.c | 60 +++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 23 deletions(-)

drivers/infiniband/sw/siw/siw.h +2 -1

index dba4535494ab..ad4f078e4587 100644
@@ -240,7 +240,8 @@ enum siw_qp_flags {
 	SIW_RDMA_READ_ENABLED = (1 << 2),
 	SIW_SIGNAL_ALL_WR = (1 << 3),
 	SIW_MPA_CRC = (1 << 4),
-	SIW_QP_IN_DESTROY = (1 << 5)
+	SIW_QP_IN_DESTROY = (1 << 5),
+	SIW_QP_DRAINED_FINAL = (1 << 6)
 };
 
 enum siw_qp_attr_mask {

drivers/infiniband/sw/siw/siw_cm.c +2 -2

index 8c1931a57f4a..fb830622d32e 100644
@@ -857,7 +857,7 @@ static int siw_proc_mpareply(struct siw_cep *cep)
 	memset(&qp_attrs, 0, sizeof(qp_attrs));
 
 	if (rep->params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.orq_size = cep->ord;

@@ -1675,7 +1675,7 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.sk = cep->sock;
 	if (cep->mpa.hdr.params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 	qp_attrs.state = SIW_QP_STATE_RTS;
 
 	siw_dbg_cep(cep, "[QP%u]: moving to rts\n", qp_id(qp));

drivers/infiniband/sw/siw/siw_main.c +0 ~~-20~~

index 05a92f997f60..fb01407a310f 100644
@@ -248,24 +248,6 @@ static struct ib_qp *siw_get_base_qp(struct ib_device *base_dev, int id)
 	return NULL;
 }
 
-static void siw_verbs_sq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
-	siw_sq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
-static void siw_verbs_rq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
-	siw_rq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
 static const struct ib_device_ops siw_device_ops = {
 	.owner = THIS_MODULE,
 	.uverbs_abi_ver = SIW_ABI_VERSION,

@@ -284,8 +266,6 @@ static const struct ib_device_ops siw_device_ops = {
 	.destroy_cq = siw_destroy_cq,
 	.destroy_qp = siw_destroy_qp,
 	.destroy_srq = siw_destroy_srq,
-	.drain_rq = siw_verbs_rq_flush,
-	.drain_sq = siw_verbs_sq_flush,
 	.get_dma_mr = siw_get_dma_mr,
 	.get_port_immutable = siw_get_port_immutable,
 	.iw_accept = siw_accept,

drivers/infiniband/sw/siw/siw_verbs.c +60 -0

84 lines not on this page · raw message

Signature · 1 line

2.23.0.rc0

Bernard Metzler

2019-09-30 15:37 UTC #1 lore raw

-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----

Quoted 9 lines

To: [email protected], [email protected]
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 09/28/2019 12:16AM
Cc: [email protected], [email protected], [email protected], "Krishnamraju Eraparaju" <[email protected]>
Subject: [EXTERNAL] [PATCH for-next] RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp

Quoted 4 lines

The storage ULPs(iSER & NVMeOF) uses ib_drain_qp() to drain QP/CQ properly. But SIW is currently using it's own routines to drain SQ & RQ, which can't help ULPs to determine the last CQE. Failing to wait until last CQE causes touch after free issues:

Hi Krishna,

Before reviewing, please let me fully understand what is going on here/why we need that patch.

Is this issue caused since the ulp expects the ib_drain_xx driver method to be blocking until all completions are reaped, and siw does not block?

Is the ULP expected to call ib_drain_xx only if no other work is pending (SQ/RQ/CQ)? If not, shall all previous work be completed with FLUSH error?

Many thanks!
Bernard.

Quoted 215 lines

[  +0.001831] general protection fault: 0000 [#1] SMP PTI
[  +0.000002] Call Trace:
[  +0.000026]  ? __ib_process_cq+0x7a/0xc0 [ib_core]
[  +0.000008]  ? ib_poll_handler+0x2c/0x80 [ib_core]
[  +0.000005]  ? irq_poll_softirq+0xae/0x110
[  +0.000005]  ? __do_softirq+0xda/0x2a8
[  +0.000004]  ? run_ksoftirqd+0x26/0x40
[  +0.000005]  ? smpboot_thread_fn+0x10e/0x160
[  +0.000004]  ? kthread+0xf8/0x130
[  +0.000003]  ? sort_range+0x20/0x20
[  +0.000003]  ? kthread_bind+0x10/0x10
[  +0.000003]  ? ret_from_fork+0x35/0x40

Signed-off-by Krishnamraju Eraparaju <[email protected]>

---

drivers/infiniband/sw/siw/siw.h       |  3 +-
drivers/infiniband/sw/siw/siw_cm.c    |  4 +-
drivers/infiniband/sw/siw/siw_main.c  | 20 ---------
drivers/infiniband/sw/siw/siw_verbs.c | 60
+++++++++++++++++++++++++++
4 files changed, 64 insertions(+), 23 deletions(-)

drivers/infiniband/sw/siw/siw.h +2 -1

index dba4535494ab..ad4f078e4587 100644
@@ -240,7 +240,8 @@ enum siw_qp_flags {
 	SIW_RDMA_READ_ENABLED = (1 << 2),
 	SIW_SIGNAL_ALL_WR = (1 << 3),
 	SIW_MPA_CRC = (1 << 4),
-	SIW_QP_IN_DESTROY = (1 << 5)
+	SIW_QP_IN_DESTROY = (1 << 5),
+	SIW_QP_DRAINED_FINAL = (1 << 6)
 };
 
 enum siw_qp_attr_mask {

drivers/infiniband/sw/siw/siw_cm.c +2 -2

index 8c1931a57f4a..fb830622d32e 100644
@@ -857,7 +857,7 @@ static int siw_proc_mpareply(struct siw_cep *cep)
 	memset(&qp_attrs, 0, sizeof(qp_attrs));
 
 	if (rep->params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.orq_size = cep->ord;

@@ -1675,7 +1675,7 @@ int siw_accept(struct iw_cm_id *id, struct
 iw_cm_conn_param *params)
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.sk = cep->sock;
 	if (cep->mpa.hdr.params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 	qp_attrs.state = SIW_QP_STATE_RTS;

	siw_dbg_cep(cep, "[QP%u]: moving to rts\n", qp_id(qp));

drivers/infiniband/sw/siw/siw_main.c +0 ~~-18~~

index 05a92f997f60..fb01407a310f 100644
@@ -248,24 +248,6 @@ static struct ib_qp *siw_get_base_qp(struct
 ib_device *base_dev, int id)
 	return NULL;
 }
 
-static void siw_verbs_sq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
-	siw_sq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
-static void siw_verbs_rq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
-	siw_rq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
 static const struct ib_device_ops siw_device_ops = {
 	.owner = THIS_MODULE,

.uverbs_abi_ver = SIW_ABI_VERSION,

drivers/infiniband/sw/siw/siw_main.c +0 -2

@@ -284,8 +266,6 @@ static const struct ib_device_ops siw_device_ops
 = {
 	.destroy_cq = siw_destroy_cq,
 	.destroy_qp = siw_destroy_qp,
 	.destroy_srq = siw_destroy_srq,
-	.drain_rq = siw_verbs_rq_flush,
-	.drain_sq = siw_verbs_sq_flush,
 	.get_dma_mr = siw_get_dma_mr,
 	.get_port_immutable = siw_get_port_immutable,

.iw_accept = siw_accept,

drivers/infiniband/sw/siw/siw_verbs.c +1 -0

index 869e02b69a01..5dd62946a649 100644
@@ -596,6 +596,13 @@ int siw_verbs_modify_qp(struct ib_qp *base_qp,
 struct ib_qp_attr *attr,
 
 	rv = siw_qp_modify(qp, &new_attrs, siw_attr_mask);
 
+	/* QP state ERROR here ensures that all the SQ & RQ entries got

drained

drivers/infiniband/sw/siw/siw_verbs.c +1 -0

+	 * completely. And henceforth, no more entries will be added to the

CQ,

drivers/infiniband/sw/siw/siw_verbs.c +5 -0

+	 * exception is special drain CQEs via ib_drain_qp().
+	 */
+	if (qp->attrs.state == SIW_QP_STATE_ERROR)
+		qp->attrs.flags |= SIW_QP_DRAINED_FINAL;
+

	up_write(&qp->state_lock);
out:
	return rv;

drivers/infiniband/sw/siw/siw_verbs.c +38 -0

@@ -687,6 +694,44 @@ static int siw_copy_inline_sgl(const struct
 ib_send_wr *core_wr,
 	return bytes;
 }
 
+/* SQ final completion routine to support ib_drain_sp(). */
+int siw_sq_final_comp(struct siw_qp *qp, const struct ib_send_wr
 *wr,
+		      const struct ib_send_wr **bad_wr)
+{
+	struct siw_sqe sqe = {};
+	int rv = 0;
+
+	while (wr) {
+		sqe.id = wr->wr_id;
+		sqe.opcode = wr->opcode;
+		rv = siw_sqe_complete(qp, &sqe, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
+		}
+		wr = wr->next;
+	}
+	return rv;
+}
+
+/* RQ final completion routine to support ib_drain_rp(). */
+int siw_rq_final_comp(struct siw_qp *qp, const struct ib_recv_wr
 *wr,
+		      const struct ib_recv_wr **bad_wr)
+{
+	struct siw_rqe rqe = {};
+	int rv = 0;
+
+	while (wr) {
+		rqe.id = wr->wr_id;
+		rv = siw_rqe_complete(qp, &rqe, 0, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
+		}
+		wr = wr->next;
+	}
+	return rv;
+}

/*
* siw_post_send()
*

drivers/infiniband/sw/siw/siw_verbs.c +2 -0

@@ -705,6 +750,15 @@ int siw_post_send(struct ib_qp *base_qp, const
 struct ib_send_wr *wr,
 	unsigned long flags;
 	int rv = 0;
 
+	/* Currently there is no way to distinguish between special drain
+	 * WRs and normal WRs(?), so we do FLUSH_ERR for all the WRs

that've

drivers/infiniband/sw/siw/siw_verbs.c +7 -0

+	 * arrived in the ERROR/SIW_QP_DRAINED_FINAL state, assuming we get
+	 * only special drain WRs in this state via ib_drain_sq().
+	 */
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_sq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}

/*
* Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.

drivers/infiniband/sw/siw/siw_verbs.c +6 -0

@@ -919,6 +973,12 @@ int siw_post_receive(struct ib_qp *base_qp,
 const struct ib_recv_wr *wr,
 		*bad_wr = wr;
 		return -EOPNOTSUPP; /* what else from errno.h? */
 	}
+
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_rq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}
+
 	/*
 	 * Try to acquire QP state lock. Must be non-blocking

* to accommodate kernel clients needs.

Signature · 1 line

2.23.0.rc0

Krishnamraju Eraparaju

author

2019-10-01 09:52 UTC #2 lore raw

Quoted 16 lines · Bernard Metzler

-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----
Quoted 9 lines
To: [email protected], [email protected]
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 09/28/2019 12:16AM
Cc: [email protected], [email protected], [email protected], "Krishnamraju Eraparaju" <[email protected]>
Subject: [EXTERNAL] [PATCH for-next] RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp
Quoted 4 lines
The storage ULPs(iSER & NVMeOF) uses ib_drain_qp() to drain QP/CQ properly. But SIW is currently using it's own routines to drain SQ & RQ, which can't help ULPs to determine the last CQE. Failing to wait until last CQE causes touch after free issues:

Quoted 8 lines

Hi Krishna,
Before reviewing, please let me fully understand what is going on here/why we need that patch.
Is this issue caused since the ulp expects the ib_drain_xx driver method to be blocking until all completions are reaped, and siw does not block?

Yes, SIW is currently using provider-specific drain_qp logic,
IE: siw_verbs_rq/sq_flush(), with this logic though all the SQ & RQ entries are flushed to CQ but ULPs cannot ensure when exactly the processing of all CQEs for those WRs, posted prior to ib_drain_xx(), got completed.
Due to this uncertainity, sometimes iSER/NVMeOF driver(assuming all the CQEs are processed) proceed to release resouces/destroy_qp, causing touch after free issues.

Quoted 4 lines

Is the ULP expected to call ib_drain_xx only if no other work is pending (SQ/RQ/CQ)? If not, shall all previous work be completed with FLUSH error?

In error cases(eg: link-down), I see iSER/NVMEOF drivers performing disconnect/drain_qp though there is some pending work to be processed. This may be valid due to the ERROR.
And all that pending work gets completed with FLUSH error.

Quoted 224 lines

Many thanks!
Bernard.

Quoted 215 lines

[  +0.001831] general protection fault: 0000 [#1] SMP PTI
[  +0.000002] Call Trace:
[  +0.000026]  ? __ib_process_cq+0x7a/0xc0 [ib_core]
[  +0.000008]  ? ib_poll_handler+0x2c/0x80 [ib_core]
[  +0.000005]  ? irq_poll_softirq+0xae/0x110
[  +0.000005]  ? __do_softirq+0xda/0x2a8
[  +0.000004]  ? run_ksoftirqd+0x26/0x40
[  +0.000005]  ? smpboot_thread_fn+0x10e/0x160
[  +0.000004]  ? kthread+0xf8/0x130
[  +0.000003]  ? sort_range+0x20/0x20
[  +0.000003]  ? kthread_bind+0x10/0x10
[  +0.000003]  ? ret_from_fork+0x35/0x40

Signed-off-by Krishnamraju Eraparaju <[email protected]>

---

drivers/infiniband/sw/siw/siw.h       |  3 +-
drivers/infiniband/sw/siw/siw_cm.c    |  4 +-
drivers/infiniband/sw/siw/siw_main.c  | 20 ---------
drivers/infiniband/sw/siw/siw_verbs.c | 60
+++++++++++++++++++++++++++
4 files changed, 64 insertions(+), 23 deletions(-)

drivers/infiniband/sw/siw/siw.h +2 -1

index dba4535494ab..ad4f078e4587 100644
@@ -240,7 +240,8 @@ enum siw_qp_flags {
 	SIW_RDMA_READ_ENABLED = (1 << 2),
 	SIW_SIGNAL_ALL_WR = (1 << 3),
 	SIW_MPA_CRC = (1 << 4),
-	SIW_QP_IN_DESTROY = (1 << 5)
+	SIW_QP_IN_DESTROY = (1 << 5),
+	SIW_QP_DRAINED_FINAL = (1 << 6)
 };
 
 enum siw_qp_attr_mask {

drivers/infiniband/sw/siw/siw_cm.c +2 -2

index 8c1931a57f4a..fb830622d32e 100644
@@ -857,7 +857,7 @@ static int siw_proc_mpareply(struct siw_cep *cep)
 	memset(&qp_attrs, 0, sizeof(qp_attrs));
 
 	if (rep->params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.orq_size = cep->ord;

@@ -1675,7 +1675,7 @@ int siw_accept(struct iw_cm_id *id, struct
 iw_cm_conn_param *params)
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.sk = cep->sock;
 	if (cep->mpa.hdr.params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 	qp_attrs.state = SIW_QP_STATE_RTS;

	siw_dbg_cep(cep, "[QP%u]: moving to rts\n", qp_id(qp));

drivers/infiniband/sw/siw/siw_main.c +0 ~~-18~~

index 05a92f997f60..fb01407a310f 100644
@@ -248,24 +248,6 @@ static struct ib_qp *siw_get_base_qp(struct
 ib_device *base_dev, int id)
 	return NULL;
 }
 
-static void siw_verbs_sq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
-	siw_sq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
-static void siw_verbs_rq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
-	siw_rq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
 static const struct ib_device_ops siw_device_ops = {
 	.owner = THIS_MODULE,

.uverbs_abi_ver = SIW_ABI_VERSION,

drivers/infiniband/sw/siw/siw_main.c +0 -2

@@ -284,8 +266,6 @@ static const struct ib_device_ops siw_device_ops
 = {
 	.destroy_cq = siw_destroy_cq,
 	.destroy_qp = siw_destroy_qp,
 	.destroy_srq = siw_destroy_srq,
-	.drain_rq = siw_verbs_rq_flush,
-	.drain_sq = siw_verbs_sq_flush,
 	.get_dma_mr = siw_get_dma_mr,
 	.get_port_immutable = siw_get_port_immutable,

.iw_accept = siw_accept,

drivers/infiniband/sw/siw/siw_verbs.c +1 -0

index 869e02b69a01..5dd62946a649 100644
@@ -596,6 +596,13 @@ int siw_verbs_modify_qp(struct ib_qp *base_qp,
 struct ib_qp_attr *attr,
 
 	rv = siw_qp_modify(qp, &new_attrs, siw_attr_mask);
 
+	/* QP state ERROR here ensures that all the SQ & RQ entries got

drained

drivers/infiniband/sw/siw/siw_verbs.c +1 -0

+	 * completely. And henceforth, no more entries will be added to the

CQ,

drivers/infiniband/sw/siw/siw_verbs.c +5 -0

+	 * exception is special drain CQEs via ib_drain_qp().
+	 */
+	if (qp->attrs.state == SIW_QP_STATE_ERROR)
+		qp->attrs.flags |= SIW_QP_DRAINED_FINAL;
+

	up_write(&qp->state_lock);
out:
	return rv;

drivers/infiniband/sw/siw/siw_verbs.c +38 -0

@@ -687,6 +694,44 @@ static int siw_copy_inline_sgl(const struct
 ib_send_wr *core_wr,
 	return bytes;
 }
 
+/* SQ final completion routine to support ib_drain_sp(). */
+int siw_sq_final_comp(struct siw_qp *qp, const struct ib_send_wr
 *wr,
+		      const struct ib_send_wr **bad_wr)
+{
+	struct siw_sqe sqe = {};
+	int rv = 0;
+
+	while (wr) {
+		sqe.id = wr->wr_id;
+		sqe.opcode = wr->opcode;
+		rv = siw_sqe_complete(qp, &sqe, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
+		}
+		wr = wr->next;
+	}
+	return rv;
+}
+
+/* RQ final completion routine to support ib_drain_rp(). */
+int siw_rq_final_comp(struct siw_qp *qp, const struct ib_recv_wr
 *wr,
+		      const struct ib_recv_wr **bad_wr)
+{
+	struct siw_rqe rqe = {};
+	int rv = 0;
+
+	while (wr) {
+		rqe.id = wr->wr_id;
+		rv = siw_rqe_complete(qp, &rqe, 0, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
+		}
+		wr = wr->next;
+	}
+	return rv;
+}

/*
* siw_post_send()
*

drivers/infiniband/sw/siw/siw_verbs.c +2 -0

@@ -705,6 +750,15 @@ int siw_post_send(struct ib_qp *base_qp, const
 struct ib_send_wr *wr,
 	unsigned long flags;
 	int rv = 0;
 
+	/* Currently there is no way to distinguish between special drain
+	 * WRs and normal WRs(?), so we do FLUSH_ERR for all the WRs

that've

drivers/infiniband/sw/siw/siw_verbs.c +7 -0

+	 * arrived in the ERROR/SIW_QP_DRAINED_FINAL state, assuming we get
+	 * only special drain WRs in this state via ib_drain_sq().
+	 */
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_sq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}

/*
* Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.

drivers/infiniband/sw/siw/siw_verbs.c +6 -0

@@ -919,6 +973,12 @@ int siw_post_receive(struct ib_qp *base_qp,
 const struct ib_recv_wr *wr,
 		*bad_wr = wr;
 		return -EOPNOTSUPP; /* what else from errno.h? */
 	}
+
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_rq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}
+
 	/*
 	 * Try to acquire QP state lock. Must be non-blocking

* to accommodate kernel clients needs.

Signature · 1 line

2.23.0.rc0

Bernard Metzler

2019-10-01 15:56 UTC #3 lore raw

-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----

Quoted 45 lines

To: "Bernard Metzler" <[email protected]>
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 10/01/2019 11:52AM
Cc: "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "Potnuri Bharat Teja" <[email protected]>, "Nirranjan Kirubaharan" <[email protected]>
Subject: [EXTERNAL] Re: [PATCH for-next] RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp
Quoted 9 lines · Bernard Metzler
-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----
Quoted 7 lines
To: [email protected], [email protected]
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 09/28/2019 12:16AM
Cc: [email protected], [email protected], [email protected], "Krishnamraju Eraparaju" <[email protected]>
Subject: [EXTERNAL] [PATCH for-next] RDMA/siw: fix SQ/RQ drain
logic
Quoted 7 lines
Quoted 6 lines
to support ib_drain_qp
The storage ULPs(iSER & NVMeOF) uses ib_drain_qp() to drain QP/CQ properly. But SIW is currently using it's own routines to drain SQ & RQ, which can't help ULPs to determine the last CQE. Failing to wait until last CQE causes touch after free issues:
Quoted 8 lines
Hi Krishna,
Before reviewing, please let me fully understand what is going on here/why we need that patch.
Is this issue caused since the ulp expects the ib_drain_xx driver method to be blocking until all completions are reaped, and siw does not block?
Yes, SIW is currently using provider-specific drain_qp logic,
IE: siw_verbs_rq/sq_flush(), with this logic though all the SQ & RQ entries are flushed to CQ but ULPs cannot ensure when exactly the processing of all CQEs for those WRs, posted prior to ib_drain_xx(), got completed.
Due to this uncertainity, sometimes iSER/NVMeOF driver(assuming all the CQEs are processed) proceed to release resouces/destroy_qp, causing touch after free issues.
Quoted 1 line

Quoted 8 lines

Quoted 3 lines
Is the ULP expected to call ib_drain_xx only if no other work is pending (SQ/RQ/CQ)? If not, shall all previous work be completed with FLUSH error?
In error cases(eg: link-down), I see iSER/NVMEOF drivers performing disconnect/drain_qp though there is some pending work to be processed.
This may be valid due to the ERROR.
And all that pending work gets completed with FLUSH error.

OK understood.

Dropping the siw private drain routines makes sense to me.

Otherwise, I think a cleaner solution is to just allow processing kernel ULPs send/recv WR's while the QP is already in ERROR state. In that case, we immediately complete with FLUSH error. We would avoid the extra state flag, and the extra check for that flag on the fast path.

I can send such patch tomorrow if you like.

Many thanks,
Bernard.

Quoted 234 lines

Quoted 24 lines

Many thanks!
Bernard.

Quoted 16 lines

[  +0.001831] general protection fault: 0000 [#1] SMP PTI
[  +0.000002] Call Trace:
[  +0.000026]  ? __ib_process_cq+0x7a/0xc0 [ib_core]
[  +0.000008]  ? ib_poll_handler+0x2c/0x80 [ib_core]
[  +0.000005]  ? irq_poll_softirq+0xae/0x110
[  +0.000005]  ? __do_softirq+0xda/0x2a8
[  +0.000004]  ? run_ksoftirqd+0x26/0x40
[  +0.000005]  ? smpboot_thread_fn+0x10e/0x160
[  +0.000004]  ? kthread+0xf8/0x130
[  +0.000003]  ? sort_range+0x20/0x20
[  +0.000003]  ? kthread_bind+0x10/0x10
[  +0.000003]  ? ret_from_fork+0x35/0x40

Hence, changing the SQ & RQ drain logic to support current IB/core drain semantics, though this drain method does not naturally

aligns

Quoted 27 lines

Quoted 27 lines
to iWARP spec(where post_send/recv calls are not allowed in QP ERROR state). More on this was described in below commit:
commit 4fe7c2962e11 ("iw_cxgb4: refactor sq/rq drain logic")
Signed-off-by Krishnamraju Eraparaju <[email protected]>
---
drivers/infiniband/sw/siw/siw.h       |  3 +-
drivers/infiniband/sw/siw/siw_cm.c    |  4 +-
drivers/infiniband/sw/siw/siw_main.c  | 20 ---------
drivers/infiniband/sw/siw/siw_verbs.c | 60
+++++++++++++++++++++++++++
4 files changed, 64 insertions(+), 23 deletions(-)
drivers/infiniband/sw/siw/siw.h +2 -1
index dba4535494ab..ad4f078e4587 100644
@@ -240,7 +240,8 @@ enum siw_qp_flags {
 	SIW_RDMA_READ_ENABLED = (1 << 2),
 	SIW_SIGNAL_ALL_WR = (1 << 3),
 	SIW_MPA_CRC = (1 << 4),
-	SIW_QP_IN_DESTROY = (1 << 5)
+	SIW_QP_IN_DESTROY = (1 << 5),
+	SIW_QP_DRAINED_FINAL = (1 << 6)
 };
 

Quoted 7 lines

drivers/infiniband/sw/siw/siw_cm.c static int siw_proc_mpareply(struct siw_cep
Quoted 7 lines
drivers/infiniband/sw/siw/siw.h +0 -0
 enum siw_qp_attr_mask {
drivers/infiniband/sw/siw/siw_cm.c +0 -0
index 8c1931a57f4a..fb830622d32e 100644
@@ -857,7 +857,7 @@ static int siw_proc_mpareply(struct siw_cep

*cep)

Quoted 42 lines

drivers/infiniband/sw/siw/siw_cm.c +2 -2

 	memset(&qp_attrs, 0, sizeof(qp_attrs));
 
 	if (rep->params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.orq_size = cep->ord;

@@ -1675,7 +1675,7 @@ int siw_accept(struct iw_cm_id *id, struct
 iw_cm_conn_param *params)
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.sk = cep->sock;
 	if (cep->mpa.hdr.params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 	qp_attrs.state = SIW_QP_STATE_RTS;

	siw_dbg_cep(cep, "[QP%u]: moving to rts\n", qp_id(qp));

drivers/infiniband/sw/siw/siw_main.c +0 ~~-14~~

index 05a92f997f60..fb01407a310f 100644
@@ -248,24 +248,6 @@ static struct ib_qp *siw_get_base_qp(struct
 ib_device *base_dev, int id)
 	return NULL;
 }
 
-static void siw_verbs_sq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
-	siw_sq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
-static void siw_verbs_rq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);

Quoted 8 lines

drivers/infiniband/sw/siw/siw_main.c static const struct ib_device_ops
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c +0 -4
-	siw_rq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
 static const struct ib_device_ops siw_device_ops = {
 	.owner = THIS_MODULE,
.uverbs_abi_ver = SIW_ABI_VERSION,
drivers/infiniband/sw/siw/siw_main.c +0 -0
@@ -284,8 +266,6 @@ static const struct ib_device_ops

siw_device_ops

Quoted 7 lines

drivers/infiniband/sw/siw/siw_main.c +0 -2

 = {
 	.destroy_cq = siw_destroy_cq,
 	.destroy_qp = siw_destroy_qp,
 	.destroy_srq = siw_destroy_srq,
-	.drain_rq = siw_verbs_rq_flush,
-	.drain_sq = siw_verbs_sq_flush,
 	.get_dma_mr = siw_get_dma_mr,

Quoted 8 lines

drivers/infiniband/sw/siw/siw_verbs.c int siw_verbs_modify_qp(struct ib_qp
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c +0 -0
 	.get_port_immutable = siw_get_port_immutable,
.iw_accept = siw_accept,
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
index 869e02b69a01..5dd62946a649 100644
@@ -596,6 +596,13 @@ int siw_verbs_modify_qp(struct ib_qp

*base_qp,

Quoted 7 lines

drivers/infiniband/sw/siw/siw_verbs.c
Quoted 7 lines
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
 struct ib_qp_attr *attr,
 
 	rv = siw_qp_modify(qp, &new_attrs, siw_attr_mask);
 
+	/* QP state ERROR here ensures that all the SQ & RQ entries got
drained
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * completely. And henceforth, no more entries will be added to

the

Quoted 50 lines

CQ,

drivers/infiniband/sw/siw/siw_verbs.c +5 -0

+	 * exception is special drain CQEs via ib_drain_qp().
+	 */
+	if (qp->attrs.state == SIW_QP_STATE_ERROR)
+		qp->attrs.flags |= SIW_QP_DRAINED_FINAL;
+

	up_write(&qp->state_lock);
out:
	return rv;

drivers/infiniband/sw/siw/siw_verbs.c +34 -0

@@ -687,6 +694,44 @@ static int siw_copy_inline_sgl(const struct
 ib_send_wr *core_wr,
 	return bytes;
 }
 
+/* SQ final completion routine to support ib_drain_sp(). */
+int siw_sq_final_comp(struct siw_qp *qp, const struct ib_send_wr
 *wr,
+		      const struct ib_send_wr **bad_wr)
+{
+	struct siw_sqe sqe = {};
+	int rv = 0;
+
+	while (wr) {
+		sqe.id = wr->wr_id;
+		sqe.opcode = wr->opcode;
+		rv = siw_sqe_complete(qp, &sqe, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
+		}
+		wr = wr->next;
+	}
+	return rv;
+}
+
+/* RQ final completion routine to support ib_drain_rp(). */
+int siw_rq_final_comp(struct siw_qp *qp, const struct ib_recv_wr
 *wr,
+		      const struct ib_recv_wr **bad_wr)
+{
+	struct siw_rqe rqe = {};
+	int rv = 0;
+
+	while (wr) {
+		rqe.id = wr->wr_id;
+		rv = siw_rqe_complete(qp, &rqe, 0, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
+		}

Quoted 8 lines

drivers/infiniband/sw/siw/siw_verbs.c int siw_post_send(struct ib_qp *base_qp,
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c +4 -0
+		wr = wr->next;
+	}
+	return rv;
+}
/*
* siw_post_send()
*
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
@@ -705,6 +750,15 @@ int siw_post_send(struct ib_qp *base_qp,

const

Quoted 5 lines

drivers/infiniband/sw/siw/siw_verbs.c int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr,
Quoted 5 lines
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
 struct ib_send_wr *wr,
 	unsigned long flags;
 	int rv = 0;
 
+	/* Currently there is no way to distinguish between special

drain

Quoted 3 lines

drivers/infiniband/sw/siw/siw_verbs.c
Quoted 3 lines
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * WRs and normal WRs(?), so we do FLUSH_ERR for all the WRs
that've
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * arrived in the ERROR/SIW_QP_DRAINED_FINAL state, assuming we

get

Quoted 28 lines

Quoted 27 lines

drivers/infiniband/sw/siw/siw_verbs.c +6 -0

+	 * only special drain WRs in this state via ib_drain_sq().
+	 */
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_sq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}

/*
* Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.

drivers/infiniband/sw/siw/siw_verbs.c +6 -0

@@ -919,6 +973,12 @@ int siw_post_receive(struct ib_qp *base_qp,
 const struct ib_recv_wr *wr,
 		*bad_wr = wr;
 		return -EOPNOTSUPP; /* what else from errno.h? */
 	}
+
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_rq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}
+
 	/*
 	 * Try to acquire QP state lock. Must be non-blocking

* to accommodate kernel clients needs.

Signature · 1 line

2.23.0.rc0

Krishnamraju Eraparaju

author

2019-10-01 17:45 UTC #4 lore raw

Quoted 61 lines · Bernard Metzler

-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----
Quoted 45 lines
To: "Bernard Metzler" <[email protected]>
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 10/01/2019 11:52AM
Cc: "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "Potnuri Bharat Teja" <[email protected]>, "Nirranjan Kirubaharan" <[email protected]>
Subject: [EXTERNAL] Re: [PATCH for-next] RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp
Quoted 9 lines · Bernard Metzler
-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----
Quoted 7 lines
To: [email protected], [email protected]
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 09/28/2019 12:16AM
Cc: [email protected], [email protected], [email protected], "Krishnamraju Eraparaju" <[email protected]>
Subject: [EXTERNAL] [PATCH for-next] RDMA/siw: fix SQ/RQ drain
logic
Quoted 7 lines
Quoted 6 lines
to support ib_drain_qp
The storage ULPs(iSER & NVMeOF) uses ib_drain_qp() to drain QP/CQ properly. But SIW is currently using it's own routines to drain SQ & RQ, which can't help ULPs to determine the last CQE. Failing to wait until last CQE causes touch after free issues:
Quoted 8 lines
Hi Krishna,
Before reviewing, please let me fully understand what is going on here/why we need that patch.
Is this issue caused since the ulp expects the ib_drain_xx driver method to be blocking until all completions are reaped, and siw does not block?
Yes, SIW is currently using provider-specific drain_qp logic,
IE: siw_verbs_rq/sq_flush(), with this logic though all the SQ & RQ entries are flushed to CQ but ULPs cannot ensure when exactly the processing of all CQEs for those WRs, posted prior to ib_drain_xx(), got completed.
Due to this uncertainity, sometimes iSER/NVMeOF driver(assuming all the CQEs are processed) proceed to release resouces/destroy_qp, causing touch after free issues.
Quoted 1 line
Quoted 8 lines
Quoted 3 lines
Is the ULP expected to call ib_drain_xx only if no other work is pending (SQ/RQ/CQ)? If not, shall all previous work be completed with FLUSH error?
In error cases(eg: link-down), I see iSER/NVMEOF drivers performing disconnect/drain_qp though there is some pending work to be processed.
This may be valid due to the ERROR.
And all that pending work gets completed with FLUSH error.
OK understood.
Dropping the siw private drain routines makes sense to me.

Quoted 8 lines

Otherwise, I think a cleaner solution is to just allow processing kernel ULPs send/recv WR's while the QP is already in ERROR state. In that case, we immediately complete with FLUSH error. We would avoid the extra state flag, and the extra check for that flag on the fast path.
I can send such patch tomorrow if you like.

Sure Bernard, it's good if we can avoiding extra check in fast path. The only condition for using ib_drain_cq(with special CQE) is: the special CQE should be the last CQE to be processed in the completion queue.

Also, we can't miss the special CQE due to down_read_trylock() failure in post_send() and post_recv() routines.
Currenlty, special CQEs are being sent only once.

Thanks,
Krishna.

Quoted 239 lines

Many thanks,
Bernard.

Quoted 234 lines

Quoted 24 lines

Many thanks!
Bernard.

Quoted 16 lines

[  +0.001831] general protection fault: 0000 [#1] SMP PTI
[  +0.000002] Call Trace:
[  +0.000026]  ? __ib_process_cq+0x7a/0xc0 [ib_core]
[  +0.000008]  ? ib_poll_handler+0x2c/0x80 [ib_core]
[  +0.000005]  ? irq_poll_softirq+0xae/0x110
[  +0.000005]  ? __do_softirq+0xda/0x2a8
[  +0.000004]  ? run_ksoftirqd+0x26/0x40
[  +0.000005]  ? smpboot_thread_fn+0x10e/0x160
[  +0.000004]  ? kthread+0xf8/0x130
[  +0.000003]  ? sort_range+0x20/0x20
[  +0.000003]  ? kthread_bind+0x10/0x10
[  +0.000003]  ? ret_from_fork+0x35/0x40

Hence, changing the SQ & RQ drain logic to support current IB/core drain semantics, though this drain method does not naturally

aligns

Quoted 27 lines

Quoted 27 lines
to iWARP spec(where post_send/recv calls are not allowed in QP ERROR state). More on this was described in below commit:
commit 4fe7c2962e11 ("iw_cxgb4: refactor sq/rq drain logic")
Signed-off-by Krishnamraju Eraparaju <[email protected]>
---
drivers/infiniband/sw/siw/siw.h       |  3 +-
drivers/infiniband/sw/siw/siw_cm.c    |  4 +-
drivers/infiniband/sw/siw/siw_main.c  | 20 ---------
drivers/infiniband/sw/siw/siw_verbs.c | 60
+++++++++++++++++++++++++++
4 files changed, 64 insertions(+), 23 deletions(-)
drivers/infiniband/sw/siw/siw.h +2 -1
index dba4535494ab..ad4f078e4587 100644
@@ -240,7 +240,8 @@ enum siw_qp_flags {
 	SIW_RDMA_READ_ENABLED = (1 << 2),
 	SIW_SIGNAL_ALL_WR = (1 << 3),
 	SIW_MPA_CRC = (1 << 4),
-	SIW_QP_IN_DESTROY = (1 << 5)
+	SIW_QP_IN_DESTROY = (1 << 5),
+	SIW_QP_DRAINED_FINAL = (1 << 6)
 };
 

Quoted 7 lines

drivers/infiniband/sw/siw/siw_cm.c static int siw_proc_mpareply(struct siw_cep
Quoted 7 lines
drivers/infiniband/sw/siw/siw.h +0 -0
 enum siw_qp_attr_mask {
drivers/infiniband/sw/siw/siw_cm.c +0 -0
index 8c1931a57f4a..fb830622d32e 100644
@@ -857,7 +857,7 @@ static int siw_proc_mpareply(struct siw_cep

*cep)

Quoted 42 lines

drivers/infiniband/sw/siw/siw_cm.c +2 -2

 	memset(&qp_attrs, 0, sizeof(qp_attrs));
 
 	if (rep->params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.orq_size = cep->ord;

@@ -1675,7 +1675,7 @@ int siw_accept(struct iw_cm_id *id, struct
 iw_cm_conn_param *params)
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.sk = cep->sock;
 	if (cep->mpa.hdr.params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 	qp_attrs.state = SIW_QP_STATE_RTS;

	siw_dbg_cep(cep, "[QP%u]: moving to rts\n", qp_id(qp));

drivers/infiniband/sw/siw/siw_main.c +0 ~~-14~~

index 05a92f997f60..fb01407a310f 100644
@@ -248,24 +248,6 @@ static struct ib_qp *siw_get_base_qp(struct
 ib_device *base_dev, int id)
 	return NULL;
 }
 
-static void siw_verbs_sq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
-	siw_sq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
-static void siw_verbs_rq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);

Quoted 8 lines

drivers/infiniband/sw/siw/siw_main.c static const struct ib_device_ops
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c +0 -4
-	siw_rq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
 static const struct ib_device_ops siw_device_ops = {
 	.owner = THIS_MODULE,
.uverbs_abi_ver = SIW_ABI_VERSION,
drivers/infiniband/sw/siw/siw_main.c +0 -0
@@ -284,8 +266,6 @@ static const struct ib_device_ops

siw_device_ops

Quoted 7 lines

drivers/infiniband/sw/siw/siw_main.c +0 -2

 = {
 	.destroy_cq = siw_destroy_cq,
 	.destroy_qp = siw_destroy_qp,
 	.destroy_srq = siw_destroy_srq,
-	.drain_rq = siw_verbs_rq_flush,
-	.drain_sq = siw_verbs_sq_flush,
 	.get_dma_mr = siw_get_dma_mr,

Quoted 8 lines

drivers/infiniband/sw/siw/siw_verbs.c int siw_verbs_modify_qp(struct ib_qp
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c +0 -0
 	.get_port_immutable = siw_get_port_immutable,
.iw_accept = siw_accept,
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
index 869e02b69a01..5dd62946a649 100644
@@ -596,6 +596,13 @@ int siw_verbs_modify_qp(struct ib_qp

*base_qp,

Quoted 7 lines

drivers/infiniband/sw/siw/siw_verbs.c
Quoted 7 lines
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
 struct ib_qp_attr *attr,
 
 	rv = siw_qp_modify(qp, &new_attrs, siw_attr_mask);
 
+	/* QP state ERROR here ensures that all the SQ & RQ entries got
drained
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * completely. And henceforth, no more entries will be added to

the

Quoted 50 lines

CQ,

drivers/infiniband/sw/siw/siw_verbs.c +5 -0

+	 * exception is special drain CQEs via ib_drain_qp().
+	 */
+	if (qp->attrs.state == SIW_QP_STATE_ERROR)
+		qp->attrs.flags |= SIW_QP_DRAINED_FINAL;
+

	up_write(&qp->state_lock);
out:
	return rv;

drivers/infiniband/sw/siw/siw_verbs.c +34 -0

@@ -687,6 +694,44 @@ static int siw_copy_inline_sgl(const struct
 ib_send_wr *core_wr,
 	return bytes;
 }
 
+/* SQ final completion routine to support ib_drain_sp(). */
+int siw_sq_final_comp(struct siw_qp *qp, const struct ib_send_wr
 *wr,
+		      const struct ib_send_wr **bad_wr)
+{
+	struct siw_sqe sqe = {};
+	int rv = 0;
+
+	while (wr) {
+		sqe.id = wr->wr_id;
+		sqe.opcode = wr->opcode;
+		rv = siw_sqe_complete(qp, &sqe, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
+		}
+		wr = wr->next;
+	}
+	return rv;
+}
+
+/* RQ final completion routine to support ib_drain_rp(). */
+int siw_rq_final_comp(struct siw_qp *qp, const struct ib_recv_wr
 *wr,
+		      const struct ib_recv_wr **bad_wr)
+{
+	struct siw_rqe rqe = {};
+	int rv = 0;
+
+	while (wr) {
+		rqe.id = wr->wr_id;
+		rv = siw_rqe_complete(qp, &rqe, 0, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
+		}

Quoted 8 lines

drivers/infiniband/sw/siw/siw_verbs.c int siw_post_send(struct ib_qp *base_qp,
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c +4 -0
+		wr = wr->next;
+	}
+	return rv;
+}
/*
* siw_post_send()
*
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
@@ -705,6 +750,15 @@ int siw_post_send(struct ib_qp *base_qp,

const

Quoted 5 lines

drivers/infiniband/sw/siw/siw_verbs.c int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr,
Quoted 5 lines
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
 struct ib_send_wr *wr,
 	unsigned long flags;
 	int rv = 0;
 
+	/* Currently there is no way to distinguish between special

drain

Quoted 3 lines

drivers/infiniband/sw/siw/siw_verbs.c
Quoted 3 lines
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * WRs and normal WRs(?), so we do FLUSH_ERR for all the WRs
that've
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * arrived in the ERROR/SIW_QP_DRAINED_FINAL state, assuming we

get

Quoted 28 lines

Quoted 27 lines

drivers/infiniband/sw/siw/siw_verbs.c +6 -0

+	 * only special drain WRs in this state via ib_drain_sq().
+	 */
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_sq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}

/*
* Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.

drivers/infiniband/sw/siw/siw_verbs.c +6 -0

@@ -919,6 +973,12 @@ int siw_post_receive(struct ib_qp *base_qp,
 const struct ib_recv_wr *wr,
 		*bad_wr = wr;
 		return -EOPNOTSUPP; /* what else from errno.h? */
 	}
+
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_rq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}
+
 	/*
 	 * Try to acquire QP state lock. Must be non-blocking

* to accommodate kernel clients needs.

Signature · 1 line

2.23.0.rc0

Bernard Metzler

2019-10-02 11:28 UTC #5 lore raw

-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----

Quoted 91 lines

To: "Bernard Metzler" <[email protected]>
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 10/01/2019 07:45PM
Cc: "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "Potnuri Bharat Teja" <[email protected]>, "Nirranjan Kirubaharan" <[email protected]>
Subject: [EXTERNAL] Re: Re: [PATCH for-next] RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp
Quoted 8 lines · Bernard Metzler
-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----
Quoted 6 lines
To: "Bernard Metzler" <[email protected]>
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 10/01/2019 11:52AM
Cc: "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "Potnuri Bharat Teja" <[email protected]>, "Nirranjan Kirubaharan"
<[email protected]>
Quoted 6 lines
Quoted 6 lines
Subject: [EXTERNAL] Re: [PATCH for-next] RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp
Quoted 1 line · Bernard Metzler
-----"Krishnamraju Eraparaju" <[email protected]> wrote:
-----
Quoted 18 lines
Quoted 18 lines
Quoted 8 lines
Quoted 8 lines
To: [email protected], [email protected]
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 09/28/2019 12:16AM
Cc: [email protected], [email protected], [email protected], "Krishnamraju Eraparaju" <[email protected]>
Subject: [EXTERNAL] [PATCH for-next] RDMA/siw: fix SQ/RQ drain
logic
Quoted 9 lines
Quoted 6 lines
to support ib_drain_qp
The storage ULPs(iSER & NVMeOF) uses ib_drain_qp() to drain QP/CQ properly. But SIW is currently using it's own routines to drain SQ & RQ, which can't help ULPs to determine the last CQE. Failing to wait until last CQE causes touch after free issues:
Hi Krishna,
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Before reviewing, please let me fully understand what is going on here/why we need that patch.
Is this issue caused since the ulp expects the ib_drain_xx driver method to be blocking until all completions are reaped, and siw does not block?
Yes, SIW is currently using provider-specific drain_qp logic,
IE: siw_verbs_rq/sq_flush(), with this logic though all the SQ &
RQ
Quoted 2 lines
Quoted 2 lines
entries are flushed to CQ but ULPs cannot ensure when exactly the processing of all CQEs for those WRs, posted prior to
ib_drain_xx(),
Quoted 2 lines
Quoted 2 lines
got completed.
Due to this uncertainity, sometimes iSER/NVMeOF driver(assuming
all
Quoted 7 lines
Quoted 7 lines
the CQEs are processed) proceed to release resouces/destroy_qp, causing touch after free issues.
Quoted 4 lines
Is the ULP expected to call ib_drain_xx only if no other work is pending (SQ/RQ/CQ)? If not, shall all previous work be completed with FLUSH error?
In error cases(eg: link-down), I see iSER/NVMEOF drivers
performing
Quoted 10 lines
Quoted 4 lines
disconnect/drain_qp though there is some pending work to be processed.
This may be valid due to the ERROR.
And all that pending work gets completed with FLUSH error.
OK understood.
Dropping the siw private drain routines makes sense to me.
Quoted 8 lines
Otherwise, I think a cleaner solution is to just allow processing kernel ULPs send/recv WR's while the QP is already in ERROR state. In that case, we immediately complete with FLUSH error. We would avoid the extra state flag, and the extra check for that flag on the fast path.
I can send such patch tomorrow if you like.
Sure Bernard, it's good if we can avoiding extra check in fast path. The only condition for using ib_drain_cq(with special CQE) is: the special CQE should be the last CQE to be processed in the completion queue.

Quoted 5 lines

Also, we can't miss the special CQE due to down_read_trylock() failure
in post_send() and post_recv() routines.
Currenlty, special CQEs are being sent only once.

Well, at NVMeF target side. I have seen even two consecutive drain_sq() calls on the same QP, which does not hurt us.

I resend as v2 and have you as 'Signed-off' as well. I hope it is the right way to signal I partially re-wrote the patch.

Many thanks,
Bernard.

Quoted 253 lines

Thanks,
Krishna.
Quoted 27 lines
Many thanks,
Bernard.
Quoted 23 lines
Quoted 23 lines
Many thanks!
Bernard.
Quoted 15 lines
[  +0.001831] general protection fault: 0000 [#1] SMP PTI
[  +0.000002] Call Trace:
[  +0.000026]  ? __ib_process_cq+0x7a/0xc0 [ib_core]
[  +0.000008]  ? ib_poll_handler+0x2c/0x80 [ib_core]
[  +0.000005]  ? irq_poll_softirq+0xae/0x110
[  +0.000005]  ? __do_softirq+0xda/0x2a8
[  +0.000004]  ? run_ksoftirqd+0x26/0x40
[  +0.000005]  ? smpboot_thread_fn+0x10e/0x160
[  +0.000004]  ? kthread+0xf8/0x130
[  +0.000003]  ? sort_range+0x20/0x20
[  +0.000003]  ? kthread_bind+0x10/0x10
[  +0.000003]  ? ret_from_fork+0x35/0x40
Hence, changing the SQ & RQ drain logic to support current
IB/core
Quoted 39 lines
Quoted 39 lines
Quoted 1 line
Quoted 1 line
drain semantics, though this drain method does not naturally
aligns
Quoted 27 lines
Quoted 27 lines
to iWARP spec(where post_send/recv calls are not allowed in QP ERROR state). More on this was described in below commit:
commit 4fe7c2962e11 ("iw_cxgb4: refactor sq/rq drain logic")
Signed-off-by Krishnamraju Eraparaju <[email protected]>
---
drivers/infiniband/sw/siw/siw.h       |  3 +-
drivers/infiniband/sw/siw/siw_cm.c    |  4 +-
drivers/infiniband/sw/siw/siw_main.c  | 20 ---------
drivers/infiniband/sw/siw/siw_verbs.c | 60
+++++++++++++++++++++++++++
4 files changed, 64 insertions(+), 23 deletions(-)
drivers/infiniband/sw/siw/siw.h +2 -1
index dba4535494ab..ad4f078e4587 100644
@@ -240,7 +240,8 @@ enum siw_qp_flags {
 	SIW_RDMA_READ_ENABLED = (1 << 2),
 	SIW_SIGNAL_ALL_WR = (1 << 3),
 	SIW_MPA_CRC = (1 << 4),
-	SIW_QP_IN_DESTROY = (1 << 5)
+	SIW_QP_IN_DESTROY = (1 << 5),
+	SIW_QP_DRAINED_FINAL = (1 << 6)
 };
 
Quoted 7 lines
drivers/infiniband/sw/siw/siw_cm.c static int siw_proc_mpareply(struct siw_cep
Quoted 7 lines
drivers/infiniband/sw/siw/siw.h +0 -0
 enum siw_qp_attr_mask {
drivers/infiniband/sw/siw/siw_cm.c +0 -0
index 8c1931a57f4a..fb830622d32e 100644
@@ -857,7 +857,7 @@ static int siw_proc_mpareply(struct siw_cep
*cep)
Quoted 2 lines
Quoted 2 lines
drivers/infiniband/sw/siw/siw_cm.c +0 -0
 	memset(&qp_attrs, 0, sizeof(qp_attrs));
 
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/infiniband/sw/siw/siw_cm.c int siw_accept(struct iw_cm_id *id,
Quoted 7 lines
drivers/infiniband/sw/siw/siw_cm.c +1 -1
 	if (rep->params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.orq_size = cep->ord;
@@ -1675,7 +1675,7 @@ int siw_accept(struct iw_cm_id *id,
struct
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/infiniband/sw/siw/siw_cm.c +1 -1
 iw_cm_conn_param *params)
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.sk = cep->sock;
 	if (cep->mpa.hdr.params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 	qp_attrs.state = SIW_QP_STATE_RTS;
 
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/infiniband/sw/siw/siw_main.c static struct ib_qp
Quoted 7 lines
	siw_dbg_cep(cep, "[QP%u]: moving to rts\n", qp_id(qp));
drivers/infiniband/sw/siw/siw_main.c +0 -0
index 05a92f997f60..fb01407a310f 100644
@@ -248,24 +248,6 @@ static struct ib_qp
*siw_get_base_qp(struct
Quoted 43 lines
Quoted 43 lines
Quoted 18 lines
Quoted 18 lines
drivers/infiniband/sw/siw/siw_main.c +0 ~~-14~~
 ib_device *base_dev, int id)
 	return NULL;
 }
 
-static void siw_verbs_sq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
-	siw_sq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
-static void siw_verbs_rq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c static const struct ib_device_ops
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c +0 -4
-	siw_rq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
 static const struct ib_device_ops siw_device_ops = {
 	.owner = THIS_MODULE,
.uverbs_abi_ver = SIW_ABI_VERSION,
drivers/infiniband/sw/siw/siw_main.c +0 -0
@@ -284,8 +266,6 @@ static const struct ib_device_ops
siw_device_ops
Quoted 7 lines
Quoted 7 lines
drivers/infiniband/sw/siw/siw_main.c +0 -2
 = {
 	.destroy_cq = siw_destroy_cq,
 	.destroy_qp = siw_destroy_qp,
 	.destroy_srq = siw_destroy_srq,
-	.drain_rq = siw_verbs_rq_flush,
-	.drain_sq = siw_verbs_sq_flush,
 	.get_dma_mr = siw_get_dma_mr,
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_verbs_modify_qp(struct ib_qp
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c +0 -0
 	.get_port_immutable = siw_get_port_immutable,
.iw_accept = siw_accept,
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
index 869e02b69a01..5dd62946a649 100644
@@ -596,6 +596,13 @@ int siw_verbs_modify_qp(struct ib_qp
*base_qp,
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_verbs_modify_qp(struct ib_qp *base_qp, struct ib_qp_attr *attr,
Quoted 5 lines
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
 struct ib_qp_attr *attr,
 
 	rv = siw_qp_modify(qp, &new_attrs, siw_attr_mask);
 
+	/* QP state ERROR here ensures that all the SQ & RQ entries
got
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/infiniband/sw/siw/siw_verbs.c
Quoted 2 lines
drained
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * completely. And henceforth, no more entries will be added
to
Quoted 11 lines
Quoted 11 lines
the
Quoted 10 lines
drivers/infiniband/sw/siw/siw_verbs.c static int siw_copy_inline_sgl(const
Quoted 10 lines
CQ,
drivers/infiniband/sw/siw/siw_verbs.c +5 -0
+	 * exception is special drain CQEs via ib_drain_qp().
+	 */
+	if (qp->attrs.state == SIW_QP_STATE_ERROR)
+		qp->attrs.flags |= SIW_QP_DRAINED_FINAL;
+
	up_write(&qp->state_lock);
out:
	return rv;
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
@@ -687,6 +694,44 @@ static int siw_copy_inline_sgl(const
struct
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/infiniband/sw/siw/siw_verbs.c static int siw_copy_inline_sgl(const struct ib_send_wr *core_wr,
Quoted 6 lines
drivers/infiniband/sw/siw/siw_verbs.c +2 -0
 ib_send_wr *core_wr,
 	return bytes;
 }
 
+/* SQ final completion routine to support ib_drain_sp(). */
+int siw_sq_final_comp(struct siw_qp *qp, const struct
ib_send_wr
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/infiniband/sw/siw/siw_verbs.c +12 -0
 *wr,
+		      const struct ib_send_wr **bad_wr)
+{
+	struct siw_sqe sqe = {};
+	int rv = 0;
+
+	while (wr) {
+		sqe.id = wr->wr_id;
+		sqe.opcode = wr->opcode;
+		rv = siw_sqe_complete(qp, &sqe, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c static int siw_copy_inline_sgl(const struct ib_send_wr *core_wr,
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c +8 -0
+		}
+		wr = wr->next;
+	}
+	return rv;
+}
+
+/* RQ final completion routine to support ib_drain_rp(). */
+int siw_rq_final_comp(struct siw_qp *qp, const struct
ib_recv_wr
Quoted 26 lines
Quoted 26 lines
Quoted 13 lines
Quoted 13 lines
drivers/infiniband/sw/siw/siw_verbs.c +12 -0
 *wr,
+		      const struct ib_recv_wr **bad_wr)
+{
+	struct siw_rqe rqe = {};
+	int rv = 0;
+
+	while (wr) {
+		rqe.id = wr->wr_id;
+		rv = siw_rqe_complete(qp, &rqe, 0, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
+		}
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_post_send(struct ib_qp *base_qp,
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c +4 -0
+		wr = wr->next;
+	}
+	return rv;
+}
/*
* siw_post_send()
*
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
@@ -705,6 +750,15 @@ int siw_post_send(struct ib_qp *base_qp,
const
Quoted 4 lines
Quoted 4 lines
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
 struct ib_send_wr *wr,
 	unsigned long flags;
 	int rv = 0;
 
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
drivers/infiniband/sw/siw/siw_verbs.c int siw_post_send(struct ib_qp *base_qp, · line 754
Quoted 1 line
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	/* Currently there is no way to distinguish between special
drain
Quoted 3 lines
drivers/infiniband/sw/siw/siw_verbs.c
Quoted 3 lines
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * WRs and normal WRs(?), so we do FLUSH_ERR for all the WRs
that've
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * arrived in the ERROR/SIW_QP_DRAINED_FINAL state, assuming
we
Quoted 11 lines
Quoted 11 lines
get
Quoted 10 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_post_receive(struct ib_qp
Quoted 10 lines
drivers/infiniband/sw/siw/siw_verbs.c +6 -0
+	 * only special drain WRs in this state via ib_drain_sq().
+	 */
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_sq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}
/*
* Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
@@ -919,6 +973,12 @@ int siw_post_receive(struct ib_qp
*base_qp,
Quoted 21 lines
Quoted 20 lines
Quoted 18 lines
Quoted 17 lines
drivers/infiniband/sw/siw/siw_verbs.c +6 -0
 const struct ib_recv_wr *wr,
 		*bad_wr = wr;
 		return -EOPNOTSUPP; /* what else from errno.h? */
 	}
+
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_rq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}
+
 	/*
 	 * Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.
Signature · 1 line
2.23.0.rc0

Krishnamraju Eraparaju

author

2019-10-03 10:51 UTC #6 lore raw

Quoted 93 lines · Bernard Metzler

-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----
Quoted 91 lines
To: "Bernard Metzler" <[email protected]>
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 10/01/2019 07:45PM
Cc: "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "Potnuri Bharat Teja" <[email protected]>, "Nirranjan Kirubaharan" <[email protected]>
Subject: [EXTERNAL] Re: Re: [PATCH for-next] RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp
Quoted 8 lines · Bernard Metzler
-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----
Quoted 6 lines
To: "Bernard Metzler" <[email protected]>
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 10/01/2019 11:52AM
Cc: "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "Potnuri Bharat Teja" <[email protected]>, "Nirranjan Kirubaharan"
<[email protected]>
Quoted 6 lines
Quoted 6 lines
Subject: [EXTERNAL] Re: [PATCH for-next] RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp
Quoted 1 line · Bernard Metzler
-----"Krishnamraju Eraparaju" <[email protected]> wrote:
-----
Quoted 18 lines
Quoted 18 lines
Quoted 8 lines
Quoted 8 lines
To: [email protected], [email protected]
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 09/28/2019 12:16AM
Cc: [email protected], [email protected], [email protected], "Krishnamraju Eraparaju" <[email protected]>
Subject: [EXTERNAL] [PATCH for-next] RDMA/siw: fix SQ/RQ drain
logic
Quoted 9 lines
Quoted 6 lines
to support ib_drain_qp
The storage ULPs(iSER & NVMeOF) uses ib_drain_qp() to drain QP/CQ properly. But SIW is currently using it's own routines to drain SQ & RQ, which can't help ULPs to determine the last CQE. Failing to wait until last CQE causes touch after free issues:
Hi Krishna,
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Before reviewing, please let me fully understand what is going on here/why we need that patch.
Is this issue caused since the ulp expects the ib_drain_xx driver method to be blocking until all completions are reaped, and siw does not block?
Yes, SIW is currently using provider-specific drain_qp logic,
IE: siw_verbs_rq/sq_flush(), with this logic though all the SQ &
RQ
Quoted 2 lines
Quoted 2 lines
entries are flushed to CQ but ULPs cannot ensure when exactly the processing of all CQEs for those WRs, posted prior to
ib_drain_xx(),
Quoted 2 lines
Quoted 2 lines
got completed.
Due to this uncertainity, sometimes iSER/NVMeOF driver(assuming
all
Quoted 7 lines
Quoted 7 lines
the CQEs are processed) proceed to release resouces/destroy_qp, causing touch after free issues.
Quoted 4 lines
Is the ULP expected to call ib_drain_xx only if no other work is pending (SQ/RQ/CQ)? If not, shall all previous work be completed with FLUSH error?
In error cases(eg: link-down), I see iSER/NVMEOF drivers
performing
Quoted 10 lines
Quoted 4 lines
disconnect/drain_qp though there is some pending work to be processed.
This may be valid due to the ERROR.
And all that pending work gets completed with FLUSH error.
OK understood.
Dropping the siw private drain routines makes sense to me.
Quoted 8 lines
Otherwise, I think a cleaner solution is to just allow processing kernel ULPs send/recv WR's while the QP is already in ERROR state. In that case, we immediately complete with FLUSH error. We would avoid the extra state flag, and the extra check for that flag on the fast path.
I can send such patch tomorrow if you like.
Sure Bernard, it's good if we can avoiding extra check in fast path. The only condition for using ib_drain_cq(with special CQE) is: the special CQE should be the last CQE to be processed in the completion queue.

Quoted 8 lines

Quoted 5 lines
Also, we can't miss the special CQE due to down_read_trylock() failure
in post_send() and post_recv() routines.
Currenlty, special CQEs are being sent only once.
Well, at NVMeF target side. I have seen even two consecutive drain_sq() calls on the same QP, which does not hurt us.

Thanks for improvising the patch.
Question:
What if siw_post_send() got invoked with special drain WR, while down_write(QPstate_lock) was already held in another thread(somehow)? Then post_send will fail with ENOTCONN, and if this is an iSER initator, then iSER driver will continue freeing resouces before CQ is fully processed.
Not sure whether this can ever happen though...

Thanks,
Krishna.

Quoted 263 lines

I resend as v2 and have you as 'Signed-off' as well. I hope it is the right way to signal I partially re-wrote the patch.
Many thanks,
Bernard.
Quoted 253 lines
Thanks,
Krishna.
Quoted 27 lines
Many thanks,
Bernard.
Quoted 23 lines
Quoted 23 lines
Many thanks!
Bernard.
Quoted 15 lines
[  +0.001831] general protection fault: 0000 [#1] SMP PTI
[  +0.000002] Call Trace:
[  +0.000026]  ? __ib_process_cq+0x7a/0xc0 [ib_core]
[  +0.000008]  ? ib_poll_handler+0x2c/0x80 [ib_core]
[  +0.000005]  ? irq_poll_softirq+0xae/0x110
[  +0.000005]  ? __do_softirq+0xda/0x2a8
[  +0.000004]  ? run_ksoftirqd+0x26/0x40
[  +0.000005]  ? smpboot_thread_fn+0x10e/0x160
[  +0.000004]  ? kthread+0xf8/0x130
[  +0.000003]  ? sort_range+0x20/0x20
[  +0.000003]  ? kthread_bind+0x10/0x10
[  +0.000003]  ? ret_from_fork+0x35/0x40
Hence, changing the SQ & RQ drain logic to support current
IB/core
Quoted 39 lines
Quoted 39 lines
Quoted 1 line
Quoted 1 line
drain semantics, though this drain method does not naturally
aligns
Quoted 27 lines
Quoted 27 lines
to iWARP spec(where post_send/recv calls are not allowed in QP ERROR state). More on this was described in below commit:
commit 4fe7c2962e11 ("iw_cxgb4: refactor sq/rq drain logic")
Signed-off-by Krishnamraju Eraparaju <[email protected]>
---
drivers/infiniband/sw/siw/siw.h       |  3 +-
drivers/infiniband/sw/siw/siw_cm.c    |  4 +-
drivers/infiniband/sw/siw/siw_main.c  | 20 ---------
drivers/infiniband/sw/siw/siw_verbs.c | 60
+++++++++++++++++++++++++++
4 files changed, 64 insertions(+), 23 deletions(-)
drivers/infiniband/sw/siw/siw.h +2 -1
index dba4535494ab..ad4f078e4587 100644
@@ -240,7 +240,8 @@ enum siw_qp_flags {
 	SIW_RDMA_READ_ENABLED = (1 << 2),
 	SIW_SIGNAL_ALL_WR = (1 << 3),
 	SIW_MPA_CRC = (1 << 4),
-	SIW_QP_IN_DESTROY = (1 << 5)
+	SIW_QP_IN_DESTROY = (1 << 5),
+	SIW_QP_DRAINED_FINAL = (1 << 6)
 };
 
Quoted 7 lines
drivers/infiniband/sw/siw/siw_cm.c static int siw_proc_mpareply(struct siw_cep
Quoted 7 lines
drivers/infiniband/sw/siw/siw.h +0 -0
 enum siw_qp_attr_mask {
drivers/infiniband/sw/siw/siw_cm.c +0 -0
index 8c1931a57f4a..fb830622d32e 100644
@@ -857,7 +857,7 @@ static int siw_proc_mpareply(struct siw_cep
*cep)
Quoted 2 lines
Quoted 2 lines
drivers/infiniband/sw/siw/siw_cm.c +0 -0
 	memset(&qp_attrs, 0, sizeof(qp_attrs));
 
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/infiniband/sw/siw/siw_cm.c int siw_accept(struct iw_cm_id *id,
Quoted 7 lines
drivers/infiniband/sw/siw/siw_cm.c +1 -1
 	if (rep->params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.orq_size = cep->ord;
@@ -1675,7 +1675,7 @@ int siw_accept(struct iw_cm_id *id,
struct
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/infiniband/sw/siw/siw_cm.c +1 -1
 iw_cm_conn_param *params)
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.sk = cep->sock;
 	if (cep->mpa.hdr.params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 	qp_attrs.state = SIW_QP_STATE_RTS;
 
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/infiniband/sw/siw/siw_main.c static struct ib_qp
Quoted 7 lines
	siw_dbg_cep(cep, "[QP%u]: moving to rts\n", qp_id(qp));
drivers/infiniband/sw/siw/siw_main.c +0 -0
index 05a92f997f60..fb01407a310f 100644
@@ -248,24 +248,6 @@ static struct ib_qp
*siw_get_base_qp(struct
Quoted 43 lines
Quoted 43 lines
Quoted 18 lines
Quoted 18 lines
drivers/infiniband/sw/siw/siw_main.c +0 ~~-14~~
 ib_device *base_dev, int id)
 	return NULL;
 }
 
-static void siw_verbs_sq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
-	siw_sq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
-static void siw_verbs_rq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c static const struct ib_device_ops
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c +0 -4
-	siw_rq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
 static const struct ib_device_ops siw_device_ops = {
 	.owner = THIS_MODULE,
.uverbs_abi_ver = SIW_ABI_VERSION,
drivers/infiniband/sw/siw/siw_main.c +0 -0
@@ -284,8 +266,6 @@ static const struct ib_device_ops
siw_device_ops
Quoted 7 lines
Quoted 7 lines
drivers/infiniband/sw/siw/siw_main.c +0 -2
 = {
 	.destroy_cq = siw_destroy_cq,
 	.destroy_qp = siw_destroy_qp,
 	.destroy_srq = siw_destroy_srq,
-	.drain_rq = siw_verbs_rq_flush,
-	.drain_sq = siw_verbs_sq_flush,
 	.get_dma_mr = siw_get_dma_mr,
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_verbs_modify_qp(struct ib_qp
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c +0 -0
 	.get_port_immutable = siw_get_port_immutable,
.iw_accept = siw_accept,
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
index 869e02b69a01..5dd62946a649 100644
@@ -596,6 +596,13 @@ int siw_verbs_modify_qp(struct ib_qp
*base_qp,
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_verbs_modify_qp(struct ib_qp *base_qp, struct ib_qp_attr *attr,
Quoted 5 lines
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
 struct ib_qp_attr *attr,
 
 	rv = siw_qp_modify(qp, &new_attrs, siw_attr_mask);
 
+	/* QP state ERROR here ensures that all the SQ & RQ entries
got
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/infiniband/sw/siw/siw_verbs.c
Quoted 2 lines
drained
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * completely. And henceforth, no more entries will be added
to
Quoted 11 lines
Quoted 11 lines
the
Quoted 10 lines
drivers/infiniband/sw/siw/siw_verbs.c static int siw_copy_inline_sgl(const
Quoted 10 lines
CQ,
drivers/infiniband/sw/siw/siw_verbs.c +5 -0
+	 * exception is special drain CQEs via ib_drain_qp().
+	 */
+	if (qp->attrs.state == SIW_QP_STATE_ERROR)
+		qp->attrs.flags |= SIW_QP_DRAINED_FINAL;
+
	up_write(&qp->state_lock);
out:
	return rv;
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
@@ -687,6 +694,44 @@ static int siw_copy_inline_sgl(const
struct
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/infiniband/sw/siw/siw_verbs.c static int siw_copy_inline_sgl(const struct ib_send_wr *core_wr,
Quoted 6 lines
drivers/infiniband/sw/siw/siw_verbs.c +2 -0
 ib_send_wr *core_wr,
 	return bytes;
 }
 
+/* SQ final completion routine to support ib_drain_sp(). */
+int siw_sq_final_comp(struct siw_qp *qp, const struct
ib_send_wr
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/infiniband/sw/siw/siw_verbs.c +12 -0
 *wr,
+		      const struct ib_send_wr **bad_wr)
+{
+	struct siw_sqe sqe = {};
+	int rv = 0;
+
+	while (wr) {
+		sqe.id = wr->wr_id;
+		sqe.opcode = wr->opcode;
+		rv = siw_sqe_complete(qp, &sqe, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c static int siw_copy_inline_sgl(const struct ib_send_wr *core_wr,
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c +8 -0
+		}
+		wr = wr->next;
+	}
+	return rv;
+}
+
+/* RQ final completion routine to support ib_drain_rp(). */
+int siw_rq_final_comp(struct siw_qp *qp, const struct
ib_recv_wr
Quoted 26 lines
Quoted 26 lines
Quoted 13 lines
Quoted 13 lines
drivers/infiniband/sw/siw/siw_verbs.c +12 -0
 *wr,
+		      const struct ib_recv_wr **bad_wr)
+{
+	struct siw_rqe rqe = {};
+	int rv = 0;
+
+	while (wr) {
+		rqe.id = wr->wr_id;
+		rv = siw_rqe_complete(qp, &rqe, 0, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
+		}
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_post_send(struct ib_qp *base_qp,
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c +4 -0
+		wr = wr->next;
+	}
+	return rv;
+}
/*
* siw_post_send()
*
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
@@ -705,6 +750,15 @@ int siw_post_send(struct ib_qp *base_qp,
const
Quoted 4 lines
Quoted 4 lines
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
 struct ib_send_wr *wr,
 	unsigned long flags;
 	int rv = 0;
 
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
drivers/infiniband/sw/siw/siw_verbs.c int siw_post_send(struct ib_qp *base_qp, · line 754
Quoted 1 line
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	/* Currently there is no way to distinguish between special
drain
Quoted 3 lines
drivers/infiniband/sw/siw/siw_verbs.c
Quoted 3 lines
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * WRs and normal WRs(?), so we do FLUSH_ERR for all the WRs
that've
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * arrived in the ERROR/SIW_QP_DRAINED_FINAL state, assuming
we
Quoted 11 lines
Quoted 11 lines
get
Quoted 10 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_post_receive(struct ib_qp
Quoted 10 lines
drivers/infiniband/sw/siw/siw_verbs.c +6 -0
+	 * only special drain WRs in this state via ib_drain_sq().
+	 */
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_sq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}
/*
* Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
@@ -919,6 +973,12 @@ int siw_post_receive(struct ib_qp
*base_qp,
Quoted 21 lines
Quoted 20 lines
Quoted 18 lines
Quoted 17 lines
drivers/infiniband/sw/siw/siw_verbs.c +6 -0
 const struct ib_recv_wr *wr,
 		*bad_wr = wr;
 		return -EOPNOTSUPP; /* what else from errno.h? */
 	}
+
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_rq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}
+
 	/*
 	 * Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.
Signature · 1 line
2.23.0.rc0

Bernard Metzler

2019-10-03 14:03 UTC #7 lore raw

-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----

Quoted 125 lines

To: "Bernard Metzler" <[email protected]>
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 10/03/2019 12:51PM
Cc: "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "Potnuri Bharat Teja" <[email protected]>, "Nirranjan Kirubaharan" <[email protected]>
Subject: [EXTERNAL] Re: Re: Re: [PATCH for-next] RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp
Quoted 8 lines · Bernard Metzler
-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----
Quoted 6 lines
To: "Bernard Metzler" <[email protected]>
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 10/01/2019 07:45PM
Cc: "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "Potnuri Bharat Teja" <[email protected]>, "Nirranjan Kirubaharan"
<[email protected]>
Quoted 4 lines
Quoted 4 lines
Subject: [EXTERNAL] Re: Re: [PATCH for-next] RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp
On Tuesday, October 10/01/19, 2019 at 15:56:45 +0000, Bernard
Metzler
Quoted 2 lines
Quoted 2 lines
Quoted 1 line · wrote:
-----"Krishnamraju Eraparaju" <[email protected]> wrote:
-----
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
To: "Bernard Metzler" <[email protected]>
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 10/01/2019 11:52AM
Cc: "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "Potnuri Bharat Teja" <[email protected]>, "Nirranjan Kirubaharan"
<[email protected]>
Quoted 1 line
Quoted 1 line
Subject: [EXTERNAL] Re: [PATCH for-next] RDMA/siw: fix SQ/RQ
drain
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
logic to support ib_drain_qp
Quoted 1 line · Bernard Metzler
-----"Krishnamraju Eraparaju" <[email protected]> wrote:
-----
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
To: [email protected], [email protected]
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 09/28/2019 12:16AM
Cc: [email protected], [email protected], [email protected], "Krishnamraju Eraparaju" <[email protected]>
Subject: [EXTERNAL] [PATCH for-next] RDMA/siw: fix SQ/RQ
drain
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
logic
Quoted 4 lines
Quoted 4 lines
to support ib_drain_qp
The storage ULPs(iSER & NVMeOF) uses ib_drain_qp() to drain QP/CQ properly. But SIW is currently using it's own routines
to
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drain SQ & RQ, which can't help ULPs to determine the last
CQE.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Failing to wait until last CQE causes touch after free
issues:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Hi Krishna,
Before reviewing, please let me fully understand what is going on here/why we need that patch.
Is this issue caused since the ulp expects the ib_drain_xx driver method to be blocking until all completions are
reaped,
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
and siw does not block?
Yes, SIW is currently using provider-specific drain_qp logic,
IE: siw_verbs_rq/sq_flush(), with this logic though all the SQ
&
Quoted 2 lines
Quoted 2 lines
RQ
Quoted 1 line
Quoted 1 line
entries are flushed to CQ but ULPs cannot ensure when exactly
the
Quoted 30 lines
Quoted 30 lines
Quoted 1 line
Quoted 1 line
processing of all CQEs for those WRs, posted prior to
ib_drain_xx(),
Quoted 2 lines
Quoted 2 lines
got completed.
Due to this uncertainity, sometimes iSER/NVMeOF driver(assuming
all
Quoted 7 lines
Quoted 7 lines
the CQEs are processed) proceed to release resouces/destroy_qp, causing touch after free issues.
Quoted 4 lines
Is the ULP expected to call ib_drain_xx only if no other work is pending (SQ/RQ/CQ)? If not, shall all previous work be completed with FLUSH error?
In error cases(eg: link-down), I see iSER/NVMEOF drivers
performing
Quoted 17 lines
Quoted 4 lines
disconnect/drain_qp though there is some pending work to be processed.
This may be valid due to the ERROR.
And all that pending work gets completed with FLUSH error.
OK understood.
Dropping the siw private drain routines makes sense to me.
Otherwise, I think a cleaner solution is to just allow processing kernel ULPs send/recv WR's while the QP is already in ERROR state. In that case, we immediately complete with FLUSH error. We would avoid the extra state flag, and the extra check for that flag on the fast path.
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
I can send such patch tomorrow if you like.
Sure Bernard, it's good if we can avoiding extra check in fast
path.
Quoted 2 lines
Quoted 2 lines
The only condition for using ib_drain_cq(with special CQE) is: the special CQE should be the last CQE to be processed in the
completion
Quoted 10 lines
Quoted 7 lines
queue.
Also, we can't miss the special CQE due to down_read_trylock() failure
in post_send() and post_recv() routines.
Currenlty, special CQEs are being sent only once.
Well, at NVMeF target side. I have seen even two consecutive drain_sq() calls on the same QP, which does not hurt us.

Quoted 9 lines

Thanks for improvising the patch.
Question:
What if siw_post_send() got invoked with special drain WR, while down_write(QPstate_lock) was already held in another thread(somehow)? Then post_send will fail with ENOTCONN, and if this is an iSER initator,
then iSER driver will continue freeing resouces before CQ is fully processed.
Not sure whether this can ever happen though...

There are other reasons why the generic __ib_drain_sq() may fail. A CQ overflow is one such candidate. Failures are not handled by the ULP, since calling a void function.

At the other hand, we know that if we have reached ERROR state, the QP will never escape back to become full functional; ERROR is the QP's final state.

So we could do an extra check if we cannot get the state lock - if we are already in ERROR. And if yes, complete immediately there as well.

I can change the patch accordingly. Makes sense?

Thanks,
Bernard.

Quoted 276 lines

Thanks,
Krishna.
Quoted 68 lines
I resend as v2 and have you as 'Signed-off' as well. I hope it is the right way to signal I partially re-wrote the patch.
Many thanks,
Bernard.
Quoted 59 lines
Thanks,
Krishna.
Quoted 27 lines
Many thanks,
Bernard.
Quoted 23 lines
Quoted 23 lines
Many thanks!
Bernard.
Quoted 15 lines
[  +0.001831] general protection fault: 0000 [#1] SMP PTI
[  +0.000002] Call Trace:
[  +0.000026]  ? __ib_process_cq+0x7a/0xc0 [ib_core]
[  +0.000008]  ? ib_poll_handler+0x2c/0x80 [ib_core]
[  +0.000005]  ? irq_poll_softirq+0xae/0x110
[  +0.000005]  ? __do_softirq+0xda/0x2a8
[  +0.000004]  ? run_ksoftirqd+0x26/0x40
[  +0.000005]  ? smpboot_thread_fn+0x10e/0x160
[  +0.000004]  ? kthread+0xf8/0x130
[  +0.000003]  ? sort_range+0x20/0x20
[  +0.000003]  ? kthread_bind+0x10/0x10
[  +0.000003]  ? ret_from_fork+0x35/0x40
Hence, changing the SQ & RQ drain logic to support current
IB/core
Quoted 29 lines
Quoted 29 lines
Quoted 1 line
Quoted 1 line
drain semantics, though this drain method does not naturally
aligns
Quoted 27 lines
Quoted 27 lines
to iWARP spec(where post_send/recv calls are not allowed in QP ERROR state). More on this was described in below commit:
commit 4fe7c2962e11 ("iw_cxgb4: refactor sq/rq drain logic")
Signed-off-by Krishnamraju Eraparaju <[email protected]>
---
drivers/infiniband/sw/siw/siw.h       |  3 +-
drivers/infiniband/sw/siw/siw_cm.c    |  4 +-
drivers/infiniband/sw/siw/siw_main.c  | 20 ---------
drivers/infiniband/sw/siw/siw_verbs.c | 60
+++++++++++++++++++++++++++
4 files changed, 64 insertions(+), 23 deletions(-)
drivers/infiniband/sw/siw/siw.h +2 -1
index dba4535494ab..ad4f078e4587 100644
@@ -240,7 +240,8 @@ enum siw_qp_flags {
 	SIW_RDMA_READ_ENABLED = (1 << 2),
 	SIW_SIGNAL_ALL_WR = (1 << 3),
 	SIW_MPA_CRC = (1 << 4),
-	SIW_QP_IN_DESTROY = (1 << 5)
+	SIW_QP_IN_DESTROY = (1 << 5),
+	SIW_QP_DRAINED_FINAL = (1 << 6)
 };
 
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/infiniband/sw/siw/siw_cm.c static int siw_proc_mpareply(struct
Quoted 7 lines
drivers/infiniband/sw/siw/siw.h +0 -0
 enum siw_qp_attr_mask {
drivers/infiniband/sw/siw/siw_cm.c +0 -0
index 8c1931a57f4a..fb830622d32e 100644
@@ -857,7 +857,7 @@ static int siw_proc_mpareply(struct
siw_cep
Quoted 70 lines
Quoted 70 lines
Quoted 10 lines
Quoted 10 lines
*cep)
Quoted 9 lines
drivers/infiniband/sw/siw/siw_cm.c int siw_accept(struct iw_cm_id *id,
Quoted 9 lines
drivers/infiniband/sw/siw/siw_cm.c +1 -1
 	memset(&qp_attrs, 0, sizeof(qp_attrs));
 
 	if (rep->params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.orq_size = cep->ord;
@@ -1675,7 +1675,7 @@ int siw_accept(struct iw_cm_id *id,
struct
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/infiniband/sw/siw/siw_cm.c +1 -1
 iw_cm_conn_param *params)
 	qp_attrs.irq_size = cep->ird;
 	qp_attrs.sk = cep->sock;
 	if (cep->mpa.hdr.params.bits & MPA_RR_FLAG_CRC)
-		qp_attrs.flags = SIW_MPA_CRC;
+		qp_attrs.flags |= SIW_MPA_CRC;
 	qp_attrs.state = SIW_QP_STATE_RTS;
 
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/infiniband/sw/siw/siw_main.c static struct ib_qp
Quoted 7 lines
	siw_dbg_cep(cep, "[QP%u]: moving to rts\n", qp_id(qp));
drivers/infiniband/sw/siw/siw_main.c +0 -0
index 05a92f997f60..fb01407a310f 100644
@@ -248,24 +248,6 @@ static struct ib_qp
*siw_get_base_qp(struct
Quoted 43 lines
Quoted 43 lines
Quoted 18 lines
Quoted 18 lines
drivers/infiniband/sw/siw/siw_main.c +0 ~~-14~~
 ib_device *base_dev, int id)
 	return NULL;
 }
 
-static void siw_verbs_sq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
-	siw_sq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
-static void siw_verbs_rq_flush(struct ib_qp *base_qp)
-{
-	struct siw_qp *qp = to_siw_qp(base_qp);
-
-	down_write(&qp->state_lock);
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c static const struct ib_device_ops
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c +0 -4
-	siw_rq_flush(qp);
-	up_write(&qp->state_lock);
-}
-
 static const struct ib_device_ops siw_device_ops = {
 	.owner = THIS_MODULE,
.uverbs_abi_ver = SIW_ABI_VERSION,
drivers/infiniband/sw/siw/siw_main.c +0 -0
@@ -284,8 +266,6 @@ static const struct ib_device_ops
siw_device_ops
Quoted 7 lines
Quoted 7 lines
drivers/infiniband/sw/siw/siw_main.c +0 -2
 = {
 	.destroy_cq = siw_destroy_cq,
 	.destroy_qp = siw_destroy_qp,
 	.destroy_srq = siw_destroy_srq,
-	.drain_rq = siw_verbs_rq_flush,
-	.drain_sq = siw_verbs_sq_flush,
 	.get_dma_mr = siw_get_dma_mr,
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_verbs_modify_qp(struct ib_qp
Quoted 8 lines
drivers/infiniband/sw/siw/siw_main.c +0 -0
 	.get_port_immutable = siw_get_port_immutable,
.iw_accept = siw_accept,
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
index 869e02b69a01..5dd62946a649 100644
@@ -596,6 +596,13 @@ int siw_verbs_modify_qp(struct ib_qp
*base_qp,
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_verbs_modify_qp(struct ib_qp · line 600
Quoted 5 lines
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
 struct ib_qp_attr *attr,
 
 	rv = siw_qp_modify(qp, &new_attrs, siw_attr_mask);
 
+	/* QP state ERROR here ensures that all the SQ & RQ
entries
Quoted 3 lines
Quoted 3 lines
got
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/infiniband/sw/siw/siw_verbs.c
Quoted 2 lines
drained
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * completely. And henceforth, no more entries will be
added
Quoted 42 lines
Quoted 42 lines
to
Quoted 11 lines
Quoted 11 lines
the
Quoted 10 lines
drivers/infiniband/sw/siw/siw_verbs.c static int siw_copy_inline_sgl(const
Quoted 10 lines
CQ,
drivers/infiniband/sw/siw/siw_verbs.c +5 -0
+	 * exception is special drain CQEs via ib_drain_qp().
+	 */
+	if (qp->attrs.state == SIW_QP_STATE_ERROR)
+		qp->attrs.flags |= SIW_QP_DRAINED_FINAL;
+
	up_write(&qp->state_lock);
out:
	return rv;
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
@@ -687,6 +694,44 @@ static int siw_copy_inline_sgl(const
struct
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/infiniband/sw/siw/siw_verbs.c static int siw_copy_inline_sgl(const · line 699
Quoted 6 lines
drivers/infiniband/sw/siw/siw_verbs.c +2 -0
 ib_send_wr *core_wr,
 	return bytes;
 }
 
+/* SQ final completion routine to support ib_drain_sp(). */
+int siw_sq_final_comp(struct siw_qp *qp, const struct
ib_send_wr
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/infiniband/sw/siw/siw_verbs.c +12 -0
 *wr,
+		      const struct ib_send_wr **bad_wr)
+{
+	struct siw_sqe sqe = {};
+	int rv = 0;
+
+	while (wr) {
+		sqe.id = wr->wr_id;
+		sqe.opcode = wr->opcode;
+		rv = siw_sqe_complete(qp, &sqe, 0, SIW_WC_WR_FLUSH_ERR);
+		if (rv) {
+			*bad_wr = wr;
+			break;
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c static int siw_copy_inline_sgl(const · line 720
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c +8 -0
+		}
+		wr = wr->next;
+	}
+	return rv;
+}
+
+/* RQ final completion routine to support ib_drain_rp(). */
+int siw_rq_final_comp(struct siw_qp *qp, const struct
ib_recv_wr
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
drivers/infiniband/sw/siw/siw_verbs.c static int siw_copy_inline_sgl(const · line 729
Quoted 9 lines
drivers/infiniband/sw/siw/siw_verbs.c +8 -0
 *wr,
+		      const struct ib_recv_wr **bad_wr)
+{
+	struct siw_rqe rqe = {};
+	int rv = 0;
+
+	while (wr) {
+		rqe.id = wr->wr_id;
+		rv = siw_rqe_complete(qp, &rqe, 0, 0,
SIW_WC_WR_FLUSH_ERR);
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/infiniband/sw/siw/siw_verbs.c +4 -0
+		if (rv) {
+			*bad_wr = wr;
+			break;
+		}
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_post_send(struct ib_qp
Quoted 8 lines
drivers/infiniband/sw/siw/siw_verbs.c +4 -0
+		wr = wr->next;
+	}
+	return rv;
+}
/*
* siw_post_send()
*
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
@@ -705,6 +750,15 @@ int siw_post_send(struct ib_qp
*base_qp,
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
const
Quoted 5 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_post_send(struct ib_qp · line 754
Quoted 5 lines
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
 struct ib_send_wr *wr,
 	unsigned long flags;
 	int rv = 0;
 
+	/* Currently there is no way to distinguish between
special
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drain
Quoted 1 line
drivers/infiniband/sw/siw/siw_verbs.c int siw_post_send(struct ib_qp · line 755
Quoted 1 line
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * WRs and normal WRs(?), so we do FLUSH_ERR for all the
WRs
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/infiniband/sw/siw/siw_verbs.c
Quoted 2 lines
that've
drivers/infiniband/sw/siw/siw_verbs.c +1 -0
+	 * arrived in the ERROR/SIW_QP_DRAINED_FINAL state,
assuming
Quoted 37 lines
Quoted 36 lines
we
Quoted 11 lines
Quoted 11 lines
get
Quoted 10 lines
drivers/infiniband/sw/siw/siw_verbs.c int siw_post_receive(struct ib_qp
Quoted 10 lines
drivers/infiniband/sw/siw/siw_verbs.c +6 -0
+	 * only special drain WRs in this state via ib_drain_sq().
+	 */
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_sq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}
/*
* Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.
drivers/infiniband/sw/siw/siw_verbs.c +0 -0
@@ -919,6 +973,12 @@ int siw_post_receive(struct ib_qp
*base_qp,
Quoted 21 lines
Quoted 20 lines
Quoted 18 lines
Quoted 17 lines
drivers/infiniband/sw/siw/siw_verbs.c +6 -0
 const struct ib_recv_wr *wr,
 		*bad_wr = wr;
 		return -EOPNOTSUPP; /* what else from errno.h? */
 	}
+
+	if (qp->attrs.flags & SIW_QP_DRAINED_FINAL) {
+		rv = siw_rq_final_comp(qp, wr, bad_wr);
+		return rv;
+	}
+
 	/*
 	 * Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.
Signature · 1 line
2.23.0.rc0

Krishnamraju Eraparaju

author

2019-10-04 04:57 UTC #8 lore raw

Quoted 4 lines · Bernard Metzler

There are other reasons why the generic __ib_drain_sq() may fail. A CQ overflow is one such candidate. Failures are not handled by the ULP, since calling a void function.

The function description of ib_drain_qp() says:
* The caller must:
*
* ensure there is room in the CQ(s), SQ, and RQ for drain work requests
* and completions.
*
* allocate the CQs using ib_alloc_cq().
*
* ensure that there are no other contexts that are posting WRs
* concurrently.
* Otherwise the drain is not guaranteed.
*/

So, it looks like ULP has to check for available CQs before calling ib_drain_xx().

Quoted 10 lines

At the other hand, we know that if we have reached ERROR state, the QP will never escape back to become full functional; ERROR is the QP's final state.
So we could do an extra check if we cannot get the state lock - if we are already in ERROR. And if yes, complete immediately there as well.
I can change the patch accordingly. Makes sense?

Yes, I think addressing this would make the fix complete.

Thanks,
Krishna.

Bernard Metzler

2019-10-04 13:48 UTC #9 lore raw

-----"Krishnamraju Eraparaju" <[email protected]> wrote: -----

Quoted 15 lines

To: "Bernard Metzler" <[email protected]>
From: "Krishnamraju Eraparaju" <[email protected]>
Date: 10/04/2019 06:57AM
Cc: "[email protected]" <[email protected]>, "[email protected]" <[email protected]>, "Potnuri Bharat Teja" <[email protected]>, "Nirranjan Kirubaharan" <[email protected]>
Subject: [EXTERNAL] Re: Re: Re: Re: [PATCH for-next] RDMA/siw: fix SQ/RQ drain logic to support ib_drain_qp
Quoted 4 lines · Bernard Metzler
There are other reasons why the generic __ib_drain_sq() may fail. A CQ overflow is one such candidate. Failures are not handled by the ULP, since calling a void function.

Quoted 14 lines

The function description of ib_drain_qp() says:
* The caller must:
*
* ensure there is room in the CQ(s), SQ, and RQ for drain work requests
* and completions.
*
* allocate the CQs using ib_alloc_cq().
*
* ensure that there are no other contexts that are posting WRs
* concurrently.
* Otherwise the drain is not guaranteed.
*/

Yes, I know. Imho, this guarantee falls into the same category as assuming a sane ULP which will not try to change the QP state at the same time while calling for sq_drain. A CQ overflow would be a miscalculation of its size by the ULP. A drain_sq in parallel with a modify_qp call just another misbehaving..? Anyway, I think you are right, let's handle explicitly all cases we can handle.

Quoted 8 lines

So, it looks like ULP has to check for available CQs before calling ib_drain_xx().
Quoted 5 lines
At the other hand, we know that if we have reached ERROR state, the QP will never escape back to become full functional; ERROR is the QP's final state.

Quoted 7 lines

Quoted 5 lines
So we could do an extra check if we cannot get the state lock - if we are already in ERROR. And if yes, complete immediately there as well.
I can change the patch accordingly. Makes sense?
Yes, I think addressing this would make the fix complete.

sent.

I'll be away whole next week from tonight on.

Thanks
Bernard.

Quoted 4 lines

Thanks,
Krishna.