[ofa-general] Re: [PATCH] IB/mthca: recv poll cq optimization

[Roland Dreier]

 > All good recv work requests generate HW completions in FIFO order, so we can use
 > rq->tail rather than hardware data. In this way, we save a branch on data path
 > for recv completions (branch is still there for send completions).
 > 
 > Roland, what do you think? This increases the overall code size but I think the
 > extra code is on the error CQE handling path.  BTW, since most kernel QPs seem
 > not to use selective signaling, it might be worth it to optimize send
 > completions in a similiar way in case selective singaling is disabled on QP.

Do you have any measurements that say this helps?  Having a bigger
I-cache footprint is really globally worse for the system, so I don't
like this part:

 > +	if (unlikely(is_error)) {
 > +		if (!is_send && !(*cur_qp)->ibqp.srq) {
 > +			s32 wqe = be32_to_cpu(cqe->wqe);
 > +			wqe_index = wqe >> wq->wqe_shift;
 > +			/*
 > +			 * WQE addr == base - 1 might be reported in receive completion
 > +			 * with error instead of (rq size - 1) by Sinai FW 1.0.800 and
 > +			 * Arbel FW 5.1.400.  This bug should be fixed in later FW revs.
 > +			 */
 > +			if (unlikely(wqe_index < 0))
 > +				wqe_index = wq->max - 1;
 > +		}
 >  
 > -	if (is_error) {

is there any way to move that into handle_error_cqe() so that it's
definitely out of the way for the normal path?  In fact, why do we
need this code at all with your change -- aren't RQ error completions
reported in FIFO order too?

I'm not sure that it's worth testing whether a SQ has selective
signaling or not -- after all, that's just changing one conditional
branch for another.  And in fact, looking at the code, I think we
could rewrite

		if (wq->last_comp < wqe_index)
			wq->tail += wqe_index - wq->last_comp;
		else
			wq->tail += wqe_index + wq->max - wq->last_comp;

as just

		wq->tail += (wqe_index + wq->max - wq->last_comp) & (wq->max - 1);

and avoid the conditional in a simpler way.  (I've not checked that
match but it looks right to me)

 - R.