net: mvneta: fix Tx interrupt delay
Subsystems (3)
PATCH 1/1 net: mvneta: fix Tx interrupt delay
The mvneta driver sets the amount of Tx coalesce packets to 16 by default. Normally that does not cause any trouble since the driver uses a much larger Tx ring size (532 packets). But some sockets might run with very small buffers, much smaller than the equivalent of 16 packets. This is what ping is doing for example, by setting SNDBUF to 324 bytes rounded up to 2kB by the kernel.
The problem is that there is no documented method to force a specific packet to emit an interrupt (eg: the last of the ring) nor is it possible to make the NIC emit an interrupt after a given delay.
In this case, it causes trouble, because when ping sends packets over its raw socket, the few first packets leave the system, and the first 15 packets will be emitted without an IRQ being generated, so without the skbs being freed. And since the socket's buffer is small, there's no way to reach that amount of packets, and the ping ends up with "send: no buffer available" after sending 6 packets. Running with 3 instances of ping in parallel is enough to hide the problem, because with 6 packets per instance, that's 18 packets total, which is enough to grant a Tx interrupt before all are sent.
The original driver in the LSP kernel worked around this design flaw by using a software timer to clean up the Tx descriptors. This timer was slow and caused terrible network performance on some Tx-bound workloads (such as routing) but was enough to make tools like ping work correctly.
Instead here, we simply set the packet counts before interrupt to 1. This ensures that each packet sent will produce an interrupt. NAPI takes care of coalescing interrupts since the interrupt is disabled once generated.
No measurable performance impact nor CPU usage were observed on small nor large packets, including when saturating the link on Tx, and this fixes tools like ping which rely on too small a send buffer. If one wants to increase this value for certain workloads where it is safe to do so, "ethtool -C $dev tx-frames" will override this default setting.
This fix needs to be applied to stable kernels starting with 3.10.
--- drivers/net/ethernet/marvell/mvneta.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/marvell/mvneta.c +1 -1
@@ -214,7 +214,7 @@/* Various constants */ /* Coalescing */ -#define MVNETA_TXDONE_COAL_PKTS 16 +#define MVNETA_TXDONE_COAL_PKTS 1#define MVNETA_RX_COAL_PKTS 32#define MVNETA_RX_COAL_USEC 100
Signature · 1 line
1.7.12.2.21.g234cd45.dirty
Quoted 53 lines · Willy Tarreau
The mvneta driver sets the amount of Tx coalesce packets to 16 by default. Normally that does not cause any trouble since the driver uses a much larger Tx ring size (532 packets). But some sockets might run with very small buffers, much smaller than the equivalent of 16 packets. This is what ping is doing for example, by setting SNDBUF to 324 bytes rounded up to 2kB by the kernel.
The problem is that there is no documented method to force a specific packet to emit an interrupt (eg: the last of the ring) nor is it possible to make the NIC emit an interrupt after a given delay.
In this case, it causes trouble, because when ping sends packets over its raw socket, the few first packets leave the system, and the first 15 packets will be emitted without an IRQ being generated, so without the skbs being freed. And since the socket's buffer is small, there's no way to reach that amount of packets, and the ping ends up with "send: no buffer available" after sending 6 packets. Running with 3 instances of ping in parallel is enough to hide the problem, because with 6 packets per instance, that's 18 packets total, which is enough to grant a Tx interrupt before all are sent.
The original driver in the LSP kernel worked around this design flaw by using a software timer to clean up the Tx descriptors. This timer was slow and caused terrible network performance on some Tx-bound workloads (such as routing) but was enough to make tools like ping work correctly.
Instead here, we simply set the packet counts before interrupt to 1. This ensures that each packet sent will produce an interrupt. NAPI takes care of coalescing interrupts since the interrupt is disabled once generated.
No measurable performance impact nor CPU usage were observed on small nor large packets, including when saturating the link on Tx, and this fixes tools like ping which rely on too small a send buffer. If one wants to increase this value for certain workloads where it is safe to do so, "ethtool -C $dev tx-frames" will override this default setting.
This fix needs to be applied to stable kernels starting with 3.10.
Tested-by Maggie Mae Roxas <[email protected]>Signed-off-by Willy Tarreau <[email protected]>--- drivers/net/ethernet/marvell/mvneta.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/net/ethernet/marvell/mvneta.c +0
-0
drivers/net/ethernet/marvell/mvneta.c line 220
Quoted 8 lines
drivers/net/ethernet/marvell/mvneta.c +1
-1@@ -214,7 +214,7 @@/* Various constants */ /* Coalescing */ -#define MVNETA_TXDONE_COAL_PKTS 16 +#define MVNETA_TXDONE_COAL_PKTS 1#define MVNETA_RX_COAL_PKTS 32#define MVNETA_RX_COAL_USEC 100
I am surprised TCP even worked correctly with this problem.
I highly suggest BQL for this driver, now this issue is fixed.
I wonder if this high setting was not because of race conditions in the driver :
mvneta_tx() seems to access skb->len too late, TX completion might have already freed skb :
u64_stats_update_begin(&stats->syncp);
stats->tx_packets++;
stats->tx_bytes += skb->len; // potential use after free
u64_stats_update_end(&stats->syncp);Thanks Willy !
PATCH 1/1 net: mvneta: fix race condition in mvneta_tx()
From: Eric Dumazet <[email protected]>
mvneta_tx() dereferences skb to get skb->len too late, as hardware might have completed the transmit and TX completion could have freed the skb from another cpu.
--- drivers/net/ethernet/marvell/mvneta.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/marvell/mvneta.c +2 -1
@@ -1721,6 +1721,7 @@ static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)u16 txq_id =skb_get_queue_mapping (skb);struct mvneta_tx_queue *txq = &pp->txqs[txq_id];struct mvneta_tx_desc *tx_desc; +int len = skb->len;int frags =0 ;u32 tx_cmd;
@@ -1788,7 +1789,7 @@ out:u64_stats_update_begin (&stats->syncp); stats->tx_packets++; - stats->tx_bytes += skb->len; + stats->tx_bytes += len;u64_stats_update_end (&stats->syncp); }else { dev->stats.tx_dropped++;
Re: [PATCH] net: mvneta: fix race condition in mvneta_tx()
Quoted 7 lines · Eric Dumazet
From: Eric Dumazet <[email protected]>
mvneta_tx()dereferences skb to getskb->lentoo late, as hardware might have completed the transmit and TX completion could have freed the skb from another cpu.Signed-off-by Eric Dumazet <[email protected]>
For completeness, this bug was added in linux-3.14 and seems a stable candidate.
Hi Eric,
Quoted 7 lines · Eric Dumazet
Quoted 7 lines
drivers/net/ethernet/marvell/mvneta.c +0
-0@@ -214,7 +214,7 @@/* Various constants */
Quoted 9 lines
drivers/net/ethernet/marvell/mvneta.c line 220
Quoted 6 lines
drivers/net/ethernet/marvell/mvneta.c +1
-1/* Coalescing */ -#define MVNETA_TXDONE_COAL_PKTS 16 +#define MVNETA_TXDONE_COAL_PKTS 1#define MVNETA_RX_COAL_PKTS 32#define MVNETA_RX_COAL_USEC 100I am surprised TCP even worked correctly with this problem.
There are multiple explanations to this :
- we used to flush Tx queues upon Rx interrupt, which used to hide the problem.
- we tend to have large socket buffers, which cover the issue. I've never had any issue at high data rates. After all only 23kB send buffer is needed to get it to work.
- most often you have multiple parallel streams which hide the issue even more.
Quoted 1 line
I highly suggest BQL for this driver, now this issue is fixed.
How does that work ?
Quoted 10 lines
I wonder if this high setting was not because of race conditions in the driver :
mvneta_tx()seems to accessskb->lentoo late, TX completion might have already freed skb :u64_stats_update_begin(&stats->syncp); stats->tx_packets++; stats->tx_bytes += skb->len; // potential use after free u64_stats_update_end(&stats->syncp);
Good catch! But no, this is unrelated since it does not fix the race either. Initially this driver used to implement a timer to flush the Tx queue after 10ms, resulting in abysmal Tx-only performance as you can easily imagine. In my opinion there's a design flaw in the chip, and they did everything they could to workaround it (let's not say "hide it"), but that was not enough. When I "fixed" the performance issue by enabling the Tx interrupt, I kept the value 16 which gave pretty good results to me, without realizing that there was this corner case :-/
Quoted 1 line
Thanks Willy !
Thanks for your review :-)
Willy
Re: [PATCH] net: mvneta: fix race condition in mvneta_tx()
In reply to Eric DumazetQuoted 7 lines · Eric Dumazet
Quoted 6 lines · Eric Dumazet
From: Eric Dumazet <[email protected]>
mvneta_tx()dereferences skb to getskb->lentoo late, as hardware might have completed the transmit and TX completion could have freed the skb from another cpu.
Quoted 8 lines
Quoted 1 line
Signed-off-by Eric Dumazet <[email protected]>For completeness, this bug was added in linux-3.14 and seems a stable candidate.
Fixes 71f6d1b31fb1 ("net: mvneta: replace Tx timer with a real interrupt")
Absolutely, we backported this one back to 3.10.
Thanks Eric!
Willy
Quoted 7 lines · Willy Tarreau
Hi Eric,
Quoted 1 line · Eric Dumazet
I highly suggest BQL for this driver, now this issue is fixed.
How does that work ?
This works very well ;)
Its super easy to implement, take a look at commits bdbc063129e811264cd6c311d8c2d9b95de01231 or 7070ce0a6419a118842298bc967061ad6cea40db
Quoted 7 lines · Eric Dumazet
Quoted 6 lines · Willy Tarreau
Hi Eric,
Quoted 1 line · Eric Dumazet
I highly suggest BQL for this driver, now this issue is fixed.
Quoted 7 lines
Quoted 1 line
How does that work ?
This works very well ;)
Its super easy to implement, take a look at commits
bdbc063129e811264cd6c311d8c2d9b95de01231or7070ce0a6419a118842298bc967061ad6cea40db
Thanks but I'm not sure I entirely understand the concept. Is it to notify the sender that the packets were already queued for the NIC ? And if so, how does that improve the situation ? I'm sorry if this sounds like a stupid question, it's just that the concept by itself is not clear to me.
Willy
Quoted 5 lines · Willy Tarreau
Thanks but I'm not sure I entirely understand the concept. Is it to notify the sender that the packets were already queued for the NIC ? And if so, how does that improve the situation ? I'm sorry if this sounds like a stupid question, it's just that the concept by itself is not clear to me.
http://lwn.net/Articles/454390/
BQL is first step to fight so called bufferbloat.
https://www.ietf.org/proceedings/86/slides/slides-86-iccrg-0.pdf
Eric,
On 12/02/2014 09:18 AM, Eric Dumazet wrote:
[..]
Quoted 5 lines
I am surprised TCP even worked correctly with this problem.
I highly suggest BQL for this driver, now this issue is fixed.
Implementing BQL for the mvneta driver was something I wanted to do a while ago, but you explained that these kind drivers (i.e. those with software TSO) didn't need BQL, because the latency that resulted from the ring was too small.
Quoting (http://www.spinics.net/lists/netdev/msg284439.html):
""
Note that a full size TSO packet (44 or 45 MSS) requires about 88 or 90 descriptors.
So I do not think BQL is really needed, because a 512 slots TX ring wont add a big latency : About 5 ms max.
BQL is generally nice for NIC supporting hardware TSO, where a 64KB TSO packet consumes 3 or 4 descriptors.
Also note that TCP Small Queues should limit TX ring occupancy of a single bulk flow anyway.
""
Maybe I misunderstood something?
Signature · 3 lines
Ezequiel García, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com
Quoted 15 lines · Ezequiel Garcia
Eric,
On 12/02/2014 09:18 AM, Eric Dumazet wrote:
[..]Quoted 5 lines
I am surprised TCP even worked correctly with this problem.
I highly suggest BQL for this driver, now this issue is fixed.
Implementing BQL for the mvneta driver was something I wanted to do a while ago, but you explained that these kind drivers (i.e. those with software TSO) didn't need BQL, because the latency that resulted from the ring was too small.
Quoted 7 lines
Quoting (http://www.spinics.net/lists/netdev/msg284439.html):
""
Note that a full size TSO packet (44 or 45 MSS) requires about 88 or 90 descriptors.So I do not think BQL is really needed, because a 512 slots TX ring wont add a big latency : About 5 ms max.
at a gigabit.
Wildly variable with pause frames. And: 50ms at 100mbit. 500ms at 10mbit.
Secondly, why accept any latency in the driver when you can get it well below 2ms in most circumstances with a couple lines of BQL code?
Quoted 3 lines
BQL is generally nice for NIC supporting hardware TSO, where a 64KB TSO packet consumes 3 or 4 descriptors.
BQL is nice at any speed, and darn useful at lower speeds, as well.
Quoted 3 lines
Also note that TCP Small Queues should limit TX ring occupancy of a single bulk flow anyway.
We only do one upload or one download at a time on our systems, followed by a ping measurement... never do more than one flow at the same time, right?
Quoted 3 lines
""
Maybe I misunderstood something?
Try it, test it, you'll like it.
Quoted 8 lines
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Dave Täht http://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks
Quoted 23 lines · Ezequiel Garcia
Eric,
On 12/02/2014 09:18 AM, Eric Dumazet wrote:
[..]Quoted 5 lines
I am surprised TCP even worked correctly with this problem.
I highly suggest BQL for this driver, now this issue is fixed.
Implementing BQL for the mvneta driver was something I wanted to do a while ago, but you explained that these kind drivers (i.e. those with software TSO) didn't need BQL, because the latency that resulted from the ring was too small.
Quoting (http://www.spinics.net/lists/netdev/msg284439.html):
""
Note that a full size TSO packet (44 or 45 MSS) requires about 88 or 90 descriptors.So I do not think BQL is really needed, because a 512 slots TX ring wont add a big latency : About 5 ms max.
Quoted 8 lines
BQL is generally nice for NIC supporting hardware TSO, where a 64KB TSO packet consumes 3 or 4 descriptors.
Also note that TCP Small Queues should limit TX ring occupancy of a single bulk flow anyway.
""Maybe I misunderstood something?
This was indeed the case, but we added recently xmit_more support, and it uses BQL information. So you might add BQL anyway, if xmit_more support is useful for this hardware.
David Miller
maintainerFrom: Willy Tarreau <[email protected]>
Date: Tue, 2 Dec 2014 08:13:04 +0100
Quoted 39 lines
The mvneta driver sets the amount of Tx coalesce packets to 16 by default. Normally that does not cause any trouble since the driver uses a much larger Tx ring size (532 packets). But some sockets might run with very small buffers, much smaller than the equivalent of 16 packets. This is what ping is doing for example, by setting SNDBUF to 324 bytes rounded up to 2kB by the kernel.
The problem is that there is no documented method to force a specific packet to emit an interrupt (eg: the last of the ring) nor is it possible to make the NIC emit an interrupt after a given delay.
In this case, it causes trouble, because when ping sends packets over its raw socket, the few first packets leave the system, and the first 15 packets will be emitted without an IRQ being generated, so without the skbs being freed. And since the socket's buffer is small, there's no way to reach that amount of packets, and the ping ends up with "send: no buffer available" after sending 6 packets. Running with 3 instances of ping in parallel is enough to hide the problem, because with 6 packets per instance, that's 18 packets total, which is enough to grant a Tx interrupt before all are sent.
The original driver in the LSP kernel worked around this design flaw by using a software timer to clean up the Tx descriptors. This timer was slow and caused terrible network performance on some Tx-bound workloads (such as routing) but was enough to make tools like ping work correctly.
Instead here, we simply set the packet counts before interrupt to 1. This ensures that each packet sent will produce an interrupt. NAPI takes care of coalescing interrupts since the interrupt is disabled once generated.
No measurable performance impact nor CPU usage were observed on small nor large packets, including when saturating the link on Tx, and this fixes tools like ping which rely on too small a send buffer. If one wants to increase this value for certain workloads where it is safe to do so, "ethtool -C $dev tx-frames" will override this default setting.
Quoted 4 lines
This fix needs to be applied to stable kernels starting with 3.10.
Tested-by Maggie Mae Roxas <[email protected]>Signed-off-by Willy Tarreau <[email protected]>
Applied and queued up for -stable, thanks.
Re: [PATCH] net: mvneta: fix race condition in mvneta_tx()
In reply to Willy TarreauFrom: Willy Tarreau <[email protected]>
Date: Tue, 2 Dec 2014 13:40:43 +0100
Quoted 12 lines · Eric Dumazet
Quoted 11 lines · Eric Dumazet
Quoted 7 lines
From: Eric Dumazet <[email protected]>
mvneta_tx()dereferences skb to getskb->lentoo late, as hardware might have completed the transmit and TX completion could have freed the skb from another cpu.Signed-off-by Eric Dumazet <[email protected]>
Quoted 6 lines
Quoted 4 lines
For completeness, this bug was added in linux-3.14 and seems a stable candidate.
Fixes 71f6d1b31fb1 ("net: mvneta: replace Tx timer with a real interrupt")Absolutely, we backported this one back to 3.10.
Applied, and queued up for -stable, thanks everyone.