Re: [PATCH 2/2] tools/virtio: make barriers stronger.

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

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Wed, Mar 06, 2013 at 03:54:42PM +1100, Rusty Russell wrote:
>> In the coming vringh_test, we share an mmap with another userspace process
>> for testing.  This requires real barriers.
>> 
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>> 
>> diff --git a/tools/virtio/asm/barrier.h b/tools/virtio/asm/barrier.h
>> index aff61e1..7a63693 100644
>> --- a/tools/virtio/asm/barrier.h
>> +++ b/tools/virtio/asm/barrier.h
>> @@ -3,8 +3,8 @@
>>  #define mb() __sync_synchronize()
>>  
>>  #define smp_mb()	mb()
>> -# define smp_rmb()	barrier()
>> -# define smp_wmb()	barrier()
>> +# define smp_rmb()	mb()
>> +# define smp_wmb()	mb()
>>  /* Weak barriers should be used. If not - it's a bug */
>>  # define rmb()	abort()
>>  # define wmb()	abort()
>
> Hmm this seems wrong on x86 which has strong order in hardware.
> It should not matter whether the other side is a userspace
> process or a kernel thread.

Actually, this code is completely generic now, though overkill for x86 smp_wmb():

Interestingly, when I try defining them, 32-bit x86 slows down (it seems
that gcc is using "lock orl $0x0,(%esp)" for __sync_synchronize()).:

On my 32-bit laptop: Intel(R) Core(TM) i5 CPU       M 560  @ 2.67GHz

Before:
	Wall time:1.660000-1.790000(1.682500)
After:
	Wall time:1.930000-3.620000(1.960625)

64 bit it's a win:
On 2.6.32-358.el6.x86_64, gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-3), Intel(R) Xeon(R) CPU           E5620  @ 2.40GHz:

Before:
	real	0m2.937000-8.339000(3.123979)s
	user	0m2.811000-8.233000(2.954813)s
	sys	0m0.052000-0.154000(0.089396)s
After:
	real	0m2.559000-2.936000(2.726729)s
	user	0m2.397000-2.651000(2.506396)s
	sys	0m0.055000-0.152000(0.090667)s

Raw performance doesn't really matter, of course, but it's tempting to
use these asm barriers for __x86_64__, and use __sync_synchronize()
everywhere for everyone else.

Thoughts?
Rusty.

diff --git a/tools/virtio/asm/barrier.h b/tools/virtio/asm/barrier.h
index 7a63693..8de720a 100644
--- a/tools/virtio/asm/barrier.h
+++ b/tools/virtio/asm/barrier.h
@@ -1,11 +1,12 @@
 #if defined(__i386__) || defined(__x86_64__)
 #define barrier() asm volatile("" ::: "memory")
-#define mb() __sync_synchronize()
 
-#define smp_mb()	mb()
-# define smp_rmb()	mb()
-# define smp_wmb()	mb()
+#define smp_mb() 	asm volatile("mfence":::"memory")
+#define smp_rmb()	asm volatile("lfence":::"memory")
+#define smp_wmb()	asm volatile("sfence" ::: "memory")
+
 /* Weak barriers should be used. If not - it's a bug */
+# define mb()	abort()
 # define rmb()	abort()
 # define wmb()	abort()
 #else