[openib-general] [PATCH] IPoIB CM Experimental support

[Roland Dreier]

OK, just a very quick scan through:

 > +ib_ipoib-$(INFINIBAND_IPOIB_CM)			+= ipoib_cm.o

Does this actually work in the Makefile without the CONFIG_ prefix?  I
don't think it's intended anyway...

 > +#define IPOIB_QPN(ha) (be32_to_cpup((__be32 *) ha) & 0xffffff)
 > +
 > +

trim one of these blank lines...

 > +	IPOIB_CM_MTU              = 0x10000 - 0x10, /* padding to align header to 16 */
 > +	IPOIB_CM_BUF_SIZE         = IPOIB_CM_MTU  + IPOIB_ENCAP_LEN,

 > +	skb = dev_alloc_skb(IPOIB_CM_BUF_SIZE + 12);

This means every RX buffer is an order-4 allocation (with 4K pages).
I think that has to be fixed for us to consider this, or else
connected mode is basically useless on a loaded system.

 > +	IPOIB_FLAG_NETIF_STOPPED  = 9,

I can't follow what this is used for.  Can you explain in small words?

Why is this:

 > +struct ipoib_cm_dev_priv {
 > +	struct ib_cq  	    *cq;
 > +	struct ib_srq  	    *srq;
 > +	struct ipoib_rx_buf *srq_ring;
 > +	struct ib_cm_id     *id;
 > +	struct list_head     passive_ids;
 > +	struct work_struct   start_task;
 > +	struct work_struct   reap_task;
 > +	struct list_head     start_list;
 > +	struct list_head     reap_list;
 > +	struct ib_wc         ibwc[IPOIB_NUM_WC];
 > +};
 > +
 >  /*
 >   * Device private locking: tx_lock protects members used in TX fast
 >   * path (and we use LLTX so upper layers don't do extra locking).
 > @@ -179,6 +226,8 @@ struct ipoib_dev_priv {
 >  	struct list_head child_intfs;
 >  	struct list_head list;
 >  
 > +	struct ipoib_cm_dev_priv cm;
 > +

outside of CONFIG_INFINIBAND_IPOIB_CM (so struct ipoib_dev_priv is
significantly larger even with CM off), but this:

 > +#ifdef CONFIG_INFINIBAND_IPOIB_CM
 > +
 > +#define IPOIB_FLAGS_RC          0x80
 > +#define IPOIB_FLAGS_UC          0x40

is inside?

 > +#define IPOIB_CM_ENABLED(ha)   (ha[0] & IPOIB_FLAGS_RC)

Should that be

+#define IPOIB_CM_ENABLED(ha)   (ha[0] & (IPOIB_FLAGS_RC | IPOIB_FLAGS_UC))

(I know you don't implement UC at all but if you're going to define
the flag, there's no point in setting a trap for the future...)

 - R.