virtio: put last seen used index into ring itself

PATCH In review Michael S. Tsirkin 2 participants 18 messages Jun 1, 2010 – Jun 6, 2010

Subsystems (4)

Network drivers KVM virtio core virtio net driver

lore

Share & tools

lore

Michael S. Tsirkin

author maintainer

2010-06-01 14:47 UTC #0 lore raw

v3 cover virtio: put last seen used index into ring itself

Changes from v2: added padding between avail idx and flags, and changed virtio to only publish used index when callbacks are enabled.

Here's a rewrite of the original patch with a new layout. I haven't tested it yet so no idea how this performs, but I think this addresses the cache bounce issue raised by Avi. Posting for early flames/comments.

Generally, the Host end of the virtio ring doesn't need to see where Guest is up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, host can reduce the number of interrupts by detecting that the guest is currently handling previous buffers.

We add a feature bit so the guest can tell the host that it's writing out the current value there, if it wants to use that.

This differs from original approach in that the used index is put after avail index (they are typically written out together). To avoid cache bounces on descriptor access,
and make future extensions easier, we put the ring itself at start of page, and move the control after it.

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

Michael S. Tsirkin (2):
virtio: support layout with avail ring before idx
virtio: publish used idx

 drivers/net/virtio_net.c     |    2 +
 drivers/virtio/virtio_ring.c |   25 +++++++++++++++------
 include/linux/virtio_ring.h  |   50 ++++++++++++++++++++++++++++++++++++-----
 3 files changed, 64 insertions(+), 13 deletions(-)

Michael S. Tsirkin

author maintainer

2010-06-01 14:47 UTC #1 lore raw

PATCH v3 1/2 virtio: support layout with avail ring before idx

This adds an (unused) option to put available ring before control (avail index, flags), and adds padding between index and flags. This avoids cache line sharing between control and ring, and also makes it possible to extend avail control without incurring extra cache misses.

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

---
 drivers/virtio/virtio_ring.c |   12 ++++++------
 include/linux/virtio_ring.h  |   40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 40 insertions(+), 12 deletions(-)

drivers/virtio/virtio_ring.c +6 -6

index 1ca8890..0f684b4 100644
@@ -223,8 +223,8 @@ add_head:
 
 	/* Put entry in available array (but don't update avail->idx until they
 	 * do sync).  FIXME: avoid modulus here? */
-	avail = (vq->vring.avail->idx + vq->num_added++) % vq->vring.num;
-	vq->vring.avail->ring[avail] = head;
+	avail = (*vq->vring.avail_idx + vq->num_added++) % vq->vring.num;
+	vq->vring.avail_ring[avail] = head;
 
 	pr_debug("Added buffer head %i to %p\n", head, vq);
 	END_USE(vq);

@@ -244,7 +244,7 @@ void virtqueue_kick(struct virtqueue *_vq)
 	 * new available array entries. */
 	virtio_wmb();
 
-	vq->vring.avail->idx += vq->num_added;
+	*vq->vring.avail_idx += vq->num_added;
 	vq->num_added = 0;
 
 	/* Need to update avail index before checking if we should notify */

@@ -335,7 +335,7 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
-	vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
+	*vq->vring.avail_flags |= VRING_AVAIL_F_NO_INTERRUPT;
 }
 EXPORT_SYMBOL_GPL(virtqueue_disable_cb);

@@ -347,7 +347,7 @@ bool virtqueue_enable_cb(struct virtqueue *_vq)
 
 	/* We optimistically turn back on interrupts, then check if there was
 	 * more to do. */
-	vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
+	vq->vring.avail_flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
 	virtio_mb();
 	if (unlikely(more_used(vq))) {
 		END_USE(vq);

@@ -425,7 +425,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 	if (!vq)
 		return NULL;
 
-	vring_init(&vq->vring, num, pages, vring_align);
+	vring_init(&vq->vring, num, pages, vring_align, false);
 	vq->vq.callback = callback;
 	vq->vq.vdev = vdev;
 	vq->vq.name = name;

include/linux/virtio_ring.h +34 -6

index e4d144b..458ce41 100644
@@ -47,6 +47,12 @@ struct vring_avail {
 	__u16 ring[];
 };
 
+struct vring_avail_ctrl {
+	__u16 idx;
+	__u8 pad[254];
+	__u16 flags;
+};
+
 /* u32 is used here for ids for padding reasons. */
 struct vring_used_elem {
 	/* Index of start of used descriptor chain. */

@@ -66,7 +72,9 @@ struct vring {
 
 	struct vring_desc *desc;
 
-	struct vring_avail *avail;
+	__u16 *avail_idx;
+	__u16 *avail_flags;
+	__u16 *avail_ring;
 
 	struct vring_used *used;
 };

@@ -79,11 +87,19 @@ struct vring {
  *	// The actual descriptors (16 bytes each)
  *	struct vring_desc desc[num];
  *
- *	// A ring of available descriptor heads with free-running index.
+ *	// A ring of available descriptor heads with a control structure
+ *      // including a free-running index.
+ *      // The ring can come either after  (legacy) or before the control.
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
  *
+ * or
+ *
+ *	__u16 available[num];
+ *	__u16 avail_idx;
+ *	__u8 pad[254]; // Padding to align flags at cache line boundary.
+ *	__u16 avail_flags;
  *	// Padding to the next align boundary.
  *	char pad[];
  *

@@ -94,13 +110,25 @@ struct vring {
  * };
  */
 static inline void vring_init(struct vring *vr, unsigned int num, void *p,
-			      unsigned long align)
+			      unsigned long align, bool avail_ring_first)
 {
+	struct vring_avail *avail = p + num * sizeof(struct vring_desc);
 	vr->num = num;
 	vr->desc = p;
-	vr->avail = p + num*sizeof(struct vring_desc);
-	vr->used = (void *)(((unsigned long)&vr->avail->ring[num] + align-1)
-			    & ~(align - 1));
+	if (avail_ring_first) {
+		struct vring_avail_ctrl *ctrl = (void *)&vr->avail_ring[num];
+		vr->avail_ring = (void*)avail;
+		vr->avail_idx = &ctrl->idx;
+		vr->avail_flags = &ctrl->flags;
+	} else {
+		vr->avail_idx = &avail->idx;
+		vr->avail_flags = &avail->flags;
+	}
+	vr->used = (void *)ALIGN((unsigned long)&avail->ring[num], align);
+	/* Verify that avail fits before used. */
+	BUG_ON((unsigned long)(vr->avail_flags + 1) > (unsigned long)vr->used);
+	BUG_ON((unsigned long)(vr->avail_idx + 1) > (unsigned long)vr->used);
+	BUG_ON((unsigned long)(&vr->avail_ring[num]) > (unsigned long)vr->used);
 }
 
 static inline unsigned vring_size(unsigned int num, unsigned long align)

Signature · 1 line

1.7.1.12.g42b7f

Rusty Russell

2010-06-04 02:34 UTC #2 lore raw

Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx

Quoted 6 lines · Michael S. Tsirkin

This adds an (unused) option to put available ring before control (avail index, flags), and adds padding between index and flags. This avoids cache line sharing between control and ring, and also makes it possible to extend avail control without incurring extra cache misses.
Signed-off-by Michael S. Tsirkin <[email protected]>

No no no no. 254? You're trying to Morton me![1]

How's this (untested):

include/linux/virtio_ring.h +11 -3

@@ -74,8 +74,8 @@ struct vring {
 /* The standard layout for the ring is a continuous chunk of memory which looks
  * like this.  We assume num is a power of 2.
  *
- * struct vring
- * {
+ * struct vring {
+ *	*** The driver writes to this part.
  *	// The actual descriptors (16 bytes each)
  *	struct vring_desc desc[num];
  *

@@ -84,9 +84,11 @@ struct vring {
  *	__u16 avail_idx;
  *	__u16 available[num];
  *
- *	// Padding to the next align boundary.
+ *	// Padding so used_flags is on the next align boundary.
  *	char pad[];
+ *	__u16 last_used; // On a cacheline of its own.
  *
+ *	*** The device writes to this part.
  *	// A ring of used descriptor heads with free-running index.
  *	__u16 used_flags;
  *	__u16 used_idx;

@@ -110,6 +112,12 @@ static inline unsigned vring_size(unsign
 		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
 }
 
+/* Last used index sits at the very end of the driver part of the struct */
+static inline __u16 *vring_last_used_idx(const struct vring *vr)
+{
+	return (__u16 *)vr->used - 1;
+}
+
 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Cheers,
Rusty.
[1] Andrew Morton has this technique where he posts a solution so ugly it forces others to fix it properly. Ego-roping, basically.

Michael S. Tsirkin

author maintainer

2010-06-04 10:35 UTC #3 lore raw

Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx

Quoted 9 lines · Rusty Russell

Quoted 6 lines · Michael S. Tsirkin
This adds an (unused) option to put available ring before control (avail index, flags), and adds padding between index and flags. This avoids cache line sharing between control and ring, and also makes it possible to extend avail control without incurring extra cache misses.
Signed-off-by Michael S. Tsirkin <[email protected]>
No no no no. 254? You're trying to Morton me![1]

Hmm, I wonder what will we do if we want a 3rd field on a separate chacheline. But ok.

Quoted 1 line

How's this (untested):

I think we also want to put flags there as well,
they are used on interrupt path, together with last used index.

Quoted 45 lines

include/linux/virtio_ring.h +11 -3

@@ -74,8 +74,8 @@ struct vring {
 /* The standard layout for the ring is a continuous chunk of memory which looks
  * like this.  We assume num is a power of 2.
  *
- * struct vring
- * {
+ * struct vring {
+ *	*** The driver writes to this part.
  *	// The actual descriptors (16 bytes each)
  *	struct vring_desc desc[num];
  *

@@ -84,9 +84,11 @@ struct vring {
  *	__u16 avail_idx;
  *	__u16 available[num];
  *
- *	// Padding to the next align boundary.
+ *	// Padding so used_flags is on the next align boundary.
  *	char pad[];
+ *	__u16 last_used; // On a cacheline of its own.
  *
+ *	*** The device writes to this part.
  *	// A ring of used descriptor heads with free-running index.
  *	__u16 used_flags;
  *	__u16 used_idx;

@@ -110,6 +112,12 @@ static inline unsigned vring_size(unsign
 		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
 }
 
+/* Last used index sits at the very end of the driver part of the struct */
+static inline __u16 *vring_last_used_idx(const struct vring *vr)
+{
+	return (__u16 *)vr->used - 1;
+}
+
 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Cheers,
Rusty.
[1] Andrew Morton has this technique where he posts a solution so ugly it forces others to fix it properly. Ego-roping, basically.

Michael S. Tsirkin

author maintainer

2010-06-04 10:40 UTC #4 lore raw

Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx

In reply to Rusty Russell

Quoted 9 lines · Rusty Russell

Quoted 6 lines · Michael S. Tsirkin
This adds an (unused) option to put available ring before control (avail index, flags), and adds padding between index and flags. This avoids cache line sharing between control and ring, and also makes it possible to extend avail control without incurring extra cache misses.
Signed-off-by Michael S. Tsirkin <[email protected]>
No no no no. 254? You're trying to Morton me![1]

Hmm, I wonder what will we do if we want a 3rd field on a separate chacheline. But ok.

Quoted 1 line

How's this (untested):

I think we also want to put flags there as well,
they are used on interrupt path, together with last used index.

Quoted 45 lines

include/linux/virtio_ring.h +11 -3

@@ -74,8 +74,8 @@ struct vring {
 /* The standard layout for the ring is a continuous chunk of memory which looks
  * like this.  We assume num is a power of 2.
  *
- * struct vring
- * {
+ * struct vring {
+ *	*** The driver writes to this part.
  *	// The actual descriptors (16 bytes each)
  *	struct vring_desc desc[num];
  *

@@ -84,9 +84,11 @@ struct vring {
  *	__u16 avail_idx;
  *	__u16 available[num];
  *
- *	// Padding to the next align boundary.
+ *	// Padding so used_flags is on the next align boundary.
  *	char pad[];
+ *	__u16 last_used; // On a cacheline of its own.
  *
+ *	*** The device writes to this part.
  *	// A ring of used descriptor heads with free-running index.
  *	__u16 used_flags;
  *	__u16 used_idx;

@@ -110,6 +112,12 @@ static inline unsigned vring_size(unsign
 		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
 }
 
+/* Last used index sits at the very end of the driver part of the struct */
+static inline __u16 *vring_last_used_idx(const struct vring *vr)
+{
+	return (__u16 *)vr->used - 1;
+}
+
 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Cheers,
Rusty.
[1] Andrew Morton has this technique where he posts a solution so ugly it forces others to fix it properly. Ego-roping, basically.

Rusty Russell

2010-06-04 11:16 UTC #5 lore raw

Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx

Quoted 11 lines · Michael S. Tsirkin

Quoted 9 lines · Rusty Russell
Quoted 6 lines · Michael S. Tsirkin
This adds an (unused) option to put available ring before control (avail index, flags), and adds padding between index and flags. This avoids cache line sharing between control and ring, and also makes it possible to extend avail control without incurring extra cache misses.
Signed-off-by Michael S. Tsirkin <[email protected]>
No no no no. 254? You're trying to Morton me![1]

Quoted 7 lines

Hmm, I wonder what will we do if we want a 3rd field on a separate chacheline. But ok.
Quoted 1 line
How's this (untested):
I think we also want to put flags there as well,
they are used on interrupt path, together with last used index.

I'm uncomfortable with moving a field.

We haven't done that before and I wonder what will break with old code.

Should we instead just abandon the flags field and use last_used only? Or, more radically, put flags == last_used when the feature is on?

Thoughts?
Rusty.

Michael S. Tsirkin

author maintainer

2010-06-04 11:42 UTC #6 lore raw

Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx

Quoted 15 lines · Rusty Russell

Quoted 14 lines · Michael S. Tsirkin
Quoted 9 lines · Rusty Russell
Quoted 6 lines · Michael S. Tsirkin
This adds an (unused) option to put available ring before control (avail index, flags), and adds padding between index and flags. This avoids cache line sharing between control and ring, and also makes it possible to extend avail control without incurring extra cache misses.
Signed-off-by Michael S. Tsirkin <[email protected]>
No no no no. 254? You're trying to Morton me![1]
Hmm, I wonder what will we do if we want a 3rd field on a separate chacheline. But ok.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
How's this (untested):
I think we also want to put flags there as well,
they are used on interrupt path, together with last used index.
I'm uncomfortable with moving a field.
We haven't done that before and I wonder what will break with old code.

With e.g. my patch, We only do this conditionally when bit is negotitated.

Quoted 5 lines

Should we instead just abandon the flags field and use last_used only? Or, more radically, put flags == last_used when the feature is on?
Thoughts?
Rusty.

Hmm, e.g. with TX and virtio net, we almost never want interrupts, whatever the index value.

Signature · 1 line

MST

Rusty Russell

2010-06-05 04:10 UTC #7 lore raw

Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines · Rusty Russell
I'm uncomfortable with moving a field.
We haven't done that before and I wonder what will break with old code.
With e.g. my patch, We only do this conditionally when bit is negotitated.

Of course, but see this change:

commit ef688e151c00e5d529703be9a04fd506df8bc54e

Author: Rusty Russell <[email protected]>
Date:   Fri Jun 12 22:16:35 2009 -0600

virtio: meet virtio spec by finalizing features before using device

Virtio devices are supposed to negotiate features before they start using the device, but the current code doesn't do this. This is because the driver's probe() function invariably has to add buffers to a virtqueue, or probe the disk (virtio_blk).

This currently doesn't matter since no existing backend is strict about the feature negotiation. But it's possible to imagine a future feature which completely changes how a device operates: in this case, we'd need to acknowledge it before using the device.

Signed-off-by Rusty Russell <[email protected]>

Now, this isn't impossible to overcome: we know that if they use the ring before completing feature negotiation then they don't understand the new format.

But we have to be aware of that on the qemu side. Are we?

Quoted 8 lines

Quoted 5 lines
Should we instead just abandon the flags field and use last_used only? Or, more radically, put flags == last_used when the feature is on?
Thoughts?
Rusty.
Hmm, e.g. with TX and virtio net, we almost never want interrupts, whatever the index value.

Good point. OK, I give in, I'll take your patch which moves the fields to the end. Is that your preference?

Please be careful with the qemu side though...

It's not inconceivable that I'll write that virtio cacheline simulator this (coming) week, too...

Thanks.
Rusty.

Michael S. Tsirkin

author maintainer

2010-06-06 09:11 UTC #8 lore raw

Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx

Quoted 26 lines · Rusty Russell

Quoted 6 lines · Michael S. Tsirkin
Quoted 3 lines · Rusty Russell
I'm uncomfortable with moving a field.
We haven't done that before and I wonder what will break with old code.
With e.g. my patch, We only do this conditionally when bit is negotitated.
Of course, but see this change:
commit ef688e151c00e5d529703be9a04fd506df8bc54e
Author: Rusty Russell <[email protected]>
Date:   Fri Jun 12 22:16:35 2009 -0600
virtio: meet virtio spec by finalizing features before using device
Virtio devices are supposed to negotiate features before they start using the device, but the current code doesn't do this. This is because the driver's probe() function invariably has to add buffers to a virtqueue, or probe the disk (virtio_blk).
This currently doesn't matter since no existing backend is strict about the feature negotiation. But it's possible to imagine a future feature which completely changes how a device operates: in this case, we'd need to acknowledge it before using the device.

Quoted 7 lines

Signed-off-by Rusty Russell <[email protected]>
Now, this isn't impossible to overcome: we know that if they use the ring before completing feature negotiation then they don't understand the new format.
But we have to be aware of that on the qemu side. Are we?

I think we are ok. virtqueue_init which sets the avail/ysed pointers is called when we write the base address. So we only need to be careful and not change this feature bit after creating the rings.

Quoted 11 lines

Quoted 8 lines
Quoted 5 lines
Should we instead just abandon the flags field and use last_used only? Or, more radically, put flags == last_used when the feature is on?
Thoughts?
Rusty.
Hmm, e.g. with TX and virtio net, we almost never want interrupts, whatever the index value.
Good point. OK, I give in, I'll take your patch which moves the fields to the end. Is that your preference?

Yes, I think so.
You mean PATCHv3 unchanged with 254 byte padding?

Quoted 7 lines

Please be careful with the qemu side though...
It's not inconceivable that I'll write that virtio cacheline simulator this (coming) week, too...
Thanks.
Rusty.

Michael S. Tsirkin

author maintainer

2010-06-06 09:11 UTC #9 lore raw

Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx

In reply to Rusty Russell

Quoted 26 lines · Rusty Russell

Quoted 6 lines · Michael S. Tsirkin
Quoted 3 lines · Rusty Russell
I'm uncomfortable with moving a field.
We haven't done that before and I wonder what will break with old code.
With e.g. my patch, We only do this conditionally when bit is negotitated.
Of course, but see this change:
commit ef688e151c00e5d529703be9a04fd506df8bc54e
Author: Rusty Russell <[email protected]>
Date:   Fri Jun 12 22:16:35 2009 -0600
virtio: meet virtio spec by finalizing features before using device
Virtio devices are supposed to negotiate features before they start using the device, but the current code doesn't do this. This is because the driver's probe() function invariably has to add buffers to a virtqueue, or probe the disk (virtio_blk).
This currently doesn't matter since no existing backend is strict about the feature negotiation. But it's possible to imagine a future feature which completely changes how a device operates: in this case, we'd need to acknowledge it before using the device.

Quoted 7 lines

Signed-off-by Rusty Russell <[email protected]>
Now, this isn't impossible to overcome: we know that if they use the ring before completing feature negotiation then they don't understand the new format.
But we have to be aware of that on the qemu side. Are we?

Quoted 11 lines

Quoted 8 lines
Quoted 5 lines
Should we instead just abandon the flags field and use last_used only? Or, more radically, put flags == last_used when the feature is on?
Thoughts?
Rusty.
Hmm, e.g. with TX and virtio net, we almost never want interrupts, whatever the index value.
Good point. OK, I give in, I'll take your patch which moves the fields to the end. Is that your preference?

Yes, I think so.
You mean PATCHv3 unchanged with 254 byte padding?

Quoted 7 lines

Please be careful with the qemu side though...
It's not inconceivable that I'll write that virtio cacheline simulator this (coming) week, too...
Thanks.
Rusty.

Rusty Russell

2010-06-05 04:10 UTC #10 lore raw

Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx

In reply to Michael S. Tsirkin

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines · Rusty Russell
I'm uncomfortable with moving a field.
We haven't done that before and I wonder what will break with old code.
With e.g. my patch, We only do this conditionally when bit is negotitated.

Of course, but see this change:

commit ef688e151c00e5d529703be9a04fd506df8bc54e

Author: Rusty Russell <[email protected]>
Date:   Fri Jun 12 22:16:35 2009 -0600

virtio: meet virtio spec by finalizing features before using device

Signed-off-by Rusty Russell <[email protected]>

Now, this isn't impossible to overcome: we know that if they use the ring before completing feature negotiation then they don't understand the new format.

But we have to be aware of that on the qemu side. Are we?

Quoted 8 lines

Quoted 5 lines
Should we instead just abandon the flags field and use last_used only? Or, more radically, put flags == last_used when the feature is on?
Thoughts?
Rusty.
Hmm, e.g. with TX and virtio net, we almost never want interrupts, whatever the index value.

Good point. OK, I give in, I'll take your patch which moves the fields to the end. Is that your preference?

Please be careful with the qemu side though...

It's not inconceivable that I'll write that virtio cacheline simulator this (coming) week, too...

Thanks.
Rusty.

Michael S. Tsirkin

author maintainer

2010-06-04 11:42 UTC #11 lore raw

Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx

In reply to Rusty Russell

Quoted 15 lines · Rusty Russell

Quoted 14 lines · Michael S. Tsirkin
Quoted 9 lines · Rusty Russell
Quoted 6 lines · Michael S. Tsirkin
This adds an (unused) option to put available ring before control (avail index, flags), and adds padding between index and flags. This avoids cache line sharing between control and ring, and also makes it possible to extend avail control without incurring extra cache misses.
Signed-off-by Michael S. Tsirkin <[email protected]>
No no no no. 254? You're trying to Morton me![1]
Hmm, I wonder what will we do if we want a 3rd field on a separate chacheline. But ok.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
How's this (untested):
I think we also want to put flags there as well,
they are used on interrupt path, together with last used index.
I'm uncomfortable with moving a field.
We haven't done that before and I wonder what will break with old code.

With e.g. my patch, We only do this conditionally when bit is negotitated.

Quoted 5 lines

Should we instead just abandon the flags field and use last_used only? Or, more radically, put flags == last_used when the feature is on?
Thoughts?
Rusty.

Hmm, e.g. with TX and virtio net, we almost never want interrupts, whatever the index value.

Signature · 1 line

MST

Rusty Russell

2010-06-04 11:16 UTC #12 lore raw

Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx

In reply to Michael S. Tsirkin

Quoted 11 lines · Michael S. Tsirkin

Quoted 9 lines · Rusty Russell
Quoted 6 lines · Michael S. Tsirkin
This adds an (unused) option to put available ring before control (avail index, flags), and adds padding between index and flags. This avoids cache line sharing between control and ring, and also makes it possible to extend avail control without incurring extra cache misses.
Signed-off-by Michael S. Tsirkin <[email protected]>
No no no no. 254? You're trying to Morton me![1]

Quoted 7 lines

Hmm, I wonder what will we do if we want a 3rd field on a separate chacheline. But ok.
Quoted 1 line
How's this (untested):
I think we also want to put flags there as well,
they are used on interrupt path, together with last used index.

I'm uncomfortable with moving a field.

We haven't done that before and I wonder what will break with old code.

Should we instead just abandon the flags field and use last_used only? Or, more radically, put flags == last_used when the feature is on?

Thoughts?
Rusty.

Rusty Russell

2010-06-04 02:34 UTC #13 lore raw

Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx

In reply to Michael S. Tsirkin

Quoted 6 lines · Michael S. Tsirkin

This adds an (unused) option to put available ring before control (avail index, flags), and adds padding between index and flags. This avoids cache line sharing between control and ring, and also makes it possible to extend avail control without incurring extra cache misses.
Signed-off-by Michael S. Tsirkin <[email protected]>

No no no no. 254? You're trying to Morton me![1]

How's this (untested):

include/linux/virtio_ring.h +11 -3

@@ -74,8 +74,8 @@ struct vring {
 /* The standard layout for the ring is a continuous chunk of memory which looks
  * like this.  We assume num is a power of 2.
  *
- * struct vring
- * {
+ * struct vring {
+ *	*** The driver writes to this part.
  *	// The actual descriptors (16 bytes each)
  *	struct vring_desc desc[num];
  *

@@ -84,9 +84,11 @@ struct vring {
  *	__u16 avail_idx;
  *	__u16 available[num];
  *
- *	// Padding to the next align boundary.
+ *	// Padding so used_flags is on the next align boundary.
  *	char pad[];
+ *	__u16 last_used; // On a cacheline of its own.
  *
+ *	*** The device writes to this part.
  *	// A ring of used descriptor heads with free-running index.
  *	__u16 used_flags;
  *	__u16 used_idx;

@@ -110,6 +112,12 @@ static inline unsigned vring_size(unsign
 		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
 }
 
+/* Last used index sits at the very end of the driver part of the struct */
+static inline __u16 *vring_last_used_idx(const struct vring *vr)
+{
+	return (__u16 *)vr->used - 1;
+}
+
 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Cheers,
Rusty.
[1] Andrew Morton has this technique where he posts a solution so ugly it forces others to fix it properly. Ego-roping, basically.

Michael S. Tsirkin

author maintainer

2010-06-01 14:47 UTC #14 lore raw

PATCH v3 1/2 virtio: support layout with avail ring before idx

In reply to Michael S. Tsirkin

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

---
 drivers/virtio/virtio_ring.c |   12 ++++++------
 include/linux/virtio_ring.h  |   40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 40 insertions(+), 12 deletions(-)

drivers/virtio/virtio_ring.c +6 -6

index 1ca8890..0f684b4 100644
@@ -223,8 +223,8 @@ add_head:
 
 	/* Put entry in available array (but don't update avail->idx until they
 	 * do sync).  FIXME: avoid modulus here? */
-	avail = (vq->vring.avail->idx + vq->num_added++) % vq->vring.num;
-	vq->vring.avail->ring[avail] = head;
+	avail = (*vq->vring.avail_idx + vq->num_added++) % vq->vring.num;
+	vq->vring.avail_ring[avail] = head;
 
 	pr_debug("Added buffer head %i to %p\n", head, vq);
 	END_USE(vq);

@@ -244,7 +244,7 @@ void virtqueue_kick(struct virtqueue *_vq)
 	 * new available array entries. */
 	virtio_wmb();
 
-	vq->vring.avail->idx += vq->num_added;
+	*vq->vring.avail_idx += vq->num_added;
 	vq->num_added = 0;
 
 	/* Need to update avail index before checking if we should notify */

@@ -335,7 +335,7 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
-	vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
+	*vq->vring.avail_flags |= VRING_AVAIL_F_NO_INTERRUPT;
 }
 EXPORT_SYMBOL_GPL(virtqueue_disable_cb);

@@ -347,7 +347,7 @@ bool virtqueue_enable_cb(struct virtqueue *_vq)
 
 	/* We optimistically turn back on interrupts, then check if there was
 	 * more to do. */
-	vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
+	vq->vring.avail_flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
 	virtio_mb();
 	if (unlikely(more_used(vq))) {
 		END_USE(vq);

@@ -425,7 +425,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 	if (!vq)
 		return NULL;
 
-	vring_init(&vq->vring, num, pages, vring_align);
+	vring_init(&vq->vring, num, pages, vring_align, false);
 	vq->vq.callback = callback;
 	vq->vq.vdev = vdev;
 	vq->vq.name = name;

include/linux/virtio_ring.h +34 -6

index e4d144b..458ce41 100644
@@ -47,6 +47,12 @@ struct vring_avail {
 	__u16 ring[];
 };
 
+struct vring_avail_ctrl {
+	__u16 idx;
+	__u8 pad[254];
+	__u16 flags;
+};
+
 /* u32 is used here for ids for padding reasons. */
 struct vring_used_elem {
 	/* Index of start of used descriptor chain. */

@@ -66,7 +72,9 @@ struct vring {
 
 	struct vring_desc *desc;
 
-	struct vring_avail *avail;
+	__u16 *avail_idx;
+	__u16 *avail_flags;
+	__u16 *avail_ring;
 
 	struct vring_used *used;
 };

@@ -79,11 +87,19 @@ struct vring {
  *	// The actual descriptors (16 bytes each)
  *	struct vring_desc desc[num];
  *
- *	// A ring of available descriptor heads with free-running index.
+ *	// A ring of available descriptor heads with a control structure
+ *      // including a free-running index.
+ *      // The ring can come either after  (legacy) or before the control.
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
  *
+ * or
+ *
+ *	__u16 available[num];
+ *	__u16 avail_idx;
+ *	__u8 pad[254]; // Padding to align flags at cache line boundary.
+ *	__u16 avail_flags;
  *	// Padding to the next align boundary.
  *	char pad[];
  *

@@ -94,13 +110,25 @@ struct vring {
  * };
  */
 static inline void vring_init(struct vring *vr, unsigned int num, void *p,
-			      unsigned long align)
+			      unsigned long align, bool avail_ring_first)
 {
+	struct vring_avail *avail = p + num * sizeof(struct vring_desc);
 	vr->num = num;
 	vr->desc = p;
-	vr->avail = p + num*sizeof(struct vring_desc);
-	vr->used = (void *)(((unsigned long)&vr->avail->ring[num] + align-1)
-			    & ~(align - 1));
+	if (avail_ring_first) {
+		struct vring_avail_ctrl *ctrl = (void *)&vr->avail_ring[num];
+		vr->avail_ring = (void*)avail;
+		vr->avail_idx = &ctrl->idx;
+		vr->avail_flags = &ctrl->flags;
+	} else {
+		vr->avail_idx = &avail->idx;
+		vr->avail_flags = &avail->flags;
+	}
+	vr->used = (void *)ALIGN((unsigned long)&avail->ring[num], align);
+	/* Verify that avail fits before used. */
+	BUG_ON((unsigned long)(vr->avail_flags + 1) > (unsigned long)vr->used);
+	BUG_ON((unsigned long)(vr->avail_idx + 1) > (unsigned long)vr->used);
+	BUG_ON((unsigned long)(&vr->avail_ring[num]) > (unsigned long)vr->used);
 }
 
 static inline unsigned vring_size(unsigned int num, unsigned long align)

Signature · 1 line

1.7.1.12.g42b7f

Michael S. Tsirkin

author maintainer

2010-06-01 14:47 UTC #15 lore raw

PATCH v3 2/2 virtio: publish used idx

In reply to Michael S. Tsirkin

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

---
 drivers/net/virtio_net.c     |    2 ++
 drivers/virtio/virtio_ring.c |   15 +++++++++++++--
 include/linux/virtio_ring.h  |   10 ++++++++++
 3 files changed, 25 insertions(+), 2 deletions(-)

drivers/net/virtio_net.c +2 -0

index 78eb319..30e7483 100644
@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/virtio.h>
 #include <linux/virtio_net.h>
+#include <linux/virtio_ring.h>
 #include <linux/scatterlist.h>
 #include <linux/if_vlan.h>
 #include <linux/slab.h>

@@ -1056,6 +1057,7 @@ static unsigned int features[] = {
 	VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO,
 	VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
 	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
+	VIRTIO_RING_F_PUBLISH_USED,
 };
 
 static struct virtio_driver virtio_net_driver = {

drivers/virtio/virtio_ring.c +13 -2

index 0f684b4..5a8c711 100644
@@ -92,6 +92,9 @@ struct vring_virtqueue
 	/* Last used index we've seen. */
 	u16 last_used_idx;
 
+	/* Publish last used index we've seen at this location. */
+	u16 *publish_last_used_idx;
+
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	void (*notify)(struct virtqueue *vq);

@@ -325,7 +328,7 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 	/* detach_buf clears data, so grab it now. */
 	ret = vq->data[i];
 	detach_buf(vq, i);
-	vq->last_used_idx++;
+	*vq->publish_last_used_idx = ++vq->last_used_idx;
 	END_USE(vq);
 	return ret;
 }

@@ -425,13 +428,19 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
 	if (!vq)
 		return NULL;
 
-	vring_init(&vq->vring, num, pages, vring_align, false);
+	vring_init(&vq->vring, num, pages, vring_align,
+		   virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_USED));
+	if (virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_USED))
+	    vq->publish_last_used_idx = &vq->vring.avail->last_used_idx;
+	else
+	    vq->publish_last_used_idx = &vq->last_used_idx;
 	vq->vq.callback = callback;
 	vq->vq.vdev = vdev;
 	vq->vq.name = name;
 	vq->notify = notify;
 	vq->broken = false;
 	vq->last_used_idx = 0;
+	*vq->publish_last_used_idx = 0;
 	vq->num_added = 0;
 	list_add_tail(&vq->vq.list, &vdev->vqs);
 #ifdef DEBUG

@@ -473,6 +482,8 @@ void vring_transport_features(struct virtio_device *vdev)
 		switch (i) {
 		case VIRTIO_RING_F_INDIRECT_DESC:
 			break;
+		case VIRTIO_RING_F_PUBLISH_USED:
+			break;
 		default:
 			/* We don't understand this bit. */
 			clear_bit(i, vdev->features);

include/linux/virtio_ring.h +10 -0

index 458ce41..87f8fd3 100644
@@ -29,6 +29,9 @@
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC	28
 
+/* The Guest publishes last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_USED	29
+
 /* Virtio ring descriptors: 16 bytes.  These can chain together via "next". */
 struct vring_desc {
 	/* Address (guest-physical). */

@@ -51,6 +54,7 @@ struct vring_avail_ctrl {
 	__u16 idx;
 	__u8 pad[254];
 	__u16 flags;
+	__u16 last_used_idx;
 };
 
 /* u32 is used here for ids for padding reasons. */

@@ -75,6 +79,7 @@ struct vring {
 	__u16 *avail_idx;
 	__u16 *avail_flags;
 	__u16 *avail_ring;
+	__u16 *last_used_idx;
 
 	struct vring_used *used;
 };

@@ -100,6 +105,7 @@ struct vring {
  *	__u16 avail_idx;
  *	__u8 pad[254]; // Padding to align flags at cache line boundary.
  *	__u16 avail_flags;
+ *	__u16 last_used_idx;
  *	// Padding to the next align boundary.
  *	char pad[];
  *

@@ -120,14 +126,18 @@ static inline void vring_init(struct vring *vr, unsigned int num, void *p,
 		vr->avail_ring = (void*)avail;
 		vr->avail_idx = &ctrl->idx;
 		vr->avail_flags = &ctrl->flags;
+		vr->last_used_idx = &ctrl->last_used_idx;
 	} else {
 		vr->avail_idx = &avail->idx;
 		vr->avail_flags = &avail->flags;
+		vr->last_used_idx = NULL;
 	}
 	vr->used = (void *)ALIGN((unsigned long)&avail->ring[num], align);
 	/* Verify that avail fits before used. */
 	BUG_ON((unsigned long)(vr->avail_flags + 1) > (unsigned long)vr->used);
 	BUG_ON((unsigned long)(vr->avail_idx + 1) > (unsigned long)vr->used);
+	BUG_ON(vr->last_used_idx && (unsigned long)(vr->last_used_idx + 1) >
+	       (unsigned long)vr->used);
 	BUG_ON((unsigned long)(&vr->avail_ring[num]) > (unsigned long)vr->used);
 }

Signature · 1 line

1.7.1.12.g42b7f

Michael S. Tsirkin

author maintainer

2010-06-01 15:12 UTC #16 lore raw

In reply to Michael S. Tsirkin

Quoted 3 lines · Michael S. Tsirkin

Changes from v2: added padding between avail idx and flags, and changed virtio to only publish used index when callbacks are enabled.

Here's the updated spec patch.

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

virtio-spec.lyx +253 -3

index ed35893..a2dcd76 100644
@@ -1803,6 +1803,36 @@ next
 \emph default
  descriptor entry (modulo the ring size).
  This starts at 0, and increases.
+\change_inserted 0 1274966643
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1274968378
+When PUBLISH_USED feature flag has 
+\emph on
+not
+\emph default
+ been negotiated, the ring follows the 
+\begin_inset Quotes eld
+\end_inset
+
+flags
+\begin_inset Quotes erd
+\end_inset
+
+ and the 
+\begin_inset Quotes eld
+\end_inset
+
+idx
+\begin_inset Quotes erd
+\end_inset
+
+ fields:
+\change_unchanged
+
 \end_layout
 
 \begin_layout Standard

@@ -1845,7 +1875,143 @@ struct vring_avail {
 
 \end_layout
 
+\begin_layout Standard
+
+\change_inserted 0 1274968432
+\begin_inset CommandInset label
+LatexCommand label
+name "PUBLISH_USED-feature"
+
+\end_inset
+
+When PUBLISH_USED feature flag has been negotiated, the control structure
+ including the 
+\begin_inset Quotes eld
+\end_inset
+
+flags and the 
+\begin_inset Quotes eld
+\end_inset
+
+idx
+\begin_inset Quotes erd
+\end_inset
+
+ fields follows the ring.
+ This leaves the room for the 
+\begin_inset Quotes eld
+\end_inset
+
+last_seen_used_idx
+\begin_inset Quotes erd
+\end_inset
+
+ field, which indicates the most recent 
+\begin_inset Quotes eld
+\end_inset
+
+idx
+\begin_inset Quotes erd
+\end_inset
+
+ value observed by guest in the used ring (see 
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "sub:Used-Ring"
+
+\end_inset
+
+ below):
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1274967396
+\begin_inset listings
+inline false
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967404
+
+struct vring_avail {
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1275404889
+
+   u16 ring[qsz]; /* qsz is the Queue Size field read from device */
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1275404891
+
+   u16 idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1275404921
+
+   u8 pad[254] /* Padding to avoid sharing cache line between flags and
+ idx fields.
+ */
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967533
+
+#define VRING_AVAIL_F_NO_INTERRUPT      1
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967533
+
+   u16 flags;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274968345
+
+   u16 last_seen_used_idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967396
+
+}; 
+\end_layout
+
+\end_inset
+
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1274967715
+If the ring is large enough, the second layout maintains the control and
+ ring structures on separate cache lines.
+\end_layout
+
 \begin_layout Subsection
+
+\change_inserted 0 1274968415
+\begin_inset CommandInset label
+LatexCommand label
+name "sub:Used-Ring"
+
+\end_inset
+
+
+\change_unchanged
 Used Ring
 \end_layout

@@ -2391,12 +2557,20 @@ status open
 
 \begin_layout Plain Layout
 
-while (vq->last_seen_used != vring->used.idx) {
+while (vq->last_seen_used
+\change_inserted 0 1274968316
+_idx
+\change_unchanged
+ != vring->used.idx) {
 \end_layout
 
 \begin_layout Plain Layout
 
-    struct vring_used_elem *e = vring.used->ring[vq->last_seen_used%vsz];
+    struct vring_used_elem *e = vring.used->ring[vq->last_seen_used
+\change_inserted 0 1274968326
+_idx
+\change_unchanged
+%vsz];
 \end_layout
 
 \begin_layout Plain Layout

@@ -2406,7 +2580,11 @@ while (vq->last_seen_used != vring->used.idx) {
 
 \begin_layout Plain Layout
 
-    vq->last_seen_used++;
+    vq->last_seen_used
+\change_inserted 0 1274968321
+_idx
+\change_unchanged
+++;
 \end_layout
 
 \begin_layout Plain Layout

@@ -2419,6 +2597,15 @@ while (vq->last_seen_used != vring->used.idx) {
 
 \end_layout
 
+\begin_layout Standard
+
+\change_inserted 0 1275405042
+If PUBLISH_USED feature is negotiated, last_seen_used value should be published
+ to the device in the avail ring.
+ This value is used by the host for interrupt mitigation, so it only need
+ to be updated when interrupts are enabled.
+\end_layout
+
 \begin_layout Subsection
 Dealing With Configuration Changes
 \end_layout

@@ -2986,6 +3173,47 @@ struct vring_avail {
 \begin_layout Plain Layout
 
 };
+\change_inserted 0 1274966477
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966484
+
+struct vring_avail_ctrl {
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966489
+
+        __u16 flags;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966494
+
+        __u16 idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966499
+
+        __u16 last_used_idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966474
+
+};
 \end_layout
 
 \begin_layout Plain Layout

@@ -3349,6 +3577,28 @@ reference "sub:Indirect-Descriptors"
 \end_inset
 
 .
+\change_inserted 0 1274967762
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 0 1274967926
+VIRTIO_F_RING_PUBLISH_USED
+\begin_inset space ~
+\end_inset
+
+(29) Negotiating this feature indicates that the avail ring layout includes
+ the used index observed by driver, see
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "PUBLISH_USED-feature"
+
+\end_inset
+
+.
+\change_unchanged
+
 \end_layout
 
 \begin_layout Description

Michael S. Tsirkin

author maintainer

2010-06-01 15:17 UTC #17 lore raw

In reply to Michael S. Tsirkin

Quoted 3 lines · Michael S. Tsirkin

Changes from v2: added padding between avail idx and flags, and changed virtio to only publish used index when callbacks are enabled.

Here's the updated spec patch.

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

virtio-spec.lyx +253 -3

index ed35893..a2dcd76 100644
@@ -1803,6 +1803,36 @@ next
 \emph default
  descriptor entry (modulo the ring size).
  This starts at 0, and increases.
+\change_inserted 0 1274966643
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1274968378
+When PUBLISH_USED feature flag has 
+\emph on
+not
+\emph default
+ been negotiated, the ring follows the 
+\begin_inset Quotes eld
+\end_inset
+
+flags
+\begin_inset Quotes erd
+\end_inset
+
+ and the 
+\begin_inset Quotes eld
+\end_inset
+
+idx
+\begin_inset Quotes erd
+\end_inset
+
+ fields:
+\change_unchanged
+
 \end_layout
 
 \begin_layout Standard

@@ -1845,7 +1875,143 @@ struct vring_avail {
 
 \end_layout
 
+\begin_layout Standard
+
+\change_inserted 0 1274968432
+\begin_inset CommandInset label
+LatexCommand label
+name "PUBLISH_USED-feature"
+
+\end_inset
+
+When PUBLISH_USED feature flag has been negotiated, the control structure
+ including the 
+\begin_inset Quotes eld
+\end_inset
+
+flags and the 
+\begin_inset Quotes eld
+\end_inset
+
+idx
+\begin_inset Quotes erd
+\end_inset
+
+ fields follows the ring.
+ This leaves the room for the 
+\begin_inset Quotes eld
+\end_inset
+
+last_seen_used_idx
+\begin_inset Quotes erd
+\end_inset
+
+ field, which indicates the most recent 
+\begin_inset Quotes eld
+\end_inset
+
+idx
+\begin_inset Quotes erd
+\end_inset
+
+ value observed by guest in the used ring (see 
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "sub:Used-Ring"
+
+\end_inset
+
+ below):
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1274967396
+\begin_inset listings
+inline false
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967404
+
+struct vring_avail {
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1275404889
+
+   u16 ring[qsz]; /* qsz is the Queue Size field read from device */
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1275404891
+
+   u16 idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1275404921
+
+   u8 pad[254] /* Padding to avoid sharing cache line between flags and
+ idx fields.
+ */
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967533
+
+#define VRING_AVAIL_F_NO_INTERRUPT      1
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967533
+
+   u16 flags;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274968345
+
+   u16 last_seen_used_idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274967396
+
+}; 
+\end_layout
+
+\end_inset
+
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 0 1274967715
+If the ring is large enough, the second layout maintains the control and
+ ring structures on separate cache lines.
+\end_layout
+
 \begin_layout Subsection
+
+\change_inserted 0 1274968415
+\begin_inset CommandInset label
+LatexCommand label
+name "sub:Used-Ring"
+
+\end_inset
+
+
+\change_unchanged
 Used Ring
 \end_layout

@@ -2391,12 +2557,20 @@ status open
 
 \begin_layout Plain Layout
 
-while (vq->last_seen_used != vring->used.idx) {
+while (vq->last_seen_used
+\change_inserted 0 1274968316
+_idx
+\change_unchanged
+ != vring->used.idx) {
 \end_layout
 
 \begin_layout Plain Layout
 
-    struct vring_used_elem *e = vring.used->ring[vq->last_seen_used%vsz];
+    struct vring_used_elem *e = vring.used->ring[vq->last_seen_used
+\change_inserted 0 1274968326
+_idx
+\change_unchanged
+%vsz];
 \end_layout
 
 \begin_layout Plain Layout

@@ -2406,7 +2580,11 @@ while (vq->last_seen_used != vring->used.idx) {
 
 \begin_layout Plain Layout
 
-    vq->last_seen_used++;
+    vq->last_seen_used
+\change_inserted 0 1274968321
+_idx
+\change_unchanged
+++;
 \end_layout
 
 \begin_layout Plain Layout

@@ -2419,6 +2597,15 @@ while (vq->last_seen_used != vring->used.idx) {
 
 \end_layout
 
+\begin_layout Standard
+
+\change_inserted 0 1275405042
+If PUBLISH_USED feature is negotiated, last_seen_used value should be published
+ to the device in the avail ring.
+ This value is used by the host for interrupt mitigation, so it only need
+ to be updated when interrupts are enabled.
+\end_layout
+
 \begin_layout Subsection
 Dealing With Configuration Changes
 \end_layout

@@ -2986,6 +3173,47 @@ struct vring_avail {
 \begin_layout Plain Layout
 
 };
+\change_inserted 0 1274966477
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966484
+
+struct vring_avail_ctrl {
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966489
+
+        __u16 flags;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966494
+
+        __u16 idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966499
+
+        __u16 last_used_idx;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 0 1274966474
+
+};
 \end_layout
 
 \begin_layout Plain Layout

@@ -3349,6 +3577,28 @@ reference "sub:Indirect-Descriptors"
 \end_inset
 
 .
+\change_inserted 0 1274967762
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 0 1274967926
+VIRTIO_F_RING_PUBLISH_USED
+\begin_inset space ~
+\end_inset
+
+(29) Negotiating this feature indicates that the avail ring layout includes
+ the used index observed by driver, see
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "PUBLISH_USED-feature"
+
+\end_inset
+
+.
+\change_unchanged
+
 \end_layout
 
 \begin_layout Description