Re: [PATCH 4/4] virtio_ring: unify direct/indirect code paths.

[Rusty Russell <rusty@rustcorp.com.au>]

Rusty Russell <rusty@rustcorp.com.au> writes:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> On Thu, May 29, 2014 at 04:56:45PM +0930, Rusty Russell wrote:
>>> virtqueue_add() populates the virtqueue descriptor table from the sgs
>>> given.  If it uses an indirect descriptor table, then it puts a single
>>> descriptor in the descriptor table pointing to the kmalloc'ed indirect
>>> table where the sg is populated.
>>> +	for (i = 0; i < total_sg; i++)
>>> +		desc[i].next = i+1;
>>> +	return desc;
>>
>> Hmm we are doing an extra walk over descriptors here.
>> This might hurt performance esp for big descriptors.
>
> Yes, this needs to be benchmarked; since it's cache hot my gut feel is
> that it's a NOOP, but on modern machines my gut feel is always wrong.

CC's trimmed.

Well, I was almost right about being wrong.

I wrote a userspace virtio_ring microbench which does 10000000
virtqueue_add_outbuf() calls (which go indirect) and not much else.

Read as <MIN>-<MAX>(<MEAN>+/-<STDDEV>):
Current kernel:           936153354- 967745359(9.44739e+08+/-6.1e+06)ns
Using sg_next:           1061485790-1104800648(1.08254e+09+/-6.6e+06)ns
Unifying indirect path:  1214289435-1272686712(1.22564e+09+/-8e+06)ns
Using indirect flag:     1125610268-1183528965(1.14172e+09+/-8e+06)ns

Of course this might be lost in the noise on real networking, so that's
my job on Monday.

Subject: vring_bench: simple benchmark for adding descriptors to a virtqueue.

This userspace benchark uses the kernel code to add 8 16-element
scatterlists to a virtqueue, then consume them and start again.

For example:
	$ for i in `seq 10`; do ./vring_bench; done | stats --trim-outliers
	936153354-967745359(9.44739e+08+/-6.1e+06)ns
	9999872 returned

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/tools/virtio/.gitignore b/tools/virtio/.gitignore
index 1cfbb0157a46..ff32cca971d8 100644
--- a/tools/virtio/.gitignore
+++ b/tools/virtio/.gitignore
@@ -1,3 +1,4 @@
 *.d
 virtio_test
 vringh_test
+vring_bench
diff --git a/tools/virtio/Makefile b/tools/virtio/Makefile
index 3187c62d9814..103101273049 100644
--- a/tools/virtio/Makefile
+++ b/tools/virtio/Makefile
@@ -1,6 +1,7 @@
 all: test mod
-test: virtio_test vringh_test
+test: virtio_test vringh_test vring_bench
 virtio_test: virtio_ring.o virtio_test.o
+vring_bench: virtio_ring.o vring_bench.o
 vringh_test: vringh_test.o vringh.o virtio_ring.o
 
 CFLAGS += -g -O2 -Wall -I. -I ../../usr/include/ -Wno-pointer-sign -fno-strict-overflow -fno-strict-aliasing -fno-common -MMD -U_FORTIFY_SOURCE
@@ -9,6 +10,6 @@ mod:
 	${MAKE} -C `pwd`/../.. M=`pwd`/vhost_test
 .PHONY: all test mod clean
 clean:
-	${RM} *.o vringh_test virtio_test vhost_test/*.o vhost_test/.*.cmd \
+	${RM} *.o vringh_test virtio_test vring_bench vhost_test/*.o vhost_test/.*.cmd \
               vhost_test/Module.symvers vhost_test/modules.order *.d
 -include *.d
diff --git a/tools/virtio/linux/kernel.h b/tools/virtio/linux/kernel.h
index fba705963968..8dcff8e3374c 100644
--- a/tools/virtio/linux/kernel.h
+++ b/tools/virtio/linux/kernel.h
@@ -109,4 +109,7 @@ static inline void free_page(unsigned long addr)
 	(void) (&_min1 == &_min2);		\
 	_min1 < _min2 ? _min1 : _min2; })
 
+/* Just make it compile */
+#define list_for_each_entry(iter, list, member)
+
 #endif /* KERNEL_H */
diff --git a/tools/virtio/vring_bench.c b/tools/virtio/vring_bench.c
new file mode 100644
index 000000000000..0d7544fd26ad
--- /dev/null
+++ b/tools/virtio/vring_bench.c
@@ -0,0 +1,125 @@
+#define _GNU_SOURCE
+#include <time.h>
+#include <getopt.h>
+#include <string.h>
+#include <poll.h>
+#include <sys/eventfd.h>
+#include <stdlib.h>
+#include <assert.h>
+#include <unistd.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ring.h>
+
+/* Unused */
+void *__kmalloc_fake, *__kfree_ignore_start, *__kfree_ignore_end;
+
+static struct vring vring;
+static uint16_t last_avail_idx;
+static unsigned int returned;
+
+static bool vq_notify(struct virtqueue *vq)
+{
+	/* "Use" them all. */
+	while (vring.avail->idx != last_avail_idx) {
+		unsigned int i, head;
+
+		i = last_avail_idx++ & (vring.num - 1);
+		head = vring.avail->ring[i];
+		assert(head < vring.num);
+
+		i = vring.used->idx & (vring.num - 1);
+		vring.used->ring[i].id = head;
+		vring.used->ring[i].len = 0;
+		vring.used->idx++;
+	}
+	return true;
+}
+
+static void vq_callback(struct virtqueue *vq)
+{
+	unsigned int len;
+	void *p;
+
+	while ((p = virtqueue_get_buf(vq, &len)) != NULL)
+		returned++;
+}
+
+/* Ring size 128, just like qemu uses */
+#define VRING_NUM 128
+#define SG_SIZE 16
+
+static inline struct timespec time_sub(struct timespec recent,
+				       struct timespec old)
+{
+	struct timespec diff;
+
+	diff.tv_sec = recent.tv_sec - old.tv_sec;
+	if (old.tv_nsec > recent.tv_nsec) {
+		diff.tv_sec--;
+		diff.tv_nsec = 1000000000 + recent.tv_nsec - old.tv_nsec;
+	} else
+		diff.tv_nsec = recent.tv_nsec - old.tv_nsec;
+
+	return diff;
+}
+
+static struct timespec time_now(void)
+{
+	struct timespec ret;
+	clock_gettime(CLOCK_REALTIME, &ret);
+	return ret;
+}
+
+static inline uint64_t time_to_nsec(struct timespec t)
+{
+	uint64_t nsec;
+
+	nsec = t.tv_nsec + (uint64_t)t.tv_sec * 1000000000;
+	return nsec;
+}
+
+int main(int argc, char *argv[])
+{
+	struct virtqueue *vq;
+	struct virtio_device vdev;
+	void *ring;
+	unsigned int i, num;
+	int e;
+	struct scatterlist sg[SG_SIZE];
+	struct timespec start;
+
+	sg_init_table(sg, SG_SIZE);
+
+	e = posix_memalign(&ring, 4096, vring_size(VRING_NUM, 4096));
+	assert(e >= 0);
+
+	vdev.features[0] = (1UL << VIRTIO_RING_F_INDIRECT_DESC) |
+		(1UL << VIRTIO_RING_F_EVENT_IDX);
+
+	vq = vring_new_virtqueue(0, VRING_NUM, 4096, &vdev, true, ring,
+				 vq_notify, vq_callback, "benchmark");
+	assert(vq);
+	vring_init(&vring, VRING_NUM, ring, 4096);
+
+	num = atoi(argv[1] ?: "10000000");
+
+	start = time_now();
+	for (i = 0; i < num; i++) {
+	again:
+		e = virtqueue_add_outbuf(vq, sg, SG_SIZE, sg, GFP_ATOMIC);
+		if (e < 0) {
+			virtqueue_kick(vq);
+			vring_interrupt(0, vq);
+			goto again;
+		}
+	}
+	printf("%lluns\n",
+	       (long long)time_to_nsec(time_sub(time_now(), start)));
+	printf("%u returned\n", returned);
+	return 0;
+}