[openib-general] [PATCH] use mmiowb after doorbell ring

[akepner at sgi.com]

On Mon, 16 Oct 2006, Roland Dreier wrote:

> OK, cool.  Sounds convincing to me.  BTW -- are there Altix systems
> with PCIe?  Have you tested the mthca_arbel_xxx (mem-free PCIe HCA)
> changes, or just the mthca_tavor_xxx (PCI-X HCA) parts?

Yes, there are PCIe Altices, but I was unable to reproduce the
hang on one of them. It was easy to reproduce on a PCIX machine,
though.

>
> I see -- the other mmiowb()s are next to the spin_unlock()s elsewhere
> because the other routines might ring doorbells during the loop if
> someone passes in a ton of work requests, right?

I tried to call mmiowb() only when it was clear that a doorbell
write had occurred. In some cases there's an obvious condition
to test, but in others (e.g., mthca_tavor_post_srq_recv()),
"if (likely(nreq))" doesn't appear to the right condition, as
nreq might be reset to 0 above with:

 	if (unlikely(nreq == MTHCA_TAVOR_MAX_WQES_PER_RECV_DB)) {
 		nreq = 0;
 		.....

(Which, I suppose, is the long way of answering your question with 
"yes".)


> .... (All the mmiowb()s
> look necessary to me but I'm just curious about the level of testing)

I've "smoke tested" this by building, installing, running on ia64
(PCIe and PCIX) and x86_64. Just some simple IPoIB apps (no smoke 
was detected.) But the more testing the better, for sure.

>
> I'm still a little puzzled by the fact that it affects performance,
> because that "if (likely(nreq))" is super-super-likely: under any
> normal workload, I would expect it always to be true.  It's really
> strange that there's any difference between
>
> 	mmiowb();
> 	qp->sq.next_ind = ind;
> 	qp->sq.head    += nreq;
>
> and
>
> 	qp->sq.next_ind = ind;
> 	qp->sq.head    += nreq;
> 	mmiowb();
>

I'd assumed any difference was attributable to a small fraction
of times when the "likely" branch wasn't taken, but I didn't verify
this (just passing on a very qualitative take on data acquired by
a co-worker; maybe the difference was just "in the noise" after all.)

I agree that the reordering of instructions as you show above would 
be very highly unlikely to have any measurable affect.


> Anyway, it all looks good.  I'll apply your patch and submit it to
> -stable for 2.6.18.x.
>

Thanks.

-- 
Arthur