virtio and vhost-net performance enhancements

Define a new feature bit for the guest to utilize a used_event index (like Xen) instead if a flag bit to enable/disable interrupts.

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

---
 include/linux/virtio_ring.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

include/linux/virtio_ring.h +2 -0

index e4d144b..f5c1b75 100644
@@ -29,6 +29,10 @@
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC	28
 
+/* The Guest publishes the used index for which it expects an interrupt
+ * at the end of the avail ring. Host should ignore the avail->flags

field. */ +#define VIRTIO_RING_F_USED_EVENT_IDX 29
+
/* Virtio ring descriptors: 16 bytes. These can chain together via
"next". */ struct vring_desc {

include/linux/virtio_ring.h +1 -0

 	/* Address (guest-physical). */

@@ -83,6 +87,7 @@ struct vring {
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
+ *	__u16 used_event_idx;
  *
  *	// Padding to the next align boundary.
  *	char pad[];

Quoted 32 lines · Michael S. Tsirkin

Define a new feature bit for the guest to utilize a used_event index (like Xen) instead if a flag bit to enable/disable interrupts.

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

---
 include/linux/virtio_ring.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

include/linux/virtio_ring.h +2 -0

index e4d144b..f5c1b75 100644
@@ -29,6 +29,10 @@
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC	28
 
+/* The Guest publishes the used index for which it expects an interrupt
+ * at the end of the avail ring. Host should ignore the avail->flags

field. */ +#define VIRTIO_RING_F_USED_EVENT_IDX 29
+
/* Virtio ring descriptors: 16 bytes. These can chain together via
"next". */ struct vring_desc {

include/linux/virtio_ring.h +1 -0

 	/* Address (guest-physical). */

@@ -83,6 +87,7 @@ struct vring {
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
+ *	__u16 used_event_idx;
  *
  *	// Padding to the next align boundary.
  *	char pad[];

include/linux/virtio_ring.h struct vring { · line 107

Quoted 10 lines

include/linux/virtio_ring.h +4 -0

@@ -93,6 +98,10 @@ struct vring {
  *	struct vring_used_elem used[num];
  * };
  */
+/* We publish the used event index at the end of the available ring.
+ * It is at the end for backwards compatibility. */
+#define vring_used_event(vr) ((vr)->avail->ring[(vr)->num])
+
 static inline void vring_init(struct vring *vr, unsigned int num, void *p,
 			      unsigned long align)
 {

You should update the vring_size procedure to account for the extra field at the end of the available ring by change the "(2 + num)" to "(3 + num)":

    return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (3 + num)

Tom

Quoted 32 lines · Michael S. Tsirkin

Define a new feature bit for the guest to utilize a used_event index (like Xen) instead if a flag bit to enable/disable interrupts.

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

---
 include/linux/virtio_ring.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

include/linux/virtio_ring.h +2 -0

index e4d144b..f5c1b75 100644
@@ -29,6 +29,10 @@
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC	28
 
+/* The Guest publishes the used index for which it expects an interrupt
+ * at the end of the avail ring. Host should ignore the avail->flags

field. */ +#define VIRTIO_RING_F_USED_EVENT_IDX 29
+
/* Virtio ring descriptors: 16 bytes. These can chain together via
"next". */ struct vring_desc {

include/linux/virtio_ring.h +1 -0

 	/* Address (guest-physical). */

@@ -83,6 +87,7 @@ struct vring {
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
+ *	__u16 used_event_idx;
  *
  *	// Padding to the next align boundary.
  *	char pad[];

include/linux/virtio_ring.h struct vring { · line 107

Quoted 10 lines

include/linux/virtio_ring.h +4 -0

@@ -93,6 +98,10 @@ struct vring {
  *	struct vring_used_elem used[num];
  * };
  */
+/* We publish the used event index at the end of the available ring.
+ * It is at the end for backwards compatibility. */
+#define vring_used_event(vr) ((vr)->avail->ring[(vr)->num])
+
 static inline void vring_init(struct vring *vr, unsigned int num, void *p,
 			      unsigned long align)
 {

You should update the vring_size procedure to account for the extra field at the end of the available ring by change the "(2 + num)" to "(3 + num)":

    return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (3 + num)

Tom

Define a new feature bit for the guest to utilize a used_event index (like Xen) instead if a flag bit to enable/disable interrupts.

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

---
 include/linux/virtio_ring.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

include/linux/virtio_ring.h +2 -0

index e4d144b..f5c1b75 100644
@@ -29,6 +29,10 @@
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC	28
 
+/* The Guest publishes the used index for which it expects an interrupt
+ * at the end of the avail ring. Host should ignore the avail->flags

field. */ +#define VIRTIO_RING_F_USED_EVENT_IDX 29
+
/* Virtio ring descriptors: 16 bytes. These can chain together via
"next". */ struct vring_desc {

include/linux/virtio_ring.h +1 -0

 	/* Address (guest-physical). */

@@ -83,6 +87,7 @@ struct vring {
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
+ *	__u16 used_event_idx;
  *
  *	// Padding to the next align boundary.
  *	char pad[];

Define a new feature bit for the host to
declare that it uses an avail_event index
(like Xen) instead of a feature bit
to enable/disable interrupts.

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

---
 include/linux/virtio_ring.h |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

include/linux/virtio_ring.h +0 -0

index f5c1b75..f791772 100644

Quoted 15 lines · Michael S. Tsirkin

Define a new feature bit for the host to
declare that it uses an avail_event index
(like Xen) instead of a feature bit
to enable/disable interrupts.

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

---
 include/linux/virtio_ring.h |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

include/linux/virtio_ring.h +0 -0

index f5c1b75..f791772 100644

include/linux/virtio_ring.h line 37

Quoted 6 lines

include/linux/virtio_ring.h +3 -0

@@ -32,6 +32,9 @@
 /* The Guest publishes the used index for which it expects an interrupt
  * at the end of the avail ring. Host should ignore the avail->flags field. */
 #define VIRTIO_RING_F_USED_EVENT_IDX	29
+/* The Host publishes the avail index for which it expects a kick
+ * at the end of the used ring. Guest should ignore the used->flags field. */
+#define VIRTIO_RING_F_AVAIL_EVENT_IDX	32

Are you really sure we want to separate the two? Seems a little simpler to have one bit to mean "we're publishing our threshold". For someone implementing this from scratch, it's a little simpler.

Or are there cases where the old style makes more sense?

Thanks,
Rusty.

Quoted 10 lines · Rusty Russell

include/linux/virtio_ring.h line 37

Quoted 4 lines · Michael S. Tsirkin

#define VIRTIO_RING_F_USED_EVENT_IDX 29

include/linux/virtio_ring.h +3 -0

+/* The Host publishes the avail index for which it expects a kick
+ * at the end of the used ring. Guest should ignore the used->flags field. */
+#define VIRTIO_RING_F_AVAIL_EVENT_IDX	32

Are you really sure we want to separate the two? Seems a little simpler to have one bit to mean "we're publishing our threshold". For someone implementing this from scratch, it's a little simpler.

Quoted 4 lines

Or are there cases where the old style makes more sense?

Thanks,
Rusty.

Hmm, it makes debugging easier as each side can disable publishing separately - I used it all the time when I saw e.g. networking stuck to guess whether I need to investigate the interrupt or the exit handling.

But I'm not hung up on this.

Let me know pls.

Quoted 23 lines · Michael S. Tsirkin

Quoted 10 lines · Rusty Russell

include/linux/virtio_ring.h line 37

Quoted 4 lines · Michael S. Tsirkin

#define VIRTIO_RING_F_USED_EVENT_IDX 29

include/linux/virtio_ring.h +3 -0

+/* The Host publishes the avail index for which it expects a kick
+ * at the end of the used ring. Guest should ignore the used->flags field. */
+#define VIRTIO_RING_F_AVAIL_EVENT_IDX	32

Are you really sure we want to separate the two? Seems a little simpler to have one bit to mean "we're publishing our threshold". For someone implementing this from scratch, it's a little simpler.

Quoted 4 lines

Or are there cases where the old style makes more sense?

Thanks,
Rusty.

Hmm, it makes debugging easier as each side can disable publishing separately - I used it all the time when I saw e.g. networking stuck to guess whether I need to investigate the interrupt or the exit handling.

But I'm not hung up on this.

Quoted 1 line

Let me know pls.

If we combine them into one, then these patches no longer depend on the feature bit expansion, which is worthwhile (though I'll take both).

Thanks,
Rusty.

Quoted 29 lines · Rusty Russell

Quoted 16 lines · Michael S. Tsirkin

Quoted 10 lines · Rusty Russell

include/linux/virtio_ring.h line 37

Quoted 4 lines · Michael S. Tsirkin

#define VIRTIO_RING_F_USED_EVENT_IDX 29

include/linux/virtio_ring.h +3 -0

+/* The Host publishes the avail index for which it expects a kick
+ * at the end of the used ring. Guest should ignore the used->flags field. */
+#define VIRTIO_RING_F_AVAIL_EVENT_IDX	32

Are you really sure we want to separate the two? Seems a little simpler to have one bit to mean "we're publishing our threshold". For someone implementing this from scratch, it's a little simpler.

Quoted 4 lines

Or are there cases where the old style makes more sense?

Thanks,
Rusty.

Quoted 8 lines

Hmm, it makes debugging easier as each side can disable publishing separately - I used it all the time when I saw e.g. networking stuck to guess whether I need to investigate the interrupt or the exit handling.

But I'm not hung up on this.

Let me know pls.

If we combine them into one, then these patches no longer depend on the feature bit expansion, which is worthwhile (though I'll take both).

Quoted 2 lines

Thanks,
Rusty.

Yes, I know. But if we do expand feature bits anyway, for debugging and profiling if nothing else it's useful to have them separate ... If you take both why does the order matter?

Quoted 29 lines · Rusty Russell

Quoted 16 lines · Michael S. Tsirkin

Quoted 10 lines · Rusty Russell

include/linux/virtio_ring.h line 37

Quoted 4 lines · Michael S. Tsirkin

#define VIRTIO_RING_F_USED_EVENT_IDX 29

include/linux/virtio_ring.h +3 -0

+/* The Host publishes the avail index for which it expects a kick
+ * at the end of the used ring. Guest should ignore the used->flags field. */
+#define VIRTIO_RING_F_AVAIL_EVENT_IDX	32

Are you really sure we want to separate the two? Seems a little simpler to have one bit to mean "we're publishing our threshold". For someone implementing this from scratch, it's a little simpler.

Quoted 4 lines

Or are there cases where the old style makes more sense?

Thanks,
Rusty.

Quoted 8 lines

Hmm, it makes debugging easier as each side can disable publishing separately - I used it all the time when I saw e.g. networking stuck to guess whether I need to investigate the interrupt or the exit handling.

But I'm not hung up on this.

Let me know pls.

If we combine them into one, then these patches no longer depend on the feature bit expansion, which is worthwhile (though I'll take both).

Quoted 2 lines

Thanks,
Rusty.

Yes, I know. But if we do expand feature bits anyway, for debugging and profiling if nothing else it's useful to have them separate ... If you take both why does the order matter?

Quoted 23 lines · Michael S. Tsirkin

Quoted 10 lines · Rusty Russell

include/linux/virtio_ring.h line 37

Quoted 4 lines · Michael S. Tsirkin

#define VIRTIO_RING_F_USED_EVENT_IDX 29

include/linux/virtio_ring.h +3 -0

+/* The Host publishes the avail index for which it expects a kick
+ * at the end of the used ring. Guest should ignore the used->flags field. */
+#define VIRTIO_RING_F_AVAIL_EVENT_IDX	32

Are you really sure we want to separate the two? Seems a little simpler to have one bit to mean "we're publishing our threshold". For someone implementing this from scratch, it's a little simpler.

Quoted 4 lines

Or are there cases where the old style makes more sense?

Thanks,
Rusty.

Hmm, it makes debugging easier as each side can disable publishing separately - I used it all the time when I saw e.g. networking stuck to guess whether I need to investigate the interrupt or the exit handling.

But I'm not hung up on this.

Quoted 1 line

Let me know pls.

If we combine them into one, then these patches no longer depend on the feature bit expansion, which is worthwhile (though I'll take both).

Thanks,
Rusty.

Quoted 10 lines · Rusty Russell

include/linux/virtio_ring.h line 37

Quoted 4 lines · Michael S. Tsirkin

#define VIRTIO_RING_F_USED_EVENT_IDX 29

include/linux/virtio_ring.h +3 -0

+/* The Host publishes the avail index for which it expects a kick
+ * at the end of the used ring. Guest should ignore the used->flags field. */
+#define VIRTIO_RING_F_AVAIL_EVENT_IDX	32

Are you really sure we want to separate the two? Seems a little simpler to have one bit to mean "we're publishing our threshold". For someone implementing this from scratch, it's a little simpler.

Quoted 4 lines

Or are there cases where the old style makes more sense?

Thanks,
Rusty.

Hmm, it makes debugging easier as each side can disable publishing separately - I used it all the time when I saw e.g. networking stuck to guess whether I need to investigate the interrupt or the exit handling.

But I'm not hung up on this.

Let me know pls.

Quoted 15 lines · Michael S. Tsirkin

Define a new feature bit for the host to
declare that it uses an avail_event index
(like Xen) instead of a feature bit
to enable/disable interrupts.

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

---
 include/linux/virtio_ring.h |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

include/linux/virtio_ring.h +0 -0

index f5c1b75..f791772 100644

include/linux/virtio_ring.h line 37

Quoted 6 lines

include/linux/virtio_ring.h +3 -0

@@ -32,6 +32,9 @@
 /* The Guest publishes the used index for which it expects an interrupt
  * at the end of the avail ring. Host should ignore the avail->flags field. */
 #define VIRTIO_RING_F_USED_EVENT_IDX	29
+/* The Host publishes the avail index for which it expects a kick
+ * at the end of the used ring. Guest should ignore the used->flags field. */
+#define VIRTIO_RING_F_AVAIL_EVENT_IDX	32

Are you really sure we want to separate the two? Seems a little simpler to have one bit to mean "we're publishing our threshold". For someone implementing this from scratch, it's a little simpler.

Or are there cases where the old style makes more sense?

Thanks,
Rusty.

Define a new feature bit for the host to
declare that it uses an avail_event index
(like Xen) instead of a feature bit
to enable/disable interrupts.

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

---
 include/linux/virtio_ring.h |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

include/linux/virtio_ring.h +0 -0

index f5c1b75..f791772 100644

With the new used_event and avail_event and features, both host and guest need similar logic to check whether events are enabled, so it helps to put the common code in the header.

Note that Xen has similar logic for notification hold-off in include/xen/interface/io/ring.h with req_event and req_prod corresponding to event_idx + 1 and new_idx respectively. +1 comes from the fact that req_event and req_prod in Xen start at 1, while event index in virtio starts at 0.

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

---
 include/linux/virtio_ring.h |   14 ++++++++++++++

 1 files changed, 14 insertions(+), 0 deletions(-)

include/linux/virtio_ring.h +0 -0

index f791772..2a3b0ea 100644

Quoted 23 lines · Michael S. Tsirkin

With the new used_event and avail_event and features, both host and guest need similar logic to check whether events are enabled, so it helps to put the common code in the header.

Note that Xen has similar logic for notification hold-off in include/xen/interface/io/ring.h with req_event and req_prod corresponding to event_idx + 1 and new_idx respectively. +1 comes from the fact that req_event and req_prod in Xen start at 1, while event index in virtio starts at 0.

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

---
 include/linux/virtio_ring.h |   14 ++++++++++++++

 1 files changed, 14 insertions(+), 0 deletions(-)

include/linux/virtio_ring.h +0 -0

index f791772..2a3b0ea 100644
@@ -124,6 +124,20 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
                + sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * num;
  }
 

include/linux/virtio_ring.h static inline unsigned vring_size(unsigned int num, unsigned long align) · line 128

Quoted 2 lines

include/linux/virtio_ring.h +2 -0

+/* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
+/* Assuming a given event_idx value from the other size, if

s/other size/other side/ ?

Stefan

Quoted 23 lines · Michael S. Tsirkin

With the new used_event and avail_event and features, both host and guest need similar logic to check whether events are enabled, so it helps to put the common code in the header.

Note that Xen has similar logic for notification hold-off in include/xen/interface/io/ring.h with req_event and req_prod corresponding to event_idx + 1 and new_idx respectively. +1 comes from the fact that req_event and req_prod in Xen start at 1, while event index in virtio starts at 0.

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

---
 include/linux/virtio_ring.h |   14 ++++++++++++++

 1 files changed, 14 insertions(+), 0 deletions(-)

include/linux/virtio_ring.h +0 -0

index f791772..2a3b0ea 100644
@@ -124,6 +124,20 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
                + sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * num;
  }
 

include/linux/virtio_ring.h static inline unsigned vring_size(unsigned int num, unsigned long align) · line 128

Quoted 2 lines

include/linux/virtio_ring.h +2 -0

+/* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
+/* Assuming a given event_idx value from the other size, if

s/other size/other side/ ?

Stefan

With the new used_event and avail_event and features, both host and guest need similar logic to check whether events are enabled, so it helps to put the common code in the header.

Note that Xen has similar logic for notification hold-off in include/xen/interface/io/ring.h with req_event and req_prod corresponding to event_idx + 1 and new_idx respectively. +1 comes from the fact that req_event and req_prod in Xen start at 1, while event index in virtio starts at 0.

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

---
 include/linux/virtio_ring.h |   14 ++++++++++++++

 1 files changed, 14 insertions(+), 0 deletions(-)

include/linux/virtio_ring.h +0 -0

index f791772..2a3b0ea 100644

Add support for the used_event idx feature: when enabling interrupts, publish the current avail index value to the host so that we get interrupts on the next update.

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

---
 drivers/virtio/virtio_ring.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

drivers/virtio/virtio_ring.c +0 -0

index 507d6eb..3a3ed75 100644
@@ -320,6 +320,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 	ret = vq->data[i];

Quoted 15 lines · Michael S. Tsirkin

Add support for the used_event idx feature: when enabling interrupts, publish the current avail index value to the host so that we get interrupts on the next update.

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

---
 drivers/virtio/virtio_ring.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

drivers/virtio/virtio_ring.c +0 -0

index 507d6eb..3a3ed75 100644
@@ -320,6 +320,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 	ret = vq->data[i];

drivers/virtio/virtio_ring.c void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len) · line 330

Quoted 10 lines

drivers/virtio/virtio_ring.c +8 -0

 	detach_buf(vq, i);
 	vq->last_used_idx++;
+	/* If we expect an interrupt for the next entry, tell host
+	 * by writing event index and flush out the write before
+	 * the read in the next get_buf call. */
+	if (!(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) {
+		vring_used_event(&vq->vring) = vq->last_used_idx;
+		virtio_mb();
+	}
+

Hmm, so you're still using the avail->flags; it's just if thresholding is enabled the host will ignore it?

It's a little subtle, but it keeps this patch small.

Perhaps we'll want to make it more explicit later.

Thanks,
Rusty.

Quoted 15 lines · Michael S. Tsirkin

Add support for the used_event idx feature: when enabling interrupts, publish the current avail index value to the host so that we get interrupts on the next update.

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

---
 drivers/virtio/virtio_ring.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

drivers/virtio/virtio_ring.c +0 -0

index 507d6eb..3a3ed75 100644
@@ -320,6 +320,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 	ret = vq->data[i];

drivers/virtio/virtio_ring.c void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len) · line 330

Quoted 10 lines

drivers/virtio/virtio_ring.c +8 -0

 	detach_buf(vq, i);
 	vq->last_used_idx++;
+	/* If we expect an interrupt for the next entry, tell host
+	 * by writing event index and flush out the write before
+	 * the read in the next get_buf call. */
+	if (!(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) {
+		vring_used_event(&vq->vring) = vq->last_used_idx;
+		virtio_mb();
+	}
+

Hmm, so you're still using the avail->flags; it's just if thresholding is enabled the host will ignore it?

It's a little subtle, but it keeps this patch small.

Perhaps we'll want to make it more explicit later.

Thanks,
Rusty.

Add support for the used_event idx feature: when enabling interrupts, publish the current avail index value to the host so that we get interrupts on the next update.

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

---
 drivers/virtio/virtio_ring.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

drivers/virtio/virtio_ring.c +0 -0

index 507d6eb..3a3ed75 100644
@@ -320,6 +320,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 	ret = vq->data[i];

Use the new avail_event feature to reduce the number of exits from the guest.

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

---
 drivers/virtio/virtio_ring.c |   39

++++++++++++++++++++++++++++++++++++++- 1 files changed, 38 insertions(+), 1 deletions(-)

drivers/virtio/virtio_ring.c +9 -0

index 3a3ed75..262dfe6 100644
@@ -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. */

drivers/virtio/virtio_ring.c +23 -1

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

Quoted 30 lines · Michael S. Tsirkin

Use the new avail_event feature to reduce the number of exits from the guest.

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

---
 drivers/virtio/virtio_ring.c |   39

++++++++++++++++++++++++++++++++++++++- 1 files changed, 38 insertions(+), 1 deletions(-)

drivers/virtio/virtio_ring.c +9 -0

index 3a3ed75..262dfe6 100644
@@ -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. */

drivers/virtio/virtio_ring.c add_head: · line 245

Quoted 9 lines

drivers/virtio/virtio_ring.c +6 -0

@@ -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

Quoted 62 lines

drivers/virtio/virtio_ring.c +23 -1

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

Quoted 30 lines · Michael S. Tsirkin

Use the new avail_event feature to reduce the number of exits from the guest.

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

---
 drivers/virtio/virtio_ring.c |   39

++++++++++++++++++++++++++++++++++++++- 1 files changed, 38 insertions(+), 1 deletions(-)

drivers/virtio/virtio_ring.c +9 -0

index 3a3ed75..262dfe6 100644
@@ -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. */

drivers/virtio/virtio_ring.c add_head: · line 245

Quoted 9 lines

drivers/virtio/virtio_ring.c +6 -0

@@ -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

Quoted 62 lines

drivers/virtio/virtio_ring.c +23 -1

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

Use the new avail_event feature to reduce the number of exits from the guest.

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

---
 drivers/virtio/virtio_ring.c |   39

++++++++++++++++++++++++++++++++++++++- 1 files changed, 38 insertions(+), 1 deletions(-)

drivers/virtio/virtio_ring.c +9 -0

index 3a3ed75..262dfe6 100644
@@ -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. */

drivers/virtio/virtio_ring.c +23 -1

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

Use the new avail_event feature to reduce the number of exits from the guest.

Quoted 2 lines · Michael S. Tsirkin

Use the new avail_event feature to reduce the number of exits from the guest.

Figures here would be nice :)

drivers/virtio/virtio_ring.c add_head: · line 245

Quoted 10 lines

drivers/virtio/virtio_ring.c +6 -0

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

Quoted 8 lines

drivers/virtio/virtio_ring.c +8 -0

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

drivers/virtio/virtio_ring.c add_head: · line 267

Quoted 5 lines

drivers/virtio/virtio_ring.c +5 -0

+	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?

drivers/virtio/virtio_ring.c void vring_transport_features(struct virtio_device *vdev) · line 524

Quoted 9 lines

drivers/virtio/virtio_ring.c +2 -0

@@ -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.

Quoted 5 lines · Rusty Russell

Quoted 2 lines · Michael S. Tsirkin

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?

Quoted 13 lines

drivers/virtio/virtio_ring.c add_head: · line 245

Quoted 11 lines

drivers/virtio/virtio_ring.c +6 -0

@@ -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:

Quoted 1 line

        WARN_ON(vq->vring.avail->idx == vq->kicked_avail);

Hmm, but does it say that somewhere?

Quoted 12 lines

drivers/virtio/virtio_ring.c void vring_transport_features(struct virtio_device *vdev) · line 524

Quoted 9 lines

drivers/virtio/virtio_ring.c +2 -0

@@ -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?

Quoted 2 lines

Thanks,
Rusty.

Well if we don't support the feature in the ring we should not ack the feature, right?

Quoted 8 lines · Michael S. Tsirkin

Quoted 5 lines · Rusty Russell

Quoted 2 lines · Michael S. Tsirkin

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.

Quoted 15 lines

Quoted 14 lines

drivers/virtio/virtio_ring.c add_head: · line 245

Quoted 11 lines

drivers/virtio/virtio_ring.c +6 -0

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

Quoted 1 line

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.

Quoted 12 lines

Quoted 12 lines

drivers/virtio/virtio_ring.c void vring_transport_features(struct virtio_device *vdev) · line 524

Quoted 9 lines

drivers/virtio/virtio_ring.c +2 -0

@@ -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?

Quoted 5 lines

Quoted 2 lines

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.

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.

Quoted 7 lines · Michael S. Tsirkin

Well one can imagine a driver doing:

	while (virtqueue_get_buf()) {
		virtqueue_add_buf()
	}
	virtqueue_kick()

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 7 lines · Michael S. Tsirkin

Well one can imagine a driver doing:

	while (virtqueue_get_buf()) {
		virtqueue_add_buf()
	}
	virtqueue_kick()

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 7 lines · Michael S. Tsirkin

Well one can imagine a driver doing:

	while (virtqueue_get_buf()) {
		virtqueue_add_buf()
	}
	virtqueue_kick()

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 7 lines · Michael S. Tsirkin

Well one can imagine a driver doing:

	while (virtqueue_get_buf()) {
		virtqueue_add_buf()
	}
	virtqueue_kick()

Quoted 2 lines

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

Quoted 2 lines

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.

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.

Quoted 8 lines · Michael S. Tsirkin

Quoted 5 lines · Rusty Russell

Quoted 2 lines · Michael S. Tsirkin

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.

Quoted 15 lines

Quoted 14 lines

drivers/virtio/virtio_ring.c add_head: · line 245

Quoted 11 lines

drivers/virtio/virtio_ring.c +6 -0

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

Quoted 1 line

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.

Quoted 12 lines

Quoted 12 lines

drivers/virtio/virtio_ring.c void vring_transport_features(struct virtio_device *vdev) · line 524

Quoted 9 lines

drivers/virtio/virtio_ring.c +2 -0

@@ -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?

Quoted 5 lines

Quoted 2 lines

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.

On Sun, 15 May 2011 16:55:41 +0300, "Michael S. Tsirkin" <[email protected]>

Quoted 2 lines · Rusty Russell

Quoted 1 line

On Wed, 4 May 2011 23:51:47 +0300, "Michael S. Tsirkin" <[email protected]>

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 17 lines

Quoted 15 lines

drivers/virtio/virtio_ring.c add_head: · line 247

Quoted 13 lines

drivers/virtio/virtio_ring.c +6 -0

@@ -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:

Quoted 1 line

        WARN_ON(vq->vring.avail->idx == vq->kicked_avail);

Quoted 1 line

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.

Quoted 17 lines

Quoted 17 lines

Quoted 4 lines

drivers/virtio/virtio_ring.c +0 -0

@@ -482,6 +517,8 @@ void vring_transport_features(struct
 virtio_device *vdev)
 
 			break;

drivers/virtio/virtio_ring.c void vring_transport_features(struct · line 524

Quoted 10 lines

drivers/virtio/virtio_ring.c +2 -0

 
 		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?

Quoted 5 lines

Quoted 2 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

On Sun, 15 May 2011 16:55:41 +0300, "Michael S. Tsirkin" <[email protected]>

Quoted 2 lines · Rusty Russell

Quoted 1 line

On Wed, 4 May 2011 23:51:47 +0300, "Michael S. Tsirkin" <[email protected]>

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 17 lines

Quoted 15 lines

drivers/virtio/virtio_ring.c add_head: · line 247

Quoted 13 lines

drivers/virtio/virtio_ring.c +6 -0

@@ -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:

Quoted 1 line

        WARN_ON(vq->vring.avail->idx == vq->kicked_avail);

Quoted 1 line

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.

Quoted 17 lines

Quoted 17 lines

Quoted 4 lines

drivers/virtio/virtio_ring.c +0 -0

@@ -482,6 +517,8 @@ void vring_transport_features(struct
 virtio_device *vdev)
 
 			break;

drivers/virtio/virtio_ring.c void vring_transport_features(struct · line 524

Quoted 10 lines

drivers/virtio/virtio_ring.c +2 -0

 
 		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?

Quoted 5 lines

Quoted 2 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Rusty Russell

Quoted 2 lines · Michael S. Tsirkin

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?

Quoted 13 lines

drivers/virtio/virtio_ring.c add_head: · line 245

Quoted 11 lines

drivers/virtio/virtio_ring.c +6 -0

@@ -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:

Quoted 1 line

        WARN_ON(vq->vring.avail->idx == vq->kicked_avail);

Hmm, but does it say that somewhere?

Quoted 12 lines

drivers/virtio/virtio_ring.c void vring_transport_features(struct virtio_device *vdev) · line 524

Quoted 9 lines

drivers/virtio/virtio_ring.c +2 -0

@@ -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?

Quoted 2 lines

Thanks,
Rusty.

Well if we don't support the feature in the ring we should not ack the feature, right?

Quoted 2 lines · Michael S. Tsirkin

Use the new avail_event feature to reduce the number of exits from the guest.

Figures here would be nice :)

drivers/virtio/virtio_ring.c add_head: · line 245

Quoted 10 lines

drivers/virtio/virtio_ring.c +6 -0

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

Quoted 8 lines

drivers/virtio/virtio_ring.c +8 -0

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

drivers/virtio/virtio_ring.c add_head: · line 267

Quoted 5 lines

drivers/virtio/virtio_ring.c +5 -0

+	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?

drivers/virtio/virtio_ring.c void vring_transport_features(struct virtio_device *vdev) · line 524

Quoted 9 lines

drivers/virtio/virtio_ring.c +2 -0

@@ -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.

Use the new avail_event feature to reduce the number of exits from the guest.