vhost-net: utilize PUBLISH_USED_IDX feature

PATCH Superseded Michael S. Tsirkin 4 participants 6 messages May 18, 2010 – May 20, 2010

Subsystems (5)

Networking Network drivers KVM virtio core +1 more subsystems

lore

Share & tools

lore

Michael S. Tsirkin

author maintainer

2010-05-18 01:23 UTC #0 lore raw

PATCH 1/1 vhost-net: utilize PUBLISH_USED_IDX feature

With PUBLISH_USED_IDX, guest tells us which used entries it has consumed. This can be used to reduce the number
of interrupts: after we write a used entry, if the guest has not yet consumed the previous entry, or if the guest has already consumed the new entry, we do not need to interrupt.
This imporves bandwidth by 30% under some workflows.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Rusty, Dave, this patch depends on the patch "virtio: put last seen used index into ring itself" which is currently destined at Rusty's tree. Rusty, if you are taking that one for 2.6.35, please take this one as well.
Dave, any objections?

 drivers/vhost/vhost.c |   27 +++++++++++++++++++++------
 drivers/vhost/vhost.h |    2 ++
 2 files changed, 23 insertions(+), 6 deletions(-)

drivers/vhost/vhost.c +21 -6

index 750effe..2a66cf3 100644
@@ -278,14 +278,15 @@ static int memory_access_ok(struct vhost_dev *d, struct vhost_memory *mem,
 	return 1;
 }
 
-static int vq_access_ok(unsigned int num,
+static int vq_access_ok(struct vhost_dev *d, unsigned int num,
 			struct vring_desc __user *desc,
 			struct vring_avail __user *avail,
 			struct vring_used __user *used)
 {
+	size_t s = vhost_has_feature(d, VIRTIO_RING_F_PUBLISH_USED) ? 2 : 0;
 	return access_ok(VERIFY_READ, desc, num * sizeof *desc) &&
 	       access_ok(VERIFY_READ, avail,
-			 sizeof *avail + num * sizeof *avail->ring) &&
+			 sizeof *avail + num * sizeof *avail->ring + s) &&
 	       access_ok(VERIFY_WRITE, used,
 			sizeof *used + num * sizeof *used->ring);
 }

@@ -312,7 +313,7 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq, void __user *log_base)
 /* Caller should have vq mutex and device mutex */
 int vhost_vq_access_ok(struct vhost_virtqueue *vq)
 {
-	return vq_access_ok(vq->num, vq->desc, vq->avail, vq->used) &&
+	return vq_access_ok(vq->dev, vq->num, vq->desc, vq->avail, vq->used,) &&
 		vq_log_access_ok(vq, vq->log_base);
 }

@@ -448,7 +449,7 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
 		 * If it is not, we don't as size might not have been setup.
 		 * We will verify when backend is configured. */
 		if (vq->private_data) {
-			if (!vq_access_ok(vq->num,
+			if (!vq_access_ok(d, vq->num,
 				(void __user *)(unsigned long)a.desc_user_addr,
 				(void __user *)(unsigned long)a.avail_user_addr,
 				(void __user *)(unsigned long)a.used_user_addr)) {

@@ -473,6 +474,7 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
 		vq->log_used = !!(a.flags & (0x1 << VHOST_VRING_F_LOG));
 		vq->desc = (void __user *)(unsigned long)a.desc_user_addr;
 		vq->avail = (void __user *)(unsigned long)a.avail_user_addr;
+		vq->last_used = (u16 __user *)&vq->avail->ring[vq->num];
 		vq->log_addr = a.log_guest_addr;
 		vq->used = (void __user *)(unsigned long)a.used_user_addr;
 		break;

@@ -993,7 +995,8 @@ void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
 
 /* After we've used one of their buffers, we tell them about it.  We'll then
  * want to notify the guest, using eventfd. */
-int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
+static int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head,
+			  int len)
 {
 	struct vring_used_elem __user *used;

@@ -1034,9 +1037,10 @@ int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
 }
 
 /* This actually signals the guest, using eventfd. */
-void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+static void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
 {
 	__u16 flags;
+	__u16 used;
 	/* Flush out used index updates. This is paired
 	 * with the barrier that the Guest executes when enabling
 	 * interrupts. */

@@ -1053,6 +1057,17 @@ void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
 	     !vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
 		return;
 
+	if (vhost_has_feature(d, VIRTIO_RING_F_PUBLISH_USED)) {
+		__u16 used;
+		if (get_user(used, vq->last_used)) {
+			vq_err(vq, "Failed to get last used idx");
+			return;
+		}
+
+		if (used != (u16)(vq->last_used_idx - 1))
+			return;
+	}
+
 	/* Signal the Guest tell them we used something up. */
 	if (vq->call_ctx)
 		eventfd_signal(vq->call_ctx, 1);

drivers/vhost/vhost.h +2 -0

index 44591ba..00e9784 100644
@@ -52,6 +52,7 @@ struct vhost_virtqueue {
 	unsigned int num;
 	struct vring_desc __user *desc;
 	struct vring_avail __user *avail;
+	u16 __user *last_used;
 	struct vring_used __user *used;
 	struct file *kick;
 	struct file *call;

@@ -148,6 +149,7 @@ void vhost_cleanup(void);
 enum {
 	VHOST_FEATURES = (1 << VIRTIO_F_NOTIFY_ON_EMPTY) |
 			 (1 << VIRTIO_RING_F_INDIRECT_DESC) |
+			 (1 << VIRTIO_RING_F_PUBLISH_USED) |
 			 (1 << VHOST_F_LOG_ALL) |
 			 (1 << VHOST_NET_F_VIRTIO_NET_HDR),
 };

Signature · 1 line

1.7.1.12.g42b7f

David Miller

maintainer Acked-by on v1 1/1

2010-05-18 04:08 UTC #1 lore raw

From: "Michael S. Tsirkin" <[email protected]>
Date: Tue, 18 May 2010 04:19:31 +0300

Quoted 8 lines

With PUBLISH_USED_IDX, guest tells us which used entries it has consumed. This can be used to reduce the number
of interrupts: after we write a used entry, if the guest has not yet consumed the previous entry, or if the guest has already consumed the new entry, we do not need to interrupt.
This imporves bandwidth by 30% under some workflows.
Signed-off-by Michael S. Tsirkin <[email protected]>

Quoted 8 lines

---
Rusty, Dave, this patch depends on the patch "virtio: put last seen used index into ring itself" which is currently destined at Rusty's tree. Rusty, if you are taking that one for 2.6.35, please take this one as well.
Dave, any objections?

None:

Acked-by David S. Miller <[email protected]>

Avi Kivity

2010-05-19 17:04 UTC #2 lore raw

In reply to Michael S. Tsirkin

Quoted 8 lines · Michael S. Tsirkin

With PUBLISH_USED_IDX, guest tells us which used entries it has consumed. This can be used to reduce the number
of interrupts: after we write a used entry, if the guest has not yet consumed the previous entry, or if the guest has already consumed the new entry, we do not need to interrupt.
This imporves bandwidth by 30% under some workflows.
Signed-off-by Michael S. Tsirkin<[email protected]>

Quoted 9 lines

---
Rusty, Dave, this patch depends on the patch "virtio: put last seen used index into ring itself" which is currently destined at Rusty's tree. Rusty, if you are taking that one for 2.6.35, please take this one as well.
Dave, any objections?

I object: I think the index should have its own cacheline, and that it should be documented before merging.

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Michael S. Tsirkin

author maintainer

2010-05-19 22:27 UTC #3 lore raw

Quoted 19 lines · Avi Kivity

Quoted 8 lines · Michael S. Tsirkin
With PUBLISH_USED_IDX, guest tells us which used entries it has consumed. This can be used to reduce the number
of interrupts: after we write a used entry, if the guest has not yet consumed the previous entry, or if the guest has already consumed the new entry, we do not need to interrupt.
This imporves bandwidth by 30% under some workflows.
Signed-off-by Michael S. Tsirkin<[email protected]>
Quoted 9 lines
---
Rusty, Dave, this patch depends on the patch "virtio: put last seen used index into ring itself" which is currently destined at Rusty's tree. Rusty, if you are taking that one for 2.6.35, please take this one as well.
Dave, any objections?

Quoted 1 line

I object: I think the index should have its own cacheline,

The issue here is that host/guest do not know each other's cache line size. I guess we could just put it at offset 128 or something like that ... Rusty?

Quoted 1 line

and that it should be documented before merging.

I think you meant to object to the virtio patch, not this one. This patch does not introduce new layout, just implements host support. virtio spec patch will follow: it is not part of linux tree so there is no patch dependency.

Quoted 2 lines

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Rusty Russell

2010-05-20 04:18 UTC #4 lore raw

Quoted 20 lines · Michael S. Tsirkin

Quoted 19 lines · Avi Kivity
Quoted 17 lines · Michael S. Tsirkin
With PUBLISH_USED_IDX, guest tells us which used entries it has consumed. This can be used to reduce the number
of interrupts: after we write a used entry, if the guest has not yet consumed the previous entry, or if the guest has already consumed the new entry, we do not need to interrupt.
This imporves bandwidth by 30% under some workflows.
Signed-off-by Michael S. Tsirkin<[email protected]>
---
Rusty, Dave, this patch depends on the patch "virtio: put last seen used index into ring itself" which is currently destined at Rusty's tree. Rusty, if you are taking that one for 2.6.35, please take this one as well.
Dave, any objections?

Quoted 5 lines

Quoted 1 line
I object: I think the index should have its own cacheline,
The issue here is that host/guest do not know each other's cache line size. I guess we could just put it at offset 128 or something like that ... Rusty?

I was assuming you'd put it at the end of the padding.

I think it's a silly optimization, but Avi obviously feels strongly about it and I respect his opinion.

Please resubmit...
Thanks,
Rusty.

Avi Kivity

2010-05-20 06:06 UTC #5 lore raw

In reply to Michael S. Tsirkin

Quoted 22 lines · Michael S. Tsirkin

Quoted 20 lines · Avi Kivity
Quoted 18 lines · Michael S. Tsirkin
With PUBLISH_USED_IDX, guest tells us which used entries it has consumed. This can be used to reduce the number
of interrupts: after we write a used entry, if the guest has not yet consumed the previous entry, or if the guest has already consumed the new entry, we do not need to interrupt.
This imporves bandwidth by 30% under some workflows.
Signed-off-by Michael S. Tsirkin<[email protected]>
---
Rusty, Dave, this patch depends on the patch "virtio: put last seen used index into ring itself" which is currently destined at Rusty's tree. Rusty, if you are taking that one for 2.6.35, please take this one as well.
Dave, any objections?

Quoted 6 lines

Quoted 2 lines
I object: I think the index should have its own cacheline,
The issue here is that host/guest do not know each other's cache line size. I guess we could just put it at offset 128 or something like that ... Rusty?

Not so pretty, but ok.

Quoted 8 lines

Quoted 3 lines
and that it should be documented before merging.
I think you meant to object to the virtio patch, not this one. This patch does not introduce new layout, just implements host support. virtio spec patch will follow: it is not part of linux tree so there is no patch dependency.

There is a reviewer dependency.

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.