Re: re(4) driver dropping packets when reading NFS files

On Fri, Nov 05, 2010 at 07:44:56PM -0400, Rick Macklem wrote:
> > On Thu, Nov 04, 2010 at 09:31:30PM -0400, Rick Macklem wrote:
> > > >
> > > > If the counter was not wrapped, it seem you lost more than 10% out
> > > > of
> > > > total RX frames. This is a lot loss and there should be a way to
> > > > mitigate it.
> > > >
> > > I've attached a patch (to the if_re.c in head, not your patched
> > > variant)
> > > that works a lot better (about 5Mbytes/sec read rate). To get that,
> > > I
> > > had to disable msi and not clear the RL_IMR register in re_intr(). I
> > > suspect that a packet would be received between when the bits in
> > > RL_IMR
> > > were cleared and when they were set at the end of re_int_task() and
> > > those
> > > were getting lost.
> > >
> > > This patch doesn't completely fix the problem. (I added your stats
> > > collecting
> > > stuff to the if_re.c in head and attached the result, which still
> > > shows some lost packets. One
> > > thought is clearing the bits in RL_ISR in re_intr() instead of
> > > re_int_task(),
> > > but then I can't see a good way to pass the old value of the status
> > > reg.
> > > through to re_int_task()?
> > >
> > 
> > Hmm, I still don't understand how the patch mitigates the issue. :-(
> > The patch does not disable interrupts in interrupt handler so
> > taskqueue runs with interrupt enabled. This may ensure not loosing
> > interrupts but it may also generate many interrupts. By chance, did
> > you check number of interrupts generated with/without your patch?
> > 
> I agree. I think all it did was create more interrupts so that re_rxeof()
> got called more frequently. (see below)
> 
> > The only guess I have at the moment is the writing RL_IMR in
> > interrupt handler at the end of taskqueue might be not immediately
> > reflected so controller can loose interrupts for the time window.
> > Would you try attach patch and let me know it makes any difference?
> > 
> Nope, it didn't make any difference.
> 
> I've added a counter of how many times re_rxeof() gets called, but then
> returns without handling any received packets (I think because
> RL_RDESC_STAT_OWN is set on the first entry it looks at in the rcv. ring.)
> 
> This count comes out as almost the same as the # of missed frames (see
> "rxe did 0:" in the attached stats).
> 
> So, I think what is happenning about 10% of the time is that re_rxeof()
> is looking at the ring descriptor before it has been "updated" and then
> returns without handling the packet and then it doesn't get called again
> because the RL_ISR bit has been cleared.
> 
> When "options DEVICE_POLLING" is specified, it works ok, because it calls
> re_rxeof() fairly frequently and it doesn't pay any attention to the RL_ISR
> bits.
> 

That's one of possible theory. See below for another theory.

> Now, I don't know if this is a hardware flaw on this machine or something
> that can be fixed in software? I know diddly about the current driver

I highly doubt it could be hardware issue.

> setup, but I assume something like this has to happen?
> - chip (or PCIe handler) forces the DMA of the descriptor change to RAM
> - then posts the interrupt
> - and the driver must read the up to date descriptor from RAM
> --> I notice that "volatile" isn't used anywhere in the driver. Wouldn't
>     the descriptor ring memory have to be defined "volatile" somehow?
>     (I don't know how to do this correctly?)

Because driver check different descriptor location in each loop,
adding volatile keyword wouldn't help. You can verify that whether
that makes any difference though.

Another possibility I have in mind is the controller would have
reported RL_ISR_RX_OVERRUN but re(4) didn't check that condition.
The old data sheet I have does not clearly mention when it is set
and what other bits of RL_ISR register is affected from the bit.
If RL_ISR_RX_OVERRUN bit is set when there are no more free RX
descriptors available to controller and RL_ISR_RX_ERR is not set
when RL_ISR_RX_OVERRUN is set, re(4) have ignored that event.
Because driver ignored that error interrupt, the next error
interrupt would be RL_ISR_FIFO_OFLOW error and this time it would
be served in re_rxeof() and will refill RX buffers.
However driver would have lost lots of frames received between the
time window RL_ISR_RX_OVERRUN error and RL_ISR_FIFO_OFLOW error.

If this theory is correct, the attached patch may mitigate the
issue.

> 
> So, if you can think of anything that will ensure that re_rxeof() reads
> "up to date" descriptor ring entries, please let me know.
> 

It's job of bus_dma(9) and I don't think barrier instructions would
be helpful here as I don't see out-of-order execution in RX
handler.

> Otherwise, I can live with "options DEVICE_POLLING".
> 

Sorry, I can't live with DEVICE_POLLING on re(4). :-(
Let's kill driver bug. No one reported this kind of issue so far
and I guess most users took it granted for the poor performance
because they are using low end consumer grade controller.

> Thanks for your help, rick

> re0 statistics:
> Transmit good frames : 101346
> Receive good frames : 133390
> Tx errors : 0
> Rx errors : 0
> Rx missed frames : 14394
> Rx frame alignment errs : 0
> Tx single collisions : 0
> Tx multiple collisions : 0
> Rx unicast frames : 133378
> Rx broadcast frames : 0
> Rx multicast frames : 12
> Tx aborts : 0
> Tx underruns : 0
> rxe did 0: 14359