<br><font size=2 face="sans-serif">Hello Roland,</font>
<br>
<br><font size=2 face="sans-serif">Thanks.</font>
<br>
<br><font size=2><tt>Roland Dreier <rdreier@cisco.com> wrote on 04/03/2006
10:50:37 PM:<br>
<br>
> Sorry I hadn't gotten a chance to read this over until now...<br>
> <br>
>  > -       IPOIB_RX_RING_SIZE    
   = 128,<br>
>  > -       IPOIB_TX_RING_SIZE    
   = 64,<br>
>  > +       IPOIB_SENDQ_SIZE      
   = 64,<br>
>  > +       IPOIB_RECVQ_SIZE      
   = 128,<br>
> <br>
> Can you explain again why it's a good idea to rename these?  Is
the<br>
> name "sendq_size" really clearer than "tx_ring_size,"
especially in<br>
> the context of a network driver?<br>
</tt></font>
<br><font size=2><tt>tx_ring and rx_ring are not good names for IPoIB.
tx_ring in IPoIB is a place</tt></font>
<br><font size=2><tt>to save pointers and rx_ring a place to allocate skb
buffs. I am working on </tt></font>
<br><font size=2><tt>a patch to remove tx_ring and replace rx_ring with
a list, which would </tt></font>
<br><font size=2><tt>reduce some tx_ring overhead. </tt></font>
<br>
<br><font size=2><tt>> <br>
>  > +       int     sendq_size;<br>
>  > +       int     recvq_size;<br>
> <br>
> Why does every device need a private copy of the ring sizes?  It
seems<br>
> better to just use ipoib_sendq_size and ipoib_recvq_size directly
--<br>
> round them up in the module init function, and I guess mark them <br>
> __read_mostly.<br>
> <br>
</tt></font>
<br><font size=2><tt>Michael suggested to save them in the priv before.
I did some test and didn't</tt></font>
<br><font size=2><tt>see the difference. I wil move them out of priv.</tt></font>
<br>
<br><font size=2><tt>> I wonder if it's also worth having masks to avoid
subtracting 1 every<br>
> time the driver does something like<br>
> <br>
>  > +       tx_req = &priv->tx_ring[priv->tx_head
& (priv->sendq_size - 1)];<br>
> <br>
</tt></font>
<br><font size=2><tt>sounds good.</tt></font>
<br>
<br><font size=2><tt>>  > +#define IPOIB_MAX_QUEUE_SIZE  
4096    /* max is 4k */<br>
>  > +#define IPOIB_MIN_QUEUE_SIZE    64    
/* min is 64 */<br>
> <br>
> Where do these limits come from?  Why shouldn't someone be able
to use<br>
> a bigger or smaller ring?<br>
> <br>
</tt></font>
<br><font size=2><tt>Done some test, too smaller value would cause network
problem,</tt></font>
<br><font size=2><tt>too big value is not helpful. 1K is sufficient in
my environment.</tt></font>
<br>
<br><font size=2><tt>We could use max 8k, min 32 instead.</tt></font>
<br>
<br><font size=2><tt>Which values do you suggest?</tt></font>
<br>
<br><font size=2><tt>>  >          
      printk(KERN_WARNING "%s: failed to allocate RX
<br>
> ring (%d entries)\n",<br>
>  > -                
     ca->name, IPOIB_RX_RING_SIZE);<br>
>  > +                
     ca->name, priv->sendq_size);<br>
> <br>
> Looks like this should be recvq_size, not sendq_size.<br>
</tt></font>
<br><font size=2><tt>Yes, you are right.</tt></font>
<br>
<br><font size=2><tt>> <br>
>  > +       printk(KERN_INFO "%s: RX_RING_SIZE
is set to %d entries\n",<br>
>  > +              ca->name,
priv->recvq_size);<br>
> <br>
> Seems kind of chatty -- I think anyone who cared could just look at<br>
> the module parameter in sysfs.<br>
> <br>
>  - R.<br>
</tt></font>
<br><font size=2><tt>Ok.</tt></font>
<br>
<br><font size=2 face="sans-serif">Thanks<br>
Shirley Ma<br>
IBM Linux Technology Center<br>
15300 SW Koll Parkway<br>
Beaverton, OR 97006-6063<br>
Phone(Fax): (503) 578-7638</font>