virtio_net tx performance fix

PATCH In review Dor Laor 4 participants 8 messages Jan 28, 2008 – Jan 29, 2008 3 messages not in the archive

Subsystems (2)

KVM virtio core

lore

Share & tools

lore

Dor Laor

author

2008-01-28 00:13 UTC #0 lore raw

PATCH 1/1 virtio_net tx performance fix

Quoted 1 line

From f582caf612b446e42f1e80d5ef12c5b7322efd03 Mon Sep 17 00:00:00 2001

From: Dor Laor <dor.laor-atKUWr5tajBWk0Htik3J/[email protected]>
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-atKUWr5tajBWk0Htik3J/[email protected]>

---
 drivers/virtio/virtio_ring.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

drivers/virtio/virtio_ring.c +1 -2

index 3a28c13..592bbc9 100644
@@ -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;

Signature · 1 line

1.5.3.7

Quoted 1 line

From f582caf612b446e42f1e80d5ef12c5b7322efd03 Mon Sep 17 00:00:00 2001

From: Dor Laor <dor.laor-atKUWr5tajBWk0Htik3J/[email protected]>
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 transmited, 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 droped.

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-atKUWr5tajBWk0Htik3J/[email protected]>

---
 drivers/virtio/virtio_ring.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

drivers/virtio/virtio_ring.c +1 -2

index 3a28c13..592bbc9 100644
@@ -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;

Signature · 1 line

1.5.3.7

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

A message that is not in the archive (sent off-list or not yet received): 1201479224.3047.37.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]

Anthony Liguori

2008-01-28 15:32 UTC #2 lore raw

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.

Quoted 5 lines · Dor Laor

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.

Quoted 5 lines

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

A message that is not in the archive (sent off-list or not yet received): 479DF5A8.8050103-r/[email protected]

Dor Laor

author

2008-01-28 15:59 UTC #4 lore raw

Quoted 11 lines · 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

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)

Quoted 2 lines

So this patch is pretty much killing performance for netperf.

Did you have hrtimer configured on the host?

Quoted 16 lines

Quoted 5 lines · Dor Laor
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.
Quoted 5 lines
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.

Quoted 4 lines

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;
	}

Quoted 5 lines

Regards,
Anthony Liguori

A message that is not in the archive (sent off-list or not yet received): 1201535954.2457.11.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]

Anthony Liguori

2008-01-28 16:11 UTC #6 lore raw

Quoted 10 lines · Dor Laor

Quoted 8 lines · 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

Quoted 9 lines

Quoted 4 lines
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.

Quoted 6 lines

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

Yup.

Quoted 11 lines

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..

Quoted 11 lines

	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

Quoted 4 lines

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

Quoted 20 lines

			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;
	}

Quoted 6 lines

Regards,
Anthony Liguori

Anthony Liguori

2008-01-28 16:11 UTC #7 lore raw

In reply to Dor Laor

Quoted 9 lines · Dor Laor

Quoted 7 lines · 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

Quoted 9 lines

Quoted 4 lines
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.

Quoted 6 lines

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

Yup.

Quoted 11 lines

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.

Quoted 11 lines

	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

Quoted 24 lines

<<< 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;
	}
Quoted 6 lines
Regards,
Anthony Liguori

Anthony Liguori

2008-01-29 04:09 UTC #8 lore raw

In reply to Dor Laor

Quoted 9 lines · Dor Laor

Quoted 7 lines · 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

Quoted 9 lines

Quoted 4 lines
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

Dor Laor

2008-01-28 15:59 UTC #9 lore raw

In reply to Anthony Liguori

Quoted 11 lines · 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

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)

Quoted 2 lines

So this patch is pretty much killing performance for netperf.

Did you have hrtimer configured on the host?

Quoted 16 lines

Quoted 5 lines · Dor Laor
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.
Quoted 5 lines
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.

Quoted 4 lines

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;
	}

Quoted 5 lines

Regards,
Anthony Liguori

Anthony Liguori

2008-01-28 15:32 UTC #10 lore raw

In reply to Dor Laor

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.

Quoted 5 lines · Dor Laor

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.

Quoted 5 lines

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