From: "Bernard Metzler" <BMT@zurich.ibm.com>
To: linux-rdma@vger.kernel.org
Cc: bharat@chelsio.com, jgg@ziepe.ca, nirranjan@chelsio.com,
krishna2@chelsio.com, bvanassche@acm.org, leon@kernel.org,
"Bernard Metzler" <BMT@zurich.ibm.com>
Subject: Re: [[PATCH v3 for-next]] RDMA/siw: Fix SQ/RQ drain logic
Date: Fri, 18 Oct 2019 13:22:11 +0000 [thread overview]
Message-ID: <OF30CE2323.3FB3125D-ON00258497.00497128-00258497.00497133@notes.na.collabserv.com> (raw)
In-Reply-To: <20191004125356.20673-1-bmt@zurich.ibm.com>
-----"Bernard Metzler" <bmt@zurich.ibm.com> wrote: -----
>To: linux-rdma@vger.kernel.org
>From: "Bernard Metzler" <bmt@zurich.ibm.com>
>Date: 10/04/2019 02:54PM
>Cc: bharat@chelsio.com, jgg@ziepe.ca, nirranjan@chelsio.com,
>krishna2@chelsio.com, bvanassche@acm.org, leon@kernel.org, "Bernard
>Metzler" <bmt@zurich.ibm.com>
>Subject: [[PATCH v3 for-next]] RDMA/siw: Fix SQ/RQ drain logic
>
>Storage ULPs (e.g. iSER & NVMeOF) use ib_drain_qp() to
>drain QP/CQ. Current SIW's own drain routines do not properly
>wait until all SQ/RQ elements are completed and reaped
>from the CQ. This may cause touch after free issues.
>New logic relies on generic __ib_drain_sq()/__ib_drain_rq()
>posting a final work request, which SIW immediately flushes
>to CQ.
>
>Fixes: 303ae1cdfdf7 ("rdma/siw: application interface")
>Signed-off-by: Krishnamraju Eraparaju <krishna2@chelsio.com>
>Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com>
>---
>v2 -> v3:
>- Handle ib_drain_sq()/ib_drain_rq() calls when QP's
> state is currently locked.
>
>v1 -> v2:
>- Accept SQ and RQ work requests, if QP is in ERROR
> state. In that case, immediately flush WR's to CQ.
> This already provides needed functionality to
> support ib_drain_sq()/ib_drain_rq() without extra
> state checking in the fast path.
>
> drivers/infiniband/sw/siw/siw_main.c | 20 ----
> drivers/infiniband/sw/siw/siw_verbs.c | 144
>++++++++++++++++++++++----
> 2 files changed, 122 insertions(+), 42 deletions(-)
>
Is there any more comment on that one? I think it has been
sufficiently discussed and it is well understood, and it fixes
the issue at hand.
Thanks very much,
Bernard.
>diff --git a/drivers/infiniband/sw/siw/siw_main.c
>b/drivers/infiniband/sw/siw/siw_main.c
>index 05a92f997f60..fb01407a310f 100644
>--- a/drivers/infiniband/sw/siw/siw_main.c
>+++ b/drivers/infiniband/sw/siw/siw_main.c
>@@ -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,
>diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
>b/drivers/infiniband/sw/siw/siw_verbs.c
>index 869e02b69a01..c0574ddc98fa 100644
>--- a/drivers/infiniband/sw/siw/siw_verbs.c
>+++ b/drivers/infiniband/sw/siw/siw_verbs.c
>@@ -687,6 +687,47 @@ static int siw_copy_inline_sgl(const struct
>ib_send_wr *core_wr,
> return bytes;
> }
>
>+/* Complete SQ WR's without processing */
>+static int siw_sq_flush_wr(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) {
>+ if (bad_wr)
>+ *bad_wr = wr;
>+ break;
>+ }
>+ wr = wr->next;
>+ }
>+ return rv;
>+}
>+
>+/* Complete RQ WR's without processing */
>+static int siw_rq_flush_wr(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) {
>+ if (bad_wr)
>+ *bad_wr = wr;
>+ break;
>+ }
>+ wr = wr->next;
>+ }
>+ return rv;
>+}
>+
> /*
> * siw_post_send()
> *
>@@ -705,26 +746,54 @@ int siw_post_send(struct ib_qp *base_qp, const
>struct ib_send_wr *wr,
> unsigned long flags;
> int rv = 0;
>
>+ if (wr && !qp->kernel_verbs) {
>+ siw_dbg_qp(qp, "wr must be empty for user mapped sq\n");
>+ *bad_wr = wr;
>+ return -EINVAL;
>+ }
>+
> /*
> * Try to acquire QP state lock. Must be non-blocking
> * to accommodate kernel clients needs.
> */
> if (!down_read_trylock(&qp->state_lock)) {
>- *bad_wr = wr;
>- siw_dbg_qp(qp, "QP locked, state %d\n", qp->attrs.state);
>- return -ENOTCONN;
>+ if (qp->attrs.state == SIW_QP_STATE_ERROR) {
>+ /*
>+ * ERROR state is final, so we can be sure
>+ * this state will not change as long as the QP
>+ * exists.
>+ *
>+ * This handles an ib_drain_sq() call with
>+ * a concurrent request to set the QP state
>+ * to ERROR.
>+ */
>+ rv = siw_sq_flush_wr(qp, wr, bad_wr);
>+ } else {
>+ siw_dbg_qp(qp, "QP locked, state %d\n",
>+ qp->attrs.state);
>+ *bad_wr = wr;
>+ rv = -ENOTCONN;
>+ }
>+ return rv;
> }
> if (unlikely(qp->attrs.state != SIW_QP_STATE_RTS)) {
>+ if (qp->attrs.state == SIW_QP_STATE_ERROR) {
>+ /*
>+ * Immediately flush this WR to CQ, if QP
>+ * is in ERROR state. SQ is guaranteed to
>+ * be empty, so WR complets in-order.
>+ *
>+ * Typically triggered by ib_drain_sq().
>+ */
>+ rv = siw_sq_flush_wr(qp, wr, bad_wr);
>+ } else {
>+ siw_dbg_qp(qp, "QP out of state %d\n",
>+ qp->attrs.state);
>+ *bad_wr = wr;
>+ rv = -ENOTCONN;
>+ }
> up_read(&qp->state_lock);
>- *bad_wr = wr;
>- siw_dbg_qp(qp, "QP out of state %d\n", qp->attrs.state);
>- return -ENOTCONN;
>- }
>- if (wr && !qp->kernel_verbs) {
>- siw_dbg_qp(qp, "wr must be empty for user mapped sq\n");
>- up_read(&qp->state_lock);
>- *bad_wr = wr;
>- return -EINVAL;
>+ return rv;
> }
> spin_lock_irqsave(&qp->sq_lock, flags);
>
>@@ -919,24 +988,55 @@ 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? */
> }
>- /*
>- * Try to acquire QP state lock. Must be non-blocking
>- * to accommodate kernel clients needs.
>- */
>- if (!down_read_trylock(&qp->state_lock)) {
>- *bad_wr = wr;
>- return -ENOTCONN;
>- }
> if (!qp->kernel_verbs) {
> siw_dbg_qp(qp, "no kernel post_recv for user mapped sq\n");
> up_read(&qp->state_lock);
> *bad_wr = wr;
> return -EINVAL;
> }
>+
>+ /*
>+ * Try to acquire QP state lock. Must be non-blocking
>+ * to accommodate kernel clients needs.
>+ */
>+ if (!down_read_trylock(&qp->state_lock)) {
>+ if (qp->attrs.state == SIW_QP_STATE_ERROR) {
>+ /*
>+ * ERROR state is final, so we can be sure
>+ * this state will not change as long as the QP
>+ * exists.
>+ *
>+ * This handles an ib_drain_rq() call with
>+ * a concurrent request to set the QP state
>+ * to ERROR.
>+ */
>+ rv = siw_rq_flush_wr(qp, wr, bad_wr);
>+ } else {
>+ siw_dbg_qp(qp, "QP locked, state %d\n",
>+ qp->attrs.state);
>+ *bad_wr = wr;
>+ rv = -ENOTCONN;
>+ }
>+ return rv;
>+ }
> if (qp->attrs.state > SIW_QP_STATE_RTS) {
>+ if (qp->attrs.state == SIW_QP_STATE_ERROR) {
>+ /*
>+ * Immediately flush this WR to CQ, if QP
>+ * is in ERROR state. RQ is guaranteed to
>+ * be empty, so WR complets in-order.
>+ *
>+ * Typically triggered by ib_drain_rq().
>+ */
>+ rv = siw_rq_flush_wr(qp, wr, bad_wr);
>+ } else {
>+ siw_dbg_qp(qp, "QP out of state %d\n",
>+ qp->attrs.state);
>+ *bad_wr = wr;
>+ rv = -ENOTCONN;
>+ }
> up_read(&qp->state_lock);
>- *bad_wr = wr;
>- return -EINVAL;
>+ return rv;
> }
> /*
> * Serialize potentially multiple producers.
>--
>2.17.2
>
>
next prev parent reply other threads:[~2019-10-18 13:22 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-04 12:53 [[PATCH v3 for-next]] RDMA/siw: Fix SQ/RQ drain logic Bernard Metzler
2019-10-18 13:22 ` Bernard Metzler [this message]
2019-10-22 17:10 ` Jason Gunthorpe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=OF30CE2323.3FB3125D-ON00258497.00497128-00258497.00497133@notes.na.collabserv.com \
--to=bmt@zurich.ibm.com \
--cc=bharat@chelsio.com \
--cc=bvanassche@acm.org \
--cc=jgg@ziepe.ca \
--cc=krishna2@chelsio.com \
--cc=leon@kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=nirranjan@chelsio.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).