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

[Pradeep Satyanarayana]

Thanks for the review. Some of them MST had already pointed out. I will 
respond to the additional ones that you make.

Pradeep
pradeep at us.ibm.com

Roland Dreier <rdreier at cisco.com> wrote on 05/03/2007 12:01:03 PM:

>  > +#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.

Agreed.

> 
>  > +/* 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?
> 

I will just call this an array. Nosrq will hog memory and my thought was 
that 1024 was pretty large. I envisioned using nosrq for a small number 
(maybe a 
few dozen), and so did not think it was necessary to make this a module 
paramater 
either. What do you suggest? 

> 
>  > -       rep.srq = 1;
> 
>  > +       if (priv->cm.srq)
>  > +               rep.srq = 1;
>  > +       else
>  > +               rep.srq = 0;
> 
> similarly I would rather see "rep.srq = !!priv->cm.srq"

ok

> 
>  > +       /* Allocate space for the rx_ring here */
>  > +       p->rx_ring = kzalloc(ipoib_recvq_size * sizeof *p->rx_ring,
>  > +                            GFP_KERNEL);
> 
> 
>  > +               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?

As you describe, the remote side will continue to attempt connecting and
will fail. That is a pretty serious scenario. Hence I leaned towards 
flooding
the logs rather than losing this among a ton of other messages, there may 
be
application speceific messages too. I can change this to ipoib_warn().

> 
>  > +       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.

Yes, there is a leak here -will fix that.

                     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.

Yes there is leak here too -will fix.