All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] best practice for setting IB_CQ_REPORT_MISSED_EVENTS
@ 2010-03-31 17:57 Ralph Campbell
       [not found] ` <20100331175742.24522.17850.stgit-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Ralph Campbell @ 2010-03-31 17:57 UTC (permalink / raw)
  To: Roland Dreier; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

The following patches update ISER, SRP and core to use the
IB_CQ_REPORT_MISSED_EVENTS when calling ib_req_notify_cq()
similar to the patch I sent out for RDS.

Since the QLogic HCAs use a separate thread to generate the CQ callback,
it is important to make sure it isn't delayed more than necessary.
--
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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3] IB/core: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks
       [not found] ` <20100331175742.24522.17850.stgit-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
@ 2010-03-31 17:57   ` Ralph Campbell
       [not found]     ` <20100331175747.24522.88472.stgit-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
  2010-03-31 17:57   ` [PATCH 2/3] IB/srp: " Ralph Campbell
  2010-03-31 17:57   ` [PATCH 3/3] IB/iser: " Ralph Campbell
  2 siblings, 1 reply; 14+ messages in thread
From: Ralph Campbell @ 2010-03-31 17:57 UTC (permalink / raw)
  To: Roland Dreier; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

ib_req_notify_cq(IB_CQ_NEXT_COMP) is not guaranteed to generate
a callback for the next completion entered since there is a race
between arming the callback and another CQE being added to the queue.
The IB_CQ_REPORT_MISSED_EVENTS flag was added to detect this
race and allow the verbs consumer to call ib_poll_cq() and
ib_req_notify_cq() again to avoid delays in processing the CQE.

Signed-off-by: Ralph Campbell <ralph.campbell-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
---

 drivers/infiniband/core/mad.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index e351b15..54c413e 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -2226,10 +2226,11 @@ static void ib_mad_completion_handler(struct work_struct *work)
 {
 	struct ib_mad_port_private *port_priv;
 	struct ib_wc wc;
+	int ret;
 
 	port_priv = container_of(work, struct ib_mad_port_private, work);
-	ib_req_notify_cq(port_priv->cq, IB_CQ_NEXT_COMP);
 
+again:
 	while (ib_poll_cq(port_priv->cq, 1, &wc) == 1) {
 		if (wc.status == IB_WC_SUCCESS) {
 			switch (wc.opcode) {
@@ -2246,6 +2247,10 @@ static void ib_mad_completion_handler(struct work_struct *work)
 		} else
 			mad_error_handler(port_priv, &wc);
 	}
+	ret = ib_req_notify_cq(port_priv->cq, IB_CQ_NEXT_COMP |
+					      IB_CQ_REPORT_MISSED_EVENTS);
+	if (ret > 0)
+		goto again;
 }
 
 static void cancel_mads(struct ib_mad_agent_private *mad_agent_priv)

--
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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/3] IB/srp: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks
       [not found] ` <20100331175742.24522.17850.stgit-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
  2010-03-31 17:57   ` [PATCH 1/3] IB/core: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks Ralph Campbell
@ 2010-03-31 17:57   ` Ralph Campbell
       [not found]     ` <20100331175752.24522.66519.stgit-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
  2010-03-31 17:57   ` [PATCH 3/3] IB/iser: " Ralph Campbell
  2 siblings, 1 reply; 14+ messages in thread
From: Ralph Campbell @ 2010-03-31 17:57 UTC (permalink / raw)
  To: Roland Dreier; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

ib_req_notify_cq(IB_CQ_NEXT_COMP) is not guaranteed to generate
a callback for the next completion entered since there is a race
between arming the callback and another CQE being added to the queue.
The IB_CQ_REPORT_MISSED_EVENTS flag was added to detect this
race and allow the verbs consumer to call ib_poll_cq() and
ib_req_notify_cq() again to avoid delays in processing the CQE.

Signed-off-by: Ralph Campbell <ralph.campbell-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
---

 drivers/infiniband/ulp/srp/ib_srp.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index ed3f9eb..b0376f1 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -921,8 +921,9 @@ static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)
 {
 	struct srp_target_port *target = target_ptr;
 	struct ib_wc wc;
+	int ret;
 
-	ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
+again:
 	while (ib_poll_cq(cq, 1, &wc) > 0) {
 		if (wc.status) {
 			shost_printk(KERN_ERR, target->scsi_host,
@@ -934,6 +935,10 @@ static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)
 
 		srp_handle_recv(target, &wc);
 	}
+	ret = ib_req_notify_cq(cq, IB_CQ_NEXT_COMP |
+				   IB_CQ_REPORT_MISSED_EVENTS);
+	if (ret > 0)
+		goto again;
 }
 
 static void srp_send_completion(struct ib_cq *cq, void *target_ptr)

--
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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/3] IB/iser: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks
       [not found] ` <20100331175742.24522.17850.stgit-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
  2010-03-31 17:57   ` [PATCH 1/3] IB/core: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks Ralph Campbell
  2010-03-31 17:57   ` [PATCH 2/3] IB/srp: " Ralph Campbell
@ 2010-03-31 17:57   ` Ralph Campbell
  2 siblings, 0 replies; 14+ messages in thread
From: Ralph Campbell @ 2010-03-31 17:57 UTC (permalink / raw)
  To: Roland Dreier; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

ib_req_notify_cq(IB_CQ_NEXT_COMP) is not guaranteed to generate
a callback for the next completion entered since there is a race
between arming the callback and another CQE being added to the queue.
The IB_CQ_REPORT_MISSED_EVENTS flag was added to detect this
race and allow the verbs consumer to call ib_poll_cq() and
ib_req_notify_cq() again to avoid delays in processing the CQE.

Signed-off-by: Ralph Campbell <ralph.campbell-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
---

 drivers/infiniband/ulp/iser/iser_verbs.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 308d17b..8cfd1c0 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -783,8 +783,11 @@ static void iser_cq_tasklet_fn(unsigned long data)
 	 unsigned long	     xfer_len;
 	struct iser_conn *ib_conn;
 	int completed_tx, completed_rx;
+	int ret;
+
 	completed_tx = completed_rx = 0;
 
+again:
 	while (ib_poll_cq(cq, 1, &wc) == 1) {
 		desc	 = (struct iser_rx_desc *) (unsigned long) wc.wr_id;
 		BUG_ON(desc == NULL);
@@ -807,9 +810,10 @@ static void iser_cq_tasklet_fn(unsigned long data)
 		if (!(completed_rx & 63))
 			completed_tx += iser_drain_tx_cq(device);
 	}
-	/* #warning "it is assumed here that arming CQ only once its empty" *
-	 * " would not cause interrupts to be missed"                       */
-	ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
+	ret = ib_req_notify_cq(cq, IB_CQ_NEXT_COMP |
+				   IB_CQ_REPORT_MISSED_EVENTS);
+	if (ret > 0)
+		goto again;
 
 	completed_tx += iser_drain_tx_cq(device);
 	iser_dbg("got %d rx %d tx completions\n", completed_rx, completed_tx);

--
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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* RE: [PATCH 1/3] IB/core: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks
       [not found]     ` <20100331175747.24522.88472.stgit-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
@ 2010-03-31 18:07       ` Sean Hefty
  2010-03-31 18:17       ` Roland Dreier
  1 sibling, 0 replies; 14+ messages in thread
From: Sean Hefty @ 2010-03-31 18:07 UTC (permalink / raw)
  To: 'Ralph Campbell', Roland Dreier; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

Can this be structure as:

do {

> 	while (ib_poll_cq(port_priv->cq, 1, &wc) == 1) {
> 		if (wc.status == IB_WC_SUCCESS) {
> 			switch (wc.opcode) {
>@@ -2246,6 +2247,10 @@ static void ib_mad_completion_handler(struct work_struct
>*work)
> 		} else
> 			mad_error_handler(port_priv, &wc);
> 	}
>+	ret = ib_req_notify_cq(port_priv->cq, IB_CQ_NEXT_COMP |
>+					      IB_CQ_REPORT_MISSED_EVENTS);

} while (ret > 0);

to avoid using a goto for loop control?  Although, there's no reason to rearm
the CQ multiple times.

--
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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] IB/srp: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks
       [not found]     ` <20100331175752.24522.66519.stgit-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
@ 2010-03-31 18:11       ` Bart Van Assche
       [not found]         ` <y2ie2e108261003311111uec1ec00g746ba0ffe3d98799-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2010-03-31 18:11 UTC (permalink / raw)
  To: Ralph Campbell; +Cc: Roland Dreier, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Wed, Mar 31, 2010 at 7:57 PM, Ralph Campbell
<ralph.campbell-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org> wrote:
>
> ib_req_notify_cq(IB_CQ_NEXT_COMP) is not guaranteed to generate
> a callback for the next completion entered since there is a race
> between arming the callback and another CQE being added to the queue.
> The IB_CQ_REPORT_MISSED_EVENTS flag was added to detect this
> race and allow the verbs consumer to call ib_poll_cq() and
> ib_req_notify_cq() again to avoid delays in processing the CQE.
>
> Signed-off-by: Ralph Campbell <ralph.campbell-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
> ---
>
>  drivers/infiniband/ulp/srp/ib_srp.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index ed3f9eb..b0376f1 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -921,8 +921,9 @@ static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)
>  {
>        struct srp_target_port *target = target_ptr;
>        struct ib_wc wc;
> +       int ret;
>
> -       ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
> +again:
>        while (ib_poll_cq(cq, 1, &wc) > 0) {
>                if (wc.status) {
>                        shost_printk(KERN_ERR, target->scsi_host,
> @@ -934,6 +935,10 @@ static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)
>
>                srp_handle_recv(target, &wc);
>        }
> +       ret = ib_req_notify_cq(cq, IB_CQ_NEXT_COMP |
> +                                  IB_CQ_REPORT_MISSED_EVENTS);
> +       if (ret > 0)
> +               goto again;
>  }
>
>  static void srp_send_completion(struct ib_cq *cq, void *target_ptr)

NAK.

The SRP initiator works fine without this patch, and this patch slows
down the SRP initiator.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] IB/core: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks
       [not found]     ` <20100331175747.24522.88472.stgit-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
  2010-03-31 18:07       ` Sean Hefty
@ 2010-03-31 18:17       ` Roland Dreier
       [not found]         ` <ada4ojwjqz7.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
  1 sibling, 1 reply; 14+ messages in thread
From: Roland Dreier @ 2010-03-31 18:17 UTC (permalink / raw)
  To: Ralph Campbell; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

 > ib_req_notify_cq(IB_CQ_NEXT_COMP) is not guaranteed to generate
 > a callback for the next completion entered since there is a race
 > between arming the callback and another CQE being added to the queue.
 > The IB_CQ_REPORT_MISSED_EVENTS flag was added to detect this
 > race and allow the verbs consumer to call ib_poll_cq() and
 > ib_req_notify_cq() again to avoid delays in processing the CQE.

I'm not sure I understand the race you're fixing here... the existing
code does the rearm before polling:

 > +	int ret;
 >  
 >  	port_priv = container_of(work, struct ib_mad_port_private, work);
 > -	ib_req_notify_cq(port_priv->cq, IB_CQ_NEXT_COMP);
 >  
 > +again:
 >  	while (ib_poll_cq(port_priv->cq, 1, &wc) == 1) {
 >  		if (wc.status == IB_WC_SUCCESS) {
 >  			switch (wc.opcode) {
 > @@ -2246,6 +2247,10 @@ static void ib_mad_completion_handler(struct work_struct *work)
 >  		} else
 >  			mad_error_handler(port_priv, &wc);
 >  	}
 > +	ret = ib_req_notify_cq(port_priv->cq, IB_CQ_NEXT_COMP |
 > +					      IB_CQ_REPORT_MISSED_EVENTS);
 > +	if (ret > 0)
 > +		goto again;

The only issue with the existing code is that it may trigger extra
events that will find the CQ empty when polling.

So this may be a valid optimization but I don't see it fixing any missed
events.  Am I missing something?

Also for all these fixes, I think you only want to rearm the CQ once and
go back and poll if you get a missed events warning; the next time the
CQ is empty, then you know another event will happen.

 - R.
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] IB/srp: use IB_CQ_REPORT_MISSED_EVENTS to avoid  missed CQ callbacks
       [not found]         ` <y2ie2e108261003311111uec1ec00g746ba0ffe3d98799-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-03-31 18:18           ` Roland Dreier
       [not found]             ` <adazl1oiccb.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Roland Dreier @ 2010-03-31 18:18 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Ralph Campbell, linux-rdma-u79uwXL29TY76Z2rM5mHXA

 > The SRP initiator works fine without this patch, and this patch slows
 > down the SRP initiator.

I do agree that there are no missed callbacks without this patch, but I
don't see how it would slow things down... it seems to me it would avoid
some CQ event callbacks, especially for HCAs where
IB_CQ_REPORT_MISSED_EVENTS never returns >0 for missed events.

 - R.
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] IB/srp: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks
       [not found]             ` <adazl1oiccb.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
@ 2010-03-31 18:36               ` Bart Van Assche
       [not found]                 ` <u2ze2e108261003311136l4bcf1627n9ba4d3414167eef3-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2010-03-31 18:36 UTC (permalink / raw)
  To: Roland Dreier; +Cc: Ralph Campbell, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Wed, Mar 31, 2010 at 8:18 PM, Roland Dreier <rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> wrote:
>
>  > The SRP initiator works fine without this patch, and this patch slows
>  > down the SRP initiator.
>
> I do agree that there are no missed callbacks without this patch, but I
> don't see how it would slow things down... it seems to me it would avoid
> some CQ event callbacks, especially for HCAs where
> IB_CQ_REPORT_MISSED_EVENTS never returns >0 for missed events.

Processing the flag IB_CQ_REPORT_MISSED_EVENTS causes several IB
drivers to lock and unlock a spinlock. Full-speed SRP I/O can cause
this operation to be invoked more than 100.000 times a second, so the
flag IB_CQ_REPORT_MISSED_EVENTS will have a small but measurable
impact on SRP I/O performance.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] IB/srp: use IB_CQ_REPORT_MISSED_EVENTS to avoid  missed CQ callbacks
       [not found]                 ` <u2ze2e108261003311136l4bcf1627n9ba4d3414167eef3-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-03-31 18:57                   ` Roland Dreier
       [not found]                     ` <adask7giak4.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Roland Dreier @ 2010-03-31 18:57 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Ralph Campbell, linux-rdma-u79uwXL29TY76Z2rM5mHXA

 > Processing the flag IB_CQ_REPORT_MISSED_EVENTS causes several IB
 > drivers to lock and unlock a spinlock. Full-speed SRP I/O can cause
 > this operation to be invoked more than 100.000 times a second, so the
 > flag IB_CQ_REPORT_MISSED_EVENTS will have a small but measurable
 > impact on SRP I/O performance.

It seems the only driver where that happens is ehca.  But on the other
hand, for mthca/mlx4 it avoids doing something like:

  cq event:
    request notify <-- CQ is not empty, another event queued
    drain CQ

  cq event:
    request notify
    drain already empty CQ

ie the overhead saved here looks a lot more.
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] IB/srp: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks
       [not found]                     ` <adask7giak4.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
@ 2010-03-31 19:16                       ` Bart Van Assche
  0 siblings, 0 replies; 14+ messages in thread
From: Bart Van Assche @ 2010-03-31 19:16 UTC (permalink / raw)
  To: Roland Dreier; +Cc: Ralph Campbell, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Wed, Mar 31, 2010 at 8:57 PM, Roland Dreier <rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> wrote:
>  > Processing the flag IB_CQ_REPORT_MISSED_EVENTS causes several IB
>  > drivers to lock and unlock a spinlock. Full-speed SRP I/O can cause
>  > this operation to be invoked more than 100.000 times a second, so the
>  > flag IB_CQ_REPORT_MISSED_EVENTS will have a small but measurable
>  > impact on SRP I/O performance.
>
> It seems the only driver where that happens is ehca.  But on the other
> hand, for mthca/mlx4 it avoids doing something like:
>
>  cq event:
>    request notify <-- CQ is not empty, another event queued
>    drain CQ
>
>  cq event:
>    request notify
>    drain already empty CQ
>
> ie the overhead saved here looks a lot more.

I agree that Ralph's patch helps to avoid the above scenario and that
this should result in a performance improvement -- at least when
IB_CQ_REPORT_MISSED_EVENTS is implemented in an efficient way.

I have to admit that a few months ago I was puzzled myself by the code
in srp_completion() and that I was wondering why it worked. Around
that time I have been experimenting with a patch similar to the one
Ralph posted, and it resulted in lower performance on mlx4 hardware.
So it would help if someone could measure the performance of the SRP
initiator with and without Ralph's patch applied.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] IB/core: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks
       [not found]         ` <ada4ojwjqz7.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
@ 2010-03-31 19:20           ` Ralph Campbell
       [not found]             ` <1270063245.4192.299.camel-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Ralph Campbell @ 2010-03-31 19:20 UTC (permalink / raw)
  To: Roland Dreier; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

The way I understand the sequence of events w/o the
patch is:

ib_req_notify_cq(IB_CQ_NEXT_COMP)
CQE 1 added to queue
	callback scheduled via tasklet
	future callbacks disarmed
callback function calls ib_req_notify_cq(IB_CQ_NEXT_COMP)
callback function calls ib_poll_cq() and gets CQE 1
callback function calls ib_poll_cq() and gets none
CQE 2 added to queue via IRQ
	callback scheduled via tasklet
	future callbacks disarmed
callback function returns
some time later, tasklet runs and calls CQ callback function.
callback function calls ib_req_notify_cq(IB_CQ_NEXT_COMP)
callback function calls ib_poll_cq() and gets CQE 2

Since a tasklet or workqueue can be scheduled in the
callback function, the second CQE isn't "missed" but
there is a scheduling delay before the callback happens
and sees CQE 2.

I guess it is a minor optimization since either CQE 2
will be seen in the first callback while looping in ib_poll_cq()
and then getting a callback later with ib_poll_cq()==0 or
seen in the second callback.

I'm willing to withdraw the 1-3 patches.

I still don't understand why the timing difference matters
to RDS.

On Wed, 2010-03-31 at 11:17 -0700, Roland Dreier wrote:
> > ib_req_notify_cq(IB_CQ_NEXT_COMP) is not guaranteed to generate
>  > a callback for the next completion entered since there is a race
>  > between arming the callback and another CQE being added to the queue.
>  > The IB_CQ_REPORT_MISSED_EVENTS flag was added to detect this
>  > race and allow the verbs consumer to call ib_poll_cq() and
>  > ib_req_notify_cq() again to avoid delays in processing the CQE.
> 
> I'm not sure I understand the race you're fixing here... the existing
> code does the rearm before polling:
> 
>  > +	int ret;
>  >  
>  >  	port_priv = container_of(work, struct ib_mad_port_private, work);
>  > -	ib_req_notify_cq(port_priv->cq, IB_CQ_NEXT_COMP);
>  >  
>  > +again:
>  >  	while (ib_poll_cq(port_priv->cq, 1, &wc) == 1) {
>  >  		if (wc.status == IB_WC_SUCCESS) {
>  >  			switch (wc.opcode) {
>  > @@ -2246,6 +2247,10 @@ static void ib_mad_completion_handler(struct work_struct *work)
>  >  		} else
>  >  			mad_error_handler(port_priv, &wc);
>  >  	}
>  > +	ret = ib_req_notify_cq(port_priv->cq, IB_CQ_NEXT_COMP |
>  > +					      IB_CQ_REPORT_MISSED_EVENTS);
>  > +	if (ret > 0)
>  > +		goto again;
> 
> The only issue with the existing code is that it may trigger extra
> events that will find the CQ empty when polling.
> 
> So this may be a valid optimization but I don't see it fixing any missed
> events.  Am I missing something?
> 
> Also for all these fixes, I think you only want to rearm the CQ once and
> go back and poll if you get a missed events warning; the next time the
> CQ is empty, then you know another event will happen.
> 
>  - R.


--
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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] IB/core: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks
       [not found]             ` <1270063245.4192.299.camel-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
@ 2010-04-01  0:12               ` Roland Dreier
       [not found]                 ` <ada4ojwghey.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Roland Dreier @ 2010-04-01  0:12 UTC (permalink / raw)
  To: Ralph Campbell; +Cc: linux-rdma@vger.kernel.org

 > The way I understand the sequence of events w/o the
 > patch is:
 > 
 > ib_req_notify_cq(IB_CQ_NEXT_COMP)
 > CQE 1 added to queue
 > 	callback scheduled via tasklet
 > 	future callbacks disarmed
 > callback function calls ib_req_notify_cq(IB_CQ_NEXT_COMP)
 > callback function calls ib_poll_cq() and gets CQE 1
 > callback function calls ib_poll_cq() and gets none
 > CQE 2 added to queue via IRQ
 > 	callback scheduled via tasklet
 > 	future callbacks disarmed
 > callback function returns
 > some time later, tasklet runs and calls CQ callback function.
 > callback function calls ib_req_notify_cq(IB_CQ_NEXT_COMP)
 > callback function calls ib_poll_cq() and gets CQE 2
 > 
 > Since a tasklet or workqueue can be scheduled in the
 > callback function, the second CQE isn't "missed" but
 > there is a scheduling delay before the callback happens
 > and sees CQE 2.

Sure but a tasklet at least is likely to have pretty low latency.  And
maybe there's a better chance of batching completion processing by
introducing a little delay there.  So maybe it helps, maybe it doesn't.

 > I'm willing to withdraw the 1-3 patches.
 > 
 > I still don't understand why the timing difference matters
 > to RDS.

I'd like to have some data showing an improvement before making this
change.  And as I already said it would be good to know what's really
going on with RDS before we change that.

 - R.
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] IB/core: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks
       [not found]                 ` <ada4ojwghey.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
@ 2010-04-02 19:54                   ` Andy Grover
  0 siblings, 0 replies; 14+ messages in thread
From: Andy Grover @ 2010-04-02 19:54 UTC (permalink / raw)
  To: Roland Dreier; +Cc: Ralph Campbell, linux-rdma-u79uwXL29TY76Z2rM5mHXA

(sorry to reply late -- bad mail rules.)

RDS used to do send and recv processing in the event handler.

It currently uses a tasklet for recv, and send is still in the event
handler. When we moved the recv processing to a tasklet, we poll the cq,
arm the cq, then poll it again, because it seems like 1) we want to do
as much work as possible before rearming the event and 2) we need to
poll after, to be sure to catch CQEs added between the time when the
handler exited and we've gotten to this point in the tasklet. How does
this approach (re-polling after calling notify_cq) compare with using
REPORT_MISSED_EVENTS? Maybe it's two different ways to do the same thing?

Anyways, this is off the point from your original patch that changed the
send cq handling, which is all done in the event handler. I don't have
an explanation for you seeing the 0xdeads, except that the comment above
that code that mentions racing cleanly with the send path makes my
antennae twitch a little bit...

Regards -- Andy

PS FWIW my dev branch moves RDS to a single CQ for send & recv events
similar to srp, except using a tasklet, and notify_cq(IB_CQ_SOLICITED),
and doing poll/arm/poll.
--
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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2010-04-02 19:54 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-31 17:57 [PATCH 0/3] best practice for setting IB_CQ_REPORT_MISSED_EVENTS Ralph Campbell
     [not found] ` <20100331175742.24522.17850.stgit-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
2010-03-31 17:57   ` [PATCH 1/3] IB/core: use IB_CQ_REPORT_MISSED_EVENTS to avoid missed CQ callbacks Ralph Campbell
     [not found]     ` <20100331175747.24522.88472.stgit-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
2010-03-31 18:07       ` Sean Hefty
2010-03-31 18:17       ` Roland Dreier
     [not found]         ` <ada4ojwjqz7.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
2010-03-31 19:20           ` Ralph Campbell
     [not found]             ` <1270063245.4192.299.camel-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
2010-04-01  0:12               ` Roland Dreier
     [not found]                 ` <ada4ojwghey.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
2010-04-02 19:54                   ` Andy Grover
2010-03-31 17:57   ` [PATCH 2/3] IB/srp: " Ralph Campbell
     [not found]     ` <20100331175752.24522.66519.stgit-/vjeY7uYZjrPXfVEPVhPGq6RkeBMCJyt@public.gmane.org>
2010-03-31 18:11       ` Bart Van Assche
     [not found]         ` <y2ie2e108261003311111uec1ec00g746ba0ffe3d98799-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-03-31 18:18           ` Roland Dreier
     [not found]             ` <adazl1oiccb.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
2010-03-31 18:36               ` Bart Van Assche
     [not found]                 ` <u2ze2e108261003311136l4bcf1627n9ba4d3414167eef3-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-03-31 18:57                   ` Roland Dreier
     [not found]                     ` <adask7giak4.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
2010-03-31 19:16                       ` Bart Van Assche
2010-03-31 17:57   ` [PATCH 3/3] IB/iser: " Ralph Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.