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

[Shirley Ma]

Hello Roland,

Thanks.

Roland Dreier <rdreier at cisco.com> wrote on 04/03/2006 10:50:37 PM:

> 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?

tx_ring and rx_ring are not good names for IPoIB. tx_ring in IPoIB is a 
place
to save pointers and rx_ring a place to allocate skb buffs. I am working 
on 
a patch to remove tx_ring and replace rx_ring with a list, which would 
reduce some tx_ring overhead. 

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

Michael suggested to save them in the priv before. I did some test and 
didn't
see the difference. I wil move them out of priv.

> 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)];
> 

sounds good.

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

Done some test, too smaller value would cause network problem,
too big value is not helpful. 1K is sufficient in my environment.

We could use max 8k, min 32 instead.

Which values do you suggest?

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

Yes, you are right.

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

Ok.

Thanks
Shirley Ma
IBM Linux Technology Center
15300 SW Koll Parkway
Beaverton, OR 97006-6063
Phone(Fax): (503) 578-7638
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/general/attachments/20060404/c4b260cd/attachment.html>