On Fri, Oct 29, 2010 at 09:58:40PM +1030, Rusty Russell wrote:

On Fri, 29 Oct 2010 09:25:09 pm Krishna Kumar2 wrote:

quoted

Rusty Russell [off-list ref] wrote on 10/29/2010 03:17:24 PM:

quoted

quoted

Oct 17 10:22:40 localhost kernel: net eth0: Unexpected TX queue

failure: -28

quoted

quoted

Oct 17 10:28:22 localhost kernel: net eth0: Unexpected TX queue

failure: -28

quoted

quoted

Oct 17 10:35:58 localhost kernel: net eth0: Unexpected TX queue

failure: -28

quoted

quoted

Oct 17 10:41:06 localhost kernel: net eth0: Unexpected TX queue

failure: -28

quoted

quoted

I initially changed the check from -ENOMEM to -ENOSPC, but
virtqueue_add_buf can return only -ENOSPC when it doesn't have
space for new request.  Patch removes redundant checks but
displays the failure errno.

Signed-off-by: Krishna Kumar <redacted>
---
 drivers/net/virtio_net.c |   15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

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

--- org/drivers/net/virtio_net.c   2010-10-11 10:20:02.000000000 +0530
+++ new/drivers/net/virtio_net.c   2010-10-21 17:37:45.000000000 +0530

@@ -570,17 +570,10 @@ static netdev_tx_t start_xmit(struct sk_

    /* This can happen with OOM and indirect buffers. */
    if (unlikely(capacity < 0)) {
-      if (net_ratelimit()) {
-         if (likely(capacity == -ENOMEM)) {
-            dev_warn(&dev->dev,
-                "TX queue failure: out of memory\n");
-         } else {
-            dev->stats.tx_fifo_errors++;
-            dev_warn(&dev->dev,
-                "Unexpected TX queue failure: %d\n",
-                capacity);
-         }
-      }
+      if (net_ratelimit())
+         dev_warn(&dev->dev,
+             "TX queue failure (%d): out of memory\n",
+             capacity);

Hold on... you were getting -ENOSPC, which shouldn't happen.  What makes

you

quoted

think it's out of memory?

virtqueue_add_buf_gfp returns only -ENOSPC on failure, whether
direct or indirect descriptors are used, so isn't -ENOSPC
"expected"? (vring_add_indirect returns -ENOMEM on memory
failure, but that is masked out and we go direct which is
the failure point).

Ah, OK, gotchya.
I'm not even sure the fallback to linear makes sense; if we're failing
kmallocs we should probably just return -ENOMEM.  Would mean we can
tell the difference between "out of space" (which should never happen
since we stop the queue when we have < 2+MAX_SKB_FRAGS slots left)
and this case.

Michael, what do you think?

Thanks,
Rusty.

Let's make sure I understand the issue: we use indirect buffers
so we assume there's still a lot of place in the ring, then
allocation for the indirect fails and so we return -ENOSPC?

So first, I agree it's a bug.  But I am not sure killing the fallback
is such a good idea: recovering from add buf failure is hard
generally, we should try to accomodate if we can. Let's just fix
the return code for now?

And generally, we should be smarter: as long as the ring is almost
empty, and s/g list is short, it is a waste to use indirect buffers.
BTW we have had a FIXME there for a long while, I think Yan suggested
increasing that threshold to 3. Yan?

Further, maybe preallocating some memory for the indirect buffers might
be a good idea.

In short, lots of good ideas, let's start with the minimal patch that is
a good 2.6.37 candidate too. How about the following (untested)?

virtio: fix add_buf return code for OOM

add_buff returned ENOSPC on out of memory: this is a bug
as at leats virtio-net expects ENOMEM and handles it
specially. Fix that.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 1475ed6..0a89098 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c

@@ -165,7 +165,7 @@ int virtqueue_add_buf_gfp(struct virtqueue *_vq,
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 	unsigned int i, avail, uninitialized_var(prev);
-	int head;
+	int head = -ENOSPC;
 
 	START_USE(vq);
 

@@ -191,7 +191,7 @@ int virtqueue_add_buf_gfp(struct virtqueue *_vq,
 		if (out)
 			vq->notify(&vq->vq);
 		END_USE(vq);
-		return -ENOSPC;
+		return head;
 	}
 
 	/* We're about to use some buffers from the free list. */

-- 
MST