* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Michael S. Tsirkin @ 2016-01-12 13:57 UTC (permalink / raw)
To: Linus Torvalds
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
virtualization, H. Peter Anvin, Thomas Gleixner, Paul E. McKenney,
Ingo Molnar
In-Reply-To: <CA+55aFynbkeuUGs9s-q+fLY6MeRBA6MjEyWWbbe7A5AaqsAknw@mail.gmail.com>
On Mon, Nov 02, 2015 at 04:06:46PM -0800, Linus Torvalds wrote:
> On Mon, Nov 2, 2015 at 12:15 PM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> >
> > So I ran some experiments on an IvyBridge (2.8GHz) and the cost of XCHG is
> > constantly cheaper (by at least half the latency) than MFENCE. While there
> > was a decent amount of variation, this difference remained rather constant.
>
> Mind testing "lock addq $0,0(%rsp)" instead of mfence? That's what we
> use on old cpu's without one (ie 32-bit).
>
> I'm not actually convinced that mfence is necessarily a good idea. I
> could easily see it being microcode, for example.
>
> At least on my Haswell, the "lock addq" is pretty much exactly half
> the cost of "mfence".
>
> Linus
mfence was high on some traces I was seeing, so I got curious, too:
---->
main.c
---->
extern volatile int x;
volatile int x;
#ifdef __x86_64__
#define SP "rsp"
#else
#define SP "esp"
#endif
#ifdef lock
#define barrier() asm("lock; addl $0,0(%%" SP ")" ::: "memory")
#endif
#ifdef xchg
#define barrier() do { int p; int ret; asm volatile ("xchgl %0, %1;": "=r"(ret) : "m"(p): "memory", "cc"); } while (0)
#endif
#ifdef xchgrz
/* same as xchg but poking at gcc red zone */
#define barrier() do { int ret; asm volatile ("xchgl %0, -4(%%" SP ");": "=r"(ret) :: "memory", "cc"); } while (0)
#endif
#ifdef mfence
#define barrier() asm("mfence" ::: "memory")
#endif
#ifdef lfence
#define barrier() asm("lfence" ::: "memory")
#endif
#ifdef sfence
#define barrier() asm("sfence" ::: "memory")
#endif
int main(int argc, char **argv)
{
int i;
int j = 1234;
/*
* Test barrier in a loop. We also poke at a volatile variable in an
* attempt to make it a bit more realistic - this way there's something
* in the store-buffer.
*/
for (i = 0; i < 10000000; ++i) {
x = i - j;
barrier();
j = x;
}
return 0;
}
---->
Makefile:
---->
ALL = xchg xchgrz lock mfence lfence sfence
CC = gcc
CFLAGS += -Wall -O2 -ggdb
PERF = perf stat -r 10 --log-fd 1 --
all: ${ALL}
clean:
rm -f ${ALL}
run: all
for file in ${ALL}; do echo ${PERF} ./$$file ; ${PERF} ./$$file; done
.PHONY: all clean run
${ALL}: main.c
${CC} ${CFLAGS} -D$@ -o $@ main.c
----->
Is this a good way to test it?
E.g. on my laptop I get:
perf stat -r 10 --log-fd 1 -- ./xchg
Performance counter stats for './xchg' (10 runs):
53.236967 task-clock # 0.992 CPUs utilized ( +- 0.09% )
10 context-switches # 0.180 K/sec ( +- 1.70% )
0 CPU-migrations # 0.000 K/sec
37 page-faults # 0.691 K/sec ( +- 1.13% )
190,997,612 cycles # 3.588 GHz ( +- 0.04% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
80,654,850 instructions # 0.42 insns per cycle ( +- 0.01% )
10,122,372 branches # 190.138 M/sec ( +- 0.01% )
4,514 branch-misses # 0.04% of all branches ( +- 3.37% )
0.053642809 seconds time elapsed ( +- 0.12% )
perf stat -r 10 --log-fd 1 -- ./xchgrz
Performance counter stats for './xchgrz' (10 runs):
53.189533 task-clock # 0.997 CPUs utilized ( +- 0.22% )
0 context-switches # 0.000 K/sec
0 CPU-migrations # 0.000 K/sec
37 page-faults # 0.694 K/sec ( +- 0.75% )
190,785,621 cycles # 3.587 GHz ( +- 0.03% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
80,602,086 instructions # 0.42 insns per cycle ( +- 0.00% )
10,112,154 branches # 190.115 M/sec ( +- 0.01% )
3,743 branch-misses # 0.04% of all branches ( +- 4.02% )
0.053343693 seconds time elapsed ( +- 0.23% )
perf stat -r 10 --log-fd 1 -- ./lock
Performance counter stats for './lock' (10 runs):
53.096434 task-clock # 0.997 CPUs utilized ( +- 0.16% )
0 context-switches # 0.002 K/sec ( +-100.00% )
0 CPU-migrations # 0.000 K/sec
37 page-faults # 0.693 K/sec ( +- 0.98% )
190,796,621 cycles # 3.593 GHz ( +- 0.02% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
80,601,376 instructions # 0.42 insns per cycle ( +- 0.01% )
10,112,074 branches # 190.447 M/sec ( +- 0.01% )
3,475 branch-misses # 0.03% of all branches ( +- 1.33% )
0.053252678 seconds time elapsed ( +- 0.16% )
perf stat -r 10 --log-fd 1 -- ./mfence
Performance counter stats for './mfence' (10 runs):
126.376473 task-clock # 0.999 CPUs utilized ( +- 0.21% )
0 context-switches # 0.002 K/sec ( +- 66.67% )
0 CPU-migrations # 0.000 K/sec
36 page-faults # 0.289 K/sec ( +- 0.84% )
456,147,770 cycles # 3.609 GHz ( +- 0.01% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
80,892,416 instructions # 0.18 insns per cycle ( +- 0.00% )
10,163,220 branches # 80.420 M/sec ( +- 0.01% )
4,653 branch-misses # 0.05% of all branches ( +- 1.27% )
0.126539273 seconds time elapsed ( +- 0.21% )
perf stat -r 10 --log-fd 1 -- ./lfence
Performance counter stats for './lfence' (10 runs):
47.617861 task-clock # 0.997 CPUs utilized ( +- 0.06% )
0 context-switches # 0.002 K/sec ( +-100.00% )
0 CPU-migrations # 0.000 K/sec
36 page-faults # 0.764 K/sec ( +- 0.45% )
170,767,856 cycles # 3.586 GHz ( +- 0.03% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
80,581,607 instructions # 0.47 insns per cycle ( +- 0.00% )
10,108,508 branches # 212.284 M/sec ( +- 0.00% )
3,320 branch-misses # 0.03% of all branches ( +- 1.12% )
0.047768505 seconds time elapsed ( +- 0.07% )
perf stat -r 10 --log-fd 1 -- ./sfence
Performance counter stats for './sfence' (10 runs):
20.156676 task-clock # 0.988 CPUs utilized ( +- 0.45% )
3 context-switches # 0.159 K/sec ( +- 12.15% )
0 CPU-migrations # 0.000 K/sec
36 page-faults # 0.002 M/sec ( +- 0.87% )
72,212,225 cycles # 3.583 GHz ( +- 0.33% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
80,479,149 instructions # 1.11 insns per cycle ( +- 0.00% )
10,090,785 branches # 500.618 M/sec ( +- 0.01% )
3,626 branch-misses # 0.04% of all branches ( +- 3.59% )
0.020411208 seconds time elapsed ( +- 0.52% )
So mfence is more expensive than locked instructions/xchg, but sfence/lfence
are slightly faster, and xchg and locked instructions are very close if
not the same.
I poked at some 10 intel and AMD machines and the numbers are different
but the results seem more or less consistent with this.
From size point of view xchg is longer and xchgrz pokes at the red zone
which seems unnecessarily hacky, so good old lock+addl is probably the
best.
There isn't any extra magic behind mfence, is there?
E.g. I think lock orders accesses to WC memory as well,
so apparently mb() can be redefined unconditionally, without
looking at XMM2:
--->
x86: drop mfence in favor of lock+addl
mfence appears to be way slower than a locked instruction - let's use
lock+add unconditionally, same as we always did on old 32-bit.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
I'll play with this some more before posting this as a
non-stand alone patch. Is there a macro-benchmark where mb
is prominent?
diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index a584e1c..f0d36e2 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -15,15 +15,15 @@
* Some non-Intel clones support out of order store. wmb() ceases to be a
* nop for these.
*/
-#define mb() alternative("lock; addl $0,0(%%esp)", "mfence", X86_FEATURE_XMM2)
+#define mb() asm volatile("lock; addl $0,0(%%esp)":::"memory")
#define rmb() alternative("lock; addl $0,0(%%esp)", "lfence", X86_FEATURE_XMM2)
#define wmb() alternative("lock; addl $0,0(%%esp)", "sfence", X86_FEATURE_XMM)
#else
+#define mb() asm volatile("lock; addl $0,0(%%rsp)":::"memory")
#define rmb() asm volatile("lfence":::"memory")
#define wmb() asm volatile("sfence" ::: "memory")
#endif
#ifdef CONFIG_X86_PPRO_FENCE
#define dma_rmb() rmb()
#else
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply related
* Re: [PATCH v3 13/41] x86: reuse asm-generic/barrier.h
From: Thomas Gleixner @ 2016-01-12 14:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Ingo Molnar,
xen-devel, Ingo Molnar, Borislav Petkov, linux-xtensa,
user-mode-linux-devel, Stefano Stabellini, adi-buildroot-devel,
Andy Lutomirski, linux-metag, linux-arm-kernel, Andrew Cooper,
linux-kernel, Joe Perches
In-Reply-To: <1452426622-4471-14-git-send-email-mst@redhat.com>
On Sun, 10 Jan 2016, Michael S. Tsirkin wrote:
> As on most architectures, on x86 read_barrier_depends and
> smp_read_barrier_depends are empty. Drop the local definitions and pull
> the generic ones from asm-generic/barrier.h instead: they are identical.
>
> This is in preparation to refactoring this code area.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
^ permalink raw reply
* Re: [PATCH v3 27/41] x86: define __smp_xxx
From: Thomas Gleixner @ 2016-01-12 14:11 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, virtualization,
H. Peter Anvin, sparclinux, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, x86, Ingo Molnar,
xen-devel, Ingo Molnar, Borislav Petkov, linux-xtensa,
user-mode-linux-devel, Stefano Stabellini, adi-buildroot-devel,
Andy Lutomirski, linux-metag, linux-arm-kernel, Andrew Cooper,
linux-kernel, Joe Perches
In-Reply-To: <1452426622-4471-28-git-send-email-mst@redhat.com>
On Sun, 10 Jan 2016, Michael S. Tsirkin wrote:
> This defines __smp_xxx barriers for x86,
> for use by virtualization.
>
> smp_xxx barriers are removed as they are
> defined correctly by asm-generic/barriers.h
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
^ permalink raw reply
* Re: [PATCH v3 01/41] lcoking/barriers, arch: Use smp barriers in smp_store_release()
From: Paul E. McKenney @ 2016-01-12 16:28 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra,
Benjamin Herrenschmidt, Heiko Carstens, virtualization,
Paul Mackerras, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Davidlohr Bueso, Russell King - ARM Linux,
Arnd Bergmann, Davidlohr Bueso, Michael Ellerman, x86,
Christian Borntraeger, Linus Torvalds, xen-devel, Ingo Molnar,
linux-xtensa, user-mode-linux-devel
In-Reply-To: <1452426622-4471-2-git-send-email-mst@redhat.com>
On Sun, Jan 10, 2016 at 04:16:32PM +0200, Michael S. Tsirkin wrote:
> From: Davidlohr Bueso <dave@stgolabs.net>
>
> With commit b92b8b35a2e ("locking/arch: Rename set_mb() to smp_store_mb()")
> it was made clear that the context of this call (and thus set_mb)
> is strictly for CPU ordering, as opposed to IO. As such all archs
> should use the smp variant of mb(), respecting the semantics and
> saving a mandatory barrier on UP.
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: <linux-arch@vger.kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: dave@stgolabs.net
> Link: http://lkml.kernel.org/r/1445975631-17047-3-git-send-email-dave@stgolabs.net
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
Aside from a need for s/lcoking/locking/ in the subject line:
Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
> arch/ia64/include/asm/barrier.h | 2 +-
> arch/powerpc/include/asm/barrier.h | 2 +-
> arch/s390/include/asm/barrier.h | 2 +-
> include/asm-generic/barrier.h | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/ia64/include/asm/barrier.h b/arch/ia64/include/asm/barrier.h
> index df896a1..209c4b8 100644
> --- a/arch/ia64/include/asm/barrier.h
> +++ b/arch/ia64/include/asm/barrier.h
> @@ -77,7 +77,7 @@ do { \
> ___p1; \
> })
>
> -#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while (0)
> +#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
>
> /*
> * The group barrier in front of the rsm & ssm are necessary to ensure
> diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
> index 0eca6ef..a7af5fb 100644
> --- a/arch/powerpc/include/asm/barrier.h
> +++ b/arch/powerpc/include/asm/barrier.h
> @@ -34,7 +34,7 @@
> #define rmb() __asm__ __volatile__ ("sync" : : : "memory")
> #define wmb() __asm__ __volatile__ ("sync" : : : "memory")
>
> -#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while (0)
> +#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
>
> #ifdef __SUBARCH_HAS_LWSYNC
> # define SMPWMB LWSYNC
> diff --git a/arch/s390/include/asm/barrier.h b/arch/s390/include/asm/barrier.h
> index d68e11e..7ffd0b1 100644
> --- a/arch/s390/include/asm/barrier.h
> +++ b/arch/s390/include/asm/barrier.h
> @@ -36,7 +36,7 @@
> #define smp_mb__before_atomic() smp_mb()
> #define smp_mb__after_atomic() smp_mb()
>
> -#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while (0)
> +#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
>
> #define smp_store_release(p, v) \
> do { \
> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index b42afad..0f45f93 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
> @@ -93,7 +93,7 @@
> #endif /* CONFIG_SMP */
>
> #ifndef smp_store_mb
> -#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while (0)
> +#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
> #endif
>
> #ifndef smp_mb__before_atomic
> --
> MST
>
^ permalink raw reply
* Re: [PATCH v3 05/41] powerpc: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-12 16:31 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra,
Benjamin Herrenschmidt, virtualization, Paul Mackerras,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Davidlohr Bueso, Russell King - ARM Linux, Arnd Bergmann,
Michael Ellerman, x86, xen-devel, Ingo Molnar, linux-xtensa,
user-mode-linux-devel, Stefano Stabellini, adi-buildroot-devel,
Thomas Gleixner, linux-metag
In-Reply-To: <1452426622-4471-6-git-send-email-mst@redhat.com>
On Sun, Jan 10, 2016 at 04:17:09PM +0200, Michael S. Tsirkin wrote:
> On powerpc read_barrier_depends, smp_read_barrier_depends
> smp_store_mb(), smp_mb__before_atomic and smp_mb__after_atomic match the
> asm-generic variants exactly. Drop the local definitions and pull in
> asm-generic/barrier.h instead.
>
> This is in preparation to refactoring this code area.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
Looks sane to me.
Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/barrier.h | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
> index a7af5fb..980ad0c 100644
> --- a/arch/powerpc/include/asm/barrier.h
> +++ b/arch/powerpc/include/asm/barrier.h
> @@ -34,8 +34,6 @@
> #define rmb() __asm__ __volatile__ ("sync" : : : "memory")
> #define wmb() __asm__ __volatile__ ("sync" : : : "memory")
>
> -#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
> -
> #ifdef __SUBARCH_HAS_LWSYNC
> # define SMPWMB LWSYNC
> #else
> @@ -60,9 +58,6 @@
> #define smp_wmb() barrier()
> #endif /* CONFIG_SMP */
>
> -#define read_barrier_depends() do { } while (0)
> -#define smp_read_barrier_depends() do { } while (0)
> -
> /*
> * This is a barrier which prevents following instructions from being
> * started until the value of the argument x is known. For example, if
> @@ -87,8 +82,8 @@ do { \
> ___p1; \
> })
>
> -#define smp_mb__before_atomic() smp_mb()
> -#define smp_mb__after_atomic() smp_mb()
> #define smp_mb__before_spinlock() smp_mb()
>
> +#include <asm-generic/barrier.h>
> +
> #endif /* _ASM_POWERPC_BARRIER_H */
> --
> MST
>
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Linus Torvalds @ 2016-01-12 17:20 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
virtualization, H. Peter Anvin, Thomas Gleixner, Paul E. McKenney,
Ingo Molnar
In-Reply-To: <20160112150032-mutt-send-email-mst@redhat.com>
On Tue, Jan 12, 2016 at 5:57 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> #ifdef xchgrz
> /* same as xchg but poking at gcc red zone */
> #define barrier() do { int ret; asm volatile ("xchgl %0, -4(%%" SP ");": "=r"(ret) :: "memory", "cc"); } while (0)
> #endif
That's not safe in general. gcc might be using its redzone, so doing
xchg into it is unsafe.
But..
> Is this a good way to test it?
.. it's fine for some basic testing. It doesn't show any subtle
interactions (ie some operations may have different dynamic behavior
when the write buffers are busy etc), but as a baseline for "how fast
can things go" the stupid raw loop is fine. And while the xchg into
the redzoen wouldn't be acceptable as a real implementation, for
timing testing it's likely fine (ie you aren't hitting the problem it
can cause).
> So mfence is more expensive than locked instructions/xchg, but sfence/lfence
> are slightly faster, and xchg and locked instructions are very close if
> not the same.
Note that we never actually *use* lfence/sfence. They are pointless
instructions when looking at CPU memory ordering, because for pure CPU
memory ordering stores and loads are already ordered.
The only reason to use lfence/sfence is after you've used nontemporal
stores for IO. That's very very rare in the kernel. So I wouldn't
worry about those.
But yes, it does sound like mfence is just a bad idea too.
> There isn't any extra magic behind mfence, is there?
No.
I think the only issue is that there has never been any real reason
for CPU designers to try to make mfence go particularly fast. Nobody
uses it, again with the exception of some odd loops that use
nontemporal stores, and for those the cost tends to always be about
the nontemporal accesses themselves (often to things like GPU memory
over PCIe), and the mfence cost of a few extra cycles is negligible.
The reason "lock ; add $0" has generally been the fastest we've found
is simply that locked ops have been important for CPU designers.
So I think the patch is fine, and we should likely drop the use of mfence..
Linus
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Michael S. Tsirkin @ 2016-01-12 17:45 UTC (permalink / raw)
To: Linus Torvalds
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
virtualization, H. Peter Anvin, Thomas Gleixner, Paul E. McKenney,
Ingo Molnar
In-Reply-To: <CA+55aFwqgUQYVbVXLw1=LL6Gs=kXqhkx0tUZOdXnWbqCMdWfXg@mail.gmail.com>
On Tue, Jan 12, 2016 at 09:20:06AM -0800, Linus Torvalds wrote:
> On Tue, Jan 12, 2016 at 5:57 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > #ifdef xchgrz
> > /* same as xchg but poking at gcc red zone */
> > #define barrier() do { int ret; asm volatile ("xchgl %0, -4(%%" SP ");": "=r"(ret) :: "memory", "cc"); } while (0)
> > #endif
>
> That's not safe in general. gcc might be using its redzone, so doing
> xchg into it is unsafe.
>
> But..
>
> > Is this a good way to test it?
>
> .. it's fine for some basic testing. It doesn't show any subtle
> interactions (ie some operations may have different dynamic behavior
> when the write buffers are busy etc), but as a baseline for "how fast
> can things go" the stupid raw loop is fine. And while the xchg into
> the redzoen wouldn't be acceptable as a real implementation, for
> timing testing it's likely fine (ie you aren't hitting the problem it
> can cause).
>
> > So mfence is more expensive than locked instructions/xchg, but sfence/lfence
> > are slightly faster, and xchg and locked instructions are very close if
> > not the same.
>
> Note that we never actually *use* lfence/sfence. They are pointless
> instructions when looking at CPU memory ordering, because for pure CPU
> memory ordering stores and loads are already ordered.
>
> The only reason to use lfence/sfence is after you've used nontemporal
> stores for IO.
By the way, the comment in barrier.h says:
/*
* Some non-Intel clones support out of order store. wmb() ceases to be
* a nop for these.
*/
and while the 1st sentence may well be true, if you have
an SMP system with out of order stores, making wmb
not a nop will not help.
Additionally as you point out, wmb is not a nop even
for regular intel CPUs because of these weird use-cases.
Drop this comment?
> That's very very rare in the kernel. So I wouldn't
> worry about those.
Right - I'll leave these alone, whoever wants to optimize this path will
have to do the necessary research.
> But yes, it does sound like mfence is just a bad idea too.
>
> > There isn't any extra magic behind mfence, is there?
>
> No.
>
> I think the only issue is that there has never been any real reason
> for CPU designers to try to make mfence go particularly fast. Nobody
> uses it, again with the exception of some odd loops that use
> nontemporal stores, and for those the cost tends to always be about
> the nontemporal accesses themselves (often to things like GPU memory
> over PCIe), and the mfence cost of a few extra cycles is negligible.
>
> The reason "lock ; add $0" has generally been the fastest we've found
> is simply that locked ops have been important for CPU designers.
>
> So I think the patch is fine, and we should likely drop the use of mfence..
>
> Linus
OK so should I repost after a bit more testing? I don't believe this
will affect the kernel build benchmark, but I'll try :)
--
MST
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Linus Torvalds @ 2016-01-12 18:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
virtualization, H. Peter Anvin, Thomas Gleixner, Paul E. McKenney,
Ingo Molnar
In-Reply-To: <20160112193027-mutt-send-email-mst@redhat.com>
On Tue, Jan 12, 2016 at 9:45 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> By the way, the comment in barrier.h says:
>
> /*
> * Some non-Intel clones support out of order store. wmb() ceases to be
> * a nop for these.
> */
>
> and while the 1st sentence may well be true, if you have
> an SMP system with out of order stores, making wmb
> not a nop will not help.
>
> Additionally as you point out, wmb is not a nop even
> for regular intel CPUs because of these weird use-cases.
>
> Drop this comment?
We should drop it, yes. We dropped support for CONFIG_X86_OOSTORE
almost two years ago. See commit 09df7c4c8097 ("x86: Remove
CONFIG_X86_OOSTORE") and it was questionable for a long time even
before that (perhaps ever).
So the comment is stale.
We *do* still use the non-nop rmb/wmb for IO barriers, but even that
is generally questionable. See our "copy_user_64.S" for an actual use
of "movnt" followed by sfence. There's a couple of other cases too. So
that's all correct, but the point is that when we use "movnt" we don't
actually use "wmb()", we are doing assembly, and the assembly should
just use sfence directly.
So it's actually very questionable to ever make even the IO
wmb()/rmb() functions use lfence/sfence. They should never really need
it.
But at the same time, I _really_ don't think we care enough. I'd
rather leave those non-smp barrier cases alone as historial unless
somebody can point to a case where they care about the performance.
We also do have the whole PPRO_FENCE thing, which we can hopefully get
rid of at some point too.
Linus
^ permalink raw reply
* Re: [PATCH v3 01/41] lcoking/barriers, arch: Use smp barriers in smp_store_release()
From: Michael S. Tsirkin @ 2016-01-12 18:40 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra,
Benjamin Herrenschmidt, Heiko Carstens, virtualization,
Paul Mackerras, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Davidlohr Bueso, Russell King - ARM Linux,
Arnd Bergmann, Davidlohr Bueso, Michael Ellerman, x86,
Christian Borntraeger, Linus Torvalds, xen-devel, Ingo Molnar,
linux-xtensa, user-mode-linux-devel
In-Reply-To: <20160112162844.GD3818@linux.vnet.ibm.com>
On Tue, Jan 12, 2016 at 08:28:44AM -0800, Paul E. McKenney wrote:
> On Sun, Jan 10, 2016 at 04:16:32PM +0200, Michael S. Tsirkin wrote:
> > From: Davidlohr Bueso <dave@stgolabs.net>
> >
> > With commit b92b8b35a2e ("locking/arch: Rename set_mb() to smp_store_mb()")
> > it was made clear that the context of this call (and thus set_mb)
> > is strictly for CPU ordering, as opposed to IO. As such all archs
> > should use the smp variant of mb(), respecting the semantics and
> > saving a mandatory barrier on UP.
> >
> > Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Cc: <linux-arch@vger.kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: dave@stgolabs.net
> > Link: http://lkml.kernel.org/r/1445975631-17047-3-git-send-email-dave@stgolabs.net
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
>
> Aside from a need for s/lcoking/locking/ in the subject line:
>
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Thanks!
Though Ingo already put this in tip tree like this,
and I need a copy in my tree to avoid breaking bisect,
so I will probably keep it exactly the same to avoid confusion.
> > ---
> > arch/ia64/include/asm/barrier.h | 2 +-
> > arch/powerpc/include/asm/barrier.h | 2 +-
> > arch/s390/include/asm/barrier.h | 2 +-
> > include/asm-generic/barrier.h | 2 +-
> > 4 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/ia64/include/asm/barrier.h b/arch/ia64/include/asm/barrier.h
> > index df896a1..209c4b8 100644
> > --- a/arch/ia64/include/asm/barrier.h
> > +++ b/arch/ia64/include/asm/barrier.h
> > @@ -77,7 +77,7 @@ do { \
> > ___p1; \
> > })
> >
> > -#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while (0)
> > +#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
> >
> > /*
> > * The group barrier in front of the rsm & ssm are necessary to ensure
> > diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
> > index 0eca6ef..a7af5fb 100644
> > --- a/arch/powerpc/include/asm/barrier.h
> > +++ b/arch/powerpc/include/asm/barrier.h
> > @@ -34,7 +34,7 @@
> > #define rmb() __asm__ __volatile__ ("sync" : : : "memory")
> > #define wmb() __asm__ __volatile__ ("sync" : : : "memory")
> >
> > -#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while (0)
> > +#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
> >
> > #ifdef __SUBARCH_HAS_LWSYNC
> > # define SMPWMB LWSYNC
> > diff --git a/arch/s390/include/asm/barrier.h b/arch/s390/include/asm/barrier.h
> > index d68e11e..7ffd0b1 100644
> > --- a/arch/s390/include/asm/barrier.h
> > +++ b/arch/s390/include/asm/barrier.h
> > @@ -36,7 +36,7 @@
> > #define smp_mb__before_atomic() smp_mb()
> > #define smp_mb__after_atomic() smp_mb()
> >
> > -#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while (0)
> > +#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
> >
> > #define smp_store_release(p, v) \
> > do { \
> > diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> > index b42afad..0f45f93 100644
> > --- a/include/asm-generic/barrier.h
> > +++ b/include/asm-generic/barrier.h
> > @@ -93,7 +93,7 @@
> > #endif /* CONFIG_SMP */
> >
> > #ifndef smp_store_mb
> > -#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); mb(); } while (0)
> > +#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); smp_mb(); } while (0)
> > #endif
> >
> > #ifndef smp_mb__before_atomic
> > --
> > MST
> >
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Andy Lutomirski @ 2016-01-12 20:30 UTC (permalink / raw)
To: Linus Torvalds, Michael S. Tsirkin
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
virtualization, H. Peter Anvin, Thomas Gleixner, Paul E. McKenney,
Ingo Molnar
In-Reply-To: <CA+55aFwqgUQYVbVXLw1=LL6Gs=kXqhkx0tUZOdXnWbqCMdWfXg@mail.gmail.com>
On 01/12/2016 09:20 AM, Linus Torvalds wrote:
> On Tue, Jan 12, 2016 at 5:57 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> #ifdef xchgrz
>> /* same as xchg but poking at gcc red zone */
>> #define barrier() do { int ret; asm volatile ("xchgl %0, -4(%%" SP ");": "=r"(ret) :: "memory", "cc"); } while (0)
>> #endif
>
> That's not safe in general. gcc might be using its redzone, so doing
> xchg into it is unsafe.
>
> But..
>
>> Is this a good way to test it?
>
> .. it's fine for some basic testing. It doesn't show any subtle
> interactions (ie some operations may have different dynamic behavior
> when the write buffers are busy etc), but as a baseline for "how fast
> can things go" the stupid raw loop is fine. And while the xchg into
> the redzoen wouldn't be acceptable as a real implementation, for
> timing testing it's likely fine (ie you aren't hitting the problem it
> can cause).
I recall reading somewhere that lock addl $0, 32(%rsp) or so (maybe even
64) was better because it avoided stomping on very-likely-to-be-hot
write buffers.
--Andy
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-12 20:45 UTC (permalink / raw)
To: Will Deacon, Peter Zijlstra
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, Arnd Bergmann, linux-sh,
Michael Ellerman, x86, xen-devel, Ingo Molnar, Paul McKenney,
linux-xtensa, james.hogan, user-mode-linux-devel,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <20160112114111.GB15737@arm.com>
(I try to answer on multiple mails in one)
First of all, it seems like some generic notes should be given here:
1. Generic MIPS "SYNC" (aka "SYNC 0") instruction is a very heavy in
some CPUs. On that CPUs it basically kills pipelines in each CPU, can do
a special memory/IO bus transaction (similar to "fence") and hold a
system until all R/W is completed. It is like Big Kernel Lock but worse.
So, the move to SMP_* kind of barriers is needed to improve performance,
especially on newest CPUs with long pipelines.
2. MIPS Arch document may be misleading because words "ordering" and
"completion" means different from Linux, the SYNC instruction
description is written for HW engineers. I wrote that in a separate
patch of the same patchset -
http://patchwork.linux-mips.org/patch/10505/ "MIPS: R6: Use lightweight
SYNC instruction in smp_* memory barriers":
> This instructions were specifically designed to work for smp_*() sort of
> memory barriers in MIPS R2/R3/R5 and R6.
>
> Unfortunately, it's description is very cryptic and is done in HW engineering
> style which prevents use of it by SW.
3. I bother MIPS Arch team long time until I completely understood that
MIPS SYNC_WMB, SYNC_MB, SYNC_RMB, SYNC_RELEASE and SYNC_ACQUIRE do an
exactly that is required in Documentation/memory-barriers.txt
In Peter Zijlstra mail:
> 1) you do not make such things selectable; either the hardware needs
> them or it doesn't. If it does you_must_ use them, however unlikely.
It is selectable only for MIPS R2 but not MIPS R6. The reason is - most
of MIPS R2 CPUs have short pipeline and that SYNC is just waste of CPU
resource, especially taking into account that "lightweight syncs" are
converted to a heavy "SYNC 0" in many of that CPUs. However the latest
MIPS/Imagination CPU have a pipeline long enough to hit a problem -
absence of SYNC at LL/SC inside atomics, barriers etc.
> And reading the MIPS64 v6.04 instruction set manual, I think 0x11/0x12
> are_NOT_ transitive and therefore cannot be used to implement the
> smp_mb__{before,after} stuff.
>
> That is, in MIPS speak, those SYNC types are Ordering Barriers, not
> Completion Barriers.
Please see above, point 2.
> That is, currently all architectures -- with exception of PPC -- have
> RCsc locks, but using these non-transitive things will get you RCpc
> locks.
>
> So yes, MIPS can go RCpc for its locks and share the burden of pain with
> PPC, but that needs to be a very concious decision.
I don't understand that - I tried hard but I can't find any word like
"RCsc", "RCpc" in Documents/ directory. Web search goes nowhere, of course.
In Will Deacon mail:
> The issue I have with the SYNC description in the text above is that it
> describes the single CPU (program order) and the dual-CPU (confusingly
> named global order) cases, but then doesn't generalise any further. That
> means we can't sensibly reason about transitivity properties when a third
> agent is involved. For example, the WRC+sync+addr test:
>
>
> P0:
> Wx = 1
>
> P1:
> Rx == 1
> SYNC
> Wy = 1
>
> P2:
> Ry == 1
> <address dep>
> Rx = 0
>
>
> I can't find anything to forbid that, given the text. The main problem
> is having the SYNC on P1 affect the write by P0.
As I understand that test, the visibility of P0: W[x] = 1 is identical
to P1 and P2 here. If P1 got X before SYNC and write to Y after SYNC
then instruction source register dependency tracking in P2 prevents a
speculative load of X before P2 obtains Y from the same place as P0/P1
and calculate address of X. If some load of X in P2 happens before
address dependency calculation it's result is discarded.
Yes, you can't find that in MIPS SYNC instruction description, it is
more likely in CM (Coherence Manager) area. I just pointed our arch team
member responsible for documents and he will think how to explain that.
- Leonid.
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Linus Torvalds @ 2016-01-12 20:54 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
virtualization, Michael S. Tsirkin, H. Peter Anvin,
Thomas Gleixner, Paul E. McKenney, Ingo Molnar
In-Reply-To: <56956276.1090705@kernel.org>
On Tue, Jan 12, 2016 at 12:30 PM, Andy Lutomirski <luto@kernel.org> wrote:
>
> I recall reading somewhere that lock addl $0, 32(%rsp) or so (maybe even 64)
> was better because it avoided stomping on very-likely-to-be-hot write
> buffers.
I suspect it could go either way. You want a small constant (for the
isntruction size), but any small constant is likely to be within the
current stack frame anyway. I don't think 0(%rsp) is particularly
likely to have a spill on it right then and there, but who knows..
And 64(%rsp) is possibly going to be cold in the L1 cache, especially
if it's just after a deep function call. Which it might be. So it
might work the other way.
So my guess would be that you wouldn't be able to measure the
difference. It might be there, but probably too small to really see in
any noise.
But numbers talk, bullshit walks. It would be interesting to be proven wrong.
Linus
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Andy Lutomirski @ 2016-01-12 20:59 UTC (permalink / raw)
To: Linus Torvalds
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
virtualization, Michael S. Tsirkin, Andy Lutomirski,
H. Peter Anvin, Thomas Gleixner, Paul E. McKenney, Ingo Molnar
In-Reply-To: <CA+55aFxhoGB9dBArw5iy0C0RhJEVRLLPR42w--KLSn5WM2sVRQ@mail.gmail.com>
On Tue, Jan 12, 2016 at 12:54 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Jan 12, 2016 at 12:30 PM, Andy Lutomirski <luto@kernel.org> wrote:
>>
>> I recall reading somewhere that lock addl $0, 32(%rsp) or so (maybe even 64)
>> was better because it avoided stomping on very-likely-to-be-hot write
>> buffers.
>
> I suspect it could go either way. You want a small constant (for the
> isntruction size), but any small constant is likely to be within the
> current stack frame anyway. I don't think 0(%rsp) is particularly
> likely to have a spill on it right then and there, but who knows..
>
> And 64(%rsp) is possibly going to be cold in the L1 cache, especially
> if it's just after a deep function call. Which it might be. So it
> might work the other way.
>
> So my guess would be that you wouldn't be able to measure the
> difference. It might be there, but probably too small to really see in
> any noise.
>
> But numbers talk, bullshit walks. It would be interesting to be proven wrong.
Here's an article with numbers:
http://shipilev.net/blog/2014/on-the-fence-with-dependencies/
I think they're suggesting using a negative offset, which is safe as
long as it doesn't page fault, even though we have the redzone
disabled.
--Andy
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Linus Torvalds @ 2016-01-12 21:37 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
virtualization, Michael S. Tsirkin, Andy Lutomirski,
H. Peter Anvin, Thomas Gleixner, Paul E. McKenney, Ingo Molnar
In-Reply-To: <CALCETrWBxiAb8KHjBfb2rRhX3KrbLfc3bzhfQnyCdE3G4mnsSA@mail.gmail.com>
On Tue, Jan 12, 2016 at 12:59 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> Here's an article with numbers:
>
> http://shipilev.net/blog/2014/on-the-fence-with-dependencies/
Well, that's with the busy loop and one set of code generation. It
doesn't show the "oops, deeper stack isn't even in the cache any more
due to call chains" issue.
But yes:
> I think they're suggesting using a negative offset, which is safe as
> long as it doesn't page fault, even though we have the redzone
> disabled.
I think a negative offset might work very well. Partly exactly
*because* we have the redzone disabled: we know that inside the
kernel, we'll never have any live stack frame accesses under the stack
pointer, so "-4(%rsp)" sounds good to me. There should never be any
pending writes in the write buffer, because even if it *was* live, it
would have been read off first.
Yeah, it potentially does extend the stack cache footprint by another
4 bytes, but that sounds very benign.
So perhaps it might be worth trying to switch the "mfence" to "lock ;
addl $0,-4(%rsp)" in the kernel for x86-64, and remove the alternate
for x86-32.
I'd still want to see somebody try to benchmark it. I doubt it's
noticeable, but making changes because you think it might save a few
cycles without then even measuring it is just wrong.
Linus
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Peter Zijlstra @ 2016-01-12 21:40 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Will Deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <569565DA.2010903@imgtec.com>
On Tue, Jan 12, 2016 at 12:45:14PM -0800, Leonid Yegoshin wrote:
> (I try to answer on multiple mails in one)
>
> First of all, it seems like some generic notes should be given here:
>
> 1. Generic MIPS "SYNC" (aka "SYNC 0") instruction is a very heavy in some
> CPUs. On that CPUs it basically kills pipelines in each CPU, can do a
> special memory/IO bus transaction (similar to "fence") and hold a system
> until all R/W is completed. It is like Big Kernel Lock but worse. So, the
> move to SMP_* kind of barriers is needed to improve performance, especially
> on newest CPUs with long pipelines.
The MIPS SYNC isn't any worse than the PPC SYNC, x86 MFENCE or arm DSB
SY, yes they're heavy, so what.
> 2. MIPS Arch document may be misleading because words "ordering" and
> "completion" means different from Linux, the SYNC instruction description is
> written for HW engineers. I wrote that in a separate patch of the same
> patchset - http://patchwork.linux-mips.org/patch/10505/ "MIPS: R6: Use
> lightweight SYNC instruction in smp_* memory barriers":
Did you actually say anything here?
> >This instructions were specifically designed to work for smp_*() sort of
> >memory barriers in MIPS R2/R3/R5 and R6.
> >
> >Unfortunately, it's description is very cryptic and is done in HW engineering
> >style which prevents use of it by SW.
>
> 3. I bother MIPS Arch team long time until I completely understood that MIPS
> SYNC_WMB, SYNC_MB, SYNC_RMB, SYNC_RELEASE and SYNC_ACQUIRE do an exactly
> that is required in Documentation/memory-barriers.txt
Ha! and you think that document covers all the really fun details?
In particular we're very much all 'confused' about the various notions
of transitivity and what barriers imply how much of it.
> In Peter Zijlstra mail:
>
> >1) you do not make such things selectable; either the hardware needs
> >them or it doesn't. If it does you_must_ use them, however unlikely.
> It is selectable only for MIPS R2 but not MIPS R6. The reason is - most of
> MIPS R2 CPUs have short pipeline and that SYNC is just waste of CPU
> resource, especially taking into account that "lightweight syncs" are
> converted to a heavy "SYNC 0" in many of that CPUs. However the latest
> MIPS/Imagination CPU have a pipeline long enough to hit a problem - absence
> of SYNC at LL/SC inside atomics, barriers etc.
What ?! Are you saying that because R2 has short pipelines its unlikely
to hit the reordering issues and we can omit barriers?
> >And reading the MIPS64 v6.04 instruction set manual, I think 0x11/0x12
> >are_NOT_ transitive and therefore cannot be used to implement the
> >smp_mb__{before,after} stuff.
> >
> >That is, in MIPS speak, those SYNC types are Ordering Barriers, not
> >Completion Barriers.
>
> Please see above, point 2.
That did not in fact enlighten things. Are they transitive/multi-copy
atomic or not?
(and here Will will go into great detail on the differences between the
two and make our collective brains explode :-)
> >That is, currently all architectures -- with exception of PPC -- have
> >RCsc locks, but using these non-transitive things will get you RCpc
> >locks.
> >
> >So yes, MIPS can go RCpc for its locks and share the burden of pain with
> >PPC, but that needs to be a very concious decision.
>
> I don't understand that - I tried hard but I can't find any word like
> "RCsc", "RCpc" in Documents/ directory. Web search goes nowhere, of course.
From: lkml.kernel.org/r/20150828153921.GF19282@twins.programming.kicks-ass.net
Yes, the difference between RCpc and RCsc is in the meaning of RELEASE +
ACQUIRE. With RCsc that implies a full memory barrier, with RCpc it does
not.
Currently PowerPC is the only arch that (can, and) does RCpc and gives a
weaker RELEASE + ACQUIRE. Only the CPU who did the ACQUIRE is guaranteed
to see the stores of the CPU which did the RELEASE in order.
As it stands, RCU is the only _known_ codebase where this matters, but
we did in fact write code for a fair number of years 'assuming' RELEASE
+ ACQUIRE was a full barrier, so who knows what else is out there.
RCsc - release consistency sequential consistency
RCpc - release consistency processor consistency
https://en.wikipedia.org/wiki/Processor_consistency
^ permalink raw reply
* [PATCH v2 0/3] x86: faster mb()+other barrier.h tweaks
From: Michael S. Tsirkin @ 2016-01-12 22:10 UTC (permalink / raw)
To: linux-kernel, Linus Torvalds
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, virtualization, H. Peter Anvin,
Thomas Gleixner, Paul E. McKenney, Ingo Molnar
mb() typically uses mfence on modern x86, but a micro-benchmark shows that it's
2 to 3 times slower than lock; addl $0,(%%e/rsp) that we use on older CPUs.
So let's use the locked variant everywhere - helps keep the code simple as
well.
While I was at it, I found some inconsistencies in comments in
arch/x86/include/asm/barrier.h
I hope I'm not splitting this up too much - the reason is I wanted to isolate
the code changes (that people might want to test for performance) from comment
changes approved by Linus, from (so far unreviewed) comment change I came up
with myself.
Lightly tested on my system.
Michael S. Tsirkin (3):
x86: drop mfence in favor of lock+addl
x86: drop a comment left over from X86_OOSTORE
x86: tweak the comment about use of wmb for IO
arch/x86/include/asm/barrier.h | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
--
MST
^ permalink raw reply
* [PATCH v2 1/3] x86: drop mfence in favor of lock+addl
From: Michael S. Tsirkin @ 2016-01-12 22:10 UTC (permalink / raw)
To: linux-kernel, Linus Torvalds
Cc: Davidlohr Bueso, Arnd Bergmann, Davidlohr Bueso, Peter Zijlstra,
Andrey Konovalov, the arch/x86 maintainers, virtualization,
Ingo Molnar, Borislav Petkov, Andy Lutomirski, H. Peter Anvin,
Thomas Gleixner, Paul E. McKenney, Ingo Molnar
In-Reply-To: <1452635935-5439-1-git-send-email-mst@redhat.com>
mfence appears to be way slower than a locked instruction - let's use
lock+add unconditionally, same as we always did on old 32-bit.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/x86/include/asm/barrier.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index a584e1c..7f99726 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -15,11 +15,12 @@
* Some non-Intel clones support out of order store. wmb() ceases to be a
* nop for these.
*/
-#define mb() alternative("lock; addl $0,0(%%esp)", "mfence", X86_FEATURE_XMM2)
+
+#define mb() asm volatile("lock; addl $0,0(%%esp)" ::: "memory")
#define rmb() alternative("lock; addl $0,0(%%esp)", "lfence", X86_FEATURE_XMM2)
#define wmb() alternative("lock; addl $0,0(%%esp)", "sfence", X86_FEATURE_XMM)
#else
-#define mb() asm volatile("mfence":::"memory")
+#define mb() asm volatile("lock; addl $0,0(%%rsp)" ::: "memory")
#define rmb() asm volatile("lfence":::"memory")
#define wmb() asm volatile("sfence" ::: "memory")
#endif
--
MST
^ permalink raw reply related
* [PATCH v2 2/3] x86: drop a comment left over from X86_OOSTORE
From: Michael S. Tsirkin @ 2016-01-12 22:10 UTC (permalink / raw)
To: linux-kernel, Linus Torvalds
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
Andrey Konovalov, the arch/x86 maintainers, virtualization,
Ingo Molnar, Borislav Petkov, Andy Lutomirski, H. Peter Anvin,
Thomas Gleixner, Paul E. McKenney, Ingo Molnar
In-Reply-To: <1452635935-5439-1-git-send-email-mst@redhat.com>
The comment about wmb being non-nop is a left over from before commit
09df7c4c8097 ("x86: Remove CONFIG_X86_OOSTORE").
It makes no sense now: if you have an SMP system with out of order
stores, making wmb not a nop will not help.
Additionally, wmb is not a nop even for regular intel CPUs because of
weird use-cases e.g. dealing with WC memory.
Drop this comment.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/x86/include/asm/barrier.h | 5 -----
1 file changed, 5 deletions(-)
diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index 7f99726..eb220b8 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -11,11 +11,6 @@
*/
#ifdef CONFIG_X86_32
-/*
- * Some non-Intel clones support out of order store. wmb() ceases to be a
- * nop for these.
- */
-
#define mb() asm volatile("lock; addl $0,0(%%esp)" ::: "memory")
#define rmb() alternative("lock; addl $0,0(%%esp)", "lfence", X86_FEATURE_XMM2)
#define wmb() alternative("lock; addl $0,0(%%esp)", "sfence", X86_FEATURE_XMM)
--
MST
^ permalink raw reply related
* [PATCH v2 3/3] x86: tweak the comment about use of wmb for IO
From: Michael S. Tsirkin @ 2016-01-12 22:10 UTC (permalink / raw)
To: linux-kernel, Linus Torvalds
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
Andrey Konovalov, the arch/x86 maintainers, virtualization,
Ingo Molnar, Borislav Petkov, Andy Lutomirski, H. Peter Anvin,
Thomas Gleixner, Paul E. McKenney, Ingo Molnar
In-Reply-To: <1452635935-5439-1-git-send-email-mst@redhat.com>
On x86, we *do* still use the non-nop rmb/wmb for IO barriers, but even
that is generally questionable.
Leave them around as historial unless somebody can point to a case where
they care about the performance, but tweak the comment so people
don't think they are strictly required in all cases.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/x86/include/asm/barrier.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index eb220b8..924cd44 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -6,7 +6,7 @@
/*
* Force strict CPU ordering.
- * And yes, this is required on UP too when we're talking
+ * And yes, this might be required on UP too when we're talking
* to devices.
*/
--
MST
^ permalink raw reply related
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Michael S. Tsirkin @ 2016-01-12 22:14 UTC (permalink / raw)
To: Linus Torvalds
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
Andy Lutomirski, Andy Lutomirski, H. Peter Anvin,
Paul E. McKenney, Thomas Gleixner, virtualization, Ingo Molnar
In-Reply-To: <CA+55aFyuR1YCZjC9++E4kpvRxgoM4sqzhNaS27EZPFh9CuKjYg@mail.gmail.com>
On Tue, Jan 12, 2016 at 01:37:38PM -0800, Linus Torvalds wrote:
> On Tue, Jan 12, 2016 at 12:59 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> >
> > Here's an article with numbers:
> >
> > http://shipilev.net/blog/2014/on-the-fence-with-dependencies/
>
> Well, that's with the busy loop and one set of code generation. It
> doesn't show the "oops, deeper stack isn't even in the cache any more
> due to call chains" issue.
>
> But yes:
>
> > I think they're suggesting using a negative offset, which is safe as
> > long as it doesn't page fault, even though we have the redzone
> > disabled.
>
> I think a negative offset might work very well. Partly exactly
> *because* we have the redzone disabled: we know that inside the
> kernel, we'll never have any live stack frame accesses under the stack
> pointer, so "-4(%rsp)" sounds good to me. There should never be any
> pending writes in the write buffer, because even if it *was* live, it
> would have been read off first.
>
> Yeah, it potentially does extend the stack cache footprint by another
> 4 bytes, but that sounds very benign.
>
> So perhaps it might be worth trying to switch the "mfence" to "lock ;
> addl $0,-4(%rsp)" in the kernel for x86-64, and remove the alternate
> for x86-32.
>
> I'd still want to see somebody try to benchmark it. I doubt it's
> noticeable, but making changes because you think it might save a few
> cycles without then even measuring it is just wrong.
>
> Linus
Oops, I posted v2 with just offset 0 before reading
the rest of this thread.
I did try with offset 0 and didn't measure any
change on any perf bench test, or on kernel build.
I wonder which benchmark stresses smp_mb the most.
I'll look into using a negative offset.
--
MST
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Michael S. Tsirkin @ 2016-01-12 22:21 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
virtualization, Andy Lutomirski, H. Peter Anvin, Thomas Gleixner,
Paul E. McKenney, Linus Torvalds, Ingo Molnar
In-Reply-To: <CALCETrWBxiAb8KHjBfb2rRhX3KrbLfc3bzhfQnyCdE3G4mnsSA@mail.gmail.com>
On Tue, Jan 12, 2016 at 12:59:58PM -0800, Andy Lutomirski wrote:
> On Tue, Jan 12, 2016 at 12:54 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Tue, Jan 12, 2016 at 12:30 PM, Andy Lutomirski <luto@kernel.org> wrote:
> >>
> >> I recall reading somewhere that lock addl $0, 32(%rsp) or so (maybe even 64)
> >> was better because it avoided stomping on very-likely-to-be-hot write
> >> buffers.
> >
> > I suspect it could go either way. You want a small constant (for the
> > isntruction size), but any small constant is likely to be within the
> > current stack frame anyway. I don't think 0(%rsp) is particularly
> > likely to have a spill on it right then and there, but who knows..
> >
> > And 64(%rsp) is possibly going to be cold in the L1 cache, especially
> > if it's just after a deep function call. Which it might be. So it
> > might work the other way.
> >
> > So my guess would be that you wouldn't be able to measure the
> > difference. It might be there, but probably too small to really see in
> > any noise.
> >
> > But numbers talk, bullshit walks. It would be interesting to be proven wrong.
>
> Here's an article with numbers:
>
> http://shipilev.net/blog/2014/on-the-fence-with-dependencies/
>
> I think they're suggesting using a negative offset, which is safe as
> long as it doesn't page fault, even though we have the redzone
> disabled.
>
> --Andy
OK so I'll have to tweak the test to put something
on stack to measure the difference: my test tweaks a
global variable instead.
I'll try that by tomorrow.
I couldn't measure any difference between mfence and lock+addl
except in a micro-benchmark, but hey since we are tweaking this,
let's do the optimal thing.
--
MST
^ permalink raw reply
* Re: [PATCH v2 0/3] x86: faster mb()+other barrier.h tweaks
From: H. Peter Anvin @ 2016-01-12 22:25 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel, Linus Torvalds
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, virtualization, Thomas Gleixner,
Paul E. McKenney, Ingo Molnar
In-Reply-To: <1452635935-5439-1-git-send-email-mst@redhat.com>
On 01/12/16 14:10, Michael S. Tsirkin wrote:
> mb() typically uses mfence on modern x86, but a micro-benchmark shows that it's
> 2 to 3 times slower than lock; addl $0,(%%e/rsp) that we use on older CPUs.
>
> So let's use the locked variant everywhere - helps keep the code simple as
> well.
>
> While I was at it, I found some inconsistencies in comments in
> arch/x86/include/asm/barrier.h
>
> I hope I'm not splitting this up too much - the reason is I wanted to isolate
> the code changes (that people might want to test for performance) from comment
> changes approved by Linus, from (so far unreviewed) comment change I came up
> with myself.
>
> Lightly tested on my system.
>
> Michael S. Tsirkin (3):
> x86: drop mfence in favor of lock+addl
> x86: drop a comment left over from X86_OOSTORE
> x86: tweak the comment about use of wmb for IO
>
I would like to get feedback from the hardware team about the
implications of this change, first.
-hpa
^ permalink raw reply
* Re: [PATCH v2 2/3] x86: drop a comment left over from X86_OOSTORE
From: One Thousand Gnomes @ 2016-01-12 22:25 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
Andrey Konovalov, the arch/x86 maintainers, linux-kernel,
virtualization, Ingo Molnar, Borislav Petkov, Andy Lutomirski,
H. Peter Anvin, Thomas Gleixner, Paul E. McKenney, Linus Torvalds,
Ingo Molnar
In-Reply-To: <1452635935-5439-3-git-send-email-mst@redhat.com>
On Wed, 13 Jan 2016 00:10:19 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> The comment about wmb being non-nop is a left over from before commit
> 09df7c4c8097 ("x86: Remove CONFIG_X86_OOSTORE").
>
> It makes no sense now: if you have an SMP system with out of order
> stores, making wmb not a nop will not help.
There were never any IDT Winchip systems with SMP support, and they were
the one system that could enable OOSTORE (and it was worth up to 30% on
some workloads). The fencing it had was just for DMA devices.
Alan
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: H. Peter Anvin @ 2016-01-12 22:55 UTC (permalink / raw)
To: Michael S. Tsirkin, Andy Lutomirski
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
virtualization, Andy Lutomirski, Thomas Gleixner,
Paul E. McKenney, Linus Torvalds, Ingo Molnar
In-Reply-To: <20160113001824-mutt-send-email-mst@redhat.com>
On 01/12/16 14:21, Michael S. Tsirkin wrote:
>
> OK so I'll have to tweak the test to put something
> on stack to measure the difference: my test tweaks a
> global variable instead.
> I'll try that by tomorrow.
>
> I couldn't measure any difference between mfence and lock+addl
> except in a micro-benchmark, but hey since we are tweaking this,
> let's do the optimal thing.
>
Be careful with this: if it only shows up in a microbenchmark, we may
introduce a hard-to-debug regression for no real benefit.
-hpa
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Linus Torvalds @ 2016-01-12 23:24 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
Linux Kernel Mailing List, Michael S. Tsirkin,
the arch/x86 maintainers, Andy Lutomirski, Andy Lutomirski,
Paul E. McKenney, Thomas Gleixner, virtualization, Ingo Molnar
In-Reply-To: <56958467.6010808@zytor.com>
On Tue, Jan 12, 2016 at 2:55 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>
> Be careful with this: if it only shows up in a microbenchmark, we may
> introduce a hard-to-debug regression for no real benefit.
So I can pretty much guarantee that it shouldn't regress from a
correctness angle, since we rely *heavily* on locked instructions
being barriers, in locking and in various other situations.
Indeed, much more so than we ever rely on "smp_mb()". The places that
rely on smp_mb() are pretty few in the end.
So I think the only issue is whether sometimes "mfence" might be
faster. So far, I've never actually heard of that being the case. The
fence instructions have always sucked when I've seen them.
But talking to the hw people about this is certainly a good idea regardless.
Linus
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox