Hi,
this series provides some fixes for a number of issues met with the
mvneta driver :
- driver lockup when reading stats while sending traffic from multiple
CPUs : this obviously only happens on SMP and is the result of missing
locking on the driver. The problem was present since the introduction
of the driver in 3.8. The first patch performs some changes that are
needed for the second one which actually fixes the issue by using
per-cpu counters. It could make sense to backport this to the relevant
stable versions.
- mvneta_tx_timeout calls various functions to reset the NIC, and these
functions sleep, which is not allowed here, resulting in a panic.
Better completely disable this Tx timeout handler for now since it is
never called. The problem was encountered while developing some new
features, it's uncertain whether it's possible to reproduce it with
regular usage, so maybe a backport to stable is not needed.
- replace the Tx timer with a real Tx IRQ. As first reported by Arnaud
Ebalard and explained by Eric Dumazet, there is no way this driver
can work correctly if it uses a driver to recycle the Tx descriptors.
If too many packets are sent at once, the driver quickly ends up with
no descriptors (which happens twice as easily in GSO) and has to wait
10ms for recycling its descriptors and being able to send again. Eric
has worked around this in the core GSO code. But still when routing
traffic or sending UDP packets, the limitation is very visible. Using
Tx IRQs allows Tx descriptors to be recycled when sent. The coalesce
value is still configurable using ethtool. This fix turns the UDP
send bitrate from 134 Mbps to 987 Mbps (ie: line rate). It's made of
two patches, one to add the relevant bits from the original Marvell's
driver, and another one to implement the change. I don't know if it
should be backported to stable, as the bug only causes poor performance.
Thanks,
Willy
---
Willy Tarreau (5):
net: mvneta: increase the 64-bit rx/tx stats out of the hot path
net: mvneta: use per_cpu stats to fix an SMP lock up
net: mvneta: do not schedule in mvneta_tx_timeout
net: mvneta: add missing bit descriptions for interrupt masks and causes
net: mvneta: replace Tx timer with a real interrupt
drivers/net/ethernet/marvell/mvneta.c | 217 ++++++++++++++++++----------------
1 file changed, 116 insertions(+), 101 deletions(-)
--
1.7.12.2.21.g234cd45.dirty
Better count packets and bytes in the stack and on 32 bit then
accumulate them at the end for once. This saves two memory writes
and two memory barriers per packet. The incoming packet rate was
increased by 4.7% on the Openblocks AX3 thanks to this.
Cc: Thomas Petazzoni <redacted>
Cc: Gregory CLEMENT <redacted>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/marvell/mvneta.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
@@ -1391,6 +1391,8 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,{structnet_device*dev=pp->dev;intrx_done,rx_filled;+u32rcvd_pkts=0;+u32rcvd_bytes=0;/* Get number of received packets */rx_done=mvneta_rxq_busy_desc_num_get(pp,rxq);
@@ -1428,10 +1430,8 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,rx_bytes=rx_desc->data_size-(ETH_FCS_LEN+MVNETA_MH_SIZE);-u64_stats_update_begin(&pp->rx_stats.syncp);-pp->rx_stats.packets++;-pp->rx_stats.bytes+=rx_bytes;-u64_stats_update_end(&pp->rx_stats.syncp);+rcvd_pkts++;+rcvd_bytes+=rx_bytes;/* Linux processing */skb_reserve(skb,MVNETA_MH_SIZE);
@@ -1452,6 +1452,13 @@ static int mvneta_rx(struct mvneta_port *pp, int rx_todo,}}+if(rcvd_pkts){+u64_stats_update_begin(&pp->rx_stats.syncp);+pp->rx_stats.packets+=rcvd_pkts;+pp->rx_stats.bytes+=rcvd_bytes;+u64_stats_update_end(&pp->rx_stats.syncp);+}+/* Update rxq management counters */mvneta_rxq_desc_num_update(pp,rxq,rx_done,rx_filled);
If a queue timeout is reported, we can oops because of some
schedules while the caller is atomic, as shown below :
mvneta d0070000.ethernet eth0: tx timeout
BUG: scheduling while atomic: bash/1528/0x00000100
Modules linked in: slhttp_ethdiv(C) [last unloaded: slhttp_ethdiv]
CPU: 2 PID: 1528 Comm: bash Tainted: G WC 3.13.0-rc4-mvebu-nf #180
[<c0011bd9>] (unwind_backtrace+0x1/0x98) from [<c000f1ab>] (show_stack+0xb/0xc)
[<c000f1ab>] (show_stack+0xb/0xc) from [<c02ad323>] (dump_stack+0x4f/0x64)
[<c02ad323>] (dump_stack+0x4f/0x64) from [<c02abe67>] (__schedule_bug+0x37/0x4c)
[<c02abe67>] (__schedule_bug+0x37/0x4c) from [<c02ae261>] (__schedule+0x325/0x3ec)
[<c02ae261>] (__schedule+0x325/0x3ec) from [<c02adb97>] (schedule_timeout+0xb7/0x118)
[<c02adb97>] (schedule_timeout+0xb7/0x118) from [<c0020a67>] (msleep+0xf/0x14)
[<c0020a67>] (msleep+0xf/0x14) from [<c01dcbe5>] (mvneta_stop_dev+0x21/0x194)
[<c01dcbe5>] (mvneta_stop_dev+0x21/0x194) from [<c01dcfe9>] (mvneta_tx_timeout+0x19/0x24)
[<c01dcfe9>] (mvneta_tx_timeout+0x19/0x24) from [<c024afc7>] (dev_watchdog+0x18b/0x1c4)
[<c024afc7>] (dev_watchdog+0x18b/0x1c4) from [<c0020b53>] (call_timer_fn.isra.27+0x17/0x5c)
[<c0020b53>] (call_timer_fn.isra.27+0x17/0x5c) from [<c0020cad>] (run_timer_softirq+0x115/0x170)
[<c0020cad>] (run_timer_softirq+0x115/0x170) from [<c001ccb9>] (__do_softirq+0xbd/0x1a8)
[<c001ccb9>] (__do_softirq+0xbd/0x1a8) from [<c001cfad>] (irq_exit+0x61/0x98)
[<c001cfad>] (irq_exit+0x61/0x98) from [<c000d4bf>] (handle_IRQ+0x27/0x60)
[<c000d4bf>] (handle_IRQ+0x27/0x60) from [<c000843b>] (armada_370_xp_handle_irq+0x33/0xc8)
[<c000843b>] (armada_370_xp_handle_irq+0x33/0xc8) from [<c000fba9>] (__irq_usr+0x49/0x60)
So for now, let's simply ignore these timeouts generally caused by bugs
only.
Cc: Thomas Petazzoni <redacted>
Cc: Gregory CLEMENT <redacted>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/marvell/mvneta.c | 11 -----------
1 file changed, 11 deletions(-)
@@ -2244,16 +2244,6 @@ static void mvneta_stop_dev(struct mvneta_port *pp)mvneta_rx_reset(pp);}-/* tx timeout callback - display a message and stop/start the network device */-staticvoidmvneta_tx_timeout(structnet_device*dev)-{-structmvneta_port*pp=netdev_priv(dev);--netdev_info(dev,"tx timeout\n");-mvneta_stop_dev(pp);-mvneta_start_dev(pp);-}-/* Return positive if MTU is valid */staticintmvneta_check_mtu_valid(structnet_device*dev,intmtu){
Right now the mvneta driver doesn't handle Tx IRQ, and relies on two
mechanisms to flush Tx descriptors : a flush at the end of mvneta_tx()
and a timer. If a burst of packets is emitted faster than the device
can send them, then the queue is stopped until next wake-up of the
timer 10ms later. This causes jerky output traffic with bursts and
pauses, making it difficult to reach line rate with very few streams.
A test on UDP traffic shows that it's not possible to go beyond 134
Mbps / 12 kpps of outgoing traffic with 1500-bytes IP packets. Routed
traffic tends to observe pauses as well if the traffic is bursty,
making it even burstier after the wake-up.
It seems that this feature was inherited from the original driver but
nothing there mentions any reason for not using the interrupt instead,
which the chip supports.
Thus, this patch enables Tx interrupts and removes the timer. It does
the two at once because it's not really possible to make the two
mechanisms coexist, so a split patch doesn't make sense.
First tests performed on a Mirabox (Armada 370) show that less CPU
seems to be used when sending traffic. One reason might be that we now
call the mvneta_tx_done_gbe() with a mask indicating which queues have
been done instead of looping over all of them.
The same UDP test above now happily reaches 987 Mbps / 87.7 kpps.
Single-stream TCP traffic can now more easily reach line rate. HTTP
transfers of 1 MB objects over a single connection went from 730 to
840 Mbps. It is even possible to go significantly higher (>900 Mbps)
by tweaking tcp_tso_win_divisor.
Cc: Thomas Petazzoni <redacted>
Cc: Gregory CLEMENT <redacted>
Cc: Arnaud Ebalard <redacted>
Cc: Eric Dumazet <redacted>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/marvell/mvneta.c | 71 ++++++-----------------------------
1 file changed, 12 insertions(+), 59 deletions(-)
@@ -1149,17 +1141,6 @@ static void mvneta_tx_done_pkts_coal_set(struct mvneta_port *pp,txq->done_pkts_coal=value;}-/* Trigger tx done timer in MVNETA_TX_DONE_TIMER_PERIOD msecs */-staticvoidmvneta_add_tx_done_timer(structmvneta_port*pp)-{-if(test_and_set_bit(MVNETA_F_TX_DONE_TIMER_BIT,&pp->flags)==0){-pp->tx_done_timer.expires=jiffies+-msecs_to_jiffies(MVNETA_TX_DONE_TIMER_PERIOD);-add_timer(&pp->tx_done_timer);-}-}--/* Handle rx descriptor fill by setting buf_cookie and buf_phys_addr */staticvoidmvneta_rx_desc_fill(structmvneta_rx_desc*rx_desc,u32phys_addr,u32cookie)
@@ -1651,15 +1632,6 @@ out:dev_kfree_skb_any(skb);}-if(txq->count>=MVNETA_TXDONE_COAL_PKTS)-mvneta_txq_done(pp,txq);--/* If after calling mvneta_txq_done, count equals-*frags,weneedtosetthetimer-*/-if(txq->count==frags&&frags>0)-mvneta_add_tx_done_timer(pp);-returnNETDEV_TX_OK;}
@@ -1935,14 +1907,22 @@ static int mvneta_poll(struct napi_struct *napi, int budget)/* Read cause register */cause_rx_tx=mvreg_read(pp,MVNETA_INTR_NEW_CAUSE)&-MVNETA_RX_INTR_MASK(rxq_number);+(MVNETA_RX_INTR_MASK(rxq_number)|MVNETA_TX_INTR_MASK(txq_number));++/* Release Tx descriptors */+if(cause_rx_tx&MVNETA_TX_INTR_MASK_ALL){+inttx_todo=0;++mvneta_tx_done_gbe(pp,(cause_rx_tx&MVNETA_TX_INTR_MASK_ALL),&tx_todo);+cause_rx_tx&=~MVNETA_TX_INTR_MASK_ALL;+}/* For the case where the last mvneta_poll did not process all*RXpackets*/cause_rx_tx|=pp->cause_rx_tx;if(rxq_number>1){-while((cause_rx_tx!=0)&&(budget>0)){+while((cause_rx_tx&MVNETA_RX_INTR_MASK_ALL)&&(budget>0)){intcount;structmvneta_rx_queue*rxq;/* get rx queue number from cause_rx_tx */
@@ -1974,7 +1954,7 @@ static int mvneta_poll(struct napi_struct *napi, int budget)napi_complete(napi);local_irq_save(flags);mvreg_write(pp,MVNETA_INTR_NEW_MASK,-MVNETA_RX_INTR_MASK(rxq_number));+MVNETA_RX_INTR_MASK(rxq_number)|MVNETA_TX_INTR_MASK(txq_number));local_irq_restore(flags);}
@@ -1982,26 +1962,6 @@ static int mvneta_poll(struct napi_struct *napi, int budget)returnrx_done;}-/* tx done timer callback */-staticvoidmvneta_tx_done_timer_callback(unsignedlongdata)-{-structnet_device*dev=(structnet_device*)data;-structmvneta_port*pp=netdev_priv(dev);-inttx_done=0,tx_todo=0;--if(!netif_running(dev))-return;--clear_bit(MVNETA_F_TX_DONE_TIMER_BIT,&pp->flags);--tx_done=mvneta_tx_done_gbe(pp,-(((1<<txq_number)-1)&-MVNETA_CAUSE_TXQ_SENT_DESC_ALL_MASK),-&tx_todo);-if(tx_todo>0)-mvneta_add_tx_done_timer(pp);-}-/* Handle rxq fill: allocates rxq skbs; called when initializing a port */staticintmvneta_rxq_fill(structmvneta_port*pp,structmvneta_rx_queue*rxq,intnum)
Stats writers are mvneta_rx() and mvneta_tx(). They don't lock anything
when they update the stats, and as a result, it randomly happens that
the stats freeze on SMP if two updates happen during stats retrieval.
This is very easily reproducible by starting two HTTP servers and binding
each of them to a different CPU, then consulting /proc/net/dev in loops
during transfers, the interface should immediately lock up. This issue
also randomly happens upon link state changes during transfers, because
the stats are collected in this situation, but it takes more attempts to
reproduce it.
The comments in netdevice.h suggest using per_cpu stats instead to get
rid of this issue.
This patch implements this. It merges both rx_stats and tx_stats into
a single "stats" member with a single syncp. Both mvneta_rx() and
mvneta_rx() now only update the a single CPU's counters.
In turn, mvneta_get_stats64() does the summing by iterating over all CPUs
to get their respective stats.
With this change, stats are still correct and no more lockup is encountered.
Note that this bug was present since the first import of the mvneta
driver. It might make sense to backport it to some stable trees. If
so, it depends on "d33dc73 net: mvneta: increase the 64-bit rx/tx stats
out of the hot path".
Cc: Thomas Petazzoni <redacted>
Cc: Gregory CLEMENT <redacted>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/marvell/mvneta.c | 84 +++++++++++++++++++++++------------
1 file changed, 55 insertions(+), 29 deletions(-)
@@ -2758,6 +2770,7 @@ static int mvneta_probe(struct platform_device *pdev)constchar*mac_from;intphy_mode;interr;+intcpu;/* Our multiqueue support is not complete, so for now, only*allowtheusageofthefirstRXqueue
@@ -2799,9 +2812,6 @@ static int mvneta_probe(struct platform_device *pdev)pp=netdev_priv(dev);-u64_stats_init(&pp->tx_stats.syncp);-u64_stats_init(&pp->rx_stats.syncp);-pp->weight=MVNETA_RX_POLL_WEIGHT;pp->phy_node=phy_node;pp->phy_interface=phy_mode;
Marvell has not published the chip's datasheet yet, so it's very hard
to find the relevant bits to manipulate to change the IRQ behaviour.
Fortunately, these bits are described in the proprietary LSP patch set
which is publicly available here :
http://www.plugcomputer.org/downloads/mirabox/
So let's put them back in the driver in order to reduce the burden of
current and future maintenance.
Cc: Thomas Petazzoni <redacted>
Cc: Gregory CLEMENT <redacted>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/marvell/mvneta.c | 44 +++++++++++++++++++++++++++++++++--
1 file changed, 42 insertions(+), 2 deletions(-)
From: Ben Hutchings <hidden> Date: 2014-01-12 16:49:55
On Sun, 2014-01-12 at 10:31 +0100, Willy Tarreau wrote:
If a queue timeout is reported, we can oops because of some
schedules while the caller is atomic, as shown below :
mvneta d0070000.ethernet eth0: tx timeout
BUG: scheduling while atomic: bash/1528/0x00000100
Modules linked in: slhttp_ethdiv(C) [last unloaded: slhttp_ethdiv]
CPU: 2 PID: 1528 Comm: bash Tainted: G WC 3.13.0-rc4-mvebu-nf #180
[<c0011bd9>] (unwind_backtrace+0x1/0x98) from [<c000f1ab>] (show_stack+0xb/0xc)
[<c000f1ab>] (show_stack+0xb/0xc) from [<c02ad323>] (dump_stack+0x4f/0x64)
[<c02ad323>] (dump_stack+0x4f/0x64) from [<c02abe67>] (__schedule_bug+0x37/0x4c)
[<c02abe67>] (__schedule_bug+0x37/0x4c) from [<c02ae261>] (__schedule+0x325/0x3ec)
[<c02ae261>] (__schedule+0x325/0x3ec) from [<c02adb97>] (schedule_timeout+0xb7/0x118)
[<c02adb97>] (schedule_timeout+0xb7/0x118) from [<c0020a67>] (msleep+0xf/0x14)
[<c0020a67>] (msleep+0xf/0x14) from [<c01dcbe5>] (mvneta_stop_dev+0x21/0x194)
[<c01dcbe5>] (mvneta_stop_dev+0x21/0x194) from [<c01dcfe9>] (mvneta_tx_timeout+0x19/0x24)
[<c01dcfe9>] (mvneta_tx_timeout+0x19/0x24) from [<c024afc7>] (dev_watchdog+0x18b/0x1c4)
[<c024afc7>] (dev_watchdog+0x18b/0x1c4) from [<c0020b53>] (call_timer_fn.isra.27+0x17/0x5c)
[<c0020b53>] (call_timer_fn.isra.27+0x17/0x5c) from [<c0020cad>] (run_timer_softirq+0x115/0x170)
[<c0020cad>] (run_timer_softirq+0x115/0x170) from [<c001ccb9>] (__do_softirq+0xbd/0x1a8)
[<c001ccb9>] (__do_softirq+0xbd/0x1a8) from [<c001cfad>] (irq_exit+0x61/0x98)
[<c001cfad>] (irq_exit+0x61/0x98) from [<c000d4bf>] (handle_IRQ+0x27/0x60)
[<c000d4bf>] (handle_IRQ+0x27/0x60) from [<c000843b>] (armada_370_xp_handle_irq+0x33/0xc8)
[<c000843b>] (armada_370_xp_handle_irq+0x33/0xc8) from [<c000fba9>] (__irq_usr+0x49/0x60)
So for now, let's simply ignore these timeouts generally caused by bugs
only.
No, don't ignore them. Schedule a work item to reset the device. (And
remember to cancel it when stopping the device.)
Ben.
@@ -2244,16 +2244,6 @@ static void mvneta_stop_dev(struct mvneta_port *pp)mvneta_rx_reset(pp);}-/* tx timeout callback - display a message and stop/start the network device */-staticvoidmvneta_tx_timeout(structnet_device*dev)-{-structmvneta_port*pp=netdev_priv(dev);--netdev_info(dev,"tx timeout\n");-mvneta_stop_dev(pp);-mvneta_start_dev(pp);-}-/* Return positive if MTU is valid */staticintmvneta_check_mtu_valid(structnet_device*dev,intmtu){
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
Hi Ben,
On Sun, Jan 12, 2014 at 04:49:51PM +0000, Ben Hutchings wrote:
(...)
quoted
So for now, let's simply ignore these timeouts generally caused by bugs
only.
No, don't ignore them. Schedule a work item to reset the device. (And
remember to cancel it when stopping the device.)
OK I can try to do that. Could you recommend me one driver which does this
successfully so that I can see exactly what needs to be taken care of ?
Thanks,
Willy
From: Ben Hutchings <hidden> Date: 2014-01-12 17:39:05
[Putting another hat on]
On Sun, 2014-01-12 at 17:55 +0100, Willy Tarreau wrote:
Hi Ben,
On Sun, Jan 12, 2014 at 04:49:51PM +0000, Ben Hutchings wrote:
(...)
quoted
quoted
So for now, let's simply ignore these timeouts generally caused by bugs
only.
No, don't ignore them. Schedule a work item to reset the device. (And
remember to cancel it when stopping the device.)
OK I can try to do that. Could you recommend me one driver which does this
successfully so that I can see exactly what needs to be taken care of ?
sfc does it, though the reset logic there is more complicated than you
would need.
I think this will DTRT, but it's compile-tested only. I have been given
an OpenBlocks AX3 but haven't set it up yet.
Ben.
---
mvneta: Defer restart from TX watchdog handler to work item
A restart requires sleeping, but the watchdog handler runs in atomic
context.
The work item can race with down-ing of the interface, so take the RTNL
lock and do nothing if the interface is already down.
Signed-off-by: Ben Hutchings <redacted>
---
@@ -2231,8 +2232,22 @@ static void mvneta_tx_timeout(struct net_device *dev)structmvneta_port*pp=netdev_priv(dev);netdev_info(dev,"tx timeout\n");-mvneta_stop_dev(pp);-mvneta_start_dev(pp);++/* defer mvneta_restart(), as we're in atomic context here */+schedule_work(&pp->restart_work);+}++staticvoidmvneta_restart(structwork_struct*work)+{+structmvneta_port*pp=+container_of(work,structmvneta_port,restart_work);++rtnl_lock();+if(netif_running(pp->dev)){+mvneta_stop_dev(pp);+mvneta_start_dev(pp);+}+rtnl_unlock();}/* Return positive if MTU is valid */
@@ -2792,6 +2807,8 @@ static int mvneta_probe(struct platform_device *pdev)pp=netdev_priv(dev);+INIT_WORK(&pp->restart_work,mvneta_restart);+u64_stats_init(&pp->tx_stats.syncp);u64_stats_init(&pp->rx_stats.syncp);
@@ -2890,6 +2907,7 @@ static int mvneta_remove(struct platform_device *pdev)structmvneta_port*pp=netdev_priv(dev);unregister_netdev(dev);+cancel_work_sync(&pp->restart_work);mvneta_deinit(pp);clk_disable_unprepare(pp->clk);iounmap(pp->base);
--
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.
From: Eric Dumazet <hidden> Date: 2014-01-12 18:07:38
On Sun, 2014-01-12 at 10:31 +0100, Willy Tarreau wrote:
Stats writers are mvneta_rx() and mvneta_tx(). They don't lock anything
when they update the stats, and as a result, it randomly happens that
the stats freeze on SMP if two updates happen during stats retrieval.
Your patch is OK, but I dont understand how this freeze can happen.
TX and RX uses a separate syncp, and TX is protected by a lock, RX
is protected by NAPI bit.
Stats retrieval uses the appropriate BH disable before the fetches...
This is very easily reproducible by starting two HTTP servers and binding
each of them to a different CPU, then consulting /proc/net/dev in loops
during transfers, the interface should immediately lock up. This issue
also randomly happens upon link state changes during transfers, because
the stats are collected in this situation, but it takes more attempts to
reproduce it.
this series provides some fixes for a number of issues met with the
mvneta driver :
- driver lockup when reading stats while sending traffic from multiple
CPUs : this obviously only happens on SMP and is the result of missing
locking on the driver. The problem was present since the introduction
of the driver in 3.8. The first patch performs some changes that are
needed for the second one which actually fixes the issue by using
per-cpu counters. It could make sense to backport this to the relevant
stable versions.
- mvneta_tx_timeout calls various functions to reset the NIC, and these
functions sleep, which is not allowed here, resulting in a panic.
Better completely disable this Tx timeout handler for now since it is
never called. The problem was encountered while developing some new
features, it's uncertain whether it's possible to reproduce it with
regular usage, so maybe a backport to stable is not needed.
- replace the Tx timer with a real Tx IRQ. As first reported by Arnaud
Ebalard and explained by Eric Dumazet, there is no way this driver
can work correctly if it uses a driver to recycle the Tx descriptors.
If too many packets are sent at once, the driver quickly ends up with
no descriptors (which happens twice as easily in GSO) and has to wait
10ms for recycling its descriptors and being able to send again. Eric
has worked around this in the core GSO code. But still when routing
traffic or sending UDP packets, the limitation is very visible. Using
Tx IRQs allows Tx descriptors to be recycled when sent. The coalesce
value is still configurable using ethtool. This fix turns the UDP
send bitrate from 134 Mbps to 987 Mbps (ie: line rate). It's made of
two patches, one to add the relevant bits from the original Marvell's
driver, and another one to implement the change. I don't know if it
should be backported to stable, as the bug only causes poor performance.
Hi Eric!
On Sun, Jan 12, 2014 at 10:07:36AM -0800, Eric Dumazet wrote:
On Sun, 2014-01-12 at 10:31 +0100, Willy Tarreau wrote:
quoted
Stats writers are mvneta_rx() and mvneta_tx(). They don't lock anything
when they update the stats, and as a result, it randomly happens that
the stats freeze on SMP if two updates happen during stats retrieval.
Your patch is OK, but I dont understand how this freeze can happen.
TX and RX uses a separate syncp, and TX is protected by a lock, RX
is protected by NAPI bit.
But we can have multiple tx in parallel, one per queue. And it's only
when I explicitly bind two servers to two distinct CPU cores that I
can trigger the issue, which seems to confirm that this is the cause
of the issue.
Stats retrieval uses the appropriate BH disable before the fetches...
From the numerous printks I have added inside the syncp blocks, it
appears that the stats themselves are not responsible for the issue,
but the concurrent Tx are. I ended up several times stuck if I had
two Tx on different CPUs right before a stats retrieval. From the
info I found on the syncp docs, the caller is responsible for locking
and I don't see where there's any lock here since the syncp are global
and not even per tx queue.
But this stuff is very new to me, I can have missed something. That
said, I'm quite certain that the lock happened within the syncp blocks
and only in this case! At least my reading of the relevant includes
seemed to confirm to me that this hypothesis was valid :-/
Thanks,
Willy
Hi Ben,
On Sun, Jan 12, 2014 at 05:38:53PM +0000, Ben Hutchings wrote:
[Putting another hat on]
On Sun, 2014-01-12 at 17:55 +0100, Willy Tarreau wrote:
quoted
Hi Ben,
On Sun, Jan 12, 2014 at 04:49:51PM +0000, Ben Hutchings wrote:
(...)
quoted
quoted
So for now, let's simply ignore these timeouts generally caused by bugs
only.
No, don't ignore them. Schedule a work item to reset the device. (And
remember to cancel it when stopping the device.)
OK I can try to do that. Could you recommend me one driver which does this
successfully so that I can see exactly what needs to be taken care of ?
sfc does it, though the reset logic there is more complicated than you
would need.
OK.
I think this will DTRT, but it's compile-tested only.
OK, I'll test it ASAP. I think I can force the tx timeout by disabling
the link state detection and unplugging the cable during a transfer.
I have been given an OpenBlocks AX3 but haven't set it up yet.
Ah you're another lucky owner of this really great device :-)
I've sent Eric Leblond a complete howto in french, so it won't be of
a big use to you but if I can find some time and you don't find other
info, I can try to redo it in english. However you may be interested
in this article I put online with a few patches to make your life
easier :
http://1wt.eu/articles/openblocks-http-server/
It's a line-rate (1.488 Mpps) HTTP server I've done on it with a few
patches that may be useful for other network tests.
Cheers,
Willy
Hi Arnaud,
On Sun, Jan 12, 2014 at 08:21:20PM +0100, Arnaud Ebalard wrote:
Hi,
Willy Tarreau [off-list ref] writes:
quoted
this series provides some fixes for a number of issues met with the
mvneta driver :
- driver lockup when reading stats while sending traffic from multiple
CPUs : this obviously only happens on SMP and is the result of missing
locking on the driver. The problem was present since the introduction
of the driver in 3.8. The first patch performs some changes that are
needed for the second one which actually fixes the issue by using
per-cpu counters. It could make sense to backport this to the relevant
stable versions.
- mvneta_tx_timeout calls various functions to reset the NIC, and these
functions sleep, which is not allowed here, resulting in a panic.
Better completely disable this Tx timeout handler for now since it is
never called. The problem was encountered while developing some new
features, it's uncertain whether it's possible to reproduce it with
regular usage, so maybe a backport to stable is not needed.
- replace the Tx timer with a real Tx IRQ. As first reported by Arnaud
Ebalard and explained by Eric Dumazet, there is no way this driver
can work correctly if it uses a driver to recycle the Tx descriptors.
If too many packets are sent at once, the driver quickly ends up with
no descriptors (which happens twice as easily in GSO) and has to wait
10ms for recycling its descriptors and being able to send again. Eric
has worked around this in the core GSO code. But still when routing
traffic or sending UDP packets, the limitation is very visible. Using
Tx IRQs allows Tx descriptors to be recycled when sent. The coalesce
value is still configurable using ethtool. This fix turns the UDP
send bitrate from 134 Mbps to 987 Mbps (ie: line rate). It's made of
two patches, one to add the relevant bits from the original Marvell's
driver, and another one to implement the change. I don't know if it
should be backported to stable, as the bug only causes poor performance.
First, thanks a lot for that work!
Funny enough, I spent some time this week-end trying to find the root
cause of some kernel freezes and panics appearing randomly after some GB
read on a ReadyNAS 102 configured as a NFS server.
I tested your fixes and performance series together on top of current
3.13.0-rc7 and I am now unable to reproduce the freeze and panics after
having read more than the 300GB of traffic from the NAS: following
bandwith with a bwm-ng shows the rate is also far more stable than w/
previous driver logic (55MB/sec). So, FWIW:
Tested-by: Arnaud Ebalard <redacted>
Thanks for this.
BTW, the "performance" series is not supposed to fix anything, and still
it seems difficult to me to find what patch might have fixed your problem.
Maybe the timer used in place of an IRQ has an even worse effect than what
we could imagine ?
Willy, I can extend the test to RN2120 if you think it is useful to also
do additional tests on a dual-core armada XP.
It's up to you. These patches have run extensively on my Mirabox (Armada370),
OpenBlocks AX3 (ArmadaXP dual core) and the XP-GP board (ArmadaXP quad core),
and fixed the stability issues and performance issues I was facing there. But
you may be interested in testing them with your workloads (none of my boxes
is used as an NFS server, NAS or whatever, they mainly see HTTP and very small
packets used in stress tests).
Now, just in case someone on netdev can find something useful in the
panics I gathered before your set, I have added those below. The fact
that the bugs have disappeared with your set would tend to confirm that
it was in the driver but at some point during the tests, I suspected the
TCP stack when the device is stressed (NFS seems to do that very well on
a 1.2GHz/512MB device). I did the following tests:
- on a 3.12.5, 3.13.0-rc4 and rc7 on a ReadyNAS 102
- 3.13.0-rc7 on a RN2120 (dual-core Armada XP w/ mvneta and 2GB of RAM):
no issue seen after 200GB of traffic transfered
- 3.13.0-rc7 on a Duo v2 (kirkwood 88F6282 @ 1.6GHz w/ mv643xx_eth): no
issue
To be completely transparent, I've already faced some panics on the mirabox
during high speed testing (when trying to send 1.488 Mpps on the two gig
ports in parallel). But I've always suspected a power supply issue and never
dug deeper. I've also read some instability reports on some mirabox, so it's
pretty possible that some design rules for the armada370 are not perfectly
respected or too hard to apply and that we seldom meet hardware issues.
Cheers,
Willy
From: Eric Dumazet <hidden> Date: 2014-01-13 00:45:05
On Sun, 2014-01-12 at 23:09 +0100, Willy Tarreau wrote:
But we can have multiple tx in parallel, one per queue. And it's only
when I explicitly bind two servers to two distinct CPU cores that I
can trigger the issue, which seems to confirm that this is the cause
of the issue.
So this driver has multiqueue ?
Definitely it should have one syncp per queue.
Or per cpu stats, as your patch did.
Thanks !
From: Eric Dumazet <hidden> Date: 2014-01-13 00:48:07
On Sun, 2014-01-12 at 10:31 +0100, Willy Tarreau wrote:
This patch implements this. It merges both rx_stats and tx_stats into
a single "stats" member with a single syncp. Both mvneta_rx() and
mvneta_rx() now only update the a single CPU's counters.
From: Eric Dumazet <hidden> Date: 2014-01-13 00:49:53
On Sun, 2014-01-12 at 10:31 +0100, Willy Tarreau wrote:
Better count packets and bytes in the stack and on 32 bit then
accumulate them at the end for once. This saves two memory writes
and two memory barriers per packet. The incoming packet rate was
increased by 4.7% on the Openblocks AX3 thanks to this.
Cc: Thomas Petazzoni <redacted>
Cc: Gregory CLEMENT <redacted>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/marvell/mvneta.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
Reviewed-by: Eric Dumazet <edumazet@google.com>
Note that with such a cost, one has to wonder why we keep 64bit stats
for this NIC on 32bit hosts...
On Sun, Jan 12, 2014 at 04:45:03PM -0800, Eric Dumazet wrote:
On Sun, 2014-01-12 at 23:09 +0100, Willy Tarreau wrote:
quoted
But we can have multiple tx in parallel, one per queue. And it's only
when I explicitly bind two servers to two distinct CPU cores that I
can trigger the issue, which seems to confirm that this is the cause
of the issue.
So this driver has multiqueue ?
Yes, it defaults to 8 queues in each direction.
Definitely it should have one syncp per queue.
Or per cpu stats, as your patch did.
OK thank you for your review and explanation then, I'm reassured :-)
Thanks,
Willy
On Sun, Jan 12, 2014 at 04:49:52PM -0800, Eric Dumazet wrote:
On Sun, 2014-01-12 at 10:31 +0100, Willy Tarreau wrote:
quoted
Better count packets and bytes in the stack and on 32 bit then
accumulate them at the end for once. This saves two memory writes
and two memory barriers per packet. The incoming packet rate was
increased by 4.7% on the Openblocks AX3 thanks to this.
Cc: Thomas Petazzoni <redacted>
Cc: Gregory CLEMENT <redacted>
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/marvell/mvneta.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
Reviewed-by: Eric Dumazet <edumazet@google.com>
Note that with such a cost, one has to wonder why we keep 64bit stats
for this NIC on 32bit hosts...
At least this avoids wrapping if stats are not retrieved often enough.
As someone who had to support 32-bit stats in production on a firewall
running on kernel 2.4, I can say it really becomes a problem to graph
activity if stats are not collected as often as every 30 seconds, which
is short in certain environments.
Thanks,
Willy
Funny enough, I spent some time this week-end trying to find the root
cause of some kernel freezes and panics appearing randomly after some GB
read on a ReadyNAS 102 configured as a NFS server.
I tested your fixes and performance series together on top of current
3.13.0-rc7 and I am now unable to reproduce the freeze and panics after
having read more than the 300GB of traffic from the NAS: following
bandwith with a bwm-ng shows the rate is also far more stable than w/
previous driver logic (55MB/sec). So, FWIW:
Tested-by: Arnaud Ebalard <redacted>
Thanks for this.
BTW, the "performance" series is not supposed to fix anything,
I was lazy and wanted to give the whole set a try in a single pass.
and still it seems difficult to me to find what patch might have fixed
your problem. Maybe the timer used in place of an IRQ has an even
worse effect than what we could imagine ?
I guess so.
quoted
Willy, I can extend the test to RN2120 if you think it is useful to also
do additional tests on a dual-core armada XP.
It's up to you. These patches have run extensively on my Mirabox (Armada370),
OpenBlocks AX3 (ArmadaXP dual core) and the XP-GP board (ArmadaXP quad core),
and fixed the stability issues and performance issues I was facing there. But
you may be interested in testing them with your workloads (none of my boxes
is used as an NFS server, NAS or whatever, they mainly see HTTP and very small
packets used in stress tests).
Well, I spent the evening on my RN104 (Aramda370 w/ 2 GbE ifaces) and my
RN2120 (Dual core ArmadaXP w/ 2GbE ifaces) using one as a router and
serving NFS traffic from the other (and then changing roles). I passed
hundreds of GB of TCP/NFS traffic and did not see any issue.
Additionally, FWIW, testing both using netperf show they easily support
routing traffic w/ line rate perf.
Regarding the patches, the problem they solve impacts all Armada boards
(370 and XP) which are used for network tasks. I think it would be nice
to have those backported to stable. I can commit to do the tests of the
backports both on XP and 370 hardware down to 3.12 or 3.11 kernel if it
can help.
Cheers,
a+
@@ -1935,14 +1907,22 @@ static int mvneta_poll(struct napi_struct *napi, int budget) /* Read cause register */ cause_rx_tx = mvreg_read(pp, MVNETA_INTR_NEW_CAUSE) &- MVNETA_RX_INTR_MASK(rxq_number);+ (MVNETA_RX_INTR_MASK(rxq_number) | MVNETA_TX_INTR_MASK(txq_number));++ /* Release Tx descriptors */+ if (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL) {+ int tx_todo = 0;++ mvneta_tx_done_gbe(pp, (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL), &tx_todo);+ cause_rx_tx &= ~MVNETA_TX_INTR_MASK_ALL;+ }
Unless I missed something, tx_todo above is just here to make the
compiler happy w/ current prototype of mvneta_tx_done_gbe() but is
otherwise unused: you could simply remove the third parameter of the
function (it is only used here) and remove tx_todo.
Additionally, as you do not use the return value of the function, you
could probably make it void and spare some additional cycles by removing
the computation of the return value. While at it, mvneta_txq_done()
could also be made void.
The patch below gives the idea, it's compile-tested only and applies on
your whole set (fixes + perf).
Index: linux/drivers/net/ethernet/marvell/mvneta.c
===================================================================
@@ -1314,25 +1314,23 @@}/* Handle end of transmission */-staticintmvneta_txq_done(structmvneta_port*pp,+staticvoidmvneta_txq_done(structmvneta_port*pp,structmvneta_tx_queue*txq){structnetdev_queue*nq=netdev_get_tx_queue(pp->dev,txq->id);inttx_done;tx_done=mvneta_txq_sent_desc_proc(pp,txq);-if(tx_done==0)-returntx_done;-mvneta_txq_bufs_free(pp,txq,tx_done);+if(tx_done){+mvneta_txq_bufs_free(pp,txq,tx_done);-txq->count-=tx_done;+txq->count-=tx_done;-if(netif_tx_queue_stopped(nq)){-if(txq->size-txq->count>=MAX_SKB_FRAGS+1)-netif_tx_wake_queue(nq);+if(netif_tx_queue_stopped(nq)){+if(txq->size-txq->count>=MAX_SKB_FRAGS+1)+netif_tx_wake_queue(nq);+}}--returntx_done;}staticvoid*mvneta_frag_alloc(conststructmvneta_port*pp)
@@ -1704,30 +1702,23 @@/* Handle tx done - called in softirq context. The <cause_tx_done> argument*mustbeavalidcauseaccordingtoMVNETA_TXQ_INTR_MASK_ALL.*/-staticu32mvneta_tx_done_gbe(structmvneta_port*pp,u32cause_tx_done,-int*tx_todo)+staticvoidmvneta_tx_done_gbe(structmvneta_port*pp,u32cause_tx_done){structmvneta_tx_queue*txq;-u32tx_done=0;structnetdev_queue*nq;-*tx_todo=0;while(cause_tx_done){txq=mvneta_tx_done_policy(pp,cause_tx_done);nq=netdev_get_tx_queue(pp->dev,txq->id);__netif_tx_lock(nq,smp_processor_id());-if(txq->count){-tx_done+=mvneta_txq_done(pp,txq);-*tx_todo+=txq->count;-}+if(txq->count)+mvneta_txq_done(pp,txq);__netif_tx_unlock(nq);cause_tx_done&=~((1<<txq->id));}--returntx_done;}/* Compute crc8 of the specified address, using a unique algorithm ,
Hi Arnaud,
On Mon, Jan 13, 2014 at 11:36:05PM +0100, Arnaud Ebalard wrote:
Hi,
Willy Tarreau [off-list ref] writes:
quoted
quoted
Funny enough, I spent some time this week-end trying to find the root
cause of some kernel freezes and panics appearing randomly after some GB
read on a ReadyNAS 102 configured as a NFS server.
I tested your fixes and performance series together on top of current
3.13.0-rc7 and I am now unable to reproduce the freeze and panics after
having read more than the 300GB of traffic from the NAS: following
bandwith with a bwm-ng shows the rate is also far more stable than w/
previous driver logic (55MB/sec). So, FWIW:
Tested-by: Arnaud Ebalard <redacted>
Thanks for this.
BTW, the "performance" series is not supposed to fix anything,
I was lazy and wanted to give the whole set a try in a single pass.
quoted
and still it seems difficult to me to find what patch might have fixed
your problem. Maybe the timer used in place of an IRQ has an even
worse effect than what we could imagine ?
I guess so.
quoted
quoted
Willy, I can extend the test to RN2120 if you think it is useful to also
do additional tests on a dual-core armada XP.
It's up to you. These patches have run extensively on my Mirabox (Armada370),
OpenBlocks AX3 (ArmadaXP dual core) and the XP-GP board (ArmadaXP quad core),
and fixed the stability issues and performance issues I was facing there. But
you may be interested in testing them with your workloads (none of my boxes
is used as an NFS server, NAS or whatever, they mainly see HTTP and very small
packets used in stress tests).
Well, I spent the evening on my RN104 (Aramda370 w/ 2 GbE ifaces) and my
RN2120 (Dual core ArmadaXP w/ 2GbE ifaces) using one as a router and
serving NFS traffic from the other (and then changing roles). I passed
hundreds of GB of TCP/NFS traffic and did not see any issue.
Additionally, FWIW, testing both using netperf show they easily support
routing traffic w/ line rate perf.
Regarding the patches, the problem they solve impacts all Armada boards
(370 and XP) which are used for network tasks. I think it would be nice
to have those backported to stable. I can commit to do the tests of the
backports both on XP and 370 hardware down to 3.12 or 3.11 kernel if it
can help.
I think so. I've been successfully using them from 3.10 and upwards.
Cheers,
Willy
On Tue, Jan 14, 2014 at 12:22:03AM +0100, Arnaud Ebalard wrote:
Hi Willy,
Willy Tarreau [off-list ref] writes:
quoted
@@ -1935,14 +1907,22 @@ static int mvneta_poll(struct napi_struct *napi, int budget) /* Read cause register */ cause_rx_tx = mvreg_read(pp, MVNETA_INTR_NEW_CAUSE) &- MVNETA_RX_INTR_MASK(rxq_number);+ (MVNETA_RX_INTR_MASK(rxq_number) | MVNETA_TX_INTR_MASK(txq_number));++ /* Release Tx descriptors */+ if (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL) {+ int tx_todo = 0;++ mvneta_tx_done_gbe(pp, (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL), &tx_todo);+ cause_rx_tx &= ~MVNETA_TX_INTR_MASK_ALL;+ }
Unless I missed something, tx_todo above is just here to make the
compiler happy w/ current prototype of mvneta_tx_done_gbe() but is
otherwise unused: you could simply remove the third parameter of the
function (it is only used here) and remove tx_todo.
A number of such changes could be done but should be merged separately,
along with the cleanup and improvement series.
Additionally, as you do not use the return value of the function, you
could probably make it void and spare some additional cycles by removing
the computation of the return value. While at it, mvneta_txq_done()
could also be made void.
The patch below gives the idea, it's compile-tested only and applies on
your whole set (fixes + perf).
You should propose your patches for net-next on top of my series, really,
it's not too late.
Please see my comments below.
@@ -1314,25 +1314,23 @@}/* Handle end of transmission */-staticintmvneta_txq_done(structmvneta_port*pp,+staticvoidmvneta_txq_done(structmvneta_port*pp,structmvneta_tx_queue*txq){structnetdev_queue*nq=netdev_get_tx_queue(pp->dev,txq->id);inttx_done;tx_done=mvneta_txq_sent_desc_proc(pp,txq);-if(tx_done==0)-returntx_done;-mvneta_txq_bufs_free(pp,txq,tx_done);+if(tx_done){+mvneta_txq_bufs_free(pp,txq,tx_done);
Better just use "if (tx_done == 0) return" above and avoid adding an
extra indent level by inverting the if, that makes the code more readable.
Hi Ben,
On Sun, Jan 12, 2014 at 05:38:53PM +0000, Ben Hutchings wrote:
I think this will DTRT, but it's compile-tested only. I have been given
an OpenBlocks AX3 but haven't set it up yet.
OK I just managed to test your patch. I managed to force a Tx timeout by
forcing the link to 100/half and transfering 1000 concurrent streams.
Unfortunately for now the patch doesn't manage to recover, and the system
randomly panics one or two seconds after the link is brought up. Twice the
system did not panic but I lost all communications until a down/up cycle,
after which a panic happened during transfers.
However I could verify that the scheduled function is correctly called. I
suspect that something else might be wrong in the driver's reset sequence
(eg: unmapping pages still in use by the NIC or I don't know what), but
your patch does exactly what it's supposed to do.
At least, if the restart function does not do anything, everything works
fine. I see that the function is called (I added printk there) and the
transfer is not perturbated at all anymore.
So now I'm wondering whether the right thing should not be to just keep
your scheduled function and make it only log that a timeout was caught.
Another point which bothers me is that I suspect we're triggering Tx
timeouts too fast, because I regularly get these on 100 Mbps during
regular traffic (which ended up in immediate panics with previous code).
Thanks,
Willy