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

[Rusty Russell <rusty@rustcorp.com.au>]

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.