[PATCH 0/6] virtio_add_buf replacement.

[PATCH 0/6] virtio_add_buf replacement.

[Rusty Russell]

OK, so I've spent a few days benchmarking.  Turns out 80% of
virtio_add_buf cases are uni-directional (including the
always-performance-sensitive networking code), and that gets no
performance penalty (though tests with real networking would be
appreciated!).

I'm not reposting all the "convert driver to virtio_add_outbuf()"
patches: just the scsi one which I didn't have before.  I won't actually
remove virtio_add_buf() until the *following* merge window, just to be
sure.

One annoying thing about benchmarking is that in some cases, speeding up
one side can make the whole thing slower, due to more wakeups.
Device-side polling techniques might be required in future to get more
performance.

Cheers,
Rusty.

[PATCH 1/6] 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>

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 ff6714e..6eff15b 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -40,6 +40,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);

[PATCH 2/6] 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>

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);
 
 /**

[PATCH 3/6] 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> 

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);

[PATCH 4/6] 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> 

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 6eff15b..b442500 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -40,6 +40,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,

[PATCH 5/6] 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>

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 7fedde8..cf33672 100644
--- a/tools/virtio/vringh_test.c
+++ b/tools/virtio/vringh_test.c
@@ -342,11 +342,11 @@ static int parallel_test(unsigned long features,
 			 * user addr */
 			__kmalloc_fake = indirects + (xfers % RINGSIZE) * 3;
 			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) {
 				char buf[128];

[PATCH 6/6] 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>

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index bae0c95..612e320 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -220,8 +220,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);

Re: [PATCH 0/6] virtio_add_buf replacement.

[Asias He]

On Wed, Mar 06, 2013 at 04:15:02PM +1100, Rusty Russell wrote:
> OK, so I've spent a few days benchmarking.  Turns out 80% of
> virtio_add_buf cases are uni-directional (including the
> always-performance-sensitive networking code), and that gets no
> performance penalty (though tests with real networking would be
> appreciated!).
> 
> I'm not reposting all the "convert driver to virtio_add_outbuf()"
> patches: just the scsi one which I didn't have before.  I won't actually
> remove virtio_add_buf() until the *following* merge window, just to be
> sure.

Why not send out all the patches in this series? It would be much easier
for people to read in one thread.

> One annoying thing about benchmarking is that in some cases, speeding up
> one side can make the whole thing slower, due to more wakeups.
> Device-side polling techniques might be required in future to get more
> performance.
> 
> Cheers,
> Rusty.
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

-- 
Asias

Re: [PATCH 4/6] virtio_ring: virtqueue_add_outbuf / virtqueue_add_inbuf.

[Asias He]

On Wed, Mar 06, 2013 at 04:23:24PM +1100, Rusty Russell wrote:
> 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> 

So, what is the plan for the following ideas discussed in the other
thread?

   '''
   > Looking at code, it seems that most users really have a single sg, in
   > low memory. So how about simply passing void * instead of sg? Whoever
   > has multiple sgs can use the rich interface.
   
   Good point, let's do that:
   1) Make virtqueue_add_outbuf()/inbuf() take a void * and len.
   2) Transfer users across to use that.
   3) Make everyone else use clean scatterlists with virtqueue_add_sgs[].
   4) Remove virtqueue_add_bufs().

   '''
 
> 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 6eff15b..b442500 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -40,6 +40,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,
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

-- 
Asias

Re: [PATCH 3/6] virtio_ring: inline internal vring functions more aggressively.

[Michael S. Tsirkin]

On Wed, Mar 06, 2013 at 04:22:09PM +1100, Rusty Russell wrote:
> 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> 

It's pretty cool that gcc is able to optimize it like this.
Which gcc version did you use here?


> 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);
>  

Re: [PATCH 3/6] virtio_ring: inline internal vring functions more aggressively.

[Rusty Russell]

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

> On Wed, Mar 06, 2013 at 04:22:09PM +1100, Rusty Russell wrote:
>> 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> 
>
> It's pretty cool that gcc is able to optimize it like this.
> Which gcc version did you use here?

Gcc has done this for quite a while now... I reported a bug about it in
2008: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35728

This was i686-linux-gnu-gcc-4.7 (Ubuntu/Linaro 4.7.2-2ubuntu1) 4.7.2.

Cheers,
Rusty.

Re: [PATCH 4/6] virtio_ring: virtqueue_add_outbuf / virtqueue_add_inbuf.

[Rusty Russell]

Asias He <asias@redhat.com> writes:
> On Wed, Mar 06, 2013 at 04:23:24PM +1100, Rusty Russell wrote:
>> 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> 
>
> So, what is the plan for the following ideas discussed in the other
> thread?
>
>    '''
>    > Looking at code, it seems that most users really have a single sg, in
>    > low memory. So how about simply passing void * instead of sg? Whoever
>    > has multiple sgs can use the rich interface.
>    
>    Good point, let's do that:
>    1) Make virtqueue_add_outbuf()/inbuf() take a void * and len.
>    2) Transfer users across to use that.
>    3) Make everyone else use clean scatterlists with virtqueue_add_sgs[].
>    4) Remove virtqueue_add_bufs().

Networking performance: there is still a performance penalty in using
virtqueue_add_sgs(), and it can't use a simple void * and len.

So I changed my mind.  Again...

Cheers,
Rusty.

Re: [PATCH 0/6] virtio_add_buf replacement.

[Rusty Russell]

Asias He <asias@redhat.com> writes:
> On Wed, Mar 06, 2013 at 04:15:02PM +1100, Rusty Russell wrote:
>> OK, so I've spent a few days benchmarking.  Turns out 80% of
>> virtio_add_buf cases are uni-directional (including the
>> always-performance-sensitive networking code), and that gets no
>> performance penalty (though tests with real networking would be
>> appreciated!).
>> 
>> I'm not reposting all the "convert driver to virtio_add_outbuf()"
>> patches: just the scsi one which I didn't have before.  I won't actually
>> remove virtio_add_buf() until the *following* merge window, just to be
>> sure.
>
> Why not send out all the patches in this series? It would be much easier
> for people to read in one thread.

I could re-spam people, but the patches are unchanged and uninteresting:
the scsi one I wanted an Ack for, however.

I really want people to review the core patches, and if they're fine,
I'll post the whole thing one last time before putting them in my
virtio-next branch (where they can't change).

Cheers,
Rusty.

Re: [PATCH 4/6] virtio_ring: virtqueue_add_outbuf / virtqueue_add_inbuf.

[Asias He]

On Thu, Mar 07, 2013 at 11:33:19AM +1100, Rusty Russell wrote:
> Asias He <asias@redhat.com> writes:
> > On Wed, Mar 06, 2013 at 04:23:24PM +1100, Rusty Russell wrote:
> >> 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> 
> >
> > So, what is the plan for the following ideas discussed in the other
> > thread?
> >
> >    '''
> >    > Looking at code, it seems that most users really have a single sg, in
> >    > low memory. So how about simply passing void * instead of sg? Whoever
> >    > has multiple sgs can use the rich interface.
> >    
> >    Good point, let's do that:
> >    1) Make virtqueue_add_outbuf()/inbuf() take a void * and len.
> >    2) Transfer users across to use that.
> >    3) Make everyone else use clean scatterlists with virtqueue_add_sgs[].
> >    4) Remove virtqueue_add_bufs().
> 
> Networking performance: there is still a performance penalty in using
> virtqueue_add_sgs(), and it can't use a simple void * and len.
> 
> So I changed my mind.  Again...

Ah, OK.

> Cheers,
> Rusty.

-- 
Asias

Re: [PATCH 0/6] virtio_add_buf replacement.

[Asias He]

On Thu, Mar 07, 2013 at 11:35:16AM +1100, Rusty Russell wrote:
> Asias He <asias@redhat.com> writes:
> > On Wed, Mar 06, 2013 at 04:15:02PM +1100, Rusty Russell wrote:
> >> OK, so I've spent a few days benchmarking.  Turns out 80% of
> >> virtio_add_buf cases are uni-directional (including the
> >> always-performance-sensitive networking code), and that gets no
> >> performance penalty (though tests with real networking would be
> >> appreciated!).
> >> 
> >> I'm not reposting all the "convert driver to virtio_add_outbuf()"
> >> patches: just the scsi one which I didn't have before.  I won't actually
> >> remove virtio_add_buf() until the *following* merge window, just to be
> >> sure.
> >
> > Why not send out all the patches in this series? It would be much easier
> > for people to read in one thread.
> 
> I could re-spam people, but the patches are unchanged and uninteresting:
> the scsi one I wanted an Ack for, however.
> 
> I really want people to review the core patches, and if they're fine,

People can skip the uninteresting patches easlily in one thread. Having
all the patches in one thread makes people see the whole picture. 

> I'll post the whole thing one last time before putting them in my
> virtio-next branch (where they can't change).

Okay, sounds good.

> Cheers,
> Rusty.

-- 
Asias