virtio: wean net driver off NETDEV_TX_BUSY
Subsystems (4)
PATCH 1/4 virtio: wean net driver off NETDEV_TX_BUSY
Herbert tells me that returning NETDEV_TX_BUSY from hard_start_xmit is seen as a poor thing to do; we should cache the packet and stop the queue.
--- drivers/net/virtio_net.c | 58 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 18 deletions(-)
drivers/net/virtio_net.c +40 -18
@@ -40,6 +40,9 @@ struct virtnet_infostruct virtqueue *rvq, *svq;struct net_device *dev;struct napi_struct napi; + +/* The skb we couldn't send because buffers were full. */ +struct sk_buff *last_xmit_skb;/* Number of input buffers, and max we've ever had. */ unsigned int num, max;
@@ -227,17 +230,16 @@ static void free_old_xmit_skbs(struct vi } } -static int start_xmit (struct sk_buff *skb,struct net_device *dev) +static int xmit_skb (struct virtnet_info *vi,struct sk_buff *skb) { -struct virtnet_info *vi =netdev_priv (dev); -int num, err; +int num;struct scatterlist sg[2 +MAX_SKB_FRAGS ];struct virtio_net_hdr *hdr;const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;sg_init_table (sg,2 +MAX_SKB_FRAGS ); -pr_debug ("%s: xmit %p " MAC_FMT "\n" , dev->name, skb, +pr_debug ("%s: xmit %p " MAC_FMT "\n" , vi->dev->name, skb, dest[0 ], dest[1 ], dest[2 ], dest[3 ], dest[4 ], dest[5 ]);
@@ -272,30 +274,50 @@ static int start_xmit(struct sk_buff *skvnet_hdr_to_sg (sg, skb); num =skb_to_sgvec (skb, sg+1 ,0 , skb->len) +1 ; -__skb_queue_head (&vi->send, skb); + +return vi->svq->vq_ops->add_buf (vi->svq, sg, num,0 , skb); +} + +static int start_xmit (struct sk_buff *skb,struct net_device *dev) +{ +struct virtnet_info *vi =netdev_priv (dev); 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); -if (err) { -pr_debug ("%s: virtio not prepared to send\n" , dev->name); -netif_stop_queue (dev); -/* Activate callback for using skbs: if this returns false it -* means some were used in the meantime. */ -if (unlikely (!vi->svq->vq_ops->enable_cb (vi->svq))) { - vi->svq->vq_ops->disable_cb (vi->svq); -netif_start_queue (dev); - goto again; +/* If we has a buffer left over from last time, send it now. */ +if (vi->last_xmit_skb) { +if (xmit_skb (vi, vi->last_xmit_skb) !=0 ) { +/* Drop this skb: we only queue one. */ +kfree_skb (skb); + goto stop_queue; } -__skb_unlink (skb, &vi->send); + vi->last_xmit_skb =NULL ; + } -return NETDEV_TX_BUSY ; +/* Put new one in send queue and do transmit */ +__skb_queue_head (&vi->send, skb); +if (xmit_skb (vi, skb) !=0 ) { + vi->last_xmit_skb = skb; + goto stop_queue; } +done: vi->svq->vq_ops->kick (vi->svq); +return NETDEV_TX_OK ; -return 0 ; +stop_queue: +pr_debug ("%s: virtio not prepared to send\n" , dev->name); +netif_stop_queue (dev); + +/* Activate callback for using skbs: if this returns false it +* means some were used in the meantime. */ +if (unlikely (!vi->svq->vq_ops->enable_cb (vi->svq))) { + vi->svq->vq_ops->disable_cb (vi->svq); +netif_start_queue (dev); + goto again; + } + goto done; }#ifdef CONFIG_NET_POLL_CONTROLLER
PATCH 2/4 virtio: finer-grained features for virtio_net
So, we previously had a 'VIRTIO_NET_F_GSO' bit which meant that 'the host can handle csum offload, and any TSO (v4&v6 incl ECN) or UFO packets you might want to send. I thought this was good enough for Linux, but it actually isn't, since we don't do UFO in software.
So, add separate feature bits for what the host can handle. Add equivalent ones for the guest to say what it can handle, because LRO is coming too (thanks Herbert!).
--- drivers/net/virtio_net.c | 9 +++++++++ include/linux/virtio_net.h | 13 +++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-)
drivers/net/virtio_net.c +9 -0
@@ -381,6 +381,15 @@ static int virtnet_probe(struct virtio_d dev->features |=NETIF_F_TSO |NETIF_F_UFO |NETIF_F_TSO_ECN |NETIF_F_TSO6 ; } +/* Individual feature bits: what can host handle? */ +if (gso && vdev->config->feature (vdev,VIRTIO_NET_F_HOST_TSO4 )) + dev->features |=NETIF_F_TSO ; +if (gso && vdev->config->feature (vdev,VIRTIO_NET_F_HOST_TSO6 )) + dev->features |=NETIF_F_TSO6 ; +if (gso && vdev->config->feature (vdev,VIRTIO_NET_F_HOST_ECN )) + dev->features |=NETIF_F_TSO_ECN ; +if (gso && vdev->config->feature (vdev,VIRTIO_NET_F_HOST_UFO )) + dev->features |=NETIF_F_UFO ; }/* Configuration may specify what MAC to use. Otherwise random. */
include/linux/virtio_net.h +11 -2
@@ -6,9 +6,18 @@#define VIRTIO_ID_NET 1/* The feature bitmap for virtio net */ -#define VIRTIO_NET_F_CSUM 0/* Can handle pkts w/ partial csum */ +#define VIRTIO_NET_F_CSUM 0/* Host handles pkts w/ partial csum */ +#define VIRTIO_NET_F_GUEST_CSUM 1/* Guest handles pkts w/ partial csum */ #define VIRTIO_NET_F_MAC 5/* Host has given MAC address. */ -#define VIRTIO_NET_F_GSO 6/* Can handle pkts w/ any GSO type */ +#define VIRTIO_NET_F_GSO 6/* Host handles pkts w/ any GSO type */ +#define VIRTIO_NET_F_GUEST_TSO4 7/* Guest can handle TSOv4 in. */ +#define VIRTIO_NET_F_GUEST_TSO6 8/* Guest can handle TSOv6 in. */ +#define VIRTIO_NET_F_GUEST_ECN 9/* Guest can handle TSO[6] w/ ECN in. */ +#define VIRTIO_NET_F_GUEST_UFO 10/* Guest can handle UFO in. */ +#define VIRTIO_NET_F_HOST_TSO4 11/* Host can handle TSOv4 in. */ +#define VIRTIO_NET_F_HOST_TSO6 12/* Host can handle TSOv6 in. */ +#define VIRTIO_NET_F_HOST_ECN 13/* Host can handle TSO[6] w/ ECN in. */ +#define VIRTIO_NET_F_HOST_UFO 14/* Host can handle UFO in. */ struct virtio_net_config {
PATCH 2/4 virtio: finer-grained features for virtio_net
In reply to Rusty RussellSo, we previously had a 'VIRTIO_NET_F_GSO' bit which meant that 'the host can handle csum offload, and any TSO (v4&v6 incl ECN) or UFO packets you might want to send. I thought this was good enough for Linux, but it actually isn't, since we don't do UFO in software.
So, add separate feature bits for what the host can handle. Add equivalent ones for the guest to say what it can handle, because LRO is coming too (thanks Herbert!).
--- drivers/net/virtio_net.c | 9 +++++++++ include/linux/virtio_net.h | 13 +++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-)
drivers/net/virtio_net.c +9 -0
@@ -381,6 +381,15 @@ static int virtnet_probe(struct virtio_d dev->features |=NETIF_F_TSO |NETIF_F_UFO |NETIF_F_TSO_ECN |NETIF_F_TSO6 ; } +/* Individual feature bits: what can host handle? */ +if (gso && vdev->config->feature (vdev,VIRTIO_NET_F_HOST_TSO4 )) + dev->features |=NETIF_F_TSO ; +if (gso && vdev->config->feature (vdev,VIRTIO_NET_F_HOST_TSO6 )) + dev->features |=NETIF_F_TSO6 ; +if (gso && vdev->config->feature (vdev,VIRTIO_NET_F_HOST_ECN )) + dev->features |=NETIF_F_TSO_ECN ; +if (gso && vdev->config->feature (vdev,VIRTIO_NET_F_HOST_UFO )) + dev->features |=NETIF_F_UFO ; }/* Configuration may specify what MAC to use. Otherwise random. */
include/linux/virtio_net.h +11 -2
@@ -6,9 +6,18 @@#define VIRTIO_ID_NET 1/* The feature bitmap for virtio net */ -#define VIRTIO_NET_F_CSUM 0/* Can handle pkts w/ partial csum */ +#define VIRTIO_NET_F_CSUM 0/* Host handles pkts w/ partial csum */ +#define VIRTIO_NET_F_GUEST_CSUM 1/* Guest handles pkts w/ partial csum */ #define VIRTIO_NET_F_MAC 5/* Host has given MAC address. */ -#define VIRTIO_NET_F_GSO 6/* Can handle pkts w/ any GSO type */ +#define VIRTIO_NET_F_GSO 6/* Host handles pkts w/ any GSO type */ +#define VIRTIO_NET_F_GUEST_TSO4 7/* Guest can handle TSOv4 in. */ +#define VIRTIO_NET_F_GUEST_TSO6 8/* Guest can handle TSOv6 in. */ +#define VIRTIO_NET_F_GUEST_ECN 9/* Guest can handle TSO[6] w/ ECN in. */ +#define VIRTIO_NET_F_GUEST_UFO 10/* Guest can handle UFO in. */ +#define VIRTIO_NET_F_HOST_TSO4 11/* Host can handle TSOv4 in. */ +#define VIRTIO_NET_F_HOST_TSO6 12/* Host can handle TSOv6 in. */ +#define VIRTIO_NET_F_HOST_ECN 13/* Host can handle TSO[6] w/ ECN in. */ +#define VIRTIO_NET_F_HOST_UFO 14/* Host can handle UFO in. */ struct virtio_net_config {
PATCH 3/4 virtio: net: Add ethtool ops for SG/GSO
Date: Fri, 18 Apr 2008 11:21:42 +0800
From: Herbert Xu <[email protected]>
This patch adds some basic ethtool operations to virtio_net so I could test SG without GSO (which was really useful because TSO turned out to be buggy :)
--- drivers/net/virtio_net.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
drivers/net/virtio_net.c +18 -0
@@ -19,6 +19,7 @@//#define DEBUG #include <linux/netdevice.h> #include <linux/etherdevice.h> +#include <linux/ethtool.h> #include <linux/module.h> #include <linux/virtio.h> #include <linux/virtio_net.h>
@@ -352,6 +353,22 @@ static int virtnet_close(struct net_devireturn 0 ; } +static int virtnet_set_tx_csum (struct net_device *dev,u32 data) +{ +struct virtnet_info *vi =netdev_priv (dev); +struct virtio_device *vdev = vi->vdev; + +if (data && !virtio_has_feature (vdev,VIRTIO_NET_F_CSUM )) +return -ENOSYS ; + +return ethtool_op_set_tx_hw_csum (dev, data); +} + +static struct ethtool_ops virtnet_ethtool_ops = { + .set_tx_csum = virtnet_set_tx_csum, + .set_sg = ethtool_op_set_sg, +}; +static int virtnet_probe (struct virtio_device *vdev) {int err;
@@ -371,6 +389,7 @@ static int virtnet_probe(struct virtio_d#ifdef CONFIG_NET_POLL_CONTROLLER dev->poll_controller = virtnet_netpoll;#endif +SET_ETHTOOL_OPS (dev, &virtnet_ethtool_ops);SET_NETDEV_DEV (dev, &vdev->dev);/* Do we support "hardware" checksums? */
PATCH 3/4 virtio: net: Add ethtool ops for SG/GSO
In reply to Rusty RussellDate: Fri, 18 Apr 2008 11:21:42 +0800
From: Herbert Xu <[email protected]>
This patch adds some basic ethtool operations to virtio_net so I could test SG without GSO (which was really useful because TSO turned out to be buggy :)
--- drivers/net/virtio_net.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
drivers/net/virtio_net.c +18 -0
@@ -19,6 +19,7 @@//#define DEBUG #include <linux/netdevice.h> #include <linux/etherdevice.h> +#include <linux/ethtool.h> #include <linux/module.h> #include <linux/virtio.h> #include <linux/virtio_net.h>
@@ -352,6 +353,22 @@ static int virtnet_close(struct net_devireturn 0 ; } +static int virtnet_set_tx_csum (struct net_device *dev,u32 data) +{ +struct virtnet_info *vi =netdev_priv (dev); +struct virtio_device *vdev = vi->vdev; + +if (data && !virtio_has_feature (vdev,VIRTIO_NET_F_CSUM )) +return -ENOSYS ; + +return ethtool_op_set_tx_hw_csum (dev, data); +} + +static struct ethtool_ops virtnet_ethtool_ops = { + .set_tx_csum = virtnet_set_tx_csum, + .set_sg = ethtool_op_set_sg, +}; +static int virtnet_probe (struct virtio_device *vdev) {int err;
@@ -371,6 +389,7 @@ static int virtnet_probe(struct virtio_d#ifdef CONFIG_NET_POLL_CONTROLLER dev->poll_controller = virtnet_netpoll;#endif +SET_ETHTOOL_OPS (dev, &virtnet_ethtool_ops);SET_NETDEV_DEV (dev, &vdev->dev);/* Do we support "hardware" checksums? */
PATCH 4/4 virtio: net: Allow receiving SG packets
Date: Fri, 18 Apr 2008 11:24:27 +0800
From: Herbert Xu <[email protected]>
Finally this patch lets virtio_net receive GSO packets in addition to sending them. This can definitely be optimised for the non-GSO case. For comparison the Xen approach stores one page in each skb and uses subsequent skb's pages to construct an SG skb instead of preallocating the maximum amount of pages per skb.
--- drivers/net/virtio_net.c | 40 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-)
drivers/net/virtio_net.c +36 -4
@@ -77,6 +77,7 @@ static void receive_skb(struct net_devic unsigned len) {struct virtio_net_hdr *hdr =skb_vnet_hdr (skb); +int err;if (unlikely (len <sizeof (struct virtio_net_hdr ) +ETH_HLEN )) {pr_debug ("%s: short packet %i\n" , dev->name, len);
@@ -84,9 +85,15 @@ static void receive_skb(struct net_devic goto drop; } len -=sizeof (struct virtio_net_hdr ); -BUG_ON (len >MAX_PACKET_LEN ); -skb_trim (skb, len); + err =pskb_trim (skb, len); +if (err) { +pr_debug ("%s: pskb_trim failed %i %d\n" , dev->name, len, err); + dev->stats.rx_dropped++; + goto drop; + } + skb->truesize += skb->data_len; + 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);
@@ -147,7 +154,7 @@ static void try_fill_recv(struct virtnet {struct sk_buff *skb;struct scatterlist sg[2 +MAX_SKB_FRAGS ]; -int num, err; +int num, err, i;sg_init_table (sg,2 +MAX_SKB_FRAGS );for (;;) {
@@ -157,6 +164,24 @@ static void try_fill_recv(struct virtnetskb_put (skb,MAX_PACKET_LEN );vnet_hdr_to_sg (sg, skb); + +if (vi->dev->features &NETIF_F_LRO ) { +for (i =0 ; i <MAX_SKB_FRAGS ; i++) { +skb_frag_t *f = &skb_shinfo (skb)->frags[i]; + f->page =alloc_page (GFP_ATOMIC ); +if (!f->page) +break ; + + f->page_offset =0 ; + f->size =PAGE_SIZE ; + + skb->data_len +=PAGE_SIZE ; + skb->len +=PAGE_SIZE ; + +skb_shinfo (skb)->nr_frags++; + } + } + num =skb_to_sgvec (skb, sg+1 ,0 , skb->len) +1 ;skb_queue_head (&vi->recv, skb);
@@ -411,6 +436,12 @@ static int virtnet_probe(struct virtio_d dev->features |=NETIF_F_UFO ; } +/* If we can receive ANY GSO packets, we must allocate large ones. */ +if (virtio_has_feature (vdev,VIRTIO_NET_F_GUEST_TSO4 ) + ||virtio_has_feature (vdev,VIRTIO_NET_F_GUEST_TSO6 ) + ||virtio_has_feature (vdev,VIRTIO_NET_F_GUEST_ECN )) + dev->features |=NETIF_F_LRO ; +/* Configuration may specify what MAC to use. Otherwise random. */ if (virtio_has_feature (vdev,VIRTIO_NET_F_MAC )) { vdev->config->get(vdev,
@@ -504,7 +535,8 @@ static unsigned int features[] = {static unsigned int features[] = {VIRTIO_NET_F_CSUM ,VIRTIO_NET_F_GSO ,VIRTIO_NET_F_MAC ,VIRTIO_NET_F_HOST_TSO4 ,VIRTIO_NET_F_HOST_UFO ,VIRTIO_NET_F_HOST_TSO6 , -VIRTIO_NET_F_HOST_ECN , +VIRTIO_NET_F_HOST_ECN ,VIRTIO_NET_F_GUEST_TSO4 ,VIRTIO_NET_F_GUEST_TSO6 , +VIRTIO_NET_F_GUEST_ECN ,/* We don't yet handle UFO input. */ };static struct virtio_driver virtio_net = {
PATCH 4/4 virtio: net: Allow receiving SG packets
In reply to Rusty RussellDate: Fri, 18 Apr 2008 11:24:27 +0800
From: Herbert Xu <[email protected]>
Finally this patch lets virtio_net receive GSO packets in addition to sending them. This can definitely be optimised for the non-GSO case. For comparison the Xen approach stores one page in each skb and uses subsequent skb's pages to construct an SG skb instead of preallocating the maximum amount of pages per skb.
--- drivers/net/virtio_net.c | 40 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-)
drivers/net/virtio_net.c +36 -4
@@ -77,6 +77,7 @@ static void receive_skb(struct net_devic unsigned len) {struct virtio_net_hdr *hdr =skb_vnet_hdr (skb); +int err;if (unlikely (len <sizeof (struct virtio_net_hdr ) +ETH_HLEN )) {pr_debug ("%s: short packet %i\n" , dev->name, len);
@@ -84,9 +85,15 @@ static void receive_skb(struct net_devic goto drop; } len -=sizeof (struct virtio_net_hdr ); -BUG_ON (len >MAX_PACKET_LEN ); -skb_trim (skb, len); + err =pskb_trim (skb, len); +if (err) { +pr_debug ("%s: pskb_trim failed %i %d\n" , dev->name, len, err); + dev->stats.rx_dropped++; + goto drop; + } + skb->truesize += skb->data_len; + 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);
@@ -147,7 +154,7 @@ static void try_fill_recv(struct virtnet {struct sk_buff *skb;struct scatterlist sg[2 +MAX_SKB_FRAGS ]; -int num, err; +int num, err, i;sg_init_table (sg,2 +MAX_SKB_FRAGS );for (;;) {
@@ -157,6 +164,24 @@ static void try_fill_recv(struct virtnetskb_put (skb,MAX_PACKET_LEN );vnet_hdr_to_sg (sg, skb); + +if (vi->dev->features &NETIF_F_LRO ) { +for (i =0 ; i <MAX_SKB_FRAGS ; i++) { +skb_frag_t *f = &skb_shinfo (skb)->frags[i]; + f->page =alloc_page (GFP_ATOMIC ); +if (!f->page) +break ; + + f->page_offset =0 ; + f->size =PAGE_SIZE ; + + skb->data_len +=PAGE_SIZE ; + skb->len +=PAGE_SIZE ; + +skb_shinfo (skb)->nr_frags++; + } + } + num =skb_to_sgvec (skb, sg+1 ,0 , skb->len) +1 ;skb_queue_head (&vi->recv, skb);
@@ -411,6 +436,12 @@ static int virtnet_probe(struct virtio_d dev->features |=NETIF_F_UFO ; } +/* If we can receive ANY GSO packets, we must allocate large ones. */ +if (virtio_has_feature (vdev,VIRTIO_NET_F_GUEST_TSO4 ) + ||virtio_has_feature (vdev,VIRTIO_NET_F_GUEST_TSO6 ) + ||virtio_has_feature (vdev,VIRTIO_NET_F_GUEST_ECN )) + dev->features |=NETIF_F_LRO ; +/* Configuration may specify what MAC to use. Otherwise random. */ if (virtio_has_feature (vdev,VIRTIO_NET_F_MAC )) { vdev->config->get(vdev,
@@ -504,7 +535,8 @@ static unsigned int features[] = {static unsigned int features[] = {VIRTIO_NET_F_CSUM ,VIRTIO_NET_F_GSO ,VIRTIO_NET_F_MAC ,VIRTIO_NET_F_HOST_TSO4 ,VIRTIO_NET_F_HOST_UFO ,VIRTIO_NET_F_HOST_TSO6 , -VIRTIO_NET_F_HOST_ECN , +VIRTIO_NET_F_HOST_ECN ,VIRTIO_NET_F_GUEST_TSO4 ,VIRTIO_NET_F_GUEST_TSO6 , +VIRTIO_NET_F_GUEST_ECN ,/* We don't yet handle UFO input. */ };static struct virtio_driver virtio_net = {
drivers/net/virtio_net.c static int start_xmit(struct sk_buff *sk · line 294
Quoted 7 lines · Rusty Russell
drivers/net/virtio_net.c +6
-0+/* If we has a buffer left over from last time, send it now. */ +if (vi->last_xmit_skb) { +if (xmit_skb (vi, vi->last_xmit_skb) !=0 ) { +/* Drop this skb: we only queue one. */ +kfree_skb (skb); + goto stop_queue;
We should increment the drop counter here.
Otherwise these patches all look good to me.
Thanks,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txtdrivers/net/virtio_net.c static int start_xmit(struct sk_buff *sk · line 294
Quoted 7 lines · Rusty Russell
drivers/net/virtio_net.c +6
-0+/* If we has a buffer left over from last time, send it now. */ +if (vi->last_xmit_skb) { +if (xmit_skb (vi, vi->last_xmit_skb) !=0 ) { +/* Drop this skb: we only queue one. */ +kfree_skb (skb); + goto stop_queue;
We should increment the drop counter here.
Otherwise these patches all look good to me.
Thanks,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt