virtio_net backport performance

virtio_net backport performance

[Anthony Liguori]

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

Hey Rusty et al,

I've got automatic backports of the virtio modules[1] working back to 
2.6.18.  Everything seems okay except that for any kernel with the older 
NAPI api, performance is extremely bad.  I get about 1gbit on TX with 
2.6.24 but I get somewhere around 20mbit on 2.6.22.

I had to refactor the NAPI poll function heavily to get it working with 
the old api so my suspicion is that I'm doing something wrong.  I'd 
appreciate if ya'll could take a look at it.  I've included a patch that 
only has the differences of the backported version and the latest 
version.  Pay careful attention to the COMPAT_napi blocks as these are 
what would be used on older kernels.

It's a bit ugly because it's automatically generated by an awk script.

[1] http://hg.codemonkey.ws/virtio-ext-modules

Thanks,

Anthony Liguori

[-- Attachment #2: virtio_net_backport.diff --]
[-- Type: text/x-patch, Size: 5380 bytes --]

--- /home/anthony/git/kvm/drivers/net/virtio_net.c	2008-01-04 12:06:27.000000000 -0600
+++ virtio_net.c	2008-01-04 16:02:17.000000000 -0600
@@ -38,7 +38,9 @@
 	struct virtio_device *vdev;
 	struct virtqueue *rvq, *svq;
 	struct net_device *dev;
+#ifndef COMPAT_napi
 	struct napi_struct napi;
+#endif
 
 	struct tasklet_struct xmit_free;
 
@@ -74,8 +76,12 @@
 	while ((skb = vi->svq->vq_ops->get_buf(vi->svq, &len)) != NULL) {
 		pr_debug("Sent skb %p\n", skb);
 		__skb_unlink(skb, &vi->send);
+#ifndef COMPAT_net_stats
 		vi->dev->stats.tx_bytes += len;
+#endif
+#ifndef COMPAT_net_stats
 		vi->dev->stats.tx_packets++;
+#endif
 		kfree_skb(skb);
 		i++;
 	}
@@ -108,7 +114,9 @@
 
 	if (unlikely(len < sizeof(struct virtio_net_hdr) + ETH_HLEN)) {
 		pr_debug("%s: short packet %i\n", dev->name, len);
+#ifndef COMPAT_net_stats
 		dev->stats.rx_length_errors++;
+#endif
 		goto drop;
 	}
 	len -= sizeof(struct virtio_net_hdr);
@@ -118,9 +126,14 @@
 	skb->protocol = eth_type_trans(skb, dev);
 	pr_debug("Receiving skb proto 0x%04x len %i type %i\n",
 		 ntohs(skb->protocol), skb->len, skb->pkt_type);
+#ifndef COMPAT_net_stats
 	dev->stats.rx_bytes += skb->len;
+#endif
+#ifndef COMPAT_net_stats
 	dev->stats.rx_packets++;
+#endif
 
+#ifndef COMPAT_csum_offset
 	if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
 		pr_debug("Needs csum!\n");
 		skb->ip_summed = CHECKSUM_PARTIAL;
@@ -135,6 +148,7 @@
 			goto frame_err;
 		}
 	}
+#endif
 
 	if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
 		pr_debug("GSO!\n");
@@ -175,7 +189,9 @@
 	return;
 
 frame_err:
+#ifndef COMPAT_net_stats
 	dev->stats.rx_frame_errors++;
+#endif
 drop:
 	dev_kfree_skb(skb);
 }
@@ -215,17 +231,40 @@
 	struct virtnet_info *vi = rvq->vdev->priv;
 	/* Suppress further interrupts. */
 	rvq->vq_ops->disable_cb(rvq);
+#ifdef COMPAT_napi
+	vi->rvq->vq_ops->enable_cb(vi->rvq);
+	if (netif_rx_schedule_prep(vi->dev)) {
+		vi->rvq->vq_ops->disable_cb(vi->rvq);
+		__netif_rx_schedule(vi->dev);
+	} else
+		vi->rvq->vq_ops->enable_cb(vi->rvq);
+#else
 	netif_rx_schedule(vi->dev, &vi->napi);
+#endif
 }
 
+#ifdef COMPAT_napi
+static int virtnet_poll(struct net_device *dev, int *budget)
+#else
 static int virtnet_poll(struct napi_struct *napi, int budget)
+#endif
 {
+#ifdef COMPAT_napi
+	struct virtnet_info *vi = netdev_priv(dev);
+	int max_received = min(dev->quota, *budget);
+	bool no_work;
+#else
 	struct virtnet_info *vi = container_of(napi, struct virtnet_info, napi);
+#endif
 	struct sk_buff *skb = NULL;
 	unsigned int len, received = 0;
 
 again:
+#ifdef COMPAT_napi
+	while (received < max_received &&
+#else
 	while (received < budget &&
+#endif
 	       (skb = vi->rvq->vq_ops->get_buf(vi->rvq, &len)) != NULL) {
 		__skb_unlink(skb, &vi->recv);
 		receive_skb(vi->dev, skb, len);
@@ -239,6 +278,27 @@
 		try_fill_recv(vi);
 
 	/* Out of packets? */
+#ifdef COMPAT_napi
+	if (skb) {
+		*budget -= received;
+		dev->quota -= received;
+		return 1;
+	}
+
+	no_work = vi->rvq->vq_ops->enable_cb(vi->rvq);
+	netif_rx_complete(vi->dev);
+
+	if (!no_work && netif_rx_reschedule(vi->dev, received)) {
+		vi->rvq->vq_ops->disable_cb(vi->rvq);
+		skb = NULL;
+		goto again;
+	}
+
+	dev->quota -= received;
+	*budget -= received;
+
+	return 0;
+#else
 	if (received < budget) {
 		netif_rx_complete(vi->dev, napi);
 		if (unlikely(!vi->rvq->vq_ops->enable_cb(vi->rvq))
@@ -247,9 +307,14 @@
 	}
 
 	return received;
+#endif
 }
 
+#ifdef COMPAT_hrtimer_func
+static int kick_xmit(struct hrtimer *t)
+#else
 static enum hrtimer_restart kick_xmit(struct hrtimer *t)
+#endif
 {
 	struct virtnet_info *vi = container_of(t,struct virtnet_info,tx_timer);
 
@@ -278,6 +343,7 @@
 
 	/* Encode metadata header at front. */
 	hdr = skb_vnet_hdr(skb);
+#ifndef COMPAT_csum_offset
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
 		hdr->csum_start = skb->csum_start - skb_headroom(skb);
@@ -286,6 +352,7 @@
 		hdr->flags = 0;
 		hdr->csum_offset = hdr->csum_start = 0;
 	}
+#endif
 
 	if (skb_is_gso(skb)) {
 		hdr->gso_size = skb_shinfo(skb)->gso_size;
@@ -356,13 +423,26 @@
 	if (vi->num == 0)
 		return -ENOMEM;
 
+#ifdef COMPAT_napi
+	netif_poll_enable(dev);
+#else
 	napi_enable(&vi->napi);
+#endif
 
 	/* If all buffers were filled by other side before we napi_enabled, we
 	 * won't get another interrupt, so process any outstanding packets
 	 * now.  virtnet_poll wants re-enable the queue, so we disable here. */
 	vi->rvq->vq_ops->disable_cb(vi->rvq);
+#ifdef COMPAT_napi
+	vi->rvq->vq_ops->enable_cb(vi->rvq);
+	if (netif_rx_schedule_prep(vi->dev)) {
+		vi->rvq->vq_ops->disable_cb(vi->rvq);
+		__netif_rx_schedule(vi->dev);
+	} else
+		vi->rvq->vq_ops->enable_cb(vi->rvq);
+#else
 	netif_rx_schedule(vi->dev, &vi->napi);
+#endif
 
 	return 0;
 }
@@ -372,7 +452,11 @@
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct sk_buff *skb;
 
+#ifdef COMPAT_napi
+	netif_poll_disable(dev);
+#else
 	napi_disable(&vi->napi);
+#endif
 
 	/* networking core has neutered skb_xmit_done/skb_recv_done, so don't
 	 * worry about races vs. get(). */
@@ -431,7 +515,12 @@
 
 	/* Set up our device-specific information */
 	vi = netdev_priv(dev);
+#ifdef COMPAT_napi
+	dev->poll = virtnet_poll;
+	dev->weight = 64;
+#else
 	netif_napi_add(dev, &vi->napi, virtnet_poll, napi_weight);
+#endif
 	vi->dev = dev;
 	vi->vdev = vdev;
 	memset(&vi->tx_timer, 0, sizeof(vi->tx_timer));

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

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

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: virtio_net backport performance

[Rusty Russell]

On Saturday 05 January 2008 09:24:40 Anthony Liguori wrote:
> @@ -215,17 +231,40 @@
>         struct virtnet_info *vi = rvq->vdev->priv;
>         /* Suppress further interrupts. */
>         rvq->vq_ops->disable_cb(rvq);
> +#ifdef COMPAT_napi
> +       vi->rvq->vq_ops->enable_cb(vi->rvq);
> +       if (netif_rx_schedule_prep(vi->dev)) {
> +               vi->rvq->vq_ops->disable_cb(vi->rvq);
> +               __netif_rx_schedule(vi->dev);
> +       } else
> +               vi->rvq->vq_ops->enable_cb(vi->rvq);
> +#else
>         netif_rx_schedule(vi->dev, &vi->napi);
> +#endif
>  }

This looks wrong.  It should be the same, except netif_rx_schedule doesn't 
take the napi arg (the napi struct used to be in the device, but multiqueue 
means everyone need to supply their own napi arg now).

Rusty.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: virtio_net backport performance

[Rusty Russell]

On Saturday 05 January 2008 09:24:40 Anthony Liguori wrote:
> Hey Rusty et al,
>
> I've got automatic backports of the virtio modules[1] working back to
> 2.6.18.  Everything seems okay except that for any kernel with the older
> NAPI api, performance is extremely bad.  I get about 1gbit on TX with
> 2.6.24 but I get somewhere around 20mbit on 2.6.22.

OK, I tested this backport and immediately got oopses.  Revealed some 
interesting races in net driver (again, it's that damn callback disable 
causing problems).  New queue fixes these, but get awful performance:
'dd bs=1M count=1000 if=/dev/zero | nc 172.20.0.1 8889' takes almost 30 
seconds.

Found one bug in your code tho: if enable_cb returns false, it means the queue 
has *not* been enabled:

--- hack-module.awk.~1~	2008-01-06 10:49:16.000000000 +1100
+++ hack-module.awk	2008-01-07 19:08:40.000000000 +1100
@@ -49,7 +49,6 @@
     print "	netif_rx_complete(vi->dev);";
     print "";
     print "	if (!no_work && netif_rx_reschedule(vi->dev, received)) {";
-    print "		vi->rvq->vq_ops->disable_cb(vi->rvq);";
     print "		skb = NULL;";
     print "		goto again;";
     print "	}";

Will continue looking for performance regression tomorrow (actually, better 
check my changes haven't introduced it in non-backports first!).

Thanks!
Rusty.

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

Re: virtio_net backport performance

[Anthony Liguori]

Rusty Russell wrote:
> On Saturday 05 January 2008 09:24:40 Anthony Liguori wrote:
>   
>> Hey Rusty et al,
>>
>> I've got automatic backports of the virtio modules[1] working back to
>> 2.6.18.  Everything seems okay except that for any kernel with the older
>> NAPI api, performance is extremely bad.  I get about 1gbit on TX with
>> 2.6.24 but I get somewhere around 20mbit on 2.6.22.
>>     
>
> OK, I tested this backport and immediately got oopses.  Revealed some 
> interesting races in net driver (again, it's that damn callback disable 
> causing problems).  New queue fixes these, but get awful performance:
> 'dd bs=1M count=1000 if=/dev/zero | nc 172.20.0.1 8889' takes almost 30 
> seconds.
>   

rx performance is pretty abysmal but tx performance seems respectable.  
I'm getting strange results with netperf but my guess is that we're 
getting around 800mbit tx but only 25mbit rx.

tx is right around what it should be but rx is about an order of 
magnitude off.

> Found one bug in your code tho: if enable_cb returns false, it means the queue 
> has *not* been enabled:
>
> --- hack-module.awk.~1~	2008-01-06 10:49:16.000000000 +1100
> +++ hack-module.awk	2008-01-07 19:08:40.000000000 +1100
> @@ -49,7 +49,6 @@
>      print "	netif_rx_complete(vi->dev);";
>      print "";
>      print "	if (!no_work && netif_rx_reschedule(vi->dev, received)) {";
> -    print "		vi->rvq->vq_ops->disable_cb(vi->rvq);";
>      print "		skb = NULL;";
>      print "		goto again;";
>      print "	}";
>   

Thanks, I've applied this to the backport tree.

> Will continue looking for performance regression tomorrow (actually, better 
> check my changes haven't introduced it in non-backports first!).
>   

Excellent, thanks!

Regards,

Anthony Liguori

> Thanks!
> Rusty.
>   


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