Message-ID: <465369AF.8080508@roinet.com>
Date:	Tue, 22 May 2007 18:07:43 -0400
From:	David Acker <dacker@...net.com>
To:	Milton Miller <miltonm@....com>
CC:	"Kok, Auke" <auke-jan.h.kok@...el.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:
>>> 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.
> 

Sorry about the style bugs.  I will be more careful about that next time.

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.

(3) The driver had these all caps constants before my patch.  They went 
away with the s-bit patch.  I just put them back.  I agree they stick 
out but I wanted to leave style changes out of a bug fix patch.

I agree about the name of the constant, RU_SUSPENDED.  It is not 
accurate and I had a patch to fix this when I experimented with using 
the S-bit.

> 
> 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.


> 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.

> 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 do not reclaim the size from 0, but clear the el-bit, that buffer 
will always return an error.

 > 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.

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.


> 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.
Sure.

> 
> 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.
This was in the driver before my patch and the s-bit patch.  I agree 
that it could go away as rx_to_clean should not be capable of changing.

> 
> 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.

> 
> 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)


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.
-Ack
-
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