[PATCH 1/5] virtio: move queue_index and num_free fields into core struct virtqueue.

[PATCH 1/5] virtio: move queue_index and num_free fields into core struct virtqueue.

[Rusty Russell]

They're generic concepts, so hoist them.  This also avoids accessor
functions.

This goes even further than Jason Wang's 17bb6d4088 patch
("virtio-ring: move queue_index to vring_virtqueue") which moved the
queue_index from the specific transport.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/virtio/virtio_mmio.c |    4 ++--
 drivers/virtio/virtio_pci.c  |    6 ++----
 drivers/virtio/virtio_ring.c |   34 +++++++++++-----------------------
 include/linux/virtio.h       |    6 ++++--
 4 files changed, 19 insertions(+), 31 deletions(-)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 6b1b7e1..5a0e1d3 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -225,7 +225,7 @@ static void vm_notify(struct virtqueue *vq)
 
 	/* We write the queue's selector into the notification register to
 	 * signal the other end */
-	writel(virtqueue_get_queue_index(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
+	writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
 }
 
 /* Notify all virtqueues on an interrupt. */
@@ -266,7 +266,7 @@ static void vm_del_vq(struct virtqueue *vq)
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
 	struct virtio_mmio_vq_info *info = vq->priv;
 	unsigned long flags, size;
-	unsigned int index = virtqueue_get_queue_index(vq);
+	unsigned int index = vq->index;
 
 	spin_lock_irqsave(&vm_dev->lock, flags);
 	list_del(&info->node);
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index b59237c..e3ecc94 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -203,8 +203,7 @@ static void vp_notify(struct virtqueue *vq)
 
 	/* we write the queue's selector into the notification register to
 	 * signal the other end */
-	iowrite16(virtqueue_get_queue_index(vq),
-		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
+	iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
 }
 
 /* Handle a configuration change: Tell driver if it wants to know. */
@@ -479,8 +478,7 @@ static void vp_del_vq(struct virtqueue *vq)
 	list_del(&info->node);
 	spin_unlock_irqrestore(&vp_dev->lock, flags);
 
-	iowrite16(virtqueue_get_queue_index(vq),
-		vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
+	iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
 
 	if (vp_dev->msix_enabled) {
 		iowrite16(VIRTIO_MSI_NO_VECTOR,
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index e639584..335dcec 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -93,8 +93,6 @@ struct vring_virtqueue
 	/* Host publishes avail event idx */
 	bool event;
 
-	/* Number of free buffers */
-	unsigned int num_free;
 	/* Head of free buffer list. */
 	unsigned int free_head;
 	/* Number we've added since last sync. */
@@ -106,9 +104,6 @@ struct vring_virtqueue
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	void (*notify)(struct virtqueue *vq);
 
-	/* Index of the queue */
-	int queue_index;
-
 #ifdef DEBUG
 	/* They're supposed to lock for us. */
 	unsigned int in_use;
@@ -160,7 +155,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
 	desc[i-1].next = 0;
 
 	/* We're about to use a buffer */
-	vq->num_free--;
+	vq->vq.num_free--;
 
 	/* Use a single buffer which doesn't continue */
 	head = vq->free_head;
@@ -174,13 +169,6 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
 	return head;
 }
 
-int virtqueue_get_queue_index(struct virtqueue *_vq)
-{
-	struct vring_virtqueue *vq = to_vvq(_vq);
-	return vq->queue_index;
-}
-EXPORT_SYMBOL_GPL(virtqueue_get_queue_index);
-
 /**
  * virtqueue_add_buf - expose buffer to other end
  * @vq: the struct virtqueue we're talking about.
@@ -228,7 +216,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
 
 	/* If the host supports indirect descriptor tables, and we have multiple
 	 * buffers, then go indirect. FIXME: tune this threshold */
-	if (vq->indirect && (out + in) > 1 && vq->num_free) {
+	if (vq->indirect && (out + in) > 1 && vq->vq.num_free) {
 		head = vring_add_indirect(vq, sg, out, in, gfp);
 		if (likely(head >= 0))
 			goto add_head;
@@ -237,9 +225,9 @@ int virtqueue_add_buf(struct virtqueue *_vq,
 	BUG_ON(out + in > vq->vring.num);
 	BUG_ON(out + in == 0);
 
-	if (vq->num_free < out + in) {
+	if (vq->vq.num_free < out + in) {
 		pr_debug("Can't add buf len %i - avail = %i\n",
-			 out + in, vq->num_free);
+			 out + in, vq->vq.num_free);
 		/* FIXME: for historical reasons, we force a notify here if
 		 * there are outgoing parts to the buffer.  Presumably the
 		 * host should service the ring ASAP. */
@@ -250,7 +238,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
 	}
 
 	/* We're about to use some buffers from the free list. */
-	vq->num_free -= out + in;
+	vq->vq.num_free -= out + in;
 
 	head = vq->free_head;
 	for (i = vq->free_head; out; i = vq->vring.desc[i].next, out--) {
@@ -296,7 +284,7 @@ add_head:
 	pr_debug("Added buffer head %i to %p\n", head, vq);
 	END_USE(vq);
 
-	return vq->num_free;
+	return vq->vq.num_free;
 }
 EXPORT_SYMBOL_GPL(virtqueue_add_buf);
 
@@ -393,13 +381,13 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
 
 	while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) {
 		i = vq->vring.desc[i].next;
-		vq->num_free++;
+		vq->vq.num_free++;
 	}
 
 	vq->vring.desc[i].next = vq->free_head;
 	vq->free_head = head;
 	/* Plus final descriptor */
-	vq->num_free++;
+	vq->vq.num_free++;
 }
 
 static inline bool more_used(const struct vring_virtqueue *vq)
@@ -599,7 +587,7 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
 		return buf;
 	}
 	/* That should have freed everything. */
-	BUG_ON(vq->num_free != vq->vring.num);
+	BUG_ON(vq->vq.num_free != vq->vring.num);
 
 	END_USE(vq);
 	return NULL;
@@ -653,12 +641,13 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
 	vq->vq.callback = callback;
 	vq->vq.vdev = vdev;
 	vq->vq.name = name;
+	vq->vq.num_free = num;
+	vq->vq.index = index;
 	vq->notify = notify;
 	vq->weak_barriers = weak_barriers;
 	vq->broken = false;
 	vq->last_used_idx = 0;
 	vq->num_added = 0;
-	vq->queue_index = index;
 	list_add_tail(&vq->vq.list, &vdev->vqs);
 #ifdef DEBUG
 	vq->in_use = false;
@@ -673,7 +662,6 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
 		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
 
 	/* Put everything in free lists. */
-	vq->num_free = num;
 	vq->free_head = 0;
 	for (i = 0; i < num-1; i++) {
 		vq->vring.desc[i].next = i+1;
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 533b115..ed4c437 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -16,12 +16,16 @@
  * @name: the name of this virtqueue (mainly for debugging)
  * @vdev: the virtio device this queue was created for.
  * @priv: a pointer for the virtqueue implementation to use.
+ * @index: the zero-based ordinal number for this queue.
+ * @num_free: number of buffers we expect to be able to fit.
  */
 struct virtqueue {
 	struct list_head list;
 	void (*callback)(struct virtqueue *vq);
 	const char *name;
 	struct virtio_device *vdev;
+	unsigned int index;
+	unsigned int num_free;
 	void *priv;
 };
 
@@ -50,8 +54,6 @@ void *virtqueue_detach_unused_buf(struct virtqueue *vq);
 
 unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
 
-int virtqueue_get_queue_index(struct virtqueue *vq);
-
 /**
  * virtio_device - representation of a device using virtio
  * @index: unique position on the virtio bus
-- 
1.7.9.5

[PATCH 2/5] virtio-net: remove unused skb_vnet_hdr->num_sg field

[Rusty Russell]

From: "Michael S. Tsirkin" <mst@redhat.com>

[Split from "correct capacity math on ring full" -- Rusty]

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/virtio_net.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index cbf8b06..20880fb 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -95,7 +95,6 @@ struct skb_vnet_hdr {
 		struct virtio_net_hdr hdr;
 		struct virtio_net_hdr_mrg_rxbuf mhdr;
 	};
-	unsigned int num_sg;
 };
 
 struct padded_vnet_hdr {
@@ -581,6 +580,7 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
 {
 	struct skb_vnet_hdr *hdr = skb_vnet_hdr(skb);
 	const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
+	unsigned num_sg;
 
 	pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
 
@@ -619,8 +619,8 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
 	else
 		sg_set_buf(vi->tx_sg, &hdr->hdr, sizeof hdr->hdr);
 
-	hdr->num_sg = skb_to_sgvec(skb, vi->tx_sg + 1, 0, skb->len) + 1;
-	return virtqueue_add_buf(vi->svq, vi->tx_sg, hdr->num_sg,
+	num_sg = skb_to_sgvec(skb, vi->tx_sg + 1, 0, skb->len) + 1;
+	return virtqueue_add_buf(vi->svq, vi->tx_sg, num_sg,
 				 0, skb, GFP_ATOMIC);
 }
 
-- 
1.7.9.5

[PATCH 3/5] virtio-net: correct capacity math on ring full

[Rusty Russell]

From: "Michael S. Tsirkin" <mst@redhat.com>

Capacity math on ring full is wrong: we are
looking at num_sg but that might be optimistic
because of indirect buffer use.

The implementation also penalizes fast path
with extra memory accesses for the benefit of
ring full condition handling which is slow path.

It's easy to query ring capacity so let's do just that.

This change also makes it easier to move vnet header
for tx around as follow-up patch does.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/virtio_net.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 20880fb..6c094c8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -556,10 +556,10 @@ again:
 	return received;
 }
 
-static unsigned int free_old_xmit_skbs(struct virtnet_info *vi)
+static void free_old_xmit_skbs(struct virtnet_info *vi)
 {
 	struct sk_buff *skb;
-	unsigned int len, tot_sgs = 0;
+	unsigned int len;
 	struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
 
 	while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
@@ -570,10 +570,8 @@ static unsigned int free_old_xmit_skbs(struct virtnet_info *vi)
 		stats->tx_packets++;
 		u64_stats_update_end(&stats->tx_syncp);
 
-		tot_sgs += skb_vnet_hdr(skb)->num_sg;
 		dev_kfree_skb_any(skb);
 	}
-	return tot_sgs;
 }
 
 static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
@@ -664,7 +662,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 		netif_stop_queue(dev);
 		if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
 			/* More just got used, free them then recheck. */
-			capacity += free_old_xmit_skbs(vi);
+			free_old_xmit_skbs(vi);
+			capacity = vi->svq->num_free;
 			if (capacity >= 2+MAX_SKB_FRAGS) {
 				netif_start_queue(dev);
 				virtqueue_disable_cb(vi->svq);
-- 
1.7.9.5

[PATCH 4/5] virtio_net: don't rely on virtqueue_add_buf() returning capacity.

[Rusty Russell]

Now we can easily use vq->num_free to determine if there are descriptors
left in the queue, we're about to change virtqueue_add_buf() to return 0
on success.  The virtio_net driver is the only one which actually uses
the return value, so change that.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/virtio_net.c |   33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6c094c8..7c7f5a9 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -471,10 +471,11 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
 			err = add_recvbuf_small(vi, gfp);
 
 		oom = err == -ENOMEM;
-		if (err < 0)
+		if (err)
 			break;
 		++vi->num;
-	} while (err > 0);
+	} while (vi->rvq->num_free);
+
 	if (unlikely(vi->num > vi->max))
 		vi->max = vi->num;
 	virtqueue_kick(vi->rvq);
@@ -625,27 +626,20 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
 static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
-	int capacity;
+	int err;
 
 	/* Free up any pending old buffers before queueing new ones. */
 	free_old_xmit_skbs(vi);
 
 	/* Try to transmit */
-	capacity = xmit_skb(vi, skb);
+	err = xmit_skb(vi, skb);
 
-	/* This can happen with OOM and indirect buffers. */
-	if (unlikely(capacity < 0)) {
-		if (likely(capacity == -ENOMEM)) {
-			if (net_ratelimit())
-				dev_warn(&dev->dev,
-					 "TX queue failure: out of memory\n");
-		} else {
-			dev->stats.tx_fifo_errors++;
-			if (net_ratelimit())
-				dev_warn(&dev->dev,
-					 "Unexpected TX queue failure: %d\n",
-					 capacity);
-		}
+	/* This should not happen! */
+	if (unlikely(err < 0)) {
+		dev->stats.tx_fifo_errors++;
+		if (net_ratelimit())
+			dev_warn(&dev->dev,
+				 "Unexpected TX queue failure: %d\n", err);
 		dev->stats.tx_dropped++;
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
@@ -658,13 +652,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* Apparently nice girls don't return TX_BUSY; stop the queue
 	 * before it gets out of hand.  Naturally, this wastes entries. */
-	if (capacity < 2+MAX_SKB_FRAGS) {
+	if (vi->svq->num_free < 2+MAX_SKB_FRAGS) {
 		netif_stop_queue(dev);
 		if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
 			/* More just got used, free them then recheck. */
 			free_old_xmit_skbs(vi);
-			capacity = vi->svq->num_free;
-			if (capacity >= 2+MAX_SKB_FRAGS) {
+			if (vi->svq->num_free >= 2+MAX_SKB_FRAGS) {
 				netif_start_queue(dev);
 				virtqueue_disable_cb(vi->svq);
 			}
-- 
1.7.9.5

[PATCH 5/5] virtio: make virtqueue_add_buf() returning 0 on success, not capacity.

[Rusty Russell]

Now noone relies on this behavior, we simplify virtqueue_add_buf() so it
return 0 or -errno.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/virtio/virtio_ring.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 335dcec..5fde312 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -181,10 +181,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
  * Caller must ensure we don't call this with other virtqueue operations
  * at the same time (except where noted).
  *
- * Returns remaining capacity of queue or a negative error
- * (ie. ENOSPC).  Note that it only really makes sense to treat all
- * positive return values as "available": indirect buffers mean that
- * we can put an entire sg[] array inside a single queue entry.
+ * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
  */
 int virtqueue_add_buf(struct virtqueue *_vq,
 		      struct scatterlist sg[],
@@ -284,7 +281,7 @@ add_head:
 	pr_debug("Added buffer head %i to %p\n", head, vq);
 	END_USE(vq);
 
-	return vq->vq.num_free;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(virtqueue_add_buf);
 
-- 
1.7.9.5

Re: [PATCH 4/5] virtio_net: don't rely on virtqueue_add_buf() returning capacity.

[Michael S. Tsirkin]

On Wed, Oct 17, 2012 at 12:00:51AM +1030, Rusty Russell wrote:
> Now we can easily use vq->num_free to determine if there are descriptors
> left in the queue, we're about to change virtqueue_add_buf() to return 0
> on success.  The virtio_net driver is the only one which actually uses
> the return value, so change that.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/net/virtio_net.c |   33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6c094c8..7c7f5a9 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -471,10 +471,11 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
>  			err = add_recvbuf_small(vi, gfp);
>  
>  		oom = err == -ENOMEM;
> -		if (err < 0)
> +		if (err)
>  			break;
>  		++vi->num;
> -	} while (err > 0);
> +	} while (vi->rvq->num_free);
> +
>  	if (unlikely(vi->num > vi->max))
>  		vi->max = vi->num;
>  	virtqueue_kick(vi->rvq);

I like this, it always bothered me that RX intentionally
triggers a failure path in virtqueue_add_buf.

> @@ -625,27 +626,20 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
>  static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
> -	int capacity;
> +	int err;
>  
>  	/* Free up any pending old buffers before queueing new ones. */
>  	free_old_xmit_skbs(vi);
>  
>  	/* Try to transmit */
> -	capacity = xmit_skb(vi, skb);
> +	err = xmit_skb(vi, skb);
>  
> -	/* This can happen with OOM and indirect buffers. */
> -	if (unlikely(capacity < 0)) {
> -		if (likely(capacity == -ENOMEM)) {
> -			if (net_ratelimit())
> -				dev_warn(&dev->dev,
> -					 "TX queue failure: out of memory\n");
> -		} else {
> -			dev->stats.tx_fifo_errors++;
> -			if (net_ratelimit())
> -				dev_warn(&dev->dev,
> -					 "Unexpected TX queue failure: %d\n",
> -					 capacity);
> -		}
> +	/* This should not happen! */
> +	if (unlikely(err < 0)) {
> +		dev->stats.tx_fifo_errors++;
> +		if (net_ratelimit())
> +			dev_warn(&dev->dev,
> +				 "Unexpected TX queue failure: %d\n", err);
>  		dev->stats.tx_dropped++;
>  		kfree_skb(skb);
>  		return NETDEV_TX_OK;
> @@ -658,13 +652,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	/* Apparently nice girls don't return TX_BUSY; stop the queue
>  	 * before it gets out of hand.  Naturally, this wastes entries. */
> -	if (capacity < 2+MAX_SKB_FRAGS) {
> +	if (vi->svq->num_free < 2+MAX_SKB_FRAGS) {
>  		netif_stop_queue(dev);
>  		if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
>  			/* More just got used, free them then recheck. */
>  			free_old_xmit_skbs(vi);
> -			capacity = vi->svq->num_free;
> -			if (capacity >= 2+MAX_SKB_FRAGS) {
> +			if (vi->svq->num_free >= 2+MAX_SKB_FRAGS) {
>  				netif_start_queue(dev);
>  				virtqueue_disable_cb(vi->svq);
>  			}
> -- 
> 1.7.9.5

Re: [PATCH 5/5] virtio: make virtqueue_add_buf() returning 0 on success, not capacity.

[Rusty Russell]

Note: I also have minor cleanups to virtio drivers to change (ret >= 0)
to (ret == 0) where it would otherwise be confusing for new readers, but
I didn't clutter the list with them.  See my virtio-wip tree for the
gory details.

Cheers,
Rusty.

Re: [PATCH 1/5] virtio: move queue_index and num_free fields into core struct virtqueue.

[Michael S. Tsirkin]

On Wed, Oct 17, 2012 at 12:00:48AM +1030, Rusty Russell wrote:
> They're generic concepts, so hoist them.  This also avoids accessor
> functions.
> 
> This goes even further than Jason Wang's 17bb6d4088 patch
> ("virtio-ring: move queue_index to vring_virtqueue") which moved the
> queue_index from the specific transport.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/virtio/virtio_mmio.c |    4 ++--
>  drivers/virtio/virtio_pci.c  |    6 ++----
>  drivers/virtio/virtio_ring.c |   34 +++++++++++-----------------------
>  include/linux/virtio.h       |    6 ++++--
>  4 files changed, 19 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 6b1b7e1..5a0e1d3 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -225,7 +225,7 @@ static void vm_notify(struct virtqueue *vq)
>  
>  	/* We write the queue's selector into the notification register to
>  	 * signal the other end */
> -	writel(virtqueue_get_queue_index(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> +	writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
>  }
>  
>  /* Notify all virtqueues on an interrupt. */
> @@ -266,7 +266,7 @@ static void vm_del_vq(struct virtqueue *vq)
>  	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
>  	struct virtio_mmio_vq_info *info = vq->priv;
>  	unsigned long flags, size;
> -	unsigned int index = virtqueue_get_queue_index(vq);
> +	unsigned int index = vq->index;
>  
>  	spin_lock_irqsave(&vm_dev->lock, flags);
>  	list_del(&info->node);
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index b59237c..e3ecc94 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -203,8 +203,7 @@ static void vp_notify(struct virtqueue *vq)
>  
>  	/* we write the queue's selector into the notification register to
>  	 * signal the other end */
> -	iowrite16(virtqueue_get_queue_index(vq),
> -		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
> +	iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
>  }
>  
>  /* Handle a configuration change: Tell driver if it wants to know. */
> @@ -479,8 +478,7 @@ static void vp_del_vq(struct virtqueue *vq)
>  	list_del(&info->node);
>  	spin_unlock_irqrestore(&vp_dev->lock, flags);
>  
> -	iowrite16(virtqueue_get_queue_index(vq),
> -		vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
> +	iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
>  
>  	if (vp_dev->msix_enabled) {
>  		iowrite16(VIRTIO_MSI_NO_VECTOR,
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index e639584..335dcec 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -93,8 +93,6 @@ struct vring_virtqueue
>  	/* Host publishes avail event idx */
>  	bool event;
>  
> -	/* Number of free buffers */
> -	unsigned int num_free;
>  	/* Head of free buffer list. */
>  	unsigned int free_head;
>  	/* Number we've added since last sync. */
> @@ -106,9 +104,6 @@ struct vring_virtqueue
>  	/* How to notify other side. FIXME: commonalize hcalls! */
>  	void (*notify)(struct virtqueue *vq);
>  
> -	/* Index of the queue */
> -	int queue_index;
> -
>  #ifdef DEBUG
>  	/* They're supposed to lock for us. */
>  	unsigned int in_use;
> @@ -160,7 +155,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
>  	desc[i-1].next = 0;
>  
>  	/* We're about to use a buffer */
> -	vq->num_free--;
> +	vq->vq.num_free--;
>  
>  	/* Use a single buffer which doesn't continue */
>  	head = vq->free_head;
> @@ -174,13 +169,6 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
>  	return head;
>  }
>  
> -int virtqueue_get_queue_index(struct virtqueue *_vq)
> -{
> -	struct vring_virtqueue *vq = to_vvq(_vq);
> -	return vq->queue_index;
> -}
> -EXPORT_SYMBOL_GPL(virtqueue_get_queue_index);
> -
>  /**
>   * virtqueue_add_buf - expose buffer to other end
>   * @vq: the struct virtqueue we're talking about.
> @@ -228,7 +216,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>  
>  	/* If the host supports indirect descriptor tables, and we have multiple
>  	 * buffers, then go indirect. FIXME: tune this threshold */
> -	if (vq->indirect && (out + in) > 1 && vq->num_free) {
> +	if (vq->indirect && (out + in) > 1 && vq->vq.num_free) {
>  		head = vring_add_indirect(vq, sg, out, in, gfp);
>  		if (likely(head >= 0))
>  			goto add_head;
> @@ -237,9 +225,9 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>  	BUG_ON(out + in > vq->vring.num);
>  	BUG_ON(out + in == 0);
>  
> -	if (vq->num_free < out + in) {
> +	if (vq->vq.num_free < out + in) {
>  		pr_debug("Can't add buf len %i - avail = %i\n",
> -			 out + in, vq->num_free);
> +			 out + in, vq->vq.num_free);
>  		/* FIXME: for historical reasons, we force a notify here if
>  		 * there are outgoing parts to the buffer.  Presumably the
>  		 * host should service the ring ASAP. */
> @@ -250,7 +238,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>  	}
>  
>  	/* We're about to use some buffers from the free list. */
> -	vq->num_free -= out + in;
> +	vq->vq.num_free -= out + in;
>  
>  	head = vq->free_head;
>  	for (i = vq->free_head; out; i = vq->vring.desc[i].next, out--) {
> @@ -296,7 +284,7 @@ add_head:
>  	pr_debug("Added buffer head %i to %p\n", head, vq);
>  	END_USE(vq);
>  
> -	return vq->num_free;
> +	return vq->vq.num_free;
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_add_buf);
>  
> @@ -393,13 +381,13 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
>  
>  	while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) {
>  		i = vq->vring.desc[i].next;
> -		vq->num_free++;
> +		vq->vq.num_free++;
>  	}
>  
>  	vq->vring.desc[i].next = vq->free_head;
>  	vq->free_head = head;
>  	/* Plus final descriptor */
> -	vq->num_free++;
> +	vq->vq.num_free++;
>  }
>  
>  static inline bool more_used(const struct vring_virtqueue *vq)
> @@ -599,7 +587,7 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
>  		return buf;
>  	}
>  	/* That should have freed everything. */
> -	BUG_ON(vq->num_free != vq->vring.num);
> +	BUG_ON(vq->vq.num_free != vq->vring.num);
>  
>  	END_USE(vq);
>  	return NULL;
> @@ -653,12 +641,13 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
>  	vq->vq.callback = callback;
>  	vq->vq.vdev = vdev;
>  	vq->vq.name = name;
> +	vq->vq.num_free = num;
> +	vq->vq.index = index;
>  	vq->notify = notify;
>  	vq->weak_barriers = weak_barriers;
>  	vq->broken = false;
>  	vq->last_used_idx = 0;
>  	vq->num_added = 0;
> -	vq->queue_index = index;
>  	list_add_tail(&vq->vq.list, &vdev->vqs);
>  #ifdef DEBUG
>  	vq->in_use = false;
> @@ -673,7 +662,6 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
>  		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
>  
>  	/* Put everything in free lists. */
> -	vq->num_free = num;
>  	vq->free_head = 0;
>  	for (i = 0; i < num-1; i++) {
>  		vq->vring.desc[i].next = i+1;
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 533b115..ed4c437 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -16,12 +16,16 @@
>   * @name: the name of this virtqueue (mainly for debugging)
>   * @vdev: the virtio device this queue was created for.
>   * @priv: a pointer for the virtqueue implementation to use.
> + * @index: the zero-based ordinal number for this queue.
> + * @num_free: number of buffers we expect to be able to fit.

Only it's not exactly buffers: a single buffer can use
multiple s/g entries, right?
Maybe clarify as 'linear buffers'?

>   */
>  struct virtqueue {
>  	struct list_head list;
>  	void (*callback)(struct virtqueue *vq);
>  	const char *name;
>  	struct virtio_device *vdev;
> +	unsigned int index;
> +	unsigned int num_free;
>  	void *priv;
>  };
>  
> @@ -50,8 +54,6 @@ void *virtqueue_detach_unused_buf(struct virtqueue *vq);
>  
>  unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
>  
> -int virtqueue_get_queue_index(struct virtqueue *vq);
> -
>  /**
>   * virtio_device - representation of a device using virtio
>   * @index: unique position on the virtio bus
> -- 
> 1.7.9.5

Re: [PATCH 1/5] virtio: move queue_index and num_free fields into core struct virtqueue.

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Wed, Oct 17, 2012 at 12:00:48AM +1030, Rusty Russell wrote:
>>   * @priv: a pointer for the virtqueue implementation to use.
>> + * @index: the zero-based ordinal number for this queue.
>> + * @num_free: number of buffers we expect to be able to fit.
>
> Only it's not exactly buffers: a single buffer can use
> multiple s/g entries, right?
> Maybe clarify as 'linear buffers'?

Yes, it needs clarification, but I don't think I can do so tersely.

Here's what I ended up with:

 * @num_free: number of elements we expect to be able to fit.
 *
 * A note on @num_free: with indirect buffers, each buffer needs one
 * element in the queue, otherwise a buffer will need one element per
 * sg element.

Thanks,
Rusty.

Re: [PATCH 1/5] virtio: move queue_index and num_free fields into core struct virtqueue.

[Michael S. Tsirkin]

On Wed, Oct 17, 2012 at 12:00:48AM +1030, Rusty Russell wrote:
> They're generic concepts, so hoist them.  This also avoids accessor
> functions.
> 
> This goes even further than Jason Wang's 17bb6d4088 patch
> ("virtio-ring: move queue_index to vring_virtqueue") which moved the
> queue_index from the specific transport.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

ACK series (with the corrected num_free description)

> ---
>  drivers/virtio/virtio_mmio.c |    4 ++--
>  drivers/virtio/virtio_pci.c  |    6 ++----
>  drivers/virtio/virtio_ring.c |   34 +++++++++++-----------------------
>  include/linux/virtio.h       |    6 ++++--
>  4 files changed, 19 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 6b1b7e1..5a0e1d3 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -225,7 +225,7 @@ static void vm_notify(struct virtqueue *vq)
>  
>  	/* We write the queue's selector into the notification register to
>  	 * signal the other end */
> -	writel(virtqueue_get_queue_index(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> +	writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
>  }
>  
>  /* Notify all virtqueues on an interrupt. */
> @@ -266,7 +266,7 @@ static void vm_del_vq(struct virtqueue *vq)
>  	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
>  	struct virtio_mmio_vq_info *info = vq->priv;
>  	unsigned long flags, size;
> -	unsigned int index = virtqueue_get_queue_index(vq);
> +	unsigned int index = vq->index;
>  
>  	spin_lock_irqsave(&vm_dev->lock, flags);
>  	list_del(&info->node);
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index b59237c..e3ecc94 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -203,8 +203,7 @@ static void vp_notify(struct virtqueue *vq)
>  
>  	/* we write the queue's selector into the notification register to
>  	 * signal the other end */
> -	iowrite16(virtqueue_get_queue_index(vq),
> -		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
> +	iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
>  }
>  
>  /* Handle a configuration change: Tell driver if it wants to know. */
> @@ -479,8 +478,7 @@ static void vp_del_vq(struct virtqueue *vq)
>  	list_del(&info->node);
>  	spin_unlock_irqrestore(&vp_dev->lock, flags);
>  
> -	iowrite16(virtqueue_get_queue_index(vq),
> -		vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
> +	iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
>  
>  	if (vp_dev->msix_enabled) {
>  		iowrite16(VIRTIO_MSI_NO_VECTOR,
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index e639584..335dcec 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -93,8 +93,6 @@ struct vring_virtqueue
>  	/* Host publishes avail event idx */
>  	bool event;
>  
> -	/* Number of free buffers */
> -	unsigned int num_free;
>  	/* Head of free buffer list. */
>  	unsigned int free_head;
>  	/* Number we've added since last sync. */
> @@ -106,9 +104,6 @@ struct vring_virtqueue
>  	/* How to notify other side. FIXME: commonalize hcalls! */
>  	void (*notify)(struct virtqueue *vq);
>  
> -	/* Index of the queue */
> -	int queue_index;
> -
>  #ifdef DEBUG
>  	/* They're supposed to lock for us. */
>  	unsigned int in_use;
> @@ -160,7 +155,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
>  	desc[i-1].next = 0;
>  
>  	/* We're about to use a buffer */
> -	vq->num_free--;
> +	vq->vq.num_free--;
>  
>  	/* Use a single buffer which doesn't continue */
>  	head = vq->free_head;
> @@ -174,13 +169,6 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
>  	return head;
>  }
>  
> -int virtqueue_get_queue_index(struct virtqueue *_vq)
> -{
> -	struct vring_virtqueue *vq = to_vvq(_vq);
> -	return vq->queue_index;
> -}
> -EXPORT_SYMBOL_GPL(virtqueue_get_queue_index);
> -
>  /**
>   * virtqueue_add_buf - expose buffer to other end
>   * @vq: the struct virtqueue we're talking about.
> @@ -228,7 +216,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>  
>  	/* If the host supports indirect descriptor tables, and we have multiple
>  	 * buffers, then go indirect. FIXME: tune this threshold */
> -	if (vq->indirect && (out + in) > 1 && vq->num_free) {
> +	if (vq->indirect && (out + in) > 1 && vq->vq.num_free) {
>  		head = vring_add_indirect(vq, sg, out, in, gfp);
>  		if (likely(head >= 0))
>  			goto add_head;
> @@ -237,9 +225,9 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>  	BUG_ON(out + in > vq->vring.num);
>  	BUG_ON(out + in == 0);
>  
> -	if (vq->num_free < out + in) {
> +	if (vq->vq.num_free < out + in) {
>  		pr_debug("Can't add buf len %i - avail = %i\n",
> -			 out + in, vq->num_free);
> +			 out + in, vq->vq.num_free);
>  		/* FIXME: for historical reasons, we force a notify here if
>  		 * there are outgoing parts to the buffer.  Presumably the
>  		 * host should service the ring ASAP. */
> @@ -250,7 +238,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>  	}
>  
>  	/* We're about to use some buffers from the free list. */
> -	vq->num_free -= out + in;
> +	vq->vq.num_free -= out + in;
>  
>  	head = vq->free_head;
>  	for (i = vq->free_head; out; i = vq->vring.desc[i].next, out--) {
> @@ -296,7 +284,7 @@ add_head:
>  	pr_debug("Added buffer head %i to %p\n", head, vq);
>  	END_USE(vq);
>  
> -	return vq->num_free;
> +	return vq->vq.num_free;
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_add_buf);
>  
> @@ -393,13 +381,13 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
>  
>  	while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) {
>  		i = vq->vring.desc[i].next;
> -		vq->num_free++;
> +		vq->vq.num_free++;
>  	}
>  
>  	vq->vring.desc[i].next = vq->free_head;
>  	vq->free_head = head;
>  	/* Plus final descriptor */
> -	vq->num_free++;
> +	vq->vq.num_free++;
>  }
>  
>  static inline bool more_used(const struct vring_virtqueue *vq)
> @@ -599,7 +587,7 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
>  		return buf;
>  	}
>  	/* That should have freed everything. */
> -	BUG_ON(vq->num_free != vq->vring.num);
> +	BUG_ON(vq->vq.num_free != vq->vring.num);
>  
>  	END_USE(vq);
>  	return NULL;
> @@ -653,12 +641,13 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
>  	vq->vq.callback = callback;
>  	vq->vq.vdev = vdev;
>  	vq->vq.name = name;
> +	vq->vq.num_free = num;
> +	vq->vq.index = index;
>  	vq->notify = notify;
>  	vq->weak_barriers = weak_barriers;
>  	vq->broken = false;
>  	vq->last_used_idx = 0;
>  	vq->num_added = 0;
> -	vq->queue_index = index;
>  	list_add_tail(&vq->vq.list, &vdev->vqs);
>  #ifdef DEBUG
>  	vq->in_use = false;
> @@ -673,7 +662,6 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
>  		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
>  
>  	/* Put everything in free lists. */
> -	vq->num_free = num;
>  	vq->free_head = 0;
>  	for (i = 0; i < num-1; i++) {
>  		vq->vring.desc[i].next = i+1;
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 533b115..ed4c437 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -16,12 +16,16 @@
>   * @name: the name of this virtqueue (mainly for debugging)
>   * @vdev: the virtio device this queue was created for.
>   * @priv: a pointer for the virtqueue implementation to use.
> + * @index: the zero-based ordinal number for this queue.
> + * @num_free: number of buffers we expect to be able to fit.
>   */
>  struct virtqueue {
>  	struct list_head list;
>  	void (*callback)(struct virtqueue *vq);
>  	const char *name;
>  	struct virtio_device *vdev;
> +	unsigned int index;
> +	unsigned int num_free;
>  	void *priv;
>  };
>  
> @@ -50,8 +54,6 @@ void *virtqueue_detach_unused_buf(struct virtqueue *vq);
>  
>  unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
>  
> -int virtqueue_get_queue_index(struct virtqueue *vq);
> -
>  /**
>   * virtio_device - representation of a device using virtio
>   * @index: unique position on the virtio bus
> -- 
> 1.7.9.5