cleanup memory barrier usage bringing it in sync with what linux guest does. The rules are simple: - read barrier after index read - write barrier before index write Also, call macros smp_rmb/smp_wmb to stress we are not syncing with a real io device here.
While I don't think compiler is crazy/powerful enough to reorder these, anyway, the bogus barriers we currently have in code will confuse anyone who tries to copy/reuse it.
Signed-off-by: Michael S. Tsirkin mst@redhat.com Cc: Gleb Natapov gleb@redhat.com ---
This depends on the NO_NOTIFY patch I sent earlier. Note: compile-tested only Gleb, could you try it out pls?
src/virtio-ring.c | 13 ++++++------- src/virtio-ring.h | 5 +++-- 2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/virtio-ring.c b/src/virtio-ring.c index 493d8b5..97a3ffe 100644 --- a/src/virtio-ring.c +++ b/src/virtio-ring.c @@ -38,8 +38,10 @@ int vring_more_used(struct vring_virtqueue *vq) { struct vring_used *used = GET_FLATPTR(vq->vring.used); - wmb(); - return GET_FLATPTR(vq->last_used_idx) != GET_FLATPTR(used->idx); + int more = GET_FLATPTR(vq->last_used_idx) != GET_FLATPTR(used->idx); + /* Make sure ring reads are done after idx read above. */ + smp_rmb(); + return more; }
/* @@ -63,7 +65,6 @@ void vring_detach(struct vring_virtqueue *vq, unsigned int head) /* link it with free list and point to it */
SET_FLATPTR(desc[i].next, GET_FLATPTR(vq->free_head)); - wmb(); SET_FLATPTR(vq->free_head, head); }
@@ -85,7 +86,6 @@ int vring_get_buf(struct vring_virtqueue *vq, unsigned int *len) // BUG_ON(!vring_more_used(vq));
elem = &used->ring[GET_FLATPTR(vq->last_used_idx) % GET_FLATPTR(vr->num)]; - wmb(); id = GET_FLATPTR(elem->id); if (len != NULL) *len = GET_FLATPTR(elem->len); @@ -136,7 +136,6 @@ void vring_add_buf(struct vring_virtqueue *vq,
av = (GET_FLATPTR(avail->idx) + num_added) % GET_FLATPTR(vr->num); SET_FLATPTR(avail->ring[av], head); - wmb(); }
void vring_kick(unsigned int ioaddr, struct vring_virtqueue *vq, int num_added) @@ -144,9 +143,9 @@ void vring_kick(unsigned int ioaddr, struct vring_virtqueue *vq, int num_added) struct vring *vr = &vq->vring; struct vring_avail *avail = GET_FLATPTR(vr->avail);
- wmb(); + /* Make sure idx update is done after ring write. */ + smp_wmb(); SET_FLATPTR(avail->idx, GET_FLATPTR(avail->idx) + num_added);
- mb(); vp_notify(ioaddr, GET_FLATPTR(vq->queue_index)); } diff --git a/src/virtio-ring.h b/src/virtio-ring.h index 3fb86fe..8b546f4 100644 --- a/src/virtio-ring.h +++ b/src/virtio-ring.h @@ -9,8 +9,9 @@
#define virt_to_phys(v) (unsigned long)(v) #define phys_to_virt(p) (void*)(p) -#define wmb() barrier() -#define mb() barrier() +/* Compiler barrier is enough as an x86 CPU does not reorder reads or writes */ +#define smp_rmb() barrier() +#define smp_wmb() barrier()
/* Status byte for guest to report progress, and synchronize features. */ /* We have seen device and processed generic fields (VIRTIO_CONFIG_F_VIRTIO) */
On Thu, May 20, 2010 at 04:36:32PM +0300, Michael S. Tsirkin wrote:
diff --git a/src/virtio-ring.h b/src/virtio-ring.h index 3fb86fe..8b546f4 100644 --- a/src/virtio-ring.h +++ b/src/virtio-ring.h @@ -9,8 +9,9 @@
#define virt_to_phys(v) (unsigned long)(v) #define phys_to_virt(p) (void*)(p) -#define wmb() barrier() -#define mb() barrier() +/* Compiler barrier is enough as an x86 CPU does not reorder reads or writes */ +#define smp_rmb() barrier() +#define smp_wmb() barrier()
I thought you were going to use real memory barriers (although we concluded compiler barrier should be enough, but it's better to be safe then sorry).
-- Gleb.
On Mon, May 24, 2010 at 12:35:21PM +0300, Gleb Natapov wrote:
On Thu, May 20, 2010 at 04:36:32PM +0300, Michael S. Tsirkin wrote:
diff --git a/src/virtio-ring.h b/src/virtio-ring.h index 3fb86fe..8b546f4 100644 --- a/src/virtio-ring.h +++ b/src/virtio-ring.h @@ -9,8 +9,9 @@
#define virt_to_phys(v) (unsigned long)(v) #define phys_to_virt(p) (void*)(p) -#define wmb() barrier() -#define mb() barrier() +/* Compiler barrier is enough as an x86 CPU does not reorder reads or writes */ +#define smp_rmb() barrier() +#define smp_wmb() barrier()
I thought you were going to use real memory barriers (although we concluded compiler barrier should be enough, but it's better to be safe then sorry).
-- Gleb.
On x86, ops of the same kind are ordered by CPU, so all we need to do is 1. avoid depending on order between reads and writes 2. prevent the compiler from reordering memory accesses
On Mon, May 24, 2010 at 12:39:41PM +0300, Michael S. Tsirkin wrote:
On Mon, May 24, 2010 at 12:35:21PM +0300, Gleb Natapov wrote:
On Thu, May 20, 2010 at 04:36:32PM +0300, Michael S. Tsirkin wrote:
diff --git a/src/virtio-ring.h b/src/virtio-ring.h index 3fb86fe..8b546f4 100644 --- a/src/virtio-ring.h +++ b/src/virtio-ring.h @@ -9,8 +9,9 @@
#define virt_to_phys(v) (unsigned long)(v) #define phys_to_virt(p) (void*)(p) -#define wmb() barrier() -#define mb() barrier() +/* Compiler barrier is enough as an x86 CPU does not reorder reads or writes */ +#define smp_rmb() barrier() +#define smp_wmb() barrier()
I thought you were going to use real memory barriers (although we concluded compiler barrier should be enough, but it's better to be safe then sorry).
-- Gleb.
On x86, ops of the same kind are ordered by CPU, so all we need to do is
- avoid depending on order between reads and writes
- prevent the compiler from reordering memory accesses
So on Linux smp_rmb() and smp_wmb() are just compiler barriers too? (look like they are). In that case lets use barrier() directly no need to obfuscate the code.
-- Gleb.
On Mon, May 24, 2010 at 12:55:30PM +0300, Gleb Natapov wrote:
On Mon, May 24, 2010 at 12:39:41PM +0300, Michael S. Tsirkin wrote:
On Mon, May 24, 2010 at 12:35:21PM +0300, Gleb Natapov wrote:
On Thu, May 20, 2010 at 04:36:32PM +0300, Michael S. Tsirkin wrote:
diff --git a/src/virtio-ring.h b/src/virtio-ring.h index 3fb86fe..8b546f4 100644 --- a/src/virtio-ring.h +++ b/src/virtio-ring.h @@ -9,8 +9,9 @@
#define virt_to_phys(v) (unsigned long)(v) #define phys_to_virt(p) (void*)(p) -#define wmb() barrier() -#define mb() barrier() +/* Compiler barrier is enough as an x86 CPU does not reorder reads or writes */ +#define smp_rmb() barrier() +#define smp_wmb() barrier()
I thought you were going to use real memory barriers (although we concluded compiler barrier should be enough, but it's better to be safe then sorry).
-- Gleb.
On x86, ops of the same kind are ordered by CPU, so all we need to do is
- avoid depending on order between reads and writes
- prevent the compiler from reordering memory accesses
So on Linux smp_rmb() and smp_wmb() are just compiler barriers too? (look like they are).
On x86, yes.
In that case lets use barrier() directly no need to obfuscate the code.
I think it's better to use smp_rmb/smp_wmb because this code tends to get copied around, it might end up on non-x86 platforms. For these, barrier() would be wrong.
Another reason is that it makes it easier to audit code comparing it with linux virtio guest.
-- Gleb.
On Mon, May 24, 2010 at 01:01:34PM +0300, Michael S. Tsirkin wrote:
On Mon, May 24, 2010 at 12:55:30PM +0300, Gleb Natapov wrote:
On Mon, May 24, 2010 at 12:39:41PM +0300, Michael S. Tsirkin wrote:
On Mon, May 24, 2010 at 12:35:21PM +0300, Gleb Natapov wrote:
On Thu, May 20, 2010 at 04:36:32PM +0300, Michael S. Tsirkin wrote:
diff --git a/src/virtio-ring.h b/src/virtio-ring.h index 3fb86fe..8b546f4 100644 --- a/src/virtio-ring.h +++ b/src/virtio-ring.h @@ -9,8 +9,9 @@
#define virt_to_phys(v) (unsigned long)(v) #define phys_to_virt(p) (void*)(p) -#define wmb() barrier() -#define mb() barrier() +/* Compiler barrier is enough as an x86 CPU does not reorder reads or writes */ +#define smp_rmb() barrier() +#define smp_wmb() barrier()
I thought you were going to use real memory barriers (although we concluded compiler barrier should be enough, but it's better to be safe then sorry).
-- Gleb.
On x86, ops of the same kind are ordered by CPU, so all we need to do is
- avoid depending on order between reads and writes
- prevent the compiler from reordering memory accesses
So on Linux smp_rmb() and smp_wmb() are just compiler barriers too? (look like they are).
On x86, yes.
Of course x86. We are talking about x86 BIOS code here ;)
In that case lets use barrier() directly no need to obfuscate the code.
I think it's better to use smp_rmb/smp_wmb because this code tends to get copied around, it might end up on non-x86 platforms. For these, barrier() would be wrong.
So they will have a bug. It will teach them to not blindly copy code.
Another reason is that it makes it easier to audit code comparing it with linux virtio guest.
If you know that on x86 smp_(w|r)mb() are just barrier() it's still easy. To be honest though this is the reason I left virt_to_phys()/phys_to_virt(), so Kevin this is your call.
-- Gleb.
On Mon, May 24, 2010 at 01:12:35PM +0300, Gleb Natapov wrote:
On Mon, May 24, 2010 at 01:01:34PM +0300, Michael S. Tsirkin wrote:
Another reason is that it makes it easier to audit code comparing it with linux virtio guest.
If you know that on x86 smp_(w|r)mb() are just barrier() it's still easy. To be honest though this is the reason I left virt_to_phys()/phys_to_virt(), so Kevin this is your call.
I don't object to these wrappers if it's to help keep code in synch.
-Kevin
On Thu, May 20, 2010 at 04:36:32PM +0300, Michael S. Tsirkin wrote:
cleanup memory barrier usage bringing it in sync with what linux guest does.
FYI - I applied both of your patches.
-Kevin
-
Gleb Natapov -
Kevin O'Connor -
Michael S. Tsirkin