Skip to contentSkip to conversation

virtio: fix net driver loop case where we fail to restart

PATCH In review Rusty Russell 4 participants 7 messages Nov 19, 2007 – Nov 20, 2007 1 message not in the archive

Subsystems (4)
Networking Network drivers virtio core virtio net driver
lore
Share & tools
lore

Rusty Russell

author
#0 lore raw

PATCH 1/2 virtio: fix net driver loop case where we fail to restart

skb is only NULL the first time around: it's more correct to test for being under-budget.

Signed-off-by Rusty Russell <rusty-8n+1lVoiYb80n/[email protected]>
drivers/net/virtio_net.c +2 -2
diff -r 2a94425ac7d5 drivers/net/virtio_net.c
@@ -198,8 +198,8 @@ again:
 	if (vi->num < vi->max / 2)
 		try_fill_recv(vi);
 
-	/* All done? */
-	if (!skb) {
+	/* Out of packets? */
+	if (received < budget) {
 		netif_rx_complete(vi->dev, napi);
 		if (unlikely(!vi->rvq->vq_ops->restart(vi->rvq))
 		    && netif_rx_reschedule(vi->dev, napi))

A message that is not in the archive (sent off-list or not yet received): 200711191120.15596.rusty-8n+1lVoiYb80n/[email protected]

author
#2 lore raw

PATCH 2/2 virtio: free transmit skbs when notified, not on next xmit.

This fixes a potential dangling xmit problem.

We also suppress refill interrupts until we need them. (Anthony and I have been working on performance recently, so this is a WIP).

Signed-off-by Rusty Russell <rusty-8n+1lVoiYb80n/[email protected]>
drivers/net/virtio_net.c +49 -20
diff -r 586fb6ff29dd drivers/net/virtio_net.c
@@ -35,6 +35,8 @@ struct virtnet_info
 	struct net_device *dev;
 	struct napi_struct napi;
 
+	struct tasklet_struct xmit_free;
+
 	/* TX coalescing. */
 	struct hrtimer tx_timer;
 

@@ -59,13 +61,40 @@ static inline void vnet_hdr_to_sg(struct
 	sg_init_one(sg, skb_vnet_hdr(skb), sizeof(struct virtio_net_hdr));
 }
 
+static unsigned free_old_xmit_skbs(struct virtnet_info *vi)
+{
+	struct sk_buff *skb;
+	unsigned int len, i = 0;
+
+	while ((skb = vi->svq->vq_ops->get_buf(vi->svq, &len)) != NULL) {
+		pr_debug("Sent skb %p\n", skb);
+		__skb_unlink(skb, &vi->send);
+		vi->dev->stats.tx_bytes += len;
+		vi->dev->stats.tx_packets++;
+		kfree_skb(skb);
+		i++;
+	}
+	return i;
+}
+
+static void xmit_free(unsigned long data)
+{
+	struct virtnet_info *vi = (void *)data;
+
+	netif_tx_lock(vi->dev);
+	free_old_xmit_skbs(vi);
+	netif_tx_unlock(vi->dev);
+
+	/* In case we were waiting for output buffers. */
+	netif_wake_queue(vi->dev);
+}	
+
 static bool skb_xmit_done(struct virtqueue *rvq)
 {
 	struct virtnet_info *vi = rvq->vdev->priv;
 
-	/* In case we were waiting for output buffers. */
-	netif_wake_queue(vi->dev);
-	return true;
+	tasklet_schedule(&vi->xmit_free);
+	return false;
 }
 
 static void receive_skb(struct net_device *dev, struct sk_buff *skb,

@@ -216,20 +245,6 @@ again:
 	return received;
 }
 
-static void free_old_xmit_skbs(struct virtnet_info *vi)
-{
-	struct sk_buff *skb;
-	unsigned int len;
-
-	while ((skb = vi->svq->vq_ops->get_buf(vi->svq, &len)) != NULL) {
-		pr_debug("Sent skb %p\n", skb);
-		__skb_unlink(skb, &vi->send);
-		vi->dev->stats.tx_bytes += len;
-		vi->dev->stats.tx_packets++;
-		kfree_skb(skb);
-	}
-}
-
 static enum hrtimer_restart kick_xmit(struct hrtimer *t)
 {
 	struct virtnet_info *vi = container_of(t,struct virtnet_info,tx_timer);

@@ -256,8 +271,6 @@ static int start_xmit(struct sk_buff *sk
 	sg_init_table(sg, 1+MAX_SKB_FRAGS);
 
 	pr_debug("%s: xmit %p %s\n", dev->name, skb, print_mac(mac, dest));
-
-	free_old_xmit_skbs(vi);
 
 	/* Encode metadata header at front. */
 	hdr = skb_vnet_hdr(skb);

@@ -290,11 +303,25 @@ static int start_xmit(struct sk_buff *sk
 	vnet_hdr_to_sg(sg, skb);
 	num = skb_to_sgvec(skb, sg+1, 0, skb->len) + 1;
 	__skb_queue_head(&vi->send, skb);
+
+again:
 	err = vi->svq->vq_ops->add_buf(vi->svq, sg, num, 0, skb);
 	if (err) {
+		/* Can we free any used skbs? */
+		if (free_old_xmit_skbs(vi))
+			goto again;
+
+		/* Activate callback for using skbs: if this fails it
+		 * means some were used in the meantime. */
+		if (unlikely(!vi->svq->vq_ops->restart(vi->svq))) {
+			printk("Unlikely: restart svq failed\n");
+			goto again;
+		}
+
+		__skb_unlink(skb, &vi->send);
+
 		pr_debug("%s: virtio not prepared to send\n", dev->name);
-		skb_unlink(skb, &vi->send);
 		if (vi->out_max != vi->out_num)
 			printk("%s: out_max changed from %u to %u\n",
 			       dev->name, vi->out_max, vi->out_num);
 		vi->out_max = vi->out_num;

@@ -400,5 +428,7 @@ static int virtnet_probe(struct virtio_d
 	vi->out_max = -1U;
 
+	tasklet_init(&vi->xmit_free, xmit_free, (unsigned long)vi);
+	
 	/* We expect two virtqueues, receive then send. */
 	vi->rvq = vdev->config->find_vq(vdev, 0, skb_recv_done);
 	if (IS_ERR(vi->rvq)) {
Acked-by on v1 2/2
#3 lore raw

Re: [PATCH 2/2] virtio: free transmit skbs when notified, not on next xmit.

Am Montag, 19. November 2007 schrieb Rusty Russell:

Quoted 7 lines

This fixes a potential dangling xmit problem.

We also suppress refill interrupts until we need them. (Anthony and I have been working on performance recently, so this is a WIP).

Signed-off-by Rusty Russell <[email protected]>

Thanks Rusty,
that was the 2nd next item on my todo list :-)

Acked-by Christian Borntraeger <[email protected]>
Acked-by on v1 2/2
#4 lore raw

Re: [PATCH 2/2] virtio: free transmit skbs when notified, not on next xmit.

In reply to Rusty Russell

Am Montag, 19. November 2007 schrieb Rusty Russell:

Quoted 7 lines

This fixes a potential dangling xmit problem.

We also suppress refill interrupts until we need them. (Anthony and I have been working on performance recently, so this is a WIP).

Signed-off-by Rusty Russell <[email protected]>

Thanks Rusty,
that was the 2nd next item on my todo list :-)

Acked-by Christian Borntraeger <[email protected]>
#5 lore raw

PATCH 2/2 virtio: free transmit skbs when notified, not on next xmit.

In reply to Rusty Russell

This fixes a potential dangling xmit problem.

We also suppress refill interrupts until we need them. (Anthony and I have been working on performance recently, so this is a WIP).

Signed-off-by Rusty Russell <[email protected]>
drivers/net/virtio_net.c +49 -20
diff -r 586fb6ff29dd drivers/net/virtio_net.c
@@ -35,6 +35,8 @@ struct virtnet_info
 	struct net_device *dev;
 	struct napi_struct napi;
 
+	struct tasklet_struct xmit_free;
+
 	/* TX coalescing. */
 	struct hrtimer tx_timer;
 

@@ -59,13 +61,40 @@ static inline void vnet_hdr_to_sg(struct
 	sg_init_one(sg, skb_vnet_hdr(skb), sizeof(struct virtio_net_hdr));
 }
 
+static unsigned free_old_xmit_skbs(struct virtnet_info *vi)
+{
+	struct sk_buff *skb;
+	unsigned int len, i = 0;
+
+	while ((skb = vi->svq->vq_ops->get_buf(vi->svq, &len)) != NULL) {
+		pr_debug("Sent skb %p\n", skb);
+		__skb_unlink(skb, &vi->send);
+		vi->dev->stats.tx_bytes += len;
+		vi->dev->stats.tx_packets++;
+		kfree_skb(skb);
+		i++;
+	}
+	return i;
+}
+
+static void xmit_free(unsigned long data)
+{
+	struct virtnet_info *vi = (void *)data;
+
+	netif_tx_lock(vi->dev);
+	free_old_xmit_skbs(vi);
+	netif_tx_unlock(vi->dev);
+
+	/* In case we were waiting for output buffers. */
+	netif_wake_queue(vi->dev);
+}	
+
 static bool skb_xmit_done(struct virtqueue *rvq)
 {
 	struct virtnet_info *vi = rvq->vdev->priv;
 
-	/* In case we were waiting for output buffers. */
-	netif_wake_queue(vi->dev);
-	return true;
+	tasklet_schedule(&vi->xmit_free);
+	return false;
 }
 
 static void receive_skb(struct net_device *dev, struct sk_buff *skb,

@@ -216,20 +245,6 @@ again:
 	return received;
 }
 
-static void free_old_xmit_skbs(struct virtnet_info *vi)
-{
-	struct sk_buff *skb;
-	unsigned int len;
-
-	while ((skb = vi->svq->vq_ops->get_buf(vi->svq, &len)) != NULL) {
-		pr_debug("Sent skb %p\n", skb);
-		__skb_unlink(skb, &vi->send);
-		vi->dev->stats.tx_bytes += len;
-		vi->dev->stats.tx_packets++;
-		kfree_skb(skb);
-	}
-}
-
 static enum hrtimer_restart kick_xmit(struct hrtimer *t)
 {
 	struct virtnet_info *vi = container_of(t,struct virtnet_info,tx_timer);

@@ -256,8 +271,6 @@ static int start_xmit(struct sk_buff *sk
 	sg_init_table(sg, 1+MAX_SKB_FRAGS);
 
 	pr_debug("%s: xmit %p %s\n", dev->name, skb, print_mac(mac, dest));
-
-	free_old_xmit_skbs(vi);
 
 	/* Encode metadata header at front. */
 	hdr = skb_vnet_hdr(skb);

@@ -290,11 +303,25 @@ static int start_xmit(struct sk_buff *sk
 	vnet_hdr_to_sg(sg, skb);
 	num = skb_to_sgvec(skb, sg+1, 0, skb->len) + 1;
 	__skb_queue_head(&vi->send, skb);
+
+again:
 	err = vi->svq->vq_ops->add_buf(vi->svq, sg, num, 0, skb);
 	if (err) {
+		/* Can we free any used skbs? */
+		if (free_old_xmit_skbs(vi))
+			goto again;
+
+		/* Activate callback for using skbs: if this fails it
+		 * means some were used in the meantime. */
+		if (unlikely(!vi->svq->vq_ops->restart(vi->svq))) {
+			printk("Unlikely: restart svq failed\n");
+			goto again;
+		}
+
+		__skb_unlink(skb, &vi->send);
+
 		pr_debug("%s: virtio not prepared to send\n", dev->name);
-		skb_unlink(skb, &vi->send);
 		if (vi->out_max != vi->out_num)
 			printk("%s: out_max changed from %u to %u\n",
 			       dev->name, vi->out_max, vi->out_num);
 		vi->out_max = vi->out_num;

@@ -400,5 +428,7 @@ static int virtnet_probe(struct virtio_d
 	vi->out_max = -1U;
 
+	tasklet_init(&vi->xmit_free, xmit_free, (unsigned long)vi);
+	
 	/* We expect two virtqueues, receive then send. */
 	vi->rvq = vdev->config->find_vq(vdev, 0, skb_recv_done);
 	if (IS_ERR(vi->rvq)) {
Acked-by on v1 1/2
#6 lore raw
In reply to Rusty Russell
Quoted 4 lines · Rusty Russell

skb is only NULL the first time around: it's more correct to test for being under-budget.

Signed-off-by Rusty Russell <[email protected]>

That looks better to me.

Acked-by Simon Horman <[email protected]>
Quoted 18 lines

diff -r 2a94425ac7d5 drivers/net/virtio_net.c

drivers/net/virtio_net.c +2 -3
@@ -198,8 +198,8 @@ again:
 	if (vi->num < vi->max / 2)
 		try_fill_recv(vi);
 
-	/* All done? */
-	if (!skb) {
+	/* Out of packets? */
+	if (received < budget) {
 		netif_rx_complete(vi->dev, napi);
 		if (unlikely(!vi->rvq->vq_ops->restart(vi->rvq))
 		    && netif_rx_reschedule(vi->dev, napi))
-

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line
Horms
Acked-by on v1 1/2
#7 lore raw
In reply to Rusty Russell
Quoted 4 lines · Rusty Russell

skb is only NULL the first time around: it's more correct to test for being under-budget.

Signed-off-by Rusty Russell <[email protected]>

That looks better to me.

Acked-by Simon Horman <[email protected]>
Quoted 18 lines

diff -r 2a94425ac7d5 drivers/net/virtio_net.c

drivers/net/virtio_net.c +2 -3
@@ -198,8 +198,8 @@ again:
 	if (vi->num < vi->max / 2)
 		try_fill_recv(vi);
 
-	/* All done? */
-	if (!skb) {
+	/* Out of packets? */
+	if (received < budget) {
 		netif_rx_complete(vi->dev, napi);
 		if (unlikely(!vi->rvq->vq_ops->restart(vi->rvq))
 		    && netif_rx_reschedule(vi->dev, napi))
-

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line
Horms