Re: [PATCH] virtio_net tx performance fix

Re: [PATCH] virtio_net tx performance fix

[Anthony Liguori]

Hi Dor,

How are you measuring performance?  The numbers I've gotten with netperf 
before and after your patch are:

tx - 647.27mbit
rx - 89.22

tx - 27.82
rx - 79.93

So this patch is pretty much killing performance for netperf.

Dor Laor wrote:
> There was a problem with the location of the notify call in
> add_buff function:
> When VRING_USED_F_NO_NOTIFY is set, the host does not kick the
> guest when packets were transmitted, as a result the guest runs
> out of tx buffers sometimes.

But even if F_NO_NOTIFY is set, if the tx buffer is full, we notify the 
guest, so this prevents that from happening.

>  This is fine but the problem lies
> when add_buf fails, it called notify and the host sends all the
> pending tx pkts. When enable_cb was called, more_used(vq) returned
> false so eventually the skb was dropped.
>   

I'm having a tough time following this part.  If add_buf fails, we 
notify unconditionally (which is, I think what we want).  I'm not sure 
how that relates to a packet getting dropped though.

Regards,

Anthony Liguori

Re: [PATCH] virtio_net tx performance fix

[Dor Laor]

On Mon, 2008-01-28 at 09:32 -0600, Anthony Liguori wrote:
> Hi Dor,
> 
> How are you measuring performance?  The numbers I've gotten with netperf 
> before and after your patch are:
> 
> tx - 647.27mbit
> rx - 89.22
> 
> tx - 27.82
> rx - 79.93
> 

I've been testing with iperf (patched with Ingo's fix).
I also tested tcp/udp (udp tx is only 550Mbps with the patch, w/o it's
only 220Mbps)

> So this patch is pretty much killing performance for netperf.
> 

Did you have hrtimer configured on the host?

> Dor Laor wrote:
> > There was a problem with the location of the notify call in
> > add_buff function:
> > When VRING_USED_F_NO_NOTIFY is set, the host does not kick the
> > guest when packets were transmitted, as a result the guest runs
> > out of tx buffers sometimes.
> 
> But even if F_NO_NOTIFY is set, if the tx buffer is full, we notify the 
> guest, so this prevents that from happening.
> 
> >  This is fine but the problem lies
> > when add_buf fails, it called notify and the host sends all the
> > pending tx pkts. When enable_cb was called, more_used(vq) returned
> > false so eventually the skb was dropped.
> >   
> 
> I'm having a tough time following this part.  If add_buf fails, we 
> notify unconditionally (which is, I think what we want).  I'm not sure 
> how that relates to a packet getting dropped though.
> 

Here is start_xmit function:

again:
	/* Free up any pending old buffers before queueing new ones. */
	free_old_xmit_skbs(vi);
	err = vi->svq->vq_ops->add_buf(vi->svq, sg, num, 0, skb);

<<< here add_buf might fail since the ring is full.
<<< once it fails, the original code called notify which triggered the
<<< host to send all the pending tx so all descriptors are used.

	if (err) {
		vi->stats.sendq_full++;

		pr_debug("%s: virtio not prepared to send\n",dev->name);
		netif_stop_queue(dev);

		/* Activate callback for using skbs: if this fails it
		 * means some were used in the meantime. */
		vi->stats.sendq_enabled++;
		if (unlikely(!vi->svq->vq_ops->enable_cb(vi->svq))) {

<<< Since before the patch enable_cb returned true, thus the goto below
<<< was not called.
<<< The patch moves the notify to enable_cb above.

			printk("Unlikely: restart svq failed\n");
			vi->stats.sendq_enable_failed++;
			netif_start_queue(dev);
			goto again;
		}
		__skb_unlink(skb, &vi->send);

		return NETDEV_TX_BUSY;
	}


> Regards,
> 
> Anthony Liguori
> 
> 

Re: [PATCH] virtio_net tx performance fix

[Anthony Liguori]

Dor Laor wrote:
> On Mon, 2008-01-28 at 09:32 -0600, Anthony Liguori wrote:
>   
>> Hi Dor,
>>
>> How are you measuring performance?  The numbers I've gotten with netperf 
>> before and after your patch are:
>>
>> tx - 647.27mbit
>> rx - 89.22
>>
>> tx - 27.82
>> rx - 79.93
>>
>>     
>
> I've been testing with iperf (patched with Ingo's fix).
> I also tested tcp/udp (udp tx is only 550Mbps with the patch, w/o it's
> only 220Mbps)
>   

I can try iperf, but I think we should also try a pretty simple dd test 
to see if there's a real impact.

>> So this patch is pretty much killing performance for netperf.
>>
>>     
>
> Did you have hrtimer configured on the host?
>   

Yup.

> Here is start_xmit function:
>
> again:
> 	/* Free up any pending old buffers before queueing new ones. */
> 	free_old_xmit_skbs(vi);
> 	err = vi->svq->vq_ops->add_buf(vi->svq, sg, num, 0, skb);
>
> <<< here add_buf might fail since the ring is full.
> <<< once it fails, the original code called notify which triggered the
> <<< host to send all the pending tx so all descriptors are used.
>   

Notify, with KVM at least, will drain the TX ring.  So now, the TX ring 
should be empty.  The notification is disabled but notification should 
always happen when the queue is drained so an interrupt should occur 
which should cause the guest to immediately try again..

> 	if (err) {
> 		vi->stats.sendq_full++;
>
> 		pr_debug("%s: virtio not prepared to send\n",dev->name);
> 		netif_stop_queue(dev);
>
> 		/* Activate callback for using skbs: if this fails it
> 		 * means some were used in the meantime. */
> 		vi->stats.sendq_enabled++;
> 		if (unlikely(!vi->svq->vq_ops->enable_cb(vi->svq))) {
>   

Even when this returns false and we don't immediately reschedule.  At 
least, that's what I'm expecting to happen.

Regards,

Anthony Liguori

> <<< Since before the patch enable_cb returned true, thus the goto below
> <<< was not called.
> <<< The patch moves the notify to enable_cb above.
>
> 			printk("Unlikely: restart svq failed\n");
> 			vi->stats.sendq_enable_failed++;
> 			netif_start_queue(dev);
> 			goto again;
> 		}
> 		__skb_unlink(skb, &vi->send);
>
> 		return NETDEV_TX_BUSY;
> 	}
>
>
>   
>> Regards,
>>
>> Anthony Liguori
>>
>>
>>     
>
>   

Re: [PATCH] virtio_net tx performance fix

[Anthony Liguori]

Dor Laor wrote:
> On Mon, 2008-01-28 at 09:32 -0600, Anthony Liguori wrote:
>   
>> Hi Dor,
>>
>> How are you measuring performance?  The numbers I've gotten with netperf 
>> before and after your patch are:
>>
>> tx - 647.27mbit
>> rx - 89.22
>>
>> tx - 27.82
>> rx - 79.93
>>
>>     
>
> I've been testing with iperf (patched with Ingo's fix).
> I also tested tcp/udp (udp tx is only 550Mbps with the patch, w/o it's
> only 220Mbps)
>   

FWIW, I repeated with an scp test copying a 200mb file from /dev/shm on 
the host to the guest and vice versa.  rx performance is around 3-4 
MB/sec.  Without your patch, tx performance is around 40 MB/sec whereas 
with your patch performance is around 30 MB/sec.  So the tx drop is 
definitely exaggerated by netperf.  I'm definitely not seeing an 
increase in throughput though.

Could you try a similar scp test?  What sort of networking setup are you 
using?

My guest kernel and host kernel are both 2.6.24 (from Ubuntu Hardy).

Regards,

Anthony Liguori

[PATCH] virtio_net tx performance fix

[Dor Laor]

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

From f582caf612b446e42f1e80d5ef12c5b7322efd03 Mon Sep 17 00:00:00 2001
From: Dor Laor <dor.laor@qumranet.com>
Date: Mon, 28 Jan 2008 02:09:48 +0200
Subject: [PATCH] virtio_net tx performance fix

There was a problem with the location of the notify call in
add_buff function:
When VRING_USED_F_NO_NOTIFY is set, the host does not kick the
guest when packets were transmitted, as a result the guest runs
out of tx buffers sometimes. This is fine but the problem lies
when add_buf fails, it called notify and the host sends all the
pending tx pkts. When enable_cb was called, more_used(vq) returned
false so eventually the skb was dropped.

Moving notify from add_buf to enable_cb fixes this flow problem.
The tx performance boosted from 220Mbps to 850Mbps.

Signed-off-by: Dor Laor <dor.laor@qumranet.com>
---
 drivers/virtio/virtio_ring.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 3a28c13..592bbc9 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -87,8 +87,6 @@ static int vring_add_buf(struct virtqueue *_vq,
 	if (vq->num_free < out + in) {
 		pr_debug("Can't add buf len %i - avail = %i\n",
 			 out + in, vq->num_free);
-		/* We notify *even if* VRING_USED_F_NO_NOTIFY is set here. */
-		vq->notify(&vq->vq);
 		END_USE(vq);
 		return -ENOSPC;
 	}
@@ -232,6 +230,7 @@ static bool vring_enable_cb(struct virtqueue *_vq)
 	vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
 	mb();
 	if (unlikely(more_used(vq))) {
+		vq->notify(&vq->vq);
 		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
 		END_USE(vq);
 		return false;
-- 
1.5.3.7



[-- Attachment #2: 0001-virtio_net-tx-performance-fix.patch --]
[-- Type: application/mbox, Size: 1675 bytes --]

[-- Attachment #3: Type: text/plain, Size: 184 bytes --]

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