Re: [PATCH 09/18] virtio: use avail_event index
On Wed, May 18, 2011 at 09:49:42AM +0930, Rusty Russell wrote:
On Tue, 17 May 2011 09:10:31 +0300, Michael S. Tsirkin m...@redhat.com
wrote:
Well one can imagine a driver doing:
while (virtqueue_get_buf()) {
virtqueue_add_buf()
}
virtqueue_kick()
which looks sensible (batch kicks) but might
process any number of bufs between kicks.
No, we currently only expose the buffers in the kick, so it can only
fill the ring doing that.
We could change that (and maybe that's worth looking at)...
That's actually what one of the early patches in the series did.
I guess I can try and reorder the patches, I do believe
it makes sense to publish immediately as this way
host can work in parallel with the guest.
If we look at drivers closely enough, I think none
of them do the equivalent of the above, but not 100% sure.
I'm pretty sure we don't have this kind of 'echo' driver yet. Drivers
tend to take OS requests and queue them. The only one which does
anything even partially sophisticated is the net driver...
Thanks,
Rusty.
I guess I'll just need to do the legwork and check then.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/18] virtio: use avail_event index
On Mon, May 16, 2011 at 04:42:21PM +0930, Rusty Russell wrote:
On Sun, 15 May 2011 16:55:41 +0300, Michael S. Tsirkin m...@redhat.com
wrote:
On Mon, May 09, 2011 at 02:03:26PM +0930, Rusty Russell wrote:
On Wed, 4 May 2011 23:51:47 +0300, Michael S. Tsirkin m...@redhat.com
wrote:
Use the new avail_event feature to reduce the number
of exits from the guest.
Figures here would be nice :)
You mean ASCII art in comments?
I mean benchmarks of some kind.
:)
@@ -228,6 +237,12 @@ add_head:
* new available array entries. */
virtio_wmb();
vq-vring.avail-idx++;
+ /* If the driver never bothers to kick in a very long while,
+* avail index might wrap around. If that happens, invalidate
+* kicked_avail index we stored. TODO: make sure all drivers
+* kick at least once in 2^16 and remove this. */
+ if (unlikely(vq-vring.avail-idx == vq-kicked_avail))
+ vq-kicked_avail_valid = true;
If they don't, they're already buggy. Simply do:
WARN_ON(vq-vring.avail-idx == vq-kicked_avail);
Hmm, but does it say that somewhere?
AFAICT it's a corollary of:
1) You have a finite ring of size = 2^16.
2) You need to kick the other side once you've done some work.
Well one can imagine a driver doing:
while (virtqueue_get_buf()) {
virtqueue_add_buf()
}
virtqueue_kick()
which looks sensible (batch kicks) but might
process any number of bufs between kicks.
If we look at drivers closely enough, I think none
of them do the equivalent of the above, but not 100% sure.
@@ -482,6 +517,8 @@ void vring_transport_features(struct virtio_device
*vdev)
break;
case VIRTIO_RING_F_USED_EVENT_IDX:
break;
+ case VIRTIO_RING_F_AVAIL_EVENT_IDX:
+ break;
default:
/* We don't understand this bit. */
clear_bit(i, vdev-features);
Does this belong in a prior patch?
Thanks,
Rusty.
Well if we don't support the feature in the ring we should not
ack the feature, right?
Ah, you're right.
Thanks,
Rusty.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/18] virtio: use avail_event index
On Monday, May 16, 2011 02:12:21 AM Rusty Russell wrote: On Sun, 15 May 2011 16:55:41 +0300, Michael S. Tsirkin m...@redhat.com wrote: On Mon, May 09, 2011 at 02:03:26PM +0930, Rusty Russell wrote: On Wed, 4 May 2011 23:51:47 +0300, Michael S. Tsirkin m...@redhat.com wrote: Use the new avail_event feature to reduce the number of exits from the guest. Figures here would be nice :) You mean ASCII art in comments? I mean benchmarks of some kind. I'm working on getting some benchmark results for the patches. I should hopefully have something in the next day or two. Tom @@ -228,6 +237,12 @@ add_head: * new available array entries. */ virtio_wmb(); vq-vring.avail-idx++; + /* If the driver never bothers to kick in a very long while, +* avail index might wrap around. If that happens, invalidate +* kicked_avail index we stored. TODO: make sure all drivers +* kick at least once in 2^16 and remove this. */ + if (unlikely(vq-vring.avail-idx == vq-kicked_avail)) + vq-kicked_avail_valid = true; If they don't, they're already buggy. Simply do: WARN_ON(vq-vring.avail-idx == vq-kicked_avail); Hmm, but does it say that somewhere? AFAICT it's a corollary of: 1) You have a finite ring of size = 2^16. 2) You need to kick the other side once you've done some work. @@ -482,6 +517,8 @@ void vring_transport_features(struct virtio_device *vdev) break; case VIRTIO_RING_F_USED_EVENT_IDX: break; + case VIRTIO_RING_F_AVAIL_EVENT_IDX: + break; default: /* We don't understand this bit. */ clear_bit(i, vdev-features); Does this belong in a prior patch? Thanks, Rusty. Well if we don't support the feature in the ring we should not ack the feature, right? Ah, you're right. Thanks, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/18] virtio: use avail_event index
On Tue, 17 May 2011 09:10:31 +0300, Michael S. Tsirkin m...@redhat.com
wrote:
Well one can imagine a driver doing:
while (virtqueue_get_buf()) {
virtqueue_add_buf()
}
virtqueue_kick()
which looks sensible (batch kicks) but might
process any number of bufs between kicks.
No, we currently only expose the buffers in the kick, so it can only
fill the ring doing that.
We could change that (and maybe that's worth looking at)...
If we look at drivers closely enough, I think none
of them do the equivalent of the above, but not 100% sure.
I'm pretty sure we don't have this kind of 'echo' driver yet. Drivers
tend to take OS requests and queue them. The only one which does
anything even partially sophisticated is the net driver...
Thanks,
Rusty.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/18] virtio: use avail_event index
On Wed, May 18, 2011 at 09:49:42AM +0930, Rusty Russell wrote:
On Tue, 17 May 2011 09:10:31 +0300, Michael S. Tsirkin m...@redhat.com
wrote:
Well one can imagine a driver doing:
while (virtqueue_get_buf()) {
virtqueue_add_buf()
}
virtqueue_kick()
which looks sensible (batch kicks) but might
process any number of bufs between kicks.
No, we currently only expose the buffers in the kick, so it can only
fill the ring doing that.
We could change that (and maybe that's worth looking at)...
Yes, I think we should - this way host and guest can process
data in parallel without a kick.
My patchset included that simply because it's one index
less to be confused about.
If we look at drivers closely enough, I think none
of them do the equivalent of the above, but not 100% sure.
I'm pretty sure we don't have this kind of 'echo' driver yet. Drivers
tend to take OS requests and queue them. The only one which does
anything even partially sophisticated is the net driver...
Thanks,
Rusty.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/18] virtio: use avail_event index
On Sun, 15 May 2011 16:55:41 +0300, Michael S. Tsirkin m...@redhat.com wrote: On Mon, May 09, 2011 at 02:03:26PM +0930, Rusty Russell wrote: On Wed, 4 May 2011 23:51:47 +0300, Michael S. Tsirkin m...@redhat.com wrote: Use the new avail_event feature to reduce the number of exits from the guest. Figures here would be nice :) You mean ASCII art in comments? I mean benchmarks of some kind. @@ -228,6 +237,12 @@ add_head: * new available array entries. */ virtio_wmb(); vq-vring.avail-idx++; + /* If the driver never bothers to kick in a very long while, + * avail index might wrap around. If that happens, invalidate + * kicked_avail index we stored. TODO: make sure all drivers + * kick at least once in 2^16 and remove this. */ + if (unlikely(vq-vring.avail-idx == vq-kicked_avail)) + vq-kicked_avail_valid = true; If they don't, they're already buggy. Simply do: WARN_ON(vq-vring.avail-idx == vq-kicked_avail); Hmm, but does it say that somewhere? AFAICT it's a corollary of: 1) You have a finite ring of size = 2^16. 2) You need to kick the other side once you've done some work. @@ -482,6 +517,8 @@ void vring_transport_features(struct virtio_device *vdev) break; case VIRTIO_RING_F_USED_EVENT_IDX: break; + case VIRTIO_RING_F_AVAIL_EVENT_IDX: + break; default: /* We don't understand this bit. */ clear_bit(i, vdev-features); Does this belong in a prior patch? Thanks, Rusty. Well if we don't support the feature in the ring we should not ack the feature, right? Ah, you're right. Thanks, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/18] virtio: use avail_event index
On Mon, May 09, 2011 at 02:03:26PM +0930, Rusty Russell wrote:
On Wed, 4 May 2011 23:51:47 +0300, Michael S. Tsirkin m...@redhat.com
wrote:
Use the new avail_event feature to reduce the number
of exits from the guest.
Figures here would be nice :)
You mean ASCII art in comments?
@@ -228,6 +237,12 @@ add_head:
* new available array entries. */
virtio_wmb();
vq-vring.avail-idx++;
+ /* If the driver never bothers to kick in a very long while,
+* avail index might wrap around. If that happens, invalidate
+* kicked_avail index we stored. TODO: make sure all drivers
+* kick at least once in 2^16 and remove this. */
+ if (unlikely(vq-vring.avail-idx == vq-kicked_avail))
+ vq-kicked_avail_valid = true;
If they don't, they're already buggy. Simply do:
WARN_ON(vq-vring.avail-idx == vq-kicked_avail);
Hmm, but does it say that somewhere?
It seems that most drivers use locking to prevent
posting more buffers while they drain the used ring,
and also kick at least once in vq-num buffers,
which I guess in the end would work out fine
as vq num is never as large as 2^15.
+static bool vring_notify(struct vring_virtqueue *vq)
+{
+ u16 old, new;
+ bool v;
+ if (!vq-event)
+ return !(vq-vring.used-flags VRING_USED_F_NO_NOTIFY);
+
+ v = vq-kicked_avail_valid;
+ old = vq-kicked_avail;
+ new = vq-kicked_avail = vq-vring.avail-idx;
+ vq-kicked_avail_valid = true;
+ if (unlikely(!v))
+ return true;
This is the only place you actually used kicked_avail_valid. Is it
possible to initialize it in such a way that you can remove this?
If we kill the code above as you suggested, likely yes.
Maybe an explicit flag is more obvious?
@@ -482,6 +517,8 @@ void vring_transport_features(struct virtio_device
*vdev)
break;
case VIRTIO_RING_F_USED_EVENT_IDX:
break;
+ case VIRTIO_RING_F_AVAIL_EVENT_IDX:
+ break;
default:
/* We don't understand this bit. */
clear_bit(i, vdev-features);
Does this belong in a prior patch?
Thanks,
Rusty.
Well if we don't support the feature in the ring we should not
ack the feature, right?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/18] virtio: use avail_event index
On Wed, 4 May 2011 23:51:47 +0300, Michael S. Tsirkin m...@redhat.com wrote:
Use the new avail_event feature to reduce the number
of exits from the guest.
Figures here would be nice :)
@@ -228,6 +237,12 @@ add_head:
* new available array entries. */
virtio_wmb();
vq-vring.avail-idx++;
+ /* If the driver never bothers to kick in a very long while,
+ * avail index might wrap around. If that happens, invalidate
+ * kicked_avail index we stored. TODO: make sure all drivers
+ * kick at least once in 2^16 and remove this. */
+ if (unlikely(vq-vring.avail-idx == vq-kicked_avail))
+ vq-kicked_avail_valid = true;
If they don't, they're already buggy. Simply do:
WARN_ON(vq-vring.avail-idx == vq-kicked_avail);
+static bool vring_notify(struct vring_virtqueue *vq)
+{
+ u16 old, new;
+ bool v;
+ if (!vq-event)
+ return !(vq-vring.used-flags VRING_USED_F_NO_NOTIFY);
+
+ v = vq-kicked_avail_valid;
+ old = vq-kicked_avail;
+ new = vq-kicked_avail = vq-vring.avail-idx;
+ vq-kicked_avail_valid = true;
+ if (unlikely(!v))
+ return true;
This is the only place you actually used kicked_avail_valid. Is it
possible to initialize it in such a way that you can remove this?
@@ -482,6 +517,8 @@ void vring_transport_features(struct virtio_device *vdev)
break;
case VIRTIO_RING_F_USED_EVENT_IDX:
break;
+ case VIRTIO_RING_F_AVAIL_EVENT_IDX:
+ break;
default:
/* We don't understand this bit. */
clear_bit(i, vdev-features);
Does this belong in a prior patch?
Thanks,
Rusty.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/18] virtio: use avail_event index
On Wed, May 04, 2011 at 04:58:18PM -0500, Tom Lendacky wrote:
On Wednesday, May 04, 2011 03:51:47 PM Michael S. Tsirkin wrote:
Use the new avail_event feature to reduce the number
of exits from the guest.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
drivers/virtio/virtio_ring.c | 39
++- 1 files changed, 38 insertions(+),
1 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 3a3ed75..262dfe6 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -82,6 +82,15 @@ struct vring_virtqueue
/* Host supports indirect buffers */
bool indirect;
+ /* Host publishes avail event idx */
+ bool event;
+
+ /* Is kicked_avail below valid? */
+ bool kicked_avail_valid;
+
+ /* avail idx value we already kicked. */
+ u16 kicked_avail;
+
/* Number of free buffers */
unsigned int num_free;
/* Head of free buffer list. */
@@ -228,6 +237,12 @@ add_head:
* new available array entries. */
virtio_wmb();
vq-vring.avail-idx++;
+ /* If the driver never bothers to kick in a very long while,
+* avail index might wrap around. If that happens, invalidate
+* kicked_avail index we stored. TODO: make sure all drivers
+* kick at least once in 2^16 and remove this. */
+ if (unlikely(vq-vring.avail-idx == vq-kicked_avail))
+ vq-kicked_avail_valid = true;
vq-kicked_avail_valid should be set to false here.
Tom
Right, good catch.
pr_debug(Added buffer head %i to %p\n, head, vq);
END_USE(vq);
@@ -236,6 +251,23 @@ add_head:
}
EXPORT_SYMBOL_GPL(virtqueue_add_buf_gfp);
+
+static bool vring_notify(struct vring_virtqueue *vq)
+{
+ u16 old, new;
+ bool v;
+ if (!vq-event)
+ return !(vq-vring.used-flags VRING_USED_F_NO_NOTIFY);
+
+ v = vq-kicked_avail_valid;
+ old = vq-kicked_avail;
+ new = vq-kicked_avail = vq-vring.avail-idx;
+ vq-kicked_avail_valid = true;
+ if (unlikely(!v))
+ return true;
+ return vring_need_event(vring_avail_event(vq-vring), new, old);
+}
+
void virtqueue_kick(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
@@ -244,7 +276,7 @@ void virtqueue_kick(struct virtqueue *_vq)
/* Need to update avail index before checking if we should notify */
virtio_mb();
- if (!(vq-vring.used-flags VRING_USED_F_NO_NOTIFY))
+ if (vring_notify(vq))
/* Prod other side to tell it about changes. */
vq-notify(vq-vq);
@@ -437,6 +469,8 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
vq-vq.name = name;
vq-notify = notify;
vq-broken = false;
+ vq-kicked_avail_valid = false;
+ vq-kicked_avail = 0;
vq-last_used_idx = 0;
list_add_tail(vq-vq.list, vdev-vqs);
#ifdef DEBUG
@@ -444,6 +478,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
#endif
vq-indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
+ vq-event = virtio_has_feature(vdev, VIRTIO_RING_F_AVAIL_EVENT_IDX);
/* No callback? Tell other side not to bother us. */
if (!callback)
@@ -482,6 +517,8 @@ void vring_transport_features(struct virtio_device
*vdev) break;
case VIRTIO_RING_F_USED_EVENT_IDX:
break;
+ case VIRTIO_RING_F_AVAIL_EVENT_IDX:
+ break;
default:
/* We don't understand this bit. */
clear_bit(i, vdev-features);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 09/18] virtio: use avail_event index
On Wednesday, May 04, 2011 03:51:47 PM Michael S. Tsirkin wrote:
Use the new avail_event feature to reduce the number
of exits from the guest.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
drivers/virtio/virtio_ring.c | 39
++- 1 files changed, 38 insertions(+),
1 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 3a3ed75..262dfe6 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -82,6 +82,15 @@ struct vring_virtqueue
/* Host supports indirect buffers */
bool indirect;
+ /* Host publishes avail event idx */
+ bool event;
+
+ /* Is kicked_avail below valid? */
+ bool kicked_avail_valid;
+
+ /* avail idx value we already kicked. */
+ u16 kicked_avail;
+
/* Number of free buffers */
unsigned int num_free;
/* Head of free buffer list. */
@@ -228,6 +237,12 @@ add_head:
* new available array entries. */
virtio_wmb();
vq-vring.avail-idx++;
+ /* If the driver never bothers to kick in a very long while,
+ * avail index might wrap around. If that happens, invalidate
+ * kicked_avail index we stored. TODO: make sure all drivers
+ * kick at least once in 2^16 and remove this. */
+ if (unlikely(vq-vring.avail-idx == vq-kicked_avail))
+ vq-kicked_avail_valid = true;
vq-kicked_avail_valid should be set to false here.
Tom
pr_debug(Added buffer head %i to %p\n, head, vq);
END_USE(vq);
@@ -236,6 +251,23 @@ add_head:
}
EXPORT_SYMBOL_GPL(virtqueue_add_buf_gfp);
+
+static bool vring_notify(struct vring_virtqueue *vq)
+{
+ u16 old, new;
+ bool v;
+ if (!vq-event)
+ return !(vq-vring.used-flags VRING_USED_F_NO_NOTIFY);
+
+ v = vq-kicked_avail_valid;
+ old = vq-kicked_avail;
+ new = vq-kicked_avail = vq-vring.avail-idx;
+ vq-kicked_avail_valid = true;
+ if (unlikely(!v))
+ return true;
+ return vring_need_event(vring_avail_event(vq-vring), new, old);
+}
+
void virtqueue_kick(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
@@ -244,7 +276,7 @@ void virtqueue_kick(struct virtqueue *_vq)
/* Need to update avail index before checking if we should notify */
virtio_mb();
- if (!(vq-vring.used-flags VRING_USED_F_NO_NOTIFY))
+ if (vring_notify(vq))
/* Prod other side to tell it about changes. */
vq-notify(vq-vq);
@@ -437,6 +469,8 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
vq-vq.name = name;
vq-notify = notify;
vq-broken = false;
+ vq-kicked_avail_valid = false;
+ vq-kicked_avail = 0;
vq-last_used_idx = 0;
list_add_tail(vq-vq.list, vdev-vqs);
#ifdef DEBUG
@@ -444,6 +478,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
#endif
vq-indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
+ vq-event = virtio_has_feature(vdev, VIRTIO_RING_F_AVAIL_EVENT_IDX);
/* No callback? Tell other side not to bother us. */
if (!callback)
@@ -482,6 +517,8 @@ void vring_transport_features(struct virtio_device
*vdev) break;
case VIRTIO_RING_F_USED_EVENT_IDX:
break;
+ case VIRTIO_RING_F_AVAIL_EVENT_IDX:
+ break;
default:
/* We don't understand this bit. */
clear_bit(i, vdev-features);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
