[PATCH 00/22] virtqueue_add_sgs, virtqueue_add_outbuf, virtqueue_add_inbuf

[PATCH 00/22] virtqueue_add_sgs, virtqueue_add_outbuf, virtqueue_add_inbuf

[Rusty Russell]

Add virtqueue_add_sgs which is more general than virtqueue_add_buf,
which makes virtio-scsi and virtio-blk nicer, then add virtqueue_add_inbuf
and virtqueue_add_outbuf which handle the more general case, and finally
delete virtqueue_add_buf().

I'm hoping this will be the final post of the whole series, and it can
move from my pending-rebases tree into virtio-next.

Thanks!
Rusty.

Paolo Bonzini (4):
  scatterlist: introduce sg_unmark_end
  virtio-blk: reorganize virtblk_add_req
  virtio-blk: use virtqueue_add_sgs on bio path
  virtio-blk: use virtqueue_add_sgs on req path

Rusty Russell (17):
  virtio_ring: virtqueue_add_sgs, to add multiple sgs.
  virtio_ring: don't count elements twice for add_buf path.
  virtio_ring: inline internal vring functions more aggressively.
  virtio_ring: virtqueue_add_outbuf / virtqueue_add_inbuf.
  tools/virtio: make vringh_test use inbuf/outbuf.
  virtio_blk: remove nents member.
  virtio_scsi: use virtqueue_add_inbuf() for virtscsi_kick_event.
  virtio_net: use virtqueue_add_sgs[] for command buffers.
  virtio_net: use simplified virtqueue accessors.
  virtio_rng: use simplified virtqueue accessors.
  virtio_console: use simplified virtqueue accessors.
  caif_virtio: use simplified virtqueue accessors.
  virtio_rpmsg_bus: use simplified virtqueue accessors.
  virtio_balloon: use simplified virtqueue accessors.
  9p/trans_virtio.c: use virtio_add_sgs[]
  tools/virtio: remove virtqueue_add_buf() from tests.
  virtio: remove virtqueue_add_buf().

Wanlong Gao (1):
  virtio-scsi: use virtqueue_add_sgs for command buffers

 block/blk-integrity.c               |    2 +-
 block/blk-merge.c                   |    2 +-
 drivers/block/virtio_blk.c          |  146 ++++++++++------------
 drivers/char/hw_random/virtio-rng.c |    2 +-
 drivers/char/virtio_console.c       |    6 +-
 drivers/net/caif/caif_virtio.c      |    3 +-
 drivers/net/virtio_net.c            |   61 +++++----
 drivers/rpmsg/virtio_rpmsg_bus.c    |    8 +-
 drivers/scsi/virtio_scsi.c          |   95 ++++++--------
 drivers/virtio/virtio_balloon.c     |    6 +-
 drivers/virtio/virtio_ring.c        |  235 +++++++++++++++++++++++++----------
 include/linux/scatterlist.h         |   16 +++
 include/linux/virtio.h              |   18 ++-
 net/9p/trans_virtio.c               |   48 +++++--
 tools/virtio/linux/scatterlist.h    |   16 +++
 tools/virtio/linux/virtio.h         |   18 ++-
 tools/virtio/virtio_test.c          |    6 +-
 tools/virtio/vringh_test.c          |   30 ++---
 18 files changed, 433 insertions(+), 285 deletions(-)

-- 
1.7.10.4

[PATCH 01/22] scatterlist: introduce sg_unmark_end

[Rusty Russell]

From: Paolo Bonzini <pbonzini@redhat.com>

This is useful in places that recycle the same scatterlist multiple
times, and do not want to incur the cost of sg_init_table every
time in hot paths.

Acked-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 block/blk-integrity.c       |    2 +-
 block/blk-merge.c           |    2 +-
 include/linux/scatterlist.h |   16 ++++++++++++++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index dabd221..03cf717 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -110,7 +110,7 @@ new_segment:
 			if (!sg)
 				sg = sglist;
 			else {
-				sg->page_link &= ~0x02;
+				sg_unmark_end(sg);
 				sg = sg_next(sg);
 			}
 
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 936a110..5f24482 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -143,7 +143,7 @@ new_segment:
 			 * termination bit to avoid doing a full
 			 * sg_init_table() in drivers for each command.
 			 */
-			(*sg)->page_link &= ~0x02;
+			sg_unmark_end(*sg);
 			*sg = sg_next(*sg);
 		}
 
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 2d8bdae..bfc47e0 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -172,6 +172,22 @@ static inline void sg_mark_end(struct scatterlist *sg)
 }
 
 /**
+ * sg_unmark_end - Undo setting the end of the scatterlist
+ * @sg:		 SG entryScatterlist
+ *
+ * Description:
+ *   Removes the termination marker from the given entry of the scatterlist.
+ *
+ **/
+static inline void sg_unmark_end(struct scatterlist *sg)
+{
+#ifdef CONFIG_DEBUG_SG
+	BUG_ON(sg->sg_magic != SG_MAGIC);
+#endif
+	sg->page_link &= ~0x02;
+}
+
+/**
  * sg_phys - Return physical address of an sg entry
  * @sg:	     SG entry
  *
-- 
1.7.10.4

[PATCH 02/22] virtio_ring: virtqueue_add_sgs, to add multiple sgs.

[Rusty Russell]

virtio_scsi can really use this, to avoid the current hack of copying
the whole sg array.  Some other things get slightly neater, too.

This causes a slowdown in virtqueue_add_buf(), which is naively
implemented as a wrapper.  This is addressed in the next patches.

for i in `seq 50`; do /usr/bin/time -f 'Wall time:%e' ./vringh_test --indirect --eventidx --parallel --fast-vringh; done 2>&1 | stats --trim-outliers:

Before:
	Using CPUS 0 and 3
	Guest: notified 0, pinged 39009-39063(39062)
	Host: notified 39009-39063(39062), pinged 0
	Wall time:1.700000-1.950000(1.723542)

After:
	Using CPUS 0 and 3
	Guest: notified 0, pinged 39019-39063(39061)
	Host: notified 39019-39063(39061), pinged 0
	Wall time:2.090000-2.520000(2.188542)

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Reviewed-by: Asias He <asias@redhat.com>
---
 drivers/virtio/virtio_ring.c     |  144 +++++++++++++++++++++++++++-----------
 include/linux/virtio.h           |    7 ++
 tools/virtio/linux/scatterlist.h |   16 +++++
 tools/virtio/linux/virtio.h      |    7 ++
 4 files changed, 132 insertions(+), 42 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 245177c..27e31d3 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -100,14 +100,16 @@ struct vring_virtqueue
 
 /* Set up an indirect table of descriptors and add it to the queue. */
 static int vring_add_indirect(struct vring_virtqueue *vq,
-			      struct scatterlist sg[],
-			      unsigned int out,
-			      unsigned int in,
+			      struct scatterlist *sgs[],
+			      unsigned int total_sg,
+			      unsigned int out_sgs,
+			      unsigned int in_sgs,
 			      gfp_t gfp)
 {
 	struct vring_desc *desc;
 	unsigned head;
-	int i;
+	struct scatterlist *sg;
+	int i, n;
 
 	/*
 	 * We require lowmem mappings for the descriptors because
@@ -116,25 +118,31 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
 	 */
 	gfp &= ~(__GFP_HIGHMEM | __GFP_HIGH);
 
-	desc = kmalloc((out + in) * sizeof(struct vring_desc), gfp);
+	desc = kmalloc(total_sg * sizeof(struct vring_desc), gfp);
 	if (!desc)
 		return -ENOMEM;
 
-	/* Transfer entries from the sg list into the indirect page */
-	for (i = 0; i < out; i++) {
-		desc[i].flags = VRING_DESC_F_NEXT;
-		desc[i].addr = sg_phys(sg);
-		desc[i].len = sg->length;
-		desc[i].next = i+1;
-		sg++;
+	/* Transfer entries from the sg lists into the indirect page */
+	i = 0;
+	for (n = 0; n < out_sgs; n++) {
+		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+			desc[i].flags = VRING_DESC_F_NEXT;
+			desc[i].addr = sg_phys(sg);
+			desc[i].len = sg->length;
+			desc[i].next = i+1;
+			i++;
+		}
 	}
-	for (; i < (out + in); i++) {
-		desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
-		desc[i].addr = sg_phys(sg);
-		desc[i].len = sg->length;
-		desc[i].next = i+1;
-		sg++;
+	for (; n < (out_sgs + in_sgs); n++) {
+		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+			desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
+			desc[i].addr = sg_phys(sg);
+			desc[i].len = sg->length;
+			desc[i].next = i+1;
+			i++;
+		}
 	}
+	BUG_ON(i != total_sg);
 
 	/* Last one doesn't continue. */
 	desc[i-1].flags &= ~VRING_DESC_F_NEXT;
@@ -176,8 +184,48 @@ int virtqueue_add_buf(struct virtqueue *_vq,
 		      void *data,
 		      gfp_t gfp)
 {
+	struct scatterlist *sgs[2];
+	unsigned int i;
+
+	sgs[0] = sg;
+	sgs[1] = sg + out;
+
+	/* Workaround until callers pass well-formed sgs. */
+	for (i = 0; i < out + in; i++)
+		sg_unmark_end(sg + i);
+
+	sg_mark_end(sg + out + in - 1);
+	if (out && in)
+		sg_mark_end(sg + out - 1);
+
+	return virtqueue_add_sgs(_vq, sgs, out ? 1 : 0, in ? 1 : 0, data, gfp);
+}
+EXPORT_SYMBOL_GPL(virtqueue_add_buf);
+
+/**
+ * virtqueue_add_sgs - expose buffers to other end
+ * @vq: the struct virtqueue we're talking about.
+ * @sgs: array of terminated scatterlists.
+ * @out_num: the number of scatterlists readable by other side
+ * @in_num: the number of scatterlists which are writable (after readable ones)
+ * @data: the token identifying the buffer.
+ * @gfp: how to do memory allocations (if necessary).
+ *
+ * Caller must ensure we don't call this with other virtqueue operations
+ * at the same time (except where noted).
+ *
+ * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
+ */
+int virtqueue_add_sgs(struct virtqueue *_vq,
+		      struct scatterlist *sgs[],
+		      unsigned int out_sgs,
+		      unsigned int in_sgs,
+		      void *data,
+		      gfp_t gfp)
+{
 	struct vring_virtqueue *vq = to_vvq(_vq);
-	unsigned int i, avail, uninitialized_var(prev);
+	struct scatterlist *sg;
+	unsigned int i, n, avail, uninitialized_var(prev), total_sg;
 	int head;
 
 	START_USE(vq);
@@ -197,46 +245,58 @@ int virtqueue_add_buf(struct virtqueue *_vq,
 	}
 #endif
 
+	/* Count them first. */
+	for (i = total_sg = 0; i < out_sgs + in_sgs; i++) {
+		struct scatterlist *sg;
+		for (sg = sgs[i]; sg; sg = sg_next(sg))
+			total_sg++;
+	}
+
 	/* 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->vq.num_free) {
-		head = vring_add_indirect(vq, sg, out, in, gfp);
+	if (vq->indirect && total_sg > 1 && vq->vq.num_free) {
+		head = vring_add_indirect(vq, sgs, total_sg, out_sgs, in_sgs,
+					  gfp);
 		if (likely(head >= 0))
 			goto add_head;
 	}
 
-	BUG_ON(out + in > vq->vring.num);
-	BUG_ON(out + in == 0);
+	BUG_ON(total_sg > vq->vring.num);
+	BUG_ON(total_sg == 0);
 
-	if (vq->vq.num_free < out + in) {
+	if (vq->vq.num_free < total_sg) {
 		pr_debug("Can't add buf len %i - avail = %i\n",
-			 out + in, vq->vq.num_free);
+			 total_sg, 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. */
-		if (out)
+		if (out_sgs)
 			vq->notify(&vq->vq);
 		END_USE(vq);
 		return -ENOSPC;
 	}
 
 	/* We're about to use some buffers from the free list. */
-	vq->vq.num_free -= out + in;
-
-	head = vq->free_head;
-	for (i = vq->free_head; out; i = vq->vring.desc[i].next, out--) {
-		vq->vring.desc[i].flags = VRING_DESC_F_NEXT;
-		vq->vring.desc[i].addr = sg_phys(sg);
-		vq->vring.desc[i].len = sg->length;
-		prev = i;
-		sg++;
+	vq->vq.num_free -= total_sg;
+
+	head = i = vq->free_head;
+	for (n = 0; n < out_sgs; n++) {
+		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+			vq->vring.desc[i].flags = VRING_DESC_F_NEXT;
+			vq->vring.desc[i].addr = sg_phys(sg);
+			vq->vring.desc[i].len = sg->length;
+			prev = i;
+			i = vq->vring.desc[i].next;
+		}
 	}
-	for (; in; i = vq->vring.desc[i].next, in--) {
-		vq->vring.desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
-		vq->vring.desc[i].addr = sg_phys(sg);
-		vq->vring.desc[i].len = sg->length;
-		prev = i;
-		sg++;
+	for (; n < (out_sgs + in_sgs); n++) {
+		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+			vq->vring.desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
+			vq->vring.desc[i].addr = sg_phys(sg);
+			vq->vring.desc[i].len = sg->length;
+			prev = i;
+			i = vq->vring.desc[i].next;
+		}
 	}
 	/* Last one doesn't continue. */
 	vq->vring.desc[prev].flags &= ~VRING_DESC_F_NEXT;
@@ -269,7 +329,7 @@ add_head:
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(virtqueue_add_buf);
+EXPORT_SYMBOL_GPL(virtqueue_add_sgs);
 
 /**
  * virtqueue_kick_prepare - first half of split virtqueue_kick call.
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 5d5b3ab..ac80288 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -41,6 +41,13 @@ int virtqueue_add_buf(struct virtqueue *vq,
 		      void *data,
 		      gfp_t gfp);
 
+int virtqueue_add_sgs(struct virtqueue *vq,
+		      struct scatterlist *sgs[],
+		      unsigned int out_sgs,
+		      unsigned int in_sgs,
+		      void *data,
+		      gfp_t gfp);
+
 void virtqueue_kick(struct virtqueue *vq);
 
 bool virtqueue_kick_prepare(struct virtqueue *vq);
diff --git a/tools/virtio/linux/scatterlist.h b/tools/virtio/linux/scatterlist.h
index b2cf7d0..68c9e2a 100644
--- a/tools/virtio/linux/scatterlist.h
+++ b/tools/virtio/linux/scatterlist.h
@@ -125,6 +125,22 @@ static inline void sg_mark_end(struct scatterlist *sg)
 	sg->page_link &= ~0x01;
 }
 
+/**
+ * sg_unmark_end - Undo setting the end of the scatterlist
+ * @sg:		 SG entryScatterlist
+ *
+ * Description:
+ *   Removes the termination marker from the given entry of the scatterlist.
+ *
+ **/
+static inline void sg_unmark_end(struct scatterlist *sg)
+{
+#ifdef CONFIG_DEBUG_SG
+	BUG_ON(sg->sg_magic != SG_MAGIC);
+#endif
+	sg->page_link &= ~0x02;
+}
+
 static inline struct scatterlist *sg_next(struct scatterlist *sg)
 {
 #ifdef CONFIG_DEBUG_SG
diff --git a/tools/virtio/linux/virtio.h b/tools/virtio/linux/virtio.h
index e4af659..5fa612a 100644
--- a/tools/virtio/linux/virtio.h
+++ b/tools/virtio/linux/virtio.h
@@ -56,6 +56,13 @@ int virtqueue_add_buf(struct virtqueue *vq,
 		      void *data,
 		      gfp_t gfp);
 
+int virtqueue_add_sgs(struct virtqueue *vq,
+		      struct scatterlist *sgs[],
+		      unsigned int out_sgs,
+		      unsigned int in_sgs,
+		      void *data,
+		      gfp_t gfp);
+
 void virtqueue_kick(struct virtqueue *vq);
 
 void *virtqueue_get_buf(struct virtqueue *vq, unsigned int *len);
-- 
1.7.10.4

[PATCH 03/22] virtio_ring: don't count elements twice for add_buf path.

[Rusty Russell]

Extract the post-counting code into virtqueue_add(), make both callers
use it.  As much for neatness as optimization.

for i in `seq 50`; do /usr/bin/time -f 'Wall time:%e' ./vringh_test --indirect --eventidx --parallel --fast-vringh; done 2>&1 | stats --trim-outliers:

Before:
	Using CPUS 0 and 3
	Guest: notified 0, pinged 39019-39063(39061)
	Host: notified 39019-39063(39061), pinged 0
	Wall time:2.090000-2.520000(2.188542)

After:
	Using CPUS 0 and 3
	Guest: notified 0, pinged 39014-39063(39062)
	Host: notified 39014-39063(39062), pinged 0
	Wall time:1.900000-2.350000(1.921875)

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 drivers/virtio/virtio_ring.c |  147 +++++++++++++++++++++++-------------------
 1 file changed, 80 insertions(+), 67 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 27e31d3..c537385 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -163,69 +163,17 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
 	return head;
 }
 
-/**
- * virtqueue_add_buf - expose buffer to other end
- * @vq: the struct virtqueue we're talking about.
- * @sg: the description of the buffer(s).
- * @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.
- * @gfp: how to do memory allocations (if necessary).
- *
- * Caller must ensure we don't call this with other virtqueue operations
- * at the same time (except where noted).
- *
- * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
- */
-int virtqueue_add_buf(struct virtqueue *_vq,
-		      struct scatterlist sg[],
-		      unsigned int out,
-		      unsigned int in,
-		      void *data,
-		      gfp_t gfp)
-{
-	struct scatterlist *sgs[2];
-	unsigned int i;
-
-	sgs[0] = sg;
-	sgs[1] = sg + out;
-
-	/* Workaround until callers pass well-formed sgs. */
-	for (i = 0; i < out + in; i++)
-		sg_unmark_end(sg + i);
-
-	sg_mark_end(sg + out + in - 1);
-	if (out && in)
-		sg_mark_end(sg + out - 1);
-
-	return virtqueue_add_sgs(_vq, sgs, out ? 1 : 0, in ? 1 : 0, data, gfp);
-}
-EXPORT_SYMBOL_GPL(virtqueue_add_buf);
-
-/**
- * virtqueue_add_sgs - expose buffers to other end
- * @vq: the struct virtqueue we're talking about.
- * @sgs: array of terminated scatterlists.
- * @out_num: the number of scatterlists readable by other side
- * @in_num: the number of scatterlists which are writable (after readable ones)
- * @data: the token identifying the buffer.
- * @gfp: how to do memory allocations (if necessary).
- *
- * Caller must ensure we don't call this with other virtqueue operations
- * at the same time (except where noted).
- *
- * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
- */
-int virtqueue_add_sgs(struct virtqueue *_vq,
-		      struct scatterlist *sgs[],
-		      unsigned int out_sgs,
-		      unsigned int in_sgs,
-		      void *data,
-		      gfp_t gfp)
+static int virtqueue_add(struct virtqueue *_vq,
+			 struct scatterlist *sgs[],
+			 unsigned int total_sg,
+			 unsigned int out_sgs,
+			 unsigned int in_sgs,
+			 void *data,
+			 gfp_t gfp)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 	struct scatterlist *sg;
-	unsigned int i, n, avail, uninitialized_var(prev), total_sg;
+	unsigned int i, n, avail, uninitialized_var(prev);
 	int head;
 
 	START_USE(vq);
@@ -245,13 +193,6 @@ int virtqueue_add_sgs(struct virtqueue *_vq,
 	}
 #endif
 
-	/* Count them first. */
-	for (i = total_sg = 0; i < out_sgs + in_sgs; i++) {
-		struct scatterlist *sg;
-		for (sg = sgs[i]; sg; sg = sg_next(sg))
-			total_sg++;
-	}
-
 	/* If the host supports indirect descriptor tables, and we have multiple
 	 * buffers, then go indirect. FIXME: tune this threshold */
 	if (vq->indirect && total_sg > 1 && vq->vq.num_free) {
@@ -329,6 +270,78 @@ add_head:
 
 	return 0;
 }
+
+/**
+ * virtqueue_add_buf - expose buffer to other end
+ * @vq: the struct virtqueue we're talking about.
+ * @sg: the description of the buffer(s).
+ * @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.
+ * @gfp: how to do memory allocations (if necessary).
+ *
+ * Caller must ensure we don't call this with other virtqueue operations
+ * at the same time (except where noted).
+ *
+ * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
+ */
+int virtqueue_add_buf(struct virtqueue *_vq,
+		      struct scatterlist sg[],
+		      unsigned int out,
+		      unsigned int in,
+		      void *data,
+		      gfp_t gfp)
+{
+	struct scatterlist *sgs[2];
+	unsigned int i;
+
+	sgs[0] = sg;
+	sgs[1] = sg + out;
+
+	/* Workaround until callers pass well-formed sgs. */
+	for (i = 0; i < out + in; i++)
+		sg_unmark_end(sg + i);
+
+	sg_mark_end(sg + out + in - 1);
+	if (out && in)
+		sg_mark_end(sg + out - 1);
+
+	return virtqueue_add(_vq, sgs, out+in, out ? 1 : 0, in ? 1 : 0,
+			     data, gfp);
+}
+EXPORT_SYMBOL_GPL(virtqueue_add_buf);
+
+/**
+ * virtqueue_add_sgs - expose buffers to other end
+ * @vq: the struct virtqueue we're talking about.
+ * @sgs: array of terminated scatterlists.
+ * @out_num: the number of scatterlists readable by other side
+ * @in_num: the number of scatterlists which are writable (after readable ones)
+ * @data: the token identifying the buffer.
+ * @gfp: how to do memory allocations (if necessary).
+ *
+ * Caller must ensure we don't call this with other virtqueue operations
+ * at the same time (except where noted).
+ *
+ * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
+ */
+int virtqueue_add_sgs(struct virtqueue *_vq,
+		      struct scatterlist *sgs[],
+		      unsigned int out_sgs,
+		      unsigned int in_sgs,
+		      void *data,
+		      gfp_t gfp)
+{
+	unsigned int i, total_sg;
+
+	/* Count them first. */
+	for (i = total_sg = 0; i < out_sgs + in_sgs; i++) {
+		struct scatterlist *sg;
+		for (sg = sgs[i]; sg; sg = sg_next(sg))
+			total_sg++;
+	}
+	return virtqueue_add(_vq, sgs, total_sg, out_sgs, in_sgs, data, gfp);
+}
 EXPORT_SYMBOL_GPL(virtqueue_add_sgs);
 
 /**
-- 
1.7.10.4

[PATCH 04/22] virtio_ring: inline internal vring functions more aggressively.

[Rusty Russell]

We use inline and get gcc to do the right thing inlining the
"indirect" traversal functions.  This also means we don't need to
clean the sgs.

for i in `seq 50`; do /usr/bin/time -f 'Wall time:%e' ./vringh_test --indirect --eventidx --parallel --fast-vringh; done 2>&1 | stats --trim-outliers:

Before:
	Using CPUS 0 and 3
	Guest: notified 0, pinged 39014-39063(39062)
	Host: notified 39014-39063(39062), pinged 0
	Wall time:1.900000-2.350000(1.921875)

After:
	Using CPUS 0 and 3
	Guest: notified 0, pinged 39062-39063(39063)
	Host: notified 39062-39063(39063), pinged 0
	Wall time:1.760000-2.220000(1.789167)

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index c537385..a78ad45 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -98,13 +98,31 @@ struct vring_virtqueue
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
 
+static inline struct scatterlist *sg_next_chained(struct scatterlist *sg,
+						  unsigned int *count)
+{
+	return sg_next(sg);
+}
+
+static inline struct scatterlist *sg_next_arr(struct scatterlist *sg,
+					      unsigned int *count)
+{
+	if (--(*count) == 0)
+		return NULL;
+	return sg + 1;
+}
+
 /* Set up an indirect table of descriptors and add it to the queue. */
-static int vring_add_indirect(struct vring_virtqueue *vq,
-			      struct scatterlist *sgs[],
-			      unsigned int total_sg,
-			      unsigned int out_sgs,
-			      unsigned int in_sgs,
-			      gfp_t gfp)
+static inline int vring_add_indirect(struct vring_virtqueue *vq,
+				     struct scatterlist *sgs[],
+				     struct scatterlist *(*next)
+				       (struct scatterlist *, unsigned int *),
+				     unsigned int total_sg,
+				     unsigned int total_out,
+				     unsigned int total_in,
+				     unsigned int out_sgs,
+				     unsigned int in_sgs,
+				     gfp_t gfp)
 {
 	struct vring_desc *desc;
 	unsigned head;
@@ -125,7 +143,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
 	/* Transfer entries from the sg lists into the indirect page */
 	i = 0;
 	for (n = 0; n < out_sgs; n++) {
-		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+		for (sg = sgs[n]; sg; sg = next(sg, &total_out)) {
 			desc[i].flags = VRING_DESC_F_NEXT;
 			desc[i].addr = sg_phys(sg);
 			desc[i].len = sg->length;
@@ -134,7 +152,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
 		}
 	}
 	for (; n < (out_sgs + in_sgs); n++) {
-		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+		for (sg = sgs[n]; sg; sg = next(sg, &total_in)) {
 			desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
 			desc[i].addr = sg_phys(sg);
 			desc[i].len = sg->length;
@@ -163,17 +181,20 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
 	return head;
 }
 
-static int virtqueue_add(struct virtqueue *_vq,
-			 struct scatterlist *sgs[],
-			 unsigned int total_sg,
-			 unsigned int out_sgs,
-			 unsigned int in_sgs,
-			 void *data,
-			 gfp_t gfp)
+static inline int virtqueue_add(struct virtqueue *_vq,
+				struct scatterlist *sgs[],
+				struct scatterlist *(*next)
+				  (struct scatterlist *, unsigned int *),
+				unsigned int total_out,
+				unsigned int total_in,
+				unsigned int out_sgs,
+				unsigned int in_sgs,
+				void *data,
+				gfp_t gfp)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 	struct scatterlist *sg;
-	unsigned int i, n, avail, uninitialized_var(prev);
+	unsigned int i, n, avail, uninitialized_var(prev), total_sg;
 	int head;
 
 	START_USE(vq);
@@ -193,11 +214,14 @@ static int virtqueue_add(struct virtqueue *_vq,
 	}
 #endif
 
+	total_sg = total_in + total_out;
+
 	/* If the host supports indirect descriptor tables, and we have multiple
 	 * buffers, then go indirect. FIXME: tune this threshold */
 	if (vq->indirect && total_sg > 1 && vq->vq.num_free) {
-		head = vring_add_indirect(vq, sgs, total_sg, out_sgs, in_sgs,
-					  gfp);
+		head = vring_add_indirect(vq, sgs, next, total_sg, total_out,
+					  total_in,
+					  out_sgs, in_sgs, gfp);
 		if (likely(head >= 0))
 			goto add_head;
 	}
@@ -222,7 +246,7 @@ static int virtqueue_add(struct virtqueue *_vq,
 
 	head = i = vq->free_head;
 	for (n = 0; n < out_sgs; n++) {
-		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+		for (sg = sgs[n]; sg; sg = next(sg, &total_out)) {
 			vq->vring.desc[i].flags = VRING_DESC_F_NEXT;
 			vq->vring.desc[i].addr = sg_phys(sg);
 			vq->vring.desc[i].len = sg->length;
@@ -231,7 +255,7 @@ static int virtqueue_add(struct virtqueue *_vq,
 		}
 	}
 	for (; n < (out_sgs + in_sgs); n++) {
-		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+		for (sg = sgs[n]; sg; sg = next(sg, &total_in)) {
 			vq->vring.desc[i].flags = VRING_DESC_F_NEXT|VRING_DESC_F_WRITE;
 			vq->vring.desc[i].addr = sg_phys(sg);
 			vq->vring.desc[i].len = sg->length;
@@ -293,21 +317,12 @@ int virtqueue_add_buf(struct virtqueue *_vq,
 		      gfp_t gfp)
 {
 	struct scatterlist *sgs[2];
-	unsigned int i;
 
 	sgs[0] = sg;
 	sgs[1] = sg + out;
 
-	/* Workaround until callers pass well-formed sgs. */
-	for (i = 0; i < out + in; i++)
-		sg_unmark_end(sg + i);
-
-	sg_mark_end(sg + out + in - 1);
-	if (out && in)
-		sg_mark_end(sg + out - 1);
-
-	return virtqueue_add(_vq, sgs, out+in, out ? 1 : 0, in ? 1 : 0,
-			     data, gfp);
+	return virtqueue_add(_vq, sgs, sg_next_arr,
+			     out, in, out ? 1 : 0, in ? 1 : 0, data, gfp);
 }
 EXPORT_SYMBOL_GPL(virtqueue_add_buf);
 
@@ -332,15 +347,21 @@ int virtqueue_add_sgs(struct virtqueue *_vq,
 		      void *data,
 		      gfp_t gfp)
 {
-	unsigned int i, total_sg;
+	unsigned int i, total_out, total_in;
 
 	/* Count them first. */
-	for (i = total_sg = 0; i < out_sgs + in_sgs; i++) {
+	for (i = total_out = total_in = 0; i < out_sgs; i++) {
+		struct scatterlist *sg;
+		for (sg = sgs[i]; sg; sg = sg_next(sg))
+			total_out++;
+	}
+	for (; i < out_sgs + in_sgs; i++) {
 		struct scatterlist *sg;
 		for (sg = sgs[i]; sg; sg = sg_next(sg))
-			total_sg++;
+			total_in++;
 	}
-	return virtqueue_add(_vq, sgs, total_sg, out_sgs, in_sgs, data, gfp);
+	return virtqueue_add(_vq, sgs, sg_next_chained,
+			     total_out, total_in, out_sgs, in_sgs, data, gfp);
 }
 EXPORT_SYMBOL_GPL(virtqueue_add_sgs);
 
-- 
1.7.10.4

[PATCH 05/22] virtio_ring: virtqueue_add_outbuf / virtqueue_add_inbuf.

[Rusty Russell]

These are specialized versions of virtqueue_add_buf(), which cover
over 80% of cases and are far clearer.

In particular, the scatterlists passed to these functions don't have
to be clean (ie. we ignore end markers).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/virtio/virtio_ring.c |   44 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h       |   10 ++++++++++
 2 files changed, 54 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a78ad45..5217baf 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -366,6 +366,50 @@ int virtqueue_add_sgs(struct virtqueue *_vq,
 EXPORT_SYMBOL_GPL(virtqueue_add_sgs);
 
 /**
+ * virtqueue_add_outbuf - expose output buffers to other end
+ * @vq: the struct virtqueue we're talking about.
+ * @sgs: array of scatterlists (need not be terminated!)
+ * @num: the number of scatterlists readable by other side
+ * @data: the token identifying the buffer.
+ * @gfp: how to do memory allocations (if necessary).
+ *
+ * Caller must ensure we don't call this with other virtqueue operations
+ * at the same time (except where noted).
+ *
+ * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
+ */
+int virtqueue_add_outbuf(struct virtqueue *vq,
+			 struct scatterlist sg[], unsigned int num,
+			 void *data,
+			 gfp_t gfp)
+{
+	return virtqueue_add(vq, &sg, sg_next_arr, num, 0, 1, 0, data, gfp);
+}
+EXPORT_SYMBOL_GPL(virtqueue_add_outbuf);
+
+/**
+ * virtqueue_add_inbuf - expose input buffers to other end
+ * @vq: the struct virtqueue we're talking about.
+ * @sgs: array of scatterlists (need not be terminated!)
+ * @num: the number of scatterlists writable by other side
+ * @data: the token identifying the buffer.
+ * @gfp: how to do memory allocations (if necessary).
+ *
+ * Caller must ensure we don't call this with other virtqueue operations
+ * at the same time (except where noted).
+ *
+ * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
+ */
+int virtqueue_add_inbuf(struct virtqueue *vq,
+			struct scatterlist sg[], unsigned int num,
+			void *data,
+			gfp_t gfp)
+{
+	return virtqueue_add(vq, &sg, sg_next_arr, 0, num, 0, 1, data, gfp);
+}
+EXPORT_SYMBOL_GPL(virtqueue_add_inbuf);
+
+/**
  * virtqueue_kick_prepare - first half of split virtqueue_kick call.
  * @vq: the struct virtqueue
  *
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index ac80288..833f17b 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -41,6 +41,16 @@ int virtqueue_add_buf(struct virtqueue *vq,
 		      void *data,
 		      gfp_t gfp);
 
+int virtqueue_add_outbuf(struct virtqueue *vq,
+			 struct scatterlist sg[], unsigned int num,
+			 void *data,
+			 gfp_t gfp);
+
+int virtqueue_add_inbuf(struct virtqueue *vq,
+			struct scatterlist sg[], unsigned int num,
+			void *data,
+			gfp_t gfp);
+
 int virtqueue_add_sgs(struct virtqueue *vq,
 		      struct scatterlist *sgs[],
 		      unsigned int out_sgs,
-- 
1.7.10.4

[PATCH 06/22] tools/virtio: make vringh_test use inbuf/outbuf.

[Rusty Russell]

The simplified accessors are faster.

Before:
	Using CPUS 0 and 3
	Guest: notified 0, pinged 39062-39063(39063)
	Host: notified 39062-39063(39063), pinged 0
	Wall time:1.760000-2.220000(1.789167)

After:
	Using CPUS 0 and 3
	Guest: notified 0, pinged 39037-39063(39062)
	Host: notified 39037-39063(39062), pinged 0
	Wall time:1.640000-1.810000(1.676875)

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 tools/virtio/linux/virtio.h |   10 ++++++++++
 tools/virtio/vringh_test.c  |    8 ++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/tools/virtio/linux/virtio.h b/tools/virtio/linux/virtio.h
index 5fa612a..6df181a 100644
--- a/tools/virtio/linux/virtio.h
+++ b/tools/virtio/linux/virtio.h
@@ -63,6 +63,16 @@ int virtqueue_add_sgs(struct virtqueue *vq,
 		      void *data,
 		      gfp_t gfp);
 
+int virtqueue_add_outbuf(struct virtqueue *vq,
+			 struct scatterlist sg[], unsigned int num,
+			 void *data,
+			 gfp_t gfp);
+
+int virtqueue_add_inbuf(struct virtqueue *vq,
+			struct scatterlist sg[], unsigned int num,
+			void *data,
+			gfp_t gfp);
+
 void virtqueue_kick(struct virtqueue *vq);
 
 void *virtqueue_get_buf(struct virtqueue *vq, unsigned int *len);
diff --git a/tools/virtio/vringh_test.c b/tools/virtio/vringh_test.c
index dd0b989..88fe02a 100644
--- a/tools/virtio/vringh_test.c
+++ b/tools/virtio/vringh_test.c
@@ -369,11 +369,11 @@ static int parallel_test(unsigned long features,
 			 * user addr */
 			__kmalloc_fake = indirects + (xfers % RINGSIZE) * 4;
 			if (output)
-				err = virtqueue_add_buf(vq, sg, num_sg, 0, dbuf,
-							GFP_KERNEL);
+				err = virtqueue_add_outbuf(vq, sg, num_sg, dbuf,
+							   GFP_KERNEL);
 			else
-				err = virtqueue_add_buf(vq, sg, 0, num_sg, dbuf,
-							GFP_KERNEL);
+				err = virtqueue_add_inbuf(vq, sg, num_sg,
+							  dbuf, GFP_KERNEL);
 
 			if (err == -ENOSPC) {
 				if (!virtqueue_enable_cb_delayed(vq))
-- 
1.7.10.4

[PATCH 07/22] virtio-blk: reorganize virtblk_add_req

[Rusty Russell]

From: Paolo Bonzini <pbonzini@redhat.com>

Right now, both virtblk_add_req and virtblk_add_req_wait call
virtqueue_add_buf.  To prepare for the next patches, abstract the call
to virtqueue_add_buf into a new function __virtblk_add_req, and include
the waiting logic directly in virtblk_add_req.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Asias He <asias@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/block/virtio_blk.c |   55 ++++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 35 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 922bcb9..b271650 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -100,50 +100,39 @@ static inline struct virtblk_req *virtblk_alloc_req(struct virtio_blk *vblk,
 	return vbr;
 }
 
-static void virtblk_add_buf_wait(struct virtio_blk *vblk,
-				 struct virtblk_req *vbr,
-				 unsigned long out,
-				 unsigned long in)
+static inline int __virtblk_add_req(struct virtqueue *vq,
+			     struct virtblk_req *vbr,
+			     unsigned long out,
+			     unsigned long in)
 {
+	return virtqueue_add_buf(vq, vbr->sg, out, in, vbr, GFP_ATOMIC);
+}
+
+static void virtblk_add_req(struct virtblk_req *vbr,
+			    unsigned int out, unsigned int in)
+{
+	struct virtio_blk *vblk = vbr->vblk;
 	DEFINE_WAIT(wait);
+	int ret;
 
-	for (;;) {
+	spin_lock_irq(vblk->disk->queue->queue_lock);
+	while (unlikely((ret = __virtblk_add_req(vblk->vq, vbr,
+						 out, in)) < 0)) {
 		prepare_to_wait_exclusive(&vblk->queue_wait, &wait,
 					  TASK_UNINTERRUPTIBLE);
 
+		spin_unlock_irq(vblk->disk->queue->queue_lock);
+		io_schedule();
 		spin_lock_irq(vblk->disk->queue->queue_lock);
-		if (virtqueue_add_buf(vblk->vq, vbr->sg, out, in, vbr,
-				      GFP_ATOMIC) < 0) {
-			spin_unlock_irq(vblk->disk->queue->queue_lock);
-			io_schedule();
-		} else {
-			virtqueue_kick(vblk->vq);
-			spin_unlock_irq(vblk->disk->queue->queue_lock);
-			break;
-		}
 
+		finish_wait(&vblk->queue_wait, &wait);
 	}
 
-	finish_wait(&vblk->queue_wait, &wait);
-}
-
-static inline void virtblk_add_req(struct virtblk_req *vbr,
-				   unsigned int out, unsigned int in)
-{
-	struct virtio_blk *vblk = vbr->vblk;
-
-	spin_lock_irq(vblk->disk->queue->queue_lock);
-	if (unlikely(virtqueue_add_buf(vblk->vq, vbr->sg, out, in, vbr,
-					GFP_ATOMIC) < 0)) {
-		spin_unlock_irq(vblk->disk->queue->queue_lock);
-		virtblk_add_buf_wait(vblk, vbr, out, in);
-		return;
-	}
 	virtqueue_kick(vblk->vq);
 	spin_unlock_irq(vblk->disk->queue->queue_lock);
 }
 
-static int virtblk_bio_send_flush(struct virtblk_req *vbr)
+static void virtblk_bio_send_flush(struct virtblk_req *vbr)
 {
 	unsigned int out = 0, in = 0;
 
@@ -155,11 +144,9 @@ static int virtblk_bio_send_flush(struct virtblk_req *vbr)
 	sg_set_buf(&vbr->sg[out + in++], &vbr->status, sizeof(vbr->status));
 
 	virtblk_add_req(vbr, out, in);
-
-	return 0;
 }
 
-static int virtblk_bio_send_data(struct virtblk_req *vbr)
+static void virtblk_bio_send_data(struct virtblk_req *vbr)
 {
 	struct virtio_blk *vblk = vbr->vblk;
 	unsigned int num, out = 0, in = 0;
@@ -188,8 +175,6 @@ static int virtblk_bio_send_data(struct virtblk_req *vbr)
 	}
 
 	virtblk_add_req(vbr, out, in);
-
-	return 0;
 }
 
 static void virtblk_bio_send_data_work(struct work_struct *work)
-- 
1.7.10.4

[PATCH 08/22] virtio-blk: use virtqueue_add_sgs on bio path

[Rusty Russell]

From: Paolo Bonzini <pbonzini@redhat.com>

(This is a respin of Paolo Bonzini's patch, but it calls
virtqueue_add_sgs() instead of his multi-part API).

Move the creation of the request header and response footer to
__virtblk_add_req.  vbr->sg only contains the data scatterlist,
the header/footer are added separately using virtqueue_add_sgs().

With this change, virtio-blk (with use_bio) is not relying anymore on
the virtio functions ignoring the end markers in a scatterlist.
The next patch will do the same for the other path.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Asias He <asias@redhat.com>
---
 drivers/block/virtio_blk.c |   58 ++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index b271650..cfbe39d 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -62,6 +62,7 @@ struct virtblk_req
 	struct virtio_blk *vblk;
 	int flags;
 	u8 status;
+	int nents;
 	struct scatterlist sg[];
 };
 
@@ -100,24 +101,36 @@ static inline struct virtblk_req *virtblk_alloc_req(struct virtio_blk *vblk,
 	return vbr;
 }
 
-static inline int __virtblk_add_req(struct virtqueue *vq,
-			     struct virtblk_req *vbr,
-			     unsigned long out,
-			     unsigned long in)
+static int __virtblk_add_req(struct virtqueue *vq,
+			     struct virtblk_req *vbr)
 {
-	return virtqueue_add_buf(vq, vbr->sg, out, in, vbr, GFP_ATOMIC);
+	struct scatterlist hdr, status, *sgs[3];
+	unsigned int num_out = 0, num_in = 0;
+
+	sg_init_one(&hdr, &vbr->out_hdr, sizeof(vbr->out_hdr));
+	sgs[num_out++] = &hdr;
+
+	if (vbr->nents) {
+		if (vbr->out_hdr.type & VIRTIO_BLK_T_OUT)
+			sgs[num_out++] = vbr->sg;
+		else
+			sgs[num_out + num_in++] = vbr->sg;
+	}
+
+	sg_init_one(&status, &vbr->status, sizeof(vbr->status));
+	sgs[num_out + num_in++] = &status;
+
+	return virtqueue_add_sgs(vq, sgs, num_out, num_in, vbr, GFP_ATOMIC);
 }
 
-static void virtblk_add_req(struct virtblk_req *vbr,
-			    unsigned int out, unsigned int in)
+static void virtblk_add_req(struct virtblk_req *vbr)
 {
 	struct virtio_blk *vblk = vbr->vblk;
 	DEFINE_WAIT(wait);
 	int ret;
 
 	spin_lock_irq(vblk->disk->queue->queue_lock);
-	while (unlikely((ret = __virtblk_add_req(vblk->vq, vbr,
-						 out, in)) < 0)) {
+	while (unlikely((ret = __virtblk_add_req(vblk->vq, vbr)) < 0)) {
 		prepare_to_wait_exclusive(&vblk->queue_wait, &wait,
 					  TASK_UNINTERRUPTIBLE);
 
@@ -134,22 +147,18 @@ static void virtblk_add_req(struct virtblk_req *vbr,
 
 static void virtblk_bio_send_flush(struct virtblk_req *vbr)
 {
-	unsigned int out = 0, in = 0;
-
 	vbr->flags |= VBLK_IS_FLUSH;
 	vbr->out_hdr.type = VIRTIO_BLK_T_FLUSH;
 	vbr->out_hdr.sector = 0;
 	vbr->out_hdr.ioprio = 0;
-	sg_set_buf(&vbr->sg[out++], &vbr->out_hdr, sizeof(vbr->out_hdr));
-	sg_set_buf(&vbr->sg[out + in++], &vbr->status, sizeof(vbr->status));
+	vbr->nents = 0;
 
-	virtblk_add_req(vbr, out, in);
+	virtblk_add_req(vbr);
 }
 
 static void virtblk_bio_send_data(struct virtblk_req *vbr)
 {
 	struct virtio_blk *vblk = vbr->vblk;
-	unsigned int num, out = 0, in = 0;
 	struct bio *bio = vbr->bio;
 
 	vbr->flags &= ~VBLK_IS_FLUSH;
@@ -157,24 +166,15 @@ static void virtblk_bio_send_data(struct virtblk_req *vbr)
 	vbr->out_hdr.sector = bio->bi_sector;
 	vbr->out_hdr.ioprio = bio_prio(bio);
 
-	sg_set_buf(&vbr->sg[out++], &vbr->out_hdr, sizeof(vbr->out_hdr));
-
-	num = blk_bio_map_sg(vblk->disk->queue, bio, vbr->sg + out);
-
-	sg_set_buf(&vbr->sg[num + out + in++], &vbr->status,
-		   sizeof(vbr->status));
-
-	if (num) {
-		if (bio->bi_rw & REQ_WRITE) {
+	vbr->nents = blk_bio_map_sg(vblk->disk->queue, bio, vbr->sg);
+	if (vbr->nents) {
+		if (bio->bi_rw & REQ_WRITE)
 			vbr->out_hdr.type |= VIRTIO_BLK_T_OUT;
-			out += num;
-		} else {
+		else
 			vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
-			in += num;
-		}
 	}
 
-	virtblk_add_req(vbr, out, in);
+	virtblk_add_req(vbr);
 }
 
 static void virtblk_bio_send_data_work(struct work_struct *work)
-- 
1.7.10.4

[PATCH 09/22] virtio-blk: use virtqueue_add_sgs on req path

[Rusty Russell]

From: Paolo Bonzini <pbonzini@redhat.com>

(This is a respin of Paolo Bonzini's patch, but it calls
virtqueue_add_sgs() instead of his multi-part API).

This is similar to the previous patch, but a bit more radical
because the bio and req paths now share the buffer construction
code.  Because the req path doesn't use vbr->sg, however, we
need to add a couple of arguments to __virtblk_add_req.

We also need to teach __virtblk_add_req how to build SCSI command
requests.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Asias He <asias@redhat.com>
---
 drivers/block/virtio_blk.c |   69 +++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 36 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index cfbe39d..cc88b29 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -102,19 +102,40 @@ static inline struct virtblk_req *virtblk_alloc_req(struct virtio_blk *vblk,
 }
 
 static int __virtblk_add_req(struct virtqueue *vq,
-			     struct virtblk_req *vbr)
+			     struct virtblk_req *vbr,
+			     struct scatterlist *data_sg,
+			     unsigned data_nents)
 {
-	struct scatterlist hdr, status, *sgs[3];
+	struct scatterlist hdr, status, cmd, sense, inhdr, *sgs[6];
 	unsigned int num_out = 0, num_in = 0;
+	int type = vbr->out_hdr.type & ~VIRTIO_BLK_T_OUT;
 
 	sg_init_one(&hdr, &vbr->out_hdr, sizeof(vbr->out_hdr));
 	sgs[num_out++] = &hdr;
 
-	if (vbr->nents) {
+	/*
+	 * If this is a packet command we need a couple of additional headers.
+	 * Behind the normal outhdr we put a segment with the scsi command
+	 * block, and before the normal inhdr we put the sense data and the
+	 * inhdr with additional status information.
+	 */
+	if (type == VIRTIO_BLK_T_SCSI_CMD) {
+		sg_init_one(&cmd, vbr->req->cmd, vbr->req->cmd_len);
+		sgs[num_out++] = &cmd;
+	}
+
+	if (data_nents) {
 		if (vbr->out_hdr.type & VIRTIO_BLK_T_OUT)
-			sgs[num_out++] = vbr->sg;
+			sgs[num_out++] = data_sg;
 		else
-			sgs[num_out + num_in++] = vbr->sg;
+			sgs[num_out + num_in++] = data_sg;
+	}
+
+	if (type == VIRTIO_BLK_T_SCSI_CMD) {
+		sg_init_one(&sense, vbr->req->sense, SCSI_SENSE_BUFFERSIZE);
+		sgs[num_out + num_in++] = &sense;
+		sg_init_one(&inhdr, &vbr->in_hdr, sizeof(vbr->in_hdr));
+		sgs[num_out + num_in++] = &inhdr;
 	}
 
 	sg_init_one(&status, &vbr->status, sizeof(vbr->status));
@@ -130,7 +151,8 @@ static void virtblk_add_req(struct virtblk_req *vbr)
 	int ret;
 
 	spin_lock_irq(vblk->disk->queue->queue_lock);
-	while (unlikely((ret = __virtblk_add_req(vblk->vq, vbr)) < 0)) {
+	while (unlikely((ret = __virtblk_add_req(vblk->vq, vbr, vbr->sg,
+						 vbr->nents)) < 0)) {
 		prepare_to_wait_exclusive(&vblk->queue_wait, &wait,
 					  TASK_UNINTERRUPTIBLE);
 
@@ -283,7 +305,7 @@ static void virtblk_done(struct virtqueue *vq)
 static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
 		   struct request *req)
 {
-	unsigned long num, out = 0, in = 0;
+	unsigned int num;
 	struct virtblk_req *vbr;
 
 	vbr = virtblk_alloc_req(vblk, GFP_ATOMIC);
@@ -320,40 +342,15 @@ static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
 		}
 	}
 
-	sg_set_buf(&vblk->sg[out++], &vbr->out_hdr, sizeof(vbr->out_hdr));
-
-	/*
-	 * If this is a packet command we need a couple of additional headers.
-	 * Behind the normal outhdr we put a segment with the scsi command
-	 * block, and before the normal inhdr we put the sense data and the
-	 * inhdr with additional status information before the normal inhdr.
-	 */
-	if (vbr->req->cmd_type == REQ_TYPE_BLOCK_PC)
-		sg_set_buf(&vblk->sg[out++], vbr->req->cmd, vbr->req->cmd_len);
-
-	num = blk_rq_map_sg(q, vbr->req, vblk->sg + out);
-
-	if (vbr->req->cmd_type == REQ_TYPE_BLOCK_PC) {
-		sg_set_buf(&vblk->sg[num + out + in++], vbr->req->sense, SCSI_SENSE_BUFFERSIZE);
-		sg_set_buf(&vblk->sg[num + out + in++], &vbr->in_hdr,
-			   sizeof(vbr->in_hdr));
-	}
-
-	sg_set_buf(&vblk->sg[num + out + in++], &vbr->status,
-		   sizeof(vbr->status));
-
+	num = blk_rq_map_sg(q, vbr->req, vblk->sg);
 	if (num) {
-		if (rq_data_dir(vbr->req) == WRITE) {
+		if (rq_data_dir(vbr->req) == WRITE)
 			vbr->out_hdr.type |= VIRTIO_BLK_T_OUT;
-			out += num;
-		} else {
+		else
 			vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
-			in += num;
-		}
 	}
 
-	if (virtqueue_add_buf(vblk->vq, vblk->sg, out, in, vbr,
-			      GFP_ATOMIC) < 0) {
+	if (__virtblk_add_req(vblk->vq, vbr, vblk->sg, num) < 0) {
 		mempool_free(vbr, vblk->pool);
 		return false;
 	}
-- 
1.7.10.4

[PATCH 10/22] virtio_blk: remove nents member.

[Rusty Russell]

It's simply a flag as to whether we have data now, so make it an
explicit function parameter rather than a member of struct
virtblk_req.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Asias He <asias@redhat.com>
---
 drivers/block/virtio_blk.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index cc88b29..6472395 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -62,7 +62,6 @@ struct virtblk_req
 	struct virtio_blk *vblk;
 	int flags;
 	u8 status;
-	int nents;
 	struct scatterlist sg[];
 };
 
@@ -104,7 +103,7 @@ static inline struct virtblk_req *virtblk_alloc_req(struct virtio_blk *vblk,
 static int __virtblk_add_req(struct virtqueue *vq,
 			     struct virtblk_req *vbr,
 			     struct scatterlist *data_sg,
-			     unsigned data_nents)
+			     bool have_data)
 {
 	struct scatterlist hdr, status, cmd, sense, inhdr, *sgs[6];
 	unsigned int num_out = 0, num_in = 0;
@@ -124,7 +123,7 @@ static int __virtblk_add_req(struct virtqueue *vq,
 		sgs[num_out++] = &cmd;
 	}
 
-	if (data_nents) {
+	if (have_data) {
 		if (vbr->out_hdr.type & VIRTIO_BLK_T_OUT)
 			sgs[num_out++] = data_sg;
 		else
@@ -144,7 +143,7 @@ static int __virtblk_add_req(struct virtqueue *vq,
 	return virtqueue_add_sgs(vq, sgs, num_out, num_in, vbr, GFP_ATOMIC);
 }
 
-static void virtblk_add_req(struct virtblk_req *vbr)
+static void virtblk_add_req(struct virtblk_req *vbr, bool have_data)
 {
 	struct virtio_blk *vblk = vbr->vblk;
 	DEFINE_WAIT(wait);
@@ -152,7 +151,7 @@ static void virtblk_add_req(struct virtblk_req *vbr)
 
 	spin_lock_irq(vblk->disk->queue->queue_lock);
 	while (unlikely((ret = __virtblk_add_req(vblk->vq, vbr, vbr->sg,
-						 vbr->nents)) < 0)) {
+						 have_data)) < 0)) {
 		prepare_to_wait_exclusive(&vblk->queue_wait, &wait,
 					  TASK_UNINTERRUPTIBLE);
 
@@ -173,30 +172,31 @@ static void virtblk_bio_send_flush(struct virtblk_req *vbr)
 	vbr->out_hdr.type = VIRTIO_BLK_T_FLUSH;
 	vbr->out_hdr.sector = 0;
 	vbr->out_hdr.ioprio = 0;
-	vbr->nents = 0;
 
-	virtblk_add_req(vbr);
+	virtblk_add_req(vbr, false);
 }
 
 static void virtblk_bio_send_data(struct virtblk_req *vbr)
 {
 	struct virtio_blk *vblk = vbr->vblk;
 	struct bio *bio = vbr->bio;
+	bool have_data;
 
 	vbr->flags &= ~VBLK_IS_FLUSH;
 	vbr->out_hdr.type = 0;
 	vbr->out_hdr.sector = bio->bi_sector;
 	vbr->out_hdr.ioprio = bio_prio(bio);
 
-	vbr->nents = blk_bio_map_sg(vblk->disk->queue, bio, vbr->sg);
-	if (vbr->nents) {
+	if (blk_bio_map_sg(vblk->disk->queue, bio, vbr->sg)) {
+		have_data = true;
 		if (bio->bi_rw & REQ_WRITE)
 			vbr->out_hdr.type |= VIRTIO_BLK_T_OUT;
 		else
 			vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
-	}
+	} else
+		have_data = false;
 
-	virtblk_add_req(vbr);
+	virtblk_add_req(vbr, have_data);
 }
 
 static void virtblk_bio_send_data_work(struct work_struct *work)
-- 
1.7.10.4

[PATCH 11/22] virtio-scsi: use virtqueue_add_sgs for command buffers

[Rusty Russell]

From: Wanlong Gao <gaowanlong@cn.fujitsu.com>

Using the new virtqueue_add_sgs function lets us simplify the queueing
path.  In particular, all data protected by the tgt_lock is just gone
(multiqueue will find a new use for the lock).

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Asias He <asias@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/scsi/virtio_scsi.c |   91 +++++++++++++++++---------------------------
 1 file changed, 34 insertions(+), 57 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 0f5dd28..bb67576 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -61,11 +61,8 @@ struct virtio_scsi_vq {
 
 /* Per-target queue state */
 struct virtio_scsi_target_state {
-	/* Protects sg.  Lock hierarchy is tgt_lock -> vq_lock.  */
+	/* Never held at the same time as vq_lock.  */
 	spinlock_t tgt_lock;
-
-	/* For sglist construction when adding commands to the virtqueue.  */
-	struct scatterlist sg[];
 };
 
 /* Driver instance state */
@@ -353,75 +350,61 @@ static void virtscsi_event_done(struct virtqueue *vq)
 	spin_unlock_irqrestore(&vscsi->event_vq.vq_lock, flags);
 };
 
-static void virtscsi_map_sgl(struct scatterlist *sg, unsigned int *p_idx,
-			     struct scsi_data_buffer *sdb)
-{
-	struct sg_table *table = &sdb->table;
-	struct scatterlist *sg_elem;
-	unsigned int idx = *p_idx;
-	int i;
-
-	for_each_sg(table->sgl, sg_elem, table->nents, i)
-		sg[idx++] = *sg_elem;
-
-	*p_idx = idx;
-}
-
 /**
- * virtscsi_map_cmd - map a scsi_cmd to a virtqueue scatterlist
- * @vscsi	: virtio_scsi state
+ * virtscsi_add_cmd - add a virtio_scsi_cmd to a virtqueue
+ * @vq		: the struct virtqueue we're talking about
  * @cmd		: command structure
- * @out_num	: number of read-only elements
- * @in_num	: number of write-only elements
  * @req_size	: size of the request buffer
  * @resp_size	: size of the response buffer
- *
- * Called with tgt_lock held.
+ * @gfp	: flags to use for memory allocations
  */
-static void virtscsi_map_cmd(struct virtio_scsi_target_state *tgt,
-			     struct virtio_scsi_cmd *cmd,
-			     unsigned *out_num, unsigned *in_num,
-			     size_t req_size, size_t resp_size)
+static int virtscsi_add_cmd(struct virtqueue *vq,
+			    struct virtio_scsi_cmd *cmd,
+			    size_t req_size, size_t resp_size, gfp_t gfp)
 {
 	struct scsi_cmnd *sc = cmd->sc;
-	struct scatterlist *sg = tgt->sg;
-	unsigned int idx = 0;
+	struct scatterlist *sgs[4], req, resp;
+	struct sg_table *out, *in;
+	unsigned out_num = 0, in_num = 0;
+
+	out = in = NULL;
+
+	if (sc && sc->sc_data_direction != DMA_NONE) {
+		if (sc->sc_data_direction != DMA_FROM_DEVICE)
+			out = &scsi_out(sc)->table;
+		if (sc->sc_data_direction != DMA_TO_DEVICE)
+			in = &scsi_in(sc)->table;
+	}
 
 	/* Request header.  */
-	sg_set_buf(&sg[idx++], &cmd->req, req_size);
+	sg_init_one(&req, &cmd->req, req_size);
+	sgs[out_num++] = &req;
 
 	/* Data-out buffer.  */
-	if (sc && sc->sc_data_direction != DMA_FROM_DEVICE)
-		virtscsi_map_sgl(sg, &idx, scsi_out(sc));
-
-	*out_num = idx;
+	if (out)
+		sgs[out_num++] = out->sgl;
 
 	/* Response header.  */
-	sg_set_buf(&sg[idx++], &cmd->resp, resp_size);
+	sg_init_one(&resp, &cmd->resp, resp_size);
+	sgs[out_num + in_num++] = &resp;
 
 	/* Data-in buffer */
-	if (sc && sc->sc_data_direction != DMA_TO_DEVICE)
-		virtscsi_map_sgl(sg, &idx, scsi_in(sc));
+	if (in)
+		sgs[out_num + in_num++] = in->sgl;
 
-	*in_num = idx - *out_num;
+	return virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, gfp);
 }
 
-static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
-			     struct virtio_scsi_vq *vq,
+static int virtscsi_kick_cmd(struct virtio_scsi_vq *vq,
 			     struct virtio_scsi_cmd *cmd,
 			     size_t req_size, size_t resp_size, gfp_t gfp)
 {
-	unsigned int out_num, in_num;
 	unsigned long flags;
 	int err;
 	bool needs_kick = false;
 
-	spin_lock_irqsave(&tgt->tgt_lock, flags);
-	virtscsi_map_cmd(tgt, cmd, &out_num, &in_num, req_size, resp_size);
-
-	spin_lock(&vq->vq_lock);
-	err = virtqueue_add_buf(vq->vq, tgt->sg, out_num, in_num, cmd, gfp);
-	spin_unlock(&tgt->tgt_lock);
+	spin_lock_irqsave(&vq->vq_lock, flags);
+	err = virtscsi_add_cmd(vq->vq, cmd, req_size, resp_size, gfp);
 	if (!err)
 		needs_kick = virtqueue_kick_prepare(vq->vq);
 
@@ -435,7 +418,6 @@ static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
 static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
 {
 	struct virtio_scsi *vscsi = shost_priv(sh);
-	struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
 	struct virtio_scsi_cmd *cmd;
 	int ret;
 
@@ -469,7 +451,7 @@ static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
 	BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
 	memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
 
-	if (virtscsi_kick_cmd(tgt, &vscsi->req_vq, cmd,
+	if (virtscsi_kick_cmd(&vscsi->req_vq, cmd,
 			      sizeof cmd->req.cmd, sizeof cmd->resp.cmd,
 			      GFP_ATOMIC) == 0)
 		ret = 0;
@@ -483,11 +465,10 @@ out:
 static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
 {
 	DECLARE_COMPLETION_ONSTACK(comp);
-	struct virtio_scsi_target_state *tgt = vscsi->tgt[cmd->sc->device->id];
 	int ret = FAILED;
 
 	cmd->comp = &comp;
-	if (virtscsi_kick_cmd(tgt, &vscsi->ctrl_vq, cmd,
+	if (virtscsi_kick_cmd(&vscsi->ctrl_vq, cmd,
 			      sizeof cmd->req.tmf, sizeof cmd->resp.tmf,
 			      GFP_NOIO) < 0)
 		goto out;
@@ -593,15 +574,11 @@ static struct virtio_scsi_target_state *virtscsi_alloc_tgt(
 	struct virtio_scsi_target_state *tgt;
 	gfp_t gfp_mask = GFP_KERNEL;
 
-	/* We need extra sg elements at head and tail.  */
-	tgt = kmalloc(sizeof(*tgt) + sizeof(tgt->sg[0]) * (sg_elems + 2),
-		      gfp_mask);
-
+	tgt = kmalloc(sizeof(*tgt), gfp_mask);
 	if (!tgt)
 		return NULL;
 
 	spin_lock_init(&tgt->tgt_lock);
-	sg_init_table(tgt->sg, sg_elems + 2);
 	return tgt;
 }
 
-- 
1.7.10.4

[PATCH 12/22] virtio_scsi: use virtqueue_add_inbuf() for virtscsi_kick_event.

[Rusty Russell]

It's a bit clearer, and add_buf is going away.

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

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index bb67576..f679b8c 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -222,8 +222,8 @@ static int virtscsi_kick_event(struct virtio_scsi *vscsi,
 
 	spin_lock_irqsave(&vscsi->event_vq.vq_lock, flags);
 
-	err = virtqueue_add_buf(vscsi->event_vq.vq, &sg, 0, 1, event_node,
-				GFP_ATOMIC);
+	err = virtqueue_add_inbuf(vscsi->event_vq.vq, &sg, 1, event_node,
+				  GFP_ATOMIC);
 	if (!err)
 		virtqueue_kick(vscsi->event_vq.vq);
 
-- 
1.7.10.4

[PATCH 13/22] virtio_net: use virtqueue_add_sgs[] for command buffers.

[Rusty Russell]

It's a bit cleaner to hand multiple sgs, rather than one big one.

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

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 57ac4b0..f19865a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -39,7 +39,6 @@ module_param(gso, bool, 0444);
 #define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
 #define GOOD_COPY_LEN	128
 
-#define VIRTNET_SEND_COMMAND_SG_MAX    2
 #define VIRTNET_DRIVER_VERSION "1.0.0"
 
 struct virtnet_stats {
@@ -767,32 +766,34 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  * never fail unless improperly formated.
  */
 static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
-				 struct scatterlist *data, int out, int in)
+				 struct scatterlist *out,
+				 struct scatterlist *in)
 {
-	struct scatterlist *s, sg[VIRTNET_SEND_COMMAND_SG_MAX + 2];
+	struct scatterlist *sgs[4], hdr, stat;
 	struct virtio_net_ctrl_hdr ctrl;
 	virtio_net_ctrl_ack status = ~0;
-	unsigned int tmp;
-	int i;
+	unsigned out_num = 0, in_num = 0, tmp;
 
 	/* Caller should know better */
-	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
-		(out + in > VIRTNET_SEND_COMMAND_SG_MAX));
-
-	out++; /* Add header */
-	in++; /* Add return status */
+	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
 
 	ctrl.class = class;
 	ctrl.cmd = cmd;
+	/* Add header */
+	sg_init_one(&hdr, &ctrl, sizeof(ctrl));
+	sgs[out_num++] = &hdr;
 
-	sg_init_table(sg, out + in);
+	if (out)
+		sgs[out_num++] = out;
+	if (in)
+		sgs[out_num + in_num++] = in;
 
-	sg_set_buf(&sg[0], &ctrl, sizeof(ctrl));
-	for_each_sg(data, s, out + in - 2, i)
-		sg_set_buf(&sg[i + 1], sg_virt(s), s->length);
-	sg_set_buf(&sg[out + in - 1], &status, sizeof(status));
+	/* Add return status. */
+	sg_init_one(&stat, &status, sizeof(status));
+	sgs[out_num + in_num++] = &hdr;
 
-	BUG_ON(virtqueue_add_buf(vi->cvq, sg, out, in, vi, GFP_ATOMIC) < 0);
+	BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC)
+	       < 0);
 
 	virtqueue_kick(vi->cvq);
 
@@ -821,7 +822,7 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
 		sg_init_one(&sg, addr->sa_data, dev->addr_len);
 		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
 					  VIRTIO_NET_CTRL_MAC_ADDR_SET,
-					  &sg, 1, 0)) {
+					  &sg, NULL)) {
 			dev_warn(&vdev->dev,
 				 "Failed to set mac address by vq command.\n");
 			return -EINVAL;
@@ -889,8 +890,7 @@ static void virtnet_ack_link_announce(struct virtnet_info *vi)
 {
 	rtnl_lock();
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
-				  VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL,
-				  0, 0))
+				  VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, NULL))
 		dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
 	rtnl_unlock();
 }
@@ -908,7 +908,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
 	sg_init_one(&sg, &s, sizeof(s));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
-				  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, 1, 0)){
+				  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, NULL)) {
 		dev_warn(&dev->dev, "Fail to set num of queue pairs to %d\n",
 			 queue_pairs);
 		return -EINVAL;
@@ -955,7 +955,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_PROMISC,
-				  sg, 1, 0))
+				  sg, NULL))
 		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
 			 promisc ? "en" : "dis");
 
@@ -963,7 +963,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_ALLMULTI,
-				  sg, 1, 0))
+				  sg, NULL))
 		dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
 			 allmulti ? "en" : "dis");
 
@@ -1000,7 +1000,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
 				  VIRTIO_NET_CTRL_MAC_TABLE_SET,
-				  sg, 2, 0))
+				  sg, NULL))
 		dev_warn(&dev->dev, "Failed to set MAC fitler table.\n");
 
 	kfree(buf);
@@ -1014,7 +1014,7 @@ static int virtnet_vlan_rx_add_vid(struct net_device *dev, u16 vid)
 	sg_init_one(&sg, &vid, sizeof(vid));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
-				  VIRTIO_NET_CTRL_VLAN_ADD, &sg, 1, 0))
+				  VIRTIO_NET_CTRL_VLAN_ADD, &sg, NULL))
 		dev_warn(&dev->dev, "Failed to add VLAN ID %d.\n", vid);
 	return 0;
 }
@@ -1027,7 +1027,7 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
 	sg_init_one(&sg, &vid, sizeof(vid));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
-				  VIRTIO_NET_CTRL_VLAN_DEL, &sg, 1, 0))
+				  VIRTIO_NET_CTRL_VLAN_DEL, &sg, NULL))
 		dev_warn(&dev->dev, "Failed to kill VLAN ID %d.\n", vid);
 	return 0;
 }
-- 
1.7.10.4

[PATCH 14/22] virtio_net: use simplified virtqueue accessors.

[Rusty Russell]

We never add buffers with input and output parts, so use the new accessors.

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

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f19865a..38c01d9 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -443,7 +443,7 @@ static int add_recvbuf_small(struct receive_queue *rq, gfp_t gfp)
 
 	skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
 
-	err = virtqueue_add_buf(rq->vq, rq->sg, 0, 2, skb, gfp);
+	err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp);
 	if (err < 0)
 		dev_kfree_skb(skb);
 
@@ -488,8 +488,8 @@ static int add_recvbuf_big(struct receive_queue *rq, gfp_t gfp)
 
 	/* chain first in list head */
 	first->private = (unsigned long)list;
-	err = virtqueue_add_buf(rq->vq, rq->sg, 0, MAX_SKB_FRAGS + 2,
-				first, gfp);
+	err = virtqueue_add_inbuf(rq->vq, rq->sg, MAX_SKB_FRAGS + 2,
+				  first, gfp);
 	if (err < 0)
 		give_pages(rq, first);
 
@@ -507,7 +507,7 @@ static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
 
 	sg_init_one(rq->sg, page_address(page), PAGE_SIZE);
 
-	err = virtqueue_add_buf(rq->vq, rq->sg, 0, 1, page, gfp);
+	err = virtqueue_add_inbuf(rq->vq, rq->sg, 1, page, gfp);
 	if (err < 0)
 		give_pages(rq, page);
 
@@ -710,8 +710,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 		sg_set_buf(sq->sg, &hdr->hdr, sizeof hdr->hdr);
 
 	num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len) + 1;
-	return virtqueue_add_buf(sq->vq, sq->sg, num_sg,
-				 0, skb, GFP_ATOMIC);
+	return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
 }
 
 static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
-- 
1.7.10.4

[PATCH 15/22] virtio_rng: use simplified virtqueue accessors.

[Rusty Russell]

We never add buffers with input and output parts, so use the new accessors.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/char/hw_random/virtio-rng.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 6bf4d47..ef46a9c 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -47,7 +47,7 @@ static void register_buffer(u8 *buf, size_t size)
 	sg_init_one(&sg, buf, size);
 
 	/* There should always be room for one buffer. */
-	if (virtqueue_add_buf(vq, &sg, 0, 1, buf, GFP_KERNEL) < 0)
+	if (virtqueue_add_inbuf(vq, &sg, 1, buf, GFP_KERNEL) < 0)
 		BUG();
 
 	virtqueue_kick(vq);
-- 
1.7.10.4

[PATCH 16/22] virtio_console: use simplified virtqueue accessors.

[Rusty Russell]

We never add buffers with input and output parts, so use the new accessors.

Cc: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/char/virtio_console.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e6ba6b7..e9f35c5 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -502,7 +502,7 @@ static int add_inbuf(struct virtqueue *vq, struct port_buffer *buf)
 
 	sg_init_one(sg, buf->buf, buf->size);
 
-	ret = virtqueue_add_buf(vq, sg, 0, 1, buf, GFP_ATOMIC);
+	ret = virtqueue_add_inbuf(vq, sg, 1, buf, GFP_ATOMIC);
 	virtqueue_kick(vq);
 	if (!ret)
 		ret = vq->num_free;
@@ -569,7 +569,7 @@ static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
 	vq = portdev->c_ovq;
 
 	sg_init_one(sg, &cpkt, sizeof(cpkt));
-	if (virtqueue_add_buf(vq, sg, 1, 0, &cpkt, GFP_ATOMIC) == 0) {
+	if (virtqueue_add_outbuf(vq, sg, 1, &cpkt, GFP_ATOMIC) == 0) {
 		virtqueue_kick(vq);
 		while (!virtqueue_get_buf(vq, &len))
 			cpu_relax();
@@ -618,7 +618,7 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 
 	reclaim_consumed_buffers(port);
 
-	err = virtqueue_add_buf(out_vq, sg, nents, 0, data, GFP_ATOMIC);
+	err = virtqueue_add_outbuf(out_vq, sg, nents, data, GFP_ATOMIC);
 
 	/* Tell Host to go! */
 	virtqueue_kick(out_vq);
-- 
1.7.10.4

[PATCH 17/22] caif_virtio: use simplified virtqueue accessors.

[Rusty Russell]

We never add buffers with input and output parts, so use the new accessors.

Cc: Sjur Brendeland <sjur.brandeland@stericsson.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/caif/caif_virtio.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c
index 8308dee..36d9af5 100644
--- a/drivers/net/caif/caif_virtio.c
+++ b/drivers/net/caif/caif_virtio.c
@@ -573,8 +573,7 @@ static int cfv_netdev_tx(struct sk_buff *skb, struct net_device *netdev)
 		goto err;
 	}
 
-	ret = virtqueue_add_buf(cfv->vq_tx, &sg, 1, 0,
-				buf_info, GFP_ATOMIC);
+	ret = virtqueue_add_outbuf(cfv->vq_tx, &sg, 1, buf_info, GFP_ATOMIC);
 	if (unlikely((ret < 0))) {
 		/* If flow control works, this shouldn't happen */
 		netdev_warn(cfv->ndev, "Failed adding buffer to TX vring:%d\n",
-- 
1.7.10.4

[PATCH 18/22] virtio_rpmsg_bus: use simplified virtqueue accessors.

[Rusty Russell]

We never add buffers with input and output parts, so use the new accessors.

Cc: Ohad Ben-Cohen <ohad@wizery.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/rpmsg/virtio_rpmsg_bus.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index a59684b..33d827b 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -757,14 +757,14 @@ int rpmsg_send_offchannel_raw(struct rpmsg_channel *rpdev, u32 src, u32 dst,
 	mutex_lock(&vrp->tx_lock);
 
 	/* add message to the remote processor's virtqueue */
-	err = virtqueue_add_buf(vrp->svq, &sg, 1, 0, msg, GFP_KERNEL);
+	err = virtqueue_add_outbuf(vrp->svq, &sg, 1, msg, GFP_KERNEL);
 	if (err) {
 		/*
 		 * need to reclaim the buffer here, otherwise it's lost
 		 * (memory won't leak, but rpmsg won't use it again for TX).
 		 * this will wait for a buffer management overhaul.
 		 */
-		dev_err(dev, "virtqueue_add_buf failed: %d\n", err);
+		dev_err(dev, "virtqueue_add_outbuf failed: %d\n", err);
 		goto out;
 	}
 
@@ -839,7 +839,7 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
 	sg_init_one(&sg, msg, RPMSG_BUF_SIZE);
 
 	/* add the buffer back to the remote processor's virtqueue */
-	err = virtqueue_add_buf(vrp->rvq, &sg, 0, 1, msg, GFP_KERNEL);
+	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
 	if (err < 0) {
 		dev_err(dev, "failed to add a virtqueue buffer: %d\n", err);
 		return;
@@ -970,7 +970,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 
 		sg_init_one(&sg, cpu_addr, RPMSG_BUF_SIZE);
 
-		err = virtqueue_add_buf(vrp->rvq, &sg, 0, 1, cpu_addr,
+		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
 								GFP_KERNEL);
 		WARN_ON(err); /* sanity check; this can't really happen */
 	}
-- 
1.7.10.4

[PATCH 19/22] virtio_balloon: use simplified virtqueue accessors.

[Rusty Russell]

We never add buffers with input and output parts, so use the new accessors.

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

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 8dab163..bd3ae32 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -108,7 +108,7 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
 	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
 
 	/* We should always be able to add one buffer to an empty queue. */
-	if (virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL) < 0)
+	if (virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL) < 0)
 		BUG();
 	virtqueue_kick(vq);
 
@@ -256,7 +256,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
 	if (!virtqueue_get_buf(vq, &len))
 		return;
 	sg_init_one(&sg, vb->stats, sizeof(vb->stats));
-	if (virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL) < 0)
+	if (virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL) < 0)
 		BUG();
 	virtqueue_kick(vq);
 }
@@ -341,7 +341,7 @@ static int init_vqs(struct virtio_balloon *vb)
 		 * use it to signal us later.
 		 */
 		sg_init_one(&sg, vb->stats, sizeof vb->stats);
-		if (virtqueue_add_buf(vb->stats_vq, &sg, 1, 0, vb, GFP_KERNEL)
+		if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
 		    < 0)
 			BUG();
 		virtqueue_kick(vb->stats_vq);
-- 
1.7.10.4

[PATCH 20/22] 9p/trans_virtio.c: use virtio_add_sgs[]

[Rusty Russell]

virtio_add_buf() is going away, replaced with virtio_add_sgs() which
takes multiple terminated scatterlists.

Cc: Eric Van Hensbergen <ericvh@gmail.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 net/9p/trans_virtio.c |   48 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index de2e950..6cb9abe 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -194,11 +194,14 @@ static int pack_sg_list(struct scatterlist *sg, int start,
 		if (s > count)
 			s = count;
 		BUG_ON(index > limit);
+		/* Make sure we don't terminate early. */
+		sg_unmark_end(&sg[index]);
 		sg_set_buf(&sg[index++], data, s);
 		count -= s;
 		data += s;
 	}
-
+	if (index-start)
+		sg_mark_end(&sg[index - 1]);
 	return index-start;
 }
 
@@ -236,12 +239,17 @@ pack_sg_list_p(struct scatterlist *sg, int start, int limit,
 		s = rest_of_page(data);
 		if (s > count)
 			s = count;
+		/* Make sure we don't terminate early. */
+		sg_unmark_end(&sg[index]);
 		sg_set_page(&sg[index++], pdata[i++], s, data_off);
 		data_off = 0;
 		data += s;
 		count -= s;
 		nr_pages--;
 	}
+
+	if (index-start)
+		sg_mark_end(&sg[index - 1]);
 	return index - start;
 }
 
@@ -256,9 +264,10 @@ static int
 p9_virtio_request(struct p9_client *client, struct p9_req_t *req)
 {
 	int err;
-	int in, out;
+	int in, out, out_sgs, in_sgs;
 	unsigned long flags;
 	struct virtio_chan *chan = client->trans;
+	struct scatterlist *sgs[2];
 
 	p9_debug(P9_DEBUG_TRANS, "9p debug: virtio request\n");
 
@@ -266,15 +275,21 @@ p9_virtio_request(struct p9_client *client, struct p9_req_t *req)
 req_retry:
 	spin_lock_irqsave(&chan->lock, flags);
 
+	out_sgs = in_sgs = 0;
 	/* Handle out VirtIO ring buffers */
 	out = pack_sg_list(chan->sg, 0,
 			   VIRTQUEUE_NUM, req->tc->sdata, req->tc->size);
+	if (out)
+		sgs[out_sgs++] = chan->sg;
 
 	in = pack_sg_list(chan->sg, out,
 			  VIRTQUEUE_NUM, req->rc->sdata, req->rc->capacity);
+	if (in)
+		sgs[out_sgs + in_sgs++] = chan->sg + out;
 
-	err = virtqueue_add_buf(chan->vq, chan->sg, out, in, req->tc,
+	err = virtqueue_add_sgs(chan->vq, sgs, out_sgs, in_sgs, req->tc,
 				GFP_ATOMIC);
+
 	if (err < 0) {
 		if (err == -ENOSPC) {
 			chan->ring_bufs_avail = 0;
@@ -289,7 +304,7 @@ req_retry:
 		} else {
 			spin_unlock_irqrestore(&chan->lock, flags);
 			p9_debug(P9_DEBUG_TRANS,
-				 "virtio rpc add_buf returned failure\n");
+				 "virtio rpc add_sgs returned failure\n");
 			return -EIO;
 		}
 	}
@@ -351,11 +366,12 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 		     char *uidata, char *uodata, int inlen,
 		     int outlen, int in_hdr_len, int kern_buf)
 {
-	int in, out, err;
+	int in, out, err, out_sgs, in_sgs;
 	unsigned long flags;
 	int in_nr_pages = 0, out_nr_pages = 0;
 	struct page **in_pages = NULL, **out_pages = NULL;
 	struct virtio_chan *chan = client->trans;
+	struct scatterlist *sgs[4];
 
 	p9_debug(P9_DEBUG_TRANS, "virtio request\n");
 
@@ -396,13 +412,22 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	req->status = REQ_STATUS_SENT;
 req_retry_pinned:
 	spin_lock_irqsave(&chan->lock, flags);
+
+	out_sgs = in_sgs = 0;
+
 	/* out data */
 	out = pack_sg_list(chan->sg, 0,
 			   VIRTQUEUE_NUM, req->tc->sdata, req->tc->size);
 
-	if (out_pages)
+	if (out)
+		sgs[out_sgs++] = chan->sg + out;
+
+	if (out_pages) {
 		out += pack_sg_list_p(chan->sg, out, VIRTQUEUE_NUM,
 				      out_pages, out_nr_pages, uodata, outlen);
+		sgs[out_sgs++] = chan->sg + out;
+	}
+		
 	/*
 	 * Take care of in data
 	 * For example TREAD have 11.
@@ -412,11 +437,16 @@ req_retry_pinned:
 	 */
 	in = pack_sg_list(chan->sg, out,
 			  VIRTQUEUE_NUM, req->rc->sdata, in_hdr_len);
-	if (in_pages)
+	if (in)
+		sgs[out_sgs + in_sgs++] = chan->sg + out + in;
+
+	if (in_pages) {
 		in += pack_sg_list_p(chan->sg, out + in, VIRTQUEUE_NUM,
 				     in_pages, in_nr_pages, uidata, inlen);
+		sgs[out_sgs + in_sgs++] = chan->sg + out + in;
+	}
 
-	err = virtqueue_add_buf(chan->vq, chan->sg, out, in, req->tc,
+	err = virtqueue_add_sgs(chan->vq, sgs, out_sgs, in_sgs, req->tc,
 				GFP_ATOMIC);
 	if (err < 0) {
 		if (err == -ENOSPC) {
@@ -432,7 +462,7 @@ req_retry_pinned:
 		} else {
 			spin_unlock_irqrestore(&chan->lock, flags);
 			p9_debug(P9_DEBUG_TRANS,
-				 "virtio rpc add_buf returned failure\n");
+				 "virtio rpc add_sgs returned failure\n");
 			err = -EIO;
 			goto err_out;
 		}
-- 
1.7.10.4

[PATCH 21/22] tools/virtio: remove virtqueue_add_buf() from tests.

[Rusty Russell]

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 tools/virtio/linux/virtio.h |    7 -------
 tools/virtio/virtio_test.c  |    6 +++---
 tools/virtio/vringh_test.c  |   22 ++++++++++++----------
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/tools/virtio/linux/virtio.h b/tools/virtio/linux/virtio.h
index 6df181a..cd80183 100644
--- a/tools/virtio/linux/virtio.h
+++ b/tools/virtio/linux/virtio.h
@@ -49,13 +49,6 @@ struct virtqueue {
 	const char *__MODULE_LICENSE_name = __MODULE_LICENSE_value
 
 /* Interfaces exported by virtio_ring. */
-int virtqueue_add_buf(struct virtqueue *vq,
-		      struct scatterlist sg[],
-		      unsigned int out_num,
-		      unsigned int in_num,
-		      void *data,
-		      gfp_t gfp);
-
 int virtqueue_add_sgs(struct virtqueue *vq,
 		      struct scatterlist *sgs[],
 		      unsigned int out_sgs,
diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
index 814ae80..da7a195 100644
--- a/tools/virtio/virtio_test.c
+++ b/tools/virtio/virtio_test.c
@@ -166,9 +166,9 @@ static void run_test(struct vdev_info *dev, struct vq_info *vq,
 		do {
 			if (started < bufs) {
 				sg_init_one(&sl, dev->buf, dev->buf_size);
-				r = virtqueue_add_buf(vq->vq, &sl, 1, 0,
-						      dev->buf + started,
-						      GFP_ATOMIC);
+				r = virtqueue_add_outbuf(vq->vq, &sl, 1,
+							 dev->buf + started,
+							 GFP_ATOMIC);
 				if (likely(r == 0)) {
 					++started;
 					virtqueue_kick(vq->vq);
diff --git a/tools/virtio/vringh_test.c b/tools/virtio/vringh_test.c
index 88fe02a..760db1c 100644
--- a/tools/virtio/vringh_test.c
+++ b/tools/virtio/vringh_test.c
@@ -388,7 +388,7 @@ static int parallel_test(unsigned long features,
 			}
 
 			if (err)
-				errx(1, "virtqueue_add_buf: %i", err);
+				errx(1, "virtqueue_add_in/outbuf: %i", err);
 
 			xfers++;
 			virtqueue_kick(vq);
@@ -431,7 +431,7 @@ int main(int argc, char *argv[])
 	struct virtio_device vdev;
 	struct virtqueue *vq;
 	struct vringh vrh;
-	struct scatterlist guest_sg[RINGSIZE];
+	struct scatterlist guest_sg[RINGSIZE], *sgs[2];
 	struct iovec host_riov[2], host_wiov[2];
 	struct vringh_iov riov, wiov;
 	struct vring_used_elem used[RINGSIZE];
@@ -492,12 +492,14 @@ int main(int argc, char *argv[])
 	sg_set_buf(&guest_sg[0], __user_addr_max - 1, 1);
 	sg_init_table(guest_sg+1, 1);
 	sg_set_buf(&guest_sg[1], __user_addr_max - 3, 2);
+	sgs[0] = &guest_sg[0];
+	sgs[1] = &guest_sg[1];
 
 	/* May allocate an indirect, so force it to allocate user addr */
 	__kmalloc_fake = __user_addr_min + vring_size(RINGSIZE, ALIGN);
-	err = virtqueue_add_buf(vq, guest_sg, 1, 1, &err, GFP_KERNEL);
+	err = virtqueue_add_sgs(vq, sgs, 1, 1, &err, GFP_KERNEL);
 	if (err)
-		errx(1, "virtqueue_add_buf: %i", err);
+		errx(1, "virtqueue_add_sgs: %i", err);
 	__kmalloc_fake = NULL;
 
 	/* Host retreives it. */
@@ -564,9 +566,9 @@ int main(int argc, char *argv[])
 
 	/* This will allocate an indirect, so force it to allocate user addr */
 	__kmalloc_fake = __user_addr_min + vring_size(RINGSIZE, ALIGN);
-	err = virtqueue_add_buf(vq, guest_sg, RINGSIZE, 0, &err, GFP_KERNEL);
+	err = virtqueue_add_outbuf(vq, guest_sg, RINGSIZE, &err, GFP_KERNEL);
 	if (err)
-		errx(1, "virtqueue_add_buf (large): %i", err);
+		errx(1, "virtqueue_add_outbuf (large): %i", err);
 	__kmalloc_fake = NULL;
 
 	/* Host picks it up (allocates new iov). */
@@ -616,9 +618,9 @@ int main(int argc, char *argv[])
 	sg_init_table(guest_sg, 1);
 	sg_set_buf(&guest_sg[0], __user_addr_max - 1, 1);
 	for (i = 0; i < RINGSIZE; i++) {
-		err = virtqueue_add_buf(vq, guest_sg, 1, 0, &err, GFP_KERNEL);
+		err = virtqueue_add_outbuf(vq, guest_sg, 1, &err, GFP_KERNEL);
 		if (err)
-			errx(1, "virtqueue_add_buf (multiple): %i", err);
+			errx(1, "virtqueue_add_outbuf (multiple): %i", err);
 	}
 
 	/* Now get many, and consume them all at once. */
@@ -665,9 +667,9 @@ int main(int argc, char *argv[])
 		sg_set_buf(&guest_sg[2], data + 6, 4);
 		sg_set_buf(&guest_sg[3], d + 3, sizeof(*d)*3);
 
-		err = virtqueue_add_buf(vq, guest_sg, 4, 0, &err, GFP_KERNEL);
+		err = virtqueue_add_outbuf(vq, guest_sg, 4, &err, GFP_KERNEL);
 		if (err)
-			errx(1, "virtqueue_add_buf (indirect): %i", err);
+			errx(1, "virtqueue_add_outbuf (indirect): %i", err);
 
 		vring_init(&vring, RINGSIZE, __user_addr_min, ALIGN);
 
-- 
1.7.10.4

[PATCH 22/22] virtio: remove virtqueue_add_buf().

[Rusty Russell]

All users changed to virtqueue_add_sg() or virtqueue_add_outbuf/inbuf.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/virtio/virtio_ring.c |   31 -------------------------------
 include/linux/virtio.h       |    7 -------
 2 files changed, 38 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5217baf..52b3658 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -296,37 +296,6 @@ add_head:
 }
 
 /**
- * virtqueue_add_buf - expose buffer to other end
- * @vq: the struct virtqueue we're talking about.
- * @sg: the description of the buffer(s).
- * @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.
- * @gfp: how to do memory allocations (if necessary).
- *
- * Caller must ensure we don't call this with other virtqueue operations
- * at the same time (except where noted).
- *
- * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
- */
-int virtqueue_add_buf(struct virtqueue *_vq,
-		      struct scatterlist sg[],
-		      unsigned int out,
-		      unsigned int in,
-		      void *data,
-		      gfp_t gfp)
-{
-	struct scatterlist *sgs[2];
-
-	sgs[0] = sg;
-	sgs[1] = sg + out;
-
-	return virtqueue_add(_vq, sgs, sg_next_arr,
-			     out, in, out ? 1 : 0, in ? 1 : 0, data, gfp);
-}
-EXPORT_SYMBOL_GPL(virtqueue_add_buf);
-
-/**
  * virtqueue_add_sgs - expose buffers to other end
  * @vq: the struct virtqueue we're talking about.
  * @sgs: array of terminated scatterlists.
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 833f17b..879ca0f 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -34,13 +34,6 @@ struct virtqueue {
 	void *priv;
 };
 
-int virtqueue_add_buf(struct virtqueue *vq,
-		      struct scatterlist sg[],
-		      unsigned int out_num,
-		      unsigned int in_num,
-		      void *data,
-		      gfp_t gfp);
-
 int virtqueue_add_outbuf(struct virtqueue *vq,
 			 struct scatterlist sg[], unsigned int num,
 			 void *data,
-- 
1.7.10.4

Re: [PATCH 16/22] virtio_console: use simplified virtqueue accessors.

[Amit Shah]

On (Mon) 18 Mar 2013 [20:04:47], Rusty Russell wrote:
> We never add buffers with input and output parts, so use the new accessors.
> 
> Cc: Amit Shah <amit.shah@redhat.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Acked-by: Amit Shah <amit.shah@redhat.com>

		Amit

Re: [PATCH 13/22] virtio_net: use virtqueue_add_sgs[] for command buffers.

[Michael S. Tsirkin]

On Mon, Mar 18, 2013 at 08:04:44PM +1030, Rusty Russell wrote:
> It's a bit cleaner to hand multiple sgs, rather than one big one.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/net/virtio_net.c |   50 +++++++++++++++++++++++-----------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 57ac4b0..f19865a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -39,7 +39,6 @@ module_param(gso, bool, 0444);
>  #define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
>  #define GOOD_COPY_LEN	128
>  
> -#define VIRTNET_SEND_COMMAND_SG_MAX    2
>  #define VIRTNET_DRIVER_VERSION "1.0.0"
>  
>  struct virtnet_stats {
> @@ -767,32 +766,34 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>   * never fail unless improperly formated.
>   */
>  static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
> -				 struct scatterlist *data, int out, int in)
> +				 struct scatterlist *out,
> +				 struct scatterlist *in)
>  {
> -	struct scatterlist *s, sg[VIRTNET_SEND_COMMAND_SG_MAX + 2];
> +	struct scatterlist *sgs[4], hdr, stat;
>  	struct virtio_net_ctrl_hdr ctrl;
>  	virtio_net_ctrl_ack status = ~0;
> -	unsigned int tmp;
> -	int i;
> +	unsigned out_num = 0, in_num = 0, tmp;
>  
>  	/* Caller should know better */
> -	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
> -		(out + in > VIRTNET_SEND_COMMAND_SG_MAX));
> -
> -	out++; /* Add header */
> -	in++; /* Add return status */
> +	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
>  
>  	ctrl.class = class;
>  	ctrl.cmd = cmd;
> +	/* Add header */
> +	sg_init_one(&hdr, &ctrl, sizeof(ctrl));
> +	sgs[out_num++] = &hdr;
>  
> -	sg_init_table(sg, out + in);
> +	if (out)
> +		sgs[out_num++] = out;
> +	if (in)
> +		sgs[out_num + in_num++] = in;
>  
> -	sg_set_buf(&sg[0], &ctrl, sizeof(ctrl));
> -	for_each_sg(data, s, out + in - 2, i)
> -		sg_set_buf(&sg[i + 1], sg_virt(s), s->length);
> -	sg_set_buf(&sg[out + in - 1], &status, sizeof(status));
> +	/* Add return status. */
> +	sg_init_one(&stat, &status, sizeof(status));
> +	sgs[out_num + in_num++] = &hdr;

Should be &stat?

Also
	BUG_ON(out_num + in_num > ARRAY_SIZE(sgs))
probably a good idea?

>  
> -	BUG_ON(virtqueue_add_buf(vi->cvq, sg, out, in, vi, GFP_ATOMIC) < 0);
> +	BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC)
> +	       < 0);
>  
>  	virtqueue_kick(vi->cvq);
>  
> @@ -821,7 +822,7 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
>  		sg_init_one(&sg, addr->sa_data, dev->addr_len);
>  		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
>  					  VIRTIO_NET_CTRL_MAC_ADDR_SET,
> -					  &sg, 1, 0)) {
> +					  &sg, NULL)) {
>  			dev_warn(&vdev->dev,
>  				 "Failed to set mac address by vq command.\n");
>  			return -EINVAL;
> @@ -889,8 +890,7 @@ static void virtnet_ack_link_announce(struct virtnet_info *vi)
>  {
>  	rtnl_lock();
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
> -				  VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL,
> -				  0, 0))
> +				  VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, NULL))
>  		dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
>  	rtnl_unlock();
>  }
> @@ -908,7 +908,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>  	sg_init_one(&sg, &s, sizeof(s));
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
> -				  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, 1, 0)){
> +				  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, NULL)) {
>  		dev_warn(&dev->dev, "Fail to set num of queue pairs to %d\n",
>  			 queue_pairs);
>  		return -EINVAL;
> @@ -955,7 +955,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
>  				  VIRTIO_NET_CTRL_RX_PROMISC,
> -				  sg, 1, 0))
> +				  sg, NULL))
>  		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
>  			 promisc ? "en" : "dis");
>  
> @@ -963,7 +963,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
>  				  VIRTIO_NET_CTRL_RX_ALLMULTI,
> -				  sg, 1, 0))
> +				  sg, NULL))
>  		dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
>  			 allmulti ? "en" : "dis");
>  
> @@ -1000,7 +1000,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
>  				  VIRTIO_NET_CTRL_MAC_TABLE_SET,
> -				  sg, 2, 0))
> +				  sg, NULL))
>  		dev_warn(&dev->dev, "Failed to set MAC fitler table.\n");
>  
>  	kfree(buf);
> @@ -1014,7 +1014,7 @@ static int virtnet_vlan_rx_add_vid(struct net_device *dev, u16 vid)
>  	sg_init_one(&sg, &vid, sizeof(vid));
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> -				  VIRTIO_NET_CTRL_VLAN_ADD, &sg, 1, 0))
> +				  VIRTIO_NET_CTRL_VLAN_ADD, &sg, NULL))
>  		dev_warn(&dev->dev, "Failed to add VLAN ID %d.\n", vid);
>  	return 0;
>  }
> @@ -1027,7 +1027,7 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
>  	sg_init_one(&sg, &vid, sizeof(vid));
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
> -				  VIRTIO_NET_CTRL_VLAN_DEL, &sg, 1, 0))
> +				  VIRTIO_NET_CTRL_VLAN_DEL, &sg, NULL))
>  		dev_warn(&dev->dev, "Failed to kill VLAN ID %d.\n", vid);
>  	return 0;
>  }
> -- 
> 1.7.10.4

Re: [PATCH 13/22] virtio_net: use virtqueue_add_sgs[] for command buffers.

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Mon, Mar 18, 2013 at 08:04:44PM +1030, Rusty Russell wrote:
>> It's a bit cleaner to hand multiple sgs, rather than one big one.
>> 
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>> ---
>>  drivers/net/virtio_net.c |   50 +++++++++++++++++++++++-----------------------
>>  1 file changed, 25 insertions(+), 25 deletions(-)
>> 
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 57ac4b0..f19865a 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -39,7 +39,6 @@ module_param(gso, bool, 0444);
>>  #define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
>>  #define GOOD_COPY_LEN	128
>>  
>> -#define VIRTNET_SEND_COMMAND_SG_MAX    2
>>  #define VIRTNET_DRIVER_VERSION "1.0.0"
>>  
>>  struct virtnet_stats {
>> @@ -767,32 +766,34 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>>   * never fail unless improperly formated.
>>   */
>>  static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
>> -				 struct scatterlist *data, int out, int in)
>> +				 struct scatterlist *out,
>> +				 struct scatterlist *in)
>>  {
>> -	struct scatterlist *s, sg[VIRTNET_SEND_COMMAND_SG_MAX + 2];
>> +	struct scatterlist *sgs[4], hdr, stat;
>>  	struct virtio_net_ctrl_hdr ctrl;
>>  	virtio_net_ctrl_ack status = ~0;
>> -	unsigned int tmp;
>> -	int i;
>> +	unsigned out_num = 0, in_num = 0, tmp;
>>  
>>  	/* Caller should know better */
>> -	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
>> -		(out + in > VIRTNET_SEND_COMMAND_SG_MAX));
>> -
>> -	out++; /* Add header */
>> -	in++; /* Add return status */
>> +	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
>>  
>>  	ctrl.class = class;
>>  	ctrl.cmd = cmd;
>> +	/* Add header */
>> +	sg_init_one(&hdr, &ctrl, sizeof(ctrl));
>> +	sgs[out_num++] = &hdr;
>>  
>> -	sg_init_table(sg, out + in);
>> +	if (out)
>> +		sgs[out_num++] = out;
>> +	if (in)
>> +		sgs[out_num + in_num++] = in;
>>  
>> -	sg_set_buf(&sg[0], &ctrl, sizeof(ctrl));
>> -	for_each_sg(data, s, out + in - 2, i)
>> -		sg_set_buf(&sg[i + 1], sg_virt(s), s->length);
>> -	sg_set_buf(&sg[out + in - 1], &status, sizeof(status));
>> +	/* Add return status. */
>> +	sg_init_one(&stat, &status, sizeof(status));
>> +	sgs[out_num + in_num++] = &hdr;
>
> Should be &stat?
>
> Also
> 	BUG_ON(out_num + in_num > ARRAY_SIZE(sgs))
> probably a good idea?

Thanks, did both.

Cheers,
Rusty.

Re: [PATCH 13/22] virtio_net: use virtqueue_add_sgs[] for command buffers.

[Wanlong Gao]

On 03/18/2013 07:10 PM, Michael S. Tsirkin wrote:
> On Mon, Mar 18, 2013 at 08:04:44PM +1030, Rusty Russell wrote:
>> It's a bit cleaner to hand multiple sgs, rather than one big one.
>>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>> ---
>>  drivers/net/virtio_net.c |   50 +++++++++++++++++++++++-----------------------
>>  1 file changed, 25 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 57ac4b0..f19865a 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -39,7 +39,6 @@ module_param(gso, bool, 0444);
>>  #define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
>>  #define GOOD_COPY_LEN	128
>>  
>> -#define VIRTNET_SEND_COMMAND_SG_MAX    2
>>  #define VIRTNET_DRIVER_VERSION "1.0.0"
>>  
>>  struct virtnet_stats {
>> @@ -767,32 +766,34 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>>   * never fail unless improperly formated.
>>   */
>>  static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
>> -				 struct scatterlist *data, int out, int in)
>> +				 struct scatterlist *out,
>> +				 struct scatterlist *in)
>>  {
>> -	struct scatterlist *s, sg[VIRTNET_SEND_COMMAND_SG_MAX + 2];
>> +	struct scatterlist *sgs[4], hdr, stat;
>>  	struct virtio_net_ctrl_hdr ctrl;
>>  	virtio_net_ctrl_ack status = ~0;
>> -	unsigned int tmp;
>> -	int i;
>> +	unsigned out_num = 0, in_num = 0, tmp;
>>  
>>  	/* Caller should know better */
>> -	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ||
>> -		(out + in > VIRTNET_SEND_COMMAND_SG_MAX));
>> -
>> -	out++; /* Add header */
>> -	in++; /* Add return status */
>> +	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
>>  
>>  	ctrl.class = class;
>>  	ctrl.cmd = cmd;
>> +	/* Add header */
>> +	sg_init_one(&hdr, &ctrl, sizeof(ctrl));
>> +	sgs[out_num++] = &hdr;
>>  
>> -	sg_init_table(sg, out + in);
>> +	if (out)
>> +		sgs[out_num++] = out;
>> +	if (in)
>> +		sgs[out_num + in_num++] = in;
>>  
>> -	sg_set_buf(&sg[0], &ctrl, sizeof(ctrl));
>> -	for_each_sg(data, s, out + in - 2, i)
>> -		sg_set_buf(&sg[i + 1], sg_virt(s), s->length);
>> -	sg_set_buf(&sg[out + in - 1], &status, sizeof(status));
>> +	/* Add return status. */
>> +	sg_init_one(&stat, &status, sizeof(status));
>> +	sgs[out_num + in_num++] = &hdr;
> 
> Should be &stat?

Good catch!!!
This is why I met the error message here:

[    9.914123] net eth0: Failed to disable promisc mode.
[    9.914958] net eth0: Failed to disable allmulti mode.
[    9.915778] net eth0: Failed to set MAC fitler table.
[    9.916564] net eth0: Failed to disable promisc mode.
[    9.917322] net eth0: Failed to disable allmulti mode.
[    9.918095] net eth0: Failed to set MAC fitler table.
[    9.918876] net eth0: Failed to disable promisc mode.
[    9.919636] net eth0: Failed to disable allmulti mode.
[    9.920403] net eth0: Failed to set MAC fitler table.
[    9.921183] net eth0: Failed to disable promisc mode.
[    9.921921] net eth0: Failed to disable allmulti mode.
[    9.922680] net eth0: Failed to set MAC fitler table.
[   11.943884] net eth0: Failed to disable promisc mode.
[   11.945460] net eth0: Failed to disable allmulti mode.
[   11.946987] net eth0: Failed to set MAC fitler table.
network[503]: Bringing up interface eth0:  [  OK  ]
[   12.242277] net eth0: Failed to disable promisc mode.
[   12.243081] net eth0: Failed to disable allmulti mode.
[   12.243824] net eth0: Failed to set MAC fitler table.


With &stat now, it has gone.

Tested-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>


Regards,
Wanlong Gao

> 
> Also
> 	BUG_ON(out_num + in_num > ARRAY_SIZE(sgs))
> probably a good idea?
> 
>>  
>> -	BUG_ON(virtqueue_add_buf(vi->cvq, sg, out, in, vi, GFP_ATOMIC) < 0);
>> +	BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC)
>> +	       < 0);
>>  
>>  	virtqueue_kick(vi->cvq);
>>  
>> @@ -821,7 +822,7 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
>>  		sg_init_one(&sg, addr->sa_data, dev->addr_len);
>>  		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
>>  					  VIRTIO_NET_CTRL_MAC_ADDR_SET,
>> -					  &sg, 1, 0)) {
>> +					  &sg, NULL)) {
>>  			dev_warn(&vdev->dev,
>>  				 "Failed to set mac address by vq command.\n");
>>  			return -EINVAL;
>> @@ -889,8 +890,7 @@ static void virtnet_ack_link_announce(struct virtnet_info *vi)
>>  {
>>  	rtnl_lock();
>>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
>> -				  VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL,
>> -				  0, 0))
>> +				  VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL, NULL))
>>  		dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
>>  	rtnl_unlock();
>>  }
>> @@ -908,7 +908,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>>  	sg_init_one(&sg, &s, sizeof(s));
>>  
>>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
>> -				  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, 1, 0)){
>> +				  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg, NULL)) {
>>  		dev_warn(&dev->dev, "Fail to set num of queue pairs to %d\n",
>>  			 queue_pairs);
>>  		return -EINVAL;
>> @@ -955,7 +955,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
>>  
>>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
>>  				  VIRTIO_NET_CTRL_RX_PROMISC,
>> -				  sg, 1, 0))
>> +				  sg, NULL))
>>  		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
>>  			 promisc ? "en" : "dis");
>>  
>> @@ -963,7 +963,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
>>  
>>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
>>  				  VIRTIO_NET_CTRL_RX_ALLMULTI,
>> -				  sg, 1, 0))
>> +				  sg, NULL))
>>  		dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
>>  			 allmulti ? "en" : "dis");
>>  
>> @@ -1000,7 +1000,7 @@ static void virtnet_set_rx_mode(struct net_device *dev)
>>  
>>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC,
>>  				  VIRTIO_NET_CTRL_MAC_TABLE_SET,
>> -				  sg, 2, 0))
>> +				  sg, NULL))
>>  		dev_warn(&dev->dev, "Failed to set MAC fitler table.\n");
>>  
>>  	kfree(buf);
>> @@ -1014,7 +1014,7 @@ static int virtnet_vlan_rx_add_vid(struct net_device *dev, u16 vid)
>>  	sg_init_one(&sg, &vid, sizeof(vid));
>>  
>>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
>> -				  VIRTIO_NET_CTRL_VLAN_ADD, &sg, 1, 0))
>> +				  VIRTIO_NET_CTRL_VLAN_ADD, &sg, NULL))
>>  		dev_warn(&dev->dev, "Failed to add VLAN ID %d.\n", vid);
>>  	return 0;
>>  }
>> @@ -1027,7 +1027,7 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
>>  	sg_init_one(&sg, &vid, sizeof(vid));
>>  
>>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
>> -				  VIRTIO_NET_CTRL_VLAN_DEL, &sg, 1, 0))
>> +				  VIRTIO_NET_CTRL_VLAN_DEL, &sg, NULL))
>>  		dev_warn(&dev->dev, "Failed to kill VLAN ID %d.\n", vid);
>>  	return 0;
>>  }
>> -- 
>> 1.7.10.4
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> 

Re: [PATCH 11/22] virtio-scsi: use virtqueue_add_sgs for command buffers

[Asias He]

On Mon, Mar 18, 2013 at 08:04:42PM +1030, Rusty Russell wrote:
> From: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> 
> Using the new virtqueue_add_sgs function lets us simplify the queueing
> path.  In particular, all data protected by the tgt_lock is just gone
> (multiqueue will find a new use for the lock).
> 
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Asias He <asias@redhat.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/scsi/virtio_scsi.c |   91 +++++++++++++++++---------------------------
>  1 file changed, 34 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 0f5dd28..bb67576 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -61,11 +61,8 @@ struct virtio_scsi_vq {
>  
>  /* Per-target queue state */
>  struct virtio_scsi_target_state {
> -	/* Protects sg.  Lock hierarchy is tgt_lock -> vq_lock.  */
> +	/* Never held at the same time as vq_lock.  */
>  	spinlock_t tgt_lock;
> -
> -	/* For sglist construction when adding commands to the virtqueue.  */
> -	struct scatterlist sg[];
>  };
>  
>  /* Driver instance state */
> @@ -353,75 +350,61 @@ static void virtscsi_event_done(struct virtqueue *vq)
>  	spin_unlock_irqrestore(&vscsi->event_vq.vq_lock, flags);
>  };
>  
> -static void virtscsi_map_sgl(struct scatterlist *sg, unsigned int *p_idx,
> -			     struct scsi_data_buffer *sdb)
> -{
> -	struct sg_table *table = &sdb->table;
> -	struct scatterlist *sg_elem;
> -	unsigned int idx = *p_idx;
> -	int i;
> -
> -	for_each_sg(table->sgl, sg_elem, table->nents, i)
> -		sg[idx++] = *sg_elem;
> -
> -	*p_idx = idx;
> -}
> -
>  /**
> - * virtscsi_map_cmd - map a scsi_cmd to a virtqueue scatterlist
> - * @vscsi	: virtio_scsi state
> + * virtscsi_add_cmd - add a virtio_scsi_cmd to a virtqueue
> + * @vq		: the struct virtqueue we're talking about
>   * @cmd		: command structure
> - * @out_num	: number of read-only elements
> - * @in_num	: number of write-only elements
>   * @req_size	: size of the request buffer
>   * @resp_size	: size of the response buffer
> - *
> - * Called with tgt_lock held.
> + * @gfp	: flags to use for memory allocations
>   */
> -static void virtscsi_map_cmd(struct virtio_scsi_target_state *tgt,
> -			     struct virtio_scsi_cmd *cmd,
> -			     unsigned *out_num, unsigned *in_num,
> -			     size_t req_size, size_t resp_size)
> +static int virtscsi_add_cmd(struct virtqueue *vq,
> +			    struct virtio_scsi_cmd *cmd,
> +			    size_t req_size, size_t resp_size, gfp_t gfp)
>  {
>  	struct scsi_cmnd *sc = cmd->sc;
> -	struct scatterlist *sg = tgt->sg;
> -	unsigned int idx = 0;
> +	struct scatterlist *sgs[4], req, resp;
> +	struct sg_table *out, *in;
> +	unsigned out_num = 0, in_num = 0;
> +
> +	out = in = NULL;
> +
> +	if (sc && sc->sc_data_direction != DMA_NONE) {
> +		if (sc->sc_data_direction != DMA_FROM_DEVICE)
> +			out = &scsi_out(sc)->table;
> +		if (sc->sc_data_direction != DMA_TO_DEVICE)
> +			in = &scsi_in(sc)->table;
> +	}
>  
>  	/* Request header.  */
> -	sg_set_buf(&sg[idx++], &cmd->req, req_size);
> +	sg_init_one(&req, &cmd->req, req_size);
> +	sgs[out_num++] = &req;
>  
>  	/* Data-out buffer.  */
> -	if (sc && sc->sc_data_direction != DMA_FROM_DEVICE)
> -		virtscsi_map_sgl(sg, &idx, scsi_out(sc));
> -
> -	*out_num = idx;
> +	if (out)
> +		sgs[out_num++] = out->sgl;
>  
>  	/* Response header.  */
> -	sg_set_buf(&sg[idx++], &cmd->resp, resp_size);
> +	sg_init_one(&resp, &cmd->resp, resp_size);
> +	sgs[out_num + in_num++] = &resp;
>  
>  	/* Data-in buffer */
> -	if (sc && sc->sc_data_direction != DMA_TO_DEVICE)
> -		virtscsi_map_sgl(sg, &idx, scsi_in(sc));
> +	if (in)
> +		sgs[out_num + in_num++] = in->sgl;
>  
> -	*in_num = idx - *out_num;
> +	return virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, gfp);
>  }
>  
> -static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
> -			     struct virtio_scsi_vq *vq,
> +static int virtscsi_kick_cmd(struct virtio_scsi_vq *vq,
>  			     struct virtio_scsi_cmd *cmd,
>  			     size_t req_size, size_t resp_size, gfp_t gfp)
>  {
> -	unsigned int out_num, in_num;
>  	unsigned long flags;
>  	int err;
>  	bool needs_kick = false;
>  
> -	spin_lock_irqsave(&tgt->tgt_lock, flags);
> -	virtscsi_map_cmd(tgt, cmd, &out_num, &in_num, req_size, resp_size);
> -
> -	spin_lock(&vq->vq_lock);
> -	err = virtqueue_add_buf(vq->vq, tgt->sg, out_num, in_num, cmd, gfp);
> -	spin_unlock(&tgt->tgt_lock);
> +	spin_lock_irqsave(&vq->vq_lock, flags);
> +	err = virtscsi_add_cmd(vq->vq, cmd, req_size, resp_size, gfp);
>  	if (!err)
>  		needs_kick = virtqueue_kick_prepare(vq->vq);
>  
> @@ -435,7 +418,6 @@ static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
>  static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
>  {
>  	struct virtio_scsi *vscsi = shost_priv(sh);
> -	struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
>  	struct virtio_scsi_cmd *cmd;
>  	int ret;
>  
> @@ -469,7 +451,7 @@ static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
>  	BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
>  	memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
>  
> -	if (virtscsi_kick_cmd(tgt, &vscsi->req_vq, cmd,
> +	if (virtscsi_kick_cmd(&vscsi->req_vq, cmd,
>  			      sizeof cmd->req.cmd, sizeof cmd->resp.cmd,
>  			      GFP_ATOMIC) == 0)
>  		ret = 0;
> @@ -483,11 +465,10 @@ out:
>  static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
>  {
>  	DECLARE_COMPLETION_ONSTACK(comp);
> -	struct virtio_scsi_target_state *tgt = vscsi->tgt[cmd->sc->device->id];
>  	int ret = FAILED;
>  
>  	cmd->comp = &comp;
> -	if (virtscsi_kick_cmd(tgt, &vscsi->ctrl_vq, cmd,
> +	if (virtscsi_kick_cmd(&vscsi->ctrl_vq, cmd,
>  			      sizeof cmd->req.tmf, sizeof cmd->resp.tmf,
>  			      GFP_NOIO) < 0)
>  		goto out;
> @@ -593,15 +574,11 @@ static struct virtio_scsi_target_state *virtscsi_alloc_tgt(
>  	struct virtio_scsi_target_state *tgt;
>  	gfp_t gfp_mask = GFP_KERNEL;
>  
> -	/* We need extra sg elements at head and tail.  */
> -	tgt = kmalloc(sizeof(*tgt) + sizeof(tgt->sg[0]) * (sg_elems + 2),
> -		      gfp_mask);
> -
> +	tgt = kmalloc(sizeof(*tgt), gfp_mask);
>  	if (!tgt)
>  		return NULL;
>  
>  	spin_lock_init(&tgt->tgt_lock);
> -	sg_init_table(tgt->sg, sg_elems + 2);
>  	return tgt;

The sg_elems is not used any more. We need to drop the parameter
sg_elems of virtscsi_alloc_tgt(). And sg_elems in virtscsi_init().

   /* We need to know how many segments before we allocate.  */
           sg_elems = virtscsi_config_get(vdev, seg_max) ?: 1;


I found this when reviewing the muti-queue virtio-scsi series.

>  }
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

-- 
Asias

[PATCH V2 11/22] virtio-scsi: use virtqueue_add_sgs for command buffers

[Wanlong Gao]

Using the new virtqueue_add_sgs function lets us simplify the queueing
path.  In particular, all data protected by the tgt_lock is just gone
(multiqueue will find a new use for the lock).

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Asias He <asias@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/scsi/virtio_scsi.c | 100 +++++++++++++++++----------------------------
 1 file changed, 37 insertions(+), 63 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 0f5dd28..77206d0 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -61,11 +61,8 @@ struct virtio_scsi_vq {
 
 /* Per-target queue state */
 struct virtio_scsi_target_state {
-	/* Protects sg.  Lock hierarchy is tgt_lock -> vq_lock.  */
+	/* Never held at the same time as vq_lock.  */
 	spinlock_t tgt_lock;
-
-	/* For sglist construction when adding commands to the virtqueue.  */
-	struct scatterlist sg[];
 };
 
 /* Driver instance state */
@@ -353,75 +350,61 @@ static void virtscsi_event_done(struct virtqueue *vq)
 	spin_unlock_irqrestore(&vscsi->event_vq.vq_lock, flags);
 };
 
-static void virtscsi_map_sgl(struct scatterlist *sg, unsigned int *p_idx,
-			     struct scsi_data_buffer *sdb)
-{
-	struct sg_table *table = &sdb->table;
-	struct scatterlist *sg_elem;
-	unsigned int idx = *p_idx;
-	int i;
-
-	for_each_sg(table->sgl, sg_elem, table->nents, i)
-		sg[idx++] = *sg_elem;
-
-	*p_idx = idx;
-}
-
 /**
- * virtscsi_map_cmd - map a scsi_cmd to a virtqueue scatterlist
- * @vscsi	: virtio_scsi state
+ * virtscsi_add_cmd - add a virtio_scsi_cmd to a virtqueue
+ * @vq		: the struct virtqueue we're talking about
  * @cmd		: command structure
- * @out_num	: number of read-only elements
- * @in_num	: number of write-only elements
  * @req_size	: size of the request buffer
  * @resp_size	: size of the response buffer
- *
- * Called with tgt_lock held.
+ * @gfp	: flags to use for memory allocations
  */
-static void virtscsi_map_cmd(struct virtio_scsi_target_state *tgt,
-			     struct virtio_scsi_cmd *cmd,
-			     unsigned *out_num, unsigned *in_num,
-			     size_t req_size, size_t resp_size)
+static int virtscsi_add_cmd(struct virtqueue *vq,
+			    struct virtio_scsi_cmd *cmd,
+			    size_t req_size, size_t resp_size, gfp_t gfp)
 {
 	struct scsi_cmnd *sc = cmd->sc;
-	struct scatterlist *sg = tgt->sg;
-	unsigned int idx = 0;
+	struct scatterlist *sgs[4], req, resp;
+	struct sg_table *out, *in;
+	unsigned out_num = 0, in_num = 0;
+
+	out = in = NULL;
+
+	if (sc && sc->sc_data_direction != DMA_NONE) {
+		if (sc->sc_data_direction != DMA_FROM_DEVICE)
+			out = &scsi_out(sc)->table;
+		if (sc->sc_data_direction != DMA_TO_DEVICE)
+			in = &scsi_in(sc)->table;
+	}
 
 	/* Request header.  */
-	sg_set_buf(&sg[idx++], &cmd->req, req_size);
+	sg_init_one(&req, &cmd->req, req_size);
+	sgs[out_num++] = &req;
 
 	/* Data-out buffer.  */
-	if (sc && sc->sc_data_direction != DMA_FROM_DEVICE)
-		virtscsi_map_sgl(sg, &idx, scsi_out(sc));
-
-	*out_num = idx;
+	if (out)
+		sgs[out_num++] = out->sgl;
 
 	/* Response header.  */
-	sg_set_buf(&sg[idx++], &cmd->resp, resp_size);
+	sg_init_one(&resp, &cmd->resp, resp_size);
+	sgs[out_num + in_num++] = &resp;
 
 	/* Data-in buffer */
-	if (sc && sc->sc_data_direction != DMA_TO_DEVICE)
-		virtscsi_map_sgl(sg, &idx, scsi_in(sc));
+	if (in)
+		sgs[out_num + in_num++] = in->sgl;
 
-	*in_num = idx - *out_num;
+	return virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, gfp);
 }
 
-static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
-			     struct virtio_scsi_vq *vq,
+static int virtscsi_kick_cmd(struct virtio_scsi_vq *vq,
 			     struct virtio_scsi_cmd *cmd,
 			     size_t req_size, size_t resp_size, gfp_t gfp)
 {
-	unsigned int out_num, in_num;
 	unsigned long flags;
 	int err;
 	bool needs_kick = false;
 
-	spin_lock_irqsave(&tgt->tgt_lock, flags);
-	virtscsi_map_cmd(tgt, cmd, &out_num, &in_num, req_size, resp_size);
-
-	spin_lock(&vq->vq_lock);
-	err = virtqueue_add_buf(vq->vq, tgt->sg, out_num, in_num, cmd, gfp);
-	spin_unlock(&tgt->tgt_lock);
+	spin_lock_irqsave(&vq->vq_lock, flags);
+	err = virtscsi_add_cmd(vq->vq, cmd, req_size, resp_size, gfp);
 	if (!err)
 		needs_kick = virtqueue_kick_prepare(vq->vq);
 
@@ -435,7 +418,6 @@ static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
 static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
 {
 	struct virtio_scsi *vscsi = shost_priv(sh);
-	struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
 	struct virtio_scsi_cmd *cmd;
 	int ret;
 
@@ -469,7 +451,7 @@ static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
 	BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
 	memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
 
-	if (virtscsi_kick_cmd(tgt, &vscsi->req_vq, cmd,
+	if (virtscsi_kick_cmd(&vscsi->req_vq, cmd,
 			      sizeof cmd->req.cmd, sizeof cmd->resp.cmd,
 			      GFP_ATOMIC) == 0)
 		ret = 0;
@@ -483,11 +465,10 @@ out:
 static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
 {
 	DECLARE_COMPLETION_ONSTACK(comp);
-	struct virtio_scsi_target_state *tgt = vscsi->tgt[cmd->sc->device->id];
 	int ret = FAILED;
 
 	cmd->comp = &comp;
-	if (virtscsi_kick_cmd(tgt, &vscsi->ctrl_vq, cmd,
+	if (virtscsi_kick_cmd(&vscsi->ctrl_vq, cmd,
 			      sizeof cmd->req.tmf, sizeof cmd->resp.tmf,
 			      GFP_NOIO) < 0)
 		goto out;
@@ -588,20 +569,16 @@ static void virtscsi_init_vq(struct virtio_scsi_vq *virtscsi_vq,
 }
 
 static struct virtio_scsi_target_state *virtscsi_alloc_tgt(
-	struct virtio_device *vdev, int sg_elems)
+	struct virtio_device *vdev)
 {
 	struct virtio_scsi_target_state *tgt;
 	gfp_t gfp_mask = GFP_KERNEL;
 
-	/* We need extra sg elements at head and tail.  */
-	tgt = kmalloc(sizeof(*tgt) + sizeof(tgt->sg[0]) * (sg_elems + 2),
-		      gfp_mask);
-
+	tgt = kmalloc(sizeof(*tgt), gfp_mask);
 	if (!tgt)
 		return NULL;
 
 	spin_lock_init(&tgt->tgt_lock);
-	sg_init_table(tgt->sg, sg_elems + 2);
 	return tgt;
 }
 
@@ -635,7 +612,7 @@ static int virtscsi_init(struct virtio_device *vdev,
 {
 	int err;
 	struct virtqueue *vqs[3];
-	u32 i, sg_elems;
+	u32 i;
 
 	vq_callback_t *callbacks[] = {
 		virtscsi_ctrl_done,
@@ -663,11 +640,8 @@ static int virtscsi_init(struct virtio_device *vdev,
 	if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
 		virtscsi_kick_event_all(vscsi);
 
-	/* We need to know how many segments before we allocate.  */
-	sg_elems = virtscsi_config_get(vdev, seg_max) ?: 1;
-
 	for (i = 0; i < num_targets; i++) {
-		vscsi->tgt[i] = virtscsi_alloc_tgt(vdev, sg_elems);
+		vscsi->tgt[i] = virtscsi_alloc_tgt(vdev);
 		if (!vscsi->tgt[i]) {
 			err = -ENOMEM;
 			goto out;
-- 
1.8.2.rc2

Re: [PATCH V2 11/22] virtio-scsi: use virtqueue_add_sgs for command buffers

[Paolo Bonzini]

Il 19/03/2013 10:48, Wanlong Gao ha scritto:
> Using the new virtqueue_add_sgs function lets us simplify the queueing
> path.  In particular, all data protected by the tgt_lock is just gone
> (multiqueue will find a new use for the lock).
> 
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Asias He <asias@redhat.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/scsi/virtio_scsi.c | 100 +++++++++++++++++----------------------------
>  1 file changed, 37 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 0f5dd28..77206d0 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -61,11 +61,8 @@ struct virtio_scsi_vq {
>  
>  /* Per-target queue state */
>  struct virtio_scsi_target_state {
> -	/* Protects sg.  Lock hierarchy is tgt_lock -> vq_lock.  */
> +	/* Never held at the same time as vq_lock.  */
>  	spinlock_t tgt_lock;
> -
> -	/* For sglist construction when adding commands to the virtqueue.  */
> -	struct scatterlist sg[];
>  };
>  
>  /* Driver instance state */
> @@ -353,75 +350,61 @@ static void virtscsi_event_done(struct virtqueue *vq)
>  	spin_unlock_irqrestore(&vscsi->event_vq.vq_lock, flags);
>  };
>  
> -static void virtscsi_map_sgl(struct scatterlist *sg, unsigned int *p_idx,
> -			     struct scsi_data_buffer *sdb)
> -{
> -	struct sg_table *table = &sdb->table;
> -	struct scatterlist *sg_elem;
> -	unsigned int idx = *p_idx;
> -	int i;
> -
> -	for_each_sg(table->sgl, sg_elem, table->nents, i)
> -		sg[idx++] = *sg_elem;
> -
> -	*p_idx = idx;
> -}
> -
>  /**
> - * virtscsi_map_cmd - map a scsi_cmd to a virtqueue scatterlist
> - * @vscsi	: virtio_scsi state
> + * virtscsi_add_cmd - add a virtio_scsi_cmd to a virtqueue
> + * @vq		: the struct virtqueue we're talking about
>   * @cmd		: command structure
> - * @out_num	: number of read-only elements
> - * @in_num	: number of write-only elements
>   * @req_size	: size of the request buffer
>   * @resp_size	: size of the response buffer
> - *
> - * Called with tgt_lock held.
> + * @gfp	: flags to use for memory allocations
>   */
> -static void virtscsi_map_cmd(struct virtio_scsi_target_state *tgt,
> -			     struct virtio_scsi_cmd *cmd,
> -			     unsigned *out_num, unsigned *in_num,
> -			     size_t req_size, size_t resp_size)
> +static int virtscsi_add_cmd(struct virtqueue *vq,
> +			    struct virtio_scsi_cmd *cmd,
> +			    size_t req_size, size_t resp_size, gfp_t gfp)
>  {
>  	struct scsi_cmnd *sc = cmd->sc;
> -	struct scatterlist *sg = tgt->sg;
> -	unsigned int idx = 0;
> +	struct scatterlist *sgs[4], req, resp;
> +	struct sg_table *out, *in;
> +	unsigned out_num = 0, in_num = 0;
> +
> +	out = in = NULL;
> +
> +	if (sc && sc->sc_data_direction != DMA_NONE) {
> +		if (sc->sc_data_direction != DMA_FROM_DEVICE)
> +			out = &scsi_out(sc)->table;
> +		if (sc->sc_data_direction != DMA_TO_DEVICE)
> +			in = &scsi_in(sc)->table;
> +	}
>  
>  	/* Request header.  */
> -	sg_set_buf(&sg[idx++], &cmd->req, req_size);
> +	sg_init_one(&req, &cmd->req, req_size);
> +	sgs[out_num++] = &req;
>  
>  	/* Data-out buffer.  */
> -	if (sc && sc->sc_data_direction != DMA_FROM_DEVICE)
> -		virtscsi_map_sgl(sg, &idx, scsi_out(sc));
> -
> -	*out_num = idx;
> +	if (out)
> +		sgs[out_num++] = out->sgl;
>  
>  	/* Response header.  */
> -	sg_set_buf(&sg[idx++], &cmd->resp, resp_size);
> +	sg_init_one(&resp, &cmd->resp, resp_size);
> +	sgs[out_num + in_num++] = &resp;
>  
>  	/* Data-in buffer */
> -	if (sc && sc->sc_data_direction != DMA_TO_DEVICE)
> -		virtscsi_map_sgl(sg, &idx, scsi_in(sc));
> +	if (in)
> +		sgs[out_num + in_num++] = in->sgl;
>  
> -	*in_num = idx - *out_num;
> +	return virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, gfp);
>  }
>  
> -static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
> -			     struct virtio_scsi_vq *vq,
> +static int virtscsi_kick_cmd(struct virtio_scsi_vq *vq,
>  			     struct virtio_scsi_cmd *cmd,
>  			     size_t req_size, size_t resp_size, gfp_t gfp)
>  {
> -	unsigned int out_num, in_num;
>  	unsigned long flags;
>  	int err;
>  	bool needs_kick = false;
>  
> -	spin_lock_irqsave(&tgt->tgt_lock, flags);
> -	virtscsi_map_cmd(tgt, cmd, &out_num, &in_num, req_size, resp_size);
> -
> -	spin_lock(&vq->vq_lock);
> -	err = virtqueue_add_buf(vq->vq, tgt->sg, out_num, in_num, cmd, gfp);
> -	spin_unlock(&tgt->tgt_lock);
> +	spin_lock_irqsave(&vq->vq_lock, flags);
> +	err = virtscsi_add_cmd(vq->vq, cmd, req_size, resp_size, gfp);
>  	if (!err)
>  		needs_kick = virtqueue_kick_prepare(vq->vq);
>  
> @@ -435,7 +418,6 @@ static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
>  static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
>  {
>  	struct virtio_scsi *vscsi = shost_priv(sh);
> -	struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
>  	struct virtio_scsi_cmd *cmd;
>  	int ret;
>  
> @@ -469,7 +451,7 @@ static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
>  	BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
>  	memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
>  
> -	if (virtscsi_kick_cmd(tgt, &vscsi->req_vq, cmd,
> +	if (virtscsi_kick_cmd(&vscsi->req_vq, cmd,
>  			      sizeof cmd->req.cmd, sizeof cmd->resp.cmd,
>  			      GFP_ATOMIC) == 0)
>  		ret = 0;
> @@ -483,11 +465,10 @@ out:
>  static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
>  {
>  	DECLARE_COMPLETION_ONSTACK(comp);
> -	struct virtio_scsi_target_state *tgt = vscsi->tgt[cmd->sc->device->id];
>  	int ret = FAILED;
>  
>  	cmd->comp = &comp;
> -	if (virtscsi_kick_cmd(tgt, &vscsi->ctrl_vq, cmd,
> +	if (virtscsi_kick_cmd(&vscsi->ctrl_vq, cmd,
>  			      sizeof cmd->req.tmf, sizeof cmd->resp.tmf,
>  			      GFP_NOIO) < 0)
>  		goto out;
> @@ -588,20 +569,16 @@ static void virtscsi_init_vq(struct virtio_scsi_vq *virtscsi_vq,
>  }
>  
>  static struct virtio_scsi_target_state *virtscsi_alloc_tgt(
> -	struct virtio_device *vdev, int sg_elems)
> +	struct virtio_device *vdev)
>  {
>  	struct virtio_scsi_target_state *tgt;
>  	gfp_t gfp_mask = GFP_KERNEL;
>  
> -	/* We need extra sg elements at head and tail.  */
> -	tgt = kmalloc(sizeof(*tgt) + sizeof(tgt->sg[0]) * (sg_elems + 2),
> -		      gfp_mask);
> -
> +	tgt = kmalloc(sizeof(*tgt), gfp_mask);
>  	if (!tgt)
>  		return NULL;
>  
>  	spin_lock_init(&tgt->tgt_lock);
> -	sg_init_table(tgt->sg, sg_elems + 2);
>  	return tgt;
>  }
>  
> @@ -635,7 +612,7 @@ static int virtscsi_init(struct virtio_device *vdev,
>  {
>  	int err;
>  	struct virtqueue *vqs[3];
> -	u32 i, sg_elems;
> +	u32 i;
>  
>  	vq_callback_t *callbacks[] = {
>  		virtscsi_ctrl_done,
> @@ -663,11 +640,8 @@ static int virtscsi_init(struct virtio_device *vdev,
>  	if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
>  		virtscsi_kick_event_all(vscsi);
>  
> -	/* We need to know how many segments before we allocate.  */
> -	sg_elems = virtscsi_config_get(vdev, seg_max) ?: 1;
> -
>  	for (i = 0; i < num_targets; i++) {
> -		vscsi->tgt[i] = virtscsi_alloc_tgt(vdev, sg_elems);
> +		vscsi->tgt[i] = virtscsi_alloc_tgt(vdev);
>  		if (!vscsi->tgt[i]) {
>  			err = -ENOMEM;
>  			goto out;
> 

Yes, it's simpler this way.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [PATCH V2 11/22] virtio-scsi: use virtqueue_add_sgs for command buffers

[Rusty Russell]

Wanlong Gao <gaowanlong@cn.fujitsu.com> writes:
> Using the new virtqueue_add_sgs function lets us simplify the queueing
> path.  In particular, all data protected by the tgt_lock is just gone
> (multiqueue will find a new use for the lock).
>
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Asias He <asias@redhat.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks... and I agree with Paulo: this is nice.

I've replaced the patch I posted with this one.

Cheers,
Rusty.

Re: [PATCH 15/22] virtio_rng: use simplified virtqueue accessors.

[Asias He]

On Mon, Mar 18, 2013 at 08:04:46PM +1030, Rusty Russell wrote:
> We never add buffers with input and output parts, so use the new accessors.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Asias He <asias@redhat.com>

> ---
>  drivers/char/hw_random/virtio-rng.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index 6bf4d47..ef46a9c 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -47,7 +47,7 @@ static void register_buffer(u8 *buf, size_t size)
>  	sg_init_one(&sg, buf, size);
>  
>  	/* There should always be room for one buffer. */
> -	if (virtqueue_add_buf(vq, &sg, 0, 1, buf, GFP_KERNEL) < 0)
> +	if (virtqueue_add_inbuf(vq, &sg, 1, buf, GFP_KERNEL) < 0)
>  		BUG();
>  
>  	virtqueue_kick(vq);
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

-- 
Asias

Re: [PATCH 12/22] virtio_scsi: use virtqueue_add_inbuf() for virtscsi_kick_event.

[Asias He]

On Mon, Mar 18, 2013 at 08:04:43PM +1030, Rusty Russell wrote:
> It's a bit clearer, and add_buf is going away.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Asias He <asias@redhat.com>

> ---
>  drivers/scsi/virtio_scsi.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index bb67576..f679b8c 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -222,8 +222,8 @@ static int virtscsi_kick_event(struct virtio_scsi *vscsi,
>  
>  	spin_lock_irqsave(&vscsi->event_vq.vq_lock, flags);
>  
> -	err = virtqueue_add_buf(vscsi->event_vq.vq, &sg, 0, 1, event_node,
> -				GFP_ATOMIC);
> +	err = virtqueue_add_inbuf(vscsi->event_vq.vq, &sg, 1, event_node,
> +				  GFP_ATOMIC);
>  	if (!err)
>  		virtqueue_kick(vscsi->event_vq.vq);
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

-- 
Asias

Re: [PATCH 14/22] virtio_net: use simplified virtqueue accessors.

[Asias He]

On Mon, Mar 18, 2013 at 08:04:45PM +1030, Rusty Russell wrote:
> We never add buffers with input and output parts, so use the new accessors.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Asias He <asias@redhat.com>

> ---
>  drivers/net/virtio_net.c |   11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index f19865a..38c01d9 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -443,7 +443,7 @@ static int add_recvbuf_small(struct receive_queue *rq, gfp_t gfp)
>  
>  	skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
>  
> -	err = virtqueue_add_buf(rq->vq, rq->sg, 0, 2, skb, gfp);
> +	err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp);
>  	if (err < 0)
>  		dev_kfree_skb(skb);
>  
> @@ -488,8 +488,8 @@ static int add_recvbuf_big(struct receive_queue *rq, gfp_t gfp)
>  
>  	/* chain first in list head */
>  	first->private = (unsigned long)list;
> -	err = virtqueue_add_buf(rq->vq, rq->sg, 0, MAX_SKB_FRAGS + 2,
> -				first, gfp);
> +	err = virtqueue_add_inbuf(rq->vq, rq->sg, MAX_SKB_FRAGS + 2,
> +				  first, gfp);
>  	if (err < 0)
>  		give_pages(rq, first);
>  
> @@ -507,7 +507,7 @@ static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
>  
>  	sg_init_one(rq->sg, page_address(page), PAGE_SIZE);
>  
> -	err = virtqueue_add_buf(rq->vq, rq->sg, 0, 1, page, gfp);
> +	err = virtqueue_add_inbuf(rq->vq, rq->sg, 1, page, gfp);
>  	if (err < 0)
>  		give_pages(rq, page);
>  
> @@ -710,8 +710,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
>  		sg_set_buf(sq->sg, &hdr->hdr, sizeof hdr->hdr);
>  
>  	num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len) + 1;
> -	return virtqueue_add_buf(sq->vq, sq->sg, num_sg,
> -				 0, skb, GFP_ATOMIC);
> +	return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
>  }
>  
>  static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

-- 
Asias

Re: [PATCH V2 11/22] virtio-scsi: use virtqueue_add_sgs for command buffers

[Asias He]

On Tue, Mar 19, 2013 at 05:48:59PM +0800, Wanlong Gao wrote:
> Using the new virtqueue_add_sgs function lets us simplify the queueing
> path.  In particular, all data protected by the tgt_lock is just gone
> (multiqueue will find a new use for the lock).
> 
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Asias He <asias@redhat.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/scsi/virtio_scsi.c | 100 +++++++++++++++++----------------------------
>  1 file changed, 37 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 0f5dd28..77206d0 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -61,11 +61,8 @@ struct virtio_scsi_vq {
>  
>  /* Per-target queue state */
>  struct virtio_scsi_target_state {
> -	/* Protects sg.  Lock hierarchy is tgt_lock -> vq_lock.  */
> +	/* Never held at the same time as vq_lock.  */
>  	spinlock_t tgt_lock;
> -
> -	/* For sglist construction when adding commands to the virtqueue.  */
> -	struct scatterlist sg[];
>  };
>  
>  /* Driver instance state */
> @@ -353,75 +350,61 @@ static void virtscsi_event_done(struct virtqueue *vq)
>  	spin_unlock_irqrestore(&vscsi->event_vq.vq_lock, flags);
>  };
>  
> -static void virtscsi_map_sgl(struct scatterlist *sg, unsigned int *p_idx,
> -			     struct scsi_data_buffer *sdb)
> -{
> -	struct sg_table *table = &sdb->table;
> -	struct scatterlist *sg_elem;
> -	unsigned int idx = *p_idx;
> -	int i;
> -
> -	for_each_sg(table->sgl, sg_elem, table->nents, i)
> -		sg[idx++] = *sg_elem;
> -
> -	*p_idx = idx;
> -}
> -
>  /**
> - * virtscsi_map_cmd - map a scsi_cmd to a virtqueue scatterlist
> - * @vscsi	: virtio_scsi state
> + * virtscsi_add_cmd - add a virtio_scsi_cmd to a virtqueue
> + * @vq		: the struct virtqueue we're talking about
>   * @cmd		: command structure
> - * @out_num	: number of read-only elements
> - * @in_num	: number of write-only elements
>   * @req_size	: size of the request buffer
>   * @resp_size	: size of the response buffer
> - *
> - * Called with tgt_lock held.
> + * @gfp	: flags to use for memory allocations
>   */
> -static void virtscsi_map_cmd(struct virtio_scsi_target_state *tgt,
> -			     struct virtio_scsi_cmd *cmd,
> -			     unsigned *out_num, unsigned *in_num,
> -			     size_t req_size, size_t resp_size)
> +static int virtscsi_add_cmd(struct virtqueue *vq,
> +			    struct virtio_scsi_cmd *cmd,
> +			    size_t req_size, size_t resp_size, gfp_t gfp)
>  {
>  	struct scsi_cmnd *sc = cmd->sc;
> -	struct scatterlist *sg = tgt->sg;
> -	unsigned int idx = 0;
> +	struct scatterlist *sgs[4], req, resp;
> +	struct sg_table *out, *in;
> +	unsigned out_num = 0, in_num = 0;
> +
> +	out = in = NULL;
> +
> +	if (sc && sc->sc_data_direction != DMA_NONE) {
> +		if (sc->sc_data_direction != DMA_FROM_DEVICE)
> +			out = &scsi_out(sc)->table;
> +		if (sc->sc_data_direction != DMA_TO_DEVICE)
> +			in = &scsi_in(sc)->table;
> +	}
>  
>  	/* Request header.  */
> -	sg_set_buf(&sg[idx++], &cmd->req, req_size);
> +	sg_init_one(&req, &cmd->req, req_size);
> +	sgs[out_num++] = &req;
>  
>  	/* Data-out buffer.  */
> -	if (sc && sc->sc_data_direction != DMA_FROM_DEVICE)
> -		virtscsi_map_sgl(sg, &idx, scsi_out(sc));
> -
> -	*out_num = idx;
> +	if (out)
> +		sgs[out_num++] = out->sgl;
>  
>  	/* Response header.  */
> -	sg_set_buf(&sg[idx++], &cmd->resp, resp_size);
> +	sg_init_one(&resp, &cmd->resp, resp_size);
> +	sgs[out_num + in_num++] = &resp;
>  
>  	/* Data-in buffer */
> -	if (sc && sc->sc_data_direction != DMA_TO_DEVICE)
> -		virtscsi_map_sgl(sg, &idx, scsi_in(sc));
> +	if (in)
> +		sgs[out_num + in_num++] = in->sgl;
>  
> -	*in_num = idx - *out_num;
> +	return virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, gfp);
>  }
>  
> -static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
> -			     struct virtio_scsi_vq *vq,
> +static int virtscsi_kick_cmd(struct virtio_scsi_vq *vq,
>  			     struct virtio_scsi_cmd *cmd,
>  			     size_t req_size, size_t resp_size, gfp_t gfp)
>  {
> -	unsigned int out_num, in_num;
>  	unsigned long flags;
>  	int err;
>  	bool needs_kick = false;
>  
> -	spin_lock_irqsave(&tgt->tgt_lock, flags);
> -	virtscsi_map_cmd(tgt, cmd, &out_num, &in_num, req_size, resp_size);
> -
> -	spin_lock(&vq->vq_lock);
> -	err = virtqueue_add_buf(vq->vq, tgt->sg, out_num, in_num, cmd, gfp);
> -	spin_unlock(&tgt->tgt_lock);
> +	spin_lock_irqsave(&vq->vq_lock, flags);
> +	err = virtscsi_add_cmd(vq->vq, cmd, req_size, resp_size, gfp);
>  	if (!err)
>  		needs_kick = virtqueue_kick_prepare(vq->vq);
>  
> @@ -435,7 +418,6 @@ static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
>  static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
>  {
>  	struct virtio_scsi *vscsi = shost_priv(sh);
> -	struct virtio_scsi_target_state *tgt = vscsi->tgt[sc->device->id];
>  	struct virtio_scsi_cmd *cmd;
>  	int ret;
>  
> @@ -469,7 +451,7 @@ static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
>  	BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
>  	memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
>  
> -	if (virtscsi_kick_cmd(tgt, &vscsi->req_vq, cmd,
> +	if (virtscsi_kick_cmd(&vscsi->req_vq, cmd,
>  			      sizeof cmd->req.cmd, sizeof cmd->resp.cmd,
>  			      GFP_ATOMIC) == 0)
>  		ret = 0;
> @@ -483,11 +465,10 @@ out:
>  static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
>  {
>  	DECLARE_COMPLETION_ONSTACK(comp);
> -	struct virtio_scsi_target_state *tgt = vscsi->tgt[cmd->sc->device->id];
>  	int ret = FAILED;
>  
>  	cmd->comp = &comp;
> -	if (virtscsi_kick_cmd(tgt, &vscsi->ctrl_vq, cmd,
> +	if (virtscsi_kick_cmd(&vscsi->ctrl_vq, cmd,
>  			      sizeof cmd->req.tmf, sizeof cmd->resp.tmf,
>  			      GFP_NOIO) < 0)
>  		goto out;
> @@ -588,20 +569,16 @@ static void virtscsi_init_vq(struct virtio_scsi_vq *virtscsi_vq,
>  }
>  
>  static struct virtio_scsi_target_state *virtscsi_alloc_tgt(
> -	struct virtio_device *vdev, int sg_elems)
> +	struct virtio_device *vdev)
>  {
>  	struct virtio_scsi_target_state *tgt;
>  	gfp_t gfp_mask = GFP_KERNEL;
>  
> -	/* We need extra sg elements at head and tail.  */
> -	tgt = kmalloc(sizeof(*tgt) + sizeof(tgt->sg[0]) * (sg_elems + 2),
> -		      gfp_mask);
> -
> +	tgt = kmalloc(sizeof(*tgt), gfp_mask);
>  	if (!tgt)
>  		return NULL;
>  
>  	spin_lock_init(&tgt->tgt_lock);
> -	sg_init_table(tgt->sg, sg_elems + 2);
>  	return tgt;
>  }
>  
> @@ -635,7 +612,7 @@ static int virtscsi_init(struct virtio_device *vdev,
>  {
>  	int err;
>  	struct virtqueue *vqs[3];
> -	u32 i, sg_elems;
> +	u32 i;
>  
>  	vq_callback_t *callbacks[] = {
>  		virtscsi_ctrl_done,
> @@ -663,11 +640,8 @@ static int virtscsi_init(struct virtio_device *vdev,
>  	if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
>  		virtscsi_kick_event_all(vscsi);
>  
> -	/* We need to know how many segments before we allocate.  */
> -	sg_elems = virtscsi_config_get(vdev, seg_max) ?: 1;
> -
>  	for (i = 0; i < num_targets; i++) {
> -		vscsi->tgt[i] = virtscsi_alloc_tgt(vdev, sg_elems);
> +		vscsi->tgt[i] = virtscsi_alloc_tgt(vdev);
>  		if (!vscsi->tgt[i]) {
>  			err = -ENOMEM;
>  			goto out;
> -- 
> 1.8.2.rc2
> 

Reviewed-by: Asias He <asias@redhat.com>

-- 
Asias

Re: [PATCH 20/22] 9p/trans_virtio.c: use virtio_add_sgs[]

[Rusty Russell]

Rusty Russell <rusty@rustcorp.com.au> writes:
> virtio_add_buf() is going away, replaced with virtio_add_sgs() which
> takes multiple terminated scatterlists.
>
> Cc: Eric Van Hensbergen <ericvh@gmail.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

This one was buggy.  Testing and reading the patch do wonderful things.

Cheers,
Rusty.

9p/trans_virtio.c: use virtio_add_sgs[]

virtio_add_buf() is going away, replaced with virtio_add_sgs() which
takes multiple terminated scatterlists.

Cc: Eric Van Hensbergen <ericvh@gmail.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index de2e950..e29d72e 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -194,11 +194,14 @@ static int pack_sg_list(struct scatterlist *sg, int start,
 		if (s > count)
 			s = count;
 		BUG_ON(index > limit);
+		/* Make sure we don't terminate early. */
+		sg_unmark_end(&sg[index]);
 		sg_set_buf(&sg[index++], data, s);
 		count -= s;
 		data += s;
 	}
-
+	if (index-start)
+		sg_mark_end(&sg[index - 1]);
 	return index-start;
 }
 
@@ -236,12 +239,17 @@ pack_sg_list_p(struct scatterlist *sg, int start, int limit,
 		s = rest_of_page(data);
 		if (s > count)
 			s = count;
+		/* Make sure we don't terminate early. */
+		sg_unmark_end(&sg[index]);
 		sg_set_page(&sg[index++], pdata[i++], s, data_off);
 		data_off = 0;
 		data += s;
 		count -= s;
 		nr_pages--;
 	}
+
+	if (index-start)
+		sg_mark_end(&sg[index - 1]);
 	return index - start;
 }
 
@@ -256,9 +264,10 @@ static int
 p9_virtio_request(struct p9_client *client, struct p9_req_t *req)
 {
 	int err;
-	int in, out;
+	int in, out, out_sgs, in_sgs;
 	unsigned long flags;
 	struct virtio_chan *chan = client->trans;
+	struct scatterlist *sgs[2];
 
 	p9_debug(P9_DEBUG_TRANS, "9p debug: virtio request\n");
 
@@ -266,15 +275,20 @@ p9_virtio_request(struct p9_client *client, struct p9_req_t *req)
 req_retry:
 	spin_lock_irqsave(&chan->lock, flags);
 
+	out_sgs = in_sgs = 0;
 	/* Handle out VirtIO ring buffers */
 	out = pack_sg_list(chan->sg, 0,
 			   VIRTQUEUE_NUM, req->tc->sdata, req->tc->size);
+	if (out)
+		sgs[out_sgs++] = chan->sg;
 
 	in = pack_sg_list(chan->sg, out,
 			  VIRTQUEUE_NUM, req->rc->sdata, req->rc->capacity);
+	if (in)
+		sgs[out_sgs + in_sgs++] = chan->sg + out;
 
-	err = virtqueue_add_buf(chan->vq, chan->sg, out, in, req->tc,
+	err = virtqueue_add_sgs(chan->vq, sgs, out_sgs, in_sgs, req->tc,
 				GFP_ATOMIC);
 	if (err < 0) {
 		if (err == -ENOSPC) {
 			chan->ring_bufs_avail = 0;
@@ -289,7 +304,7 @@ req_retry:
 		} else {
 			spin_unlock_irqrestore(&chan->lock, flags);
 			p9_debug(P9_DEBUG_TRANS,
-				 "virtio rpc add_buf returned failure\n");
+				 "virtio rpc add_sgs returned failure\n");
 			return -EIO;
 		}
 	}
@@ -351,11 +366,12 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 		     char *uidata, char *uodata, int inlen,
 		     int outlen, int in_hdr_len, int kern_buf)
 {
-	int in, out, err;
+	int in, out, err, out_sgs, in_sgs;
 	unsigned long flags;
 	int in_nr_pages = 0, out_nr_pages = 0;
 	struct page **in_pages = NULL, **out_pages = NULL;
 	struct virtio_chan *chan = client->trans;
+	struct scatterlist *sgs[4];
 
 	p9_debug(P9_DEBUG_TRANS, "virtio request\n");
 
@@ -396,13 +412,22 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	req->status = REQ_STATUS_SENT;
 req_retry_pinned:
 	spin_lock_irqsave(&chan->lock, flags);
+
+	out_sgs = in_sgs = 0;
+
 	/* out data */
 	out = pack_sg_list(chan->sg, 0,
 			   VIRTQUEUE_NUM, req->tc->sdata, req->tc->size);
 
-	if (out_pages)
+	if (out)
+		sgs[out_sgs++] = chan->sg;
+
+	if (out_pages) {
+		sgs[out_sgs++] = chan->sg + out;
 		out += pack_sg_list_p(chan->sg, out, VIRTQUEUE_NUM,
 				      out_pages, out_nr_pages, uodata, outlen);
+	}
+		
 	/*
 	 * Take care of in data
 	 * For example TREAD have 11.
@@ -412,11 +437,17 @@ req_retry_pinned:
 	 */
 	in = pack_sg_list(chan->sg, out,
 			  VIRTQUEUE_NUM, req->rc->sdata, in_hdr_len);
-	if (in_pages)
+	if (in)
+		sgs[out_sgs + in_sgs++] = chan->sg + out;
+
+	if (in_pages) {
+		sgs[out_sgs + in_sgs++] = chan->sg + out + in;
 		in += pack_sg_list_p(chan->sg, out + in, VIRTQUEUE_NUM,
 				     in_pages, in_nr_pages, uidata, inlen);
+	}
 
-	err = virtqueue_add_buf(chan->vq, chan->sg, out, in, req->tc,
+	BUG_ON(out_sgs + in_sgs > ARRAY_SIZE(sgs));
+	err = virtqueue_add_sgs(chan->vq, sgs, out_sgs, in_sgs, req->tc,
 				GFP_ATOMIC);
 	if (err < 0) {
 		if (err == -ENOSPC) {
@@ -432,7 +463,7 @@ req_retry_pinned:
 		} else {
 			spin_unlock_irqrestore(&chan->lock, flags);
 			p9_debug(P9_DEBUG_TRANS,
-				 "virtio rpc add_buf returned failure\n");
+				 "virtio rpc add_sgs returned failure\n");
 			err = -EIO;
 			goto err_out;
 		}

Re: [PATCH 00/22] virtqueue_add_sgs, virtqueue_add_outbuf, virtqueue_add_inbuf

[Rusty Russell]

Rusty Russell <rusty@rustcorp.com.au> writes:
> Add virtqueue_add_sgs which is more general than virtqueue_add_buf,
> which makes virtio-scsi and virtio-blk nicer, then add virtqueue_add_inbuf
> and virtqueue_add_outbuf which handle the more general case, and finally
> delete virtqueue_add_buf().
>
> I'm hoping this will be the final post of the whole series, and it can
> move from my pending-rebases tree into virtio-next.

OK, thanks for the feedback, reviews and corrections everyone.

I folded the following patches into one commit:

 Subject: [PATCH 02/22] virtio_ring: virtqueue_add_sgs, to add multiple sgs.
 Subject: [PATCH 03/22] virtio_ring: don't count elements twice for add_buf path.
 Subject: [PATCH 04/22] virtio_ring: inline internal vring functions more aggres

I've put it into virtio-next, so please send any further fixes as diffs
against that.

Thanks,
Rusty.

Re: [PATCH 00/22] virtqueue_add_sgs, virtqueue_add_outbuf, virtqueue_add_inbuf

[Asias He]

On Wed, Mar 20, 2013 at 03:50:33PM +1030, Rusty Russell wrote:
> Rusty Russell <rusty@rustcorp.com.au> writes:
> > Add virtqueue_add_sgs which is more general than virtqueue_add_buf,
> > which makes virtio-scsi and virtio-blk nicer, then add virtqueue_add_inbuf
> > and virtqueue_add_outbuf which handle the more general case, and finally
> > delete virtqueue_add_buf().
> >
> > I'm hoping this will be the final post of the whole series, and it can
> > move from my pending-rebases tree into virtio-next.
> 
> OK, thanks for the feedback, reviews and corrections everyone.
> 
> I folded the following patches into one commit:
> 
>  Subject: [PATCH 02/22] virtio_ring: virtqueue_add_sgs, to add multiple sgs.
>  Subject: [PATCH 03/22] virtio_ring: don't count elements twice for add_buf path.
>  Subject: [PATCH 04/22] virtio_ring: inline internal vring functions more aggres
> 
> I've put it into virtio-next, so please send any further fixes as diffs
> against that.

Cheers!

-- 
Asias

Re: [PATCH 18/22] virtio_rpmsg_bus: use simplified virtqueue accessors.

[Ohad Ben-Cohen]

On Mon, Mar 18, 2013 at 11:34 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> We never add buffers with input and output parts, so use the new accessors.
>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Acked-by: Ohad Ben-Cohen <ohad@wizery.com>

Re: [PATCH 18/22] virtio_rpmsg_bus: use simplified virtqueue accessors.

[Rusty Russell]

Ohad Ben-Cohen <ohad@wizery.com> writes:
> On Mon, Mar 18, 2013 at 11:34 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> We never add buffers with input and output parts, so use the new accessors.
>>
>> Cc: Ohad Ben-Cohen <ohad@wizery.com>
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Acked-by: Ohad Ben-Cohen <ohad@wizery.com>

Thanks.  Unfortunately, a few hours after I pushed it into virtio-next.

Cheers,
Rusty.