[openib-general] HP ZX1 and HP IB cards...

[Grant Grundler]

On Thu, Dec 09, 2004 at 04:05:36PM -0800, Grant Grundler wrote:
> Roland,
> Would you consider committing the set_ci patch and backing the ncmd patch out?

Roland,
Attached is a patch that does three things (LART me for combining them,
but I can split them up later if you want):
o reverse most of the mthca_cmd_complete() patch
o add set_ci logic so consumer index is updated inside the event loop
o eliminate "work" variable (nicely simplifies the code).

If I understood the code correctly, "work" was always being set
if next_eqe_sw(eq) was non-zero. 

It is so expensive to get to the interrupt handler in the first place
that if nothing needs to be done:
a) the problem is NOT in the interrupt handler.
b) the spurious set_eq_ci() is light weight enough to be in the noise

I do understand the wmb()/set_eq_ci() disturb the PCI data flows but
we shouldn't be seeing spurious interrupts either. If they are happening
often enough to disturb performance, something else is wrong.

grant


Index: hw/mthca/mthca_cmd.c
===================================================================
--- hw/mthca/mthca_cmd.c	(revision 1317)
+++ hw/mthca/mthca_cmd.c	(working copy)
@@ -293,12 +293,6 @@
 	complete(&context->done);
 }
 
-void mthca_cmd_complete(struct mthca_dev *dev, int ncomp)
-{
-	while (ncomp--)
-		up(&dev->cmd.event_sem);
-}
-
 static void event_timeout(unsigned long context_ptr)
 {
 	struct mthca_cmd_context *context =
@@ -363,6 +357,7 @@
 	dev->cmd.free_head = context - dev->cmd.context;
 	spin_unlock(&dev->cmd.context_lock);
 
+	up(&dev->cmd.event_sem);
 	return err;
 }
 
Index: hw/mthca/mthca_eq.c
===================================================================
--- hw/mthca/mthca_eq.c	(revision 1317)
+++ hw/mthca/mthca_eq.c	(working copy)
@@ -218,15 +218,10 @@
 {
 	struct mthca_eqe *eqe;
 	int disarm_cqn;
-	int work = 0;
-	int ncmd = 0;
 
-	while (1) {
-		if (!next_eqe_sw(eq))
-			break;
-
+	while (next_eqe_sw(eq)) {
+		int set_ci = 0;
 		eqe = get_eqe(eq, eq->cons_index);
-		work = 1;
 
 		switch (eqe->type) {
 		case MTHCA_EVENT_TYPE_COMP:
@@ -275,7 +270,11 @@
 					be16_to_cpu(eqe->event.cmd.token),
 					eqe->event.cmd.status,
 					be64_to_cpu(eqe->event.cmd.out_param));
-			++ncmd;
+			/* cmd_event() may add more commands.
+			 * The card will think the queue has overflowed if
+			 * we don't tell it we've been processing events.
+			 */
+			set_ci = 1;
 			break;
 
 		case MTHCA_EVENT_TYPE_PORT_CHANGE:
@@ -298,25 +297,25 @@
 
 		set_eqe_hw(eq, eq->cons_index);
 		eq->cons_index = (eq->cons_index + 1) & (eq->nent - 1);
-	}
 
-	if (work) {
-		/*
-		 * This barrier makes sure that all updates to
-		 * ownership bits done by set_eqe_hw() hit memory
-		 * before the consumer index is updated.  set_eq_ci()
-		 * allows the HCA to possibly write more EQ entries,
-		 * and we want to avoid the exceedingly unlikely
-		 * possibility of the HCA writing an entry and then
-		 * having set_eqe_hw() overwrite the owner field.
-		 */
-		wmb();
-		set_eq_ci(dev, eq->eqn, eq->cons_index);
+		if (set_ci) {
+			wmb(); /* see comment below */
+			set_eq_ci(dev, eq->eqn, eq->cons_index);
+			set_ci = 0;
+		}	
 	}
 
-	if (ncmd)
-		mthca_cmd_complete(dev, ncmd);
-
+	/*
+	 * This barrier makes sure that all updates to
+	 * ownership bits done by set_eqe_hw() hit memory
+	 * before the consumer index is updated.  set_eq_ci()
+	 * allows the HCA to possibly write more EQ entries,
+	 * and we want to avoid the exceedingly unlikely
+	 * possibility of the HCA writing an entry and then
+	 * having set_eqe_hw() overwrite the owner field.
+	 */
+	wmb();
+	set_eq_ci(dev, eq->eqn, eq->cons_index);
 	eq_req_not(dev, eq->eqn);
 }
 
Index: hw/mthca/mthca_cmd.h
===================================================================
--- hw/mthca/mthca_cmd.h	(revision 1317)
+++ hw/mthca/mthca_cmd.h	(working copy)
@@ -205,7 +205,6 @@
 void mthca_cmd_use_polling(struct mthca_dev *dev);
 void mthca_cmd_event(struct mthca_dev *dev, u16 token,
 		     u8  status, u64 out_param);
-void mthca_cmd_complete(struct mthca_dev *dev, int ncomp);
 
 int mthca_SYS_EN(struct mthca_dev *dev, u8 *status);
 int mthca_SYS_DIS(struct mthca_dev *dev, u8 *status);