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
> [email protected]
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
_______________________________________________
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
