netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Rusty Russell <rusty@rustcorp.com.au>
To: Jeff Garzik <jeff@garzik.org>
Cc: netdev@vger.kernel.org,
	virtualization@lists.linux-foundation.org,
	Mark McLoughlin <markmc@redhat.com>
Subject: [PATCH 3/4] virtio: virtio_net free transmit skbs in a timer
Date: Sun, 8 Jun 2008 20:50:56 +1000	[thread overview]
Message-ID: <200806082050.56535.rusty@rustcorp.com.au> (raw)
In-Reply-To: <200806082050.00149.rusty@rustcorp.com.au>

From: Mark McLoughlin <markmc@redhat.com>

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 <markmc@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/virtio_net.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -43,6 +43,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;
@@ -238,9 +240,23 @@ static void free_old_xmit_skbs(struct 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;
@@ -283,7 +299,11 @@ static int xmit_skb(struct virtnet_info 
 	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)
@@ -452,6 +472,8 @@ static int virtnet_probe(struct virtio_d
 
 	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");
@@ -488,6 +510,8 @@ static void virtnet_remove(struct virtio
 
 	/* 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) {

  reply	other threads:[~2008-06-08 10:51 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-06-08 10:49 [PATCH 1/4] virtio_net: Fix skb->csum_start computation Rusty Russell
2008-06-08 10:49 ` [PATCH 2/4] virtio: Fix typo in virtio_net_hdr comments Rusty Russell
2008-06-08 10:50   ` Rusty Russell [this message]
2008-06-08 10:51     ` [PATCH 4/4] virtio: use callback on empty in virtio_net Rusty Russell
2008-06-10 22:21 ` [PATCH 1/4] virtio_net: Fix skb->csum_start computation Jeff Garzik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200806082050.56535.rusty@rustcorp.com.au \
    --to=rusty@rustcorp.com.au \
    --cc=jeff@garzik.org \
    --cc=markmc@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=virtualization@lists.linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).