similar to: bug in virtio network driver?

On Thursday 01 May 2008 00:31:46 Mark McLoughlin wrote: > virtio_net currently only frees old transmit skbs just > before queueing new ones. If the queue is full, it then > enables interrupts and waits for notification that more > work has been performed. Hi Mark, This patch is fine, but it's better to do it from skb_xmit_done(). Of course, this is usually called from an

On Thursday 01 May 2008 00:31:46 Mark McLoughlin wrote: > virtio_net currently only frees old transmit skbs just > before queueing new ones. If the queue is full, it then > enables interrupts and waits for notification that more > work has been performed. Hi Mark, This patch is fine, but it's better to do it from skb_xmit_done(). Of course, this is usually called from an

Various drivers do skb_orphan() because they do not free transmitted skbs in a timely manner (causing apps which hit their socket limits to stall, socket close to hang, etc.). DaveM points out that there are advantages to doing it generally (it's more likely to be on same CPU than after xmit), and I couldn't find any new starvation issues in simple benchmarking here. This patch adds

Various drivers do skb_orphan() because they do not free transmitted skbs in a timely manner (causing apps which hit their socket limits to stall, socket close to hang, etc.). DaveM points out that there are advantages to doing it generally (it's more likely to be on same CPU than after xmit), and I couldn't find any new starvation issues in simple benchmarking here. This patch adds

OK, here's a new attempt to use the new capacity api. I also added more comments to clarify the logic. Hope this is more readable. Let me know pls. This is on top of the patches applied by Rusty. Warning: untested. Posting now to give people chance to comment on the API. Changes from v1: - fix comment in patch 2 to correct confusion noted by Rusty - rewrite patch 3 along the lines

OK, here's a new attempt to use the new capacity api. I also added more comments to clarify the logic. Hope this is more readable. Let me know pls. This is on top of the patches applied by Rusty. Warning: untested. Posting now to give people chance to comment on the API. Changes from v1: - fix comment in patch 2 to correct confusion noted by Rusty - rewrite patch 3 along the lines

On newer hosts that support delayed tx interrupts, we probably don't have much to gain from orphaning packets early. Based on patch by Jason Wang. Note: this will likely degrade performance for hosts without event idx support. Various fallback options are available, including orphaning conditionally. Testing TBD. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> ---

On newer hosts that support delayed tx interrupts, we probably don't have much to gain from orphaning packets early. Based on patch by Jason Wang. Note: this will likely degrade performance for hosts without event idx support. Various fallback options are available, including orphaning conditionally. Testing TBD. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> ---

skb is only NULL the first time around: it's more correct to test for being under-budget. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> diff -r 2a94425ac7d5 drivers/net/virtio_net.c --- a/drivers/net/virtio_net.c Thu Nov 15 13:47:28 2007 +1100 +++ b/drivers/net/virtio_net.c Thu Nov 15 23:10:44 2007 +1100 @@ -198,8 +198,8 @@ again: if (vi->num < vi->max / 2)

skb is only NULL the first time around: it's more correct to test for being under-budget. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> diff -r 2a94425ac7d5 drivers/net/virtio_net.c --- a/drivers/net/virtio_net.c Thu Nov 15 13:47:28 2007 +1100 +++ b/drivers/net/virtio_net.c Thu Nov 15 23:10:44 2007 +1100 @@ -198,8 +198,8 @@ again: if (vi->num < vi->max / 2)

According to David, proper accounting and queueing (at all levels, not just TCP sockets) is more important than trying to skim a bunch of cycles by avoiding TX interrupts. Having an event to free the SKB is absolutely essential for the stack to operate correctly. This series tries to enable tx interrupt for virtio-net. The idea is simple: enable tx interrupt and schedule a tx napi to free old

According to David, proper accounting and queueing (at all levels, not just TCP sockets) is more important than trying to skim a bunch of cycles by avoiding TX interrupts. Having an event to free the SKB is absolutely essential for the stack to operate correctly. This series tries to enable tx interrupt for virtio-net. The idea is simple: enable tx interrupt and schedule a tx napi to free old

If we fail to transmit a packet, we assume the queue is full and put the skb into last_xmit_skb. However, if more space frees up before we xmit it, we loop, and the result can be transmitting the same skb twice. Fix is simple: set skb to NULL if we've used it in some way, and check before sending. Signed-off-by: Rusty Russell <rusty at rustcorp.com.au> --- drivers/net/virtio_net.c |

If we fail to transmit a packet, we assume the queue is full and put the skb into last_xmit_skb. However, if more space frees up before we xmit it, we loop, and the result can be transmitting the same skb twice. Fix is simple: set skb to NULL if we've used it in some way, and check before sending. Signed-off-by: Rusty Russell <rusty at rustcorp.com.au> --- drivers/net/virtio_net.c |

Hi, Back in October I reported that I noticed a problem whereby flow control breaks down when openvswitch is configured to mirror a port[1]. I have (finally) looked into this further and the problem appears to relate to cloning of skbs, as Jesse Gross originally suspected. More specifically, in do_execute_actions[2] the first n-1 times that an skb needs to be transmitted it is cloned first and

Hi, Back in October I reported that I noticed a problem whereby flow control breaks down when openvswitch is configured to mirror a port[1]. I have (finally) looked into this further and the problem appears to relate to cloning of skbs, as Jesse Gross originally suspected. More specifically, in do_execute_actions[2] the first n-1 times that an skb needs to be transmitted it is cloned first and

We free transmitted packets in ndo_start_xmit() in the past to get better performance in the past. One side effect is that skb_orphan() needs to be called in ndo_start_xmit() which makes sk_wmem_alloc not accurate in fact. For TCP protocol, this means several optimization could not work well such as TCP small queue and auto corking. This can lead extra low throughput of small packets stream.

We free transmitted packets in ndo_start_xmit() in the past to get better performance in the past. One side effect is that skb_orphan() needs to be called in ndo_start_xmit() which makes sk_wmem_alloc not accurate in fact. For TCP protocol, this means several optimization could not work well such as TCP small queue and auto corking. This can lead extra low throughput of small packets stream.

Herbert tells me that returning NETDEV_TX_BUSY from hard_start_xmit is seen as a poor thing to do; we should cache the packet and stop the queue. Signed-off-by: Rusty Russell <rusty at rustcorp.com.au> --- drivers/net/virtio_net.c | 58 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 18 deletions(-) diff -r 26ba2dd67f46 drivers/net/virtio_net.c ---

Herbert tells me that returning NETDEV_TX_BUSY from hard_start_xmit is seen as a poor thing to do; we should cache the packet and stop the queue. Signed-off-by: Rusty Russell <rusty at rustcorp.com.au> --- drivers/net/virtio_net.c | 58 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 18 deletions(-) diff -r 26ba2dd67f46 drivers/net/virtio_net.c ---