[openib-general] Re: [PATCH] repost: IPoIB queue size tune patch

[Roland Dreier]

Sorry I hadn't gotten a chance to read this over until now...

 > -       IPOIB_RX_RING_SIZE        = 128,
 > -       IPOIB_TX_RING_SIZE        = 64,
 > +       IPOIB_SENDQ_SIZE          = 64,
 > +       IPOIB_RECVQ_SIZE          = 128,

Can you explain again why it's a good idea to rename these?  Is the
name "sendq_size" really clearer than "tx_ring_size," especially in
the context of a network driver?

 > +       int     sendq_size;
 > +       int     recvq_size;

Why does every device need a private copy of the ring sizes?  It seems
better to just use ipoib_sendq_size and ipoib_recvq_size directly --
round them up in the module init function, and I guess mark them __read_mostly.

I wonder if it's also worth having masks to avoid subtracting 1 every
time the driver does something like

 > +       tx_req = &priv->tx_ring[priv->tx_head & (priv->sendq_size - 1)];

 > +#define IPOIB_MAX_QUEUE_SIZE   4096    /* max is 4k */
 > +#define IPOIB_MIN_QUEUE_SIZE    64     /* min is 64 */

Where do these limits come from?  Why shouldn't someone be able to use
a bigger or smaller ring?

 >                 printk(KERN_WARNING "%s: failed to allocate RX ring (%d entries)\n",
 > -                      ca->name, IPOIB_RX_RING_SIZE);
 > +                      ca->name, priv->sendq_size);

Looks like this should be recvq_size, not sendq_size.

 > +       printk(KERN_INFO "%s: RX_RING_SIZE is set to %d entries\n",
 > +              ca->name, priv->recvq_size);

Seems kind of chatty -- I think anyone who cared could just look at
the module parameter in sysfs.

 - R.