On May 21, 2007, at 12:45 PM, Kok, Auke wrote:

Milton Miller wrote:

quoted

On May 18, 2007, at 12:11 PM, David Acker wrote:

quoted

Kok, Auke wrote:

quoted

First impression just came in: It seems RX performance is dropped 
to 10mbit. TX is unaffected and runs at 94mbit/tcp, but RX the new 
code seems to misbehave and  fluctuate, dropping below 10mbit after 
a few netperf runs and staying there...
ideas?

I found the problem.  Another casualty of working with two different 
kernels at once...arg.
The blank rfd needs to have its el-bit clear now.  Here is the new 
and improved patch.

...

quoted

Proceeding with the review:
Coding style:
(1) if body on seperate line.
(2) space after if before (
(3) The other enums in this driver are not ALL_CAPS
(4) This driver doesn't do CONSTANT != value but value != enum
     (see nic->mac for examples)

I sent Milton my copy of this patch which has these style issues 
corrected and
applies cleanly to a recent git tree. If anyone else specifically 
wants a copy
let me know.

Auke

It addressed 1 and 2, and applies, but did not address 3 and 4.


But the bigger point is it didn't address the holes I identified.

I think we need to change the logic to reclaim the size from 0
only if we are restarting, and make rx_indicate look ahead to
rx->next if it encounters a !EL size 0 buffer.  Without this we
are doing a "prohibited" rx_start to a running machine.  The
device can still see this size 0 !EL state.  Also we will get
stuck when the device finds the window between the two writes.

We can remove some register pressure by finding old_before_last_rfd
when we are ready to use it, just comparing old_before_last_rx
to new.

Also, as I pointed out, the rx_to_start change to start_reciever
is compicated and unnecessary, as rx_to_clean can always be used
and it was the starting point before the changes.

As far as the RU_SUSPENDED, I don't think we need it, instead
we should poll the device.

Here is my proposal:
rx_indicate can stop when it hits the packet with EL.  If it
hits a packet with size 0, look ahead to rx->next to see if
it is complete, if so complete this one otherwise leave it
as next to clean.  After the rx_indicate loop, try to allocate
more skbs.  If we are successful, then fixup the before-next
as we do now.  Then check the device status for RNR, and if
its stopped then set rx_to_clean rfd size back to ETH_LEN
and restart the reciever.

This does have a small hole: if we only add one packet at
a time we will end up with all size 0 descriptors in the
lopp.   We can detect that and not remove EL from the old
before-next unless we are restarting.  That would imply
moving the status poll before we allocate the list.

milton