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

[Roland Dreier]

 > +#define IPOIB_CM_OP_NOSRQ (1ul << 29)

I don't understand the point of this... the only places you do anything
with it are:

 > +       priv->cm.rx_wr.wr_id = wr_id << 32 | index | IPOIB_CM_OP_NOSRQ;
 > +       index = (wc->wr_id & ~IPOIB_CM_OP_NOSRQ) & NOSRQ_INDEX_MASK ;
 > +       if ((wc->wr_id & IPOIB_CM_OP_SRQ) || (wc->wr_id & IPOIB_CM_OP_NOSRQ))

so probably the most sensible thing to do is just to rename
IPOIB_CM_OP_SRQ to IPOIB_OP_CM_RECV.

 > +/* These two go hand in hand */
 > +#define NOSRQ_INDEX_RING_SIZE 1024
 > +#define NOSRQ_INDEX_MASK      0x00000000000003ff

Rather than having a comment, I would just do

#define NOSRQ_INDEX_RING_SIZE 1024
#define NOSRQ_INDEX_MASK      (NOSRQ_INDEX_RING_SIZE - 1)

also I think the RING name is wrong -- it's not a ring, it's a table,
right?  I don't like having a static limit on the number of nosrq
connections; could this be a hash table instead?

Some of these changes seem strange:

 > -               priv->cm.rx_sge[i].addr = priv->cm.srq_ring[id].mapping[i];
 > +               priv->cm.rx_sge[i].addr = 
 > +               priv->cm.srq_ring[id].mapping[i];

 > -                                     priv->cm.srq_ring[id].mapping);
 > +                                     priv->cm.srq_ring[id].mapping);

please try to put any changes like this that you want to make into a
separate patch.

 > +       if (priv->cm.srq) 
 > +               priv->cm.srq_ring[id].skb = skb;
 > +       else {
 > +               index = id  & NOSRQ_INDEX_MASK ;
 > +               wr_id = id >> 32;
 > +               spin_lock_irqsave(&priv->lock, flags);
 > +               rx_ptr = priv->cm.rx_index_ring[index];
 > +               spin_unlock_irqrestore(&priv->lock, flags);
 > +
 > +               rx_ptr->rx_ring[wr_id].skb = skb;

why does the nosrq case need to take a lock when the srq case doesn't?
A comment would be welcome here...

 > -               .srq = priv->cm.srq,
 > 
 > +       if (priv->cm.srq)
 > +               attr.srq = priv->cm.srq;
 > +       else
 > +               attr.srq = NULL;

isn't the code you use to replace the assignment just an obfuscated
version of the original assignment?

 > -       rep.srq = 1;

 > +       if (priv->cm.srq)
 > +               rep.srq = 1;
 > +       else
 > +               rep.srq = 0;

similarly I would rather see "rep.srq = !!priv->cm.srq"

 > +       /* Allocate space for the rx_ring here */
 > +       p->rx_ring = kzalloc(ipoib_recvq_size * sizeof *p->rx_ring,
 > +                            GFP_KERNEL);

in general comments are good, but I don't like to see redundancy like:

	/* do something here */
        do_something();

 > +       if ( index == NOSRQ_INDEX_RING_SIZE) {

no space between ( and index please.

 > +               printk(KERN_WARNING "NOSRQ supports a max of %d RC "
 > +                      "QPs. That limit has now been reached\n",
 > +                      NOSRQ_INDEX_RING_SIZE);

ipoib_warn() instead of printk?  Also isn't this going to flood logs
if the remote side keeps trying to connect?

 > +       ret = ipoib_cm_modify_rx_qp(dev, cm_id, p->qp, psn);
 > +       if (ret) {
 > +               ipoib_warn(priv, "ipoib_cm_modify_rx_qp() failed %d\n", 
 > ret);
 > +               goto err_modify_nosrq;
 > +       }

It's good to goto to unwind code, but in this case you just have a
return at err_modify_nosrq -- why not just return directly?  However
you seem to leak rx_ring here, so it would be better to use the unwind
code more consistently instead of using return later.

 > +       for (i = 0; i < ipoib_recvq_size; ++i) {
 > +               if (!ipoib_cm_alloc_rx_skb(dev, i << 32 | index,
 > +                                          IPOIB_CM_RX_SG - 1,
 > +                                          p->rx_ring[i].mapping)) {
 > +                       ipoib_warn(priv, "failed to allocate receive "
 > +                                  "buffer %d\n", i);
 > +                       ipoib_cm_dev_cleanup(dev);
 > +                       /* Free rx_ring previously allocated */

this comment doesn't tell me anything I couldn't see for myself

 > +                       kfree(p->rx_ring);
 > +                       return -ENOMEM;
 > +               }
 > +
 > +               /* Can we call the nosrq version? */
 > +               if (ipoib_cm_post_receive(dev, i << 32 | index)) {
 > +                       ipoib_warn(priv, "ipoib_ib_post_receive "
 > +                                  "failed for  buf %d\n", i);
 > +                       ipoib_cm_dev_cleanup(dev);

seems like you're missing the call to kfree(p->rx_ring) here?
this code could probably benefit from a goto to unwind code.

 > +                       return -EIO;
 > +               }


 > +       } /* end for */

and another useless comment here...

+       if (priv->cm.srq == NULL) { /* NOSRQ */

I prefer "if (!priv->cm.srq)" to "== NULL".  Also I don't think this
comment tells me anything.

+               psn = random32() & 0xffffff;
+               if (ret = allocate_and_post_rbuf_nosrq(cm_id, p, psn))
+                       goto err_modify;
+       } else { /* SRQ */
+               p->rx_ring = NULL; /* This is used only by NOSRQ */
+               psn = random32() & 0xffffff;
+               ret = ipoib_cm_modify_rx_qp(dev, cm_id, p->qp, psn);
+               if (ret)
+                       goto err_modify;
+       }

move the psn assignment out of the if?  

 > +       struct ipoib_dev_priv *priv = netdev_priv(dev);
 > +       int ret;
 > + 
 > +

please avoid double spacing.

 > -       attr.srq = priv->cm.srq;
 > +       if (priv->cm.srq)
 > +               attr.srq = priv->cm.srq;
 > +       else
 > +               attr.srq = NULL;

adding the if seems like pure obfuscation here...

+       if (attr.max_srq)
+               supports_srq = 1; /* This device supports SRQ */
+       else {
+               supports_srq = 0; 
        }

I don't see what the supports_srq temporary variable buys you.  Also
please don't put { } around one-line blocks.

 > +               priv->cm.rx_index_ring = kzalloc(NOSRQ_INDEX_RING_SIZE * 
 > +                                        sizeof *priv->cm.rx_index_ring,
 > +                                        GFP_KERNEL);
 > +       } 

Handle allocation failure here?

 - R.