From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:16:38
Changes since v2:
- extended checkpatch tests for barriers, and added patches
teaching it to warn about incorrect usage of barriers
(__smp_xxx barriers are for use by asm-generic code only),
should help prevent misuse by arch code
to address comments by Russell King
- patched more instances of xen to use virt_ barriers
as suggested by Stefano Stabellini
- implemented a 2 byte xchg on sh instead of hacking around it
as suggested by Peter Zijlstra and Rich Felker
- added a patch to drop some s390 arch-specific smp_xxx barriers - generic
versions are more efficient
as suggested by Peter Zijlstra and Martin Schwidefsky
- added a patch to replace before/after atomic barriers with barrier()
on s390 as suggested by Peter Zijlstra and Martin Schwidefsky
- included acks from multiple arch maintainers
thanks a lot for the review!
Changes since v1:
- replaced an asm-generic patch with an equivalent patch already in tip
- add wrappers with virt_ prefix for better code annotation,
as suggested by David Miller
- dropped XXX in patch names as this makes vger choke, Cc all relevant
mailing lists on all patches (not personal email, as the list becomes
too long then)
I parked this in vhost tree for now, though the inclusion of patch 1 from tip
creates a merge conflict - but one that is trivial to resolve.
So I intend to just merge it all through my tree, including the
duplicate patch, and assume conflict will be resolved.
I would really appreciate some feedback on arch bits (especially the x86 bits),
and acks for merging this through the vhost tree.
Thanks!
What really started me off is trying to cleanup some virt code, as suggested by
Peter, who said
You could of course go fix that instead of mutilating things into
sort-of functional state.
This work is needed for virtio, so it's probably easiest to
merge it through my tree - is this fine by everyone?
Note to arch maintainers: please don't cherry-pick patches out of this patchset
as it's been structured in this order to avoid breaking bisect.
Please send acks instead!
=====
Sometimes, virtualization is weird. For example, virtio does this (conceptually):
#ifdef CONFIG_SMP
smp_mb();
#else
mb();
#endif
Similarly, Xen calls mb() when it's not doing any MMIO at all.
Of course it's wrong in the sense that it's suboptimal. What we would really
like is to have, on UP, exactly the same barrier as on SMP. This is because a
UP guest can run on an SMP host.
But Linux doesn't provide this ability: if CONFIG_SMP is not defined is
optimizes most barriers out to a compiler barrier.
Consider for example x86: what we want is xchg (NOT mfence - there's no real IO
going on here - just switching out of the VM - more like a function call
really) but if built without CONFIG_SMP smp_store_mb does not include this.
Virt in general is probably the only use-case, because this really is an
artifact of interfacing with an SMP host while running an UP kernel,
but since we have (at least) two users, it seems to make sense to
put these APIs in a central place.
In fact, smp_ barriers are stubs on !SMP, so they can be defined as follows:
arch/XXX/include/asm/barrier.h:
#define __smp_mb() DOSOMETHING
include/asm-generic/barrier.h:
#ifdef CONFIG_SMP
#define smp_mb() __smp_mb()
#else
#define smp_mb() barrier()
#endif
This has the benefit of cleaning out a bunch of duplicated
ifdefs on a bunch of architectures - this patchset brings
about a net reduction in LOC, more than compensated for
later by performance enhancements, extra documentation and tools :)
Then virt can use __smp_XXX when talking to an SMP host.
To make those users explicit, this patchset adds virt_xxx wrappers
for them.
Touching all archs is a tad tedious, but its fairly straight forward.
The patchset is structured as follows:
-. Patch 1 fixes a bug in asm-generic.
It is already in tip, included here for completeness.
-. Patches 2-12 make sure barrier.h on all remaining
architectures includes asm-generic/barrier.h:
after the change in Patch 1, code there matches
asm-generic/barrier.h almost verbatim.
Minor code tweaks were required in a couple of places.
Macros duplicated from asm-generic/barrier.h are dropped
in the process.
After all that preparatory work, we are getting to the actual change.
-. Patch 13 adds generic smp_XXX wrappers in asm-generic:
these select __smp_XXX or barrier() depending on CONFIG_SMP
-. Patches 14-27 change all architectures to
define __smp_XXX macros; the generic code in asm-generic/barrier.h
then defines smp_XXX macros
I compiled the affected arches before and after the changes,
dumped the .text section (using objdump -O binary) and
made sure that the object code is exactly identical
before and after the change.
Note: the changes were intentionally done in a way
that avoids generated code changes.
When I got feedback from arch maintainers that the
barriers can be improved, I made this in a separate
patch on top, to allow this testing by binary comparisons.
Unfortunately, I don't have a metag cross-build toolset ready.
Hoping for some acks on this architecture.
Next, the following patches put the __smp_xxx APIs to work for virt:
-. Patch 28 adds virt_ wrappers for __smp_, and documents them.
After all this work, this requires very few lines of code in
the generic header.
-. Patches 29,30 convert virtio drivers to use the virt_xxx APIs
tested on x86
-. Patches 31-33 teach virtio to use virt_store_mb
sh architecture was missing a 2-byte xchg,
needed for 2 byte smp_store_mb,
so I had to add this support for sh
-. Patches 34-36 teach checkpatch to warn about
misuse of the new barriers
-. Patches 37-39 convert xen drivers to use the virt_xxx APIs
compiled only (by intel 0-day infrastructure)
-. Patch 40 makes some smp barriers on s390 more efficient
included here to avoid merge conflicts, at maintainer's request
tested on x86
Davidlohr Bueso (1):
lcoking/barriers, arch: Use smp barriers in smp_store_release()
Michael S. Tsirkin (40):
asm-generic: guard smp_store_release/load_acquire
ia64: rename nop->iosapic_nop
ia64: reuse asm-generic/barrier.h
powerpc: reuse asm-generic/barrier.h
s390: reuse asm-generic/barrier.h
sparc: reuse asm-generic/barrier.h
arm: reuse asm-generic/barrier.h
arm64: reuse asm-generic/barrier.h
metag: reuse asm-generic/barrier.h
mips: reuse asm-generic/barrier.h
x86/um: reuse asm-generic/barrier.h
x86: reuse asm-generic/barrier.h
asm-generic: add __smp_xxx wrappers
powerpc: define __smp_xxx
arm64: define __smp_xxx
arm: define __smp_xxx
blackfin: define __smp_xxx
ia64: define __smp_xxx
metag: define __smp_xxx
mips: define __smp_xxx
s390: define __smp_xxx
sh: define __smp_xxx, fix smp_store_mb for !SMP
sparc: define __smp_xxx
tile: define __smp_xxx
xtensa: define __smp_xxx
x86: define __smp_xxx
asm-generic: implement virt_xxx memory barriers
Revert "virtio_ring: Update weak barriers to use dma_wmb/rmb"
virtio_ring: update weak barriers to use virt_xxx
sh: support 1 and 2 byte xchg
sh: move xchg_cmpxchg to a header by itself
virtio_ring: use virt_store_mb
checkpatch.pl: add missing memory barriers
checkpatch: check for __smp outside barrier.h
checkpatch: add virt barriers
xenbus: use virt_xxx barriers
xen/io: use virt_xxx barriers
xen/events: use virt_xxx barriers
s390: use generic memory barriers
s390: more efficient smp barriers
arch/arm/include/asm/barrier.h | 35 ++----------
arch/arm64/include/asm/barrier.h | 19 ++-----
arch/blackfin/include/asm/barrier.h | 4 +-
arch/ia64/include/asm/barrier.h | 24 +++-----
arch/metag/include/asm/barrier.h | 55 ++++++-------------
arch/mips/include/asm/barrier.h | 51 ++++++-----------
arch/powerpc/include/asm/barrier.h | 33 ++++-------
arch/s390/include/asm/barrier.h | 23 ++++----
arch/sh/include/asm/barrier.h | 3 +-
arch/sh/include/asm/cmpxchg-grb.h | 22 ++++++++
arch/sh/include/asm/cmpxchg-irq.h | 11 ++++
arch/sh/include/asm/cmpxchg-llsc.h | 25 +--------
arch/sh/include/asm/cmpxchg-xchg.h | 51 +++++++++++++++++
arch/sh/include/asm/cmpxchg.h | 3 +
arch/sparc/include/asm/barrier_32.h | 1 -
arch/sparc/include/asm/barrier_64.h | 29 ++--------
arch/sparc/include/asm/processor.h | 3 -
arch/tile/include/asm/barrier.h | 9 +--
arch/x86/include/asm/barrier.h | 36 +++++-------
arch/x86/um/asm/barrier.h | 9 +--
arch/xtensa/include/asm/barrier.h | 4 +-
include/asm-generic/barrier.h | 106 +++++++++++++++++++++++++++++++++---
include/linux/virtio_ring.h | 21 +++++--
include/xen/interface/io/ring.h | 16 +++---
arch/ia64/kernel/iosapic.c | 6 +-
drivers/virtio/virtio_ring.c | 15 +++--
drivers/xen/events/events_fifo.c | 3 +-
drivers/xen/xenbus/xenbus_comms.c | 8 +--
Documentation/memory-barriers.txt | 28 ++++++++--
scripts/checkpatch.pl | 31 ++++++++++-
30 files changed, 382 insertions(+), 302 deletions(-)
create mode 100644 arch/sh/include/asm/cmpxchg-xchg.h
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:16:52
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 <redacted>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Heiko Carstens <redacted>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <redacted>
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>
---
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(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:16:56
Allow architectures to override smp_store_release
and smp_load_acquire by guarding the defines
in asm-generic/barrier.h with ifndef directives.
This is in preparation to reusing asm-generic/barrier.h
on architectures which have their own definition
of these macros.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
include/asm-generic/barrier.h | 4 ++++
1 file changed, 4 insertions(+)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:17:07
asm-generic/barrier.h defines a nop() macro.
To be able to use this header on ia64, we shouldn't
call local functions/variables nop().
There's one instance where this breaks on ia64:
rename the function to iosapic_nop to avoid the conflict.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Tony Luck <tony.luck@intel.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
arch/ia64/kernel/iosapic.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -256,7 +256,7 @@ set_rte (unsigned int gsi, unsigned int irq, unsigned int dest, int mask)}staticvoid-nop(structirq_data*data)+iosapic_nop(structirq_data*data){/* do nothing... */}
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:17:15
On ia64 smp_rmb, smp_wmb, read_barrier_depends, smp_read_barrier_depends
and smp_store_mb() 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: Tony Luck <tony.luck@intel.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
arch/ia64/include/asm/barrier.h | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:17:23
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>
---
arch/powerpc/include/asm/barrier.h | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:17:34
On s390 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>
---
arch/s390/include/asm/barrier.h | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:17:40
On sparc 64 bit dma_rmb, dma_wmb, smp_store_mb, smp_mb, smp_rmb,
smp_wmb, read_barrier_depends and smp_read_barrier_depends match the
asm-generic variants exactly. Drop the local definitions and pull in
asm-generic/barrier.h instead.
nop uses __asm__ __volatile but is otherwise identical to
the generic version, drop that as well.
This is in preparation to refactoring this code area.
Note: nop() was in processor.h and not in barrier.h as on other
architectures. Nothing seems to depend on it being there though.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: David S. Miller <davem@davemloft.net>
---
arch/sparc/include/asm/barrier_32.h | 1 -
arch/sparc/include/asm/barrier_64.h | 21 ++-------------------
arch/sparc/include/asm/processor.h | 3 ---
3 files changed, 2 insertions(+), 23 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:17:49
On arm smp_store_mb, read_barrier_depends, smp_read_barrier_depends,
smp_store_release, smp_load_acquire, 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>
Acked-by: Russell King <redacted>
---
arch/arm/include/asm/barrier.h | 23 +----------------------
1 file changed, 1 insertion(+), 22 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:17:59
On arm64 nop, 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>
---
arch/arm64/include/asm/barrier.h | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:18:07
On metag dma_rmb, dma_wmb, smp_store_mb, read_barrier_depends,
smp_read_barrier_depends, smp_store_release and smp_load_acquire 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>
---
arch/metag/include/asm/barrier.h | 25 ++-----------------------
1 file changed, 2 insertions(+), 23 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:18:15
On mips dma_rmb, dma_wmb, smp_store_mb, read_barrier_depends,
smp_read_barrier_depends, smp_store_release and smp_load_acquire 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>
---
arch/mips/include/asm/barrier.h | 25 ++-----------------------
1 file changed, 2 insertions(+), 23 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:18:26
On x86/um CONFIG_SMP is never defined. As a result, several macros
match the asm-generic variant 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>
Acked-by: Richard Weinberger <richard@nod.at>
---
arch/x86/um/asm/barrier.h | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:18:32
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>
---
arch/x86/include/asm/barrier.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
@@ -43,9 +43,6 @@#define smp_store_mb(var, value) do { WRITE_ONCE(var, value); barrier(); } while (0)#endif /* SMP */-#define read_barrier_depends() do { } while (0)-#define smp_read_barrier_depends() do { } while (0)-#if defined(CONFIG_X86_PPRO_FENCE)/*
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:18:39
On !SMP, most architectures define their
barriers as compiler barriers.
On SMP, most need an actual barrier.
Make it possible to remove the code duplication for
!SMP by defining low-level __smp_xxx barriers
which do not depend on the value of SMP, then
use them from asm-generic conditionally.
Besides reducing code duplication, these low level APIs will also be
useful for virtualization, where a barrier is sometimes needed even if
!SMP since we might be talking to another kernel on the same SMP system.
Both virtio and Xen drivers will benefit.
The smp_xxx variants should use __smp_XXX ones or barrier() depending on
SMP, identically for all architectures.
We keep ifndef guards around them for now - once/if all
architectures are converted to use the generic
code, we'll be able to remove these.
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
include/asm-generic/barrier.h | 91 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 82 insertions(+), 9 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:18:50
This defines __smp_xxx barriers for powerpc
for use by virtualization.
smp_xxx barriers are removed as they are
defined correctly by asm-generic/barriers.h
This reduces the amount of arch-specific boiler-plate code.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Boqun Feng <redacted>
---
arch/powerpc/include/asm/barrier.h | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:18:57
This defines __smp_xxx barriers for arm64,
for use by virtualization.
smp_xxx barriers are removed as they are
defined correctly by asm-generic/barriers.h
Note: arm64 does not support !SMP config,
so smp_xxx and __smp_xxx are always equivalent.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
arch/arm64/include/asm/barrier.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:19:07
This defines __smp_xxx barriers for arm,
for use by virtualization.
smp_xxx barriers are removed as they are
defined correctly by asm-generic/barriers.h
This reduces the amount of arch-specific boiler-plate code.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Russell King <redacted>
---
arch/arm/include/asm/barrier.h | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:19:14
This defines __smp_xxx barriers for blackfin,
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>
---
arch/blackfin/include/asm/barrier.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:19:23
This defines __smp_xxx barriers for ia64,
for use by virtualization.
smp_xxx barriers are removed as they are
defined correctly by asm-generic/barriers.h
This reduces the amount of arch-specific boiler-plate code.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Tony Luck <tony.luck@intel.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
arch/ia64/include/asm/barrier.h | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:19:31
This defines __smp_xxx barriers for metag,
for use by virtualization.
smp_xxx barriers are removed as they are
defined correctly by asm-generic/barriers.h
Note: as __smp_XX macros should not depend on CONFIG_SMP, they can not
use the existing fence() macro since that is defined differently between
SMP and !SMP. For this reason, this patch introduces a wrapper
metag_fence() that doesn't depend on CONFIG_SMP.
fence() is then defined using that, depending on CONFIG_SMP.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
arch/metag/include/asm/barrier.h | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:19:40
This defines __smp_xxx barriers for mips,
for use by virtualization.
smp_xxx barriers are removed as they are
defined correctly by asm-generic/barriers.h
Note: the only exception is smp_mb__before_llsc which is mips-specific.
We define both the __smp_mb__before_llsc variant (for use in
asm/barriers.h) and smp_mb__before_llsc (for use elsewhere on this
architecture).
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
arch/mips/include/asm/barrier.h | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
@@ -111,6 +111,7 @@#ifdef CONFIG_CPU_CAVIUM_OCTEON#define smp_mb__before_llsc() smp_wmb()+#define __smp_mb__before_llsc() __smp_wmb()/* Cause previous writes to become visible on all CPUs as soon as possible */#define nudge_writes() __asm__ __volatile__(".set push\n\t" \".set arch=octeon\n\t"\
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:19:49
This defines __smp_xxx barriers for s390,
for use by virtualization.
Some smp_xxx barriers are removed as they are
defined correctly by asm-generic/barriers.h
Note: smp_mb, smp_rmb and smp_wmb are defined as full barriers
unconditionally on this architecture.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Martin Schwidefsky <redacted>
---
arch/s390/include/asm/barrier.h | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:19:59
sh variant of smp_store_mb() calls xchg() on !SMP which is stronger than
implied by both the name and the documentation.
define __smp_store_mb instead: code in asm-generic/barrier.h
will then define smp_store_mb correctly depending on
CONFIG_SMP.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
arch/sh/include/asm/barrier.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:20:06
This defines __smp_xxx barriers for sparc,
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>
Acked-by: David S. Miller <davem@davemloft.net>
---
arch/sparc/include/asm/barrier_64.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:20:15
This defines __smp_xxx barriers for tile,
for use by virtualization.
Some smp_xxx barriers are removed as they are
defined correctly by asm-generic/barriers.h
Note: for 32 bit, keep smp_mb__after_atomic around since it's faster
than the generic implementation.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
arch/tile/include/asm/barrier.h | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:20:21
This defines __smp_xxx barriers for xtensa,
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>
---
arch/xtensa/include/asm/barrier.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:20:31
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>
---
arch/x86/include/asm/barrier.h | 31 ++++++++++++-------------------
1 file changed, 12 insertions(+), 19 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:20:39
Guests running within virtual machines might be affected by SMP effects even if
the guest itself is compiled without SMP support. This is an artifact of
interfacing with an SMP host while running an UP kernel. Using mandatory
barriers for this use-case would be possible but is often suboptimal.
In particular, virtio uses a bunch of confusing ifdefs to work around
this, while xen just uses the mandatory barriers.
To better handle this case, low-level virt_mb() etc macros are made available.
These are implemented trivially using the low-level __smp_xxx macros,
the purpose of these wrappers is to annotate those specific cases.
These have the same effect as smp_mb() etc when SMP is enabled, but generate
identical code for SMP and non-SMP systems. For example, virtual machine guests
should use virt_mb() rather than smp_mb() when synchronizing against a
(possibly SMP) host.
Suggested-by: David Miller <davem@davemloft.net>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/asm-generic/barrier.h | 11 +++++++++++
Documentation/memory-barriers.txt | 28 +++++++++++++++++++++++-----
2 files changed, 34 insertions(+), 5 deletions(-)
@@ -1655,17 +1655,18 @@ macro is a good place to start looking. SMP memory barriers are reduced to compiler barriers on uniprocessor compiled systems because it is assumed that a CPU will appear to be self-consistent, and will order overlapping accesses correctly with respect to itself.+However, see the subsection on "Virtual Machine Guests" below. [!] Note that SMP memory barriers _must_ be used to control the ordering of references to shared memory on SMP systems, though the use of locking instead is sufficient. Mandatory barriers should not be used to control SMP effects, since mandatory-barriers unnecessarily impose overhead on UP systems. They may, however, be-used to control MMIO effects on accesses through relaxed memory I/O windows.-These are required even on non-SMP systems as they affect the order in which-memory operations appear to a device by prohibiting both the compiler and the-CPU from reordering them.+barriers impose unnecessary overhead on both SMP and UP systems. They may,+however, be used to control MMIO effects on accesses through relaxed memory I/O+windows. These barriers are required even on non-SMP systems as they affect+the order in which memory operations appear to a device by prohibiting both the+compiler and the CPU from reordering them. There are some more advanced barrier functions:
@@ -2948,6 +2949,23 @@ The Alpha defines the Linux kernel's memory barrier model. See the subsection on "Cache Coherency" above.+VIRTUAL MACHINE GUESTS+-------------------++Guests running within virtual machines might be affected by SMP effects even if+the guest itself is compiled without SMP support. This is an artifact of+interfacing with an SMP host while running an UP kernel. Using mandatory+barriers for this use-case would be possible but is often suboptimal.++To handle this case optimally, low-level virt_mb() etc macros are available.+These have the same effect as smp_mb() etc when SMP is enabled, but generate+identical code for SMP and non-SMP systems. For example, virtual machine guests+should use virt_mb() rather than smp_mb() when synchronizing against a+(possibly SMP) host.++These are equivalent to smp_mb() etc counterparts in all other respects,+in particular, they do not control MMIO effects: to control+MMIO effects, use mandatory barriers. ============ EXAMPLE USES
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:20:46
This reverts commit 9e1a27ea42691429e31f158cce6fc61bc79bb2e9.
While that commit optimizes !CONFIG_SMP, it mixes
up DMA and SMP concepts, making the code hard
to figure out.
A better way to optimize this is with the new __smp_XXX
barriers.
As a first step, go back to full rmb/wmb barriers
for !SMP.
We switch to __smp_XXX barriers in the next patch.
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexander Duyck <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/virtio_ring.h | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:20:55
virtio ring uses smp_wmb on SMP and wmb on !SMP,
the reason for the later being that it might be
talking to another kernel on the same SMP machine.
This is exactly what virt_xxx barriers do,
so switch to these instead of homegrown ifdef hacks.
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexander Duyck <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/virtio_ring.h | 25 ++++---------------------
1 file changed, 4 insertions(+), 21 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:21:05
This completes the xchg implementation for sh architecture. Note: The
llsc variant is tricky since this only supports 4 byte atomics, the
existing implementation of 1 byte xchg is wrong: we need to do a 4 byte
cmpxchg and retry if any bytes changed meanwhile.
Write this in C for clarity.
Suggested-by: Rich Felker <dalias@libc.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/sh/include/asm/cmpxchg-grb.h | 22 +++++++++++++++
arch/sh/include/asm/cmpxchg-irq.h | 11 ++++++++
arch/sh/include/asm/cmpxchg-llsc.h | 58 +++++++++++++++++++++++---------------
arch/sh/include/asm/cmpxchg.h | 3 ++
4 files changed, 72 insertions(+), 22 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:21:12
Looks like future sh variants will support a 4-byte cas which will be
used to implement 1 and 2 byte xchg.
This is exactly what we do for llsc now, move the portable part of the
code into a separate header so it's easy to reuse.
Suggested-by: Rich Felker <dalias@libc.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/sh/include/asm/cmpxchg-llsc.h | 35 +-------------------------
arch/sh/include/asm/cmpxchg-xchg.h | 51 ++++++++++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 34 deletions(-)
create mode 100644 arch/sh/include/asm/cmpxchg-xchg.h
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:21:21
We need a full barrier after writing out event index, using
virt_store_mb there seems better than open-coding. As usual, we need a
wrapper to account for strong barriers.
It's tempting to use this in vhost as well, for that, we'll
need a variant of smp_store_mb that works on __user pointers.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/virtio_ring.h | 11 +++++++++++
drivers/virtio/virtio_ring.c | 15 +++++++++------
2 files changed, 20 insertions(+), 6 deletions(-)
@@ -517,10 +517,10 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)/* If we expect an interrupt for the next entry, tell host*bywritingeventindexandflushoutthewritebefore*thereadinthenextget_bufcall.*/-if(!(vq->avail_flags_shadow&VRING_AVAIL_F_NO_INTERRUPT)){-vring_used_event(&vq->vring)=cpu_to_virtio16(_vq->vdev,vq->last_used_idx);-virtio_mb(vq->weak_barriers);-}+if(!(vq->avail_flags_shadow&VRING_AVAIL_F_NO_INTERRUPT))+virtio_store_mb(vq->weak_barriers,+&vring_used_event(&vq->vring),+cpu_to_virtio16(_vq->vdev,vq->last_used_idx));#ifdef DEBUGvq->last_add_time_valid=false;
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:21:28
SMP-only barriers were missing in checkpatch.pl
Refactor code slightly to make adding more variants easier.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
@@ -5116,7 +5116,25 @@ sub process {}}#checkformemorybarrierswithoutacomment.-if($line=~/\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/){++my$barriers=qr{+mb|+rmb|+wmb|+read_barrier_depends+}x;+my$smp_barriers=qr{+store_release|+load_acquire|+store_mb|+($barriers)+}x;+my$all_barriers=qr{+$barriers|+smp_($smp_barriers)+}x;++if($line=~/\b($all_barriers)\s*\(/){if(!ctx_has_comment($first_line,$linenr)){WARN("MEMORY_BARRIER","memory barrier without comment\n".$herecurr);
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:21:34
Introduction of __smp barriers cleans up a bunch of duplicate code, but
it gives people an additional handle onto a "new" set of barriers - just
because they're prefixed with __* unfortunately doesn't stop anyone from
using it (as happened with other arch stuff before.)
Add a checkpatch test so it will trigger a warning.
Reported-by: Russell King <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 10 ++++++++++
1 file changed, 10 insertions(+)
@@ -5141,6 +5141,16 @@ sub process {}}+my$underscore_smp_barriers=qr{__smp_($smp_barriers)}x;++if($realfile!~m@^include/asm-generic/@&&+$realfile!~m@/barrier\.h$@&&+$line=~m/\b($underscore_smp_barriers)\s*\(/&&+$line!~m/^.\s*\#\s*define\s+($underscore_smp_barriers)\s*\(/){+WARN("MEMORY_BARRIER",+"__smp memory barriers shouldn't be used outside barrier.h and asm-generic\n".$herecurr);+}+#checkforwaitqueue_activewithoutacomment.if($line=~/\bwaitqueue_active\s*\(/){if(!ctx_has_comment($first_line,$linenr)){
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:21:45
Add virt_ barriers to list of barriers to check for
presence of a comment.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
scripts/checkpatch.pl | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -5131,7 +5131,8 @@ sub process {}x;my$all_barriers=qr{$barriers|-smp_($smp_barriers)+smp_($smp_barriers)|+virt_($smp_barriers)}x;if($line=~/\b($all_barriers)\s*\(/){
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:21:52
drivers/xen/xenbus/xenbus_comms.c uses
full memory barriers to communicate with the other side.
For guests compiled with CONFIG_SMP, smp_wmb and smp_mb
would be sufficient, so mb() and wmb() here are only needed if
a non-SMP guest runs on an SMP host.
Switch to virt_xxx barriers which serve this exact purpose.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: David Vrabel <redacted>
---
drivers/xen/xenbus/xenbus_comms.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -123,14 +123,14 @@ int xb_write(const void *data, unsigned len)avail=len;/* Must write data /after/ reading the consumer index. */-mb();+virt_mb();memcpy(dst,data,avail);data+=avail;len-=avail;/* Other side must not see new producer until data is there. */-wmb();+virt_wmb();intf->req_prod+=avail;/* Implies mb(): other side will see the updated producer. */
@@ -180,14 +180,14 @@ int xb_read(void *data, unsigned len)avail=len;/* Must read data /after/ reading the producer index. */-rmb();+virt_rmb();memcpy(data,src,avail);data+=avail;len-=avail;/* Other side must not see free space until we've copied out */-mb();+virt_mb();intf->rsp_cons+=avail;pr_debug("Finished read of %i bytes (%i to go)\n",avail,len);
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:22:00
include/xen/interface/io/ring.h uses
full memory barriers to communicate with the other side.
For guests compiled with CONFIG_SMP, smp_wmb and smp_mb
would be sufficient, so mb() and wmb() here are only needed if
a non-SMP guest runs on an SMP host.
Switch to virt_xxx barriers which serve this exact purpose.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: David Vrabel <redacted>
---
include/xen/interface/io/ring.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
@@ -208,12 +208,12 @@ struct __name##_back_ring { \#define RING_PUSH_REQUESTS(_r) do { \-wmb();/* back sees requests /before/ updated producer index */\+virt_wmb();/* back sees requests /before/ updated producer index */\(_r)->sring->req_prod=(_r)->req_prod_pvt;\}while(0)#define RING_PUSH_RESPONSES(_r) do { \-wmb();/* front sees responses /before/ updated producer index */\+virt_wmb();/* front sees responses /before/ updated producer index */\(_r)->sring->rsp_prod=(_r)->rsp_prod_pvt;\}while(0)
@@ -250,9 +250,9 @@ struct __name##_back_ring { \#define RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(_r, _notify) do { \RING_IDX__old=(_r)->sring->req_prod;\RING_IDX__new=(_r)->req_prod_pvt;\-wmb();/* back sees requests /before/ updated producer index */\+virt_wmb();/* back sees requests /before/ updated producer index */\(_r)->sring->req_prod=__new;\-mb();/* back sees new requests /before/ we check req_event */\+virt_mb();/* back sees new requests /before/ we check req_event */\(_notify)=((RING_IDX)(__new-(_r)->sring->req_event)<\(RING_IDX)(__new-__old));\}while(0)
@@ -260,9 +260,9 @@ struct __name##_back_ring { \#define RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(_r, _notify) do { \RING_IDX__old=(_r)->sring->rsp_prod;\RING_IDX__new=(_r)->rsp_prod_pvt;\-wmb();/* front sees responses /before/ updated producer index */\+virt_wmb();/* front sees responses /before/ updated producer index */\(_r)->sring->rsp_prod=__new;\-mb();/* front sees new responses /before/ we check rsp_event */\+virt_mb();/* front sees new responses /before/ we check rsp_event */\(_notify)=((RING_IDX)(__new-(_r)->sring->rsp_event)<\(RING_IDX)(__new-__old));\}while(0)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:22:11
drivers/xen/events/events_fifo.c uses rmb() to communicate with the
other side.
For guests compiled with CONFIG_SMP, smp_rmb would be sufficient, so
rmb() here is only needed if a non-SMP guest runs on an SMP host.
Switch to the virt_rmb barrier which serves this exact purpose.
Pull in asm/barrier.h here to make sure the file is self-contained.
Suggested-by: David Vrabel <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/xen/events/events_fifo.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -296,7 +297,7 @@ static void consume_one_event(unsigned cpu,*controlblock.*/if(head==0){-rmb();/* Ensure word is up-to-date before reading head. */+virt_rmb();/* Ensure word is up-to-date before reading head. */head=control_block->head[priority];}
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:22:18
The s390 kernel is SMP to 99.99%, we just didn't bother with a
non-smp variant for the memory-barriers. If the generic header
is used we'd get the non-smp version for free. It will save a
small amount of text space for CONFIG_SMP=n.
Suggested-by: Martin Schwidefsky <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/s390/include/asm/barrier.h | 3 ---
1 file changed, 3 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-10 14:22:27
As per: lkml.kernel.org/r/20150921112252.3c2937e1@mschwide
atomics imply a barrier on s390, so s390 should change
smp_mb__before_atomic and smp_mb__after_atomic to barrier() instead of
smp_mb() and hence should not use the generic versions.
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Martin Schwidefsky <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/s390/include/asm/barrier.h | 3 +++
1 file changed, 3 insertions(+)
From: David Vrabel <hidden> Date: 2016-01-11 11:12:16
On 10/01/16 14:21, Michael S. Tsirkin wrote:
drivers/xen/events/events_fifo.c uses rmb() to communicate with the
other side.
For guests compiled with CONFIG_SMP, smp_rmb would be sufficient, so
rmb() here is only needed if a non-SMP guest runs on an SMP host.
Switch to the virt_rmb barrier which serves this exact purpose.
Pull in asm/barrier.h here to make sure the file is self-contained.
Suggested-by: David Vrabel <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
From: Leonid Yegoshin <hidden> Date: 2016-01-12 01:14:23
On 01/10/2016 06:18 AM, Michael S. Tsirkin wrote:
On mips dma_rmb, dma_wmb, smp_store_mb, read_barrier_depends,
smp_read_barrier_depends, smp_store_release and smp_load_acquire match
the asm-generic variants exactly. Drop the local definitions and pull in
asm-generic/barrier.h instead.
This statement doesn't fit MIPS barriers variations. Moreover, there is
a reason to extend that even more specific, at least for
smp_store_release and smp_load_acquire, look into
http://patchwork.linux-mips.org/patch/10506/
- Leonid.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-12 08:43:50
On Mon, Jan 11, 2016 at 05:14:14PM -0800, Leonid Yegoshin wrote:
On 01/10/2016 06:18 AM, Michael S. Tsirkin wrote:
quoted
On mips dma_rmb, dma_wmb, smp_store_mb, read_barrier_depends,
smp_read_barrier_depends, smp_store_release and smp_load_acquire match
the asm-generic variants exactly. Drop the local definitions and pull in
asm-generic/barrier.h instead.
This statement doesn't fit MIPS barriers variations. Moreover, there is a
reason to extend that even more specific, at least for smp_store_release and
smp_load_acquire, look into
http://patchwork.linux-mips.org/patch/10506/
- Leonid.
Fine, but it matches what current code is doing. Since that
MIPS_LIGHTWEIGHT_SYNC patch didn't go into linux-next yet, do
you see a problem reworking it on top of this patchset?
--
MST
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-12 09:28:00
On Mon, Jan 11, 2016 at 05:14:14PM -0800, Leonid Yegoshin wrote:
This statement doesn't fit MIPS barriers variations. Moreover, there is a
reason to extend that even more specific, at least for smp_store_release and
smp_load_acquire, look into
http://patchwork.linux-mips.org/patch/10506/
Dude, that's one horrible patch.
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.
2) the changelog _completely_ fails to explain the sync 0x11 and sync
0x12 semantics nor does it provide a publicly accessible link to
documentation that does.
3) it really should have explained what you did with
smp_llsc_mb/smp_mb__before_llsc() in _detail_.
And I agree that ideally it should be split into parts.
Seriously, this is _NOT_ OK.
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-12 09:52:51
On Tue, Jan 12, 2016 at 10:43:36AM +0200, Michael S. Tsirkin wrote:
On Mon, Jan 11, 2016 at 05:14:14PM -0800, Leonid Yegoshin wrote:
quoted
On 01/10/2016 06:18 AM, Michael S. Tsirkin wrote:
quoted
On mips dma_rmb, dma_wmb, smp_store_mb, read_barrier_depends,
smp_read_barrier_depends, smp_store_release and smp_load_acquire match
the asm-generic variants exactly. Drop the local definitions and pull in
asm-generic/barrier.h instead.
This statement doesn't fit MIPS barriers variations. Moreover, there is a
reason to extend that even more specific, at least for smp_store_release and
smp_load_acquire, look into
http://patchwork.linux-mips.org/patch/10506/
- Leonid.
Fine, but it matches what current code is doing. Since that
MIPS_LIGHTWEIGHT_SYNC patch didn't go into linux-next yet, do
you see a problem reworking it on top of this patchset?
That patch is a complete doorstop atm. It needs a lot more work before
it can go anywhere. Don't worry about it.
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-12 10:26:40
On Tue, Jan 12, 2016 at 10:27:11AM +0100, Peter Zijlstra wrote:
2) the changelog _completely_ fails to explain the sync 0x11 and sync
0x12 semantics nor does it provide a publicly accessible link to
documentation that does.
3) it really should have explained what you did with
smp_llsc_mb/smp_mb__before_llsc() in _detail_.
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. They need not be globally performed.
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-12 10:40:52
On Tue, Jan 12, 2016 at 11:25:55AM +0100, Peter Zijlstra wrote:
On Tue, Jan 12, 2016 at 10:27:11AM +0100, Peter Zijlstra wrote:
quoted
2) the changelog _completely_ fails to explain the sync 0x11 and sync
0x12 semantics nor does it provide a publicly accessible link to
documentation that does.
3) it really should have explained what you did with
smp_llsc_mb/smp_mb__before_llsc() in _detail_.
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. They need not be globally performed.
Which if true; and I know Will has some questions here; would also mean
that you 'cannot' use the ACQUIRE/RELEASE barriers for your locks as was
recently suggested by David Daney.
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.
From: Will Deacon <hidden> Date: 2016-01-12 11:41:21
On Tue, Jan 12, 2016 at 11:40:12AM +0100, Peter Zijlstra wrote:
On Tue, Jan 12, 2016 at 11:25:55AM +0100, Peter Zijlstra wrote:
quoted
On Tue, Jan 12, 2016 at 10:27:11AM +0100, Peter Zijlstra wrote:
quoted
2) the changelog _completely_ fails to explain the sync 0x11 and sync
0x12 semantics nor does it provide a publicly accessible link to
documentation that does.
3) it really should have explained what you did with
smp_llsc_mb/smp_mb__before_llsc() in _detail_.
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. They need not be globally performed.
Which if true; and I know Will has some questions here; would also mean
that you 'cannot' use the ACQUIRE/RELEASE barriers for your locks as was
recently suggested by David Daney.
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.
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 think it's much worse than RCpc, given my interpretation of the wording.
Will
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-12 12:50:39
On Sun, Jan 10, 2016 at 04:16:22PM +0200, Michael S. Tsirkin wrote:
I parked this in vhost tree for now, though the inclusion of patch 1 from tip
creates a merge conflict - but one that is trivial to resolve.
So I intend to just merge it all through my tree, including the
duplicate patch, and assume conflict will be resolved.
I would really appreciate some feedback on arch bits (especially the x86 bits),
and acks for merging this through the vhost tree.
Thanks for doing this, looks good to me.
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
From: Thomas Gleixner <hidden> Date: 2016-01-12 14:12:59
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>
From: Thomas Gleixner <hidden> Date: 2016-01-12 14:14:34
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>
From: Paul E. McKenney <hidden> Date: 2016-01-12 16:29:14
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 <redacted>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Heiko Carstens <redacted>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <redacted>
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 <redacted>
From: Paul E. McKenney <hidden> Date: 2016-01-12 16:40:15
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 <redacted>
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-12 18:41:10
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:
quoted
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 <redacted>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Heiko Carstens <redacted>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <redacted>
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 <redacted>
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.
From: Leonid Yegoshin <hidden> Date: 2016-01-12 20:45:28
(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.
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-12 21:40:24
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?
quoted
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:
quoted
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?
quoted
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 :-)
quoted
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
From: Leonid Yegoshin <hidden> Date: 2016-01-13 00:21:54
On 01/12/2016 01:40 PM, Peter Zijlstra wrote:
quoted
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?
It was my guess to explain - why barriers was not included originally.
You can check with Ralf, he knows more about that time MIPS Linux code.
I bother with this more than 2 years and I just try to solve that issue
- in recent CPUs the load after LL/SC synchronization instruction loop
can get ahead of SC for sure, it was tested.
quoted
quoted
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?
Peter Zijlstra recently wrote: "In particular we're very much all
'confused' about the various notions of transitivity". I am actually
confused too and need some examples here.
(and here Will will go into great detail on the differences between the
two and make our collective brains explode :-)
quoted
quoted
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.
MIPS Arch starting from R2 requires that. If some CPU can't, it should
execute a full "SYNC 0" instead, which is a full memory barrier.
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.
Yes, it was a goal for SYNC_ACQUIRE and SYNC_RELEASE.
Caveats:
- "Full memory barrier" on MIPS means - full barrier for any device
in coherent domain. In MIPS Tech/Imagination Tech MIPS-based CPU it is
"for any device connected to CM or IOCU + directly connected memory".
- It is not applied to instruction fetch. However, I-Cache flushes
and SYNCI are consistent with that. There is also hazard barrier
instructions to clear CPU pipeline to some extent - to help with this
limitation.
I don't think that these caveats prevent a correct Acquire/Release semantic.
- Leonid.
From: Will Deacon <hidden> Date: 2016-01-13 10:45:27
On Tue, Jan 12, 2016 at 12:45:14PM -0800, Leonid Yegoshin wrote:
quoted
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.
I don't think the address dependency is enough on its own. By that
reasoning, the following variant (WRC+addr+addr) would work too:
P0:
Wx = 1
P1:
Rx == 1
<address dep>
Wy = 1
P2:
Ry == 1
<address dep>
Rx = 0
So are you saying that this is also forbidden?
Imagine that P0 and P1 are two threads that share a store buffer. What
then?
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.
I tried grepping the linked documents for "coherence manager" but couldn't
find anything. Is the description you refer to available anywhere?
Will
From: Leonid Yegoshin <hidden> Date: 2016-01-13 19:02:45
On 01/13/2016 02:45 AM, Will Deacon wrote:
On Tue, Jan 12, 2016 at 12:45:14PM -0800, Leonid Yegoshin wrote:
quoted
I don't think the address dependency is enough on its own. By that
reasoning, the following variant (WRC+addr+addr) would work too:
P0:
Wx = 1
P1:
Rx == 1
<address dep>
Wy = 1
P2:
Ry == 1
<address dep>
Rx = 0
So are you saying that this is also forbidden?
Imagine that P0 and P1 are two threads that share a store buffer. What
then?
I ask HW team about it but I have a question - has it any relationship
with replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)? You use
any barrier or do not use it and I just voice an intention to use a more
efficient instruction instead of bold hummer (SYNC instruction). If you
don't use any barrier here then it is a different issue.
May be it has sense to return back to original issue?
- Leonid
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-13 20:49:29
On Wed, Jan 13, 2016 at 11:02:35AM -0800, Leonid Yegoshin wrote:
I ask HW team about it but I have a question - has it any relationship with
replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)?
Of course. If you cannot explain the semantics of the primitives you
introduce, how can we judge the patch.
This barrier business is hard enough as it is, but magic unexplained
hardware makes it impossible.
Rest assured, you (MIPS) isn't the first (nor likely the last) to go
through all this. We've had these discussions (and to a certain extend
are still having them) for x86, PPC, Alpha, ARM, etc..
Any every time new barriers instructions get introduced we had better
have a full and comprehensive explanation to go along with them.
From: Leonid Yegoshin <hidden> Date: 2016-01-13 20:58:34
On 01/13/2016 12:48 PM, Peter Zijlstra wrote:
On Wed, Jan 13, 2016 at 11:02:35AM -0800, Leonid Yegoshin wrote:
quoted
I ask HW team about it but I have a question - has it any relationship with
replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)?
Of course. If you cannot explain the semantics of the primitives you
introduce, how can we judge the patch.
You missed a point - it is a question about replacement of SYNC with
lightweight primitives. It is NOT a question about multithread system
behavior without any SYNC. The answer on a latest Will's question lies
in different area.
- Leonid.
From: Leonid Yegoshin <hidden> Date: 2016-01-13 22:26:27
On 01/13/2016 02:45 AM, Will Deacon wrote:
quoted
I don't think the address dependency is enough on its own. By that
reasoning, the following variant (WRC+addr+addr) would work too:
P0:
Wx = 1
P1:
Rx == 1
<address dep>
Wy = 1
P2:
Ry == 1
<address dep>
Rx = 0
So are you saying that this is also forbidden?
Imagine that P0 and P1 are two threads that share a store buffer. What
then?
OK, I collected answers and it is:
In MIPS R6 this test passes OK, I mean - P2: Rx = 1 if Ry is read
as 1. By design.
However, it is unclear that happens in MIPS R2 1004K.
Moreover, there are voices against guarantee that it will be in
future and that voices point me to Documentation/memory-barriers.txt
section "DATA DEPENDENCY BARRIERS" examples which require SYNC_RMB
between loading address/index and using that for loading data based on
that address or index for shared data (look on CPU2 pseudo-code):
To deal with this, a data dependency barrier or better must be inserted
between the address load and the data load:
CPU 1 CPU 2
=============== ===============
{ A == 1, B == 2, C = 3, P == &A, Q == &C }
B = 4;
<write barrier>
WRITE_ONCE(P, &B);
Q = READ_ONCE(P);
<data dependency barrier> <-----------
SYNC_RMB is here
D = *Q;
...
Another example of where data dependency barriers might be required is
where a
number is read from memory and then used to calculate the index for an
array
access:
CPU 1 CPU 2
=============== ===============
{ M[0] == 1, M[1] == 2, M[3] = 3, P == 0, Q == 3 }
M[1] = 4;
<write barrier>
WRITE_ONCE(P, 1);
Q = READ_ONCE(P);
<data dependency barrier> <------------
SYNC_RMB is here
D = M[Q];
That voices say that there is a legitimate reason to relax HW here for
performance if SYNC_RMB is needed anyway to work with this sequence of
shared data.
And all that is out-of-topic here in my mind. I just want to be sure
that this patchset still provides a use of a specific lightweight SYNCs
on MIPS vs bold and heavy generalized "SYNC 0" in any case.
- Leonid.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-14 09:24:18
On Wed, Jan 13, 2016 at 02:26:16PM -0800, Leonid Yegoshin wrote:
And all that is out-of-topic here in my mind. I just want to be sure that
this patchset still provides a use of a specific lightweight SYNCs on MIPS
vs bold and heavy generalized "SYNC 0" in any case.
- Leonid.
Of course it does. All this patchset does is rename smp_mb/rmb/wmb
to __smp_mb()/__smp_rmb()/__smp_wmb()
and then asm-generic does #define smp_mb __smp_mb
or #define smp_mb barrier depending on CONFIG_SMP.
Why is that needed? So we can implement
[PATCH v3 28/41] asm-generic: implement virt_xxx memory barriers
--
MST
From: Will Deacon <hidden> Date: 2016-01-14 12:04:54
On Wed, Jan 13, 2016 at 12:58:22PM -0800, Leonid Yegoshin wrote:
On 01/13/2016 12:48 PM, Peter Zijlstra wrote:
quoted
On Wed, Jan 13, 2016 at 11:02:35AM -0800, Leonid Yegoshin wrote:
quoted
I ask HW team about it but I have a question - has it any relationship with
replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)?
Of course. If you cannot explain the semantics of the primitives you
introduce, how can we judge the patch.
You missed a point - it is a question about replacement of SYNC with
lightweight primitives. It is NOT a question about multithread system
behavior without any SYNC. The answer on a latest Will's question lies in
different area.
The reason we (Peter and I) care about this isn't because we enjoy being
obstructive. It's because there is a whole load of core (i.e. portable)
kernel code that is written to the *kernel* memory model. For example,
the scheduler, RCU, mutex implementations, perf, drivers, you name it.
Consequently, it's important that the architecture back-ends implement
these portable primitives (e.g. smp_mb()) in a way that satisfies the
kernel memory model so that core code doesn't need to worry about the
underlying architecture for synchronisation purposes. You could turn
around and say "but if MIPS gets it wrong, then that's MIPS's problem",
but actually not having a general understanding of the ordering guarantees
provided by each architecture makes it very difficult for us to extend
the kernel memory model in such a way that it can be implemented
efficiently across the board *and* relied upon by core code.
The virtio patch at the start of the thread doesn't particularly concern
me. It's the other patches you linked to that implement acquire/release
that have me worried.
Will
From: Will Deacon <hidden> Date: 2016-01-14 12:14:58
On Wed, Jan 13, 2016 at 02:26:16PM -0800, Leonid Yegoshin wrote:
On 01/13/2016 02:45 AM, Will Deacon wrote:
quoted
quoted
I don't think the address dependency is enough on its own. By that
reasoning, the following variant (WRC+addr+addr) would work too:
P0:
Wx = 1
P1:
Rx == 1
<address dep>
Wy = 1
P2:
Ry == 1
<address dep>
Rx = 0
So are you saying that this is also forbidden?
Imagine that P0 and P1 are two threads that share a store buffer. What
then?
OK, I collected answers and it is:
In MIPS R6 this test passes OK, I mean - P2: Rx = 1 if Ry is read as 1.
By design.
However, it is unclear that happens in MIPS R2 1004K.
How can it be unclear? If, for example, the outcome is permitted on that
CPU, then your original reasoning for the WRC+sync+addr doesn't apply
there and SYNC is not transitive. That's what I'm trying to get to the
bottom of.
Does the MIPS kernel target a particular CPU at compile time?
Moreover, there are voices against guarantee that it will be in future
and that voices point me to Documentation/memory-barriers.txt section "DATA
DEPENDENCY BARRIERS" examples which require SYNC_RMB between loading
address/index and using that for loading data based on that address or index
for shared data (look on CPU2 pseudo-code):
quoted
To deal with this, a data dependency barrier or better must be inserted
between the address load and the data load:
CPU 1 CPU 2
=============== ===============
{ A == 1, B == 2, C = 3, P == &A, Q == &C }
B = 4;
<write barrier>
WRITE_ONCE(P, &B);
Q = READ_ONCE(P);
<data dependency barrier> <-----------
SYNC_RMB is here
D = *Q;
...
quoted
Another example of where data dependency barriers might be required is
where a
number is read from memory and then used to calculate the index for an
array
access:
CPU 1 CPU 2
=============== ===============
{ M[0] == 1, M[1] == 2, M[3] = 3, P == 0, Q == 3 }
M[1] = 4;
<write barrier>
WRITE_ONCE(P, 1);
Q = READ_ONCE(P);
<data dependency barrier> <------------
SYNC_RMB is here
D = M[Q];
That voices say that there is a legitimate reason to relax HW here for
performance if SYNC_RMB is needed anyway to work with this sequence of
shared data.
Are you saying that MIPS needs to implement [smp_]read_barrier_depends?
And all that is out-of-topic here in my mind. I just want to be sure that
this patchset still provides a use of a specific lightweight SYNCs on MIPS
vs bold and heavy generalized "SYNC 0" in any case.
We may be highjacking the thread slightly, but there are much bigger
issues at play here if you want to start using lightweight barriers to
implement relaxed kernel primitives such as smp_load_acquire and
smp_store_release.
Will
From: Paul E. McKenney <hidden> Date: 2016-01-14 17:34:55
On Thu, Jan 14, 2016 at 12:04:45PM +0000, Will Deacon wrote:
On Wed, Jan 13, 2016 at 12:58:22PM -0800, Leonid Yegoshin wrote:
quoted
On 01/13/2016 12:48 PM, Peter Zijlstra wrote:
quoted
On Wed, Jan 13, 2016 at 11:02:35AM -0800, Leonid Yegoshin wrote:
quoted
I ask HW team about it but I have a question - has it any relationship with
replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)?
Of course. If you cannot explain the semantics of the primitives you
introduce, how can we judge the patch.
You missed a point - it is a question about replacement of SYNC with
lightweight primitives. It is NOT a question about multithread system
behavior without any SYNC. The answer on a latest Will's question lies in
different area.
The reason we (Peter and I) care about this isn't because we enjoy being
obstructive. It's because there is a whole load of core (i.e. portable)
kernel code that is written to the *kernel* memory model. For example,
the scheduler, RCU, mutex implementations, perf, drivers, you name it.
Consequently, it's important that the architecture back-ends implement
these portable primitives (e.g. smp_mb()) in a way that satisfies the
kernel memory model so that core code doesn't need to worry about the
underlying architecture for synchronisation purposes. You could turn
around and say "but if MIPS gets it wrong, then that's MIPS's problem",
but actually not having a general understanding of the ordering guarantees
provided by each architecture makes it very difficult for us to extend
the kernel memory model in such a way that it can be implemented
efficiently across the board *and* relied upon by core code.
What Will said!
Yes, you can cut corners within MIPS architecture-specific code,
but primitives that are used in the core kernel really do need to
work as expected.
Thanx, Paul
The virtio patch at the start of the thread doesn't particularly concern
me. It's the other patches you linked to that implement acquire/release
that have me worried.
Will
From: Leonid Yegoshin <hidden> Date: 2016-01-14 19:28:54
On 01/14/2016 04:14 AM, Will Deacon wrote:
On Wed, Jan 13, 2016 at 02:26:16PM -0800, Leonid Yegoshin wrote:
quoted
Moreover, there are voices against guarantee that it will be in future
and that voices point me to Documentation/memory-barriers.txt section "DATA
DEPENDENCY BARRIERS" examples which require SYNC_RMB between loading
address/index and using that for loading data based on that address or index
for shared data (look on CPU2 pseudo-code):
quoted
To deal with this, a data dependency barrier or better must be inserted
between the address load and the data load:
CPU 1 CPU 2
=============== ===============
{ A == 1, B == 2, C = 3, P == &A, Q == &C }
B = 4;
<write barrier>
WRITE_ONCE(P, &B);
Q = READ_ONCE(P);
<data dependency barrier> <-----------
SYNC_RMB is here
D = *Q;
...
quoted
Another example of where data dependency barriers might be required is
where a
number is read from memory and then used to calculate the index for an
array
access:
CPU 1 CPU 2
=============== ===============
{ M[0] == 1, M[1] == 2, M[3] = 3, P == 0, Q == 3 }
M[1] = 4;
<write barrier>
WRITE_ONCE(P, 1);
Q = READ_ONCE(P);
<data dependency barrier> <------------
SYNC_RMB is here
D = M[Q];
That voices say that there is a legitimate reason to relax HW here for
performance if SYNC_RMB is needed anyway to work with this sequence of
shared data.
Are you saying that MIPS needs to implement [smp_]read_barrier_depends?
It is not me, it is Documentation/memory-barriers.txt from kernel sources.
HW team can't work on voice statements, it should do a work on written
documents. If that is written (see above the lines which I marked by
"SYNC_RMB") then anybody should use it and never mind how many
CPUs/Threads are in play. This examples explicitly requires to insert
"data dependency barrier" between reading a shared pointer/index and
using it to fetch a shared data. So, your WRC+addr+addr test is a
violation of that recommendation.
- Leonid.
From: Leonid Yegoshin <hidden> Date: 2016-01-14 19:42:35
On 01/14/2016 08:16 AM, Paul E. McKenney wrote:
On Thu, Jan 14, 2016 at 12:04:45PM +0000, Will Deacon wrote:
quoted
On Wed, Jan 13, 2016 at 12:58:22PM -0800, Leonid Yegoshin wrote:
quoted
On 01/13/2016 12:48 PM, Peter Zijlstra wrote:
quoted
On Wed, Jan 13, 2016 at 11:02:35AM -0800, Leonid Yegoshin wrote:
quoted
I ask HW team about it but I have a question - has it any relationship with
replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)?
Of course. If you cannot explain the semantics of the primitives you
introduce, how can we judge the patch.
You missed a point - it is a question about replacement of SYNC with
lightweight primitives. It is NOT a question about multithread system
behavior without any SYNC. The answer on a latest Will's question lies in
different area.
What Will said!
Yes, you can cut corners within MIPS architecture-specific code,
but primitives that are used in the core kernel really do need to
work as expected.
Thanx, Paul
Absolutelly! Please use SYNC - right now it is not.
An the only point - please use an appropriate SYNC_* barriers instead of
heavy bold hammer. That stuff was design explicitly to support the
requirements of Documentation/memory-barriers.txt
It is easy - just use smp_acquire instead of plain smp_mb
insmp_load_acquire, at least for MIPS.
- Leonid.
- Leonid.
From: Leonid Yegoshin <hidden> Date: 2016-01-14 20:13:25
On 01/14/2016 04:04 AM, Will Deacon wrote:
Consequently, it's important that the architecture back-ends implement
these portable primitives (e.g. smp_mb()) in a way that satisfies the
kernel memory model so that core code doesn't need to worry about the
underlying architecture for synchronisation purposes.
It seems you don't listen me. I said multiple times - MIPS
implementation of SYNC_RMB/SYNC_WMB/SYNC_MB/SYNC_ACQUIRE/SYNC_RELEASE
instructions matches the description of
smp_rmb/smp_wmb/smp_mb/sync_acquire/sync_release from
Documentation/memory-barriers.txt file.
What else do you want from me - RTL or microArch design for that?
- Leonid.
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-14 20:15:51
On Thu, Jan 14, 2016 at 11:42:02AM -0800, Leonid Yegoshin wrote:
An the only point - please use an appropriate SYNC_* barriers instead of
heavy bold hammer. That stuff was design explicitly to support the
requirements of Documentation/memory-barriers.txt
That's madness. That document changes from version to version as to what
we _think_ the actual hardware does. It is _NOT_ a specification.
You cannot design hardware from that. Its incomplete and fails to
specify a bunch of things. It not a mathematically sound definition of a
memory model.
Please stop referring to that document for what a particular barrier
_should_ do. Explain what MIPS does, so we can attempt to integrate
this knowledge with our knowledge of PPC/ARM/Alpha/x86/etc. and improve
upon our understanding of hardware and improve the Linux memory model.
From: Paul E. McKenney <hidden> Date: 2016-01-14 20:34:56
On Thu, Jan 14, 2016 at 11:28:18AM -0800, Leonid Yegoshin wrote:
On 01/14/2016 04:14 AM, Will Deacon wrote:
quoted
On Wed, Jan 13, 2016 at 02:26:16PM -0800, Leonid Yegoshin wrote:
quoted
Moreover, there are voices against guarantee that it will be in future
and that voices point me to Documentation/memory-barriers.txt section "DATA
DEPENDENCY BARRIERS" examples which require SYNC_RMB between loading
address/index and using that for loading data based on that address or index
for shared data (look on CPU2 pseudo-code):
quoted
To deal with this, a data dependency barrier or better must be inserted
between the address load and the data load:
CPU 1 CPU 2
=============== ===============
{ A == 1, B == 2, C = 3, P == &A, Q == &C }
B = 4;
<write barrier>
WRITE_ONCE(P, &B);
Q = READ_ONCE(P);
<data dependency barrier> <-----------
SYNC_RMB is here
D = *Q;
...
quoted
Another example of where data dependency barriers might be required is
where a
number is read from memory and then used to calculate the index for an
array
access:
CPU 1 CPU 2
=============== ===============
{ M[0] == 1, M[1] == 2, M[3] = 3, P == 0, Q == 3 }
M[1] = 4;
<write barrier>
WRITE_ONCE(P, 1);
Q = READ_ONCE(P);
<data dependency barrier> <------------
SYNC_RMB is here
D = M[Q];
That voices say that there is a legitimate reason to relax HW here for
performance if SYNC_RMB is needed anyway to work with this sequence of
shared data.
Are you saying that MIPS needs to implement [smp_]read_barrier_depends?
It is not me, it is Documentation/memory-barriers.txt from kernel sources.
HW team can't work on voice statements, it should do a work on
written documents. If that is written (see above the lines which I
marked by "SYNC_RMB") then anybody should use it and never mind how
many CPUs/Threads are in play. This examples explicitly requires to
insert "data dependency barrier" between reading a shared
pointer/index and using it to fetch a shared data. So, your
WRC+addr+addr test is a violation of that recommendation.
Perhaps Documentation/memory-barriers.txt needs additional clarification.
It would not be the first time.
If your CPU implicitly maintains ordering based on address and
data dependencies, then you don't need any instructions for
<data dependency barrier>.
The WRC+addr+addr is OK because data dependencies are not required to be
transitive, in other words, they are not required to flow from one CPU to
another without the help of an explicit memory barrier. Transitivity is
instead supplied by smp_mb() and by smp_store_release()-smp_load_acquire()
chains. Here is the Linux kernel code for WRC+addr+addr, give or take
(and no, I have no idea why anyone would want to write code like this):
struct foo {
struct foo **a;
};
struct foo b;
struct foo c;
struct foo d;
struct foo e;
struct foo f = { &d };
struct foo g = { &e };
struct foo *x = &b;
void cpu0(void)
{
WRITE_ONCE(x, &f);
}
void cpu1(void)
{
struct foo *p;
p = lockless_dereference(x);
WRITE_ONCE(p->a, &x);
}
void cpu2(void)
{
r1 = lockless_dereference(f.a);
WRITE_ONCE(*r1, &c);
}
It is legal to end the run with x==&f and r1==&x. To prevent this outcome,
we do the following:
struct foo {
struct foo **a;
};
struct foo b;
struct foo c;
struct foo d;
struct foo e;
struct foo f = { &d };
struct foo g = { &e };
struct foo *x = &b;
void cpu0(void)
{
WRITE_ONCE(x, &f);
}
void cpu1(void)
{
struct foo *p;
p = lockless_dereference(x);
smp_store_release(&p->a, &x); /* Additional ordering. */
}
void cpu2(void)
{
r1 = lockless_dereference(f.a);
WRITE_ONCE(*r1, &c);
}
And I still don't know why anyone would need this sort of code. ;-)
Alternatively, we pull cpu2() into cpu1():
struct foo {
struct foo **a;
};
struct foo b;
struct foo c;
struct foo d;
struct foo e;
struct foo f = { &d };
struct foo g = { &e };
struct foo *x = &b;
void cpu0(void)
{
WRITE_ONCE(x, &f);
}
void cpu1(void)
{
struct foo *p;
p = lockless_dereference(x);
WRITE_ONCE(p->a, &x);
r1 = lockless_dereference(f.a);
WRITE_ONCE(*r1, &c);
}
The ordering is now enforced by being within a single thread. In fact,
the second lockless_dereference() can be READ_ONCE().
So, does MIPS maintain ordering within a given CPU based on address and
data dependencies? If so, you don't need to emit memory-barrier instructions
for read_barrier_depends().
Thanx, Paul
From: Paul E. McKenney <hidden> Date: 2016-01-14 20:36:59
On Thu, Jan 14, 2016 at 09:15:13PM +0100, Peter Zijlstra wrote:
On Thu, Jan 14, 2016 at 11:42:02AM -0800, Leonid Yegoshin wrote:
quoted
An the only point - please use an appropriate SYNC_* barriers instead of
heavy bold hammer. That stuff was design explicitly to support the
requirements of Documentation/memory-barriers.txt
That's madness. That document changes from version to version as to what
we _think_ the actual hardware does. It is _NOT_ a specification.
There is work in progress on a specification, but please don't hold
your breath. And I am not as optimistic as I might be about any formal
specification keeping up with the Linux kernel or with the hardware that
it supports. But it seems worth a good try.
You cannot design hardware from that. Its incomplete and fails to
specify a bunch of things. It not a mathematically sound definition of a
memory model.
Please stop referring to that document for what a particular barrier
_should_ do. Explain what MIPS does, so we can attempt to integrate
this knowledge with our knowledge of PPC/ARM/Alpha/x86/etc. and improve
upon our understanding of hardware and improve the Linux memory model.
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-14 20:46:46
On Thu, Jan 14, 2016 at 09:15:13PM +0100, Peter Zijlstra wrote:
On Thu, Jan 14, 2016 at 11:42:02AM -0800, Leonid Yegoshin wrote:
quoted
An the only point - please use an appropriate SYNC_* barriers instead of
heavy bold hammer. That stuff was design explicitly to support the
requirements of Documentation/memory-barriers.txt
That's madness. That document changes from version to version as to what
we _think_ the actual hardware does. It is _NOT_ a specification.
You cannot design hardware from that. Its incomplete and fails to
specify a bunch of things. It not a mathematically sound definition of a
memory model.
Please stop referring to that document for what a particular barrier
_should_ do. Explain what MIPS does, so we can attempt to integrate
this knowledge with our knowledge of PPC/ARM/Alpha/x86/etc. and improve
upon our understanding of hardware and improve the Linux memory model.
That is, if you'd managed to read that file at the right point in time,
you might have through we'd be OK with requiring a barrier for
control dependencies.
We got rid of that mistake. It was based on a flawed reading of the
Alpha docs. See: 105ff3cbf225 ("atomic: remove all traces of
READ_ONCE_CTRL() and atomic*_read_ctrl()")
Similarly, while the document goes to great length to explain the
read_barrier_depends thing, nobody actually thinks its a brilliant idea
to have. Ideally we'd kill the thing the moment we drop Alpha support.
Again, memory-barriers.txt is _NOT_, I repeat, _NOT_ a hardware spec, it
is not even a recommendation. It are our best effort (but flawed)
scribbles of what we think is makes sense given the huge amount of
actual hardware we have to run on.
As to the ACQUIRE/RELEASE semantics, ARM64 actually has
multi-copy-atomic acquire/release (as does ia64, although in reality it
doesn't actually have acquire/release). PPC otoh does _NOT_ have this,
and is currently the only arch to suffer RCpc locks.
Now for a long long time we assumed our locks were RCsc, and we've
written code assuming UNLOCK x + LOCK y was in fact a full barrier with
transitiviy. Then we figured out PPC didn't actually match that. RCU is
the only piece of code we _know_ relied on that, but there might be more
out there...
So we document, for new code, that UNLOCK+LOCK isn't a MB, while at the
same time we lobby PPC to stick a full barrier in and get rid of this
stuff.
Nobody really likes RCpc locks, esp. given the history we have of
assuming RCsc.
The current document allowing for RCpc is not an endorsement thereof.
Ideally we'd _NOT_ have to worry about that. We can do without these
head-aches.
So again, stop referring to our document as a spec. Also please don't
make MIPS push the limits of weak memory models, we really can do
without the pain.