From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-17 23:19:11
This can be called from cp_tx_timeout() with interrupts disabled.
Spotted by Francois Romieu [off-list ref]
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-17 23:21:56
Unless we reset the RX config, on real hardware I don't seem to receive
any packets after a TX timeout.
Signed-off-by: David Woodhouse <redacted>
---
Now it does actually recover from the TX timeout, lots of the time.
Sometimes it still hits that IRQ storm, which probably explains the
apparent lockup right after the 'popf'... although I thought we handled
it more gracefully than that these days.
That's probably a race with the RX handling code, or something. I'll
try harder to reproduce the TX timeout with the debugging enabled.
Which might shed some light on this, and also on the reason why it
happens in the first place. If we're lucky.
drivers/net/ethernet/realtek/8139cp.c | 1 +
1 file changed, 1 insertion(+)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-18 11:37:20
The TX timeout handling has been observed to trigger RX IRQ storms. And
since cp_interrupt() just keeps saying that it handled the interrupt,
the machine then dies. Fix the return value from cp_interrupt(), and
the offending IRQ gets disabled and the machine survives.
Signed-off-by: David Woodhouse <redacted>
---
... and I have to make fewer trips to the cupboard under the stairs.
There follows a fix to prevent the IRQ storm from happening at all.
drivers/net/ethernet/realtek/8139cp.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-18 12:17:50
If an RX interrupt was already received but NAPI has not yet run when
the RX timeout happens, we end up in cp_tx_timeout() with RX interrupts
already disabled. Blindly re-enabling them will cause an IRQ storm.
This is somewhat less painful than it was a few minutes ago before I
fixed the return value from cp_interrupt(), but still suboptimal.
Unconditionally leave RX interrupts disabled after the reset, and
schedule NAPI to check the receive ring and re-enable them.
Signed-off-by: David Woodhouse <redacted>
---
It might seem a little odd to deliberately schedule NAPI when we know
there's going to be nothing there since we just cleared the rings.
But given that we only get here if the hardware is playing silly
buggers, I don't much like the idea of preserving the existing IntrMask
that we read from the hardware. And there's no pretty way of
determining whether NAPI is already scheduled. So just ensuring that
it *is* scheduled seems simplest. It's not like we're going to
care about the performance implications of one fruitless poll when
we've just suffered a TX timeout and reset the hardware.
In future I think I might want to fix cp_tx_timeout() to *only* reset
the TX ring anyway. Especially as I'm entirely unsure about its locking
w.r.t. cp_rx_poll()... what prevents cp_rx_poll() from running while
we're in the middle of screwing around with the RX rings?
drivers/net/ethernet/realtek/8139cp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Unless we reset the RX config, on real hardware I don't seem to
receive
quoted
any packets after a TX timeout.
Signed-off-by: David Woodhouse <redacted>
Applied.
Thanks. I'll send another batch, including the original patches 3/2 and
4/3 from this series, in reply to this message.
After which, I think we might be able to turn on TX checksumming by
default and I also have a way to implement early detection of the TX
stall I've been seeing.
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-21 14:01:52
From: David Woodhouse <redacted>
The TX timeout handling has been observed to trigger RX IRQ storms. And
since cp_interrupt() just keeps saying that it handled the interrupt,
the machine then dies. Fix the return value from cp_interrupt(), and
the offending IRQ gets disabled and the machine survives.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-21 14:02:11
From: David Woodhouse <redacted>
If an RX interrupt was already received but NAPI has not yet run when
the RX timeout happens, we end up in cp_tx_timeout() with RX interrupts
already disabled. Blindly re-enabling them will cause an IRQ storm.
This is somewhat less painful than it was a few minutes ago before I
fixed the return value from cp_interrupt(), but still suboptimal.
Unconditionally leave RX interrupts disabled after the reset, and
schedule NAPI to check the receive ring and re-enable them.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-21 14:02:36
From: David Woodhouse <redacted>
After a certain amount of staring at the debug output of this driver, I
realised it was lying to me.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-21 14:02:48
From: David Woodhouse <redacted>
When sending a TSO frame in multiple buffers, we were neglecting to set
the first descriptor up in TSO mode.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-21 14:03:03
From: David Woodhouse <redacted>
The low 16 bits of the 'opts1' field in the TX descriptor are supposed
to still contain the buffer length when the descriptor is handed back to
us. In practice, at least on my hardware, they don't. So stash the
original value of the opts1 field and get the length to unmap from
there.
There are other ways we could have worked out the length, but I actually
want a stash of the opts1 field anyway so that I can dump it alongside
the contents of the descriptor ring when we suffer a TX timeout.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-21 14:03:23
From: David Woodhouse <redacted>
We are seeing unexplained TX timeouts under heavy load. Let's try to get
a better idea of what's going on.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
@@ -157,6 +157,7 @@ enum {NWayAdvert=0x66,/* MII ADVERTISE */NWayLPAR=0x68,/* MII LPA */NWayExpansion=0x6A,/* MII Expansion */+TxDmaOkLowDesc=0x82,/* Low 16 bit address of a Tx descriptor. */Config5=0xD8,/* Config5 */TxPoll=0xD9,/* Tell chip to check Tx descriptors for work */RxMaxSize=0xDA,/* Max size of an Rx packet (8169 only) */
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-21 14:05:44
From: David Woodhouse <redacted>
This is a minor optimisation, but as a side-effect it means we can know
precisely which descriptors were already in the ring when we last
prodded it to run.
This will give us a better chance to catch the case where we get a
TxEmpty interrupt when it hasn't actually finished the descriptors we
*know* it should have seen, before it ends up being a full-blown TX
timeout and reset.
Since QEMU's emulation doesn't give TxEmpty interrupts, *always* bash on
TxPoll until we see the first TxEmpty interrupt and cp->txempty_seen
gets set.
Signed-off-by: David Woodhouse <redacted>
---
I'm actually having second thoughts about this one since realising that
QEMU doesn't implement it correctly. The workaround isn't *that* horrid
but it's not clear it's enough of a performance win — or whether it's
entirely necessary for catching my TX stall.
drivers/net/ethernet/realtek/8139cp.c | 37 +++++++++++++++++++++++++++++------
1 file changed, 31 insertions(+), 6 deletions(-)
@@ -611,6 +614,22 @@ static irqreturn_t cp_interrupt (int irq, void *dev_instance)if(status&(TxOK|TxErr|TxEmpty|SWInt))handled|=cp_tx(cp);+if((status&TxEmpty)&&cp->tx_running){+handled=1;+/* Qemu's emulation doesn't give TxEmpty interrupts */+cp->txempty_seen=1;+if(cp->tx_head==cp->tx_tail){+/* Out of descriptors and we have nothing more for it.+Letitstop.*/+cp->tx_running=0;+}else{+/* The hardware raced with us adding a new descriptor,+andwedidn'tgettheTxEmptyIRQintimesowe+didn'tprodit.Proditnowtorestart.*/+cpw8(TxPoll,NormalTxPoll);+}+}+if(status&LinkChg){handled=1;mii_check_media(&cp->mii_if,netif_msg_link(cp),false);
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-21 14:11:59
On Mon, 2015-09-21 at 14:59 +0100, David Woodhouse wrote:
After which, I think we might be able to turn on TX checksumming by
default and I also have a way to implement early detection of the TX
stall I've been seeing.
This is a patch I've been playing with to catch the TX stall.
When it happens we get a TxEmpty interrupt *without* TxDone.
After loading the driver, we never get TxEmpty without TxDone until the
problem has happened. I've got a minimal cp_tx_timeout() which just
clears the TxOn bit in the Cmd register and turns it back on again,
after moving the descriptors all down to the start of the TX ring.
Strangely, *after* this has happened we do see a lot of instances of
TxEmpty without TxDone. But then the TxDone usually comes immediately
afterwards. Until the driver is reloaded, at which point the next
instance of TxEmpty without TxDone *is* the hardware stalling again.
I'm very confused about what's going on there. I think I possibly need
to set a timer when the TxEmpty/!TxDone interrupt happens, and do a
preemptive reset of the TX queue (as shown below) if the timer manages
to expire before the TxDone actually happens.
@@ -623,6 +623,10 @@ static irqreturn_t cp_interrupt (int irq, void *dev_instance)Letitstop.*/cp->tx_running=0;}else{+if(!(status&(TxOK|TxErr)))+netdev_warn(dev,"TxEmpty without TxDone. h %d t %d d %04x\n",+cp->tx_head,cp->tx_tail,cpr16(TxDmaOkLowDesc));+/* The hardware raced with us adding a new descriptor,andwedidn'tgettheTxEmptyIRQintimesowedidn'tprodit.Proditnowtorestart.*/
@@ -1307,12 +1311,49 @@ static void cp_tx_timeout(struct net_device *dev)le64_to_cpu(cp->tx_ring[i].addr),cp->tx_skb[i]);}-+#if 1+/* Stop the TX (which is already stopped), move the+descriptorsdown,andstartitagain*/+cpw8_f(Cmd,RxOn);+if(cp->tx_tail==0){+/* Do nothing */+}elseif(cp->tx_head>cp->tx_tail){+for(i=0;i<cp->tx_head-cp->tx_tail;i++){+intfrom=i+cp->tx_tail;+cp->tx_ring[i].addr=cp->tx_ring[from].addr;+cp->tx_ring[i].opts2=cp->tx_ring[from].opts2;+cp->tx_ring[i].opts1=cp->tx_ring[from].opts1;+cp->tx_ring[from].opts1&=~cpu_to_le32(DescOwn);+cp->tx_opts[i]=cp->tx_opts[from];+cp->tx_skb[i]=cp->tx_skb[from];+cp->tx_skb[from]=NULL;+printk("Ring move %d->%d\n",from,i);+}+}else{+for(i=cp->tx_tail-cp->tx_head-1;i>=0;i--){+intfrom=(i+cp->tx_tail)&(CP_TX_RING_SIZE-1);+cp->tx_ring[i].addr=cp->tx_ring[from].addr;+cp->tx_ring[i].opts2=cp->tx_ring[from].opts2;+cp->tx_ring[i].opts1=cp->tx_ring[from].opts1;+cp->tx_ring[from].opts1&=~cpu_to_le32(DescOwn);+cp->tx_opts[i]=cp->tx_opts[from];+cp->tx_skb[i]=cp->tx_skb[from];+cp->tx_skb[from]=NULL;+printk("Ring move %d->%d\n",from,i);+}+}+cpw8_f(Cmd,RxOn|TxOn);+cp->tx_head=(cp->tx_head-cp->tx_tail)&(CP_TX_RING_SIZE-1);+cp->tx_tail=0;+printk("head %d tail %d\n",cp->tx_head,cp->tx_tail);+cpw8(TxPoll,NormalTxPoll);+#elsecp_stop_hw(cp);cp_clean_rings(cp);rc=cp_init_rings(cp);cp_start_hw(cp);__cp_set_rx_mode(dev);+#endifcpw16_f(IntrMask,cp_norx_intr_mask);netif_wake_queue(dev);
From: David Woodhouse <redacted>
The TX timeout handling has been observed to trigger RX IRQ storms. And
since cp_interrupt() just keeps saying that it handled the interrupt,
the machine then dies. Fix the return value from cp_interrupt(), and
the offending IRQ gets disabled and the machine survives.
I am not fond of the way it dissociates the hardware status word and the
software "handled" variable.
What you are describing - RX IRQ storms - looks like a problem between
the irq and poll handlers. That's where I expect the problem to be solved.
Sprinkling "handled" operations does not make me terribly confortable,
especially as I'd happily trade the old-style part irq, part napi
processing for a plain napi processing (I can get over it though :o) ).
Once the code is past the "if (!status || (status == 0xFFFF))" test -
or whatever test against some mask - I don't see why the driver could
refuse to take ownership of the irq.
--
Ueimor
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-21 20:52:33
On Mon, 2015-09-21 at 22:25 +0200, Francois Romieu wrote:
David Woodhouse [off-list ref] :
quoted
From: David Woodhouse <redacted>
The TX timeout handling has been observed to trigger RX IRQ storms. And
since cp_interrupt() just keeps saying that it handled the interrupt,
the machine then dies. Fix the return value from cp_interrupt(), and
the offending IRQ gets disabled and the machine survives.
I am not fond of the way it dissociates the hardware status word and the
software "handled" variable.
Oh, I like that part very much :)
The practice of returning a 'handled' status only when you've actually
*done* something you expect to mitigate the interrupt, is a useful way
of also protecting against both hardware misbehaviour and software
bugs.
What you are describing - RX IRQ storms - looks like a problem between
the irq and poll handlers. That's where I expect the problem to be solved.
I already fixed that, in the next patch in the series. But the failure
mode *should* have been 'IRQ disabled' and the device continuing to
work via polling. Not a complete death of the machine. That's the
difference this patch makes.
Sprinkling "handled" operations does not make me terribly confortable,
especially as I'd happily trade the old-style part irq, part napi
processing for a plain napi processing (I can get over it though :o) ).
The existing cp_rx_poll() function mostly runs without taking cp->lock.
But cp_tx() *does* need cp->lock for the tx_head/tx_tail manipulations.
I'm guessing that's why it's still being called directly from the hard
IRQ handler? I suppose we could probably find a way to move that out.
But I was mostly just trying to fix stuff that was actually broken...
:)
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
I'm actually having second thoughts about this one since realising that
QEMU doesn't implement it correctly. The workaround isn't *that* horrid
but it's not clear it's enough of a performance win — or whether it's
entirely necessary for catching my TX stall.
It don't indulge in this kind of fetish but I'm fine with people who want
to play with QEMU 8139cp emulation where they could use virtio. They should
imvho realize that it's their job to fix QEMU 8139cp emulation, not the
other way around.
Please keep things sane (*cough*) and avoid the qemu workaround.
--
Ueimor
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-21 21:10:36
On Mon, 2015-09-21 at 22:54 +0200, Francois Romieu wrote:
David Woodhouse [off-list ref] :
[...]
quoted
I'm actually having second thoughts about this one since realising that
QEMU doesn't implement it correctly. The workaround isn't *that* horrid
but it's not clear it's enough of a performance win — or whether it's
entirely necessary for catching my TX stall.
It don't indulge in this kind of fetish but I'm fine with people who want
to play with QEMU 8139cp emulation where they could use virtio. They should
imvho realize that it's their job to fix QEMU 8139cp emulation, not the
other way around.
Oh, I'll fix the QEMU emulation (but not this week; updating my router
has taken long enough already and I have Real Work™ to do).
But those fixes will take time to propagate to actual users. I'm not
sure it's so reasonable to knowingly break the 8139cp driver for all
currently-released versions of QEMU.
It wouldn't surprise me to find that QEMU probably accounts for the
*majority* of 8139cp use on modern Linux kernels. I've had to fix
regressions which *only* fail on real hardware, and were *only* tested
on QEMU :)
Please keep things sane (*cough*) and avoid the qemu workaround.
I don't even know that I need it. As you saw in my last WIP patch for
catching the TX stall, I was just checking for TxEmpty without TxDone.
An earlier iteration had actually remembered the last slot that was
already present when we prodded the TxPoll, and would warn on getting a
TxEmpty interrupt while that slot was still owned by the hardware. But
that has the *same* false positives, only *after* the initial stall,
that my current version has.
So I'm just not sure I care about eliminating the gratuitous TxPoll
writes. It's not as if we even wait for them. It's an MMIO write which
can complete later.
--
dwmw2
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-21 21:47:51
On Mon, 2015-09-21 at 23:01 +0200, Francois Romieu wrote:
Can you pile a patch to replace BUG with WARN_ON_ONCE(1) ?
Let's avoid having three copies of the same damn code, while we're at
it... http://git.infradead.org/users/dwmw2/linux-8139cp.git has this
and the appropriate minor fixes to subsequent patches in the series.
What do you think of finally enabling hw csum and TSO by default, btw?
Subject: [PATCH 4½/7] 8139cp: Reduce duplicate csum/tso code in cp_start_xmit()
We calculate the value of the opts1 descriptor field in three different
places. With two different behaviours when given an invalid packet to
be checksummed — none of them correct. Sort that out.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 61 ++++++++++++-----------------------
1 file changed, 20 insertions(+), 41 deletions(-)
@@ -760,6 +760,21 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,mss=skb_shinfo(skb)->gso_size;opts2=cpu_to_le32(cp_tx_vlan_tag(skb));+opts1=DescOwn;+if(mss)+opts1|=LargeSend|((mss&MSSMask)<<MSSShift);+elseif(skb->ip_summed==CHECKSUM_PARTIAL){+conststructiphdr*ip=ip_hdr(skb);+if(ip->protocol==IPPROTO_TCP)+opts1|=IPCS|TCPCS;+elseif(ip->protocol==IPPROTO_UDP)+opts1|=IPCS|UDPCS;+else{+WARN_ONCE(1,+"Net bug: asked to checksum invalid Legacy IP packet\n");+gotoout_dma_error;+}+}if(skb_shinfo(skb)->nr_frags==0){structcp_desc*txd=&cp->tx_ring[entry];
@@ -775,21 +790,9 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,txd->addr=cpu_to_le64(mapping);wmb();-flags=eor|len|DescOwn|FirstFrag|LastFrag;--if(mss)-flags|=LargeSend|((mss&MSSMask)<<MSSShift);-elseif(skb->ip_summed==CHECKSUM_PARTIAL){-conststructiphdr*ip=ip_hdr(skb);-if(ip->protocol==IPPROTO_TCP)-flags|=IPCS|TCPCS;-elseif(ip->protocol==IPPROTO_UDP)-flags|=IPCS|UDPCS;-else-WARN_ON(1);/* we need a WARN() */-}+opts1|=eor|len|FirstFrag|LastFrag;-txd->opts1=cpu_to_le32(flags);+txd->opts1=cpu_to_le32(opts1);wmb();cp->tx_skb[entry]=skb;
@@ -800,7 +803,6 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,u32first_len,first_eor,ctrl;dma_addr_tfirst_mapping;intfrag,first_entry=entry;-conststructiphdr*ip=ip_hdr(skb);/* We must give this initial chunk to the device last.*Otherwisewecouldracewiththedevice.
From: David Woodhouse <redacted>
The TX timeout handling has been observed to trigger RX IRQ storms. And
since cp_interrupt() just keeps saying that it handled the interrupt,
the machine then dies. Fix the return value from cp_interrupt(), and
the offending IRQ gets disabled and the machine survives.
Signed-off-by: David Woodhouse <redacted>
Like Francois, I don't like this.
First of all, there are only 3 bits not handled explicitly by
cp_interrupt(). And for those if they are set and no other condition
was rasied, you should report the event and the status bits set, and
then forcibly clear the interrupt.
And if we are getting Rx* interrupts with napi_schedule_prep()
returning false, that's a serious problem. It can mean that the TX
timeout handler's resetting of the chip is either miscoded or is
racing with either NAPI polling or this interrupt handler.
And if that's the case your patch is making the chip's IRQ line get
disabled when this race triggers.
This change is even worse, in my opinion, if patch #2 indeed makes
the problem go away.
From: David Woodhouse <redacted>
If an RX interrupt was already received but NAPI has not yet run when
the RX timeout happens, we end up in cp_tx_timeout() with RX interrupts
already disabled. Blindly re-enabling them will cause an IRQ storm.
This is somewhat less painful than it was a few minutes ago before I
fixed the return value from cp_interrupt(), but still suboptimal.
Unconditionally leave RX interrupts disabled after the reset, and
schedule NAPI to check the receive ring and re-enable them.
Signed-off-by: David Woodhouse <redacted>
As I said, if this makes your IRQ storms go away then therefore it
obviates the need for patch #1.
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-23 08:14:13
On Tue, 2015-09-22 at 16:45 -0700, David Miller wrote:
And if we are getting Rx* interrupts with napi_schedule_prep()
returning false, that's a serious problem. It can mean that the TX
timeout handler's resetting of the chip is either miscoded or is
racing with either NAPI polling or this interrupt handler.
Such bugs happen. That's why we have IRQ-storm detection, to help
protect us.
And if that's the case your patch is making the chip's IRQ line get
disabled when this race triggers.
This change is even worse, in my opinion, if patch #2 indeed makes
the problem go away.
Even worse than what?
When a bug like the one I've fixed in #2 happens, let's look at the
options:
- With this patch, the IRQ storm is detected. We disable the IRQ
line and the driver continues to work, albeit with a higher
latency.
- Without this patch, the box just dies completely. First the
hardware watchdog was triggering an apparently spontaneous reset.
Then when I disabled the hardware watchdog, the machine locked up
and doesn't even respond to serial sysrq. Eventually I got a
little bit of sense out of it using the NMI watchdog.
Trust me, the disabled IRQ you get with my patch really *isn't* worse
than what happened without it :)
If cp_interrupt() had already worked the way I suggest, I would
literally have two days of my life back right now. I spent a *lot* of
time working out that it was dying in an interrupt storm. And trying to
find the underlying problem while having to physically visit it under
the stairs and reset it all the time.
In fact, I've been seeing undebuggable spontaneous resets of this
router for a while now. If I'd had an 'IRQ disabled' backtrace and a
smoking gun, I might have been able to fix it a long time ago.
I even discounted the IRQ-storm hypothesis, early on, on the basis that
"Linux handles those quite gracefully now, and disables the interrupt".
Except of course *that* relies on the IRQ handler returning an
appropriate value. So it doesn't work with 8139cp as things stand.
That's why I'm really quite keen on fixing cp_interrupt() to be more
defensive in when it claims to have handled the interrupt.
Surely that's *why* we have the IRQ-storm detection in the first place
— to help us survive precisely this situation. IRQ storms are either
caused by software bugs like the one fixed in patch #2, or hardware
misbehaviour. And if IRQ handlers blindly return 'handled' just because
the hardware actually *did* assert an interrupt, regardless of whether
they've made any *progress*, then we don't get that protection in a
*lot* of the situations where we'd want it.
But sure, for now I'll drop this from the series and I can try to
convince you separately. In fact I think it only affects the last patch
in the series which reduces the banging on TxPoll. And I'm going to
drop that one too for now anyway. I'll repost shortly. And I think I'll
add a new one at the end, enabling TX checksums, SG and TSO by default.
--
dwmw2
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-23 08:43:44
From: David Woodhouse <redacted>
If an RX interrupt was already received but NAPI has not yet run when
the RX timeout happens, we end up in cp_tx_timeout() with RX interrupts
already disabled. Blindly re-enabling them will cause an IRQ storm.
(This is made particularly horrid by the fact that cp_interrupt() always
returns that it's handled the interrupt, even when it hasn't actually
done anything. If it didn't do that, the core IRQ code would have
detected the storm and handled it, I'd have had a clear smoking gun
backtrace instead of just a spontaneously resetting router, and I'd have
at *least* two days of my life back. Changing the return value of
cp_interrupt() will be argued about under separate cover.)
Unconditionally leave RX interrupts disabled after the reset, and
schedule NAPI to check the receive ring and re-enable them.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-23 08:44:12
From: David Woodhouse <redacted>
After a certain amount of staring at the debug output of this driver, I
realised it was lying to me.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-23 08:44:41
From: David Woodhouse <redacted>
When sending a TSO frame in multiple buffers, we were neglecting to set
the first descriptor up in TSO mode.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-23 08:45:00
From: David Woodhouse <redacted>
We calculate the value of the opts1 descriptor field in three different
places. With two different behaviours when given an invalid packet to
be checksummed — none of them correct. Sort that out.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 61 ++++++++++++-----------------------
1 file changed, 20 insertions(+), 41 deletions(-)
@@ -753,6 +753,21 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,mss=skb_shinfo(skb)->gso_size;opts2=cpu_to_le32(cp_tx_vlan_tag(skb));+opts1=DescOwn;+if(mss)+opts1|=LargeSend|((mss&MSSMask)<<MSSShift);+elseif(skb->ip_summed==CHECKSUM_PARTIAL){+conststructiphdr*ip=ip_hdr(skb);+if(ip->protocol==IPPROTO_TCP)+opts1|=IPCS|TCPCS;+elseif(ip->protocol==IPPROTO_UDP)+opts1|=IPCS|UDPCS;+else{+WARN_ONCE(1,+"Net bug: asked to checksum invalid Legacy IP packet\n");+gotoout_dma_error;+}+}if(skb_shinfo(skb)->nr_frags==0){structcp_desc*txd=&cp->tx_ring[entry];
@@ -768,21 +783,9 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,txd->addr=cpu_to_le64(mapping);wmb();-flags=eor|len|DescOwn|FirstFrag|LastFrag;--if(mss)-flags|=LargeSend|((mss&MSSMask)<<MSSShift);-elseif(skb->ip_summed==CHECKSUM_PARTIAL){-conststructiphdr*ip=ip_hdr(skb);-if(ip->protocol==IPPROTO_TCP)-flags|=IPCS|TCPCS;-elseif(ip->protocol==IPPROTO_UDP)-flags|=IPCS|UDPCS;-else-WARN_ON(1);/* we need a WARN() */-}+opts1|=eor|len|FirstFrag|LastFrag;-txd->opts1=cpu_to_le32(flags);+txd->opts1=cpu_to_le32(opts1);wmb();cp->tx_skb[entry]=skb;
@@ -793,7 +796,6 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,u32first_len,first_eor,ctrl;dma_addr_tfirst_mapping;intfrag,first_entry=entry;-conststructiphdr*ip=ip_hdr(skb);/* We must give this initial chunk to the device last.*Otherwisewecouldracewiththedevice.
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-23 08:45:19
From: David Woodhouse <redacted>
The low 16 bits of the 'opts1' field in the TX descriptor are supposed
to still contain the buffer length when the descriptor is handed back to
us. In practice, at least on my hardware, they don't. So stash the
original value of the opts1 field and get the length to unmap from
there.
There are other ways we could have worked out the length, but I actually
want a stash of the opts1 field anyway so that I can dump it alongside
the contents of the descriptor ring when we suffer a TX timeout.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-23 08:45:35
From: David Woodhouse <redacted>
We are seeing unexplained TX timeouts under heavy load. Let's try to get
a better idea of what's going on.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
@@ -157,6 +157,7 @@ enum {NWayAdvert=0x66,/* MII ADVERTISE */NWayLPAR=0x68,/* MII LPA */NWayExpansion=0x6A,/* MII Expansion */+TxDmaOkLowDesc=0x82,/* Low 16 bit address of a Tx descriptor. */Config5=0xD8,/* Config5 */TxPoll=0xD9,/* Tell chip to check Tx descriptors for work */RxMaxSize=0xDA,/* Max size of an Rx packet (8169 only) */
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-23 08:46:11
From: David Woodhouse <redacted>
I fixed TSO. Hardware checksum and scatter/gather also appear to be
working correctly both on real hardware and in QEMU's emulation.
Let's enable them by default and see if anyone screams...
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
But sure, for now I'll drop this from the series and I can try to
convince you separately.
Yes, let's discuss this independantly to the nice bug fixing
that's happening in the rest of this series.
Since you explicitly call them 'bug fixes'... I suppose the last patch
in the latest series (enabling the various offloads by default) isn't a
bug fix and should be held back for net-next.
I think I can live with pushing the rest for the net tree. Just to
recap, the series is:
8139cp: Do not re-enable RX interrupts in cp_tx_timeout()
8139cp: Fix tx_queued debug message to print correct slot numbers
8139cp: Fix TSO/scatter-gather descriptor setup
8139cp: Reduce duplicate csum/tso code in cp_start_xmit()
8139cp: Fix DMA unmapping of transmitted buffers
8139cp: Dump contents of descriptor ring on TX timeout
8139cp: Enable offload features by default
The penultimate patch is also not strictly a bug fix, but it's only
adding additional diagnostics to a code path which was already broken
until I fixed it anyway, so I can live with that in net although I'll
also be happy if you want to defer it.
The fourth patch which removes the three duplicate versions of the
csum/tso checks might also be deferrable but really, it's *also*
fixing the failure mode when we see an inappropriate CHECKSUM_PARTIAL
packet, and it's touching a code path which I've *also* fixed in this
patch set. So it might as well stay.
I can shuffle things around if you disagree, but assuming Francois
concurs I'd suggest merging patches 1-6 (or just pulling from
git://git.infradead.org/users/dwmw2/linux-8139cp.git fixes-only)
and then I'll resubmit the last patch some time later, after you
next pull net into net-next.
Otherwise, please let me know if/how you'd like me to reorganise it.
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
Since you explicitly call them 'bug fixes'... I suppose the last patch
in the latest series (enabling the various offloads by default) isn't a
bug fix and should be held back for net-next.
I think I can live with pushing the rest for the net tree. Just to
recap, the series is:
I've applied patches #1-#6 and left #7 sitting around in patchwork
for when I next merge net into net-next, thanks.
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-23 22:00:06
On Wed, 2015-09-23 at 14:48 -0700, David Miller wrote:
I've applied patches #1-#6 and left #7 sitting around in patchwork
for when I next merge net into net-next, thanks.
Great, thanks. I've got one more for the net tree; just masking the
gso_size to the maximum (0xfff) that the hardware can handle, and never
telling the net stack that that's our maximum, doesn't seem like such a
good idea...
I think I really am going to stop looking now :)
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-23 22:02:24
The maximum MSS value we can set is 0xFFF. When fixing the TSO code I
noticed we just mask the gso_size value to the hardware's maximum and
don't care about the consequences. That seems... unsagacious.
Instead of masking, WARN and drop the packet if it's too large. And use
netif_set_gso_max_size() so it shouldn't happen in the first place.
Signed-off-by: David Woodhouse <redacted>
---
drivers/net/ethernet/realtek/8139cp.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
But sure, for now I'll drop this from the series and I can try to
convince you separately.
You may as well change the IRQ storm avoidance logic so that it does not
require conformant driver code if you do not want to waste more time :o)
--
Ueimor
I can shuffle things around if you disagree, but assuming Francois
concurs I'd suggest merging patches 1-6 (or just pulling from
git://git.infradead.org/users/dwmw2/linux-8139cp.git fixes-only)
and then I'll resubmit the last patch some time later, after you
next pull net into net-next.
No objection but I do not really trust whatever review I do this late.
Did you try #5 with the DMA allocation debug code enabled ?
--
Ueimor
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-23 23:09:56
On Thu, 2015-09-24 at 00:44 +0200, Francois Romieu wrote:
David Woodhouse [off-list ref] :
[...]
quoted
I can shuffle things around if you disagree, but assuming Francois
concurs I'd suggest merging patches 1-6 (or just pulling from
git://git.infradead.org/users/dwmw2/linux-8139cp.git fixes-only)
and then I'll resubmit the last patch some time later, after you
next pull net into net-next.
No objection but I do not really trust whatever review I do this late.
Did you try #5 with the DMA allocation debug code enabled ?
Only in QEMU (where there wasn't a problem in the first place). But
yes.
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-23 23:18:13
On Thu, 2015-09-24 at 00:44 +0200, Francois Romieu wrote:
David Woodhouse [off-list ref] :
[...]
quoted
But sure, for now I'll drop this from the series and I can try to
convince you separately.
You may as well change the IRQ storm avoidance logic so that it does not
require conformant driver code if you do not want to waste more time :o)
That's not entirely trivial — at a certain level we *do* require that
drivers don't lie to the core IRQ logic.
I suppose even if a rogue driver is returning IRQ_HANDLED in an IRQ
storm, when it's *not* actually doing anything or making any progress,
perhaps there's a way we could detect that we're always going straight
back into the next interrupt handler as *soon* as we exit the previous
one. Perhaps there's merit in exploring that option.
I'm not sure that gives you a valid excuse for not wanting to fix the
driver though :)
I might start by "clarifying" the documentation on the IRQ handler's
return value...
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
Great, thanks. I've got one more for the net tree; just masking the
gso_size to the maximum (0xfff) that the hardware can handle, and never
telling the net stack that that's our maximum, doesn't seem like such a
good idea...
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-24 08:58:37
When fixing the TSO support I noticed we just mask skb->gso_size with
the MSSMask value and don't care about the consequences.
That seems... unsagacious.
Instead of masking, WARN and drop the packet if the maximum is exceeded.
And call netif_set_gso_max_size() so it shouldn't happen in the first
place.
Finally, Francois Romieu noticed that we didn't even have the right
value for MSSMask anyway; it should be 0x7ff (11 bits) not 0xfff.
Signed-off-by: David Woodhouse <redacted>
---
Nice and simple and obvious, right?
So why does it stop TSO from happening at all?
When I call netif_set_gso_max_size() with a value of 0xaad (2733) or
higher, I see TSO being used (with an MSS of 1214; this is just between
a VM and its host; Legacy IP with an MTU of 1500).
If I set a maximum of 0xaac or lower, TSO never gets used. Am I doing
something wrong? Or is there a problem somewhere else?
drivers/net/ethernet/realtek/8139cp.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-24 10:38:26
When fixing the TSO support I noticed we just mask ->gso_size with the
MSSMask value and don't care about the consequences.
Provide a .ndo_features_check() method which drops the NETIF_F_TSO
feature for any skb which would exceed the maximum, and thus forces it
to be segmented by software.
Then we can stop the masking in cp_start_xmit(), and just WARN if the
maximum is exceeded, which should now never happen.
Finally, Francois Romieu noticed that we didn't even have the right
value for MSSMask anyway; it should be 0x7ff (11 bits) not 0xfff.
Signed-off-by: David Woodhouse <redacted>
---
OK, netif_set_gso_max_size() was *not*, as I had naïvely assumed, a way
to set the maximum value of ->gso_size. That's an unfortunate set of
naming decisions.
I don't see a simple way to tell the net stack about the maximum MSS
value we can cope with in the general case. But we can do it this way
by clearing NETIF_F_TSO on a per-packet basis instead.
Incidentally, other drivers might benefit from doing the same if this
is considered reasonable. For example it looks like the r8169 driver
manually calls skb_gso_segment() for its fallback case, and I don't see
what then prevents it from hitting the "BUG!" at the start of
rtl8169_start_xmit() if it runs out of space in the descriptor ring.
drivers/net/ethernet/realtek/8139cp.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
From: Eric Dumazet <hidden> Date: 2015-09-24 12:05:27
On Thu, 2015-09-24 at 11:38 +0100, David Woodhouse wrote:
When fixing the TSO support I noticed we just mask ->gso_size with the
MSSMask value and don't care about the consequences.
Provide a .ndo_features_check() method which drops the NETIF_F_TSO
feature for any skb which would exceed the maximum, and thus forces it
to be segmented by software.
Then we can stop the masking in cp_start_xmit(), and just WARN if the
maximum is exceeded, which should now never happen.
Finally, Francois Romieu noticed that we didn't even have the right
value for MSSMask anyway; it should be 0x7ff (11 bits) not 0xfff.
Signed-off-by: David Woodhouse <redacted>
---
OK, netif_set_gso_max_size() was *not*, as I had naïvely assumed, a way
to set the maximum value of ->gso_size. That's an unfortunate set of
naming decisions.
Right, netif_skb_features() only has the following checks :
if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs)
features &= ~NETIF_F_GSO_MASK;
But now we have .ndo_features_check() we could remove this generic check
from fast path.
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-24 12:31:44
On Thu, 2015-09-24 at 05:05 -0700, Eric Dumazet wrote:
Right, netif_skb_features() only has the following checks :
if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs)
features &= ~NETIF_F_GSO_MASK;
But now we have .ndo_features_check() we could remove this generic
check from fast path.
Perhaps so, yes.
Any thoughts on the other reason I was staring at this same code path
this week? I am able to reliably feed inappropriate packets to a
NETIF_F_IP_CSUM-capable device with the test program at
http://bombadil.infradead.org/~dwmw2/raw.c (and equivalent code paths
via virtio_net, tun and others).
They're *supposed* to get checksummed by software if the device can't
cope, but netif_skb_features() returns the wrong answer, so we fail to
do that and they're fed with CHECKSUM_PARTIAL to a device which can't
handle them. Causing a WARN() or a BUG() or a silent corruption,
depending on the driver.
--
dwmw2
When fixing the TSO support I noticed we just mask ->gso_size with the
MSSMask value and don't care about the consequences.
Provide a .ndo_features_check() method which drops the NETIF_F_TSO
feature for any skb which would exceed the maximum, and thus forces it
to be segmented by software.
Then we can stop the masking in cp_start_xmit(), and just WARN if the
maximum is exceeded, which should now never happen.
Finally, Francois Romieu noticed that we didn't even have the right
value for MSSMask anyway; it should be 0x7ff (11 bits) not 0xfff.
Signed-off-by: David Woodhouse <redacted>
I'm going to apply this to net-next, thanks David.
From: Tom Herbert <hidden> Date: 2015-09-28 05:37:50
On Thu, Sep 24, 2015 at 5:31 AM, David Woodhouse [off-list ref] wrote:
On Thu, 2015-09-24 at 05:05 -0700, Eric Dumazet wrote:
quoted
Right, netif_skb_features() only has the following checks :
if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs)
features &= ~NETIF_F_GSO_MASK;
But now we have .ndo_features_check() we could remove this generic
check from fast path.
Perhaps so, yes.
Any thoughts on the other reason I was staring at this same code path
this week? I am able to reliably feed inappropriate packets to a
NETIF_F_IP_CSUM-capable device with the test program at
http://bombadil.infradead.org/~dwmw2/raw.c (and equivalent code paths
via virtio_net, tun and others).
They're *supposed* to get checksummed by software if the device can't
cope, but netif_skb_features() returns the wrong answer, so we fail to
do that and they're fed with CHECKSUM_PARTIAL to a device which can't
handle them. Causing a WARN() or a BUG() or a silent corruption,
depending on the driver.
Which drivers are doing this? It is up to the driver to determine
whether a particular packet being sent can have checksum offloaded to
the device. If it cannot offload the checksum it must call
skb_checksum_help.
Tom
From: David Woodhouse <dwmw2@infradead.org> Date: 2015-09-28 07:21:58
On Sun, 2015-09-27 at 22:37 -0700, Tom Herbert wrote:
Which drivers are doing this? It is up to the driver to determine
whether a particular packet being sent can have checksum offloaded to
the device. If it cannot offload the checksum it must call
skb_checksum_help.
Not so.
A driver sets the NETIF_F_IP_CSUM feature to indicate that it can do
the checksum on Legacy IP TCP or UDP frames and *nothing* else.
It most certainly does not expect to be handed any other kind of packet
for checksumming, and bad things will often happen if it is. If drivers
*do* spot that they've been given something they don't handle, I see
BUG() calls and warnings, but I don't see any of them calling
skb_checksum_help() to silently cope. Many of them just feed it to the
hardware and don't even notice at all because it's the *hardware* which
decides whether to do a TCP or a UDP checksum. So who knows what'll
happen.
The check is supposed to be done in can_checksum_protocol(), called
from harmonize_features(). But as noted, that check has false positives
and lets some inappropriate packets through — for NETIF_F_IP_CSUM it
lets through *all* skbuffs with ->protocol == ETH_P_IP instead of only
TCP and UDP.
I originally couldn't see how to deal with this except by looking at
the contents of the packet, which sucked. But I think I've found a
somewhat more acceptable approach now:
http://lists.openwall.net/netdev/2015/09/25/85
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
But sure, for now I'll drop this from the series and I can try to
convince you separately.
Yes, let's discuss this independantly to the nice bug fixing
that's happening in the rest of this series.
Thanks.
This is the patch we discussed earlier, and I came away from the
conversation believing that I had achieved my goal of trying to
convince you separately :)
The patch in patchwork at https://patchwork.ozlabs.org/patch/520318/
still applies cleanly; is that sufficient or would you like it
resubmitted (perhaps after -rc1)?
Thanks.
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation