[ofa-general] IPOB CM (NOSRQ) [PATCH V7] patch resubmit

[Roland Dreier]

There's still some rather obvious problems with this patch.  It would
really help if you would read over your patch again I think... anyway:

 > +#define CM_PACKET_SIZE (1ul << 16)

This duplicates IPOIB_CM_MTU I think... certainly it needs to be kept
in sync with it somehow.

 > @@ -564,10 +574,9 @@ static inline void ipoib_cm_skb_too_long
 >  	dev_kfree_skb_any(skb);
 >  }
 >  
 > -static inline void ipoib_cm_handle_rx_wc(struct net_device *dev, struct ib_wc *wc)
 > +void ipoib_cm_handle_rx_wc(struct net_device *dev, struct ib_wc *wc)

Why is this change here?  (This is in the CONFIG_INFINIBAND_IPOIB_CM=n
part of ipoib.h)

 >  }
 > -
 >  #endif

Please try to avoid adding extraneous noise to your patch... it makes
it harder to focus on the real content.

 > +int max_rc_qp = NOSRQ_INDEX_TABLE_SIZE;
 > +int max_recv_buf = 1024; /* Default is 1024 MB */
 > +
 > +module_param_named(nosrq_max_rc_qp, max_rc_qp, int, 0644);
 > +MODULE_PARM_DESC(nosrq_max_rc_qp, "Max number of NOSRQ RC QPs supported");
 > +
 > +module_param_named(max_receive_buffer, max_recv_buf, int, 0644);
 > +MODULE_PARM_DESC(max_receive_buffer, "Max Receive Buffer Size in MB");
 > +
 > +atomic_t current_rc_qp; /* Active number of RC QPs for NOSRQ */

everything here can be static I think ("make namespacecheck" might be
worth running).  And you can use ATOMIC_INIT() instead of putting the
initialization into code.

 > -		ipoib_warn(priv, "post srq failed for buf %d (%d)\n", id, ret);
 > +		ipoib_warn(priv, "post srq failed for buf %ld (%d)\n", id, ret);

extra noise here (and still wrong -- id might be long long on some
architectures).

 > -		.event_handler = ipoib_cm_rx_event_handler,

why?  seems harmless to just leave this alone for all QPs even if an
SRQ isn't attached.

 > +	recv_mem_used = (u64)ipoib_recvq_size *
 > +			(u64)atomic_inc_return(&current_rc_qp)
 > +			* CM_PACKET_SIZE; /* packets are 64K */

packets might not always be 64K ... just let CM_PACKET_SIZE document
itself (or pick a better name if you think it needs to be clearer).

 > +	if ((index == max_rc_qp) ||
 > +	( recv_mem_used >= max_recv_buf * (1ul << 20))) {

formatting went awry here...

 > +		spin_unlock_irq(&priv->lock);
 > +		ipoib_warn(priv, "NOSRQ has reached the configurable limit "
 > +			   "of either %d RC QPs or, max recv buf size of "
 > +			   "0x%x MB\n", max_rc_qp, max_recv_buf);

 > +		ib_send_cm_rej(cm_id, IB_CM_REJ_NO_QP, NULL, 0, NULL, 0);
 > +		ret = -EINVAL;
 > +		goto err_alloc_and_post;

there's a bug here... you never undo the atomic_inc() of the number of
RC QPs even though you exit without creating a new connection.

 > -	if (!p)
 > +	if (!p) {
 > +		printk(KERN_WARNING "Failed to allocate RX control block when "
 > +		       "REQ arrived\n");
 >  		return -ENOMEM;
 > +	}

more unrelated changes... (feel free to send these as separate
patches)

 >  		kfree(p);
 >  	}
 >  
 > +
 >  	cancel_delayed_work(&priv->cm.stale_task);
 >  }

extra noise in the patch

 > +		if (!priv->cm.srq) {
 > +			atomic_dec(&current_rc_qp);
 > +		}

no need for { } here

 > +	/* We increase the size of the CQ in the NOSRQ case to prevent CQ
 > +	 * overflow. Every new REQ creates a new RX QP and each QP has an
 > +	 * RX ring associated with it. Therefore we could have
 > +	 * NOSRQ_INDEX_TABLE_SIZE*ipoib_recvq_size + ipoib_sendq_size CQEs
 > +	 * in a CQ.
 > +	 */
 > +	if (!priv->cm.srq)
 > +		size += (NOSRQ_INDEX_TABLE_SIZE -1) * ipoib_recvq_size;

only need to do this if CM is enabled

space after - here please too.

that's just from a quick skim of the patch...