Earlier approach to improving small packet performance went
along the lines of dropping packets when the txq is full to
avoid stop/start of the txq. Though performance improved
significantly (upto 3x) for a single thread, multiple netperf
sessions showed a regression of upto -17% (starting from 4
sessions).
This patch proposes a different approach with the following
changes:
A. virtio:
- Provide a API to get available number of slots.
B. virtio-net:
- Remove stop/start txq's and associated callback.
- Pre-calculate the number of slots needed to transmit
the skb in xmit_skb and bail out early if enough space
is not available. My testing shows that 2.5-3% of
packets are benefited by using this API.
- Do not drop skbs but instead return TX_BUSY like other
drivers.
- When returning EBUSY, set a per-txq variable to indicate
to dev_queue_xmit() whether to restart xmits on this txq.
C. net/sched/sch_generic.c:
Since virtio-net now returns EBUSY, the skb is requeued to
gso_skb. This allows adding the addional check for restart
xmits in just the slow-path (the first re-queued packet
case of dequeue_skb, where it checks for gso_skb) before
deciding whether to call the driver or not.
Patch was also tested between two servers with Emulex OneConnect
10G cards to confirm there is no regression. Though the patch is
an attempt to improve only small packet performance, there was
improvement for 1K, 2K and also 16K both in BW and SD. Results
from Guest -> Remote Host (BW in Mbps) for 1K and 16K I/O sizes:
________________________________________________________
I/O Size: 1K
# BW1 BW2 (%) SD1 SD2 (%)
________________________________________________________
1 1226 3313 (170.2) 6.6 1.9 (-71.2)
2 3223 7705 (139.0) 18.0 7.1 (-60.5)
4 7223 8716 (20.6) 36.5 29.7 (-18.6)
8 8689 8693 (0) 131.5 123.0 (-6.4)
16 8059 8285 (2.8) 578.3 506.2 (-12.4)
32 7758 7955 (2.5) 2281.4 2244.2 (-1.6)
64 7503 7895 (5.2) 9734.0 9424.4 (-3.1)
96 7496 7751 (3.4) 21980.9 20169.3 (-8.2)
128 7389 7741 (4.7) 40467.5 34995.5 (-13.5)
________________________________________________________
Summary: BW: 16.2% SD: -10.2%
________________________________________________________
I/O Size: 16K
# BW1 BW2 (%) SD1 SD2 (%)
________________________________________________________
1 6684 7019 (5.0) 1.1 1.1 (0)
2 7674 7196 (-6.2) 5.0 4.8 (-4.0)
4 7358 8032 (9.1) 21.3 20.4 (-4.2)
8 7393 8015 (8.4) 82.7 82.0 (-.8)
16 7958 8366 (5.1) 283.2 310.7 (9.7)
32 7792 8113 (4.1) 1257.5 1363.0 (8.3)
64 7673 8040 (4.7) 5723.1 5812.4 (1.5)
96 7462 7883 (5.6) 12731.8 12119.8 (-4.8)
128 7338 7800 (6.2) 21331.7 21094.7 (-1.1)
________________________________________________________
Summary: BW: 4.6% SD: -1.5%
Signed-off-by: Krishna Kumar <redacted>
---
Add a per-txq field that can (optionally) be set by participating
drivers to indicate when to restart tx.
Signed-off-by: Krishna Kumar <redacted>
---
include/linux/netdevice.h | 1 +
1 file changed, 1 insertion(+)
diff -ruNp org/include/linux/netdevice.h new/include/linux/netdevice.h
Introduce virtqueue_get_capacity() to help bail out of transmit
path early. Also remove notification when we run out of space (I
am not sure if this should be under a feature bit).
Signed-off-by: Krishna Kumar <redacted>
---
drivers/virtio/virtio_ring.c | 13 ++++++++-----
include/linux/virtio.h | 5 +++++
2 files changed, 13 insertions(+), 5 deletions(-)
diff -ruNp org/include/linux/virtio.h new/include/linux/virtio.h
@@ -156,6 +156,14 @@ static int vring_add_indirect(struct vrireturnhead;}+intvirtqueue_get_capacity(structvirtqueue*_vq)+{+structvring_virtqueue*vq=to_vvq(_vq);++returnvq->num_free;+}+EXPORT_SYMBOL_GPL(virtqueue_get_capacity);+intvirtqueue_add_buf_gfp(structvirtqueue*_vq,structscatterlistsg[],unsignedintout,
@@ -185,11 +193,6 @@ int virtqueue_add_buf_gfp(struct virtqueif(vq->num_free<out+in){pr_debug("Can't add buf len %i - avail = %i\n",out+in,vq->num_free);-/* FIXME: for historical reasons, we force a notify here if-*thereareoutgoingpartstothebuffer.Presumablythe-*hostshouldservicetheringASAP.*/-if(out)-vq->notify(&vq->vq);END_USE(vq);return-ENOSPC;}
Changes:
1. Remove xmit notification
2. free_old_xmit_skbs() frees upto a limit to reduce tx jitter.
3. xmit_skb() precalculates the number of slots and checks if
that is available. It assumes that we are not using
indirect descriptors at this time.
4. start_xmit() becomes a small routine that removes most error
checks, does not drop packets but instead returns EBUSY if
there is no space to transmit. It also sets when to restart
xmits in future.
Signed-off-by: Krishna Kumar <redacted>
---
drivers/net/virtio_net.c | 70 ++++++++++---------------------------
1 file changed, 20 insertions(+), 50 deletions(-)
diff -ruNp org/drivers/net/virtio_net.c new/drivers/net/virtio_net.c
@@ -117,17 +117,6 @@ static struct page *get_a_page(struct vireturnp;}-staticvoidskb_xmit_done(structvirtqueue*svq)-{-structvirtnet_info*vi=svq->vdev->priv;--/* Suppress further interrupts. */-virtqueue_disable_cb(svq);--/* We were probably waiting for more output buffers. */-netif_wake_queue(vi->dev);-}-staticvoidset_skb_frag(structsk_buff*skb,structpage*page,unsignedintoffset,unsignedint*len){
@@ -531,6 +519,12 @@ static int xmit_skb(struct virtnet_info pr_debug("%s: xmit %p %pM\n",vi->dev->name,skb,dest);+hdr->num_sg=skb_to_sgvec(skb,vi->tx_sg+1,0,skb->len)+1;+if(unlikely(hdr->num_sg>virtqueue_get_capacity(vi->svq))){+/* Don't rely on indirect descriptors when reaching capacity */+return-ENOSPC;+}+if(skb->ip_summed==CHECKSUM_PARTIAL){hdr->hdr.flags=VIRTIO_NET_HDR_F_NEEDS_CSUM;hdr->hdr.csum_start=skb_checksum_start_offset(skb);
@@ -566,7 +560,6 @@ static int xmit_skb(struct virtnet_info elsesg_set_buf(vi->tx_sg,&hdr->hdr,sizeofhdr->hdr);-hdr->num_sg=skb_to_sgvec(skb,vi->tx_sg+1,0,skb->len)+1;returnvirtqueue_add_buf(vi->svq,vi->tx_sg,hdr->num_sg,0,skb);}
@@ -574,30 +567,21 @@ static int xmit_skb(struct virtnet_info staticnetdev_tx_tstart_xmit(structsk_buff*skb,structnet_device*dev){structvirtnet_info*vi=netdev_priv(dev);-intcapacity;/* Free up any pending old buffers before queueing new ones. */free_old_xmit_skbs(vi);/* Try to transmit */-capacity=xmit_skb(vi,skb);+if(unlikely(xmit_skb(vi,skb)<0)){+structnetdev_queue*txq;-/* This can happen with OOM and indirect buffers. */-if(unlikely(capacity<0)){-if(net_ratelimit()){-if(likely(capacity==-ENOMEM)){-dev_warn(&dev->dev,-"TX queue failure: out of memory\n");-}else{-dev->stats.tx_fifo_errors++;-dev_warn(&dev->dev,-"Unexpected TX queue failure: %d\n",-capacity);-}-}-dev->stats.tx_dropped++;-kfree_skb(skb);-returnNETDEV_TX_OK;+/*+*Tellkerneltorestartxmitsafter1jiffytohelpthe+*hostcatchup.+*/+txq=netdev_get_tx_queue(dev,0);+txq->xmit_restart_jiffies=jiffies+1;+returnNETDEV_TX_BUSY;}virtqueue_kick(vi->svq);
@@ -605,20 +589,6 @@ static netdev_tx_t start_xmit(struct sk_skb_orphan(skb);nf_reset(skb);-/* Apparently nice girls don't return TX_BUSY; stop the queue-*beforeitgetsoutofhand.Naturally,thiswastesentries.*/-if(capacity<2+MAX_SKB_FRAGS){-netif_stop_queue(dev);-if(unlikely(!virtqueue_enable_cb(vi->svq))){-/* More just got used, free them then recheck. */-capacity+=free_old_xmit_skbs(vi);-if(capacity>=2+MAX_SKB_FRAGS){-netif_start_queue(dev);-virtqueue_disable_cb(vi->svq);-}-}-}-returnNETDEV_TX_OK;}
@@ -881,7 +851,7 @@ static int virtnet_probe(struct virtio_dstructnet_device*dev;structvirtnet_info*vi;structvirtqueue*vqs[3];-vq_callback_t*callbacks[]={skb_recv_done,skb_xmit_done,NULL};+vq_callback_t*callbacks[]={skb_recv_done,NULL,NULL};constchar*names[]={"input","output","control"};intnvqs;
Dequeue_skb has an additional check, for the first packet that
is requeued, to see if the device has requested xmits after a
interval. This is intended to not affect the fast xmit path, and
have minimal overhead to the slow path. Drivers setting the
restart time should not stop/start their tx queues, and hence
the frozen/stopped check can be avoided.
Signed-off-by: Krishna Kumar <redacted>
---
net/sched/sch_generic.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff -ruNp org/net/sched/sch_generic.c new/net/sched/sch_generic.c
@@ -50,17 +50,30 @@ static inline int dev_requeue_skb(structreturn0;}+/*+*Thisfunctioncanreturnararefalsepositivefordriverssetting+*xmit_restart_jiffies(e.g.virtio-net)whenxmit_restart_jiffiesis+*zerobutthedevicemaynotbeready.Thatonlyleadstotheskb+*beingrequeuedagain.+*/+staticinlineintcan_restart_xmit(structQdisc*q,structsk_buff*skb)+{+structnet_device*dev=qdisc_dev(q);+structnetdev_queue*txq;++txq=netdev_get_tx_queue(dev,skb_get_queue_mapping(skb));+if(unlikely(txq->xmit_restart_jiffies))+returntime_after_eq(jiffies,txq->xmit_restart_jiffies);+return!netif_tx_queue_frozen_or_stopped(txq);+}+staticinlinestructsk_buff*dequeue_skb(structQdisc*q){structsk_buff*skb=q->gso_skb;if(unlikely(skb)){-structnet_device*dev=qdisc_dev(q);-structnetdev_queue*txq;-/* check the reason of requeuing without tx lock first */-txq=netdev_get_tx_queue(dev,skb_get_queue_mapping(skb));-if(!netif_tx_queue_frozen_or_stopped(txq)){+if(can_restart_xmit(q,skb)){q->gso_skb=NULL;q->q.qlen--;}else
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2011-05-04 14:46:22
On Wed, May 04, 2011 at 07:32:58PM +0530, Krishna Kumar wrote:
Earlier approach to improving small packet performance went
along the lines of dropping packets when the txq is full to
avoid stop/start of the txq. Though performance improved
significantly (upto 3x) for a single thread, multiple netperf
sessions showed a regression of upto -17% (starting from 4
sessions).
This patch proposes a different approach with the following
changes:
A. virtio:
- Provide a API to get available number of slots.
B. virtio-net:
- Remove stop/start txq's and associated callback.
- Pre-calculate the number of slots needed to transmit
the skb in xmit_skb and bail out early if enough space
is not available. My testing shows that 2.5-3% of
packets are benefited by using this API.
- Do not drop skbs but instead return TX_BUSY like other
drivers.
- When returning EBUSY, set a per-txq variable to indicate
to dev_queue_xmit() whether to restart xmits on this txq.
C. net/sched/sch_generic.c:
Since virtio-net now returns EBUSY, the skb is requeued to
gso_skb. This allows adding the addional check for restart
xmits in just the slow-path (the first re-queued packet
case of dequeue_skb, where it checks for gso_skb) before
deciding whether to call the driver or not.
Patch was also tested between two servers with Emulex OneConnect
10G cards to confirm there is no regression. Though the patch is
an attempt to improve only small packet performance, there was
improvement for 1K, 2K and also 16K both in BW and SD. Results
from Guest -> Remote Host (BW in Mbps) for 1K and 16K I/O sizes:
________________________________________________________
I/O Size: 1K
# BW1 BW2 (%) SD1 SD2 (%)
________________________________________________________
1 1226 3313 (170.2) 6.6 1.9 (-71.2)
2 3223 7705 (139.0) 18.0 7.1 (-60.5)
4 7223 8716 (20.6) 36.5 29.7 (-18.6)
8 8689 8693 (0) 131.5 123.0 (-6.4)
16 8059 8285 (2.8) 578.3 506.2 (-12.4)
32 7758 7955 (2.5) 2281.4 2244.2 (-1.6)
64 7503 7895 (5.2) 9734.0 9424.4 (-3.1)
96 7496 7751 (3.4) 21980.9 20169.3 (-8.2)
128 7389 7741 (4.7) 40467.5 34995.5 (-13.5)
________________________________________________________
Summary: BW: 16.2% SD: -10.2%
________________________________________________________
I/O Size: 16K
# BW1 BW2 (%) SD1 SD2 (%)
________________________________________________________
1 6684 7019 (5.0) 1.1 1.1 (0)
2 7674 7196 (-6.2) 5.0 4.8 (-4.0)
4 7358 8032 (9.1) 21.3 20.4 (-4.2)
8 7393 8015 (8.4) 82.7 82.0 (-.8)
16 7958 8366 (5.1) 283.2 310.7 (9.7)
32 7792 8113 (4.1) 1257.5 1363.0 (8.3)
64 7673 8040 (4.7) 5723.1 5812.4 (1.5)
96 7462 7883 (5.6) 12731.8 12119.8 (-4.8)
128 7338 7800 (6.2) 21331.7 21094.7 (-1.1)
________________________________________________________
Summary: BW: 4.6% SD: -1.5%
Signed-off-by: Krishna Kumar <redacted>
---
So IIUC, we delay transmit by an arbitrary value and hope
that the host is done with the packets by then?
Interesting.
I am currently testing an approach where
we tell the host explicitly to interrupt us only after
a large part of the queue is empty.
With 256 entries in a queue, we should get 1 interrupt per
on the order of 100 packets which does not seem like a lot.
I can post it, mind testing this?
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2011-05-04 14:50:39
On Wed, May 04, 2011 at 07:33:19PM +0530, Krishna Kumar wrote:
quoted hunk
Introduce virtqueue_get_capacity() to help bail out of transmit
path early. Also remove notification when we run out of space (I
am not sure if this should be under a feature bit).
Signed-off-by: Krishna Kumar <redacted>
---
drivers/virtio/virtio_ring.c | 13 ++++++++-----
include/linux/virtio.h | 5 +++++
2 files changed, 13 insertions(+), 5 deletions(-)
diff -ruNp org/include/linux/virtio.h new/include/linux/virtio.h
@@ -156,6 +156,14 @@ static int vring_add_indirect(struct vrireturnhead;}+intvirtqueue_get_capacity(structvirtqueue*_vq)+{+structvring_virtqueue*vq=to_vvq(_vq);++returnvq->num_free;+}+EXPORT_SYMBOL_GPL(virtqueue_get_capacity);+intvirtqueue_add_buf_gfp(structvirtqueue*_vq,structscatterlistsg[],unsignedintout,
@@ -185,11 +193,6 @@ int virtqueue_add_buf_gfp(struct virtqueif(vq->num_free<out+in){pr_debug("Can't add buf len %i - avail = %i\n",out+in,vq->num_free);-/* FIXME: for historical reasons, we force a notify here if-*thereareoutgoingpartstothebuffer.Presumablythe-*hostshouldservicetheringASAP.*/-if(out)-vq->notify(&vq->vq);END_USE(vq);return-ENOSPC;}
This will break qemu versions 0.13 and back.
I'm adding some new virtio ring flags, we'll be
able to reuse one of these to mean 'no need for
work around', I think.
--
MST
"Michael S. Tsirkin" [off-list ref] wrote on 05/04/2011 08:16:22 PM:
quoted
A. virtio:
- Provide a API to get available number of slots.
B. virtio-net:
- Remove stop/start txq's and associated callback.
- Pre-calculate the number of slots needed to transmit
the skb in xmit_skb and bail out early if enough space
is not available. My testing shows that 2.5-3% of
packets are benefited by using this API.
- Do not drop skbs but instead return TX_BUSY like other
drivers.
- When returning EBUSY, set a per-txq variable to indicate
to dev_queue_xmit() whether to restart xmits on this txq.
C. net/sched/sch_generic.c:
Since virtio-net now returns EBUSY, the skb is requeued to
gso_skb. This allows adding the addional check for restart
xmits in just the slow-path (the first re-queued packet
case of dequeue_skb, where it checks for gso_skb) before
deciding whether to call the driver or not.
Patch was also tested between two servers with Emulex OneConnect
10G cards to confirm there is no regression. Though the patch is
an attempt to improve only small packet performance, there was
improvement for 1K, 2K and also 16K both in BW and SD. Results
from Guest -> Remote Host (BW in Mbps) for 1K and 16K I/O sizes:
________________________________________________________
I/O Size: 1K
# BW1 BW2 (%) SD1 SD2 (%)
________________________________________________________
1 1226 3313 (170.2) 6.6 1.9 (-71.2)
2 3223 7705 (139.0) 18.0 7.1 (-60.5)
4 7223 8716 (20.6) 36.5 29.7 (-18.6)
8 8689 8693 (0) 131.5 123.0 (-6.4)
16 8059 8285 (2.8) 578.3 506.2 (-12.4)
32 7758 7955 (2.5) 2281.4 2244.2 (-1.6)
64 7503 7895 (5.2) 9734.0 9424.4 (-3.1)
96 7496 7751 (3.4) 21980.9 20169.3 (-8.2)
128 7389 7741 (4.7) 40467.5 34995.5 (-13.5)
________________________________________________________
Summary: BW: 16.2% SD: -10.2%
________________________________________________________
I/O Size: 16K
# BW1 BW2 (%) SD1 SD2 (%)
________________________________________________________
1 6684 7019 (5.0) 1.1 1.1 (0)
2 7674 7196 (-6.2) 5.0 4.8 (-4.0)
4 7358 8032 (9.1) 21.3 20.4 (-4.2)
8 7393 8015 (8.4) 82.7 82.0 (-.8)
16 7958 8366 (5.1) 283.2 310.7 (9.7)
32 7792 8113 (4.1) 1257.5 1363.0 (8.3)
64 7673 8040 (4.7) 5723.1 5812.4 (1.5)
96 7462 7883 (5.6) 12731.8 12119.8 (-4.8)
128 7338 7800 (6.2) 21331.7 21094.7 (-1.1)
________________________________________________________
Summary: BW: 4.6% SD: -1.5%
Signed-off-by: Krishna Kumar <redacted>
---
So IIUC, we delay transmit by an arbitrary value and hope
that the host is done with the packets by then?
Not "hope" exactly. If the device is not ready, then
the packet is requeued. The main idea is to avoid
drops/stop/starts, etc.
Interesting.
I am currently testing an approach where
we tell the host explicitly to interrupt us only after
a large part of the queue is empty.
With 256 entries in a queue, we should get 1 interrupt per
on the order of 100 packets which does not seem like a lot.
I can post it, mind testing this?
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2011-05-04 19:58:18
On Wed, May 04, 2011 at 07:33:19PM +0530, Krishna Kumar wrote:
quoted hunk
@@ -185,11 +193,6 @@ int virtqueue_add_buf_gfp(struct virtque if (vq->num_free < out + in) { pr_debug("Can't add buf len %i - avail = %i\n", out + in, vq->num_free);- /* FIXME: for historical reasons, we force a notify here if- * there are outgoing parts to the buffer. Presumably the- * host should service the ring ASAP. */- if (out)- vq->notify(&vq->vq); END_USE(vq); return -ENOSPC; }
I thought about it some more. We should typically not get into this
state with the current driver as we check capacity upfront.
So why would this change help performance?
Shirley, any idea?
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2011-05-04 20:00:49
On Wed, May 04, 2011 at 05:50:19PM +0300, Michael S. Tsirkin wrote:
quoted
@@ -185,11 +193,6 @@ int virtqueue_add_buf_gfp(struct virtque if (vq->num_free < out + in) { pr_debug("Can't add buf len %i - avail = %i\n", out + in, vq->num_free);- /* FIXME: for historical reasons, we force a notify here if- * there are outgoing parts to the buffer. Presumably the- * host should service the ring ASAP. */- if (out)- vq->notify(&vq->vq); END_USE(vq); return -ENOSPC; }
This will break qemu versions 0.13 and back.
I'm adding some new virtio ring flags, we'll be
able to reuse one of these to mean 'no need for
work around', I think.
Not really, it wont. We shall almost never get here at all.
But then, why would this help performance?
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2011-05-04 21:23:59
On Wed, May 04, 2011 at 08:29:44PM +0530, Krishna Kumar2 wrote:
"Michael S. Tsirkin" [off-list ref] wrote on 05/04/2011 08:16:22 PM:
quoted
quoted
A. virtio:
- Provide a API to get available number of slots.
B. virtio-net:
- Remove stop/start txq's and associated callback.
- Pre-calculate the number of slots needed to transmit
the skb in xmit_skb and bail out early if enough space
is not available. My testing shows that 2.5-3% of
packets are benefited by using this API.
- Do not drop skbs but instead return TX_BUSY like other
drivers.
- When returning EBUSY, set a per-txq variable to indicate
to dev_queue_xmit() whether to restart xmits on this txq.
C. net/sched/sch_generic.c:
Since virtio-net now returns EBUSY, the skb is requeued to
gso_skb. This allows adding the addional check for restart
xmits in just the slow-path (the first re-queued packet
case of dequeue_skb, where it checks for gso_skb) before
deciding whether to call the driver or not.
Patch was also tested between two servers with Emulex OneConnect
10G cards to confirm there is no regression. Though the patch is
an attempt to improve only small packet performance, there was
improvement for 1K, 2K and also 16K both in BW and SD. Results
from Guest -> Remote Host (BW in Mbps) for 1K and 16K I/O sizes:
________________________________________________________
I/O Size: 1K
# BW1 BW2 (%) SD1 SD2 (%)
________________________________________________________
1 1226 3313 (170.2) 6.6 1.9 (-71.2)
2 3223 7705 (139.0) 18.0 7.1 (-60.5)
4 7223 8716 (20.6) 36.5 29.7 (-18.6)
8 8689 8693 (0) 131.5 123.0 (-6.4)
16 8059 8285 (2.8) 578.3 506.2 (-12.4)
32 7758 7955 (2.5) 2281.4 2244.2 (-1.6)
64 7503 7895 (5.2) 9734.0 9424.4 (-3.1)
96 7496 7751 (3.4) 21980.9 20169.3 (-8.2)
128 7389 7741 (4.7) 40467.5 34995.5 (-13.5)
________________________________________________________
Summary: BW: 16.2% SD: -10.2%
________________________________________________________
I/O Size: 16K
# BW1 BW2 (%) SD1 SD2 (%)
________________________________________________________
1 6684 7019 (5.0) 1.1 1.1 (0)
2 7674 7196 (-6.2) 5.0 4.8 (-4.0)
4 7358 8032 (9.1) 21.3 20.4 (-4.2)
8 7393 8015 (8.4) 82.7 82.0 (-.8)
16 7958 8366 (5.1) 283.2 310.7 (9.7)
32 7792 8113 (4.1) 1257.5 1363.0 (8.3)
64 7673 8040 (4.7) 5723.1 5812.4 (1.5)
96 7462 7883 (5.6) 12731.8 12119.8 (-4.8)
128 7338 7800 (6.2) 21331.7 21094.7 (-1.1)
________________________________________________________
Summary: BW: 4.6% SD: -1.5%
Signed-off-by: Krishna Kumar <redacted>
---
So IIUC, we delay transmit by an arbitrary value and hope
that the host is done with the packets by then?
Not "hope" exactly. If the device is not ready, then
the packet is requeued. The main idea is to avoid
drops/stop/starts, etc.
Yes, I see that, definitely. I guess it's a win if the
interrupt takes at least a jiffy to arrive anyway,
and a loss if not. Is there some reason interrupts
might be delayed until the next jiffy?
quoted
Interesting.
I am currently testing an approach where
we tell the host explicitly to interrupt us only after
a large part of the queue is empty.
With 256 entries in a queue, we should get 1 interrupt per
on the order of 100 packets which does not seem like a lot.
I can post it, mind testing this?
"Michael S. Tsirkin" [off-list ref] wrote on 05/05/2011 01:30:23 AM:
quoted
quoted
@@ -185,11 +193,6 @@ int virtqueue_add_buf_gfp(struct virtque if (vq->num_free < out + in) { pr_debug("Can't add buf len %i - avail = %i\n", out + in, vq->num_free);- /* FIXME: for historical reasons, we force a notify here if- * there are outgoing parts to the buffer. Presumably the- * host should service the ring ASAP. */- if (out)- vq->notify(&vq->vq); END_USE(vq); return -ENOSPC; }
This will break qemu versions 0.13 and back.
I'm adding some new virtio ring flags, we'll be
able to reuse one of these to mean 'no need for
work around', I think.
Not really, it wont. We shall almost never get here at all.
But then, why would this help performance?
Yes, it is not needed. I will be testing it without this
also.
thanks,
- KK
"Michael S. Tsirkin" [off-list ref] wrote on 05/05/2011 02:53:59 AM:
quoted
Not "hope" exactly. If the device is not ready, then
the packet is requeued. The main idea is to avoid
drops/stop/starts, etc.
Yes, I see that, definitely. I guess it's a win if the
interrupt takes at least a jiffy to arrive anyway,
and a loss if not. Is there some reason interrupts
might be delayed until the next jiffy?
I can explain this a bit as I have three debug counters
in start_xmit() just for this:
1. Whether the current xmit call was good, i.e. we had
returned BUSY last time and this xmit was successful.
2. Whether the current xmit call was bad, i.e. we had
returned BUSY last time and this xmit still failed.
3. The free capacity when we *resumed* xmits. This is
after calling free_old_xmit_skbs where this function
is not throttled, in effect it processes *all* the
completed skbs. This counter is a sum:
if (If_I_had_returned_EBUSY_last_iteration)
free_slots += virtqueue_get_capacity();
The counters after a 30 min run of 1K,2K,16K netperf
sessions are:
Good: 1059172
Bad: 31226
Sum of slots: 47551557
(Total of Good+Bad tallies with the total number of requeues
as shown by tc:
qdisc pfifo_fast 0: root refcnt 2 bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1
1 1 1 1
Sent 1560854473453 bytes 1075873684 pkt (dropped 718379, overlimits 0
requeues 1090398)
backlog 0b 0p requeues 1090398
)
It shows that 2.9% of the time, the 1 jiffy was not enough
to free up space in the txq. That could also mean that we
had set xmit_restart just before jiffies changed. But the
average free capacity when we *resumed* xmits is:
Sum of slots / (Good + Bad) = 43.
So the delay of 1 jiffy helped the host clean up, on average,
just 43 entries, which is 16% of total entries. This is
intended to show that the guest is not sitting idle waiting
for the jiffy to expire.
quoted
quoted
I can post it, mind testing this?
Sure.
Just posted. Would appreciate feedback.
Do I need to apply all the patches and simply test?
Thanks,
- KK
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2011-05-05 09:04:58
On Thu, May 05, 2011 at 01:33:14PM +0530, Krishna Kumar2 wrote:
"Michael S. Tsirkin" [off-list ref] wrote on 05/05/2011 02:53:59 AM:
quoted
quoted
Not "hope" exactly. If the device is not ready, then
the packet is requeued. The main idea is to avoid
drops/stop/starts, etc.
Yes, I see that, definitely. I guess it's a win if the
interrupt takes at least a jiffy to arrive anyway,
and a loss if not. Is there some reason interrupts
might be delayed until the next jiffy?
I can explain this a bit as I have three debug counters
in start_xmit() just for this:
1. Whether the current xmit call was good, i.e. we had
returned BUSY last time and this xmit was successful.
2. Whether the current xmit call was bad, i.e. we had
returned BUSY last time and this xmit still failed.
3. The free capacity when we *resumed* xmits. This is
after calling free_old_xmit_skbs where this function
is not throttled, in effect it processes *all* the
completed skbs. This counter is a sum:
if (If_I_had_returned_EBUSY_last_iteration)
free_slots += virtqueue_get_capacity();
The counters after a 30 min run of 1K,2K,16K netperf
sessions are:
Good: 1059172
Bad: 31226
Sum of slots: 47551557
(Total of Good+Bad tallies with the total number of requeues
as shown by tc:
qdisc pfifo_fast 0: root refcnt 2 bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1
1 1 1 1
Sent 1560854473453 bytes 1075873684 pkt (dropped 718379, overlimits 0
requeues 1090398)
backlog 0b 0p requeues 1090398
)
It shows that 2.9% of the time, the 1 jiffy was not enough
to free up space in the txq.
How common is it to free up space in *less than* 1 jiffy?
That could also mean that we
had set xmit_restart just before jiffies changed. But the
average free capacity when we *resumed* xmits is:
Sum of slots / (Good + Bad) = 43.
So the delay of 1 jiffy helped the host clean up, on average,
just 43 entries, which is 16% of total entries. This is
intended to show that the guest is not sitting idle waiting
for the jiffy to expire.
OK, nice, this is exactly what my patchset is trying
to do, without playing with timers: tell the host
to interrupt us after 3/4 of the ring is free.
Why 3/4 and not all of the ring? My hope is we can
get some parallelism with the host this way.
Why 3/4 and not 7/8? No idea :)
quoted
quoted
quoted
I can post it, mind testing this?
Sure.
Just posted. Would appreciate feedback.
Do I need to apply all the patches and simply test?
Thanks,
- KK
Exactly. You can also try to tune the threshold
for interrupts as well.
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2011-05-05 09:13:38
On Wed, May 04, 2011 at 11:00:23PM +0300, Michael S. Tsirkin wrote:
On Wed, May 04, 2011 at 05:50:19PM +0300, Michael S. Tsirkin wrote:
quoted
quoted
@@ -185,11 +193,6 @@ int virtqueue_add_buf_gfp(struct virtque if (vq->num_free < out + in) { pr_debug("Can't add buf len %i - avail = %i\n", out + in, vq->num_free);- /* FIXME: for historical reasons, we force a notify here if- * there are outgoing parts to the buffer. Presumably the- * host should service the ring ASAP. */- if (out)- vq->notify(&vq->vq); END_USE(vq); return -ENOSPC; }
This will break qemu versions 0.13 and back.
I'm adding some new virtio ring flags, we'll be
able to reuse one of these to mean 'no need for
work around', I think.
Not really, it wont. We shall almost never get here at all.
But then, why would this help performance?
I think I understand this finally.
By itself, this patch does not help performance and does not
hurt it. But later patch makes us try to xmit and fail there
instead of doing capacity checks. With *that* patch applied
on top of this one, and with qemu 0.13 and older, performance
will be hurt.
We need to either
- ignore these older hosts
- add a feature bit (or use one of the new ones I added: for example
with avail_event userspace never needs this behaviour as it can
ask to get events when ring gets full)
- keep doing capacity checks, which will make us almost never get here
"Michael S. Tsirkin" [off-list ref] wrote on 05/05/2011 02:34:39 PM:
quoted
It shows that 2.9% of the time, the 1 jiffy was not enough
to free up space in the txq.
How common is it to free up space in *less than* 1 jiffy?
True, but the point is that the space freed is just
enough for 43 entries, keeping it lower means a flood
of (psuedo) stop's and restart's.
quoted
That could also mean that we
had set xmit_restart just before jiffies changed. But the
average free capacity when we *resumed* xmits is:
Sum of slots / (Good + Bad) = 43.
So the delay of 1 jiffy helped the host clean up, on average,
just 43 entries, which is 16% of total entries. This is
intended to show that the guest is not sitting idle waiting
for the jiffy to expire.
OK, nice, this is exactly what my patchset is trying
to do, without playing with timers: tell the host
to interrupt us after 3/4 of the ring is free.
Why 3/4 and not all of the ring? My hope is we can
get some parallelism with the host this way.
Why 3/4 and not 7/8? No idea :)
quoted
quoted
quoted
quoted
I can post it, mind testing this?
Sure.
Just posted. Would appreciate feedback.
Do I need to apply all the patches and simply test?
Thanks,
- KK
Exactly. You can also try to tune the threshold
for interrupts as well.
Could you send me (privately) the entire virtio-net/vhost
patch in a single file? It will help me quite a bit :)
Either attachment or inline is fine.
thanks,
- KK
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2011-05-05 10:12:49
On Thu, May 05, 2011 at 03:13:43PM +0530, Krishna Kumar2 wrote:
"Michael S. Tsirkin" [off-list ref] wrote on 05/05/2011 02:34:39 PM:
quoted
quoted
It shows that 2.9% of the time, the 1 jiffy was not enough
to free up space in the txq.
How common is it to free up space in *less than* 1 jiffy?
True,
Sorry, which statement do you say is true? That interrupt
after less than 1 jiffy is common?
but the point is that the space freed is just
enough for 43 entries, keeping it lower means a flood
of (psuedo) stop's and restart's.
quoted
quoted
That could also mean that we
had set xmit_restart just before jiffies changed. But the
average free capacity when we *resumed* xmits is:
Sum of slots / (Good + Bad) = 43.
So the delay of 1 jiffy helped the host clean up, on average,
just 43 entries, which is 16% of total entries. This is
intended to show that the guest is not sitting idle waiting
for the jiffy to expire.
OK, nice, this is exactly what my patchset is trying
to do, without playing with timers: tell the host
to interrupt us after 3/4 of the ring is free.
Why 3/4 and not all of the ring? My hope is we can
get some parallelism with the host this way.
Why 3/4 and not 7/8? No idea :)
quoted
quoted
quoted
quoted
I can post it, mind testing this?
Sure.
Just posted. Would appreciate feedback.
Do I need to apply all the patches and simply test?
Thanks,
- KK
Exactly. You can also try to tune the threshold
for interrupts as well.
Could you send me (privately) the entire virtio-net/vhost
patch in a single file? It will help me quite a bit :)
Either attachment or inline is fine.
thanks,
- KK
Better yet, here they are in git:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-net-next-event-idx-v1
git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu-kvm.git virtio-net-event-idx-v1
--
MST
"Michael S. Tsirkin" [off-list ref] wrote on 05/05/2011 03:42:29 PM:
quoted
quoted
quoted
It shows that 2.9% of the time, the 1 jiffy was not enough
to free up space in the txq.
How common is it to free up space in *less than* 1 jiffy?
True,
Sorry, which statement do you say is true? That interrupt
after less than 1 jiffy is common?
I meant to say that, 97% of the time, space was enough for
the next xmit to succeed. This is keeping in mind that on
average 43 slots were freed up, indicating that the guest
was not waiting around for too long.
Regarding whether interrupts in less than 1 jiffy is
common, I think most of the time it should. But
increasing the limit as to when to do the cb would
increase to a jiffy.
To confirm, I just put some counters in the original
code and found that interrupts happen in less than a
jiffy around 96.75% of the time, only 3.25% took 1
jiffy. But as expected, this is with the host
interrupting immediately, which leads to many
stop/start/interrupts due to very little free capacity.
quoted
but the point is that the space freed is just
enough for 43 entries, keeping it lower means a flood
of (psuedo) stop's and restart's.
Better yet, here they are in git:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-
net-next-event-idx-v1
git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu-kvm.git
virtio-net-event-idx-v1
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2011-05-05 12:28:32
On Wed, May 04, 2011 at 07:33:32PM +0530, Krishna Kumar wrote:
quoted hunk
Changes:
1. Remove xmit notification
2. free_old_xmit_skbs() frees upto a limit to reduce tx jitter.
3. xmit_skb() precalculates the number of slots and checks if
that is available. It assumes that we are not using
indirect descriptors at this time.
4. start_xmit() becomes a small routine that removes most error
checks, does not drop packets but instead returns EBUSY if
there is no space to transmit. It also sets when to restart
xmits in future.
Signed-off-by: Krishna Kumar <redacted>
---
drivers/net/virtio_net.c | 70 ++++++++++---------------------------
1 file changed, 20 insertions(+), 50 deletions(-)
diff -ruNp org/drivers/net/virtio_net.c new/drivers/net/virtio_net.c
@@ -117,17 +117,6 @@ static struct page *get_a_page(struct vireturnp;}-staticvoidskb_xmit_done(structvirtqueue*svq)-{-structvirtnet_info*vi=svq->vdev->priv;--/* Suppress further interrupts. */-virtqueue_disable_cb(svq);--/* We were probably waiting for more output buffers. */-netif_wake_queue(vi->dev);-}-staticvoidset_skb_frag(structsk_buff*skb,structpage*page,unsignedintoffset,unsignedint*len){
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2011-05-05 15:34:34
On Thu, May 05, 2011 at 08:57:13PM +0530, Krishna Kumar2 wrote:
"Michael S. Tsirkin" [off-list ref] wrote on 05/05/2011 02:34:39 PM:
quoted
quoted
Do I need to apply all the patches and simply test?
Thanks,
- KK
Exactly. You can also try to tune the threshold
for interrupts as well.
I haven't tuned the threshhold, it is left it at 3/4. I ran
the new qemu/vhost/guest, and the results for 1K, 2K and 16K
are below. Note this is a different kernel version from my
earlier test results. So, f.e., BW1 represents 2.6.39-rc2,
the original kernel; while BW2 represents 2.6.37-rc5 (MST's
kernel).
Weird. My kernel is actually 2.6.39-rc2. So which is which?
This also isn't with the fixes you have sent just
now. I will get a run with that either late tonight or
tomorrow.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2011-05-05 15:42:45
On Thu, May 05, 2011 at 08:57:13PM +0530, Krishna Kumar2 wrote:
"Michael S. Tsirkin" [off-list ref] wrote on 05/05/2011 02:34:39 PM:
quoted
quoted
Do I need to apply all the patches and simply test?
Thanks,
- KK
Exactly. You can also try to tune the threshold
for interrupts as well.
I haven't tuned the threshhold, it is left it at 3/4. I ran
the new qemu/vhost/guest, and the results for 1K, 2K and 16K
are below. Note this is a different kernel version from my
earlier test results. So, f.e., BW1 represents 2.6.39-rc2,
the original kernel; while BW2 represents 2.6.37-rc5 (MST's
kernel). This also isn't with the fixes you have sent just
now. I will get a run with that either late tonight or
tomorrow.
One thing I'd suggest is merging v2.6.39-rc6 into that tree.
rc2 is still pretty early, reason I use it is because that is
what net-next is.
--
MST
"Michael S. Tsirkin" [off-list ref] wrote on 05/05/2011 09:04:13 PM:
quoted
I haven't tuned the threshhold, it is left it at 3/4. I ran
the new qemu/vhost/guest, and the results for 1K, 2K and 16K
are below. Note this is a different kernel version from my
earlier test results. So, f.e., BW1 represents 2.6.39-rc2,
the original kernel; while BW2 represents 2.6.37-rc5 (MST's
kernel).
Weird. My kernel is actually 2.6.39-rc2. So which is which?
I cloned git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git
# git branch -a
vhost
* vhost-net-next-event-idx-v1
remotes/origin/HEAD -> origin/vhost
remotes/origin/for-linus
remotes/origin/master
remotes/origin/net-2.6
remotes/origin/vhost
remotes/origin/vhost-broken
remotes/origin/vhost-devel
remotes/origin/vhost-mrg-rxbuf
remotes/origin/vhost-net
remotes/origin/vhost-net-next
remotes/origin/vhost-net-next-event-idx-v1
remotes/origin/vhost-net-next-rebased
remotes/origin/virtio-layout-aligned
remotes/origin/virtio-layout-minimal
remotes/origin/virtio-layout-original
remotes/origin/virtio-layout-padded
remotes/origin/virtio-publish-used
# git checkout vhost-net-next-event-idx-v1
Already on 'vhost-net-next-event-idx-v1'
# head -4 Makefile
VERSION = 2
PATCHLEVEL = 6
SUBLEVEL = 37
EXTRAVERSION = -rc5
I am not sure what I am missing.
thanks,
- KK