Re: [patch] ns83820 0.17 (Re: Broadcom 5700/5701 Gigabit Ethernet Adapters)

[Benjamin LaHaise]

On Thu, Mar 14, 2002 at 04:54:03AM -0500, Jeff Garzik wrote:
> Comments:
> 
> 1) What were the test conditions leading to your decision to decrease 
> the RX/TX ring count?  I'm not questioning the decision, but looking to 
> be better informed...  other gigabit drivers normally have larger rings. 
>  I also wonder if the slowdown you see could be related to a small-sized 
> FIFO on the natsemi chips, that would probably not be present on other 
> gigabit chips.

Smaller rings lead to better thruput, especially on the slower cpus.  Not 
that in part the slowness was caused by having slab debugging enabled.  
Turning slab debugging off brought the p3s up to ~500mbit and the athlons 
over 900.

> 2) PCI latency timer is set with pci_set_master(), as in:  if it's not 
> correctly set, fix it up.  If it's correctly set, leave it alone and let 
> the user tune in BIOS Setup.

Ah.  That part is something I was thinking of deleting, and now will do.

> 3) Seeing "volatile" in your code.  Cruft?  volatile's meaning change in 
> recent gcc versions implies to me that your code may need some addition 
> rmb/wmb calls perhaps, which are getting hidden via the driver's 
> dependency on a compiler-version-specific implementation of "volatile."

Paranoia during writing.  I'll reaudit.  That said, volatile behaviour 
is not compiler version specific.

> 4) Do you really mean to allocate memory for "REAL_RX_BUF_SIZE + 16"? 
>  Why not plain old REAL_RX_BUF_SIZE?

The +16 is for alignment (just like the comment says).  The hardware 
requires that rx buffers be 64 bit aligned.

> 5) Random question, do you call netif_carrier_{on,off,ok} for link 
> status manipulation?  (if not, you should...)

Ah, api updates.  Added to the todo.

> 6) skb_mangle_for_davem is pretty gross...  curious: what sort of NIC 
> alignment restrictions are there on rx and tx buffers (not descriptors)? 
>  None? 32-bit?  Ignore CPU alignment for a moment here...

tx descriptors have no alignment restriction, rx descriptors must be 
64 bit aligned.  Someone chose not to include the transistors for a 
barrel shifter in the rx engine.

> 7) What are the criteria for netif_wake_queue?  If you are waking when 
> the TX is still "mostly full" you probably generate excessive wakeups...

Hrm?  Currently it will do a wakeup when at least one packet (8 sg 
descriptors) can be sent.  Given that the tx done code is only called 
when a tx desc (every 1/4 or so of the tx queue) or txidle interrupt 
occurs, it shouldn't be that often.

		-ben
-- 
"A man with a bass just walked in,
 and he's putting it down
 on the floor."