Skip to contentSkip to conversation

virtio: use smp_XX barriers on SMP

PATCH In review Michael S. Tsirkin 2 participants 7 messages Jan 27, 2010 – Jan 30, 2010 virtio core

lore
Share & tools
lore

Michael S. Tsirkin

author maintainer
#0 lore raw

PATCH v2 1/1 virtio: use smp_XX barriers on SMP

virtio is communicating with a virtual "device" that actually runs on another host processor. Thus SMP barriers can be used to control memory access ordering.

Where possible, we should use SMP barriers which are more lightweight than mandatory barriers, because mandatory barriers also control MMIO effects on accesses through relaxed memory I/O windows (which virtio does not use) (compare specifically smp_rmb and rmb on x86_64).

We can't just use smp_mb and friends though, because
we must force memory ordering even if guest is UP since host could be running on another CPU, but SMP barriers are defined to barrier() in that configuration. So, for UP fall back to mandatory barriers instead.

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

---

Changes from v1: fall back on mandatory barriers for UP

 drivers/virtio/virtio_ring.c |   30 ++++++++++++++++++++++++------
 1 files changed, 24 insertions(+), 6 deletions(-)
drivers/virtio/virtio_ring.c +24 -6
index fbd2ecd..6794662 100644
@@ -21,6 +21,24 @@
 #include <linux/virtio_config.h>
 #include <linux/device.h>
 
+/* virtio guest is communicating with a virtual "device" that actually runs on
+ * a host processor.  Memory barriers are used to control SMP effects. */
+#ifdef CONFIG_SMP
+/* Where possible, use SMP barriers which are more lightweight than mandatory
+ * barriers, because mandatory barriers control MMIO effects on accesses
+ * through relaxed memory I/O windows (which virtio does not use). */
+#define virtio_mb() smp_mb()
+#define virtio_rmb() smp_rmb()
+#define virtio_wmb() smp_wmb()
+#else
+/* We must force memory ordering even if guest is UP since host could be
+ * running on another CPU, but SMP barriers are defined to barrier() in that
+ * configuration. So fall back to mandatory barriers instead. */
+#define virtio_mb() mb()
+#define virtio_rmb() rmb()
+#define virtio_wmb() wmb()
+#endif
+
 #ifdef DEBUG
 /* For development, we want to crash whenever the ring is screwed. */
 #define BAD_RING(_vq, fmt, args...)				\

@@ -36,10 +54,10 @@
 			panic("%s:in_use = %i\n",		\
 			      (_vq)->vq.name, (_vq)->in_use);	\
 		(_vq)->in_use = __LINE__;			\
-		mb();						\
+		virtio_mb();						\
 	} while (0)
 #define END_USE(_vq) \
-	do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; mb(); } while(0)
+	do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; virtio_mb(); } while(0)
 #else
 #define BAD_RING(_vq, fmt, args...)				\
 	do {							\

@@ -221,13 +239,13 @@ static void vring_kick(struct virtqueue *_vq)
 	START_USE(vq);
 	/* Descriptors and available array need to be set before we expose the
 	 * new available array entries. */
-	wmb();
+	virtio_wmb();
 
 	vq->vring.avail->idx += vq->num_added;
 	vq->num_added = 0;
 
 	/* Need to update avail index before checking if we should notify */
-	mb();
+	virtio_mb();
 
 	if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY))
 		/* Prod other side to tell it about changes. */

@@ -286,7 +304,7 @@ static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len)
 	}
 
 	/* Only get used array entries after they have been exposed by host. */
-	rmb();
+	virtio_rmb();
 
 	i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
 	*len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;

@@ -324,7 +342,7 @@ static bool vring_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;
-	mb();
+	virtio_mb();
 	if (unlikely(more_used(vq))) {
 		END_USE(vq);
 		return false;
Signature · 1 line
1.6.6.144.g5c3af
#1 lore raw
Quoted 8 lines · Michael S. Tsirkin

virtio is communicating with a virtual "device" that actually runs on another host processor. Thus SMP barriers can be used to control memory access ordering.

Where possible, we should use SMP barriers which are more lightweight than mandatory barriers, because mandatory barriers also control MMIO effects on accesses through relaxed memory I/O windows (which virtio does not use) (compare specifically smp_rmb and rmb on x86_64).

Xen had a similar issue, in that UP guests running on SMP hosts need to issue SMP barriers. Is this not also a requirement for virtio?

But I'm not sure what came out of the discussion: Jeremy?

Cheers,
Rusty.

author maintainer
#2 lore raw
Quoted 5 lines · Rusty Russell
Quoted 4 lines · Michael S. Tsirkin

virtio is communicating with a virtual "device" that actually runs on another host processor. Thus SMP barriers can be used to control memory access ordering.

Quoted 7 lines
Quoted 4 lines

Where possible, we should use SMP barriers which are more lightweight than mandatory barriers, because mandatory barriers also control MMIO effects on accesses through relaxed memory I/O windows (which virtio does not use) (compare specifically smp_rmb and rmb on x86_64).

Xen had a similar issue, in that UP guests running on SMP hosts need to issue SMP barriers. Is this not also a requirement for virtio?

Of course it is. That's why I have ifdef CONFIG_SMP and use mandatory barriers on UP.

Quoted 4 lines

But I'm not sure what came out of the discussion: Jeremy?

Cheers,
Rusty.

#3 lore raw
Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines · Rusty Russell
Quoted 4 lines · Michael S. Tsirkin

Where possible, we should use SMP barriers which are more lightweight than mandatory barriers, because mandatory barriers also control MMIO effects on accesses through relaxed memory I/O windows (which virtio does not use) (compare specifically smp_rmb and rmb on x86_64).

Quoted 5 lines
Quoted 2 lines

Xen had a similar issue, in that UP guests running on SMP hosts need to issue SMP barriers. Is this not also a requirement for virtio?

Of course it is. That's why I have ifdef CONFIG_SMP and use mandatory barriers on UP.

Sorry, this was an off-the-cuff comment. I was concerned that UP barriers might be insufficient on some archs. But I can't find any such archs when I actually look (at least, x86, s390 and powerpc).

I've applied your patch, minus the first two substitutions:

@@ -36,10 +54,10 @@

                        panic("%s:in_use = %i\n",               \
                              (_vq)->vq.name, (_vq)->in_use);   \
                (_vq)->in_use = __LINE__;                       \
-               mb();                                           \
+               virtio_mb();                                            \
        } while (0)

#define END_USE(_vq) \
- do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; mb(); } while(0)
+ do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; virtio_mb(); } while(0)
#else

 #define BAD_RING(_vq, fmt, args...)                            \
        do {                                                    \

These barriers are actually trying to make sure in_use is set in a timely manner (this is debug code). They're bogus AFAICT: if you don't have any other synchronization then you're in danger of nesting and you want to know. And barriers might change timing too much when DEBUG is defined.

Thanks!
Rusty.

author maintainer
#4 lore raw
Quoted 34 lines · Rusty Russell
Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines · Rusty Russell
Quoted 4 lines · Michael S. Tsirkin

Where possible, we should use SMP barriers which are more lightweight than mandatory barriers, because mandatory barriers also control MMIO effects on accesses through relaxed memory I/O windows (which virtio does not use) (compare specifically smp_rmb and rmb on x86_64).

Quoted 5 lines
Quoted 2 lines

Xen had a similar issue, in that UP guests running on SMP hosts need to issue SMP barriers. Is this not also a requirement for virtio?

Of course it is. That's why I have ifdef CONFIG_SMP and use mandatory barriers on UP.

Sorry, this was an off-the-cuff comment. I was concerned that UP barriers might be insufficient on some archs. But I can't find any such archs when I actually look (at least, x86, s390 and powerpc).

I've applied your patch, minus the first two substitutions:

drivers/virtio/virtio_ring.c +2 -2
@@ -36,10 +54,10 @@
                        panic("%s:in_use = %i\n",               \
                              (_vq)->vq.name, (_vq)->in_use);   \
                (_vq)->in_use = __LINE__;                       \
-               mb();                                           \
+               virtio_mb();                                            \
        } while (0)
 #define END_USE(_vq) \
-       do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; mb(); } while(0)
+       do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; virtio_mb(); } while(0)
 #else
 #define BAD_RING(_vq, fmt, args...)                            \
        do {                                                    \
Quoted 7 lines

These barriers are actually trying to make sure in_use is set in a timely manner (this is debug code). They're bogus AFAICT: if you don't have any other synchronization then you're in danger of nesting and you want to know. And barriers might change timing too much when DEBUG is defined.

Thanks!
Rusty.

So .. let's just drop these barriers, and replace in_use with an atomic?

Signature · 1 line
MST
#5 lore raw
Quoted 5 lines · Michael S. Tsirkin
Quoted 2 lines · Rusty Russell

These barriers are actually trying to make sure in_use is set in a timely manner (this is debug code). They're bogus AFAICT:

So .. let's just drop these barriers, and replace in_use with an atomic?

Nope, just deleted them. See below.

Thanks!
Rusty.

virtio: remove bogus barriers from DEBUG version of virtio_ring.c

With DEBUG defined, we add an ->in_use flag to detect if the caller invokes two virtio methods in parallel. The barriers attempt to ensure timely update of the ->in_use flag.

But they're voodoo: if we need these barriers it implies that the calling code doesn't have sufficient synchronization to ensure the code paths aren't invoked at the same time anyway, and we want to detect it.

Also, adding barriers changes timing, so turning on debug has more chance of hiding real problems.

Thanks to MST for drawing my attention to this code...

CC Michael S. Tsirkin <[email protected]>
Signed-off-by Rusty Russell <[email protected]>
---
 drivers/virtio/virtio_ring.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/virtio/virtio_ring.c +1 -2
@@ -36,10 +36,9 @@
 			panic("%s:in_use = %i\n",		\
 			      (_vq)->vq.name, (_vq)->in_use);	\
 		(_vq)->in_use = __LINE__;			\
-		mb();						\
 	} while (0)
 #define END_USE(_vq) \
-	do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; mb(); } while(0)
+	do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; } while(0)
 #else
 #define BAD_RING(_vq, fmt, args...)				\
 	do {							\
author maintainer
#6 lore raw
In reply to Rusty Russell
Quoted 11 lines · Rusty Russell
Quoted 10 lines · Michael S. Tsirkin
Quoted 8 lines · Rusty Russell
Quoted 4 lines · Michael S. Tsirkin

Where possible, we should use SMP barriers which are more lightweight than mandatory barriers, because mandatory barriers also control MMIO effects on accesses through relaxed memory I/O windows (which virtio does not use) (compare specifically smp_rmb and rmb on x86_64).

Xen had a similar issue, in that UP guests running on SMP hosts need to issue SMP barriers. Is this not also a requirement for virtio?

Quoted 6 lines
Quoted 2 lines

Of course it is. That's why I have ifdef CONFIG_SMP and use mandatory barriers on UP.

Sorry, this was an off-the-cuff comment. I was concerned that UP barriers might be insufficient on some archs. But I can't find any such archs when I actually look (at least, x86, s390 and powerpc).

I see. Yes, smp barriers seem in practice to be a subset of mandatory barriers. Maybe a documentation patch specifying this is in order.

Quoted 23 lines

I've applied your patch, minus the first two substitutions:

drivers/virtio/virtio_ring.c +2 -2
@@ -36,10 +54,10 @@
                        panic("%s:in_use = %i\n",               \
                              (_vq)->vq.name, (_vq)->in_use);   \
                (_vq)->in_use = __LINE__;                       \
-               mb();                                           \
+               virtio_mb();                                            \
        } while (0)
 #define END_USE(_vq) \
-       do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; mb(); } while(0)
+       do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; virtio_mb(); } while(0)
 #else
 #define BAD_RING(_vq, fmt, args...)                            \
        do {                                                    \

These barriers are actually trying to make sure in_use is set in a timely manner (this is debug code). They're bogus AFAICT: if you don't have any other synchronization then you're in danger of nesting and you want to know. And barriers might change timing too much when DEBUG is defined.

Thanks!
Rusty.