linux-kernel - [tip:locking/core] locking/x86: Use LOCK ADD for smp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <tip-5374b516af79988163a16ee5afb508dfe39348b0@git.kernel.org>
Date:   Fri, 10 Nov 2017 01:40:37 -0800
From:   "tip-bot for Michael S. Tsirkin" <tipbot@...or.com>
To:     linux-tip-commits@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, mingo@...nel.org,
        torvalds@...ux-foundation.org, luto@...capital.net,
        peterz@...radead.org, tglx@...utronix.de, jpoimboe@...hat.com,
        dvlasenk@...hat.com, luto@...nel.org, akpm@...ux-foundation.org,
        bp@...en8.de, brgerst@...il.com, hpa@...or.com, mst@...hat.com,
        paulmck@...ux.vnet.ibm.com
Subject: [tip:locking/core] locking/x86: Use LOCK ADD for smp_mb() instead
 of MFENCE

Commit-ID:  5374b516af79988163a16ee5afb508dfe39348b0
Gitweb:     https://git.kernel.org/tip/5374b516af79988163a16ee5afb508dfe39348b0
Author:     Michael S. Tsirkin <mst@...hat.com>
AuthorDate: Fri, 27 Oct 2017 19:14:31 +0300
Committer:  Ingo Molnar <mingo@...nel.org>
CommitDate: Fri, 10 Nov 2017 08:03:21 +0100

locking/x86: Use LOCK ADD for smp_mb() instead of MFENCE

MFENCE appears to be way slower than a locked instruction - let's use
LOCK ADD unconditionally, as we always did on old 32-bit.

Performance testing results:

  perf stat -r 10 -- ./virtio_ring_0_9 --sleep --host-affinity 0 --guest-affinity 0
  Before:
         0.922565990 seconds time elapsed                                          ( +-  1.15% )
  After:
         0.578667024 seconds time elapsed                                          ( +-  1.21% )

i.e. about ~60% faster.

Just poking at SP would be the most natural, but if we then read the
value from SP, we get a false dependency which will slow us down.

This was noted in this article:

  http://shipilev.net/blog/2014/on-the-fence-with-dependencies/

And is easy to reproduce by sticking a barrier in a small non-inline
function.

So let's use a negative offset - which avoids this problem since we
build with the red zone disabled.

For userspace, use an address just below the redzone.

The one difference between LOCK ADD and MFENCE is that LOCK ADD does
not affect CLFLUSH, previous patches converted all uses of CLFLUSH to
call mb(), such that changes to smp_mb() won't affect it.

Update mb/rmb/wmb() on 32-bit to use the negative offset, too, for
consistency.

As a follow-up, it might be worth considering switching users
of CLFLUSH to another API (e.g. clflush_mb()?) - we will
then be able to convert mb() to smp_mb() again.

Also arguably, GCC should switch to use LOCK ADD for __sync_synchronize().
This might be worth pursuing separately.

Suggested-by: Andy Lutomirski <luto@...capital.net>
Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>
Cc: Andy Lutomirski <luto@...nel.org>
Cc: Borislav Petkov <bp@...en8.de>
Cc: Brian Gerst <brgerst@...il.com>
Cc: Denys Vlasenko <dvlasenk@...hat.com>
Cc: H. Peter Anvin <hpa@...or.com>
Cc: Josh Poimboeuf <jpoimboe@...hat.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Thomas Gleixner <tglx@...utronix.de>
Cc: qemu-devel@...gnu.org
Cc: virtualization@...ts.linux-foundation.org
Link: http://lkml.kernel.org/r/1509118355-4890-1-git-send-email-mst@redhat.com
Signed-off-by: Ingo Molnar <mingo@...nel.org>
---
 arch/x86/include/asm/barrier.h | 12 ++++++++----
 tools/virtio/ringtest/main.h   |  4 ++++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index 01727db..7fb3362 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -12,11 +12,11 @@
  */
 
 #ifdef CONFIG_X86_32
-#define mb() asm volatile(ALTERNATIVE("lock; addl $0,0(%%esp)", "mfence", \
+#define mb() asm volatile(ALTERNATIVE("lock; addl $0,-4(%%esp)", "mfence", \
 				      X86_FEATURE_XMM2) ::: "memory", "cc")
-#define rmb() asm volatile(ALTERNATIVE("lock; addl $0,0(%%esp)", "lfence", \
+#define rmb() asm volatile(ALTERNATIVE("lock; addl $0,-4(%%esp)", "lfence", \
 				       X86_FEATURE_XMM2) ::: "memory", "cc")
-#define wmb() asm volatile(ALTERNATIVE("lock; addl $0,0(%%esp)", "sfence", \
+#define wmb() asm volatile(ALTERNATIVE("lock; addl $0,-4(%%esp)", "sfence", \
 				       X86_FEATURE_XMM2) ::: "memory", "cc")
 #else
 #define mb() 	asm volatile("mfence":::"memory")
@@ -31,7 +31,11 @@
 #endif
 #define dma_wmb()	barrier()
 
-#define __smp_mb()	mb()
+#ifdef CONFIG_X86_32
+#define __smp_mb()	asm volatile("lock; addl $0,-4(%%esp)" ::: "memory", "cc")
+#else
+#define __smp_mb()	asm volatile("lock; addl $0,-4(%%rsp)" ::: "memory", "cc")
+#endif
 #define __smp_rmb()	dma_rmb()
 #define __smp_wmb()	barrier()
 #define __smp_store_mb(var, value) do { (void)xchg(&var, value); } while (0)
diff --git a/tools/virtio/ringtest/main.h b/tools/virtio/ringtest/main.h
index 90b0133..5706e07 100644
--- a/tools/virtio/ringtest/main.h
+++ b/tools/virtio/ringtest/main.h
@@ -110,11 +110,15 @@ static inline void busy_wait(void)
 		barrier();
 } 
 
+#if defined(__x86_64__) || defined(__i386__)
+#define smp_mb()     asm volatile("lock; addl $0,-128(%%rsp)" ::: "memory", "cc")
+#else
 /*
  * Not using __ATOMIC_SEQ_CST since gcc docs say they are only synchronized
  * with other __ATOMIC_SEQ_CST calls.
  */
 #define smp_mb() __sync_synchronize()
+#endif
 
 /*
  * This abuses the atomic builtins for thread fences, and