[PATCHv2] virtio: use smp_XX barriers on SMP

[PATCHv2] virtio: use smp_XX barriers on SMP

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

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 <mst@redhat.com>
---

Changes from v1: fall back on mandatory barriers for UP

 drivers/virtio/virtio_ring.c |   30 ++++++++++++++++++++++++------
 1 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index fbd2ecd..6794662 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -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;
-- 
1.6.6.144.g5c3af

Re: [PATCHv2] virtio: use smp_XX barriers on SMP

[Rusty Russell]

On Thu, 28 Jan 2010 09:12:23 am Michael S. Tsirkin wrote:
> 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.

Re: [PATCHv2] virtio: use smp_XX barriers on SMP

[Michael S. Tsirkin]

On Thu, Jan 28, 2010 at 10:01:09AM +1030, Rusty Russell wrote:
> On Thu, 28 Jan 2010 09:12:23 am Michael S. Tsirkin wrote:
> > 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?

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

> But I'm not sure what came out of the discussion: Jeremy?
> 
> Cheers,
> Rusty.

Re: [PATCHv2] virtio: use smp_XX barriers on SMP

[Rusty Russell]

On Fri, 29 Jan 2010 12:07:08 am Michael S. Tsirkin wrote:
> On Thu, Jan 28, 2010 at 10:01:09AM +1030, Rusty Russell wrote:
> > On Thu, 28 Jan 2010 09:12:23 am Michael S. Tsirkin wrote:
> > > 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.

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.

Re: [PATCHv2] virtio: use smp_XX barriers on SMP

[Michael S. Tsirkin]

On Fri, Jan 29, 2010 at 07:44:59AM +1030, Rusty Russell wrote:
> On Fri, 29 Jan 2010 12:07:08 am Michael S. Tsirkin wrote:
> > On Thu, Jan 28, 2010 at 10:01:09AM +1030, Rusty Russell wrote:
> > > On Thu, 28 Jan 2010 09:12:23 am Michael S. Tsirkin wrote:
> > > > 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.
> 
> 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.

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

-- 
MST

Re: [PATCHv2] virtio: use smp_XX barriers on SMP

[Michael S. Tsirkin]

On Fri, Jan 29, 2010 at 07:44:59AM +1030, Rusty Russell wrote:
> On Fri, 29 Jan 2010 12:07:08 am Michael S. Tsirkin wrote:
> > On Thu, Jan 28, 2010 at 10:01:09AM +1030, Rusty Russell wrote:
> > > On Thu, 28 Jan 2010 09:12:23 am Michael S. Tsirkin wrote:
> > > > 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.
> 
> 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.

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

Re: [PATCHv2] virtio: use smp_XX barriers on SMP

[Rusty Russell]

On Fri, 29 Jan 2010 09:57:18 pm Michael S. Tsirkin wrote:
> On Fri, Jan 29, 2010 at 07:44:59AM +1030, Rusty Russell wrote:
> > 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 <mst@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/virtio/virtio_ring.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -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 {							\