Date:	Wed, 23 May 2007 09:02:06 -0500
From:	Milton Miller <miltonm@....com>
To:	David Acker <dacker@...net.com>
Cc:	Jeff Garzik <jgarzik@...ox.com>,
	"Kok, Auke" <auke-jan.h.kok@...el.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, Scott Feldman <sfeldma@...ox.com>
Subject: Re: [PATCH] fix e100 rx path on ARM (was [PATCH] e100 rx: or s and el bits)

I tried to remove anything we were in agreement with.

On May 22, 2007, at 5:07 PM, David Acker wrote:
> Milton Miller wrote:

> Many of the issues you bring have been in the e100 for some time.  If 
> you ignore the s-bit patch, I basically did the the following:
> moved the el-bit to before the last buffer so that the last buffer was 
> protected during chaining.
> set the el-bit buffer (next-to-last) size to 0 so that it is not 
> written to while we are writing to it.
>
> This seemed the only way to protect the buffers we are changing while 
> having code to stay ahead of the hardware.

I agree with this part of the approach.  I just think we need
a bit more work on the "what to do when we are ready for
hardware to not stop" part.

>> But the bigger point is it didn't address the holes I identified.
>>
>> I also see that the driver sets the size from 0 back to frame
>> size.  While there is a wmb() between the removal of EL and the
>> restore of the frame size, there is only one pci_sync for both
>> of them.  Since the size is in a separate word, depending on skb
>> alignment it may be in a different cache line and we may end up
>> with all orderings being visible to the device.
>
> Hmmm...interesting.  The start of the data of the skb may not be 
> aligned on a cache line.  Ok DMA experts...what happens when you sync 
> across cache lines?  It should dump both of them, right?  I guess the 
> orderings could vary...hence why I saw completions with size set but 
> the el-bit still set.  So perhaps the skb alloc needs to be aligned by 
> cache line?     This issue existed before my patches as well.

The sync is required to push both cache lines, but there is no
ordering guarantee.  This probably is why you saw size and el
set.  Aligning the RFD to a cache line conflicts with aligning
the payload (IP header and data) to a word boundary, and
depending on cache line size it may be impossible to do both.

And it won't fix the hole for coherent dma machines.

It wasn't an issue before because we never set two fields.

>> This patch adds a lot of code that checks if the next RFD has
>> EL set and assumes that if it sees the next frame to clean has
>> EL set then the device will have seen it and entered the stopped
>> state.   In theory, the device might be held off on the PCI bus
>> and not have yet read the descriptor and stopped.   In addition,
>> while it anticipates the RNR interrupt and restarts the receiver
>> from the clean process, it doesn't clear the pending interrupt
>> and the cpu will still take the interrupt, although it will read
>> the status and not actually restart the RU.
>
> So the device is just before the el-bit buffer and stops.  We get a 
> poll call with interrupts disabled and see the el-bit buffer and 
> decide we need to restart.  First we alloc new buffers and move the 
> el-bit/size 0 down.  The restart occurs on the next poll (interrupts 
> are not turned on if we did any work).  In theory, the hardware could 
> have read the buffer that no longer has a size of 0 and tried to use 
> it when we hit it with the start.  I am not sure how the hardware 
> handles this.  Perhaps it is ignored and thus harmless.  We could poll 
> the hardware to check before actually sending the start...it adds an 
> extra pci operation but would avoid a command that is illegal in the 
> manual.
>
> If we ignore a buffer with the el-bit set but without the complete 
> bit, we must wait for the hardware to RNR interrupt.  I have found 
> that when a buffer has both the el-bit set and the size set to 0, it 
> will not set the complete bit on that buffer.  This was part of the 
> reason why my first patch had hardware just using the list of buffers 
> given all the way up until the end.


My current reading of the manual is that the C bit will not be
set on an RFD that is size 0.  It goes on to processes EL and
S, and decides to stop and interrupt RNR or suspend, or just
go to the next packet.

>> 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 hardware is only restarted when we enter the RU_SUSPENDED state. 
> This only happens when we:
> 1) get an RNR interrupt
> 2) get an EL-bit buffer without a completion
> 3) get an el-bit buffer with a completion (hardware saw size set but 
> not el-bit clear)
>
> State 2) seems to be the problem.  Get rid of that and we wait for an 
> interrupt to tell us it saw the buffer in most cases.  Of course, then 
> we always wait.

If we poll the device, I think it will be rare that it
is not stopped when we see the EL bit bit it has not
yet read the next RFD.  But it is a race and I think
a device read is worth avoiding something prohibited
in the manual.

> If we do not reclaim the size from 0, but clear the el-bit, that 
> buffer will always return an error.

I disagree.  If the size is 0, it is a special case and no
error will be generated, instead the RU will just process
EL and S then proceed to the next buffer.

If the size was smaller than the incoming packet but not 0
then it would be completed in error.

It does make are "did the device go past" logic harder.
If the buffer ever was 0, then we need to see if the
next never-zero buffer has completed, and reclaim all
the buffers in between.

>> 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.
>
>> This window also means that the device may have skipped this
>> previously 0-length descriptor and gone on to the next one,
>> so we will stick waiting for the device to write to this
>> descriptor while it went on to the next one.
>
> If the hardware sees both size 0 and EL, it RNR interrupts, does not 
> write to the buffer at all and stops.
>
> If the hardware sees size 0 and !EL, (which I never saw in testing but 
> could happen) it would return the buffer with an error and continue. 
> This is ok since we already have written a new size 0 and EL.  Our 
> polling code would just find a packet completed in error.

[A bit repetitious since we are replying to two threads]

Again, I think there is no completion, error or not, and our
clean will get stuck while the hardware goes on.

> If the hardware sees size set and EL, it RNR interrupts, and does 
> write data and status to the buffer, and then stops.  I have see this 
> one occur and it isn't a problem.
>
> If hardware sees size set and !EL, it writes data and continues.
>
>
>> As far as the RU_SUSPENDED, I don't think we need it, instead
>> we should poll the device.
> RU_SUSPENDED is pretty much used as it was before all the patches. The 
> only waste I found is that if we find the EL-bit while interrupts are 
> disabled (say during a NAPI poll), we set RU_SUSPENDED and then have 
> to wait for the next poll to figure out that we need to restart the 
> receiver.

I have no problem with reading the device when we think it may
have hit EL and restarting on this poll.  I even prefer it.

If we do restart, we should take the time to ack the RNR
interrupt so we don't process it again.

>> 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.
>
> Can you send a patch or pseudo-code?  It would help to see some main 
> parts:
> 1. blank_rfd setup
> 2. single buffer alloc
> 3. el-bit handling (when do we set it and when do we clear it)
> 4. size handling (when do we set it and when do we clear it)

I was afraid you were going to ask for code :-)   I'll try
to find some time to write some.   I'll need to look for
an adapter to do any testing.  They exist in some of the
machines at work.

> What if we just fixed the alignment of rfd to make sure it is cache 
> aligned?  Then we know our syncs will be a single cache flush.

That doesn't help in the coherent dma case; the device can still
read in between, and we need to handle the !EL size 0 case.

Actually, I thought about it some more after yesterdays note.
We can set the size !0 after making sure the device sees EL
(wmb and pci_sync_for_device), as long as we remember this
buffer used to be size 0 when doing RX clean, and look ahead
for another complete packet as long as the size was ever 0.

My proposal was just to use "size is still 0" as that flag,
wasting the space allocated for that skb.

Another aproach is to always allocate a rfd-only packet
at the beginning of the loop, and inserting th

-
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