[PATCH 1/2] virtio: Add a can_add_buf helper

[PATCH 1/2] virtio: Add a can_add_buf helper

[Amit Shah]

This helper returns 1 if a call to add_buf will not fail
with -ENOSPC.

This will help callers that do

while(1) {
	alloc()
	if (add_buf()) {
		free();
		break;
	}
}

This will result in one less alloc/free exercise.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_ring.c |    8 ++++++++
 include/linux/virtio.h       |    5 +++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a882f26..e2e5eb7 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -137,6 +137,13 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
 	return head;
 }
 
+static bool vring_can_add_buf(struct virtqueue *_vq)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+
+	return vq->num_free ? 1 : 0;
+}
+
 static int vring_add_buf(struct virtqueue *_vq,
 			 struct scatterlist sg[],
 			 unsigned int out,
@@ -350,6 +357,7 @@ EXPORT_SYMBOL_GPL(vring_interrupt);
 static struct virtqueue_ops vring_vq_ops = {
 	.add_buf = vring_add_buf,
 	.get_buf = vring_get_buf,
+	.can_add_buf = vring_can_add_buf,
 	.kick = vring_kick,
 	.disable_cb = vring_disable_cb,
 	.enable_cb = vring_enable_cb,
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 4fca4f5..cc0e3aa 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -35,6 +35,9 @@ struct virtqueue {
  *	in_num: the number of sg which are writable (after readable ones)
  *	data: the token identifying the buffer.
  *      Returns 0 or an error.
+ * @can_add_buf: indicates whether we have a free slot to add a buffer
+ *      vq: the struct virtqueue we're talking about.
+ *      Returns 0 or 1.
  * @kick: update after add_buf
  *	vq: the struct virtqueue
  *	After one or more add_buf calls, invoke this to kick the other side.
@@ -65,6 +68,8 @@ struct virtqueue_ops {
 		       unsigned int in_num,
 		       void *data);
 
+	bool (*can_add_buf)(struct virtqueue *vq);
+
 	void (*kick)(struct virtqueue *vq);
 
 	void *(*get_buf)(struct virtqueue *vq, unsigned int *len);
-- 
1.6.2.5

[PATCH 2/2] virtio_net: Use can_add_buf to test for enough room to add

[Amit Shah]

Use the can_add_buf virtqueue operation to test if
there's room to add another buf to the queue.

Saves us one cycle of alloc-add-free if the queue
was full.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/net/virtio_net.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2a6e81d..fd1d628 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -280,7 +280,7 @@ static void try_fill_recv_maxbufs(struct virtnet_info *vi)
 	int num, err, i;
 
 	sg_init_table(sg, 2+MAX_SKB_FRAGS);
-	for (;;) {
+	while (vi->rvq->vq_ops->can_add_buf(vi->rvq)) {
 		struct virtio_net_hdr *hdr;
 
 		skb = netdev_alloc_skb(vi->dev, MAX_PACKET_LEN + NET_IP_ALIGN);
-- 
1.6.2.5

Re: [PATCH 1/2] virtio: Add a can_add_buf helper

[Rusty Russell]

On Wed, 19 Aug 2009 01:31:29 am Amit Shah wrote:
> This helper returns 1 if a call to add_buf will not fail
> with -ENOSPC.
> 
> This will help callers that do
> 
> while(1) {
> 	alloc()
> 	if (add_buf()) {
> 		free();
> 		break;
> 	}
> }
> 
> This will result in one less alloc/free exercise.

Hi Amit,

    This is what I have in my tree; similar motivation.  I didn't
adapt virtio_net like you did tho.

virtio: make add_buf return capacity remaining

This API change means that virtio_net can tell how much capacity
remains for buffers.  It's necessarily fuzzy, since
VIRTIO_RING_F_INDIRECT_DESC means we can fit any number of descriptors
in one, *if* we can kmalloc.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: Dinesh Subhraveti <dineshs@us.ibm.com>
---
 drivers/block/virtio_blk.c          |    2 +-
 drivers/char/hw_random/virtio-rng.c |    2 +-
 drivers/char/virtio_console.c       |    4 ++--
 drivers/net/virtio_net.c            |    8 ++++----
 drivers/virtio/virtio_balloon.c     |    2 +-
 drivers/virtio/virtio_ring.c        |    6 +++++-
 include/linux/virtio.h              |    2 +-
 net/9p/trans_virtio.c               |    2 +-
 8 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -139,7 +139,7 @@ static bool do_req(struct request_queue 
 		}
 	}
 
-	if (vblk->vq->vq_ops->add_buf(vblk->vq, vblk->sg, out, in, vbr)) {
+	if (vblk->vq->vq_ops->add_buf(vblk->vq, vblk->sg, out, in, vbr) < 0) {
 		mempool_free(vbr, vblk->pool);
 		return false;
 	}
diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -51,7 +51,7 @@ static void register_buffer(void)
 
 	sg_init_one(&sg, random_data+data_left, RANDOM_DATA_SIZE-data_left);
 	/* There should always be room for one buffer. */
-	if (vq->vq_ops->add_buf(vq, &sg, 0, 1, random_data) != 0)
+	if (vq->vq_ops->add_buf(vq, &sg, 0, 1, random_data) < 0)
 		BUG();
 	vq->vq_ops->kick(vq);
 }
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -65,7 +65,7 @@ static int put_chars(u32 vtermno, const 
 
 	/* add_buf wants a token to identify this buffer: we hand it any
 	 * non-NULL pointer, since there's only ever one buffer. */
-	if (out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, (void *)1) == 0) {
+	if (out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, (void *)1) >= 0) {
 		/* Tell Host to go! */
 		out_vq->vq_ops->kick(out_vq);
 		/* Chill out until it's done with the buffer. */
@@ -85,7 +85,7 @@ static void add_inbuf(void)
 	sg_init_one(sg, inbuf, PAGE_SIZE);
 
 	/* We should always be able to add one buffer to an empty queue. */
-	if (in_vq->vq_ops->add_buf(in_vq, sg, 0, 1, inbuf) != 0)
+	if (in_vq->vq_ops->add_buf(in_vq, sg, 0, 1, inbuf) < 0)
 		BUG();
 	in_vq->vq_ops->kick(in_vq);
 }
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
@@ -300,7 +300,7 @@ static void try_fill_recv_maxbufs(struct
 		skb_queue_head(&vi->recv, skb);
 
 		err = vi->rvq->vq_ops->add_buf(vi->rvq, sg, 0, num, skb);
-		if (err) {
+		if (err < 0) {
 			skb_unlink(skb, &vi->recv);
 			trim_pages(vi, skb);
 			kfree_skb(skb);
@@ -349,7 +349,7 @@ static void try_fill_recv(struct virtnet
 		skb_queue_head(&vi->recv, skb);
 
 		err = vi->rvq->vq_ops->add_buf(vi->rvq, sg, 0, 1, skb);
-		if (err) {
+		if (err < 0) {
 			skb_unlink(skb, &vi->recv);
 			kfree_skb(skb);
 			break;
@@ -480,7 +480,7 @@ again:
 
 	/* Put new one in send queue and do transmit */
 	__skb_queue_head(&vi->send, skb);
-	if (likely(xmit_skb(vi, skb) == 0)) {
+	if (likely(xmit_skb(vi, skb) >= 0)) {
 		vi->svq->vq_ops->kick(vi->svq);
 		/* Don't wait up for transmitted skbs to be freed. */
 		skb_orphan(skb);
@@ -577,7 +577,7 @@ static bool virtnet_send_command(struct 
 		sg_set_buf(&sg[i + 1], sg_virt(s), s->length);
 	sg_set_buf(&sg[out + in - 1], &status, sizeof(status));
 
-	BUG_ON(vi->cvq->vq_ops->add_buf(vi->cvq, sg, out, in, vi));
+	BUG_ON(vi->cvq->vq_ops->add_buf(vi->cvq, sg, out, in, vi) < 0);
 
 	vi->cvq->vq_ops->kick(vi->cvq);
 
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -84,7 +84,7 @@ static void tell_host(struct virtio_ball
 	init_completion(&vb->acked);
 
 	/* We should always be able to add one buffer to an empty queue. */
-	if (vq->vq_ops->add_buf(vq, &sg, 1, 0, vb) != 0)
+	if (vq->vq_ops->add_buf(vq, &sg, 1, 0, vb) < 0)
 		BUG();
 	vq->vq_ops->kick(vq);
 
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -213,7 +213,11 @@ add_head:
 
 	pr_debug("Added buffer head %i to %p\n", head, vq);
 	END_USE(vq);
-	return 0;
+
+	/* If we're indirect, we can fit many (assuming not OOM). */
+	if (vq->indirect)
+		return vq->num_free ? vq->vring.num : 0;
+	return vq->num_free;
 }
 
 static void vring_kick(struct virtqueue *_vq)
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -34,7 +34,7 @@ struct virtqueue {
  *	out_num: the number of sg readable by other side
  *	in_num: the number of sg which are writable (after readable ones)
  *	data: the token identifying the buffer.
- *      Returns 0 or an error.
+ *      Returns remaining capacity of queue (sg segments) or a negative error.
  * @kick: update after add_buf
  *	vq: the struct virtqueue
  *	After one or more add_buf calls, invoke this to kick the other side.
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -203,7 +203,7 @@ p9_virtio_request(struct p9_client *clie
 
 	req->status = REQ_STATUS_SENT;
 
-	if (chan->vq->vq_ops->add_buf(chan->vq, chan->sg, out, in, req->tc)) {
+	if (chan->vq->vq_ops->add_buf(chan->vq, chan->sg, out, in, req->tc) < 0) {
 		P9_DPRINTK(P9_DEBUG_TRANS,
 			"9p debug: virtio rpc add_buf returned failure");
 		return -EIO;

Re: [PATCH 1/2] virtio: Add a can_add_buf helper

[Rusty Russell]

On Mon, 24 Aug 2009 05:09:44 pm Amit Shah wrote:
>  	sg_init_table(sg, 2+MAX_SKB_FRAGS);
> -	for (;;) {
> +	ret = 1;
> +	while(ret > 0) { /* Is there space to add another buffer to the vq */

This is called a "do" loop :)

Please test, and I'll apply.

Cheers,
Rusty.

Re: [PATCH 1/2] virtio: Add a can_add_buf helper

[Amit Shah]

On (Tue) Aug 25 2009 [23:57:54], Rusty Russell wrote:
> On Mon, 24 Aug 2009 05:09:44 pm Amit Shah wrote:
> >  	sg_init_table(sg, 2+MAX_SKB_FRAGS);
> > -	for (;;) {
> > +	ret = 1;
> > +	while(ret > 0) { /* Is there space to add another buffer to the vq */
> 
> This is called a "do" loop :)
> 
> Please test, and I'll apply.

:-) drat! shouldn't have done that on a Sunday anyway

I'll test it soon.

		Amit