All of lore.kernel.org
 help / color / mirror / Atom feed
* bug in virtio network driver?
@ 2007-08-21  8:48 Christian Borntraeger
       [not found] ` <200708211048.33534.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
  2007-08-21  9:09 ` Rusty Russell
  0 siblings, 2 replies; 12+ messages in thread
From: Christian Borntraeger @ 2007-08-21  8:48 UTC (permalink / raw)
  To: Rusty Russell; +Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, virtualization

Hello Rusty,

I think I have found a problem in the virtio network driver. virtio_net 
reclaims sent skbs on xmit. That means that there is always one skb 
outstanding and the netdev packet statistic is always one packet to low.

Documentation/networking/drivers.txt says

3) Do not forget that once you return 0 from your hard_start_xmit
   method, it is your driver's responsibility to free up the SKB
   and in some finite amount of time.

   For example, this means that 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.
   This error can deadlock sockets waiting for send buffer room
   to be freed up.

One solution would be to use the xmit_done interrupt. Unfortunately this would 
require additional locking as multiple interrupts can happen at two or more 
cpus. Do you have any better ideas?

Christian

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug in virtio network driver?
@ 2007-08-21  8:48 Christian Borntraeger
  0 siblings, 0 replies; 12+ messages in thread
From: Christian Borntraeger @ 2007-08-21  8:48 UTC (permalink / raw)
  To: Rusty Russell; +Cc: kvm-devel, virtualization

Hello Rusty,

I think I have found a problem in the virtio network driver. virtio_net 
reclaims sent skbs on xmit. That means that there is always one skb 
outstanding and the netdev packet statistic is always one packet to low.

Documentation/networking/drivers.txt says

3) Do not forget that once you return 0 from your hard_start_xmit
   method, it is your driver's responsibility to free up the SKB
   and in some finite amount of time.

   For example, this means that 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.
   This error can deadlock sockets waiting for send buffer room
   to be freed up.

One solution would be to use the xmit_done interrupt. Unfortunately this would 
require additional locking as multiple interrupts can happen at two or more 
cpus. Do you have any better ideas?

Christian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug in virtio network driver?
  2007-08-21  8:48 bug in virtio network driver? Christian Borntraeger
       [not found] ` <200708211048.33534.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
@ 2007-08-21  9:09 ` Rusty Russell
  1 sibling, 0 replies; 12+ messages in thread
From: Rusty Russell @ 2007-08-21  9:09 UTC (permalink / raw)
  To: Christian Borntraeger; +Cc: kvm-devel, virtualization

On Tue, 2007-08-21 at 10:48 +0200, Christian Borntraeger wrote:
> Hello Rusty,
> 
> I think I have found a problem in the virtio network driver. virtio_net 
> reclaims sent skbs on xmit. That means that there is always one skb 
> outstanding and the netdev packet statistic is always one packet to low.

Hi Christian,

	Good catch!

> One solution would be to use the xmit_done interrupt. Unfortunately this would 
> require additional locking as multiple interrupts can happen at two or more 
> cpus. Do you have any better ideas?

The only reason that we don't do it in skb_xmit_done() is because
kfree_skb() isn't supposed to be called from an interrupt.  But there's
dev_kfree_skb_any() which can be used.

Cheers,
Rusty.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug in virtio network driver?
       [not found] ` <200708211048.33534.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
@ 2007-08-21  9:09   ` Rusty Russell
  2007-08-21 15:06     ` Arnd Bergmann
                       ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Rusty Russell @ 2007-08-21  9:09 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, virtualization

On Tue, 2007-08-21 at 10:48 +0200, Christian Borntraeger wrote:
> Hello Rusty,
> 
> I think I have found a problem in the virtio network driver. virtio_net 
> reclaims sent skbs on xmit. That means that there is always one skb 
> outstanding and the netdev packet statistic is always one packet to low.

Hi Christian,

	Good catch!

> One solution would be to use the xmit_done interrupt. Unfortunately this would 
> require additional locking as multiple interrupts can happen at two or more 
> cpus. Do you have any better ideas?

The only reason that we don't do it in skb_xmit_done() is because
kfree_skb() isn't supposed to be called from an interrupt.  But there's
dev_kfree_skb_any() which can be used.

Cheers,
Rusty.



-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug in virtio network driver?
  2007-08-21  9:09   ` Rusty Russell
@ 2007-08-21 15:06     ` Arnd Bergmann
  2007-08-22  3:08       ` Rusty Russell
       [not found]     ` <1187687377.19435.176.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
  2007-08-21 16:02     ` Christian Borntraeger
  2 siblings, 1 reply; 12+ messages in thread
From: Arnd Bergmann @ 2007-08-21 15:06 UTC (permalink / raw)
  To: virtualization

On Tuesday 21 August 2007, Rusty Russell wrote:
> > One solution would be to use the xmit_done interrupt. Unfortunately this would 
> > require additional locking as multiple interrupts can happen at two or more 
> > cpus. Do you have any better ideas?
> 
> The only reason that we don't do it in skb_xmit_done() is because
> kfree_skb() isn't supposed to be called from an interrupt.  But there's
> dev_kfree_skb_any() which can be used.

That will simply trigger a softirq, right? If you need the softirq anyway,
why not do all the tx handling in the poll() method like other drivers?

	Arnd <><

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug in virtio network driver?
  2007-08-21  9:09   ` Rusty Russell
  2007-08-21 15:06     ` Arnd Bergmann
       [not found]     ` <1187687377.19435.176.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
@ 2007-08-21 16:02     ` Christian Borntraeger
  2 siblings, 0 replies; 12+ messages in thread
From: Christian Borntraeger @ 2007-08-21 16:02 UTC (permalink / raw)
  To: virtualization; +Cc: kvm-devel

Am Dienstag, 21. August 2007 schrieb Rusty Russell:
> The only reason that we don't do it in skb_xmit_done() is because
> kfree_skb() isn't supposed to be called from an interrupt.  But there's
> dev_kfree_skb_any() which can be used.

Ok, I now hacked something that works but I really dont like the 
local_irq_disable bits. I sent this patch nevertheless, but I will look into 
Arnds suggestion.

--- linux-2.6.22.orig/drivers/net/virtio_net.c
+++ linux-2.6.22/drivers/net/virtio_net.c
@@ -53,12 +52,31 @@ static void vnet_hdr_to_sg(struct scatte
 	sg->length = sizeof(struct virtio_net_hdr);
 }
 
+static void free_old_xmit_skbs(struct virtnet_info *vi)
+{
+	struct sk_buff *skb;
+	unsigned int len;
+
+	netif_tx_lock(vi->ndev);
+	while ((skb = vi->vq_send->ops->get_buf(vi->vq_send, &len)) != NULL) {
+		/* They cannot have written to the packet. */
+		BUG_ON(len != 0);
+		pr_debug("Sent skb %p\n", skb);
+		__skb_unlink(skb, &vi->send);
+		vi->ndev->stats.tx_bytes += skb->len;
+		vi->ndev->stats.tx_packets++;
+		dev_kfree_skb_irq(skb);
+	}
+	netif_tx_unlock(vi->ndev);
+}
+
 static bool skb_xmit_done(struct virtqueue *vq)
 {
 	struct virtnet_info *vi = vq->priv;
 
 	/* In case we were waiting for output buffers. */
 	netif_wake_queue(vi->ndev);
+	free_old_xmit_skbs(vi);
 	return true;
 }
 
@@ -214,22 +232,6 @@ again:
 	return 0;
 }
 
-static void free_old_xmit_skbs(struct virtnet_info *vi)
-{
-	struct sk_buff *skb;
-	unsigned int len;
-
-	while ((skb = vi->vq_send->ops->get_buf(vi->vq_send, &len)) != NULL) {
-		/* They cannot have written to the packet. */
-		BUG_ON(len != 0);
-		pr_debug("Sent skb %p\n", skb);
-		__skb_unlink(skb, &vi->send);
-		vi->ndev->stats.tx_bytes += skb->len;
-		vi->ndev->stats.tx_packets++;
-		kfree_skb(skb);
-	}
-}
-
 static int start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -238,12 +240,12 @@ static int start_xmit(struct sk_buff *sk
 	struct virtio_net_hdr *hdr;
 	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
 
+	local_irq_disable();
+	netif_tx_lock(vi->ndev);
 	pr_debug("%s: xmit %p %02x:%02x:%02x:%02x:%02x:%02x\n",
 		 dev->name, skb,
 		 dest[0], dest[1], dest[2], dest[3], dest[4], dest[5]);
 
-	free_old_xmit_skbs(vi);
-
 	/* Encode metadata header at front. */
 	hdr = skb_vnet_hdr(skb);
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
@@ -280,10 +282,13 @@ static int start_xmit(struct sk_buff *sk
 		pr_debug("%s: virtio not prepared to send\n", dev->name);
 		skb_unlink(skb, &vi->send);
 		netif_stop_queue(dev);
+		netif_tx_unlock(vi->ndev);
+		local_irq_enable();
 		return NETDEV_TX_BUSY;
 	}
 	vi->vq_send->ops->sync(vi->vq_send);
-
+	netif_tx_unlock(vi->ndev);
+	local_irq_enable();
 	return 0;
 }
 
@@ -343,7 +348,7 @@ struct net_device *virtnet_probe(struct 
 	dev->poll = virtnet_poll;
 	dev->hard_start_xmit = start_xmit;
 	dev->weight = 16;
-	dev->features = features;
+	dev->features = features | NETIF_F_LLTX;
 	SET_NETDEV_DEV(dev, device);
 
 	vi = netdev_priv(dev);

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug in virtio network driver?
       [not found]     ` <1187687377.19435.176.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
@ 2007-08-21 16:02       ` Christian Borntraeger
  2007-08-29 21:07         ` Rusty Russell
       [not found]         ` <200708211802.04103.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
  0 siblings, 2 replies; 12+ messages in thread
From: Christian Borntraeger @ 2007-08-21 16:02 UTC (permalink / raw)
  To: virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
  Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f

Am Dienstag, 21. August 2007 schrieb Rusty Russell:
> The only reason that we don't do it in skb_xmit_done() is because
> kfree_skb() isn't supposed to be called from an interrupt.  But there's
> dev_kfree_skb_any() which can be used.

Ok, I now hacked something that works but I really dont like the 
local_irq_disable bits. I sent this patch nevertheless, but I will look into 
Arnds suggestion.

--- linux-2.6.22.orig/drivers/net/virtio_net.c
+++ linux-2.6.22/drivers/net/virtio_net.c
@@ -53,12 +52,31 @@ static void vnet_hdr_to_sg(struct scatte
 	sg->length = sizeof(struct virtio_net_hdr);
 }
 
+static void free_old_xmit_skbs(struct virtnet_info *vi)
+{
+	struct sk_buff *skb;
+	unsigned int len;
+
+	netif_tx_lock(vi->ndev);
+	while ((skb = vi->vq_send->ops->get_buf(vi->vq_send, &len)) != NULL) {
+		/* They cannot have written to the packet. */
+		BUG_ON(len != 0);
+		pr_debug("Sent skb %p\n", skb);
+		__skb_unlink(skb, &vi->send);
+		vi->ndev->stats.tx_bytes += skb->len;
+		vi->ndev->stats.tx_packets++;
+		dev_kfree_skb_irq(skb);
+	}
+	netif_tx_unlock(vi->ndev);
+}
+
 static bool skb_xmit_done(struct virtqueue *vq)
 {
 	struct virtnet_info *vi = vq->priv;
 
 	/* In case we were waiting for output buffers. */
 	netif_wake_queue(vi->ndev);
+	free_old_xmit_skbs(vi);
 	return true;
 }
 
@@ -214,22 +232,6 @@ again:
 	return 0;
 }
 
-static void free_old_xmit_skbs(struct virtnet_info *vi)
-{
-	struct sk_buff *skb;
-	unsigned int len;
-
-	while ((skb = vi->vq_send->ops->get_buf(vi->vq_send, &len)) != NULL) {
-		/* They cannot have written to the packet. */
-		BUG_ON(len != 0);
-		pr_debug("Sent skb %p\n", skb);
-		__skb_unlink(skb, &vi->send);
-		vi->ndev->stats.tx_bytes += skb->len;
-		vi->ndev->stats.tx_packets++;
-		kfree_skb(skb);
-	}
-}
-
 static int start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -238,12 +240,12 @@ static int start_xmit(struct sk_buff *sk
 	struct virtio_net_hdr *hdr;
 	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
 
+	local_irq_disable();
+	netif_tx_lock(vi->ndev);
 	pr_debug("%s: xmit %p %02x:%02x:%02x:%02x:%02x:%02x\n",
 		 dev->name, skb,
 		 dest[0], dest[1], dest[2], dest[3], dest[4], dest[5]);
 
-	free_old_xmit_skbs(vi);
-
 	/* Encode metadata header at front. */
 	hdr = skb_vnet_hdr(skb);
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
@@ -280,10 +282,13 @@ static int start_xmit(struct sk_buff *sk
 		pr_debug("%s: virtio not prepared to send\n", dev->name);
 		skb_unlink(skb, &vi->send);
 		netif_stop_queue(dev);
+		netif_tx_unlock(vi->ndev);
+		local_irq_enable();
 		return NETDEV_TX_BUSY;
 	}
 	vi->vq_send->ops->sync(vi->vq_send);
-
+	netif_tx_unlock(vi->ndev);
+	local_irq_enable();
 	return 0;
 }
 
@@ -343,7 +348,7 @@ struct net_device *virtnet_probe(struct 
 	dev->poll = virtnet_poll;
 	dev->hard_start_xmit = start_xmit;
 	dev->weight = 16;
-	dev->features = features;
+	dev->features = features | NETIF_F_LLTX;
 	SET_NETDEV_DEV(dev, device);
 
 	vi = netdev_priv(dev);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug in virtio network driver?
  2007-08-21 15:06     ` Arnd Bergmann
@ 2007-08-22  3:08       ` Rusty Russell
  0 siblings, 0 replies; 12+ messages in thread
From: Rusty Russell @ 2007-08-22  3:08 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: virtualization

On Tue, 2007-08-21 at 17:06 +0200, Arnd Bergmann wrote:
> On Tuesday 21 August 2007, Rusty Russell wrote:
> > > One solution would be to use the xmit_done interrupt. Unfortunately this would 
> > > require additional locking as multiple interrupts can happen at two or more 
> > > cpus. Do you have any better ideas?
> > 
> > The only reason that we don't do it in skb_xmit_done() is because
> > kfree_skb() isn't supposed to be called from an interrupt.  But there's
> > dev_kfree_skb_any() which can be used.
> 
> That will simply trigger a softirq, right? If you need the softirq anyway,
> why not do all the tx handling in the poll() method like other drivers?

That would work, too.  It seems a little non-obvious to me to trigger
poll when a packet has been transmitted, but if that's the Standard I'll
follow.

Cheers,
Rusty.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug in virtio network driver?
  2007-08-21 16:02       ` Christian Borntraeger
@ 2007-08-29 21:07         ` Rusty Russell
       [not found]         ` <200708211802.04103.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
  1 sibling, 0 replies; 12+ messages in thread
From: Rusty Russell @ 2007-08-29 21:07 UTC (permalink / raw)
  To: Christian Borntraeger; +Cc: kvm-devel, virtualization

On Tue, 2007-08-21 at 18:02 +0200, Christian Borntraeger wrote:
> Am Dienstag, 21. August 2007 schrieb Rusty Russell:
> > The only reason that we don't do it in skb_xmit_done() is because
> > kfree_skb() isn't supposed to be called from an interrupt.  But there's
> > dev_kfree_skb_any() which can be used.
> 
> Ok, I now hacked something that works but I really dont like the 
> local_irq_disable bits. I sent this patch nevertheless, but I will look into 
> Arnds suggestion.

Hi Christian,

	What's the status of this?  Should I apply this patch, or do you want
me to wait for a ->poll patch as per Arnd's suggestion?

Rusty.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug in virtio network driver?
       [not found]         ` <200708211802.04103.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
@ 2007-08-29 21:07           ` Rusty Russell
  2007-08-30 11:28             ` Christian Borntraeger
       [not found]             ` <1188421640.5531.141.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
  0 siblings, 2 replies; 12+ messages in thread
From: Rusty Russell @ 2007-08-29 21:07 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA

On Tue, 2007-08-21 at 18:02 +0200, Christian Borntraeger wrote:
> Am Dienstag, 21. August 2007 schrieb Rusty Russell:
> > The only reason that we don't do it in skb_xmit_done() is because
> > kfree_skb() isn't supposed to be called from an interrupt.  But there's
> > dev_kfree_skb_any() which can be used.
> 
> Ok, I now hacked something that works but I really dont like the 
> local_irq_disable bits. I sent this patch nevertheless, but I will look into 
> Arnds suggestion.

Hi Christian,

	What's the status of this?  Should I apply this patch, or do you want
me to wait for a ->poll patch as per Arnd's suggestion?

Rusty.



-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug in virtio network driver?
  2007-08-29 21:07           ` Rusty Russell
@ 2007-08-30 11:28             ` Christian Borntraeger
       [not found]             ` <1188421640.5531.141.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
  1 sibling, 0 replies; 12+ messages in thread
From: Christian Borntraeger @ 2007-08-30 11:28 UTC (permalink / raw)
  To: Rusty Russell; +Cc: kvm-devel, arnd, virtualization

Am Mittwoch, 29. August 2007 schrieb Rusty Russell:
> On Tue, 2007-08-21 at 18:02 +0200, Christian Borntraeger wrote:
> > Am Dienstag, 21. August 2007 schrieb Rusty Russell:
> > > The only reason that we don't do it in skb_xmit_done() is because
> > > kfree_skb() isn't supposed to be called from an interrupt.  But there's
> > > dev_kfree_skb_any() which can be used.
> > 
> > Ok, I now hacked something that works but I really dont like the 
> > local_irq_disable bits. I sent this patch nevertheless, but I will look 
into 
> > Arnds suggestion.
> 
> Hi Christian,
> 
> 	What's the status of this?  Should I apply this patch, or do you want
> me to wait for a ->poll patch as per Arnd's suggestion?

I looked at Arnds suggestions. It seems that even with a cleanup in the poll 
function, we have no guarantee that the outstanding skb is reclaimed because 
there might be no incoming packet. Other drivers (like spider_net) solve this 
by using an additional tx_timer.
I start to think that my latest patch is actually not that bad: lets do the 
reclaim when the host tells us its done. 

Arnd, do you have an opinion about that?

Christian

for reference:

---
 drivers/net/virtio_net.c |   47 
++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

Index: linux-2.6.22/drivers/net/virtio_net.c
===================================================================
--- linux-2.6.22.orig/drivers/net/virtio_net.c
+++ linux-2.6.22/drivers/net/virtio_net.c
@@ -53,12 +52,31 @@ static void vnet_hdr_to_sg(struct scatte
 	sg->length = sizeof(struct virtio_net_hdr);
 }
 
+static void free_old_xmit_skbs(struct virtnet_info *vi)
+{
+	struct sk_buff *skb;
+	unsigned int len;
+
+	netif_tx_lock(vi->ndev);
+	while ((skb = vi->vq_send->ops->get_buf(vi->vq_send, &len)) != NULL) {
+		/* They cannot have written to the packet. */
+		BUG_ON(len != 0);
+		pr_debug("Sent skb %p\n", skb);
+		__skb_unlink(skb, &vi->send);
+		vi->ndev->stats.tx_bytes += skb->len;
+		vi->ndev->stats.tx_packets++;
+		dev_kfree_skb_irq(skb);
+	}
+	netif_tx_unlock(vi->ndev);
+}
+
 static bool skb_xmit_done(struct virtqueue *vq)
 {
 	struct virtnet_info *vi = vq->priv;
 
 	/* In case we were waiting for output buffers. */
 	netif_wake_queue(vi->ndev);
+	free_old_xmit_skbs(vi);
 	return true;
 }
 
@@ -214,22 +232,6 @@ again:
 	return 0;
 }
 
-static void free_old_xmit_skbs(struct virtnet_info *vi)
-{
-	struct sk_buff *skb;
-	unsigned int len;
-
-	while ((skb = vi->vq_send->ops->get_buf(vi->vq_send, &len)) != NULL) {
-		/* They cannot have written to the packet. */
-		BUG_ON(len != 0);
-		pr_debug("Sent skb %p\n", skb);
-		__skb_unlink(skb, &vi->send);
-		vi->ndev->stats.tx_bytes += skb->len;
-		vi->ndev->stats.tx_packets++;
-		kfree_skb(skb);
-	}
-}
-
 static int start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -238,12 +240,12 @@ static int start_xmit(struct sk_buff *sk
 	struct virtio_net_hdr *hdr;
 	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
 
+	local_irq_disable();
+	netif_tx_lock(vi->ndev);
 	pr_debug("%s: xmit %p %02x:%02x:%02x:%02x:%02x:%02x\n",
 		 dev->name, skb,
 		 dest[0], dest[1], dest[2], dest[3], dest[4], dest[5]);
 
-	free_old_xmit_skbs(vi);
-
 	/* Encode metadata header at front. */
 	hdr = skb_vnet_hdr(skb);
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
@@ -280,10 +282,13 @@ static int start_xmit(struct sk_buff *sk
 		pr_debug("%s: virtio not prepared to send\n", dev->name);
 		skb_unlink(skb, &vi->send);
 		netif_stop_queue(dev);
+		netif_tx_unlock(vi->ndev);
+		local_irq_enable();
 		return NETDEV_TX_BUSY;
 	}
 	vi->vq_send->ops->sync(vi->vq_send);
-
+	netif_tx_unlock(vi->ndev);
+	local_irq_enable();
 	return 0;
 }
 
@@ -343,7 +348,7 @@ struct net_device *virtnet_probe(struct 
 	dev->poll = virtnet_poll;
 	dev->hard_start_xmit = start_xmit;
 	dev->weight = 16;
-	dev->features = features;
+	dev->features = features | NETIF_F_LLTX;
 	SET_NETDEV_DEV(dev, device);
 
 	vi = netdev_priv(dev);

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: bug in virtio network driver?
       [not found]             ` <1188421640.5531.141.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
@ 2007-08-30 11:28               ` Christian Borntraeger
  0 siblings, 0 replies; 12+ messages in thread
From: Christian Borntraeger @ 2007-08-30 11:28 UTC (permalink / raw)
  To: Rusty Russell
  Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA

Am Mittwoch, 29. August 2007 schrieb Rusty Russell:
> On Tue, 2007-08-21 at 18:02 +0200, Christian Borntraeger wrote:
> > Am Dienstag, 21. August 2007 schrieb Rusty Russell:
> > > The only reason that we don't do it in skb_xmit_done() is because
> > > kfree_skb() isn't supposed to be called from an interrupt.  But there's
> > > dev_kfree_skb_any() which can be used.
> > 
> > Ok, I now hacked something that works but I really dont like the 
> > local_irq_disable bits. I sent this patch nevertheless, but I will look 
into 
> > Arnds suggestion.
> 
> Hi Christian,
> 
> 	What's the status of this?  Should I apply this patch, or do you want
> me to wait for a ->poll patch as per Arnd's suggestion?

I looked at Arnds suggestions. It seems that even with a cleanup in the poll 
function, we have no guarantee that the outstanding skb is reclaimed because 
there might be no incoming packet. Other drivers (like spider_net) solve this 
by using an additional tx_timer.
I start to think that my latest patch is actually not that bad: lets do the 
reclaim when the host tells us its done. 

Arnd, do you have an opinion about that?

Christian

for reference:

---
 drivers/net/virtio_net.c |   47 
++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

Index: linux-2.6.22/drivers/net/virtio_net.c
===================================================================
--- linux-2.6.22.orig/drivers/net/virtio_net.c
+++ linux-2.6.22/drivers/net/virtio_net.c
@@ -53,12 +52,31 @@ static void vnet_hdr_to_sg(struct scatte
 	sg->length = sizeof(struct virtio_net_hdr);
 }
 
+static void free_old_xmit_skbs(struct virtnet_info *vi)
+{
+	struct sk_buff *skb;
+	unsigned int len;
+
+	netif_tx_lock(vi->ndev);
+	while ((skb = vi->vq_send->ops->get_buf(vi->vq_send, &len)) != NULL) {
+		/* They cannot have written to the packet. */
+		BUG_ON(len != 0);
+		pr_debug("Sent skb %p\n", skb);
+		__skb_unlink(skb, &vi->send);
+		vi->ndev->stats.tx_bytes += skb->len;
+		vi->ndev->stats.tx_packets++;
+		dev_kfree_skb_irq(skb);
+	}
+	netif_tx_unlock(vi->ndev);
+}
+
 static bool skb_xmit_done(struct virtqueue *vq)
 {
 	struct virtnet_info *vi = vq->priv;
 
 	/* In case we were waiting for output buffers. */
 	netif_wake_queue(vi->ndev);
+	free_old_xmit_skbs(vi);
 	return true;
 }
 
@@ -214,22 +232,6 @@ again:
 	return 0;
 }
 
-static void free_old_xmit_skbs(struct virtnet_info *vi)
-{
-	struct sk_buff *skb;
-	unsigned int len;
-
-	while ((skb = vi->vq_send->ops->get_buf(vi->vq_send, &len)) != NULL) {
-		/* They cannot have written to the packet. */
-		BUG_ON(len != 0);
-		pr_debug("Sent skb %p\n", skb);
-		__skb_unlink(skb, &vi->send);
-		vi->ndev->stats.tx_bytes += skb->len;
-		vi->ndev->stats.tx_packets++;
-		kfree_skb(skb);
-	}
-}
-
 static int start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -238,12 +240,12 @@ static int start_xmit(struct sk_buff *sk
 	struct virtio_net_hdr *hdr;
 	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
 
+	local_irq_disable();
+	netif_tx_lock(vi->ndev);
 	pr_debug("%s: xmit %p %02x:%02x:%02x:%02x:%02x:%02x\n",
 		 dev->name, skb,
 		 dest[0], dest[1], dest[2], dest[3], dest[4], dest[5]);
 
-	free_old_xmit_skbs(vi);
-
 	/* Encode metadata header at front. */
 	hdr = skb_vnet_hdr(skb);
 	if (skb->ip_summed == CHECKSUM_PARTIAL) {
@@ -280,10 +282,13 @@ static int start_xmit(struct sk_buff *sk
 		pr_debug("%s: virtio not prepared to send\n", dev->name);
 		skb_unlink(skb, &vi->send);
 		netif_stop_queue(dev);
+		netif_tx_unlock(vi->ndev);
+		local_irq_enable();
 		return NETDEV_TX_BUSY;
 	}
 	vi->vq_send->ops->sync(vi->vq_send);
-
+	netif_tx_unlock(vi->ndev);
+	local_irq_enable();
 	return 0;
 }
 
@@ -343,7 +348,7 @@ struct net_device *virtnet_probe(struct 
 	dev->poll = virtnet_poll;
 	dev->hard_start_xmit = start_xmit;
 	dev->weight = 16;
-	dev->features = features;
+	dev->features = features | NETIF_F_LLTX;
 	SET_NETDEV_DEV(dev, device);
 
 	vi = netdev_priv(dev);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2007-08-30 11:28 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-08-21  8:48 bug in virtio network driver? Christian Borntraeger
     [not found] ` <200708211048.33534.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
2007-08-21  9:09   ` Rusty Russell
2007-08-21 15:06     ` Arnd Bergmann
2007-08-22  3:08       ` Rusty Russell
     [not found]     ` <1187687377.19435.176.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2007-08-21 16:02       ` Christian Borntraeger
2007-08-29 21:07         ` Rusty Russell
     [not found]         ` <200708211802.04103.borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
2007-08-29 21:07           ` Rusty Russell
2007-08-30 11:28             ` Christian Borntraeger
     [not found]             ` <1188421640.5531.141.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2007-08-30 11:28               ` Christian Borntraeger
2007-08-21 16:02     ` Christian Borntraeger
2007-08-21  9:09 ` Rusty Russell
  -- strict thread matches above, loose matches on Subject: below --
2007-08-21  8:48 Christian Borntraeger

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.