[ofa-general] Re: [PATCH] mlx4_ib: fix for bugzilla 1383 (LSO packet processing)

[Jack Morgenstein]

On Tuesday 30 December 2008 00:02, Roland Dreier wrote:
> 
> I would love it if everyone tried feeding the emails they send into "git
> am" and then examined the result, and fixed things until the git tree
> that you get looks right.

I apologize, I've been swamped and just pushed this one out the door.  I guess, though,
everyone here is always swamped -- I'll put in the extra little bit of effort and
do things properly next time.

> 
> Anyway, I guess the bug is that build_lso_seg() overwrites the stamping
> and allows the WQE to start executing before the gather list is written?
Correct.

> 
>  > @@ -1606,13 +1605,21 @@ int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>  >  			size += sizeof (struct mlx4_wqe_datagram_seg) / 16;
>  >  
>  >  			if (wr->opcode == IB_WR_LSO) {
>  > -				err = build_lso_seg(wqe, wr, qp, &seglen);
     +	LSO data filled in HERE: err = build_lso_seg(wqe, wr, qp, &seglen, &lso_hdr_sz);
>  >  				if (unlikely(err)) {
>  >  					*bad_wr = wr;
>  >  					goto out;
>  >  				}
>  > +				lso_wqe = (__be32 *) wqe;
>  >  				wqe  += seglen;
>  > -				size += seglen / 16;
>  > +				dseg = wqe;
>  > +				dseg += wr->num_sge - 1;
>  > +				size += (seglen / 16) + wr->num_sge *
>  > +						(sizeof (struct mlx4_wqe_data_seg) / 16);
>  > +				for (i = wr->num_sge - 1; i >= 0; --i, --dseg)
>  > +					set_data_seg(dseg, wr->sg_list + i);
                           *** wmb is called in set_data_seg.
     +	stamping dword written here:	*lso_wqe = lso_hdr_sz;
>  > +				goto lso_continue;
> 
> rather than duplicating all this code and adding the ugly goto, can we
> just add another "if (wr->opcode == IB_WR_LSO)" after writing the data
> segments and do the *lso_wqe = lso_hdr_sz there?

this is in the data path, so I put in a lot of effort to guarantee that
there would not be an extra "if" for ALL post_sends.  This way, although
a bit ugly, only LSO packets are affected, and there is no extra "if".

> Also is your code missing a memory barrier between the set_data_seg()
> loop and the lso_wqe assignment?  It seems that an out-of-order CPU
> could make the lso_wqe visible before all the data segments are visible,
> so the bug could show up there anyway.
The memory barrier is unnecessary -- since the lso segment (without stamping)
is written first, then all the data segments are written (wmb() called for each segment),
then finally the lso stamping dword.  Thus, this will always follow a wmb().
(no one would send an LSO without any data).

- Jack