Skip to contentSkip to conversation

virtio: indirect ring entries

PATCH In review Mark McLoughlin 5 participants 29 messages Dec 18, 2008 – May 19, 2009

Subsystems (4)
Networking Network drivers Documentation virtio core
lore
Share & tools
lore

Mark McLoughlin

author
#0 lore raw

cover virtio: indirect ring entries

Hi Rusty,
Here's something that has (apparently) been kicked around for
a while now and I think makes some sense.

Avi has been especially pushing for it lately in the context
of high performance block I/O - I'll let him explain his thinking there.

The patches themselves are fairly trivial, there shouldn't be
anything too surprising here.

Cheers,
Mark.

author
#1 lore raw

PATCH 1/3 virtio: teach virtio_has_feature() about transport features

Drivers don't add transport features to their table, so we shouldn't check these with virtio_check_driver_offered_feature().

We could perhaps add an ->offered_feature() virtio_config_op, but that perhaps that would be overkill for a consitency check like this.

Signed-off-by Mark McLoughlin <[email protected]>
---
 include/linux/virtio_config.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
include/linux/virtio_config.h +3 -1
index bf8ec28..e4ba694 100644
@@ -99,7 +99,9 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev,
 	if (__builtin_constant_p(fbit))
 		BUILD_BUG_ON(fbit >= 32);
 
-	virtio_check_driver_offered_feature(vdev, fbit);
+	if (fbit < VIRTIO_TRANSPORT_F_START)
+		virtio_check_driver_offered_feature(vdev, fbit);
+
 	return test_bit(fbit, vdev->features);
 }
Signature · 1 line
1.6.0.5
author
#2 lore raw

PATCH 2/3 virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

Add a new feature flag for indirect ring entries. These are ring entries which point to a table of buffer descriptors.

The idea here is to increase the ring capacity by allowing a larger effective ring size whereby the ring size dictates the number of requests that may be outstanding, rather than the size of those requests.

This should be most effective in the case of block I/O where we can potentially benefit by concurrently dispatching a large number of large requests. Even in the simple case of single segment block requests, this results in a threefold increase in ring capacity.

Signed-off-by Mark McLoughlin <[email protected]>
---
 drivers/virtio/virtio_ring.c |   75 ++++++++++++++++++++++++++++++++++++++++-
 include/linux/virtio_ring.h  |    5 +++
 2 files changed, 78 insertions(+), 2 deletions(-)
drivers/virtio/virtio_ring.c +73 -2
index 5777196..2330c4b 100644
@@ -46,6 +46,9 @@ struct vring_virtqueue
 	/* Other side has made a mess, don't try any more. */
 	bool broken;
 
+	/* Host supports indirect buffers */
+	bool indirect;
+
 	/* Number of free buffers */
 	unsigned int num_free;
 	/* Head of free buffer list. */

@@ -70,6 +73,55 @@ struct vring_virtqueue
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
 
+/* 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 vring_desc *desc;
+	unsigned head;
+	int i;
+
+	desc = kmalloc((out + in) * sizeof(struct vring_desc), GFP_ATOMIC);
+	if (!desc)
+		return vq->vring.num;
+
+	/* 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++;
+	}
+	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++;
+	}
+
+	/* Last one doesn't continue. */
+	desc[i-1].flags &= ~VRING_DESC_F_NEXT;
+	desc[i-1].next = 0;
+
+	/* We're about to use a buffer */
+	vq->num_free--;
+
+	/* Use a single buffer which doesn't continue */
+	head = vq->free_head;
+	vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT;
+	vq->vring.desc[head].addr = virt_to_phys(desc);
+	vq->vring.desc[head].len = i * sizeof(struct vring_desc);
+
+	/* Update free pointer */
+	vq->free_head = vq->vring.desc[head].next;
+
+	return head;
+}
+
 static int vring_add_buf(struct virtqueue *_vq,
 			 struct scatterlist sg[],
 			 unsigned int out,

@@ -79,12 +131,21 @@ static int vring_add_buf(struct virtqueue *_vq,
 	struct vring_virtqueue *vq = to_vvq(_vq);
 	unsigned int i, avail, head, uninitialized_var(prev);
 
+	START_USE(vq);
+
 	BUG_ON(data == NULL);
+
+	/* If the host supports indirect descriptor tables, and we have multiple
+	 * buffers, then go indirect. FIXME: tune this threshold */
+	if (vq->indirect && (out + in) > 1 && vq->num_free) {
+		head = vring_add_indirect(vq, sg, out, in);
+		if (head != vq->vring.num)
+			goto add_head;
+	}
+
 	BUG_ON(out + in > vq->vring.num);
 	BUG_ON(out + in == 0);
 
-	START_USE(vq);
-
 	if (vq->num_free < out + in) {
 		pr_debug("Can't add buf len %i - avail = %i\n",
 			 out + in, vq->num_free);

@@ -121,6 +182,7 @@ static int vring_add_buf(struct virtqueue *_vq,
 	/* Update free pointer */
 	vq->free_head = i;
 
+add_head:
 	/* Set token. */
 	vq->data[head] = data;
 

@@ -164,6 +226,11 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
 
 	/* Put back on free list: find end */
 	i = head;
+
+	/* Free the indirect table */
+	if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT)
+		kfree(phys_to_virt(vq->vring.desc[i].addr));
+
 	while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) {
 		i = vq->vring.desc[i].next;
 		vq->num_free++;

@@ -305,6 +372,8 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 	vq->in_use = false;
 #endif
 
+	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
+
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback)
 		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;

@@ -332,6 +401,8 @@ void vring_transport_features(struct virtio_device *vdev)
 
 	for (i = VIRTIO_TRANSPORT_F_START; i < VIRTIO_TRANSPORT_F_END; i++) {
 		switch (i) {
+		case VIRTIO_RING_F_INDIRECT_DESC:
+			break;
 		default:
 			/* We don't understand this bit. */
 			clear_bit(i, vdev->features);
include/linux/virtio_ring.h +5 -0
index 71e0372..3828ae2 100644
@@ -14,6 +14,8 @@
 #define VRING_DESC_F_NEXT	1
 /* This marks a buffer as write-only (otherwise read-only). */
 #define VRING_DESC_F_WRITE	2
+/* This means the buffer contains a list of buffer descriptors. */
+#define VRING_DESC_F_INDIRECT	4
 
 /* The Host uses this in used->flags to advise the Guest: don't kick me when
  * you add a buffer.  It's unreliable, so it's simply an optimization.  Guest

@@ -24,6 +26,9 @@
  * optimization.  */
 #define VRING_AVAIL_F_NO_INTERRUPT	1
 
+/* We support indirect buffer descriptors */
+#define VIRTIO_RING_F_INDIRECT_DESC	28
+
 /* Virtio ring descriptors: 16 bytes.  These can chain together via "next". */
 struct vring_desc
 {
Signature · 1 line
1.6.0.5
author
#3 lore raw

PATCH 3/3 lguest: add support for indirect ring entries

Support the VIRTIO_RING_F_INDIRECT_DESC feature.

This is a simple matter of changing the descriptor walking code to operate on a struct vring_desc* and supplying it with an indirect table if detected.

Signed-off-by Mark McLoughlin <[email protected]>
---
 Documentation/lguest/lguest.c |   41 +++++++++++++++++++++++++++++------------
 1 files changed, 29 insertions(+), 12 deletions(-)
Documentation/lguest/lguest.c +29 -12
index f2dbbf3..3cd388f 100644
@@ -623,20 +623,21 @@ static void *_check_pointer(unsigned long addr, unsigned int size,
 /* Each buffer in the virtqueues is actually a chain of descriptors.  This
  * function returns the next descriptor in the chain, or vq->vring.num if we're
  * at the end. */
-static unsigned next_desc(struct virtqueue *vq, unsigned int i)
+static unsigned next_desc(struct vring_desc *desc,
+			  unsigned int i, unsigned int max)
 {
 	unsigned int next;
 
 	/* If this descriptor says it doesn't chain, we're done. */
-	if (!(vq->vring.desc[i].flags & VRING_DESC_F_NEXT))
-		return vq->vring.num;
+	if (!(desc[i].flags & VRING_DESC_F_NEXT))
+		return max;
 
 	/* Check they're not leading us off end of descriptors. */
-	next = vq->vring.desc[i].next;
+	next = desc[i].next;
 	/* Make sure compiler knows to grab that: we don't want it changing! */
 	wmb();
 
-	if (next >= vq->vring.num)
+	if (next >= max)
 		errx(1, "Desc next is %u", next);
 
 	return next;

@@ -653,7 +654,8 @@ static unsigned get_vq_desc(struct virtqueue *vq,
 			    struct iovec iov[],
 			    unsigned int *out_num, unsigned int *in_num)
 {
-	unsigned int i, head;
+	struct vring_desc *desc;
+	unsigned int i, head, max;
 	u16 last_avail;
 
 	/* Check it isn't doing very strange things with descriptor numbers. */

@@ -678,15 +680,28 @@ static unsigned get_vq_desc(struct virtqueue *vq,
 	/* When we start there are none of either input nor output. */
 	*out_num = *in_num = 0;
 
+	max = vq->vring.num;
+	desc = vq->vring.desc;
 	i = head;
+
+	/* If this is an indirect entry, then this buffer contains a descriptor
+	 * table which we handle as if it's any normal descriptor chain. */
+	if (desc[i].flags & VRING_DESC_F_INDIRECT) {
+		if (desc[i].len % sizeof(struct vring_desc))
+			errx(1, "Invalid size for indirect buffer table");
+
+		max = desc[i].len / sizeof(struct vring_desc);
+		desc = check_pointer(desc[i].addr, desc[i].len);
+		i = 0;
+	}
+
 	do {
 		/* Grab the first descriptor, and check it's OK. */
-		iov[*out_num + *in_num].iov_len = vq->vring.desc[i].len;
+		iov[*out_num + *in_num].iov_len = desc[i].len;
 		iov[*out_num + *in_num].iov_base
-			= check_pointer(vq->vring.desc[i].addr,
-					vq->vring.desc[i].len);
+			= check_pointer(desc[i].addr, desc[i].len);
 		/* If this is an input descriptor, increment that count. */
-		if (vq->vring.desc[i].flags & VRING_DESC_F_WRITE)
+		if (desc[i].flags & VRING_DESC_F_WRITE)
 			(*in_num)++;
 		else {
 			/* If it's an output descriptor, they're all supposed

@@ -697,9 +712,9 @@ static unsigned get_vq_desc(struct virtqueue *vq,
 		}
 
 		/* If we've got too many, that implies a descriptor loop. */
-		if (*out_num + *in_num > vq->vring.num)
+		if (*out_num + *in_num > max)
 			errx(1, "Looped descriptor");
-	} while ((i = next_desc(vq, i)) != vq->vring.num);
+	} while ((i = next_desc(desc, i, max)) != max);
 
 	vq->inflight++;
 	return head;

@@ -1502,6 +1517,8 @@ static void setup_tun_net(char *arg)
 	add_feature(dev, VIRTIO_NET_F_HOST_TSO4);
 	add_feature(dev, VIRTIO_NET_F_HOST_TSO6);
 	add_feature(dev, VIRTIO_NET_F_HOST_ECN);
+	/* we handle indirect ring entries */
+	add_feature(dev, VIRTIO_RING_F_INDIRECT_DESC);
 	set_config(dev, sizeof(conf), &conf);
 
 	/* We don't need the socket any more; setup is done. */
Signature · 1 line
1.6.0.5
#4 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Mark McLoughlin

Hi Mark,

Quoted 9 lines · Mark McLoughlin
drivers/virtio/virtio_ring.c +1 -0
index 5777196..2330c4b 100644
@@ -70,6 +73,55 @@ struct vring_virtqueue
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
 
+/* Set up an indirect table of descriptors and add it to the queue. */

drivers/virtio/virtio_ring.c struct vring_virtqueue · line 86

Quoted 10 lines
drivers/virtio/virtio_ring.c +10 -0
+static int vring_add_indirect(struct vring_virtqueue *vq,
+			      struct scatterlist sg[],
+			      unsigned int out,
+			      unsigned int in)
+{
+	struct vring_desc *desc;
+	unsigned head;
+	int i;
+
+	desc = kmalloc((out + in) * sizeof(struct vring_desc), GFP_ATOMIC);

kmalloc() returns ZERO_SIZE_PTR, if (out + in) == 0

Quoted 12 lines
drivers/virtio/virtio_ring.c +12 -0
+	if (!desc)
+		return vq->vring.num;
+
+	/* 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++;
+	}
+	for (; i < (out + in); i++) {

drivers/virtio/virtio_ring.c struct vring_virtqueue · line 108

Quoted 10 lines
drivers/virtio/virtio_ring.c +10 -0
+		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++;
+	}
+
+	/* Last one doesn't continue. */
+	desc[i-1].flags &= ~VRING_DESC_F_NEXT;
+	desc[i-1].next = 0;

So this array index can fail (be -1).
Please check and avoid within this function.

Best Regards

Ingo Oeser

#5 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Mark McLoughlin

Hi Mark,

Quoted 9 lines · Mark McLoughlin
drivers/virtio/virtio_ring.c +1 -0
index 5777196..2330c4b 100644
@@ -70,6 +73,55 @@ struct vring_virtqueue
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
 
+/* Set up an indirect table of descriptors and add it to the queue. */

drivers/virtio/virtio_ring.c struct vring_virtqueue · line 86

Quoted 10 lines
drivers/virtio/virtio_ring.c +10 -0
+static int vring_add_indirect(struct vring_virtqueue *vq,
+			      struct scatterlist sg[],
+			      unsigned int out,
+			      unsigned int in)
+{
+	struct vring_desc *desc;
+	unsigned head;
+	int i;
+
+	desc = kmalloc((out + in) * sizeof(struct vring_desc), GFP_ATOMIC);

kmalloc() returns ZERO_SIZE_PTR, if (out + in) == 0

Quoted 12 lines
drivers/virtio/virtio_ring.c +12 -0
+	if (!desc)
+		return vq->vring.num;
+
+	/* 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++;
+	}
+	for (; i < (out + in); i++) {

drivers/virtio/virtio_ring.c struct vring_virtqueue · line 108

Quoted 10 lines
drivers/virtio/virtio_ring.c +10 -0
+		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++;
+	}
+
+	/* Last one doesn't continue. */
+	desc[i-1].flags &= ~VRING_DESC_F_NEXT;
+	desc[i-1].next = 0;

So this array index can fail (be -1).
Please check and avoid within this function.

Best Regards

Ingo Oeser

author
#6 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

Hi Ingo,

Quoted 11 lines · Ingo Oeser

Hi Mark,

Quoted 8 lines · Mark McLoughlin
drivers/virtio/virtio_ring.c +0 -0
index 5777196..2330c4b 100644
@@ -70,6 +73,55 @@ struct vring_virtqueue
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
Quoted 13 lines

drivers/virtio/virtio_ring.c struct vring_virtqueue · line 86

Quoted 11 lines
drivers/virtio/virtio_ring.c +11 -0
+/* 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 vring_desc *desc;
+	unsigned head;
+	int i;
+
+	desc = kmalloc((out + in) * sizeof(struct vring_desc), GFP_ATOMIC);

kmalloc() returns ZERO_SIZE_PTR, if (out + in) == 0

vring_add_buf() has:

BUG_ON(out + in == 0)

I should just add that here too before the kmalloc() call.

Thanks,
Mark.

author
#7 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Ingo Oeser

Hi Ingo,

Quoted 11 lines · Ingo Oeser

Hi Mark,

Quoted 8 lines · Mark McLoughlin
drivers/virtio/virtio_ring.c +0 -0
index 5777196..2330c4b 100644
@@ -70,6 +73,55 @@ struct vring_virtqueue
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
Quoted 13 lines

drivers/virtio/virtio_ring.c struct vring_virtqueue · line 86

Quoted 11 lines
drivers/virtio/virtio_ring.c +11 -0
+/* 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 vring_desc *desc;
+	unsigned head;
+	int i;
+
+	desc = kmalloc((out + in) * sizeof(struct vring_desc), GFP_ATOMIC);

kmalloc() returns ZERO_SIZE_PTR, if (out + in) == 0

vring_add_buf() has:

BUG_ON(out + in == 0)

I should just add that here too before the kmalloc() call.

Thanks,
Mark.

author
#8 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Mark McLoughlin

Hi Rusty,

Quoted 8 lines · Mark McLoughlin

Add a new feature flag for indirect ring entries. These are ring entries which point to a table of buffer descriptors.

The idea here is to increase the ring capacity by allowing a larger effective ring size whereby the ring size dictates the number of requests that may be outstanding, rather than the size of those requests.

Quoted 4 lines

This should be most effective in the case of block I/O where we can potentially benefit by concurrently dispatching a large number of large requests. Even in the simple case of single segment block requests, this results in a threefold increase in ring capacity.

Apparently, this would also be useful for the windows virtio-net drivers.

Dor can explain further, but apparently Windows has been observed passing the driver a packet with >256 fragments when using TSO.

With a ring size of 256, the guest can either drop the packet or copy it into a single buffer. We'd much rather if we could use an indirect ring entry to pass this number of fragments without copying.

For reference the original patch was here:

http://lkml.org/lkml/2008/12/18/212

Cheers,
Mark.

author
#9 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Mark McLoughlin

Hi Rusty,

Quoted 8 lines · Mark McLoughlin

Add a new feature flag for indirect ring entries. These are ring entries which point to a table of buffer descriptors.

The idea here is to increase the ring capacity by allowing a larger effective ring size whereby the ring size dictates the number of requests that may be outstanding, rather than the size of those requests.

Quoted 4 lines

This should be most effective in the case of block I/O where we can potentially benefit by concurrently dispatching a large number of large requests. Even in the simple case of single segment block requests, this results in a threefold increase in ring capacity.

Apparently, this would also be useful for the windows virtio-net drivers.

Dor can explain further, but apparently Windows has been observed passing the driver a packet with >256 fragments when using TSO.

With a ring size of 256, the guest can either drop the packet or copy it into a single buffer. We'd much rather if we could use an indirect ring entry to pass this number of fragments without copying.

For reference the original patch was here:

http://lkml.org/lkml/2008/12/18/212

Cheers,
Mark.

#10 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

Quoted 21 lines · Mark McLoughlin

Hi Rusty,

Quoted 8 lines · Mark McLoughlin

Add a new feature flag for indirect ring entries. These are ring entries which point to a table of buffer descriptors.

The idea here is to increase the ring capacity by allowing a larger effective ring size whereby the ring size dictates the number of requests that may be outstanding, rather than the size of those requests.

Quoted 5 lines

This should be most effective in the case of block I/O where we can potentially benefit by concurrently dispatching a large number of large requests. Even in the simple case of single segment block requests, this results in a threefold increase in ring capacity.

Apparently, this would also be useful for the windows virtio-net drivers.

Quoted 7 lines

Dor can explain further, but apparently Windows has been observed passing the driver a packet with >256 fragments when using TSO.

With a ring size of 256, the guest can either drop the packet or copy it into a single buffer. We'd much rather if we could use an indirect ring entry to pass this number of fragments without copying.

Correct. This is what we do in Windows today.
The problem arises when using sending lots of small packets from the win guest and TSO. Windows prepare very big scatter gather list, bigger than the ring size (270 fragments).
Having indirect ring entries is good both for this and also for block io, as described
above.

Cheers,
Dor

Quoted 12 lines

For reference the original patch was here:

http://lkml.org/lkml/2008/12/18/212

Cheers,
Mark.

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/virtualization

#11 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

Quoted 5 lines · Dor Laor
Quoted 4 lines · Mark McLoughlin

Hi Rusty,

On Thu, 2008-12-18 at 17:10 +0000, Mark McLoughlin wrote:

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

Add a new feature flag for indirect ring entries. These are ring entries which point to a table of buffer descriptors.

The idea here is to increase the ring capacity by allowing a larger effective ring size whereby the ring size dictates the number of requests that may be outstanding, rather than the size of those requests.

OK, just so we track our mistakes.

1) virtio_rings must be physically contiguous, even though they actually have two independent parts.
2) The number of elements in a ring must be a power of 2.
3) virtio_pci tells the guest what number of elements to use.
4) The guest has to allocate that much physically contiguous memory, or fail.

In practice, 128 elements = 2 pages, 256 elements = 3 pages, 512 elements = 5 pages. Order 1, order 2, order 3 under Linux. 1 is OK, 2 is iffy, 3 is hard.

Blocked from doing the simpler thing, we've decided to go with a layer of indirection. But the patch is simple and clean, so there's nothing fundamental to object to.

I can't find 3/3, did it go missing?

Thanks,
Rusty.

Quoted 40 lines
Quoted 10 lines
Quoted 6 lines

This should be most effective in the case of block I/O where we can potentially benefit by concurrently dispatching a large number of large requests. Even in the simple case of single segment block requests, this results in a threefold increase in ring capacity.

Apparently, this would also be useful for the windows virtio-net drivers.

Quoted 7 lines

Dor can explain further, but apparently Windows has been observed passing the driver a packet with >256 fragments when using TSO.

With a ring size of 256, the guest can either drop the packet or copy it into a single buffer. We'd much rather if we could use an indirect ring entry to pass this number of fragments without copying.

Correct. This is what we do in Windows today.
The problem arises when using sending lots of small packets from the win guest and TSO. Windows prepare very big scatter gather list, bigger than the ring size (270 fragments).
Having indirect ring entries is good both for this and also for block io, as described
above.

Cheers,
Dor

Quoted 12 lines

For reference the original patch was here:

http://lkml.org/lkml/2008/12/18/212

Cheers,
Mark.

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/virtualization

author
#12 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

Quoted 22 lines · Rusty Russell
Quoted 5 lines · Dor Laor
Quoted 4 lines · Mark McLoughlin

Hi Rusty,

On Thu, 2008-12-18 at 17:10 +0000, Mark McLoughlin wrote:

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

Add a new feature flag for indirect ring entries. These are ring entries which point to a table of buffer descriptors.

The idea here is to increase the ring capacity by allowing a larger effective ring size whereby the ring size dictates the number of requests that may be outstanding, rather than the size of those requests.

OK, just so we track our mistakes.

1) virtio_rings must be physically contiguous, even though they actually have two independent parts.
2) The number of elements in a ring must be a power of 2.
3) virtio_pci tells the guest what number of elements to use.
4) The guest has to allocate that much physically contiguous memory, or fail.

Quoted 7 lines

In practice, 128 elements = 2 pages, 256 elements = 3 pages, 512 elements = 5 pages. Order 1, order 2, order 3 under Linux. 1 is OK, 2 is iffy, 3 is hard.

Blocked from doing the simpler thing, we've decided to go with a layer of indirection. But the patch is simple and clean, so there's nothing fundamental to object to.

Still have one FIXME in the patch worth looking at - at what point should we use an indirect entry rather than consuming N entries?

Quoted 1 line

I can't find 3/3, did it go missing?

Following up with all three patches again.

Cheers,
Mark.

author
#13 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Rusty Russell
Quoted 22 lines · Rusty Russell
Quoted 5 lines · Dor Laor
Quoted 4 lines · Mark McLoughlin

Hi Rusty,

On Thu, 2008-12-18 at 17:10 +0000, Mark McLoughlin wrote:

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

Add a new feature flag for indirect ring entries. These are ring entries which point to a table of buffer descriptors.

The idea here is to increase the ring capacity by allowing a larger effective ring size whereby the ring size dictates the number of requests that may be outstanding, rather than the size of those requests.

OK, just so we track our mistakes.

1) virtio_rings must be physically contiguous, even though they actually have two independent parts.
2) The number of elements in a ring must be a power of 2.
3) virtio_pci tells the guest what number of elements to use.
4) The guest has to allocate that much physically contiguous memory, or fail.

Quoted 7 lines

In practice, 128 elements = 2 pages, 256 elements = 3 pages, 512 elements = 5 pages. Order 1, order 2, order 3 under Linux. 1 is OK, 2 is iffy, 3 is hard.

Blocked from doing the simpler thing, we've decided to go with a layer of indirection. But the patch is simple and clean, so there's nothing fundamental to object to.

Still have one FIXME in the patch worth looking at - at what point should we use an indirect entry rather than consuming N entries?

Quoted 1 line

I can't find 3/3, did it go missing?

Following up with all three patches again.

Cheers,
Mark.

author
#14 lore raw

PATCH 1/3 virtio: teach virtio_has_feature() about transport features

Drivers don't add transport features to their table, so we shouldn't check these with virtio_check_driver_offered_feature().

We could perhaps add an ->offered_feature() virtio_config_op, but that perhaps that would be overkill for a consitency check like this.

Signed-off-by Mark McLoughlin <[email protected]>
---
 include/linux/virtio_config.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
include/linux/virtio_config.h +3 -1
index bf8ec28..e4ba694 100644
@@ -99,7 +99,9 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev,
 	if (__builtin_constant_p(fbit))
 		BUILD_BUG_ON(fbit >= 32);
 
-	virtio_check_driver_offered_feature(vdev, fbit);
+	if (fbit < VIRTIO_TRANSPORT_F_START)
+		virtio_check_driver_offered_feature(vdev, fbit);
+
 	return test_bit(fbit, vdev->features);
 }
Signature · 1 line
1.6.0.6
author
#15 lore raw

PATCH 2/3 virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

Add a new feature flag for indirect ring entries. These are ring entries which point to a table of buffer descriptors.

The idea here is to increase the ring capacity by allowing a larger effective ring size whereby the ring size dictates the number of requests that may be outstanding, rather than the size of those requests.

This should be most effective in the case of block I/O where we can potentially benefit by concurrently dispatching a large number of large requests. Even in the simple case of single segment block requests, this results in a threefold increase in ring capacity.

Signed-off-by Mark McLoughlin <[email protected]>
---
 drivers/virtio/virtio_ring.c |   75 ++++++++++++++++++++++++++++++++++++++++-
 include/linux/virtio_ring.h  |    5 +++
 2 files changed, 78 insertions(+), 2 deletions(-)
drivers/virtio/virtio_ring.c +73 -2
index 5c52369..ebccea8 100644
@@ -52,6 +52,9 @@ struct vring_virtqueue
 	/* Other side has made a mess, don't try any more. */
 	bool broken;
 
+	/* Host supports indirect buffers */
+	bool indirect;
+
 	/* Number of free buffers */
 	unsigned int num_free;
 	/* Head of free buffer list. */

@@ -76,6 +79,55 @@ struct vring_virtqueue
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
 
+/* 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 vring_desc *desc;
+	unsigned head;
+	int i;
+
+	desc = kmalloc((out + in) * sizeof(struct vring_desc), GFP_ATOMIC);
+	if (!desc)
+		return vq->vring.num;
+
+	/* 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++;
+	}
+	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++;
+	}
+
+	/* Last one doesn't continue. */
+	desc[i-1].flags &= ~VRING_DESC_F_NEXT;
+	desc[i-1].next = 0;
+
+	/* We're about to use a buffer */
+	vq->num_free--;
+
+	/* Use a single buffer which doesn't continue */
+	head = vq->free_head;
+	vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT;
+	vq->vring.desc[head].addr = virt_to_phys(desc);
+	vq->vring.desc[head].len = i * sizeof(struct vring_desc);
+
+	/* Update free pointer */
+	vq->free_head = vq->vring.desc[head].next;
+
+	return head;
+}
+
 static int vring_add_buf(struct virtqueue *_vq,
 			 struct scatterlist sg[],
 			 unsigned int out,

@@ -85,12 +137,21 @@ static int vring_add_buf(struct virtqueue *_vq,
 	struct vring_virtqueue *vq = to_vvq(_vq);
 	unsigned int i, avail, head, uninitialized_var(prev);
 
+	START_USE(vq);
+
 	BUG_ON(data == NULL);
+
+	/* If the host supports indirect descriptor tables, and we have multiple
+	 * buffers, then go indirect. FIXME: tune this threshold */
+	if (vq->indirect && (out + in) > 1 && vq->num_free) {
+		head = vring_add_indirect(vq, sg, out, in);
+		if (head != vq->vring.num)
+			goto add_head;
+	}
+
 	BUG_ON(out + in > vq->vring.num);
 	BUG_ON(out + in == 0);
 
-	START_USE(vq);
-
 	if (vq->num_free < out + in) {
 		pr_debug("Can't add buf len %i - avail = %i\n",
 			 out + in, vq->num_free);

@@ -127,6 +188,7 @@ static int vring_add_buf(struct virtqueue *_vq,
 	/* Update free pointer */
 	vq->free_head = i;
 
+add_head:
 	/* Set token. */
 	vq->data[head] = data;
 

@@ -170,6 +232,11 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
 
 	/* Put back on free list: find end */
 	i = head;
+
+	/* Free the indirect table */
+	if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT)
+		kfree(phys_to_virt(vq->vring.desc[i].addr));
+
 	while (vq->vring.desc[i].flags & VRING_DESC_F_NEXT) {
 		i = vq->vring.desc[i].next;
 		vq->num_free++;

@@ -311,6 +378,8 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 	vq->in_use = false;
 #endif
 
+	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
+
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback)
 		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;

@@ -338,6 +407,8 @@ void vring_transport_features(struct virtio_device *vdev)
 
 	for (i = VIRTIO_TRANSPORT_F_START; i < VIRTIO_TRANSPORT_F_END; i++) {
 		switch (i) {
+		case VIRTIO_RING_F_INDIRECT_DESC:
+			break;
 		default:
 			/* We don't understand this bit. */
 			clear_bit(i, vdev->features);
include/linux/virtio_ring.h +5 -0
index 71e0372..3828ae2 100644
@@ -14,6 +14,8 @@
 #define VRING_DESC_F_NEXT	1
 /* This marks a buffer as write-only (otherwise read-only). */
 #define VRING_DESC_F_WRITE	2
+/* This means the buffer contains a list of buffer descriptors. */
+#define VRING_DESC_F_INDIRECT	4
 
 /* The Host uses this in used->flags to advise the Guest: don't kick me when
  * you add a buffer.  It's unreliable, so it's simply an optimization.  Guest

@@ -24,6 +26,9 @@
  * optimization.  */
 #define VRING_AVAIL_F_NO_INTERRUPT	1
 
+/* We support indirect buffer descriptors */
+#define VIRTIO_RING_F_INDIRECT_DESC	28
+
 /* Virtio ring descriptors: 16 bytes.  These can chain together via "next". */
 struct vring_desc
 {
Signature · 1 line
1.6.0.6
author
#16 lore raw

PATCH 3/3 lguest: add support for indirect ring entries

Support the VIRTIO_RING_F_INDIRECT_DESC feature.

This is a simple matter of changing the descriptor walking code to operate on a struct vring_desc* and supplying it with an indirect table if detected.

Signed-off-by Mark McLoughlin <[email protected]>
---
 Documentation/lguest/lguest.c |   41 +++++++++++++++++++++++++++++------------
 1 files changed, 29 insertions(+), 12 deletions(-)
Documentation/lguest/lguest.c +29 -12
index d36fcc0..010c8bc 100644
@@ -623,20 +623,21 @@ static void *_check_pointer(unsigned long addr, unsigned int size,
 /* Each buffer in the virtqueues is actually a chain of descriptors.  This
  * function returns the next descriptor in the chain, or vq->vring.num if we're
  * at the end. */
-static unsigned next_desc(struct virtqueue *vq, unsigned int i)
+static unsigned next_desc(struct vring_desc *desc,
+			  unsigned int i, unsigned int max)
 {
 	unsigned int next;
 
 	/* If this descriptor says it doesn't chain, we're done. */
-	if (!(vq->vring.desc[i].flags & VRING_DESC_F_NEXT))
-		return vq->vring.num;
+	if (!(desc[i].flags & VRING_DESC_F_NEXT))
+		return max;
 
 	/* Check they're not leading us off end of descriptors. */
-	next = vq->vring.desc[i].next;
+	next = desc[i].next;
 	/* Make sure compiler knows to grab that: we don't want it changing! */
 	wmb();
 
-	if (next >= vq->vring.num)
+	if (next >= max)
 		errx(1, "Desc next is %u", next);
 
 	return next;

@@ -653,7 +654,8 @@ static unsigned get_vq_desc(struct virtqueue *vq,
 			    struct iovec iov[],
 			    unsigned int *out_num, unsigned int *in_num)
 {
-	unsigned int i, head;
+	struct vring_desc *desc;
+	unsigned int i, head, max;
 	u16 last_avail;
 
 	/* Check it isn't doing very strange things with descriptor numbers. */

@@ -678,15 +680,28 @@ static unsigned get_vq_desc(struct virtqueue *vq,
 	/* When we start there are none of either input nor output. */
 	*out_num = *in_num = 0;
 
+	max = vq->vring.num;
+	desc = vq->vring.desc;
 	i = head;
+
+	/* If this is an indirect entry, then this buffer contains a descriptor
+	 * table which we handle as if it's any normal descriptor chain. */
+	if (desc[i].flags & VRING_DESC_F_INDIRECT) {
+		if (desc[i].len % sizeof(struct vring_desc))
+			errx(1, "Invalid size for indirect buffer table");
+
+		max = desc[i].len / sizeof(struct vring_desc);
+		desc = check_pointer(desc[i].addr, desc[i].len);
+		i = 0;
+	}
+
 	do {
 		/* Grab the first descriptor, and check it's OK. */
-		iov[*out_num + *in_num].iov_len = vq->vring.desc[i].len;
+		iov[*out_num + *in_num].iov_len = desc[i].len;
 		iov[*out_num + *in_num].iov_base
-			= check_pointer(vq->vring.desc[i].addr,
-					vq->vring.desc[i].len);
+			= check_pointer(desc[i].addr, desc[i].len);
 		/* If this is an input descriptor, increment that count. */
-		if (vq->vring.desc[i].flags & VRING_DESC_F_WRITE)
+		if (desc[i].flags & VRING_DESC_F_WRITE)
 			(*in_num)++;
 		else {
 			/* If it's an output descriptor, they're all supposed

@@ -697,9 +712,9 @@ static unsigned get_vq_desc(struct virtqueue *vq,
 		}
 
 		/* If we've got too many, that implies a descriptor loop. */
-		if (*out_num + *in_num > vq->vring.num)
+		if (*out_num + *in_num > max)
 			errx(1, "Looped descriptor");
-	} while ((i = next_desc(vq, i)) != vq->vring.num);
+	} while ((i = next_desc(desc, i, max)) != max);
 
 	vq->inflight++;
 	return head;

@@ -1502,6 +1517,8 @@ static void setup_tun_net(char *arg)
 	add_feature(dev, VIRTIO_NET_F_HOST_TSO4);
 	add_feature(dev, VIRTIO_NET_F_HOST_TSO6);
 	add_feature(dev, VIRTIO_NET_F_HOST_ECN);
+	/* we handle indirect ring entries */
+	add_feature(dev, VIRTIO_RING_F_INDIRECT_DESC);
 	set_config(dev, sizeof(conf), &conf);
 
 	/* We don't need the socket any more; setup is done. */
Signature · 1 line
1.6.0.6
#17 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Mark McLoughlin
Quoted 6 lines · Mark McLoughlin
Quoted 3 lines

Blocked from doing the simpler thing, we've decided to go with a layer of indirection. But the patch is simple and clean, so there's nothing fundamental to object to.

Still have one FIXME in the patch worth looking at - at what point should we use an indirect entry rather than consuming N entries?

OK, I've applied these as is. I'm doing some virtio net benchmarking (under lguest); I'll see if I can get a reasonable figure. I don't think there's an obvious right answer; it depends how many more packets are coming as well as how many descriptors each will use.

Thanks,
Rusty.

#18 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Mark McLoughlin
Quoted 6 lines · Mark McLoughlin
Quoted 3 lines

Blocked from doing the simpler thing, we've decided to go with a layer of indirection. But the patch is simple and clean, so there's nothing fundamental to object to.

Still have one FIXME in the patch worth looking at - at what point should we use an indirect entry rather than consuming N entries?

OK, I've applied these as is. I'm doing some virtio net benchmarking (under lguest); I'll see if I can get a reasonable figure. I don't think there's an obvious right answer; it depends how many more packets are coming as well as how many descriptors each will use.

Thanks,
Rusty.

#19 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Mark McLoughlin
Quoted 2 lines · Mark McLoughlin

Still have one FIXME in the patch worth looking at - at what point should we use an indirect entry rather than consuming N entries?

Is this overkill?

Rusty.

virtio: use indirect buffers based on demand (heuristic)

virtio_ring uses a ring buffer of descriptors: indirect support allows a single descriptor to refer to a table of descriptors. This saves space in the ring, but requires a kmalloc/kfree.

Rather than try to figure out what the right threshold at which to use indirect buffers, we drop the threshold dynamically when the ring is under stress.

Note: to stress this, I reduced the ring size to 32 in lguest, and a 1G send reduced the threshold to 9.

Note2: I moved the BUG_ON()s above the indirect test, where they belong (indirect falls thru on OOM, so the constraints still apply).

Signed-off-by Rusty Russell <[email protected]>
drivers/virtio/virtio_ring.c +51 -9
@@ -63,6 +63,8 @@ struct vring_virtqueue
 
 	/* Host supports indirect buffers */
 	bool indirect;
+	/* Threshold before we go indirect. */
+	unsigned int indirect_threshold;
 
 	/* Number of free buffers */
 	unsigned int num_free;

@@ -137,6 +141,32 @@ static int vring_add_indirect(struct vri
 	return head;
 }
 
+static void adjust_threshold(struct vring_virtqueue *vq,
+			     unsigned int out, unsigned int in)
+{
+	/* There are really two species of virtqueue, and it matters here.
+	 * If there are no output parts, it's a "normally full" receive queue,
+	 * otherwise it's a "normally empty" send queue. */
+	if (out) {
+		/* Leave threshold unless we're full. */
+		if (out + in < vq->num_free)
+			return;
+	} else {
+		/* Leave threshold unless we're empty. */
+		if (vq->num_free != vq->vring.num)
+			return;
+	}
+
+	/* Never drop threshold below 1 */
+	vq->indirect_threshold /= 2;
+	vq->indirect_threshold |= 1;
+
+	printk("%s %s: indirect threshold %u (%u+%u vs %u)\n", 
+	       dev_name(&vq->vq.vdev->dev),
+	       vq->vq.name, vq->indirect_threshold,
+	       out, in, vq->num_free);
+}
+
 static int vring_add_buf(struct virtqueue *_vq,
 			 struct scatterlist sg[],
 			 unsigned int out,

@@ -149,18 +179,31 @@ static int vring_add_buf(struct virtqueu
 	START_USE(vq);
 
 	BUG_ON(data == NULL);
-
-	/* If the host supports indirect descriptor tables, and we have multiple
-	 * buffers, then go indirect. FIXME: tune this threshold */
-	if (vq->indirect && (out + in) > 1 && vq->num_free) {
-		head = vring_add_indirect(vq, sg, out, in);
-		if (head != vq->vring.num)
-			goto add_head;
-	}
-
 	BUG_ON(out + in > vq->vring.num);
 	BUG_ON(out + in == 0);
 
+	/* If the host supports indirect descriptor tables, consider it. */
+	if (vq->indirect) {
+		bool try_indirect;
+
+		/* We tweak the threshold automatically. */
+		adjust_threshold(vq, out, in);
+
+		/* If we can't fit any at all, fall through. */
+		if (vq->num_free == 0)
+			try_indirect = false;
+		else if (out + in > vq->num_free)
+			try_indirect = true;
+		else
+			try_indirect = (out + in > vq->indirect_threshold);
+
+		if (try_indirect) {
+			head = vring_add_indirect(vq, sg, out, in);
+			if (head != vq->vring.num)
+				goto add_head;
+		}
+	}
+
 	if (vq->num_free < out + in) {
 		pr_debug("Can't add buf len %i - avail = %i\n",
 			 out + in, vq->num_free);

@@ -391,6 +434,7 @@ struct virtqueue *vring_new_virtqueue(un
 #endif
 
 	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
+	vq->indirect_threshold = num;
 
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback)
#20 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Mark McLoughlin
Quoted 2 lines · Mark McLoughlin

Still have one FIXME in the patch worth looking at - at what point should we use an indirect entry rather than consuming N entries?

Is this overkill?

Rusty.

virtio: use indirect buffers based on demand (heuristic)

virtio_ring uses a ring buffer of descriptors: indirect support allows a single descriptor to refer to a table of descriptors. This saves space in the ring, but requires a kmalloc/kfree.

Rather than try to figure out what the right threshold at which to use indirect buffers, we drop the threshold dynamically when the ring is under stress.

Note: to stress this, I reduced the ring size to 32 in lguest, and a 1G send reduced the threshold to 9.

Note2: I moved the BUG_ON()s above the indirect test, where they belong (indirect falls thru on OOM, so the constraints still apply).

Signed-off-by Rusty Russell <[email protected]>
drivers/virtio/virtio_ring.c +51 -9
@@ -63,6 +63,8 @@ struct vring_virtqueue
 
 	/* Host supports indirect buffers */
 	bool indirect;
+	/* Threshold before we go indirect. */
+	unsigned int indirect_threshold;
 
 	/* Number of free buffers */
 	unsigned int num_free;

@@ -137,6 +141,32 @@ static int vring_add_indirect(struct vri
 	return head;
 }
 
+static void adjust_threshold(struct vring_virtqueue *vq,
+			     unsigned int out, unsigned int in)
+{
+	/* There are really two species of virtqueue, and it matters here.
+	 * If there are no output parts, it's a "normally full" receive queue,
+	 * otherwise it's a "normally empty" send queue. */
+	if (out) {
+		/* Leave threshold unless we're full. */
+		if (out + in < vq->num_free)
+			return;
+	} else {
+		/* Leave threshold unless we're empty. */
+		if (vq->num_free != vq->vring.num)
+			return;
+	}
+
+	/* Never drop threshold below 1 */
+	vq->indirect_threshold /= 2;
+	vq->indirect_threshold |= 1;
+
+	printk("%s %s: indirect threshold %u (%u+%u vs %u)\n", 
+	       dev_name(&vq->vq.vdev->dev),
+	       vq->vq.name, vq->indirect_threshold,
+	       out, in, vq->num_free);
+}
+
 static int vring_add_buf(struct virtqueue *_vq,
 			 struct scatterlist sg[],
 			 unsigned int out,

@@ -149,18 +179,31 @@ static int vring_add_buf(struct virtqueu
 	START_USE(vq);
 
 	BUG_ON(data == NULL);
-
-	/* If the host supports indirect descriptor tables, and we have multiple
-	 * buffers, then go indirect. FIXME: tune this threshold */
-	if (vq->indirect && (out + in) > 1 && vq->num_free) {
-		head = vring_add_indirect(vq, sg, out, in);
-		if (head != vq->vring.num)
-			goto add_head;
-	}
-
 	BUG_ON(out + in > vq->vring.num);
 	BUG_ON(out + in == 0);
 
+	/* If the host supports indirect descriptor tables, consider it. */
+	if (vq->indirect) {
+		bool try_indirect;
+
+		/* We tweak the threshold automatically. */
+		adjust_threshold(vq, out, in);
+
+		/* If we can't fit any at all, fall through. */
+		if (vq->num_free == 0)
+			try_indirect = false;
+		else if (out + in > vq->num_free)
+			try_indirect = true;
+		else
+			try_indirect = (out + in > vq->indirect_threshold);
+
+		if (try_indirect) {
+			head = vring_add_indirect(vq, sg, out, in);
+			if (head != vq->vring.num)
+				goto add_head;
+		}
+	}
+
 	if (vq->num_free < out + in) {
 		pr_debug("Can't add buf len %i - avail = %i\n",
 			 out + in, vq->num_free);

@@ -391,6 +434,7 @@ struct virtqueue *vring_new_virtqueue(un
 #endif
 
 	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
+	vq->indirect_threshold = num;
 
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback)
#21 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

drivers/virtio/virtio_ring.c static int vring_add_indirect(struct vri · line 149

Quoted 8 lines · Rusty Russell
drivers/virtio/virtio_ring.c +6 -0
+static void adjust_threshold(struct vring_virtqueue *vq,
+			     unsigned int out, unsigned int in)
+{
+	/* There are really two species of virtqueue, and it matters here.
+	 * If there are no output parts, it's a "normally full" receive queue,
+	 * otherwise it's a "normally empty" send queue. */

This comment is true for networking, but not for block. ++overkill with a ->adjust_threshold op.

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
#22 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Rusty Russell

drivers/virtio/virtio_ring.c static int vring_add_indirect(struct vri · line 149

Quoted 8 lines · Rusty Russell
drivers/virtio/virtio_ring.c +6 -0
+static void adjust_threshold(struct vring_virtqueue *vq,
+			     unsigned int out, unsigned int in)
+{
+	/* There are really two species of virtqueue, and it matters here.
+	 * If there are no output parts, it's a "normally full" receive queue,
+	 * otherwise it's a "normally empty" send queue. */

This comment is true for networking, but not for block. ++overkill with a ->adjust_threshold op.

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
#23 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

Quoted 10 lines · Avi Kivity

drivers/virtio/virtio_ring.c static int vring_add_indirect(struct vri · line 149

Quoted 6 lines · Rusty Russell
drivers/virtio/virtio_ring.c +6 -0
+static void adjust_threshold(struct vring_virtqueue *vq,
+			     unsigned int out, unsigned int in)
+{
+	/* There are really two species of virtqueue, and it matters here.
+	 * If there are no output parts, it's a "normally full" receive queue,
+	 * otherwise it's a "normally empty" send queue. */

This comment is true for networking, but not for block. ++overkill with a ->adjust_threshold op.

No, it's true for block. It has output parts, so we should reduce threshold when it's full. Network recvq is an example which should reduce threshold when it's empty.

->adjust_threshold is better as an arg to vring_new_virtqueue, but it's still not clear what the answer would be.

Rusty.

#24 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Avi Kivity
Quoted 10 lines · Avi Kivity

drivers/virtio/virtio_ring.c static int vring_add_indirect(struct vri · line 149

Quoted 6 lines · Rusty Russell
drivers/virtio/virtio_ring.c +6 -0
+static void adjust_threshold(struct vring_virtqueue *vq,
+			     unsigned int out, unsigned int in)
+{
+	/* There are really two species of virtqueue, and it matters here.
+	 * If there are no output parts, it's a "normally full" receive queue,
+	 * otherwise it's a "normally empty" send queue. */

This comment is true for networking, but not for block. ++overkill with a ->adjust_threshold op.

No, it's true for block. It has output parts, so we should reduce threshold when it's full. Network recvq is an example which should reduce threshold when it's empty.

->adjust_threshold is better as an arg to vring_new_virtqueue, but it's still not clear what the answer would be.

Rusty.

#25 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

Quoted 15 lines · Rusty Russell
Quoted 12 lines · Avi Kivity

drivers/virtio/virtio_ring.c static int vring_add_indirect(struct vri · line 149

Quoted 7 lines · Rusty Russell
drivers/virtio/virtio_ring.c +6 -0
+static void adjust_threshold(struct vring_virtqueue *vq,
+			     unsigned int out, unsigned int in)
+{
+	/* There are really two species of virtqueue, and it matters here.
+	 * If there are no output parts, it's a "normally full" receive queue,
+	 * otherwise it's a "normally empty" send queue. */

This comment is true for networking, but not for block. ++overkill with a ->adjust_threshold op.

Quoted 4 lines

No, it's true for block. It has output parts, so we should reduce threshold when it's full. Network recvq is an example which should reduce threshold when it's empty.

You mean the header that contains the sector number? It's a little incidental, but I guess it works.

Signature · 1 line
error compiling committee.c: too many arguments to function
#26 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Rusty Russell
Quoted 15 lines · Rusty Russell
Quoted 12 lines · Avi Kivity

drivers/virtio/virtio_ring.c static int vring_add_indirect(struct vri · line 149

Quoted 7 lines · Rusty Russell
drivers/virtio/virtio_ring.c +6 -0
+static void adjust_threshold(struct vring_virtqueue *vq,
+			     unsigned int out, unsigned int in)
+{
+	/* There are really two species of virtqueue, and it matters here.
+	 * If there are no output parts, it's a "normally full" receive queue,
+	 * otherwise it's a "normally empty" send queue. */

This comment is true for networking, but not for block. ++overkill with a ->adjust_threshold op.

Quoted 2 lines

No, it's true for block. It has output parts, so we should reduce threshold when it's full. Network recvq is an example which should reduce threshold when it's empty.

You mean the header that contains the sector number? It's a little incidental, but I guess it works.

Signature · 1 line
error compiling committee.c: too many arguments to function
#27 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

Quoted 7 lines · Avi Kivity
Quoted 3 lines · Rusty Russell

No, it's true for block. It has output parts, so we should reduce threshold when it's full. Network recvq is an example which should reduce threshold when it's empty.

You mean the header that contains the sector number? It's a little incidental, but I guess it works.

Yes, the one which breaks is randomness. You really do just throw buffers with no metadata in them and the request is implied. If/when we care, we can add a hint flag.

Cheers,
Rusty.

#28 lore raw

Re: [PATCH 2/3] virtio: indirect ring entries (VIRTIO_RING_F_INDIRECT_DESC)

In reply to Avi Kivity
Quoted 7 lines · Avi Kivity
Quoted 3 lines · Rusty Russell

No, it's true for block. It has output parts, so we should reduce threshold when it's full. Network recvq is an example which should reduce threshold when it's empty.

You mean the header that contains the sector number? It's a little incidental, but I guess it works.

Yes, the one which breaks is randomness. You really do just throw buffers with no metadata in them and the request is implied. If/when we care, we can add a hint flag.

Cheers,
Rusty.