From 23cde76d801246a702e7a84c3fe3d655b35c89a1 Mon Sep 17 00:00:00 2001 From: Mark McLoughlin Date: Sun, 8 Jun 2008 20:49:00 +1000 Subject: virtio_net: Fix skb->csum_start computation hdr->csum_start is the offset from the start of the ethernet header to the transport layer checksum field. skb->csum_start is the offset from skb->head. skb_partial_csum_set() assumes that skb->data points to the ethernet header - i.e. it computes skb->csum_start by adding the headroom to hdr->csum_start. Since eth_type_trans() skb_pull()s the ethernet header, skb_partial_csum_set() should be called before eth_type_trans(). (Without this patch, GSO packets from a guest to the world outside the host are corrupted). Signed-off-by: Mark McLoughlin Acked-by: Herbert Xu Signed-off-by: Rusty Russell Signed-off-by: Jeff Garzik --- drivers/net/virtio_net.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'drivers/net/virtio_net.c') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 5450eac9e26..9a3b85e55cc 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -94,9 +94,7 @@ static void receive_skb(struct net_device *dev, struct sk_buff *skb, BUG_ON(len > MAX_PACKET_LEN); skb_trim(skb, 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); + dev->stats.rx_bytes += skb->len; dev->stats.rx_packets++; @@ -106,6 +104,10 @@ static void receive_skb(struct net_device *dev, struct sk_buff *skb, goto frame_err; } + 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); + if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) { pr_debug("GSO!\n"); switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) { -- cgit v1.2.3-70-g09d2 From 14c998f034bdc9a5bfa53bca18fbd0738cbc65e8 Mon Sep 17 00:00:00 2001 From: Mark McLoughlin Date: Sun, 8 Jun 2008 20:50:56 +1000 Subject: virtio: virtio_net free transmit skbs in a timer virtio_net currently only frees old transmit skbs just before queueing new ones. If the queue is full, it then enables interrupts and waits for notification that more work has been performed. However, a side-effect of this scheme is that there are always xmit skbs left dangling when no new packets are sent, against the Documentation/networking/driver.txt guideline: "... it is not allowed for your TX mitigation scheme to let TX packets "hang out" in the TX ring unreclaimed forever if no new TX packets are sent." Add a timer to ensure that any time we queue new TX skbs, we will shortly free them again. This fixes an easily reproduced hang at shutdown where iptables attempts to unload nf_conntrack and nf_conntrack waits for an skb it is tracking to be freed, but virtio_net never frees it. Signed-off-by: Mark McLoughlin Signed-off-by: Rusty Russell Signed-off-by: Jeff Garzik --- drivers/net/virtio_net.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) (limited to 'drivers/net/virtio_net.c') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 9a3b85e55cc..156d76fee16 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -44,6 +44,8 @@ struct virtnet_info /* The skb we couldn't send because buffers were full. */ struct sk_buff *last_xmit_skb; + struct timer_list xmit_free_timer; + /* Number of input buffers, and max we've ever had. */ unsigned int num, max; @@ -240,9 +242,23 @@ static void free_old_xmit_skbs(struct virtnet_info *vi) } } +static void xmit_free(unsigned long data) +{ + struct virtnet_info *vi = (void *)data; + + netif_tx_lock(vi->dev); + + free_old_xmit_skbs(vi); + + if (!skb_queue_empty(&vi->send)) + mod_timer(&vi->xmit_free_timer, jiffies + (HZ/10)); + + netif_tx_unlock(vi->dev); +} + static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb) { - int num; + int num, err; struct scatterlist sg[2+MAX_SKB_FRAGS]; struct virtio_net_hdr *hdr; const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest; @@ -285,7 +301,11 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb) vnet_hdr_to_sg(sg, skb); num = skb_to_sgvec(skb, sg+1, 0, skb->len) + 1; - return vi->svq->vq_ops->add_buf(vi->svq, sg, num, 0, skb); + err = vi->svq->vq_ops->add_buf(vi->svq, sg, num, 0, skb); + if (!err) + mod_timer(&vi->xmit_free_timer, jiffies + (HZ/10)); + + return err; } static void xmit_tasklet(unsigned long data) @@ -456,6 +476,8 @@ static int virtnet_probe(struct virtio_device *vdev) tasklet_init(&vi->tasklet, xmit_tasklet, (unsigned long)vi); + setup_timer(&vi->xmit_free_timer, xmit_free, (unsigned long)vi); + err = register_netdev(dev); if (err) { pr_debug("virtio_net: registering device failed\n"); @@ -493,6 +515,8 @@ static void virtnet_remove(struct virtio_device *vdev) /* Stop all the virtqueues. */ vdev->config->reset(vdev); + del_timer_sync(&vi->xmit_free_timer); + /* Free our skbs in send and recv queues, if any. */ while ((skb = __skb_dequeue(&vi->recv)) != NULL) { kfree_skb(skb); -- cgit v1.2.3-70-g09d2 From 363f15149cfba67d29f1e6a6103dda079f27f3fa Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 8 Jun 2008 20:51:55 +1000 Subject: virtio: use callback on empty in virtio_net virtio_net uses a timer to free old transmitted packets, rather than leaving callbacks enabled all the time. If the host promises to always notify us when the transmit ring is empty, we can free packets at that point and avoid the timer. Signed-off-by: Rusty Russell Signed-off-by: Jeff Garzik --- drivers/net/virtio_net.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) (limited to 'drivers/net/virtio_net.c') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 156d76fee16..4452306d532 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -44,6 +44,7 @@ struct virtnet_info /* The skb we couldn't send because buffers were full. */ struct sk_buff *last_xmit_skb; + /* If we need to free in a timer, this is it. */ struct timer_list xmit_free_timer; /* Number of input buffers, and max we've ever had. */ @@ -51,6 +52,7 @@ struct virtnet_info /* For cleaning up after transmission. */ struct tasklet_struct tasklet; + bool free_in_tasklet; /* Receive & send queues. */ struct sk_buff_head recv; @@ -74,7 +76,7 @@ static void skb_xmit_done(struct virtqueue *svq) /* Suppress further interrupts. */ svq->vq_ops->disable_cb(svq); - /* We were waiting for more output buffers. */ + /* We were probably waiting for more output buffers. */ netif_wake_queue(vi->dev); /* Make sure we re-xmit last_xmit_skb: if there are no more packets @@ -242,6 +244,8 @@ static void free_old_xmit_skbs(struct virtnet_info *vi) } } +/* If the virtio transport doesn't always notify us when all in-flight packets + * are consumed, we fall back to using this function on a timer to free them. */ static void xmit_free(unsigned long data) { struct virtnet_info *vi = (void *)data; @@ -302,7 +306,7 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb) num = skb_to_sgvec(skb, sg+1, 0, skb->len) + 1; err = vi->svq->vq_ops->add_buf(vi->svq, sg, num, 0, skb); - if (!err) + if (!err && !vi->free_in_tasklet) mod_timer(&vi->xmit_free_timer, jiffies + (HZ/10)); return err; @@ -317,6 +321,8 @@ static void xmit_tasklet(unsigned long data) vi->svq->vq_ops->kick(vi->svq); vi->last_xmit_skb = NULL; } + if (vi->free_in_tasklet) + free_old_xmit_skbs(vi); netif_tx_unlock_bh(vi->dev); } @@ -457,6 +463,10 @@ static int virtnet_probe(struct virtio_device *vdev) vi->vdev = vdev; vdev->priv = vi; + /* If they give us a callback when all buffers are done, we don't need + * the timer. */ + vi->free_in_tasklet = virtio_has_feature(vdev,VIRTIO_F_NOTIFY_ON_EMPTY); + /* We expect two virtqueues, receive then send. */ vi->rvq = vdev->config->find_vq(vdev, 0, skb_recv_done); if (IS_ERR(vi->rvq)) { @@ -476,7 +486,8 @@ static int virtnet_probe(struct virtio_device *vdev) tasklet_init(&vi->tasklet, xmit_tasklet, (unsigned long)vi); - setup_timer(&vi->xmit_free_timer, xmit_free, (unsigned long)vi); + if (!vi->free_in_tasklet) + setup_timer(&vi->xmit_free_timer, xmit_free, (unsigned long)vi); err = register_netdev(dev); if (err) { @@ -515,7 +526,8 @@ static void virtnet_remove(struct virtio_device *vdev) /* Stop all the virtqueues. */ vdev->config->reset(vdev); - del_timer_sync(&vi->xmit_free_timer); + if (!vi->free_in_tasklet) + del_timer_sync(&vi->xmit_free_timer); /* Free our skbs in send and recv queues, if any. */ while ((skb = __skb_dequeue(&vi->recv)) != NULL) { @@ -540,7 +552,7 @@ static struct virtio_device_id id_table[] = { 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_F_NOTIFY_ON_EMPTY, }; static struct virtio_driver virtio_net = { -- cgit v1.2.3-70-g09d2