Message-ID: <4651DAC1.7050604@intel.com>
Date:	Mon, 21 May 2007 10:45:37 -0700
From:	"Kok, Auke" <auke-jan.h.kok@...el.com>
To:	Milton Miller <miltonm@....com>
CC:	David Acker <dacker@...net.com>, Jeff Garzik <jgarzik@...ox.com>,
	Scott Feldman <sfeldma@...ox.com>,
	e1000-devel@...ts.sourceforge.net,
	Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
	John Ronciak <john.ronciak@...el.com>,
	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH] fix e100 rx path on ARM (was [PATCH] e100 rx: or s and
 el bits)

Milton Miller wrote:
> On May 18, 2007, at 12:11 PM, David Acker wrote:
> 
>> Kok, Auke wrote:
>>> 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.
>>
>> On the ARM, their is a race condition between software allocating a 
>> new receive
>> buffer and hardware writing into a buffer.  The two race on touching 
>> the last
>> Receive Frame Descriptor (RFD).  It has its el-bit set and its next 
>> link equal
>> to 0.  When hardware encounters this buffer it attempts to write data 
>> to it
>> and then update Status Word bits and Actual Count in the RFD.  At the 
>> same time
>> software may try to clear the el-bit and set the link address to a new 
>> buffer.
>> Since the entire RFD is once cache-line, the two write operations can
>> collide.  This can lead to the receive unit stalling or freed receive 
>> buffers
>> getting written to.
>>
>> The fix is to set the el-bit on and the size to 0 on the next to last 
>> buffer
>> in the chain.  When the hardware encounters this buffer it stops and 
>> does
>> not write to it at all.  The hardware issues an RNR interrupt with the
>> receive unit in the No Resources state.  When software allocates 
>> buffers,
>> it can update the tail of the list because it knows the hardware will 
>> stop
>> at the buffer before it.  Once it has a new next to last buffer 
>> prepared,
>> it can clear the el-bit and set the size on the previous one.  The 
>> race on
>> this buffer is safe since the link already points to a valid next 
>> buffer.
>> If the hardware sees the el-bit cleared without the size set, it will
>> move on to the next buffer and complete that one in error.  If it sees
>> the size set but the el-bit still set, it will complete that buffer
>> and then RNR interrupt and wait.
>>
>>
>> Signed-off-by: David Acker <dacker@...net.com>
>>
> 
> 
> This patch doesn't apply.  It appears white space damaged somewhere:
> 
> (1) blank lines in diff are empty not <space>
> (2) unchanged lines starting with tab are <space><space><tab>
> 
> After fixing the above I still get:
> 
> patching file drivers/net/e100.c
> Hunk #1 FAILED at 285.
> Hunk #4 FAILED at 1749.
> Hunk #8 FAILED at 1865.
> Hunk #10 succeeded at 1965 with fuzz 1.
> Hunk #11 succeeded at 1982 with fuzz 1.
> 3 out of 14 hunks FAILED -- saving rejects to file 
> drivers/net/e100.c.rej
> 
> although I haven't figured out what is wrong.
> 
> 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
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists