On Fri, Dec 03, 2004 at 06:25:27PM -0800, Roland Dreier wrote:
> Roland> Hmm, I guess that wasn't the problem (or I didn't fix it
> Roland> properly).
>
> The latter (fix was bogus, I made a cut-and-paste error). Here's a
> better version:
Yes - that works. Please commit.
I was still trying to sort out how set_ci when I gave up on friday.
You explanation helped though I'm familiar with consumer/producer
(tg3 has a similar construct) indexes.
> Index: infiniband/hw/mthca/mthca_eq.c
> ===================================================================
> --- infiniband/hw/mthca/mthca_eq.c (revision 1310)
> +++ infiniband/hw/mthca/mthca_eq.c (working copy)
> @@ -219,11 +219,14 @@
> struct mthca_eqe *eqe;
> int disarm_cqn;
> int work = 0;
> + int set_ci = 0;
>
> while (1) {
> if (!next_eqe_sw(eq))
> break;
>
> + set_ci = 0;
> +
> eqe = get_eqe(eq, eq->cons_index);
> work = 1;
>
> @@ -274,6 +277,13 @@
> be16_to_cpu(eqe->event.cmd.token),
> eqe->event.cmd.status,
> be64_to_cpu(eqe->event.cmd.out_param));
> + /*
> + * Need to set the CI inside the loop for
> + * command completion events, because this
> + * event allows another command to be posted
> + * and we may overflow the EQ.
> + */
The comment inside "case MTHCA_EVENT_TYPE_CMD" now makes sense
and it didn't make sense to me on friday...perhaps showing it
was indeed time for a break.
> + set_ci = 1;
> break;
>
> case MTHCA_EVENT_TYPE_PORT_CHANGE:
> @@ -296,9 +306,14 @@
>
> set_eqe_hw(eq, eq->cons_index);
> eq->cons_index = (eq->cons_index + 1) & (eq->nent - 1);
This line of code assumes eq->nent is a power of 2.
I didn't see anything in mthca_create_eq() that enforces
the assumption that nent is a power of 2.
It mthca_create_eq() isn't in the perf path, I think it would be good
to enforcement the requirement or at least check for it (e.g. WARN_ON).
I just don't want to make the performance patch longer.
> +
> + if (set_ci) {
> + wmb();
this wmb() isn't necessary since set_eq_ci() calls mthca_write64()
and the latter *must* enforce wmb() for the MMIO write.
I'd also like to see doorbell[2] go away and just pass the two u32 values
to mthca_write64(). Perhaps combine them explicitly instead depending on
load/store model of the stack.
While it seems to work, I'm very nervous about this dependency and
wonder if it would even work on parisc-linux (ok, I'm the only who cares :^)
which has an upward growing stack.
> + set_eq_ci(dev, eq->eqn, eq->cons_index);
> + }
> }
>
> - if (work) {
> + if (work && !set_ci) {
> wmb();
ditto.
> set_eq_ci(dev, eq->eqn, eq->cons_index);
> }
thanks,
grant
_______________________________________________
openib-general mailing list
[EMAIL PROTECTED]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
