Re: [RFR] gianfar ethernet driver

Andy Fleming wrote:

Ok, I will change the defaults.  Our performance testing indicates that 
ttcp performance with mtu=1500 drops if NAPI is enabled vs. when it is 
not.  This is for both TCP and UDP packets.  This made sense to me, as 
NAPI incurs a slight latency penalty, and for 1518 byte frames, it does 
not gain much advantage from interrupt reduction.

NAPI only begins to mitigate at the weighted traffic level.  Maybe your 
poll weight is too low?  It sounds like you still need to do some tuning 
and profiling.

For periodic but not high volume traffic, NAPI shouldn't incur latency 
penalties as you should be re-enabling interrupts shortly thereafter.

Only when you start hitting the NAPI thresholds does it start reducing 
interrupts.  At which point you should be at traffic levels where 
eliminating interrupt overhead should -decrease- your CPU usage.

Hmm...  Does it call dev->set_mac_addr()?  Because that only seems to 
copy the new address into the dev structure.  While I haven't been able 
to trace the notification chain to discover the exact sequence of events 
when someone changes the MAC address, I was thought that, in order for 
the change to take effect, the interface needed to be brought down, then 
up.  gfar_set_mac_address() is only called during gfar_open(), so I am 
not aware of any way someone else could cause it to be called without 
restarting the interface.  How should I set it up so that some other 
code could call it?

[...]

Yes, which is why gfar_change_mtu() halts the controller (thus ceasing 
all DMA activity, and freeing buffer memory), then changes the MTU, and 
then starts the controller again, causing buffers to be reallocated.  I 
feel like I'm missing something obvious.  Perhaps I don't understand 
exactly what you mean when  you say "synchronization"?  Are we talking 
about locking, or making sure the hardware is configured without 
screwing things up?  Or...what?

Through ifconfig, you can test changing MTU and MAC address while the 
interface is up.  Probably other tools (/sbin/ip?) also.

Synchronization means, what happens if all of the following occur at the 
same time:
1) RX packet received
2) net stack calls dev->hard_start_xmit() to transmit a packet
3) user decides he wants to change MAC addr/MTU

?

By synchronization, I mean that you must ensure all entry points into 
your driver are protected from racing against each other, or 
simultaneously accessing the same data structure(s).

quoted

quoted

quoted

quoted

33) merge-stopper:  mii_parse_sr().  never wait for autonegotiation to
complete.  it is an asynchronous operation that could exceed 30 
seconds.

Hmm...

quoted

quoted

34) merge-stopper:  dm9161_wait().  same thing as #33.

This may be a problem.  That function is there to work around an 
issue in the PHY, wherein if you try to configure it before it has 
come up all the way, it refuses to bring the link up.  We've sworn at 
this code many times, but there has, as of yet, not been a good 
suggestion as to how we can ensure that the 9161 is  ready before we 
configure it.

I interpret that as a driver bug :)

As common sense, regardless of phy bugs, you should not be trying to 
configure the MAC _or_ the phy in the middle of autonegotiation. 
Presumeably you are using a combination of netif_stop_queue(), 
netif_carrier_off(), and netif_device_detach() to achieve this.

Hrm... This will require some fun, then.  My current scheme for PHY 
configuration/management does not allow me to stop if autonegotiation 
isn't done.  Clearly, I will have to rework the PHY code...

I'm not sure I understand the "My current scheme" sentence.  Clearly 
RX/TX DMA is not running, if you are in the middle of autonegotiation. 
So just make efforts to ensure that the kernel doesn't try to TX packets 
during that time (netif_carrier_off and/or netif_stop_queue), and make 
sure that random user ioctls (such as change-mtu or set-mac-addr :)) do 
not stomp all over the MAC while the phy is in the middle of autoneg.

Good hardware will give you an interrupt when it link is up, otherwise 
you'll probably have to use a timer to poll for link (which isn't 
present until autoneg ends, obviously).

quoted

quoted

quoted

2) Also its pretty scary if you are doing:
txbdp->status |= TXBD_INTERRUPT for every packet.
Look at other drivers, they try to do this every few packets;
or enable tx mitigation to slow the rate  of those interupts.

I don't believe this is as dire as you think.  This bit only 
indicates that, if the conditions are right, an interrupt will be 
generated once that descriptor is processed, and its data sent.  
Conditions which mitigate that are:
1) coalescing is on, and the timer and counter have not triggered the 
interrupt yet
2) NAPI is enabled, and so interrupts are disabled after the first 
packet arrives
3) NAPI is disabled, but the driver is currently handling a previous 
interrupt, so the interrupts are disabled for now.

Even at 10/100 speeds, you really don't want to be generating one 
interrupt per Tx...

Hmm... My concern is this from Documentation/networking/driver.txt:

"For example, this means that it is not allowed for your TX
   mitigation scheme to let TX packets "hang out" in the TX
   ring unreclaimed forever if no new TX packets are sent.
   This error can deadlock sockets waiting for send buffer room
   to be freed up."

Is this no longer accurate?  Anyway, as long as coalescing is on, there 
won't be one interrupt per frame.

Hopefully your hardware provides something other than purely 
packet-based intr mitigation?  Usually there is a timeout, if no TX (or 
RX) interrupt has been sent in X time period.

	Jeff