From 288369cc2580178ef6ed7c5c63cc1ef08c803250 Mon Sep 17 00:00:00 2001 From: Wang Chen Date: Thu, 22 May 2008 18:07:43 +0800 Subject: VIRTIO: Use __skb_queue_purge() Use standard routine for queue purging. Signed-off-by: Wang Chen Signed-off-by: Jeff Garzik --- drivers/net/virtio_net.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers/net/virtio_net.c') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index f926b5ab3d09..fe7cdf2a2a23 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -470,8 +470,7 @@ static void virtnet_remove(struct virtio_device *vdev) kfree_skb(skb); vi->num--; } - while ((skb = __skb_dequeue(&vi->send)) != NULL) - kfree_skb(skb); + __skb_queue_purge(&vi->send); BUG_ON(vi->num != 0); -- cgit v1.2.3 From 7eb2e25112bf920bb0a4d1cca445f3d96874c25f Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 26 May 2008 17:42:42 +1000 Subject: virtio: fix virtio_net xmit of freed skb bug If we fail to transmit a packet, we assume the queue is full and put the skb into last_xmit_skb. However, if more space frees up before we xmit it, we loop, and the result can be transmitting the same skb twice. Fix is simple: set skb to NULL if we've used it in some way, and check before sending. Signed-off-by: Rusty Russell Signed-off-by: Jeff Garzik --- drivers/net/virtio_net.c | 14 +++++++++----- 1 file changed, 9 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 fe7cdf2a2a23..d50f4fe352b3 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -287,21 +287,25 @@ again: free_old_xmit_skbs(vi); /* If we has a buffer left over from last time, send it now. */ - if (vi->last_xmit_skb) { + if (unlikely(vi->last_xmit_skb)) { if (xmit_skb(vi, vi->last_xmit_skb) != 0) { /* Drop this skb: we only queue one. */ vi->dev->stats.tx_dropped++; kfree_skb(skb); + skb = NULL; goto stop_queue; } vi->last_xmit_skb = NULL; } /* 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; + if (likely(skb)) { + __skb_queue_head(&vi->send, skb); + if (xmit_skb(vi, skb) != 0) { + vi->last_xmit_skb = skb; + skb = NULL; + goto stop_queue; + } } done: vi->svq->vq_ops->kick(vi->svq); -- cgit v1.2.3 From 11a3a1546d0adc36485c2ad4af7ab950712df6ff Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 26 May 2008 17:48:13 +1000 Subject: virtio: fix delayed xmit of packet and freeing of old packets. Because we cache the last failed-to-xmit packet, if there are no packets queued behind that one we may never send it (reproduced here as TCP stalls, "cured" by an outgoing ping). Cc: Mark McLoughlin Signed-off-by: Rusty Russell Signed-off-by: Jeff Garzik --- drivers/net/virtio_net.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) (limited to 'drivers/net/virtio_net.c') diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index d50f4fe352b3..5450eac9e263 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -47,6 +47,9 @@ struct virtnet_info /* Number of input buffers, and max we've ever had. */ unsigned int num, max; + /* For cleaning up after transmission. */ + struct tasklet_struct tasklet; + /* Receive & send queues. */ struct sk_buff_head recv; struct sk_buff_head send; @@ -68,8 +71,13 @@ static void skb_xmit_done(struct virtqueue *svq) /* Suppress further interrupts. */ svq->vq_ops->disable_cb(svq); + /* We were waiting for more output buffers. */ netif_wake_queue(vi->dev); + + /* Make sure we re-xmit last_xmit_skb: if there are no more packets + * queued, start_xmit won't be called. */ + tasklet_schedule(&vi->tasklet); } static void receive_skb(struct net_device *dev, struct sk_buff *skb, @@ -278,6 +286,18 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb) return vi->svq->vq_ops->add_buf(vi->svq, sg, num, 0, skb); } +static void xmit_tasklet(unsigned long data) +{ + struct virtnet_info *vi = (void *)data; + + netif_tx_lock_bh(vi->dev); + if (vi->last_xmit_skb && xmit_skb(vi, vi->last_xmit_skb) == 0) { + vi->svq->vq_ops->kick(vi->svq); + vi->last_xmit_skb = NULL; + } + netif_tx_unlock_bh(vi->dev); +} + static int start_xmit(struct sk_buff *skb, struct net_device *dev) { struct virtnet_info *vi = netdev_priv(dev); @@ -432,6 +452,8 @@ static int virtnet_probe(struct virtio_device *vdev) skb_queue_head_init(&vi->recv); skb_queue_head_init(&vi->send); + tasklet_init(&vi->tasklet, xmit_tasklet, (unsigned long)vi); + err = register_netdev(dev); if (err) { pr_debug("virtio_net: registering device failed\n"); -- cgit v1.2.3 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 5450eac9e263..9a3b85e55ccc 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 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 9a3b85e55ccc..156d76fee164 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 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 156d76fee164..4452306d5328 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