virtio and vhost-net capacity handling

Add API to check ring capacity. Because of the option to use indirect buffers, this returns the worst case, not the normal case capacity.

Quoted 3 lines · Michael S. Tsirkin

Add API to check ring capacity. Because of the option to use indirect buffers, this returns the worst case, not the normal case capacity.

Can we drop the silly "add_buf() returns capacity" hack then?

Thanks,
Rusty.

Quoted 3 lines · Michael S. Tsirkin

Add API to check ring capacity. Because of the option to use indirect buffers, this returns the worst case, not the normal case capacity.

Can we drop the silly "add_buf() returns capacity" hack then?

Thanks,
Rusty.

Add API to check ring capacity. Because of the option to use indirect buffers, this returns the worst case, not the normal case capacity.

In the (rare) case where new descriptors are used
while virtio_net enables vq callback for the TX vq, virtio_net uses the number of sg entries in the skb it frees to calculate how many descriptors in the ring have just been made available. But this value is an overestimate: with indirect buffers each skb only uses one descriptor entry, meaning we may wake the queue only to find we still can't transmit anything.

Quoted 7 lines · Michael S. Tsirkin

In the (rare) case where new descriptors are used
while virtio_net enables vq callback for the TX vq, virtio_net uses the number of sg entries in the skb it frees to calculate how many descriptors in the ring have just been made available. But this value is an overestimate: with indirect buffers each skb only uses one descriptor entry, meaning we may wake the queue only to find we still can't transmit anything.

This is a bit misleading.

The value is an overestimate, but so is the requirement for 2+MAX_SKB_FRAGS, *unless* we suddenly drop into direct mode due to OOM.

Thanks,
Rusty.

Quoted 7 lines · Michael S. Tsirkin

In the (rare) case where new descriptors are used
while virtio_net enables vq callback for the TX vq, virtio_net uses the number of sg entries in the skb it frees to calculate how many descriptors in the ring have just been made available. But this value is an overestimate: with indirect buffers each skb only uses one descriptor entry, meaning we may wake the queue only to find we still can't transmit anything.

This is a bit misleading.

The value is an overestimate, but so is the requirement for 2+MAX_SKB_FRAGS, *unless* we suddenly drop into direct mode due to OOM.

Thanks,
Rusty.

In the (rare) case where new descriptors are used
while virtio_net enables vq callback for the TX vq, virtio_net uses the number of sg entries in the skb it frees to calculate how many descriptors in the ring have just been made available. But this value is an overestimate: with indirect buffers each skb only uses one descriptor entry, meaning we may wake the queue only to find we still can't transmit anything.

Current code might introduce a lot of latency variation if there are many pending bufs at the time we attempt to transmit a new one. This is bad for real-time applications and can't be good for TCP either.

Free up just enough to both clean up all buffers eventually and to be able to xmit the next packet.

Quoted 7 lines · Michael S. Tsirkin

Current code might introduce a lot of latency variation if there are many pending bufs at the time we attempt to transmit a new one. This is bad for real-time applications and can't be good for TCP either.

Free up just enough to both clean up all buffers eventually and to be able to xmit the next packet.

OK, I found this quite confusing to read.

drivers/net/virtio_net.c again: · line 533

Quoted 13 lines

drivers/net/virtio_net.c +6 -1

-	while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
+	while ((r = virtqueue_min_capacity(vi->svq) < MAX_SKB_FRAGS + 2) ||
+	       min_skbs-- > 0) {
+		skb = virtqueue_get_buf(vi->svq, &len);
+		if (unlikely(!skb))
+			break;
 		pr_debug("Sent skb %p\n", skb);
 		vi->dev->stats.tx_bytes += skb->len;
 		vi->dev->stats.tx_packets++;
 		dev_kfree_skb_any(skb);
 	}
+	return r;
 }

Gah... what a horrible loop.

Basically, this patch makes hard-to-read code worse, and we should try to make it better.

Currently, xmit *can* fail when an xmit interrupt wakes the queue, but the packet(s) xmitted didn't free up enough space for the new packet. With indirect buffers this only happens if we hit OOM (and thus go to direct buffers).

We could solve this by only waking the queue in skb_xmit_done if the capacity is >= 2 + MAX_SKB_FRAGS. But can we do it without a race?

If not, then I'd really prefer to see this, because I think it's clearer:

        // Try to free 2 buffers for every 1 xmit, to stay ahead.
        free_old_buffers(2)

        if (!add_buf()) {
                // Screw latency, free them all.
                free_old_buffers(UINT_MAX)
                // OK, this can happen if we are using direct buffers,
                // and the xmit interrupt woke us but the packets
                // xmitted were smaller than this one.  Rare though.
                if (!add_buf())
                        Whinge and stop queue, maybe loop.
        }

        if (capacity < 2 + MAX_SKB_FRAGS) {
                // We don't have enough for the next packet?  Try
                // freeing more.
                free_old_buffers(UINT_MAX);
                if (capacity < 2 + MAX_SKB_FRAGS) {
                        Stop queue, maybe loop.
        }

The current code makes my head hurt :(

Thoughts?
Rusty.

OK, I have something very similar, but I still dislike the screw the latency part: this path is exactly what the IBM guys seem to hit. So I created two functions: one tries to free a constant number and another one up to capacity. I'll post that now.

Quoted 4 lines

OK, I have something very similar, but I still dislike the screw the latency part: this path is exactly what the IBM guys seem to hit. So I created two functions: one tries to free a constant number and another one up to capacity. I'll post that now.

Please review this patch to see if it looks reasonable:

1. Picked comments/code from MST's code and Rusty's review.
2. virtqueue_min_capacity() needs to be called only if it returned empty the last time it was called.
3. Fix return value bug in free_old_xmit_skbs (hangs guest).
4. Stop queue only if capacity is not enough for next xmit.

5. Fix/clean some likely/unlikely checks (hopefully).

I have done some minimal netperf tests with this.

With this patch, add_buf returning capacity seems to be useful - it allows less virtio API calls.

Signed-off-by Krishna Kumar <[email protected]>

---
 drivers/net/virtio_net.c |  105 ++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 41 deletions(-)

diff -ruNp org/drivers/net/virtio_net.c new/drivers/net/virtio_net.c

drivers/net/virtio_net.c +23 -16

@@ -509,27 +509,43 @@ again:
 	return received;
 }
 
-/* Check capacity and try to free enough pending old buffers to enable queueing
- * new ones.  If min_skbs > 0, try to free at least the specified number of skbs
- * even if the ring already has sufficient capacity.  Return true if we can
- * guarantee that a following virtqueue_add_buf will succeed. */
-static bool free_old_xmit_skbs(struct virtnet_info *vi, int min_skbs)
+/* Return true if freed a skb, else false */
+static inline bool free_one_old_xmit_skb(struct virtnet_info *vi)
 {
 	struct sk_buff *skb;
 	unsigned int len;
-	bool r;
 
-	while ((r = virtqueue_min_capacity(vi->svq) < MAX_SKB_FRAGS + 2) ||
-	       min_skbs-- > 0) {
-		skb = virtqueue_get_buf(vi->svq, &len);
-		if (unlikely(!skb))
+	skb = virtqueue_get_buf(vi->svq, &len);
+	if (unlikely(!skb))
+		return 0;
+
+	pr_debug("Sent skb %p\n", skb);
+	vi->dev->stats.tx_bytes += skb->len;
+	vi->dev->stats.tx_packets++;
+	dev_kfree_skb_any(skb);
+	return 1;
+}
+
+static bool free_old_xmit_skbs(struct virtnet_info *vi, int to_free)
+{
+	bool empty = virtqueue_min_capacity(vi->svq) < MAX_SKB_FRAGS + 2;
+
+	do {
+		if (!free_one_old_xmit_skb(vi)) {
+			/* No more skbs to free up */
 			break;
-		pr_debug("Sent skb %p\n", skb);
-		vi->dev->stats.tx_bytes += skb->len;
-		vi->dev->stats.tx_packets++;
-		dev_kfree_skb_any(skb);
-	}
-	return r;
+		}
+
+		if (empty) {

drivers/net/virtio_net.c +4 -9

 static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)

@@ -582,46 +598,53 @@ static int xmit_skb(struct virtnet_info 
 static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
-	int ret, n;
+	int capacity;
 
-	/* Free up space in the ring in case this is the first time we get
-	 * woken up after ring full condition.  Note: this might try to free
-	 * more than strictly necessary if the skb has a small
-	 * number of fragments, but keep it simple. */
-	free_old_xmit_skbs(vi, 0);
+	/* Try to free 2 buffers for every 1 xmit, to stay ahead. */
+	free_old_xmit_skbs(vi, 2);
 
 	/* Try to transmit */
-	ret = xmit_skb(vi, skb);
+	capacity = xmit_skb(vi, skb);
 
-	/* Failure to queue is unlikely. It's not a bug though: it might happen
-	 * if we get an interrupt while the queue is still mostly full.

drivers/net/virtio_net.c +8 -13

+		 */
 		if (net_ratelimit())
-			dev_info(&dev->dev, "TX queue failure: %d\n", ret);
+			dev_info(&dev->dev, "TX queue failure: %d\n", capacity);
 		dev->stats.tx_dropped++;
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}
+
 	virtqueue_kick(vi->svq);
 
 	/* Don't wait up for transmitted skbs to be freed. */
 	skb_orphan(skb);
 	nf_reset(skb);
 
-	/* Apparently nice girls don't return TX_BUSY; check capacity and stop
-	 * the queue before it gets out of hand.
-	 * Naturally, this wastes entries. */
-	/* We transmit one skb, so try to free at least two pending skbs.
-	 * This is so that we don't hog the skb memory unnecessarily. */
-	if (!likely(free_old_xmit_skbs(vi, 2))) {
-		netif_stop_queue(dev);
-		if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
-			/* More just got used, free them and recheck. */
-			if (!likely(free_old_xmit_skbs(vi, 0))) {
-				netif_start_queue(dev);
-				virtqueue_disable_cb(vi->svq);
+	/*
+	 * Apparently nice girls don't return TX_BUSY; check capacity and
+	 * stop the queue before it gets out of hand. Naturally, this wastes
+	 * entries. 
+	 */

drivers/net/virtio_net.c +3 -0

+					netif_start_queue(dev);
+					virtqueue_disable_cb(vi->svq);
+				}
 			}
 		}
 	}

Quoted 1 line

Please review this patch to see if it looks reasonable:

Hmm, since you decided to work on top of my patch, I'd appreciate split-up fixes.

Quoted 5 lines

1. Picked comments/code from MST's code and Rusty's review.
2. virtqueue_min_capacity() needs to be called only if it returned empty the last time it was called.
3. Fix return value bug in free_old_xmit_skbs (hangs guest).
4. Stop queue only if capacity is not enough for next xmit.

That's what we always did ...

Quoted 6 lines

5. Fix/clean some likely/unlikely checks (hopefully).

I have done some minimal netperf tests with this.

With this patch, add_buf returning capacity seems to be useful - it allows less virtio API calls.

Why bother? It's cheap ...

Quoted 21 lines

(quoted diff) +14 -6

+static bool free_old_xmit_skbs(struct virtnet_info *vi, int to_free)
+{
+   bool empty = virtqueue_min_capacity(vi->svq) < MAX_SKB_FRAGS + 2;
+
+   do {
+      if (!free_one_old_xmit_skb(vi)) {
+         /* No more skbs to free up */
          break;
-      pr_debug("Sent skb %p\n", skb);
-      vi->dev->stats.tx_bytes += skb->len;
-      vi->dev->stats.tx_packets++;
-      dev_kfree_skb_any(skb);
-   }
-   return r;
+      }
+
+      if (empty) {
+         /* Check again if there is enough space */
+         empty = virtqueue_min_capacity(vi->svq) <
+            MAX_SKB_FRAGS + 2;
+      } else {

Review

Quoted 6 lines

(quoted diff) +5 -0

+         --to_free;
+      }
+   } while (to_free > 0);
+
+   return !empty;
 }

Why bother doing the capacity check in this function?

Review

Quoted 8 lines

(quoted diff) +7 -1

-   if (unlikely(ret < 0)) {
+   if (unlikely(capacity < 0)) {
+      /*
+       * Failure to queue should be impossible. The only way to
+       * reach here is if we got a cb before 3/4th of space was
+       * available. We could stop the queue and re-enable
+       * callbacks (and possibly return TX_BUSY), but we don't
+       * bother since this is impossible.

It's far from impossible. The 3/4 thing is only a hint, and old devices don't support it anyway.

Review

Quoted 10 lines

(quoted diff) +3 -7

-   if (!likely(free_old_xmit_skbs(vi, 2))) {
-      netif_stop_queue(dev);
-      if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
-         /* More just got used, free them and recheck. */
-         if (!likely(free_old_xmit_skbs(vi, 0))) {
-            netif_start_queue(dev);
-            virtqueue_disable_cb(vi->svq);
+   /*
+    * Apparently nice girls don't return TX_BUSY; check capacity and
+    * stop the queue before it gets out of hand. Naturally, this

Review

Quoted 12 lines

(quoted diff) +12 -0

+    * entries.
+    */
+   if (capacity < 2+MAX_SKB_FRAGS) {
+      /*
+       * We don't have enough space for the next packet. Try
+       * freeing more.
+       */
+      if (likely(!free_old_xmit_skbs(vi, UINT_MAX))) {
+         netif_stop_queue(dev);
+         if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
+            /* More just got used, free them and recheck. */
+            if (likely(free_old_xmit_skbs(vi, UINT_MAX))) {

Is this where the bug was?

"Michael S. Tsirkin" <[email protected]> wrote on 06/02/2011 08:13:46 PM:

Quoted 4 lines

Quoted 1 line

Please review this patch to see if it looks reasonable:

Hmm, since you decided to work on top of my patch, I'd appreciate split-up fixes.

OK (that also explains your next comment).

Quoted 7 lines

Quoted 5 lines

1. Picked comments/code from MST's code and Rusty's review.
2. virtqueue_min_capacity() needs to be called only if it returned empty the last time it was called.
3. Fix return value bug in free_old_xmit_skbs (hangs guest).
4. Stop queue only if capacity is not enough for next xmit.

That's what we always did ...

I had made the patch against your patch, hence this change (sorry for the confusion!).

Quoted 8 lines

Quoted 6 lines

5. Fix/clean some likely/unlikely checks (hopefully).

I have done some minimal netperf tests with this.

With this patch, add_buf returning capacity seems to be useful - it allows less virtio API calls.

Why bother? It's cheap ...

If add_buf retains it's functionality to return the capacity (it is going to need a change to return 0 otherwise anyway), is it useful to call another function at each xmit?

Quoted 21 lines

Quoted 21 lines

(quoted diff) +14 -6

+static bool free_old_xmit_skbs(struct virtnet_info *vi, int to_free)
+{
+   bool empty = virtqueue_min_capacity(vi->svq) < MAX_SKB_FRAGS + 2;
+
+   do {
+      if (!free_one_old_xmit_skb(vi)) {
+         /* No more skbs to free up */
          break;
-      pr_debug("Sent skb %p\n", skb);
-      vi->dev->stats.tx_bytes += skb->len;
-      vi->dev->stats.tx_packets++;
-      dev_kfree_skb_any(skb);
-   }
-   return r;
+      }
+
+      if (empty) {
+         /* Check again if there is enough space */
+         empty = virtqueue_min_capacity(vi->svq) <
+            MAX_SKB_FRAGS + 2;
+      } else {

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +5 -0

+         --to_free;
+      }
+   } while (to_free > 0);
+
+   return !empty;
 }

Why bother doing the capacity check in this function?

To return whether we have enough space for next xmit. It should call it only once unless space is running out. Does it sound OK?

Quoted 11 lines

Review

Quoted 8 lines

(quoted diff) +7 -1

-   if (unlikely(ret < 0)) {
+   if (unlikely(capacity < 0)) {
+      /*
+       * Failure to queue should be impossible. The only way to
+       * reach here is if we got a cb before 3/4th of space was
+       * available. We could stop the queue and re-enable
+       * callbacks (and possibly return TX_BUSY), but we don't
+       * bother since this is impossible.

It's far from impossible. The 3/4 thing is only a hint, and old devices don't support it anyway.

OK, I will re-put back your comment.

Quoted 10 lines

Review

Quoted 10 lines

(quoted diff) +3 -7

-   if (!likely(free_old_xmit_skbs(vi, 2))) {
-      netif_stop_queue(dev);
-      if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
-         /* More just got used, free them and recheck. */
-         if (!likely(free_old_xmit_skbs(vi, 0))) {
-            netif_start_queue(dev);
-            virtqueue_disable_cb(vi->svq);
+   /*
+    * Apparently nice girls don't return TX_BUSY; check capacity and
+    * stop the queue before it gets out of hand. Naturally, this

wastes

Quoted 13 lines

Quoted 12 lines

(quoted diff) +12 -0

+    * entries.
+    */
+   if (capacity < 2+MAX_SKB_FRAGS) {
+      /*
+       * We don't have enough space for the next packet. Try
+       * freeing more.
+       */
+      if (likely(!free_old_xmit_skbs(vi, UINT_MAX))) {
+         netif_stop_queue(dev);
+         if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
+            /* More just got used, free them and recheck. */
+            if (likely(free_old_xmit_skbs(vi, UINT_MAX))) {

Quoted 1 line

Is this where the bug was?

Return value in free_old_xmit() was wrong. I will re-do against the mainline kernel.

Thanks,

- KK

Quoted 8 lines

Quoted 1 line

Is this where the bug was?

Return value in free_old_xmit() was wrong. I will re-do against the mainline kernel.

Thanks,

- KK

Just noting that I'm working on that patch as well, it might be more efficient if we don't both of us do this in parallel :)

Quoted 8 lines

Quoted 1 line

Is this where the bug was?

Return value in free_old_xmit() was wrong. I will re-do against the mainline kernel.

Thanks,

- KK

Just noting that I'm working on that patch as well, it might be more efficient if we don't both of us do this in parallel :)

"Michael S. Tsirkin" <[email protected]> wrote on 06/02/2011 08:13:46 PM:

Quoted 4 lines

Quoted 1 line

Please review this patch to see if it looks reasonable:

Hmm, since you decided to work on top of my patch, I'd appreciate split-up fixes.

OK (that also explains your next comment).

Quoted 7 lines

Quoted 5 lines

1. Picked comments/code from MST's code and Rusty's review.
2. virtqueue_min_capacity() needs to be called only if it returned empty the last time it was called.
3. Fix return value bug in free_old_xmit_skbs (hangs guest).
4. Stop queue only if capacity is not enough for next xmit.

That's what we always did ...

I had made the patch against your patch, hence this change (sorry for the confusion!).

Quoted 8 lines

Quoted 6 lines

5. Fix/clean some likely/unlikely checks (hopefully).

I have done some minimal netperf tests with this.

With this patch, add_buf returning capacity seems to be useful - it allows less virtio API calls.

Why bother? It's cheap ...

If add_buf retains it's functionality to return the capacity (it is going to need a change to return 0 otherwise anyway), is it useful to call another function at each xmit?

Quoted 21 lines

Quoted 21 lines

(quoted diff) +14 -6

+static bool free_old_xmit_skbs(struct virtnet_info *vi, int to_free)
+{
+   bool empty = virtqueue_min_capacity(vi->svq) < MAX_SKB_FRAGS + 2;
+
+   do {
+      if (!free_one_old_xmit_skb(vi)) {
+         /* No more skbs to free up */
          break;
-      pr_debug("Sent skb %p\n", skb);
-      vi->dev->stats.tx_bytes += skb->len;
-      vi->dev->stats.tx_packets++;
-      dev_kfree_skb_any(skb);
-   }
-   return r;
+      }
+
+      if (empty) {
+         /* Check again if there is enough space */
+         empty = virtqueue_min_capacity(vi->svq) <
+            MAX_SKB_FRAGS + 2;
+      } else {

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +5 -0

+         --to_free;
+      }
+   } while (to_free > 0);
+
+   return !empty;
 }

Why bother doing the capacity check in this function?

To return whether we have enough space for next xmit. It should call it only once unless space is running out. Does it sound OK?

Quoted 11 lines

Review

Quoted 8 lines

(quoted diff) +7 -1

-   if (unlikely(ret < 0)) {
+   if (unlikely(capacity < 0)) {
+      /*
+       * Failure to queue should be impossible. The only way to
+       * reach here is if we got a cb before 3/4th of space was
+       * available. We could stop the queue and re-enable
+       * callbacks (and possibly return TX_BUSY), but we don't
+       * bother since this is impossible.

It's far from impossible. The 3/4 thing is only a hint, and old devices don't support it anyway.

OK, I will re-put back your comment.

Quoted 10 lines

Review

Quoted 10 lines

(quoted diff) +3 -7

-   if (!likely(free_old_xmit_skbs(vi, 2))) {
-      netif_stop_queue(dev);
-      if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
-         /* More just got used, free them and recheck. */
-         if (!likely(free_old_xmit_skbs(vi, 0))) {
-            netif_start_queue(dev);
-            virtqueue_disable_cb(vi->svq);
+   /*
+    * Apparently nice girls don't return TX_BUSY; check capacity and
+    * stop the queue before it gets out of hand. Naturally, this

wastes

Quoted 13 lines

Quoted 12 lines

(quoted diff) +12 -0

+    * entries.
+    */
+   if (capacity < 2+MAX_SKB_FRAGS) {
+      /*
+       * We don't have enough space for the next packet. Try
+       * freeing more.
+       */
+      if (likely(!free_old_xmit_skbs(vi, UINT_MAX))) {
+         netif_stop_queue(dev);
+         if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
+            /* More just got used, free them and recheck. */
+            if (likely(free_old_xmit_skbs(vi, UINT_MAX))) {

Quoted 1 line

Is this where the bug was?

Return value in free_old_xmit() was wrong. I will re-do against the mainline kernel.

Thanks,

- KK

Return value in free_old_xmit() was wrong.

Return value in free_old_xmit() was wrong.

Quoted 1 line

Please review this patch to see if it looks reasonable:

Hmm, since you decided to work on top of my patch, I'd appreciate split-up fixes.

Quoted 5 lines

1. Picked comments/code from MST's code and Rusty's review.
2. virtqueue_min_capacity() needs to be called only if it returned empty the last time it was called.
3. Fix return value bug in free_old_xmit_skbs (hangs guest).
4. Stop queue only if capacity is not enough for next xmit.

That's what we always did ...

Quoted 6 lines

5. Fix/clean some likely/unlikely checks (hopefully).

I have done some minimal netperf tests with this.

With this patch, add_buf returning capacity seems to be useful - it allows less virtio API calls.

Why bother? It's cheap ...

Quoted 21 lines

(quoted diff) +14 -6

+static bool free_old_xmit_skbs(struct virtnet_info *vi, int to_free)
+{
+   bool empty = virtqueue_min_capacity(vi->svq) < MAX_SKB_FRAGS + 2;
+
+   do {
+      if (!free_one_old_xmit_skb(vi)) {
+         /* No more skbs to free up */
          break;
-      pr_debug("Sent skb %p\n", skb);
-      vi->dev->stats.tx_bytes += skb->len;
-      vi->dev->stats.tx_packets++;
-      dev_kfree_skb_any(skb);
-   }
-   return r;
+      }
+
+      if (empty) {
+         /* Check again if there is enough space */
+         empty = virtqueue_min_capacity(vi->svq) <
+            MAX_SKB_FRAGS + 2;
+      } else {

Review

Quoted 6 lines

(quoted diff) +5 -0

+         --to_free;
+      }
+   } while (to_free > 0);
+
+   return !empty;
 }

Why bother doing the capacity check in this function?

Review

Quoted 8 lines

(quoted diff) +7 -1

-   if (unlikely(ret < 0)) {
+   if (unlikely(capacity < 0)) {
+      /*
+       * Failure to queue should be impossible. The only way to
+       * reach here is if we got a cb before 3/4th of space was
+       * available. We could stop the queue and re-enable
+       * callbacks (and possibly return TX_BUSY), but we don't
+       * bother since this is impossible.

It's far from impossible. The 3/4 thing is only a hint, and old devices don't support it anyway.

Review

Quoted 10 lines

(quoted diff) +3 -7

-   if (!likely(free_old_xmit_skbs(vi, 2))) {
-      netif_stop_queue(dev);
-      if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
-         /* More just got used, free them and recheck. */
-         if (!likely(free_old_xmit_skbs(vi, 0))) {
-            netif_start_queue(dev);
-            virtqueue_disable_cb(vi->svq);
+   /*
+    * Apparently nice girls don't return TX_BUSY; check capacity and
+    * stop the queue before it gets out of hand. Naturally, this

Review

Quoted 12 lines

(quoted diff) +12 -0

+    * entries.
+    */
+   if (capacity < 2+MAX_SKB_FRAGS) {
+      /*
+       * We don't have enough space for the next packet. Try
+       * freeing more.
+       */
+      if (likely(!free_old_xmit_skbs(vi, UINT_MAX))) {
+         netif_stop_queue(dev);
+         if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
+            /* More just got used, free them and recheck. */
+            if (likely(free_old_xmit_skbs(vi, UINT_MAX))) {

Is this where the bug was?

Quoted 4 lines

OK, I have something very similar, but I still dislike the screw the latency part: this path is exactly what the IBM guys seem to hit. So I created two functions: one tries to free a constant number and another one up to capacity. I'll post that now.

Please review this patch to see if it looks reasonable:

1. Picked comments/code from MST's code and Rusty's review.
2. virtqueue_min_capacity() needs to be called only if it returned empty the last time it was called.
3. Fix return value bug in free_old_xmit_skbs (hangs guest).
4. Stop queue only if capacity is not enough for next xmit.

5. Fix/clean some likely/unlikely checks (hopefully).

I have done some minimal netperf tests with this.

With this patch, add_buf returning capacity seems to be useful - it allows less virtio API calls.

Signed-off-by Krishna Kumar <[email protected]>

---
 drivers/net/virtio_net.c |  105 ++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 41 deletions(-)

diff -ruNp org/drivers/net/virtio_net.c new/drivers/net/virtio_net.c

drivers/net/virtio_net.c +23 -16

@@ -509,27 +509,43 @@ again:
 	return received;
 }
 
-/* Check capacity and try to free enough pending old buffers to enable queueing
- * new ones.  If min_skbs > 0, try to free at least the specified number of skbs
- * even if the ring already has sufficient capacity.  Return true if we can
- * guarantee that a following virtqueue_add_buf will succeed. */
-static bool free_old_xmit_skbs(struct virtnet_info *vi, int min_skbs)
+/* Return true if freed a skb, else false */
+static inline bool free_one_old_xmit_skb(struct virtnet_info *vi)
 {
 	struct sk_buff *skb;
 	unsigned int len;
-	bool r;
 
-	while ((r = virtqueue_min_capacity(vi->svq) < MAX_SKB_FRAGS + 2) ||
-	       min_skbs-- > 0) {
-		skb = virtqueue_get_buf(vi->svq, &len);
-		if (unlikely(!skb))
+	skb = virtqueue_get_buf(vi->svq, &len);
+	if (unlikely(!skb))
+		return 0;
+
+	pr_debug("Sent skb %p\n", skb);
+	vi->dev->stats.tx_bytes += skb->len;
+	vi->dev->stats.tx_packets++;
+	dev_kfree_skb_any(skb);
+	return 1;
+}
+
+static bool free_old_xmit_skbs(struct virtnet_info *vi, int to_free)
+{
+	bool empty = virtqueue_min_capacity(vi->svq) < MAX_SKB_FRAGS + 2;
+
+	do {
+		if (!free_one_old_xmit_skb(vi)) {
+			/* No more skbs to free up */
 			break;
-		pr_debug("Sent skb %p\n", skb);
-		vi->dev->stats.tx_bytes += skb->len;
-		vi->dev->stats.tx_packets++;
-		dev_kfree_skb_any(skb);
-	}
-	return r;
+		}
+
+		if (empty) {

drivers/net/virtio_net.c +4 -9

 static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)

@@ -582,46 +598,53 @@ static int xmit_skb(struct virtnet_info 
 static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
-	int ret, n;
+	int capacity;
 
-	/* Free up space in the ring in case this is the first time we get
-	 * woken up after ring full condition.  Note: this might try to free
-	 * more than strictly necessary if the skb has a small
-	 * number of fragments, but keep it simple. */
-	free_old_xmit_skbs(vi, 0);
+	/* Try to free 2 buffers for every 1 xmit, to stay ahead. */
+	free_old_xmit_skbs(vi, 2);
 
 	/* Try to transmit */
-	ret = xmit_skb(vi, skb);
+	capacity = xmit_skb(vi, skb);
 
-	/* Failure to queue is unlikely. It's not a bug though: it might happen
-	 * if we get an interrupt while the queue is still mostly full.

drivers/net/virtio_net.c +8 -13

+		 */
 		if (net_ratelimit())
-			dev_info(&dev->dev, "TX queue failure: %d\n", ret);
+			dev_info(&dev->dev, "TX queue failure: %d\n", capacity);
 		dev->stats.tx_dropped++;
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}
+
 	virtqueue_kick(vi->svq);
 
 	/* Don't wait up for transmitted skbs to be freed. */
 	skb_orphan(skb);
 	nf_reset(skb);
 
-	/* Apparently nice girls don't return TX_BUSY; check capacity and stop
-	 * the queue before it gets out of hand.
-	 * Naturally, this wastes entries. */
-	/* We transmit one skb, so try to free at least two pending skbs.
-	 * This is so that we don't hog the skb memory unnecessarily. */
-	if (!likely(free_old_xmit_skbs(vi, 2))) {
-		netif_stop_queue(dev);
-		if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
-			/* More just got used, free them and recheck. */
-			if (!likely(free_old_xmit_skbs(vi, 0))) {
-				netif_start_queue(dev);
-				virtqueue_disable_cb(vi->svq);
+	/*
+	 * Apparently nice girls don't return TX_BUSY; check capacity and
+	 * stop the queue before it gets out of hand. Naturally, this wastes
+	 * entries. 
+	 */

drivers/net/virtio_net.c +3 -0

+					netif_start_queue(dev);
+					virtqueue_disable_cb(vi->svq);
+				}
 			}
 		}
 	}

OK, I have something very similar, but I still dislike the screw the latency part: this path is exactly what the IBM guys seem to hit. So I created two functions: one tries to free a constant number and another one up to capacity. I'll post that now.

Quoted 7 lines · Michael S. Tsirkin

Current code might introduce a lot of latency variation if there are many pending bufs at the time we attempt to transmit a new one. This is bad for real-time applications and can't be good for TCP either.

Free up just enough to both clean up all buffers eventually and to be able to xmit the next packet.

OK, I found this quite confusing to read.

drivers/net/virtio_net.c again: · line 533

Quoted 13 lines

drivers/net/virtio_net.c +6 -1

-	while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
+	while ((r = virtqueue_min_capacity(vi->svq) < MAX_SKB_FRAGS + 2) ||
+	       min_skbs-- > 0) {
+		skb = virtqueue_get_buf(vi->svq, &len);
+		if (unlikely(!skb))
+			break;
 		pr_debug("Sent skb %p\n", skb);
 		vi->dev->stats.tx_bytes += skb->len;
 		vi->dev->stats.tx_packets++;
 		dev_kfree_skb_any(skb);
 	}
+	return r;
 }

Gah... what a horrible loop.

Basically, this patch makes hard-to-read code worse, and we should try to make it better.

Currently, xmit *can* fail when an xmit interrupt wakes the queue, but the packet(s) xmitted didn't free up enough space for the new packet. With indirect buffers this only happens if we hit OOM (and thus go to direct buffers).

We could solve this by only waking the queue in skb_xmit_done if the capacity is >= 2 + MAX_SKB_FRAGS. But can we do it without a race?

If not, then I'd really prefer to see this, because I think it's clearer:

        // Try to free 2 buffers for every 1 xmit, to stay ahead.
        free_old_buffers(2)

        if (!add_buf()) {
                // Screw latency, free them all.
                free_old_buffers(UINT_MAX)
                // OK, this can happen if we are using direct buffers,
                // and the xmit interrupt woke us but the packets
                // xmitted were smaller than this one.  Rare though.
                if (!add_buf())
                        Whinge and stop queue, maybe loop.
        }

        if (capacity < 2 + MAX_SKB_FRAGS) {
                // We don't have enough for the next packet?  Try
                // freeing more.
                free_old_buffers(UINT_MAX);
                if (capacity < 2 + MAX_SKB_FRAGS) {
                        Stop queue, maybe loop.
        }

The current code makes my head hurt :(

Thoughts?
Rusty.

Current code might introduce a lot of latency variation if there are many pending bufs at the time we attempt to transmit a new one. This is bad for real-time applications and can't be good for TCP either.

Free up just enough to both clean up all buffers eventually and to be able to xmit the next packet.