[openib-general] IPoIB connected mode review comments

[Steve Wise]

On Thu, 2007-02-01 at 20:48 -0800, Roland Dreier wrote:
>  > Have you had a chance to review this?
> 
> Still on my list.
> 
> Can we trade?  Can you look at the IPoIB connected mode stuff in the
> ipoib-cm branch in
> 
>     git://git.kernel.org/pub/scm/linux/kernel/git/roland/infiniband.git
> 
> and let me know if you see anything you don't like?
> 
>  - R.

Here are my comments.  I'm not an ib cm expert though.  These are mostly
questions:


Since IPoIB is using IP addresses already, wouldn't it be simpler to use
the rdma cm to setup connections?  

Could you optimize this design and only signal some of the tx wrs?

In ipoib_cm_send() you call ipoib_cm_skb_too_long() if the packet is too
large for the interface mtu.  And you print a warning.  But
ipoib_cm_skb_too_long() actually queues the packet for the cm case.  For
ud it just drops the packet.  The skb task for cm then will send a
ICMP_DEST_UNREACH for these packets.  Why the difference?  Also if this
packet came from the local stack via a local application, you don't want
to send  DEST_UNREACH, right?  (I'm probably just confused about the
purpose of this).

In ipoib_cm_tx_completion() you rearm, then drain the cq.  I thought
there was some reason that it was better to do drain/rearm/drain?
Something about if you rearm and there's a cq entry mthca does another
immediate interrupt?  

In ipoib_cm_handle_tx_wc():

When can a tx completion happen with a wr_id that isn't within the
ipoib_sendq_size range?  This looks like it is really a bug condition
that should never happen.  I see the same code in the rx completion path
too.  

Also, what's up with the /* FIXME */ comment?

You lock the priv->lock inside of the priv->tx_lock.  Is this ordering
correct and consistent across all the code?


ipoib_cm_handle_rx_wc() - what's up with the XXX comment?

What's the algorithm to keep enough buffers posted in the SRQ?