From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2015-12-31 19:05:53
Changes since v1:
- replaced my 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, but the inclusion of patch 1 from tip
creates a merge conflict (even though it's easy to resolve).
Would tip maintainers prefer merging it through tip tree instead
(including the virtio patches)?
Or should I just merge it all through my tree, including the
duplicate patch, and assume conflict will be resolved?
If the second, acks will be appreciated.
Thanks!
This is really 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?
Arnd, if you agree, could you ack this please?
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, even with new barriers and extra documentation :)
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 rest of 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.
-. Patches 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.
I couldn't fully build sh,tile,xtensa but I did this test
kernel/rcu/tree.o kernel/sched/wait.o and
kernel/futex.o and tested these instead.
Unfortunately, I don't have a metag cross-build toolset ready.
Hoping for some acks on this architecture.
Finally, 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,33,34 convert virtio xen drivers to use the virt_xxx APIs
xen patches are untested
virtio ones have been tested on x86
-. Patches 31-32 teach virtio to use virt_store_mb
sh architecture was missing a 2-byte smp_store_mb,
the fix is trivial although my code is not optimal:
if anyone cares, pls send me a patch to apply on top.
I didn't build this architecture, but intel's 0-day
infrastructure builds it.
tested on x86
Davidlohr Bueso (1):
lcoking/barriers, arch: Use smp barriers in smp_store_release()
Michael S. Tsirkin (33):
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 __smp_XXX
sh: support a 2-byte smp_store_mb
virtio_ring: use virt_store_mb
xenbus: use virt_xxx barriers
xen/io: use virt_xxx 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 | 25 ++++-----
arch/sh/include/asm/barrier.h | 11 +++-
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 | 102 ++++++++++++++++++++++++++++++++----
include/linux/virtio_ring.h | 22 +++++---
include/xen/interface/io/ring.h | 16 +++---
arch/ia64/kernel/iosapic.c | 6 +--
drivers/virtio/virtio_ring.c | 15 +++---
drivers/xen/xenbus/xenbus_comms.c | 8 +--
Documentation/memory-barriers.txt | 28 ++++++++--
23 files changed, 266 insertions(+), 279 deletions(-)
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2015-12-31 19:06:06
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: 2015-12-31 19:06:20
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: 2015-12-31 19:06:27
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: 2015-12-31 19:06:38
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: 2015-12-31 19:06:43
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: 2015-12-31 19:06:56
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>
---
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: 2015-12-31 19:07:06
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>
---
arch/arm/include/asm/barrier.h | 23 +----------------------
1 file changed, 1 insertion(+), 22 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2015-12-31 19:07:10
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: 2015-12-31 19:07:17
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: 2015-12-31 19:07:27
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: 2015-12-31 19:07:34
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>
---
arch/x86/um/asm/barrier.h | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2015-12-31 19:07:42
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: 2015-12-31 19:07:49
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: 2015-12-31 19:07:58
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>
---
arch/powerpc/include/asm/barrier.h | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2015-12-31 19:08:07
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: 2015-12-31 19:08:19
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>
---
arch/arm/include/asm/barrier.h | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2015-12-31 19:08:28
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: 2015-12-31 19:08:31
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: 2015-12-31 19:08:40
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: 2015-12-31 19:08:55
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>
---
arch/s390/include/asm/barrier.h | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2015-12-31 19:09:00
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: 2015-12-31 19:09:11
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>
---
arch/sparc/include/asm/barrier_64.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2015-12-31 19:09:21
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: 2015-12-31 19:09:32
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: 2015-12-31 19:09:38
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: 2015-12-31 19:09:48
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: 2015-12-31 19:10:00
At the moment, xchg on sh only supports 4 and 1 byte values, so using it
from smp_store_mb means attempts to store a 2 byte value using this
macro fail.
And happens to be exactly what virtio drivers want to do.
Check size and fall back to a slower, but safe, WRITE_ONCE+smp_mb.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/sh/include/asm/barrier.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2015-12-31 19:10:10
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 | 12 ++++++++++++
drivers/virtio/virtio_ring.c | 15 +++++++++------
2 files changed, 21 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: 2015-12-31 19:10:24
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>
---
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: 2015-12-31 19:10:50
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>
---
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: 2015-12-31 19:11:56
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: 2015-12-31 19:12:50
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: 2015-12-31 19:16:11
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: David Miller <davem@davemloft.net> Date: 2015-12-31 19:43:57
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 31 Dec 2015 21:06:38 +0200
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>
From: David Miller <davem@davemloft.net> Date: 2015-12-31 19:44:53
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 31 Dec 2015 21:08:53 +0200
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>
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-01 09:39:54
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 __smp_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-01 10:22:04
On Fri, Jan 01, 2016 at 11:39:40AM +0200, Michael S. Tsirkin wrote:
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 __smp_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>
The subject and commit log should say
virt_xxx and not __smp_xxx - I fixed this up in
my tree.
From: Sergei Shtylyov <hidden> Date: 2016-01-01 17:23:56
Hello.
On 12/31/2015 10:09 PM, Michael S. Tsirkin wrote:
quoted hunk
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 | 12 ++++++++++++
drivers/virtio/virtio_ring.c | 15 +++++++++------
2 files changed, 21 insertions(+), 6 deletions(-)
From: Russell King - ARM Linux <hidden> Date: 2016-01-02 11:20:41
On Thu, Dec 31, 2015 at 09:06:46PM +0200, Michael S. Tsirkin wrote:
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>
Thanks, the asm-generic versions looks identical to me, so this should
result in no code generation difference.
Acked-by: Russell King <redacted>
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
From: Russell King - ARM Linux <hidden> Date: 2016-01-02 11:24:51
On Thu, Dec 31, 2015 at 09:07:59PM +0200, Michael S. Tsirkin wrote:
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>
In combination with patch 14, this looks like it should result in no
change to the resulting code.
Acked-by: Russell King <redacted>
My only concern is that 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 (been there with
other arch stuff before.)
I wonder whether we should consider making the smp memory barriers
inline functions, so these __smp_xxx() variants can be undef'd
afterwards, thereby preventing drivers getting their hands on these
new macros?
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-03 09:01:21
On Fri, Jan 01, 2016 at 08:23:46PM +0300, Sergei Shtylyov wrote:
Hello.
On 12/31/2015 10:09 PM, Michael S. Tsirkin wrote:
quoted
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 | 12 ++++++++++++
drivers/virtio/virtio_ring.c | 15 +++++++++------
2 files changed, 21 insertions(+), 6 deletions(-)
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-03 09:12:58
On Sat, Jan 02, 2016 at 11:24:38AM +0000, Russell King - ARM Linux wrote:
On Thu, Dec 31, 2015 at 09:07:59PM +0200, Michael S. Tsirkin wrote:
quoted
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>
In combination with patch 14, this looks like it should result in no
change to the resulting code.
Acked-by: Russell King <redacted>
My only concern is that 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 (been there with
other arch stuff before.)
I wonder whether we should consider making the smp memory barriers
inline functions, so these __smp_xxx() variants can be undef'd
afterwards, thereby preventing drivers getting their hands on these
new macros?
That'd be tricky to do cleanly since asm-generic depends on
ifndef to add generic variants where needed.
But it would be possible to add a checkpatch test for this.
From: David Vrabel <hidden> Date: 2016-01-04 11:32:11
On 31/12/15 19:10, Michael S. Tsirkin wrote:
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.
Acked-by: David Vrabel <redacted>
If you're feeling particularly keen there's a rmb() consume_one_event()
in drivers/xen/events/events_fifo.c that can be converted to virt_rmb()
as well.
David
From: David Vrabel <hidden> Date: 2016-01-04 11:32:38
On 31/12/15 19:10, Michael S. Tsirkin wrote:
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.
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>
Reviewed-by: Stefano Stabellini <redacted>
Are you also going to take care of
drivers/xen/grant-table.c
drivers/xen/evtchn.c
drivers/xen/events/events_fifo.c
drivers/xen/xen-scsiback.c
drivers/xen/tmem.c
drivers/xen/xen-pciback/pci_stub.c
drivers/xen/xen-pciback/pciback_ops.c
?
@@ -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);
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>
@@ -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: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-04 13:20:58
On Thu, Dec 31, 2015 at 09:06:30PM +0200, Michael S. Tsirkin wrote:
quoted hunk
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(-)
@@ -30,14 +30,6 @@#define smp_rmb() rmb()#define smp_wmb() wmb()-#define read_barrier_depends() do { } while (0)-#define smp_read_barrier_depends() do { } while (0)--#define smp_mb__before_atomic() smp_mb()-#define smp_mb__after_atomic() smp_mb()
As per:
lkml.kernel.org/r/20150921112252.3c2937e1@mschwide
s390 should change this to barrier() instead of smp_mb() and hence
should not use the generic versions.
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-04 13:37:13
On Sun, Jan 03, 2016 at 11:12:44AM +0200, Michael S. Tsirkin wrote:
On Sat, Jan 02, 2016 at 11:24:38AM +0000, Russell King - ARM Linux wrote:
quoted
My only concern is that 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 (been there with
other arch stuff before.)
I wonder whether we should consider making the smp memory barriers
inline functions, so these __smp_xxx() variants can be undef'd
afterwards, thereby preventing drivers getting their hands on these
new macros?
That'd be tricky to do cleanly since asm-generic depends on
ifndef to add generic variants where needed.
But it would be possible to add a checkpatch test for this.
Wasn't the whole purpose of these things for 'drivers' (namely
virtio/xen hypervisor interaction) to use these?
And I suppose most of virtio would actually be modules, so you cannot do
what I did with preempt_enable_no_resched() either.
But yes, it would be good to limit the use of these things.
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-04 13:45:58
On Thu, Dec 31, 2015 at 09:08:38PM +0200, Michael S. Tsirkin wrote:
quoted hunk
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>
---
arch/s390/include/asm/barrier.h | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-04 13:54:42
On Mon, Jan 04, 2016 at 02:36:58PM +0100, Peter Zijlstra wrote:
On Sun, Jan 03, 2016 at 11:12:44AM +0200, Michael S. Tsirkin wrote:
quoted
On Sat, Jan 02, 2016 at 11:24:38AM +0000, Russell King - ARM Linux wrote:
quoted
quoted
My only concern is that 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 (been there with
other arch stuff before.)
I wonder whether we should consider making the smp memory barriers
inline functions, so these __smp_xxx() variants can be undef'd
afterwards, thereby preventing drivers getting their hands on these
new macros?
That'd be tricky to do cleanly since asm-generic depends on
ifndef to add generic variants where needed.
But it would be possible to add a checkpatch test for this.
Wasn't the whole purpose of these things for 'drivers' (namely
virtio/xen hypervisor interaction) to use these?
Ah, I see, you add virt_*mb() stuff later on for that use case.
So, assuming everybody does include asm-generic/barrier.h, you could
simply #undef the __smp version at the end of that, once we've generated
all the regular primitives from it, no?
From: Russell King - ARM Linux <hidden> Date: 2016-01-04 13:59:58
On Mon, Jan 04, 2016 at 02:54:20PM +0100, Peter Zijlstra wrote:
On Mon, Jan 04, 2016 at 02:36:58PM +0100, Peter Zijlstra wrote:
quoted
On Sun, Jan 03, 2016 at 11:12:44AM +0200, Michael S. Tsirkin wrote:
quoted
On Sat, Jan 02, 2016 at 11:24:38AM +0000, Russell King - ARM Linux wrote:
quoted
quoted
My only concern is that 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 (been there with
other arch stuff before.)
I wonder whether we should consider making the smp memory barriers
inline functions, so these __smp_xxx() variants can be undef'd
afterwards, thereby preventing drivers getting their hands on these
new macros?
That'd be tricky to do cleanly since asm-generic depends on
ifndef to add generic variants where needed.
But it would be possible to add a checkpatch test for this.
Wasn't the whole purpose of these things for 'drivers' (namely
virtio/xen hypervisor interaction) to use these?
Ah, I see, you add virt_*mb() stuff later on for that use case.
So, assuming everybody does include asm-generic/barrier.h, you could
simply #undef the __smp version at the end of that, once we've generated
all the regular primitives from it, no?
Not so simple - that's why I mentioned using inline functions.
The new smp_* _macros_ are:
+#define smp_mb() __smp_mb()
which means if we simply #undef __smp_mb(), smp_mb() then points at
something which is no longer available, and we'll end up with errors
saying that __smp_mb() doesn't exist.
My suggestion was to change:
#ifndef smp_mb
#define smp_mb() __smp_mb()
#endif
to:
#ifndef smp_mb
static inline void smp_mb(void)
{
__smp_mb();
}
#endif
which then means __smp_mb() and friends can be #undef'd afterwards.
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-04 14:05:18
On Thu, Dec 31, 2015 at 09:09:47PM +0200, Michael S. Tsirkin wrote:
quoted hunk
At the moment, xchg on sh only supports 4 and 1 byte values, so using it
from smp_store_mb means attempts to store a 2 byte value using this
macro fail.
And happens to be exactly what virtio drivers want to do.
Check size and fall back to a slower, but safe, WRITE_ONCE+smp_mb.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/sh/include/asm/barrier.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -32,7 +32,15 @@#define ctrl_barrier() __asm__ __volatile__ ("nop;nop;nop;nop;nop;nop;nop;nop")#endif-#define __smp_store_mb(var, value) do { (void)xchg(&var, value); } while (0)+#define __smp_store_mb(var, value) do { \+if(sizeof(var)!=4&&sizeof(var)!=1){\+WRITE_ONCE(var,value);\+__smp_mb();\+}else{\+(void)xchg(&var,value);\+}\+}while(0)
So SH is an orphaned arch, which is also why I did not comment on using
xchg() for the UP smp_store_mb() thing.
But I really think we should try fixing the xchg() implementation
instead of this duct-tape.
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-04 14:10:00
On Thu, Dec 31, 2015 at 09:10:01PM +0200, Michael S. Tsirkin wrote:
quoted hunk
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>
---
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();
So its possible to remove this barrier entirely, see the "CONTROL
DEPENDNCIES" chunk of memory-barrier.txt.
But do that in a separate patch series and only if you really really
need the performance.
From: Martin Schwidefsky <hidden> Date: 2016-01-04 15:05:31
On Mon, 4 Jan 2016 14:20:42 +0100
Peter Zijlstra [off-list ref] wrote:
On Thu, Dec 31, 2015 at 09:06:30PM +0200, Michael S. Tsirkin wrote:
quoted
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(-)
@@ -30,14 +30,6 @@#define smp_rmb() rmb()#define smp_wmb() wmb()-#define read_barrier_depends() do { } while (0)-#define smp_read_barrier_depends() do { } while (0)--#define smp_mb__before_atomic() smp_mb()-#define smp_mb__after_atomic() smp_mb()
As per:
lkml.kernel.org/r/20150921112252.3c2937e1@mschwide
s390 should change this to barrier() instead of smp_mb() and hence
should not use the generic versions.
Yes, we wanted to simplify this. Thanks for the reminder, I'll queue
a patch.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
From: James Hogan <hidden> Date: 2016-01-04 15:26:08
Hi Peter,
On Mon, Jan 04, 2016 at 02:41:28PM +0100, Peter Zijlstra wrote:
On Thu, Dec 31, 2015 at 09:08:22PM +0200, Michael S. Tsirkin wrote:
quoted
+#ifdef CONFIG_SMP
+#define fence() metag_fence()
+#else
+#define fence() do { } while (0)
#endif
James, it strikes me as odd that fence() is a no-op instead of a
barrier() for UP, can you verify/explain?
fence() is an unfortunate workaround for a specific issue on a certain
SoC, where writes from different hw threads get reordered outside of the
core, resulting in incoherency between RAM and cache. It has slightly
different semantics to the normal SMP barriers, since I was assured it
is required before a write rather than after it.
Here's the comment:
This is needed before a write to shared memory in a critical section,
to prevent external reordering of writes before the fence on other
threads with writes after the fence on this thread (and to prevent the
ensuing cache-memory incoherence). It is therefore ineffective if used
after and on the same thread as a write.
It is used along with the metag specific __global_lock1() (global
voluntary lock between hw threads) whenever a write is performed, and by
smp_mb/smp_rmb to try to catch other cases, but I've never been
confident this fixes every single corner case, since there could be
other places where multiple CPUs perform unsynchronised writes to the
same memory location, and expect cache not to become incoherent at that
location.
It seemed to be sufficient to achieve stability however, and SMP on Meta
Linux never made it into a product anyway, since the other hw thread
tended to be used for RTOS stuff, so it didn't seem worth extending the
generic barrier API for it.
Cheers
James
From: Peter Zijlstra <peterz@infradead.org> Date: 2016-01-04 15:30:48
On Mon, Jan 04, 2016 at 03:25:58PM +0000, James Hogan wrote:
It is used along with the metag specific __global_lock1() (global
voluntary lock between hw threads) whenever a write is performed, and by
smp_mb/smp_rmb to try to catch other cases, but I've never been
confident this fixes every single corner case, since there could be
other places where multiple CPUs perform unsynchronised writes to the
same memory location, and expect cache not to become incoherent at that
location.
Ah, yuck, I thought blackfin was the only one attempting !coherent SMP.
And yes, this is bound to break in lots of places in subtle ways. We
very much assume cache coherency for SMP in generic code.
It seemed to be sufficient to achieve stability however, and SMP on Meta
Linux never made it into a product anyway, since the other hw thread
tended to be used for RTOS stuff, so it didn't seem worth extending the
generic barrier API for it.
*phew*, should we take it out then, just to be sure nobody accidentally
tries to use it then?
From: James Hogan <hidden> Date: 2016-01-04 16:05:04
On Mon, Jan 04, 2016 at 04:30:36PM +0100, Peter Zijlstra wrote:
On Mon, Jan 04, 2016 at 03:25:58PM +0000, James Hogan wrote:
quoted
It is used along with the metag specific __global_lock1() (global
voluntary lock between hw threads) whenever a write is performed, and by
smp_mb/smp_rmb to try to catch other cases, but I've never been
confident this fixes every single corner case, since there could be
other places where multiple CPUs perform unsynchronised writes to the
same memory location, and expect cache not to become incoherent at that
location.
Ah, yuck, I thought blackfin was the only one attempting !coherent SMP.
And yes, this is bound to break in lots of places in subtle ways. We
very much assume cache coherency for SMP in generic code.
Well, its usually completely coherent, its just a bit dodgy in a
particular hardware corner case, which was pretty hard to hit, even
without these workarounds.
quoted
It seemed to be sufficient to achieve stability however, and SMP on Meta
Linux never made it into a product anyway, since the other hw thread
tended to be used for RTOS stuff, so it didn't seem worth extending the
generic barrier API for it.
*phew*, should we take it out then, just to be sure nobody accidentally
tries to use it then?
SMP support on this SoC you mean? I doubt it'll be a problem tbh, and
it'd work fine in QEMU when emulating this SoC, so I'd prefer to keep it
in.
Cheers
James
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-04 20:13:08
On Mon, Jan 04, 2016 at 02:36:58PM +0100, Peter Zijlstra wrote:
On Sun, Jan 03, 2016 at 11:12:44AM +0200, Michael S. Tsirkin wrote:
quoted
On Sat, Jan 02, 2016 at 11:24:38AM +0000, Russell King - ARM Linux wrote:
quoted
quoted
My only concern is that 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 (been there with
other arch stuff before.)
I wonder whether we should consider making the smp memory barriers
inline functions, so these __smp_xxx() variants can be undef'd
afterwards, thereby preventing drivers getting their hands on these
new macros?
That'd be tricky to do cleanly since asm-generic depends on
ifndef to add generic variants where needed.
But it would be possible to add a checkpatch test for this.
Wasn't the whole purpose of these things for 'drivers' (namely
virtio/xen hypervisor interaction) to use these?
My take out from discussion with you was that virtualization is probably
the only valid use-case. So at David Miller's suggestion there's a
patch later in the series that adds virt_xxxx wrappers and these are
then used by virtio xen and later maybe others.
And I suppose most of virtio would actually be modules, so you cannot do
what I did with preempt_enable_no_resched() either.
But yes, it would be good to limit the use of these things.
Right so the trick is checkpatch warns about use of
__smp_xxx and hopefully people are not crazy enough
to use virt_xxx variants for non-virtual drivers.
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-04 20:19:12
On Mon, Jan 04, 2016 at 02:45:25PM +0100, Peter Zijlstra wrote:
On Thu, Dec 31, 2015 at 09:08:38PM +0200, Michael S. Tsirkin wrote:
quoted
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>
---
arch/s390/include/asm/barrier.h | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
Why define the smp_*mb() primitives here? Would not the inclusion of
asm-generic/barrier.h do this?
No because the generic one is a nop on !SMP, this one isn't.
Pls note this patch is just reordering code without making
functional changes.
And at the moment, on s390 smp_xxx barriers are always non empty.
Some of this could be sub-optimal, but
since on s390 Linux always runs on a hypervisor,
I am not sure it's safe to use the generic version -
in other words, it just might be that for s390 smp_ and virt_
barriers must be equivalent.
If in fact this turns out to be wrong, I can pick up
a patch to change this, but I'd rather make this
a patch on top so that my patches are testable
just by compiling and comparing the binary.
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-04 20:35:12
On Mon, Jan 04, 2016 at 02:20:42PM +0100, Peter Zijlstra wrote:
On Thu, Dec 31, 2015 at 09:06:30PM +0200, Michael S. Tsirkin wrote:
quoted
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(-)
@@ -30,14 +30,6 @@#define smp_rmb() rmb()#define smp_wmb() wmb()-#define read_barrier_depends() do { } while (0)-#define smp_read_barrier_depends() do { } while (0)--#define smp_mb__before_atomic() smp_mb()-#define smp_mb__after_atomic() smp_mb()
As per:
lkml.kernel.org/r/20150921112252.3c2937e1@mschwide
s390 should change this to barrier() instead of smp_mb() and hence
should not use the generic versions.
Thanks Peter!
OK so I will just rename this to __smp_mb__before_atomic and
__smp_mb__after_atomic but keep them around.
I'm not changing these - that's best left to s390 maintainers.
Should I add a TODO comment to change them to barrier so
we don't forget?
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-04 20:39:32
On Mon, Jan 04, 2016 at 02:54:20PM +0100, Peter Zijlstra wrote:
On Mon, Jan 04, 2016 at 02:36:58PM +0100, Peter Zijlstra wrote:
quoted
On Sun, Jan 03, 2016 at 11:12:44AM +0200, Michael S. Tsirkin wrote:
quoted
On Sat, Jan 02, 2016 at 11:24:38AM +0000, Russell King - ARM Linux wrote:
quoted
quoted
My only concern is that 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 (been there with
other arch stuff before.)
I wonder whether we should consider making the smp memory barriers
inline functions, so these __smp_xxx() variants can be undef'd
afterwards, thereby preventing drivers getting their hands on these
new macros?
That'd be tricky to do cleanly since asm-generic depends on
ifndef to add generic variants where needed.
But it would be possible to add a checkpatch test for this.
Wasn't the whole purpose of these things for 'drivers' (namely
virtio/xen hypervisor interaction) to use these?
Ah, I see, you add virt_*mb() stuff later on for that use case.
So, assuming everybody does include asm-generic/barrier.h, you could
simply #undef the __smp version at the end of that, once we've generated
all the regular primitives from it, no?
Maybe I misunderstand, but I don't think so:
------>
#define __smp_xxx FOO
#define smp_xxx __smp_xxx
#undef __smp_xxx
smp_xxx
<------
resolves to __smp_xxx, not FOO.
That's why I went the checkpatch way.
--
MST
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-04 20:42:59
On Mon, Jan 04, 2016 at 04:03:39PM +0100, Martin Schwidefsky wrote:
On Mon, 4 Jan 2016 14:20:42 +0100
Peter Zijlstra [off-list ref] wrote:
quoted
On Thu, Dec 31, 2015 at 09:06:30PM +0200, Michael S. Tsirkin wrote:
quoted
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(-)
@@ -30,14 +30,6 @@#define smp_rmb() rmb()#define smp_wmb() wmb()-#define read_barrier_depends() do { } while (0)-#define smp_read_barrier_depends() do { } while (0)--#define smp_mb__before_atomic() smp_mb()-#define smp_mb__after_atomic() smp_mb()
As per:
lkml.kernel.org/r/20150921112252.3c2937e1@mschwide
s390 should change this to barrier() instead of smp_mb() and hence
should not use the generic versions.
Yes, we wanted to simplify this. Thanks for the reminder, I'll queue
a patch.
Could you base on my patchset maybe, to avoid conflicts,
and I'll merge it?
Or if it's just replacing these 2 with barrier() I can do this
myself easily.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
From: James Hogan <hidden> Date: 2016-01-04 23:24:15
On Thu, Dec 31, 2015 at 09:07:02PM +0200, Michael S. Tsirkin wrote:
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>
Looks good, and confirmed no text change (once patch 1 is applied that
is).
Acked-by: James Hogan <redacted>
Thanks
James
From: James Hogan <hidden> Date: 2016-01-05 00:09:39
Hi Michael,
On Thu, Dec 31, 2015 at 09:08:22PM +0200, Michael S. Tsirkin wrote:
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.
I'm not a fan of the inconsistent commit message wrapping. I wrap to 72
columns (although I now notice SubmittingPatches says to use 75...).
!SMP kernel text differs, but only because of new presence of unused
metag_fence() inline function. If I #if 0 that out, then it matches, so
thats fine.
quoted hunk
-
#ifdef CONFIG_METAG_SMP_WRITE_REORDERING
/*
* Write to the atomic memory unlock system event register (command 0). This is
@@ -60,26 +53,31 @@ static inline void wr_fence(void) * incoherence). It is therefore ineffective if used after and on the same * thread as a write. */-static inline void fence(void)+static inline void metag_fence(void) { volatile int *flushptr = (volatile int *) LINSYSEVENT_WR_ATOMIC_UNLOCK; barrier(); *flushptr = 0; barrier(); }-#define smp_mb() fence()-#define smp_rmb() fence()-#define smp_wmb() barrier()+#define __smp_mb() metag_fence()+#define __smp_rmb() metag_fence()+#define __smp_wmb() barrier() #else-#define fence() do { } while (0)-#define smp_mb() barrier()-#define smp_rmb() barrier()-#define smp_wmb() barrier()+#define metag_fence() do { } while (0)+#define __smp_mb() barrier()+#define __smp_rmb() barrier()+#define __smp_wmb() barrier()
Whitespace is now messed up. Admitedly its already inconsistent
tabs/spaces, but it'd be nice if the definitions at least still all
lined up. You're touching all the definitions which use spaces anyway,
so feel free to convert them to tabs while you're at it.
Other than those niggles, it looks sensible to me:
Acked-by: James Hogan <redacted>
Cheers
James
Hi Michael,
On Thu, Dec 31, 2015 at 09:07:42PM +0200, Michael S. Tsirkin wrote:
quoted hunk
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>
---
arch/powerpc/include/asm/barrier.h | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
, therefore this will emit an lwsync no matter SMP or UP.
Another thing is that smp_lwsync() may have a third user(other than
smp_load_acquire() and smp_store_release()):
http://article.gmane.org/gmane.linux.ports.ppc.embedded/89877
I'm OK to change my patch accordingly, but do we really want
smp_lwsync() get involved in this cleanup? If I understand you
correctly, this cleanup focuses on external API like smp_{r,w,}mb(),
while smp_lwsync() is internal to PPC.
Regards,
Boqun
WRITE_ONCE(*p, v); \
} while (0)
-#define smp_load_acquire(p) \
+#define __smp_load_acquire(p) \
({ \
typeof(*p) ___p1 = READ_ONCE(*p); \
compiletime_assert_atomic_type(*p); \
- smp_lwsync(); \
+ __smp_lwsync(); \
___p1; \
})
--
MST
--
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/
From: Martin Schwidefsky <hidden> Date: 2016-01-05 08:03:20
On Mon, 4 Jan 2016 22:42:44 +0200
"Michael S. Tsirkin" [off-list ref] wrote:
On Mon, Jan 04, 2016 at 04:03:39PM +0100, Martin Schwidefsky wrote:
quoted
On Mon, 4 Jan 2016 14:20:42 +0100
Peter Zijlstra [off-list ref] wrote:
quoted
On Thu, Dec 31, 2015 at 09:06:30PM +0200, Michael S. Tsirkin wrote:
quoted
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(-)
@@ -30,14 +30,6 @@#define smp_rmb() rmb()#define smp_wmb() wmb()-#define read_barrier_depends() do { } while (0)-#define smp_read_barrier_depends() do { } while (0)--#define smp_mb__before_atomic() smp_mb()-#define smp_mb__after_atomic() smp_mb()
As per:
lkml.kernel.org/r/20150921112252.3c2937e1@mschwide
s390 should change this to barrier() instead of smp_mb() and hence
should not use the generic versions.
Yes, we wanted to simplify this. Thanks for the reminder, I'll queue
a patch.
Could you base on my patchset maybe, to avoid conflicts,
and I'll merge it?
Or if it's just replacing these 2 with barrier() I can do this
myself easily.
Probably the easiest solution if you do the patch yourself and
include it in your patch set.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
From: Martin Schwidefsky <hidden> Date: 2016-01-05 08:13:39
On Mon, 4 Jan 2016 22:18:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:
On Mon, Jan 04, 2016 at 02:45:25PM +0100, Peter Zijlstra wrote:
quoted
On Thu, Dec 31, 2015 at 09:08:38PM +0200, Michael S. Tsirkin wrote:
quoted
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>
---
arch/s390/include/asm/barrier.h | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
Why define the smp_*mb() primitives here? Would not the inclusion of
asm-generic/barrier.h do this?
No because the generic one is a nop on !SMP, this one isn't.
Pls note this patch is just reordering code without making
functional changes.
And at the moment, on s390 smp_xxx barriers are always non empty.
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.
Some of this could be sub-optimal, but
since on s390 Linux always runs on a hypervisor,
I am not sure it's safe to use the generic version -
in other words, it just might be that for s390 smp_ and virt_
barriers must be equivalent.
The definition of the memory barriers is independent from the fact
if the system is running on an hypervisor or not. Is there really
an architecture where you need special virt_xxx barriers?!?
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-05 08:51:36
On Tue, Jan 05, 2016 at 09:36:55AM +0800, Boqun Feng wrote:
Hi Michael,
On Thu, Dec 31, 2015 at 09:07:42PM +0200, Michael S. Tsirkin wrote:
quoted
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
I think this is the part that was missed in review.
quoted
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>
---
arch/powerpc/include/asm/barrier.h | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
, therefore this will emit an lwsync no matter SMP or UP.
Absolutely. But smp_store_release (without __) will not.
Please note I did test this: for ppc code before and after
this patch generates exactly the same binary on SMP and UP.
Another thing is that smp_lwsync() may have a third user(other than
smp_load_acquire() and smp_store_release()):
http://article.gmane.org/gmane.linux.ports.ppc.embedded/89877
I'm OK to change my patch accordingly, but do we really want
smp_lwsync() get involved in this cleanup? If I understand you
correctly, this cleanup focuses on external API like smp_{r,w,}mb(),
while smp_lwsync() is internal to PPC.
Regards,
Boqun
I think you missed the leading ___ :)
smp_store_release is external and it needs __smp_lwsync as
defined here.
I can duplicate some code and have smp_lwsync *not* call __smp_lwsync
but why do this? Still, if you prefer it this way,
please let me know.
quoted
WRITE_ONCE(*p, v); \
} while (0)
-#define smp_load_acquire(p) \
+#define __smp_load_acquire(p) \
({ \
typeof(*p) ___p1 = READ_ONCE(*p); \
compiletime_assert_atomic_type(*p); \
- smp_lwsync(); \
+ __smp_lwsync(); \
___p1; \
})
--
MST
--
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/
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-05 09:30:39
On Tue, Jan 05, 2016 at 09:13:19AM +0100, Martin Schwidefsky wrote:
On Mon, 4 Jan 2016 22:18:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:
quoted
On Mon, Jan 04, 2016 at 02:45:25PM +0100, Peter Zijlstra wrote:
quoted
On Thu, Dec 31, 2015 at 09:08:38PM +0200, Michael S. Tsirkin wrote:
quoted
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>
---
arch/s390/include/asm/barrier.h | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
Why define the smp_*mb() primitives here? Would not the inclusion of
asm-generic/barrier.h do this?
No because the generic one is a nop on !SMP, this one isn't.
Pls note this patch is just reordering code without making
functional changes.
And at the moment, on s390 smp_xxx barriers are always non empty.
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.
OK, so I'll queue a patch to do this then?
Just to make sure: the question would be, are smp_xxx barriers ever used
in s390 arch specific code to flush in/out memory accesses for
synchronization with the hypervisor?
I went over s390 arch code and it seems to me the answer is no
(except of course for virtio).
But I also see a lot of weirdness on this architecture.
I found these calls:
arch/s390/include/asm/bitops.h: smp_mb__before_atomic();
arch/s390/include/asm/bitops.h: smp_mb();
Not used in arch specific code so this is likely OK.
arch/s390/kernel/vdso.c: smp_mb();
Looking at
Author: Christian Borntraeger [off-list ref]
Date: Fri Sep 11 16:23:06 2015 +0200
s390/vdso: use correct memory barrier
By definition smp_wmb only orders writes against writes. (Finish all
previous writes, and do not start any future write). To protect the
vdso init code against early reads on other CPUs, let's use a full
smp_mb at the end of vdso init. As right now smp_wmb is implemented
as full serialization, this needs no stable backport, but this change
will be necessary if we reimplement smp_wmb.
ok from hypervisor point of view, but it's also strange:
1. why isn't this paired with another mb somewhere?
this seems to violate barrier pairing rules.
2. how does smp_mb protect against early reads on other CPUs?
It normally does not: it orders reads from this CPU versus writes
from same CPU. But init code does not appear to read anything.
Maybe this is some s390 specific trick?
I could not figure out the above commit.
arch/s390/kvm/kvm-s390.c: smp_mb();
Does not appear to be paired with anything.
arch/s390/lib/spinlock.c: smp_mb();
arch/s390/lib/spinlock.c: smp_mb();
Seems ok, and appears paired properly.
Just to make sure - spinlock is not paravirtualized on s390, is it?
rch/s390/kernel/time.c: smp_wmb();
arch/s390/kernel/time.c: smp_wmb();
arch/s390/kernel/time.c: smp_wmb();
arch/s390/kernel/time.c: smp_wmb();
It's all around vdso, so I'm guessing userspace is using this,
this is why there's no pairing.
quoted
Some of this could be sub-optimal, but
since on s390 Linux always runs on a hypervisor,
I am not sure it's safe to use the generic version -
in other words, it just might be that for s390 smp_ and virt_
barriers must be equivalent.
The definition of the memory barriers is independent from the fact
if the system is running on an hypervisor or not.
Is there really
an architecture where you need special virt_xxx barriers?!?
It is whenever host and guest or two guests access memory at
the same time.
The optimization where smp_xxx barriers are compiled out when
CONFIG_SMP is cleared means that two UP guests running
on an SMP host can not use smp_xxx barriers for communication.
See explanation here:
http://thread.gmane.org/gmane.linux.kernel.virtualization/26555
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Tue, Jan 05, 2016 at 10:51:17AM +0200, Michael S. Tsirkin wrote:
On Tue, Jan 05, 2016 at 09:36:55AM +0800, Boqun Feng wrote:
quoted
Hi Michael,
On Thu, Dec 31, 2015 at 09:07:42PM +0200, Michael S. Tsirkin wrote:
quoted
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
I think this is the part that was missed in review.
Yes, I realized my mistake after reread the series. But smp_lwsync() is
not defined in asm-generic/barriers.h, right?
quoted
quoted
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>
---
arch/powerpc/include/asm/barrier.h | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
, therefore this will emit an lwsync no matter SMP or UP.
Absolutely. But smp_store_release (without __) will not.
Please note I did test this: for ppc code before and after
this patch generates exactly the same binary on SMP and UP.
Yes, you're right, sorry for my mistake...
quoted
Another thing is that smp_lwsync() may have a third user(other than
smp_load_acquire() and smp_store_release()):
http://article.gmane.org/gmane.linux.ports.ppc.embedded/89877
I'm OK to change my patch accordingly, but do we really want
smp_lwsync() get involved in this cleanup? If I understand you
correctly, this cleanup focuses on external API like smp_{r,w,}mb(),
while smp_lwsync() is internal to PPC.
Regards,
Boqun
I think you missed the leading ___ :)
What I mean here was smp_lwsync() was originally internal to PPC, but
never mind ;-)
smp_store_release is external and it needs __smp_lwsync as
defined here.
I can duplicate some code and have smp_lwsync *not* call __smp_lwsync
You mean bringing smp_lwsync() back? because I haven't seen you defining
in asm-generic/barriers.h in previous patches and you just delete it in
this patch.
but why do this? Still, if you prefer it this way,
please let me know.
I think deleting smp_lwsync() is fine, though I need to change atomic
variants patches on PPC because of it ;-/
Regards,
Boqun
quoted
quoted
WRITE_ONCE(*p, v); \
} while (0)
-#define smp_load_acquire(p) \
+#define __smp_load_acquire(p) \
({ \
typeof(*p) ___p1 = READ_ONCE(*p); \
compiletime_assert_atomic_type(*p); \
- smp_lwsync(); \
+ __smp_lwsync(); \
___p1; \
})
--
MST
--
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/
From: Martin Schwidefsky <hidden> Date: 2016-01-05 12:09:14
On Tue, 5 Jan 2016 11:30:19 +0200
"Michael S. Tsirkin" [off-list ref] wrote:
On Tue, Jan 05, 2016 at 09:13:19AM +0100, Martin Schwidefsky wrote:
quoted
On Mon, 4 Jan 2016 22:18:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:
quoted
On Mon, Jan 04, 2016 at 02:45:25PM +0100, Peter Zijlstra wrote:
quoted
On Thu, Dec 31, 2015 at 09:08:38PM +0200, Michael S. Tsirkin wrote:
quoted
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>
---
arch/s390/include/asm/barrier.h | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
Why define the smp_*mb() primitives here? Would not the inclusion of
asm-generic/barrier.h do this?
No because the generic one is a nop on !SMP, this one isn't.
Pls note this patch is just reordering code without making
functional changes.
And at the moment, on s390 smp_xxx barriers are always non empty.
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.
OK, so I'll queue a patch to do this then?
Yes please.
Just to make sure: the question would be, are smp_xxx barriers ever used
in s390 arch specific code to flush in/out memory accesses for
synchronization with the hypervisor?
I went over s390 arch code and it seems to me the answer is no
(except of course for virtio).
Correct. Guest to host communication either uses instructions which
imply a memory barrier or QDIO which uses atomics.
But I also see a lot of weirdness on this architecture.
Mostly historical, s390 actually is one of the easiest architectures in
regard to memory barriers.
I found these calls:
arch/s390/include/asm/bitops.h: smp_mb__before_atomic();
arch/s390/include/asm/bitops.h: smp_mb();
Not used in arch specific code so this is likely OK.
This has been introduced with git commit 5402ea6af11dc5a9, the smp_mb
and smp_mb__before_atomic are used in clear_bit_unlock and
__clear_bit_unlock which are 1:1 copies from the code in
include/asm-generic/bitops/lock.h. Only test_and_set_bit_lock differs
from the generic implementation.
arch/s390/kernel/vdso.c: smp_mb();
Looking at
Author: Christian Borntraeger [off-list ref]
Date: Fri Sep 11 16:23:06 2015 +0200
s390/vdso: use correct memory barrier
By definition smp_wmb only orders writes against writes. (Finish all
previous writes, and do not start any future write). To protect the
vdso init code against early reads on other CPUs, let's use a full
smp_mb at the end of vdso init. As right now smp_wmb is implemented
as full serialization, this needs no stable backport, but this change
will be necessary if we reimplement smp_wmb.
ok from hypervisor point of view, but it's also strange:
1. why isn't this paired with another mb somewhere?
this seems to violate barrier pairing rules.
2. how does smp_mb protect against early reads on other CPUs?
It normally does not: it orders reads from this CPU versus writes
from same CPU. But init code does not appear to read anything.
Maybe this is some s390 specific trick?
I could not figure out the above commit.
That smp_mb can be removed. The initial s390 vdso code is heavily influenced
by the powerpc version which does have a smp_wmb in vdso_init right before
the vdso_ready=1 assignment. s390 has no need for that.
arch/s390/kvm/kvm-s390.c: smp_mb();
Does not appear to be paired with anything.
This one does not make sense to me. Imho can be removed as well.
arch/s390/lib/spinlock.c: smp_mb();
arch/s390/lib/spinlock.c: smp_mb();
Seems ok, and appears paired properly.
Just to make sure - spinlock is not paravirtualized on s390, is it?
s390 just uses the compare-and-swap instruction for the basic lock/unlock
operation, this implies the memory barrier. We do call the hypervisor for
contended locks if the lock can not be acquired after a number of retries.
A while ago we did play with ticket spinlocks but they behaved badly in
out usual virtualized environments. If we find the time we might take a
closer look at the para-virtualized queued spinlocks.
rch/s390/kernel/time.c: smp_wmb();
arch/s390/kernel/time.c: smp_wmb();
arch/s390/kernel/time.c: smp_wmb();
arch/s390/kernel/time.c: smp_wmb();
It's all around vdso, so I'm guessing userspace is using this,
this is why there's no pairing.
Correct, this is the update count mechanics with the vdso user space code.
quoted
quoted
Some of this could be sub-optimal, but
since on s390 Linux always runs on a hypervisor,
I am not sure it's safe to use the generic version -
in other words, it just might be that for s390 smp_ and virt_
barriers must be equivalent.
The definition of the memory barriers is independent from the fact
if the system is running on an hypervisor or not.
Is there really
an architecture where you need special virt_xxx barriers?!?
It is whenever host and guest or two guests access memory at
the same time.
The optimization where smp_xxx barriers are compiled out when
CONFIG_SMP is cleared means that two UP guests running
on an SMP host can not use smp_xxx barriers for communication.
See explanation here:
http://thread.gmane.org/gmane.linux.kernel.virtualization/26555
Got it, makes sense.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2016-01-05 13:04:58
On Tue, Jan 05, 2016 at 01:08:52PM +0100, Martin Schwidefsky wrote:
On Tue, 5 Jan 2016 11:30:19 +0200
"Michael S. Tsirkin" [off-list ref] wrote:
quoted
On Tue, Jan 05, 2016 at 09:13:19AM +0100, Martin Schwidefsky wrote:
quoted
On Mon, 4 Jan 2016 22:18:58 +0200
"Michael S. Tsirkin" [off-list ref] wrote:
quoted
On Mon, Jan 04, 2016 at 02:45:25PM +0100, Peter Zijlstra wrote:
quoted
On Thu, Dec 31, 2015 at 09:08:38PM +0200, Michael S. Tsirkin wrote:
quoted
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>
---
arch/s390/include/asm/barrier.h | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
Why define the smp_*mb() primitives here? Would not the inclusion of
asm-generic/barrier.h do this?
No because the generic one is a nop on !SMP, this one isn't.
Pls note this patch is just reordering code without making
functional changes.
And at the moment, on s390 smp_xxx barriers are always non empty.
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.
OK, so I'll queue a patch to do this then?
Yes please.
OK, I'll add a patch on top in v3.
quoted
Just to make sure: the question would be, are smp_xxx barriers ever used
in s390 arch specific code to flush in/out memory accesses for
synchronization with the hypervisor?
I went over s390 arch code and it seems to me the answer is no
(except of course for virtio).
Correct. Guest to host communication either uses instructions which
imply a memory barrier or QDIO which uses atomics.
And atomics imply a barrier on s390, right?
quoted
But I also see a lot of weirdness on this architecture.
Mostly historical, s390 actually is one of the easiest architectures in
regard to memory barriers.
quoted
I found these calls:
arch/s390/include/asm/bitops.h: smp_mb__before_atomic();
arch/s390/include/asm/bitops.h: smp_mb();
Not used in arch specific code so this is likely OK.
This has been introduced with git commit 5402ea6af11dc5a9, the smp_mb
and smp_mb__before_atomic are used in clear_bit_unlock and
__clear_bit_unlock which are 1:1 copies from the code in
include/asm-generic/bitops/lock.h. Only test_and_set_bit_lock differs
from the generic implementation.
something to keep in mind, but
I'd rather not touch bitops at the moment - this patchset is already too big.
quoted
arch/s390/kernel/vdso.c: smp_mb();
Looking at
Author: Christian Borntraeger [off-list ref]
Date: Fri Sep 11 16:23:06 2015 +0200
s390/vdso: use correct memory barrier
By definition smp_wmb only orders writes against writes. (Finish all
previous writes, and do not start any future write). To protect the
vdso init code against early reads on other CPUs, let's use a full
smp_mb at the end of vdso init. As right now smp_wmb is implemented
as full serialization, this needs no stable backport, but this change
will be necessary if we reimplement smp_wmb.
ok from hypervisor point of view, but it's also strange:
1. why isn't this paired with another mb somewhere?
this seems to violate barrier pairing rules.
2. how does smp_mb protect against early reads on other CPUs?
It normally does not: it orders reads from this CPU versus writes
from same CPU. But init code does not appear to read anything.
Maybe this is some s390 specific trick?
I could not figure out the above commit.
That smp_mb can be removed. The initial s390 vdso code is heavily influenced
by the powerpc version which does have a smp_wmb in vdso_init right before
the vdso_ready=1 assignment. s390 has no need for that.
quoted
arch/s390/kvm/kvm-s390.c: smp_mb();
Does not appear to be paired with anything.
This one does not make sense to me. Imho can be removed as well.
quoted
arch/s390/lib/spinlock.c: smp_mb();
arch/s390/lib/spinlock.c: smp_mb();
Seems ok, and appears paired properly.
Just to make sure - spinlock is not paravirtualized on s390, is it?
s390 just uses the compare-and-swap instruction for the basic lock/unlock
operation, this implies the memory barrier. We do call the hypervisor for
contended locks if the lock can not be acquired after a number of retries.
A while ago we did play with ticket spinlocks but they behaved badly in
out usual virtualized environments. If we find the time we might take a
closer look at the para-virtualized queued spinlocks.
quoted
rch/s390/kernel/time.c: smp_wmb();
arch/s390/kernel/time.c: smp_wmb();
arch/s390/kernel/time.c: smp_wmb();
arch/s390/kernel/time.c: smp_wmb();
It's all around vdso, so I'm guessing userspace is using this,
this is why there's no pairing.
Correct, this is the update count mechanics with the vdso user space code.
quoted
quoted
quoted
Some of this could be sub-optimal, but
since on s390 Linux always runs on a hypervisor,
I am not sure it's safe to use the generic version -
in other words, it just might be that for s390 smp_ and virt_
barriers must be equivalent.
The definition of the memory barriers is independent from the fact
if the system is running on an hypervisor or not.
Is there really
an architecture where you need special virt_xxx barriers?!?
It is whenever host and guest or two guests access memory at
the same time.
The optimization where smp_xxx barriers are compiled out when
CONFIG_SMP is cleared means that two UP guests running
on an SMP host can not use smp_xxx barriers for communication.
See explanation here:
http://thread.gmane.org/gmane.linux.kernel.virtualization/26555
Got it, makes sense.
An ack would be appreciated.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.