[ofa-general] [PATCH 1/2] IB/ipath - fix UD send with immediate

[Ralph Campbell]

On Tue, 2008-01-15 at 20:06 -0800, Roland Dreier wrote:
>  > This fixes a small bug which incorrectly calculated the header size
>  > for UD send with immediate and therefore dropped packets.
> 
> What's the bug?  It's not clear how this patch fixes anything...
> 
>  > @@ -301,8 +301,6 @@ int ipath_make_ud_req(struct ipath_qp *qp)
>  >  
>  >  	/* header size in 32-bit words LRH+BTH+DETH = (8+12+8)/4. */
>  >  	qp->s_hdrwords = 7;
>  > -	if (wqe->wr.opcode == IB_WR_SEND_WITH_IMM)
>  > -		qp->s_hdrwords++;
>  >  	qp->s_cur_size = wqe->length;
>  >  	qp->s_cur_sge = &qp->s_sge;
>  >  	qp->s_wqe = wqe;
>  > @@ -327,6 +325,7 @@ int ipath_make_ud_req(struct ipath_qp *qp)
>  >  		ohdr = &qp->s_hdr.u.oth;
>  >  	}
>  >  	if (wqe->wr.opcode == IB_WR_SEND_WITH_IMM) {
>  > +		qp->s_hdrwords++;
>  >  		ohdr->u.ud.imm_data = wqe->wr.imm_data;

This is just eliminating an "if". A trivial optimization.

> This looks like it doesn't make any difference, since I don't see any
> place qp->s_hdrwords is used in between the old location and the new
> location of the increment...
> 
>  > +	/*
>  > +	 * The opcode is in the low byte when its in network order
>  > +	 * (top byte when in host order).
>  > +	 */
>  > +	opcode = be32_to_cpu(ohdr->bth[0]) >> 24;
>  > +	if (qp->ibqp.qp_num > 1 &&
>  > +	    opcode == IB_OPCODE_UD_SEND_ONLY_WITH_IMMEDIATE) {
>  > +		if (header_in_data) {
>  > +			wc.imm_data = *(__be32 *) data;
>  > +			data += sizeof(__be32);
>  > +		} else
>  > +			wc.imm_data = ohdr->u.ud.imm_data;
>  > +		wc.wc_flags = IB_WC_WITH_IMM;
>  > +		hdrsize += sizeof(u32);
>  > +	} else if (opcode == IB_OPCODE_UD_SEND_ONLY) {
>  > +		wc.imm_data = 0;
>  > +		wc.wc_flags = 0;
>  > +	} else {
>  > +		dev->n_pkt_drops++;
>  > +		goto bail;
>  > +	}
>  > +
>  >  	/* Get the number of bytes the message was padded by. */
>  >  	pad = (be32_to_cpu(ohdr->bth[0]) >> 20) & 3;
>  >  	if (unlikely(tlen < (hdrsize + pad + 4))) {

hdrsize is used here so the code to initialize it has to happen
before this point.

> I guess the bug is that you need to set hdrsize to the length
> including the immediate data before this last test?

Correct.

> And this bug causes problems because MVAPICH uses UD sends with
> immediate data??  How bad is the impact of this -- do we really need
> it for 2.6.24?

MVAPICH-UD doesn't work at all without this change.
HP MPI also uses UD with immediate to do the initial discovery.

> By the way, is there anything in the IB spec that forbids sends with
> immediate data on QP1?  I'm wondering why your code treats QP1
> differently here.
> 
>  - R.
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
> 
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general