[ofa-general] IPOIB CM (NOSRQ) patch for review

Roland Dreier rdreier at cisco.com
Fri Mar 9 15:10:44 PST 2007


 > +EXTRA_CFLAGS += -DIPOIB_CM_NOSRQ

This type of compile-time selection is obviously unacceptable for
anything that we actually merge upstream.  What is needed is for the
IPoIB driver to decide at runtime what to do, so that on a system with
multiple different types of HCAs installed, IPoIB CM uses SRQs on the
HCAs that support SRQs, and does not use SRQs on the HCAs that don't.

Not to mention the fact that basically mixing together two different
implementations with a liberal sprinkling of #ifdef IPOIB_CM_NOSRQ
makes the code basically unreadable and unmaintainable.

 > +       /* This is missing in Michael's code -Do we need this */

seems like it would be easy to answer this question -- just try it
without the change.  And I think the answer is no, there's no reason
to move QPs that are not used to send data to the RTS state.

 > void ipoib_cm_handle_rx_wc(struct net_device *dev, struct ib_wc *wc)

 > +#ifdef IPOIB_CM_NOSRQ
 > +        spin_lock_irqsave(&priv->lock, flags);
 > +        list_for_each_entry(rx_ptr, &priv->cm.passive_ids, list)
 > +                if(qp_num == rx_ptr->qp->qp_num) {
 > +                       found = 1;
 > +                        break;
 > +               }
 > +        spin_unlock_irqrestore(&priv->lock, flags);

This seems crazy -- you do a linear search through a list of QPs
(which potentially has 100s of entries) for every receive completion!
Just the spinlock alone is something we would want to avoid in the hot
performance path.

 > +                /* Temporary hack till ehca fixes wc->qp = NULL */

Why do you need wc->qp without SRQ?  Surely the wr_id alone should be
enough to figure out which QP the work request was posted on.

 - R.



More information about the general mailing list