Re: [RFC] virtio: orphan skbs if we're relying on timer to free them

2009-05-21 Thread Rusty Russell
On Tue, 19 May 2009 12:10:13 pm David Miller wrote:
 From: Rusty Russell ru...@rustcorp.com.au
 Date: Mon, 18 May 2009 22:18:47 +0930
  We check for finished xmit skbs on every xmit, or on a timer (unless
  the host promises to force an interrupt when the xmit ring is empty).
  This can penalize userspace tasks which fill their sockbuf.  Not much
  difference with TSO, but measurable with large numbers of packets.
 
  There are a finite number of packets which can be in the transmission
  queue.  We could fire the timer more than every 100ms, but that would
  just hurt performance for a corner case.  This seems neatest.
...
  Signed-off-by: Rusty Russell ru...@rustcorp.com.au

 If this is so great for virtio it would also be a great idea
 universally, but we don't do it.

 What you're doing by orphan'ing is creating a situation where a single
 UDP socket can loop doing sends and monopolize the TX queue of a
 device.  The only control we have over a sender for fairness in
 datagram protocols is that send buffer allocation.

Urgh, that hadn't even occurred to me.  Good point.

 I'm guilty of doing this too in the NIU driver, also because there I
 lack a TX queue empty interrupt and this can keep TCP sockets from
 getting stuck.

 I think we need a generic solution to this issue because it is getting
 quite common to see cases where the packets in the TX queue of a
 device can sit there indefinitely.

I haven't thought this through properly, but how about a hack where we don't 
orphan packets if the ring is over half full?

Then I guess we could overload the watchdog as a more general timer-after-no-
xmit?

Rusty.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization


Re: [RFC] virtio: orphan skbs if we're relying on timer to free them

2009-05-21 Thread David Miller
From: Rusty Russell ru...@rustcorp.com.au
Date: Thu, 21 May 2009 16:27:05 +0930

 On Tue, 19 May 2009 12:10:13 pm David Miller wrote:
 What you're doing by orphan'ing is creating a situation where a single
 UDP socket can loop doing sends and monopolize the TX queue of a
 device.  The only control we have over a sender for fairness in
 datagram protocols is that send buffer allocation.
 
 Urgh, that hadn't even occurred to me.  Good point.

Now this all is predicated on this actually mattering. :-)

You could argue that the scheduler as well as the size of the
TX queue should be limiting and enforcing fairness.

Someone really needs to test this.  Just skb_orphan() every packet
at the beginning of dev_hard_start_xmit(), then run some test
program with two clients looping out UDP packets to see if one
can monopolize the device and get a significantly larger amount
of TX resources than the other.  Repeat for 3, 4, 5, etc. clients.

 I haven't thought this through properly, but how about a hack where
 we don't orphan packets if the ring is over half full?

That would also work.  And for the NIU case this would be great
because I DO have a marker bit for triggering interrupts in the TX
descriptors.  There's just no all empty interrupt on TX (who
designs these things? :( ).

 Then I guess we could overload the watchdog as a more general
 timer-after-no- xmit?

Yes, but it means that teardown of a socket can be delayed up to
the amount of that timer.  Factor in all of this crazy
round_jiffies() stuff people do these days and it could cause
pauses for real use cases and drive users batty.

Probably the most profitable avenue is to see if this is a real issue
afterall (see above).  If we can get away with having the socket
buffer represent socket -- device space only, that's the most ideal
solution.  It will probably also improve performance a lot across the
board, especially on NUMA/SMP boxes as our TX complete events tend to
be in difference places than the SKB producer.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization


[RFC] virtio: orphan skbs if we're relying on timer to free them

2009-05-18 Thread Rusty Russell
We check for finished xmit skbs on every xmit, or on a timer (unless
the host promises to force an interrupt when the xmit ring is empty).
This can penalize userspace tasks which fill their sockbuf.  Not much
difference with TSO, but measurable with large numbers of packets.

There are a finite number of packets which can be in the transmission
queue.  We could fire the timer more than every 100ms, but that would
just hurt performance for a corner case.  This seems neatest.

With interrupt when Tx ring empty:
Seconds TxPkts  TxIRQs
 1G TCP Guest-Host:3.7632833   32758
 1M normal pings:   111 108 997463
 1M 1k pings (-l 120):  55  107 488920

Without interrupt, without orphaning:
 1G TCP Guest-Host:3.6432806   1
 1M normal pings:   106 108 1
 1M 1k pings (-l 120):  68  105 1

With orphaning:
 1G TCP Guest-Host:3.8632821   1
 1M normal pings:   102 107 1
 1M 1k pings (-l 120):  43  105 1

Signed-off-by: Rusty Russell ru...@rustcorp.com.au
---
 drivers/net/virtio_net.c |5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -522,6 +522,11 @@ static int start_xmit(struct sk_buff *sk
 {
struct virtnet_info *vi = netdev_priv(dev);
 
+   /* We queue a limited number; don't let that delay writers if
+* we are slow in getting tx interrupt. */
+   if (!vi-free_in_tasklet)
+   skb_orphan(skb);
+
 again:
/* Free up any pending old buffers before queueing new ones. */
free_old_xmit_skbs(vi);

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization


Re: [RFC] virtio: orphan skbs if we're relying on timer to free them

2009-05-18 Thread David Miller
From: Rusty Russell ru...@rustcorp.com.au
Date: Mon, 18 May 2009 22:18:47 +0930

 We check for finished xmit skbs on every xmit, or on a timer (unless
 the host promises to force an interrupt when the xmit ring is empty).
 This can penalize userspace tasks which fill their sockbuf.  Not much
 difference with TSO, but measurable with large numbers of packets.
 
 There are a finite number of packets which can be in the transmission
 queue.  We could fire the timer more than every 100ms, but that would
 just hurt performance for a corner case.  This seems neatest.
 ...
 Signed-off-by: Rusty Russell ru...@rustcorp.com.au

If this is so great for virtio it would also be a great idea
universally, but we don't do it.

What you're doing by orphan'ing is creating a situation where a single
UDP socket can loop doing sends and monopolize the TX queue of a
device.  The only control we have over a sender for fairness in
datagram protocols is that send buffer allocation.

I'm guilty of doing this too in the NIU driver, also because there I
lack a TX queue empty interrupt and this can keep TCP sockets from
getting stuck.

I think we need a generic solution to this issue because it is getting
quite common to see cases where the packets in the TX queue of a
device can sit there indefinitely.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization