[PATCH v2 0/3] new ib_drain_qp() API

[PATCH v2 0/3] new ib_drain_qp() API

[Steve Wise]

This series creates a new helper API for draining a queue pair.  It is a
rework of an original patch created by Christoph Hellwig as part of the CQ
API rework and was dropped to be resubmitted by me with iw_cxgb4 support.

Original thread: http://www.spinics.net/lists/linux-rdma/msg30296.html

Changes since v1:

- added comments to the ib_drain_qp() function header specifying the
consumer requirements

- in __ib_drain_qp(), if the ib_post_send() fails, still wait for the
recv wr to drain since we already posted it.

- CC the SRP maintainer, bart.vanassche-XdAiOPVOjtvowKkBSvOlow@public.gmane.org

---

Steve Wise (3):
  IB: new common API for draining a queue pair
  iw_cxgb4: add drain_qp function
  IB/srp: use ib_drain_qp()

 drivers/infiniband/core/verbs.c        | 77 ++++++++++++++++++++++++++++++++++
 drivers/infiniband/hw/cxgb4/cq.c       |  6 ++-
 drivers/infiniband/hw/cxgb4/iw_cxgb4.h |  2 +
 drivers/infiniband/hw/cxgb4/provider.c |  1 +
 drivers/infiniband/hw/cxgb4/qp.c       |  8 ++++
 drivers/infiniband/ulp/srp/ib_srp.c    | 45 ++++----------------
 include/rdma/ib_verbs.h                |  2 +
 7 files changed, 102 insertions(+), 39 deletions(-)

-- 
2.7.0

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

[PATCH 2/3] iw_cxgb4: add drain_qp function

[Steve Wise]

Add a completion object, named qp_drained, to the c4iw_qp struct.
This completion object is signaled when the last CQE is drained from
the CQs for the QP.

Add c4iw_dain_qp() to block until qp_drained is completed.

Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---
 drivers/infiniband/hw/cxgb4/cq.c       | 6 +++++-
 drivers/infiniband/hw/cxgb4/iw_cxgb4.h | 2 ++
 drivers/infiniband/hw/cxgb4/provider.c | 1 +
 drivers/infiniband/hw/cxgb4/qp.c       | 8 ++++++++
 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c
index cf21df4..6fdcf78 100644
--- a/drivers/infiniband/hw/cxgb4/cq.c
+++ b/drivers/infiniband/hw/cxgb4/cq.c
@@ -815,8 +815,12 @@ static int c4iw_poll_cq_one(struct c4iw_cq *chp, struct ib_wc *wc)
 		}
 	}
 out:
-	if (wq)
+	if (wq) {
+		if (unlikely(qhp->attr.state != C4IW_QP_STATE_RTS &&
+			     t4_sq_empty(wq) && t4_rq_empty(wq)))
+			complete(&qhp->qp_drained);
 		spin_unlock(&qhp->lock);
+	}
 	return ret;
 }
 
diff --git a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
index fb2de75..fdb9d9a 100644
--- a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
+++ b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
@@ -476,6 +476,7 @@ struct c4iw_qp {
 	wait_queue_head_t wait;
 	struct timer_list timer;
 	int sq_sig_all;
+	struct completion qp_drained;
 };
 
 static inline struct c4iw_qp *to_c4iw_qp(struct ib_qp *ibqp)
@@ -1016,6 +1017,7 @@ extern int c4iw_wr_log;
 extern int db_fc_threshold;
 extern int db_coalescing_threshold;
 extern int use_dsgl;
+void c4iw_drain_qp(struct ib_qp *qp);
 
 
 #endif
diff --git a/drivers/infiniband/hw/cxgb4/provider.c b/drivers/infiniband/hw/cxgb4/provider.c
index ec04272..0ab942f 100644
--- a/drivers/infiniband/hw/cxgb4/provider.c
+++ b/drivers/infiniband/hw/cxgb4/provider.c
@@ -564,6 +564,7 @@ int c4iw_register_device(struct c4iw_dev *dev)
 	dev->ibdev.get_protocol_stats = c4iw_get_mib;
 	dev->ibdev.uverbs_abi_ver = C4IW_UVERBS_ABI_VERSION;
 	dev->ibdev.get_port_immutable = c4iw_port_immutable;
+	dev->ibdev.drain_qp = c4iw_drain_qp;
 
 	dev->ibdev.iwcm = kmalloc(sizeof(struct iw_cm_verbs), GFP_KERNEL);
 	if (!dev->ibdev.iwcm)
diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
index e99345e..2e70c01 100644
--- a/drivers/infiniband/hw/cxgb4/qp.c
+++ b/drivers/infiniband/hw/cxgb4/qp.c
@@ -1697,6 +1697,7 @@ struct ib_qp *c4iw_create_qp(struct ib_pd *pd, struct ib_qp_init_attr *attrs,
 	qhp->attr.max_ird = 0;
 	qhp->sq_sig_all = attrs->sq_sig_type == IB_SIGNAL_ALL_WR;
 	spin_lock_init(&qhp->lock);
+	init_completion(&qhp->qp_drained);
 	mutex_init(&qhp->mutex);
 	init_waitqueue_head(&qhp->wait);
 	atomic_set(&qhp->refcnt, 1);
@@ -1888,3 +1889,10 @@ int c4iw_ib_query_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
 	init_attr->sq_sig_type = qhp->sq_sig_all ? IB_SIGNAL_ALL_WR : 0;
 	return 0;
 }
+
+void c4iw_drain_qp(struct ib_qp *ibqp)
+{
+	struct c4iw_qp *qp = to_c4iw_qp(ibqp);
+
+	wait_for_completion(&qp->qp_drained);
+}
-- 
2.7.0

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

[PATCH 3/3] IB/srp: use ib_drain_qp()

[Steve Wise]

Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 45 ++++++-------------------------------
 1 file changed, 7 insertions(+), 38 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 03022f6..95670ae 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -446,49 +446,17 @@ static struct srp_fr_pool *srp_alloc_fr_pool(struct srp_target_port *target)
 				  dev->max_pages_per_mr);
 }
 
-static void srp_drain_done(struct ib_cq *cq, struct ib_wc *wc)
-{
-	struct srp_rdma_ch *ch = cq->cq_context;
-
-	complete(&ch->done);
-}
-
-static struct ib_cqe srp_drain_cqe = {
-	.done		= srp_drain_done,
-};
-
 /**
  * srp_destroy_qp() - destroy an RDMA queue pair
  * @ch: SRP RDMA channel.
  *
- * Change a queue pair into the error state and wait until all receive
- * completions have been processed before destroying it. This avoids that
- * the receive completion handler can access the queue pair while it is
+ * Drain the qp before destroying it.  This avoids that the receive
+ * completion handler can access the queue pair while it is
  * being destroyed.
  */
 static void srp_destroy_qp(struct srp_rdma_ch *ch)
 {
-	static struct ib_qp_attr attr = { .qp_state = IB_QPS_ERR };
-	static struct ib_recv_wr wr = { 0 };
-	struct ib_recv_wr *bad_wr;
-	int ret;
-
-	wr.wr_cqe = &srp_drain_cqe;
-	/* Destroying a QP and reusing ch->done is only safe if not connected */
-	WARN_ON_ONCE(ch->connected);
-
-	ret = ib_modify_qp(ch->qp, &attr, IB_QP_STATE);
-	WARN_ONCE(ret, "ib_cm_init_qp_attr() returned %d\n", ret);
-	if (ret)
-		goto out;
-
-	init_completion(&ch->done);
-	ret = ib_post_recv(ch->qp, &wr, &bad_wr);
-	WARN_ONCE(ret, "ib_post_recv() returned %d\n", ret);
-	if (ret == 0)
-		wait_for_completion(&ch->done);
-
-out:
+	ib_drain_qp(ch->qp);
 	ib_destroy_qp(ch->qp);
 }
 
@@ -508,7 +476,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 	if (!init_attr)
 		return -ENOMEM;
 
-	/* queue_size + 1 for ib_drain_qp */
+	/* queue_size + 1 for ib_drain_qp() */
 	recv_cq = ib_alloc_cq(dev->dev, ch, target->queue_size + 1,
 				ch->comp_vector, IB_POLL_SOFTIRQ);
 	if (IS_ERR(recv_cq)) {
@@ -516,7 +484,8 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 		goto err;
 	}
 
-	send_cq = ib_alloc_cq(dev->dev, ch, m * target->queue_size,
+	/* queue_size + 1 for ib_drain_qp() */
+	send_cq = ib_alloc_cq(dev->dev, ch, m * target->queue_size + 1,
 				ch->comp_vector, IB_POLL_DIRECT);
 	if (IS_ERR(send_cq)) {
 		ret = PTR_ERR(send_cq);
@@ -524,7 +493,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 	}
 
 	init_attr->event_handler       = srp_qp_event;
-	init_attr->cap.max_send_wr     = m * target->queue_size;
+	init_attr->cap.max_send_wr     = m * target->queue_size + 1;
 	init_attr->cap.max_recv_wr     = target->queue_size + 1;
 	init_attr->cap.max_recv_sge    = 1;
 	init_attr->cap.max_send_sge    = 1;
-- 
2.7.0

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

[PATCH 1/3] IB: new common API for draining a queue pair

[Steve Wise]

From: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+p6NamaJ0bNTAL8bYrjMMd8@public.gmane.org>

Add provider-specific drain_qp function for providers needing special
drain logic.

Add static function __ib_drain_qp() which posts noop WRs to the RQ and
SQ and blocks until their completions are processed.  This ensures the
applications completions have all been processed.

Add API function ib_drain_qp() which calls the provider-specific drain
if it exists or __ib_drain_qp().

Reviewed-by: Chuck Lever <chuck.lever-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---
 drivers/infiniband/core/verbs.c | 77 +++++++++++++++++++++++++++++++++++++++++
 include/rdma/ib_verbs.h         |  2 ++
 2 files changed, 79 insertions(+)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 5af6d02..98ddcb4 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1657,3 +1657,80 @@ next_page:
 	return i;
 }
 EXPORT_SYMBOL(ib_sg_to_pages);
+
+struct ib_drain_cqe {
+	struct ib_cqe cqe;
+	struct completion done;
+};
+
+static void ib_drain_qp_done(struct ib_cq *cq, struct ib_wc *wc)
+{
+	struct ib_drain_cqe *cqe = container_of(wc->wr_cqe, struct ib_drain_cqe,
+						cqe);
+
+	complete(&cqe->done);
+}
+
+/*
+ * Post a WR and block until its completion is reaped for both the RQ and SQ.
+ */
+static void __ib_drain_qp(struct ib_qp *qp)
+{
+	struct ib_qp_attr attr = { .qp_state = IB_QPS_ERR };
+	struct ib_drain_cqe rdrain, sdrain;
+	struct ib_recv_wr rwr = {}, *bad_rwr;
+	struct ib_send_wr swr = {}, *bad_swr;
+	int ret;
+
+	rwr.wr_cqe = &rdrain.cqe;
+	rdrain.cqe.done = ib_drain_qp_done;
+	init_completion(&rdrain.done);
+
+	swr.wr_cqe = &sdrain.cqe;
+	sdrain.cqe.done = ib_drain_qp_done;
+	init_completion(&sdrain.done);
+
+	ret = ib_modify_qp(qp, &attr, IB_QP_STATE);
+	if (ret) {
+		WARN_ONCE(ret, "failed to drain QP: %d\n", ret);
+		return;
+	}
+
+	ret = ib_post_recv(qp, &rwr, &bad_rwr);
+	if (ret) {
+		WARN_ONCE(ret, "failed to drain recv queue: %d\n", ret);
+		return;
+	}
+
+	ret = ib_post_send(qp, &swr, &bad_swr);
+	if (ret) {
+		WARN_ONCE(ret, "failed to drain send queue: %d\n", ret);
+		goto err_drain_rq;
+	}
+
+	wait_for_completion(&sdrain.done);
+err_drain_rq:
+	wait_for_completion(&rdrain.done);
+}
+
+/**
+ * ib_drain_qp() - Block until all CQEs have been consumed by the
+ *		   application.
+ * @qp:            queue pair to drain
+ *
+ * If the device has a provider-specific drain function, then
+ * call that.  Otherwise call the generic drain function
+ * __ib_drain_qp().
+ *
+ * The consumer must ensure there is room* in the CQ, SQ, and RQ
+ * for a drain work request.  Also the consumer must allocate the
+ * CQ using ib_alloc_cq() and thus not directly polling the CQ.
+ */
+void ib_drain_qp(struct ib_qp *qp)
+{
+	if (qp->device->drain_qp)
+		qp->device->drain_qp(qp);
+	else
+		__ib_drain_qp(qp);
+}
+EXPORT_SYMBOL(ib_drain_qp);
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 284b00c..d8533ab 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1846,6 +1846,7 @@ struct ib_device {
 	int			   (*check_mr_status)(struct ib_mr *mr, u32 check_mask,
 						      struct ib_mr_status *mr_status);
 	void			   (*disassociate_ucontext)(struct ib_ucontext *ibcontext);
+	void			   (*drain_qp)(struct ib_qp *qp);
 
 	struct ib_dma_mapping_ops   *dma_ops;
 
@@ -3094,4 +3095,5 @@ int ib_sg_to_pages(struct ib_mr *mr,
 		   int sg_nents,
 		   int (*set_page)(struct ib_mr *, u64));
 
+void ib_drain_qp(struct ib_qp *qp);
 #endif /* IB_VERBS_H */
-- 
2.7.0

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

RE: [PATCH v2 0/3] new ib_drain_qp() API

[Steve Wise]

> 
> This series creates a new helper API for draining a queue pair.  It is a
> rework of an original patch created by Christoph Hellwig as part of the CQ
> API rework and was dropped to be resubmitted by me with iw_cxgb4 support.

I finally got SRP running to test this series and it isn't working. :(  Anyway, there will be v3 posted once I fix it.

Steve.


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

RE: [PATCH v2 0/3] new ib_drain_qp() API

[Steve Wise]

> > This series creates a new helper API for draining a queue pair.  It is a
> > rework of an original patch created by Christoph Hellwig as part of the CQ
> > API rework and was dropped to be resubmitted by me with iw_cxgb4 support.
> 
> I finally got SRP running to test this series and it isn't working. :(  Anyway, there will be v3 posted once I fix it.
> 
> Steve.

The problem is SRP creates its SQ CQ with IB_POLL_DIRECT context, so the CQ is never armed for interrupts, and SRP polls the CQ
directly.  So the drain logic won't work.  

I propose ib_drain_qp() takes a new parameter that tells it which queues to drain.  Then the SRP code will only drain the RQ (which
is what it is doing prior to this series).

Thoughts?

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

Re: [PATCH v2 0/3] new ib_drain_qp() API

[Bart Van Assche]

On 02/09/2016 12:50 PM, Steve Wise wrote:
>>> This series creates a new helper API for draining a queue pair.  It is a
>>> rework of an original patch created by Christoph Hellwig as part of the CQ
>>> API rework and was dropped to be resubmitted by me with iw_cxgb4 support.
>>
>> I finally got SRP running to test this series and it isn't working. :(  Anyway, there will be v3 posted once I fix it.
>>
>> Steve.
>
> The problem is SRP creates its SQ CQ with IB_POLL_DIRECT context, so the CQ is never armed for interrupts, and SRP polls the CQ
> directly.  So the drain logic won't work.
>
> I propose ib_drain_qp() takes a new parameter that tells it which queues to drain.  Then the SRP code will only drain the RQ (which
> is what it is doing prior to this series).

Hello Steve,

How about creating three functions - one that drains the receive queue, 
one that drains the send queue and a third function that drains both ? 
The latter function then can call the two former functions. And since 
only one of these three functions will have a user in your patch series 
(the function that drains the RQ), how about only introducing only that 
function now and to wait with introducing the two other functions until 
these have a user ?

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

RE: [PATCH v2 0/3] new ib_drain_qp() API

[Steve Wise]

> > The problem is SRP creates its SQ CQ with IB_POLL_DIRECT context, so the CQ is never armed for interrupts, and SRP polls the CQ
> > directly.  So the drain logic won't work.
> >
> > I propose ib_drain_qp() takes a new parameter that tells it which queues to drain.  Then the SRP code will only drain the RQ
(which
> > is what it is doing prior to this series).
> 
> Hello Steve,
> 
> How about creating three functions - one that drains the receive queue,
> one that drains the send queue and a third function that drains both ?
> The latter function then can call the two former functions. And since
> only one of these three functions will have a user in your patch series
> (the function that drains the RQ), how about only introducing only that
> function now and to wait with introducing the two other functions until
> these have a user ?

That sounds reasonable.  Simpler too perhaps.  We'll see if anyone else has an opinion.

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

Re: [PATCH v2 0/3] new ib_drain_qp() API

[Chuck Lever]

> On Feb 9, 2016, at 4:03 PM, Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> On 02/09/2016 12:50 PM, Steve Wise wrote:
>>>> This series creates a new helper API for draining a queue pair.  It is a
>>>> rework of an original patch created by Christoph Hellwig as part of the CQ
>>>> API rework and was dropped to be resubmitted by me with iw_cxgb4 support.
>>> 
>>> I finally got SRP running to test this series and it isn't working. :(  Anyway, there will be v3 posted once I fix it.
>>> 
>>> Steve.
>> 
>> The problem is SRP creates its SQ CQ with IB_POLL_DIRECT context, so the CQ is never armed for interrupts, and SRP polls the CQ
>> directly.  So the drain logic won't work.
>> 
>> I propose ib_drain_qp() takes a new parameter that tells it which queues to drain.  Then the SRP code will only drain the RQ (which
>> is what it is doing prior to this series).
> 
> Hello Steve,
> 
> How about creating three functions - one that drains the receive queue, one that drains the send queue and a third function that drains both ? The latter function then can call the two former functions. And since only one of these three functions will have a user in your patch series (the function that drains the RQ), how about only introducing only that function now and to wait with introducing the two other functions until these have a user ?

I would like xprtrdma to use the function that drains both.
Is that one of the functions that is being left out?

--
Chuck Lever




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

RE: [PATCH v2 0/3] new ib_drain_qp() API

[Steve Wise]

> > Hello Steve,
> >
> > How about creating three functions - one that drains the receive queue, one that drains the send queue and a third function that
drains
> both ? The latter function then can call the two former functions. And since only one of these three functions will have a user in
your patch
> series (the function that drains the RQ), how about only introducing only that function now and to wait with introducing the two
other
> functions until these have a user ?
> 
> I would like xprtrdma to use the function that drains both.
> Is that one of the functions that is being left out?
> 

That is Bart's proposal.  Perhaps we could add an xprtrdma patch to the series?  Then we'll have a user for flushing both.  Is the
change trivial?

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

Re: [PATCH v2 0/3] new ib_drain_qp() API

[Chuck Lever]

> On Feb 9, 2016, at 4:20 PM, Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org> wrote:
> 
>>> Hello Steve,
>>> 
>>> How about creating three functions - one that drains the receive queue, one that drains the send queue and a third function that
> drains
>> both ? The latter function then can call the two former functions. And since only one of these three functions will have a user in
> your patch
>> series (the function that drains the RQ), how about only introducing only that function now and to wait with introducing the two
> other
>> functions until these have a user ?
>> 
>> I would like xprtrdma to use the function that drains both.
>> Is that one of the functions that is being left out?
>> 
> 
> That is Bart's proposal.  Perhaps we could add an xprtrdma patch to the series?  Then we'll have a user for flushing both.  Is the
> change trivial?

Basically it will replace rpcrdma_flush_cqs(), so trivial in
a code change sense.

But xprtrdma does not use the new CQ API yet. I have patches
in test for 4.6 to do that conversion.


--
Chuck Lever




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

RE: [PATCH v2 0/3] new ib_drain_qp() API

[Steve Wise]

> > Hello Steve,
> >
> > How about creating three functions - one that drains the receive queue,
> > one that drains the send queue and a third function that drains both ?
> > The latter function then can call the two former functions. And since
> > only one of these three functions will have a user in your patch series
> > (the function that drains the RQ), how about only introducing only that
> > function now and to wait with introducing the two other functions until
> > these have a user ?
> 
> That sounds reasonable.  Simpler too perhaps.  We'll see if anyone else
> has an opinion.

Another option is for ib_drain_qp() to just skip queues with IB_POLL_DIRECT CQ processing.

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

Re: [PATCH v2 0/3] new ib_drain_qp() API

[Chuck Lever]

> On Feb 9, 2016, at 4:58 PM, Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org> wrote:
> 
> 
>>> Hello Steve,
>>> 
>>> How about creating three functions - one that drains the receive queue,
>>> one that drains the send queue and a third function that drains both ?
>>> The latter function then can call the two former functions. And since
>>> only one of these three functions will have a user in your patch series
>>> (the function that drains the RQ), how about only introducing only that
>>> function now and to wait with introducing the two other functions until
>>> these have a user ?
>> 
>> That sounds reasonable.  Simpler too perhaps.  We'll see if anyone else
>> has an opinion.
> 
> Another option is for ib_drain_qp() to just skip queues with IB_POLL_DIRECT CQ processing.

Does ib_drain_qp() recognize when one CQ is new-style, and one
is old school? How about QPs with a single CQ? Since there is
a requirement that each CQ is allocated via ib_alloc_cq, it
would be good to assert that is true in ib_drain_qp() before
you prepare and post the blank WRs.

Or if you prefer not to handle it automatically, you could add
a flag parameter to ib_drain_qp (IB_DRAIN_RQ, IB_DRAIN_SQ, or
IB_DRAIN_BOTH).


--
Chuck Lever




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

Re: [PATCH v2 0/3] new ib_drain_qp() API

[Sagi Grimberg]

>>> Hello Steve,
>>>
>>> How about creating three functions - one that drains the receive queue,
>>> one that drains the send queue and a third function that drains both ?
>>> The latter function then can call the two former functions. And since
>>> only one of these three functions will have a user in your patch series
>>> (the function that drains the RQ), how about only introducing only that
>>> function now and to wait with introducing the two other functions until
>>> these have a user ?
>>
>> That sounds reasonable.  Simpler too perhaps.  We'll see if anyone else
>> has an opinion.
>
> Another option is for ib_drain_qp() to just skip queues with IB_POLL_DIRECT CQ processing.

I'd rather not skip silently anything. ib_drain_qp() semantics is that
it drains all the pending posts on the queue-pair (i.e. both send and
receive queues).

We can split to send and receive, but the fact that srp uses direct
polling CQ is not sufficient for that, most if not all will benefit from
both.

I'd suggest to look at the CQ context and act accordingly, something
like:

	if (cq->poll_ctx == IB_POLL_DIRECT) {
		while (!wait_for_completion_timeout(&sdrain.done,
				mescs_to_jiffies(100))
			ib_process_cq_direct(cq, 1024)
	} else {
		wait_for_completion(&sdrain.done);
	}

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

Re: [PATCH v2 0/3] new ib_drain_qp() API

[Bart Van Assche]

On 02/10/16 02:33, Sagi Grimberg wrote:
>
>>>> Hello Steve,
>>>>
>>>> How about creating three functions - one that drains the receive queue,
>>>> one that drains the send queue and a third function that drains both ?
>>>> The latter function then can call the two former functions. And since
>>>> only one of these three functions will have a user in your patch series
>>>> (the function that drains the RQ), how about only introducing only that
>>>> function now and to wait with introducing the two other functions until
>>>> these have a user ?
>>>
>>> That sounds reasonable.  Simpler too perhaps.  We'll see if anyone else
>>> has an opinion.
>>
>> Another option is for ib_drain_qp() to just skip queues with IB_POLL_DIRECT CQ processing.
>
> I'd rather not skip silently anything. ib_drain_qp() semantics is that
> it drains all the pending posts on the queue-pair (i.e. both send and
> receive queues).
>
> We can split to send and receive, but the fact that srp uses direct
> polling CQ is not sufficient for that, most if not all will benefit from
> both.
>
> I'd suggest to look at the CQ context and act accordingly, something
> like:
>
> 	if (cq->poll_ctx == IB_POLL_DIRECT) {
> 		while (!wait_for_completion_timeout(&sdrain.done,
> 				mescs_to_jiffies(100))
> 			ib_process_cq_direct(cq, 1024)
> 	} else {
> 		wait_for_completion(&sdrain.done);
> 	}
>
> Thoughts?

Hello Sagi,

The above code will only work as expected if the caller won't call 
ib_process_cq_direct() for the same queue from another context. This 
needs to be documented clearly and all drivers in which a call to that 
code is introduced have to be verified to see whether that assumption holds.

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

Re: [PATCH v2 0/3] new ib_drain_qp() API

[Sagi Grimberg]

>> I'd suggest to look at the CQ context and act accordingly, something
>> like:
>>
>>     if (cq->poll_ctx == IB_POLL_DIRECT) {
>>         while (!wait_for_completion_timeout(&sdrain.done,
>>                 mescs_to_jiffies(100))
>>             ib_process_cq_direct(cq, 1024)
>>     } else {
>>         wait_for_completion(&sdrain.done);
>>     }
>>
>> Thoughts?
>
> Hello Sagi,
>
> The above code will only work as expected if the caller won't call
> ib_process_cq_direct() for the same queue from another context.

Why? ib_poll_cq is safe from multiple contexts. Is it because the
completion structures are on stack? because if so we can easily move
them to the cq...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] new ib_drain_qp() API

[Bart Van Assche]

On 02/11/16 02:46, Sagi Grimberg wrote:
>>> I'd suggest to look at the CQ context and act accordingly, something
>>> like:
>>>
>>>      if (cq->poll_ctx == IB_POLL_DIRECT) {
>>>          while (!wait_for_completion_timeout(&sdrain.done,
>>>                  mescs_to_jiffies(100))
>>>              ib_process_cq_direct(cq, 1024)
>>>      } else {
>>>          wait_for_completion(&sdrain.done);
>>>      }
>>>
>>> Thoughts?
>>
>> Hello Sagi,
>>
>> The above code will only work as expected if the caller won't call
>> ib_process_cq_direct() for the same queue from another context.
>
> Why? ib_poll_cq is safe from multiple contexts. Is it because the
> completion structures are on stack? because if so we can easily move
> them to the cq...

Hello Sagi,

The srp_send_done() function accesses the ch->free_tx list without 
locking. This is safe because all existing 
ib_process_cq_direct(ch->send_cq, ...) calls occur while holding 
ch->lock. The approach suggested above breaks that assumption.

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

Re: [PATCH v2 0/3] new ib_drain_qp() API

[Sagi Grimberg]

> Hello Sagi,
>
> The srp_send_done() function accesses the ch->free_tx list without
> locking. This is safe because all existing
> ib_process_cq_direct(ch->send_cq, ...) calls occur while holding
> ch->lock. The approach suggested above breaks that assumption.

But srp_send_done won't get invoked for the ib_drain_qp post send
because ->done doesn't point to it, Am I missing something?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] new ib_drain_qp() API

[Bart Van Assche]

On 02/11/2016 08:11 AM, Sagi Grimberg wrote:
>> The srp_send_done() function accesses the ch->free_tx list without
>> locking. This is safe because all existing
>> ib_process_cq_direct(ch->send_cq, ...) calls occur while holding
>> ch->lock. The approach suggested above breaks that assumption.
>
> But srp_send_done won't get invoked for the ib_drain_qp post send
> because ->done doesn't point to it, Am I missing something?

Hello Sagi,

If it would be known that the send completion queue is empty then it 
wouldn't be needed to drain the send queue. And if it is not known 
whether or not the send queue is empty then for the SRP driver it is 
possible that calling ib_process_cq_direct() from inside the drain 
function will trigger a call to srp_send_done().

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

RE: [PATCH v2 0/3] new ib_drain_qp() API

[Steve Wise]

> >>> Hello Steve,
> >>>
> >>> How about creating three functions - one that drains the receive queue,
> >>> one that drains the send queue and a third function that drains both ?
> >>> The latter function then can call the two former functions. And since
> >>> only one of these three functions will have a user in your patch series
> >>> (the function that drains the RQ), how about only introducing only that
> >>> function now and to wait with introducing the two other functions until
> >>> these have a user ?
> >>
> >> That sounds reasonable.  Simpler too perhaps.  We'll see if anyone else
> >> has an opinion.
> >
> > Another option is for ib_drain_qp() to just skip queues with IB_POLL_DIRECT CQ processing.
> 
> I'd rather not skip silently anything. ib_drain_qp() semantics is that
> it drains all the pending posts on the queue-pair (i.e. both send and
> receive queues).
> 
> We can split to send and receive, but the fact that srp uses direct
> polling CQ is not sufficient for that, most if not all will benefit from
> both.
>

If we split it into ib_drain_rq(), ib_drain_sq(), and ib_drain_qp(), then srp would only call ib_drain_rq().  Others can call
ib_drain_qp() if they want both SQ and RQ drained.
 
> I'd suggest to look at the CQ context and act accordingly, something
> like:
> 
> 	if (cq->poll_ctx == IB_POLL_DIRECT) {
> 		while (!wait_for_completion_timeout(&sdrain.done,
> 				mescs_to_jiffies(100))
> 			ib_process_cq_direct(cq, 1024)
> 	} else {
> 		wait_for_completion(&sdrain.done);
> 	}
> 
> Thoughts?

I don't like the forced 100ms block.  You could call ib_process_cq_direct() first and check the return code, then block if needed.
But is there utility in providing drain for IB_POLL_DIRECT users?  

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

RE: [PATCH v2 0/3] new ib_drain_qp() API

[Steve Wise]

> > I'd suggest to look at the CQ context and act accordingly, something
> > like:
> >
> > 	if (cq->poll_ctx == IB_POLL_DIRECT) {
> > 		while (!wait_for_completion_timeout(&sdrain.done,
> > 				mescs_to_jiffies(100))
> > 			ib_process_cq_direct(cq, 1024)
> > 	} else {
> > 		wait_for_completion(&sdrain.done);
> > 	}
> >
> > Thoughts?
> 
> I don't like the forced 100ms block.  You could call ib_process_cq_direct() first and check the return code, then block if needed.

My bad:  there is no forced 100ms block.  If the completion is ready, then it will not wait...



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

Re: [PATCH v2 0/3] new ib_drain_qp() API

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On 02/10/2016 10:27 AM, Steve Wise wrote:
>>>>> Hello Steve,
>>>>>
>>>>> How about creating three functions - one that drains the receive queue,
>>>>> one that drains the send queue and a third function that drains both ?
>>>>> The latter function then can call the two former functions. And since
>>>>> only one of these three functions will have a user in your patch series
>>>>> (the function that drains the RQ), how about only introducing only that
>>>>> function now and to wait with introducing the two other functions until
>>>>> these have a user ?
>>>>
>>>> That sounds reasonable.  Simpler too perhaps.  We'll see if anyone else
>>>> has an opinion.
>>>
>>> Another option is for ib_drain_qp() to just skip queues with IB_POLL_DIRECT CQ processing.
>>
>> I'd rather not skip silently anything. ib_drain_qp() semantics is that
>> it drains all the pending posts on the queue-pair (i.e. both send and
>> receive queues).
>>
>> We can split to send and receive, but the fact that srp uses direct
>> polling CQ is not sufficient for that, most if not all will benefit from
>> both.
>>
> 
> If we split it into ib_drain_rq(), ib_drain_sq(), and ib_drain_qp(), then srp would only call ib_drain_rq().  Others can call
> ib_drain_qp() if they want both SQ and RQ drained.

I'm in favor of this kind of flexibility at the API level.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH v2 0/3] new ib_drain_qp() API

[Sagi Grimberg]

>>
>> If we split it into ib_drain_rq(), ib_drain_sq(), and ib_drain_qp(), then srp would only call ib_drain_rq().  Others can call
>> ib_drain_qp() if they want both SQ and RQ drained.
>
> I'm in favor of this kind of flexibility at the API level.

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

RE: [PATCH v2 0/3] new ib_drain_qp() API

[Steve Wise]

> >>
> >> If we split it into ib_drain_rq(), ib_drain_sq(), and ib_drain_qp(), then srp would only call ib_drain_rq().  Others can call
> >> ib_drain_qp() if they want both SQ and RQ drained.
> >
> > I'm in favor of this kind of flexibility at the API level.
> 
> I'm fine with having three flavors...

Me too.  I'll respin. 

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