<br>
<br><tt><font size=2>Roland Dreier <rdreier@cisco.com> wrote on 03/09/2007
03:10:44 PM:<br>
<br>
>  > +EXTRA_CFLAGS += -DIPOIB_CM_NOSRQ<br>
> <br>
> This type of compile-time selection is obviously unacceptable for<br>
> anything that we actually merge upstream.  What is needed is
for the<br>
> IPoIB driver to decide at runtime what to do, so that on a system
with<br>
> multiple different types of HCAs installed, IPoIB CM uses SRQs on
the<br>
> HCAs that support SRQs, and does not use SRQs on the HCAs that don't.</font></tt>
<br>
<br><tt><font size=2>I dug through the spec and found that ib_query_device()
tells one if the </font></tt>
<br><tt><font size=2>HCA supports SRQ or not. Is that what you had in mind?</font></tt>
<br><tt><font size=2><br>
> <br>
> Not to mention the fact that basically mixing together two different<br>
> implementations with a liberal sprinkling of #ifdef IPOIB_CM_NOSRQ<br>
> makes the code basically unreadable and unmaintainable.</font></tt>
<br>
<br><tt><font size=2>One way to alleviate this problem would be to duplicate
mainly the </font></tt>
<br><tt><font size=2>receive side functions and name them something like
xyz_nosrq(). </font></tt>
<br><tt><font size=2>However, there will still be many instances of</font></tt>
<br>
<br><tt><font size=2>if(SRQ)</font></tt>
<br><tt><font size=2>        xyz();</font></tt>
<br><tt><font size=2>else</font></tt>
<br><tt><font size=2>        xyz_nosrq();</font></tt>
<br>
<br><tt><font size=2>Is that a better solution than the #ifdef IPOIB_CM_NOSRQ?
On the other hand,</font></tt>
<br><tt><font size=2>this will add duplicate code and may pose some maintainability
issues in the</font></tt>
<br><tt><font size=2>future. I would like to understand as to which one
is the preferred approach.</font></tt>
<br><tt><font size=2><br>
> <br>
>  > +       /* This is missing in Michael's
code -Do we need this */<br>
> <br>
> seems like it would be easy to answer this question -- just try it<br>
> without the change.  And I think the answer is no, there's no
reason<br>
> to move QPs that are not used to send data to the RTS state.</font></tt>
<br>
<br><tt><font size=2>Yes, you are right. I should have dropped this before
submitting the patch.</font></tt>
<br>
<br><tt><font size=2><br>
> <br>
>  > void ipoib_cm_handle_rx_wc(struct net_device *dev, struct
ib_wc *wc)<br>
> <br>
>  > +#ifdef IPOIB_CM_NOSRQ<br>
>  > +        spin_lock_irqsave(&priv->lock,
flags);<br>
>  > +        list_for_each_entry(rx_ptr,
&priv->cm.passive_ids, list)<br>
>  > +                if(qp_num
== rx_ptr->qp->qp_num) {<br>
>  > +                
      found = 1;<br>
>  > +                
       break;<br>
>  > +               }<br>
>  > +        spin_unlock_irqrestore(&priv->lock,
flags);<br>
> <br>
> This seems crazy -- you do a linear search through a list of QPs<br>
> (which potentially has 100s of entries) for every receive completion!<br>
> Just the spinlock alone is something we would want to avoid in the
hot<br>
> performance path.</font></tt>
<br>
<br><tt><font size=2>I envisaged the NOSRQ case for small clusters only.
Othewise, this may be a</font></tt>
<br><tt><font size=2>memory hog and affect (other) application performance.</font></tt>
<br><tt><font size=2><br>
Pradeep</font></tt>
<br><tt><font size=2>pradeep@us.ibm.com</font></tt>