* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Linus Torvalds @ 2016-01-12 23:24 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
Linux Kernel Mailing List, Michael S. Tsirkin,
the arch/x86 maintainers, Andy Lutomirski, Andy Lutomirski,
Paul E. McKenney, Thomas Gleixner, virtualization, Ingo Molnar
In-Reply-To: <56958467.6010808@zytor.com>
On Tue, Jan 12, 2016 at 2:55 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>
> Be careful with this: if it only shows up in a microbenchmark, we may
> introduce a hard-to-debug regression for no real benefit.
So I can pretty much guarantee that it shouldn't regress from a
correctness angle, since we rely *heavily* on locked instructions
being barriers, in locking and in various other situations.
Indeed, much more so than we ever rely on "smp_mb()". The places that
rely on smp_mb() are pretty few in the end.
So I think the only issue is whether sometimes "mfence" might be
faster. So far, I've never actually heard of that being the case. The
fence instructions have always sucked when I've seen them.
But talking to the hw people about this is certainly a good idea regardless.
Linus
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-13 0:21 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Will Deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160112214003.GQ6357@twins.programming.kicks-ass.net>
On 01/12/2016 01:40 PM, Peter Zijlstra wrote:
>
>> It is selectable only for MIPS R2 but not MIPS R6. The reason is - most of
>> MIPS R2 CPUs have short pipeline and that SYNC is just waste of CPU
>> resource, especially taking into account that "lightweight syncs" are
>> converted to a heavy "SYNC 0" in many of that CPUs. However the latest
>> MIPS/Imagination CPU have a pipeline long enough to hit a problem - absence
>> of SYNC at LL/SC inside atomics, barriers etc.
> What ?! Are you saying that because R2 has short pipelines its unlikely
> to hit the reordering issues and we can omit barriers?
It was my guess to explain - why barriers was not included originally.
You can check with Ralf, he knows more about that time MIPS Linux code.
I bother with this more than 2 years and I just try to solve that issue
- in recent CPUs the load after LL/SC synchronization instruction loop
can get ahead of SC for sure, it was tested.
>
>>> And reading the MIPS64 v6.04 instruction set manual, I think 0x11/0x12
>>> are_NOT_ transitive and therefore cannot be used to implement the
>>> smp_mb__{before,after} stuff.
>>>
>>> That is, in MIPS speak, those SYNC types are Ordering Barriers, not
>>> Completion Barriers.
>> Please see above, point 2.
> That did not in fact enlighten things. Are they transitive/multi-copy
> atomic or not?
Peter Zijlstra recently wrote: "In particular we're very much all
'confused' about the various notions of transitivity". I am actually
confused too and need some examples here.
>
> (and here Will will go into great detail on the differences between the
> two and make our collective brains explode :-)
>
>>> That is, currently all architectures -- with exception of PPC -- have
>>> RCsc locks, but using these non-transitive things will get you RCpc
>>> locks.
>>>
>>> So yes, MIPS can go RCpc for its locks and share the burden of pain with
>>> PPC, but that needs to be a very concious decision.
>> I don't understand that - I tried hard but I can't find any word like
>> "RCsc", "RCpc" in Documents/ directory. Web search goes nowhere, of course.
> From: lkml.kernel.org/r/20150828153921.GF19282@twins.programming.kicks-ass.net
>
> Yes, the difference between RCpc and RCsc is in the meaning of RELEASE +
> ACQUIRE. With RCsc that implies a full memory barrier, with RCpc it does
> not.
MIPS Arch starting from R2 requires that. If some CPU can't, it should
execute a full "SYNC 0" instead, which is a full memory barrier.
>
> Currently PowerPC is the only arch that (can, and) does RCpc and gives a
> weaker RELEASE + ACQUIRE. Only the CPU who did the ACQUIRE is guaranteed
> to see the stores of the CPU which did the RELEASE in order.
Yes, it was a goal for SYNC_ACQUIRE and SYNC_RELEASE.
Caveats:
- "Full memory barrier" on MIPS means - full barrier for any device
in coherent domain. In MIPS Tech/Imagination Tech MIPS-based CPU it is
"for any device connected to CM or IOCU + directly connected memory".
- It is not applied to instruction fetch. However, I-Cache flushes
and SYNCI are consistent with that. There is also hazard barrier
instructions to clear CPU pipeline to some extent - to help with this
limitation.
I don't think that these caveats prevent a correct Acquire/Release semantic.
- Leonid.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Will Deacon @ 2016-01-13 10:45 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <569565DA.2010903@imgtec.com>
On Tue, Jan 12, 2016 at 12:45:14PM -0800, Leonid Yegoshin wrote:
> >The issue I have with the SYNC description in the text above is that it
> >describes the single CPU (program order) and the dual-CPU (confusingly
> >named global order) cases, but then doesn't generalise any further. That
> >means we can't sensibly reason about transitivity properties when a third
> >agent is involved. For example, the WRC+sync+addr test:
> >
> >
> >P0:
> >Wx = 1
> >
> >P1:
> >Rx == 1
> >SYNC
> >Wy = 1
> >
> >P2:
> >Ry == 1
> ><address dep>
> >Rx = 0
> >
> >
> >I can't find anything to forbid that, given the text. The main problem
> >is having the SYNC on P1 affect the write by P0.
>
> As I understand that test, the visibility of P0: W[x] = 1 is identical to P1
> and P2 here. If P1 got X before SYNC and write to Y after SYNC then
> instruction source register dependency tracking in P2 prevents a speculative
> load of X before P2 obtains Y from the same place as P0/P1 and calculate
> address of X. If some load of X in P2 happens before address dependency
> calculation it's result is discarded.
I don't think the address dependency is enough on its own. By that
reasoning, the following variant (WRC+addr+addr) would work too:
P0:
Wx = 1
P1:
Rx == 1
<address dep>
Wy = 1
P2:
Ry == 1
<address dep>
Rx = 0
So are you saying that this is also forbidden?
Imagine that P0 and P1 are two threads that share a store buffer. What
then?
> Yes, you can't find that in MIPS SYNC instruction description, it is more
> likely in CM (Coherence Manager) area. I just pointed our arch team member
> responsible for documents and he will think how to explain that.
I tried grepping the linked documents for "coherence manager" but couldn't
find anything. Is the description you refer to available anywhere?
Will
^ permalink raw reply
* [PATCH] uapi: use __u8 from linux/types.h
From: Gleb Fotengauer-Malinovskiy @ 2016-01-13 16:10 UTC (permalink / raw)
To: David Airlie, Gerd Hoffmann, Michael S. Tsirkin
Cc: linux-api, linux-kernel, dri-devel, virtualization
Kernel headers should use linux/types.h based definitions.
Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
---
include/uapi/linux/virtio_gpu.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index 7a63faa..4b04ead 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -287,7 +287,7 @@ struct virtio_gpu_get_capset {
/* VIRTIO_GPU_RESP_OK_CAPSET */
struct virtio_gpu_resp_capset {
struct virtio_gpu_ctrl_hdr hdr;
- uint8_t capset_data[];
+ __u8 capset_data[];
};
#define VIRTIO_GPU_EVENT_DISPLAY (1 << 0)
--
glebfm
^ permalink raw reply related
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Borislav Petkov @ 2016-01-13 16:17 UTC (permalink / raw)
To: Linus Torvalds
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
Linux Kernel Mailing List, Michael S. Tsirkin,
the arch/x86 maintainers, Andy Lutomirski, Andy Lutomirski,
H. Peter Anvin, Paul E. McKenney, Thomas Gleixner, virtualization,
Ingo Molnar
In-Reply-To: <CA+55aFz44oYK5GD3=-Y2E7bNwmJ0Ko5GaZ+iMQDamLALiw7RqQ@mail.gmail.com>
On Tue, Jan 12, 2016 at 03:24:05PM -0800, Linus Torvalds wrote:
> But talking to the hw people about this is certainly a good idea regardless.
I'm not seeing it in this thread but I might've missed it too. Anyway,
I'm being reminded that the ADD will change rFLAGS while MFENCE doesn't
touch them.
Do we care?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Michael S. Tsirkin @ 2016-01-13 16:20 UTC (permalink / raw)
To: Linus Torvalds
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
Andy Lutomirski, Andy Lutomirski, H. Peter Anvin,
Paul E. McKenney, Thomas Gleixner, virtualization, Ingo Molnar
In-Reply-To: <CA+55aFyuR1YCZjC9++E4kpvRxgoM4sqzhNaS27EZPFh9CuKjYg@mail.gmail.com>
On Tue, Jan 12, 2016 at 01:37:38PM -0800, Linus Torvalds wrote:
> On Tue, Jan 12, 2016 at 12:59 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> >
> > Here's an article with numbers:
> >
> > http://shipilev.net/blog/2014/on-the-fence-with-dependencies/
>
> Well, that's with the busy loop and one set of code generation. It
> doesn't show the "oops, deeper stack isn't even in the cache any more
> due to call chains" issue.
It's an interesting read, thanks!
So sp is read on return from function I think. I added a function and sure
enough, it slows the add 0(sp) variant down. It's still faster than mfence for
me though! Testing code + results below. Reaching below stack, or
allocating extra 4 bytes above the stack pointer gives us back the performance.
> But yes:
>
> > I think they're suggesting using a negative offset, which is safe as
> > long as it doesn't page fault, even though we have the redzone
> > disabled.
>
> I think a negative offset might work very well. Partly exactly
> *because* we have the redzone disabled: we know that inside the
> kernel, we'll never have any live stack frame accesses under the stack
> pointer, so "-4(%rsp)" sounds good to me. There should never be any
> pending writes in the write buffer, because even if it *was* live, it
> would have been read off first.
>
> Yeah, it potentially does extend the stack cache footprint by another
> 4 bytes, but that sounds very benign.
>
> So perhaps it might be worth trying to switch the "mfence" to "lock ;
> addl $0,-4(%rsp)" in the kernel for x86-64, and remove the alternate
> for x86-32.
>
>
> I'd still want to see somebody try to benchmark it. I doubt it's
> noticeable, but making changes because you think it might save a few
> cycles without then even measuring it is just wrong.
>
> Linus
I'll try this in the kernel now, will report, though I'm
not optimistic a high level benchmark can show this
kind of thing.
---------------
main.c:
---------------
extern volatile int x;
volatile int x;
#ifdef __x86_64__
#define SP "rsp"
#else
#define SP "esp"
#endif
#ifdef lock
#define barrier() do { int p; asm volatile ("lock; addl $0,%0" ::"m"(p): "memory"); } while (0)
#endif
#ifdef locksp
#define barrier() asm("lock; addl $0,0(%%" SP ")" ::: "memory")
#endif
#ifdef lockrz
#define barrier() asm("lock; addl $0,-4(%%" SP ")" ::: "memory")
#endif
#ifdef xchg
#define barrier() do { int p; int ret; asm volatile ("xchgl %0, %1;": "=r"(ret) : "m"(p): "memory", "cc"); } while (0)
#endif
#ifdef xchgrz
/* same as xchg but poking at gcc red zone */
#define barrier() do { int ret; asm volatile ("xchgl %0, -4(%%" SP ");": "=r"(ret) :: "memory", "cc"); } while (0)
#endif
#ifdef mfence
#define barrier() asm("mfence" ::: "memory")
#endif
#ifdef lfence
#define barrier() asm("lfence" ::: "memory")
#endif
#ifdef sfence
#define barrier() asm("sfence" ::: "memory")
#endif
void __attribute__ ((noinline)) test(int i, int *j)
{
/*
* Test barrier in a loop. We also poke at a volatile variable in an
* attempt to make it a bit more realistic - this way there's something
* in the store-buffer.
*/
x = i - *j;
barrier();
*j = x;
}
int main(int argc, char **argv)
{
int i;
int j = 1234;
for (i = 0; i < 10000000; ++i)
test(i, &j);
return 0;
}
---------------
ALL = xchg xchgrz lock locksp lockrz mfence lfence sfence
CC = gcc
CFLAGS += -Wall -O2 -ggdb
PERF = perf stat -r 10 --log-fd 1 --
TIME = /usr/bin/time -f %e
FILTER = cat
all: ${ALL}
clean:
rm -f ${ALL}
run: all
for file in ${ALL}; do echo ${RUN} ./$$file "|" ${FILTER}; ${RUN} ./$$file | ${FILTER}; done
perf time: run
time: RUN=${TIME}
perf: RUN=${PERF}
perf: FILTER=grep elapsed
.PHONY: all clean run perf time
xchgrz: CFLAGS += -mno-red-zone
${ALL}: main.c
${CC} ${CFLAGS} -D$@ -o $@ main.c
--------------------------------------------
perf stat -r 10 --log-fd 1 -- ./xchg | grep elapsed
0.080420565 seconds time elapsed ( +- 2.31% )
perf stat -r 10 --log-fd 1 -- ./xchgrz | grep elapsed
0.087798571 seconds time elapsed ( +- 2.58% )
perf stat -r 10 --log-fd 1 -- ./lock | grep elapsed
0.083023724 seconds time elapsed ( +- 2.44% )
perf stat -r 10 --log-fd 1 -- ./locksp | grep elapsed
0.102880750 seconds time elapsed ( +- 0.13% )
perf stat -r 10 --log-fd 1 -- ./lockrz | grep elapsed
0.084917420 seconds time elapsed ( +- 3.28% )
perf stat -r 10 --log-fd 1 -- ./mfence | grep elapsed
0.156014715 seconds time elapsed ( +- 0.16% )
perf stat -r 10 --log-fd 1 -- ./lfence | grep elapsed
0.077731443 seconds time elapsed ( +- 0.12% )
perf stat -r 10 --log-fd 1 -- ./sfence | grep elapsed
0.036655741 seconds time elapsed ( +- 0.21% )
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Michael S. Tsirkin @ 2016-01-13 16:25 UTC (permalink / raw)
To: Borislav Petkov
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
Andy Lutomirski, Andy Lutomirski, H. Peter Anvin,
Paul E. McKenney, Thomas Gleixner, virtualization, Linus Torvalds,
Ingo Molnar
In-Reply-To: <20160113161704.GA13146@pd.tnic>
On Wed, Jan 13, 2016 at 05:17:04PM +0100, Borislav Petkov wrote:
> On Tue, Jan 12, 2016 at 03:24:05PM -0800, Linus Torvalds wrote:
> > But talking to the hw people about this is certainly a good idea regardless.
>
> I'm not seeing it in this thread but I might've missed it too. Anyway,
> I'm being reminded that the ADD will change rFLAGS while MFENCE doesn't
> touch them.
>
> Do we care?
Which flag do you refer to, exactly?
> --
> Regards/Gruss,
> Boris.
>
> ECO tip #101: Trim your mails when you reply.
^ permalink raw reply
* Re: [PATCH] uapi: use __u8 from linux/types.h
From: Michael S. Tsirkin @ 2016-01-13 16:25 UTC (permalink / raw)
To: Gleb Fotengauer-Malinovskiy
Cc: David Airlie, linux-api, linux-kernel, dri-devel, virtualization
In-Reply-To: <20160113160958.GA29108@glebfm.cloud.tilaa.com>
On Wed, Jan 13, 2016 at 07:10:15PM +0300, Gleb Fotengauer-Malinovskiy wrote:
> Kernel headers should use linux/types.h based definitions.
>
> Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> include/uapi/linux/virtio_gpu.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
> index 7a63faa..4b04ead 100644
> --- a/include/uapi/linux/virtio_gpu.h
> +++ b/include/uapi/linux/virtio_gpu.h
> @@ -287,7 +287,7 @@ struct virtio_gpu_get_capset {
> /* VIRTIO_GPU_RESP_OK_CAPSET */
> struct virtio_gpu_resp_capset {
> struct virtio_gpu_ctrl_hdr hdr;
> - uint8_t capset_data[];
> + __u8 capset_data[];
> };
>
> #define VIRTIO_GPU_EVENT_DISPLAY (1 << 0)
> --
> glebfm
^ permalink raw reply
* [PULL] virtio: barrier rework+fixes
From: Michael S. Tsirkin @ 2016-01-13 16:28 UTC (permalink / raw)
Cc: aquini, kvm, mst, peterz, benh, heiko.carstens, alexander.duyck,
virtualization, mingo, linux-arch, dave, linux, dbueso, richard,
bjorn.andersson, rmk+kernel, paulmck, james.hogan, arnd,
julian.calaby, boqun.feng, schwidefsky, stefanha, tglx, tony.luck,
netdev, linux-kernel, stable, minchan, david.vrabel, joe, akpm,
torvalds, davem
The following changes since commit afd2ff9b7e1b367172f18ba7f693dfb62bdcb2dc:
Linux 4.4 (2016-01-10 15:01:32 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
for you to fetch changes up to 43e361f23c49dbddf74f56ddf6cdd85c5dbff6da:
checkpatch: add virt barriers (2016-01-12 20:47:08 +0200)
----------------------------------------------------------------
virtio: barrier rework+fixes
This adds a new kind of barrier, and reworks virtio and xen
to use it.
Plus some fixes here and there.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Yes I know that the patch by Davidlohr Bueso has a typo in the subject :(
Davidlohr Bueso (1):
lcoking/barriers, arch: Use smp barriers in smp_store_release()
Michael S. Tsirkin (40):
asm-generic: guard smp_store_release/load_acquire
ia64: rename nop->iosapic_nop
ia64: reuse asm-generic/barrier.h
powerpc: reuse asm-generic/barrier.h
s390: reuse asm-generic/barrier.h
sparc: reuse asm-generic/barrier.h
arm: reuse asm-generic/barrier.h
arm64: reuse asm-generic/barrier.h
metag: reuse asm-generic/barrier.h
mips: reuse asm-generic/barrier.h
x86/um: reuse asm-generic/barrier.h
x86: reuse asm-generic/barrier.h
asm-generic: add __smp_xxx wrappers
powerpc: define __smp_xxx
arm64: define __smp_xxx
arm: define __smp_xxx
blackfin: define __smp_xxx
ia64: define __smp_xxx
metag: define __smp_xxx
mips: define __smp_xxx
s390: define __smp_xxx
sh: define __smp_xxx, fix smp_store_mb for !SMP
sparc: define __smp_xxx
tile: define __smp_xxx
xtensa: define __smp_xxx
x86: define __smp_xxx
asm-generic: implement virt_xxx memory barriers
Revert "virtio_ring: Update weak barriers to use dma_wmb/rmb"
virtio_ring: update weak barriers to use virt_xxx
sh: support 1 and 2 byte xchg
sh: move xchg_cmpxchg to a header by itself
virtio_ring: use virt_store_mb
xenbus: use virt_xxx barriers
xen/io: use virt_xxx barriers
xen/events: use virt_xxx barriers
s390: use generic memory barriers
s390: more efficient smp barriers
checkpatch.pl: add missing memory barriers
checkpatch: check for __smp outside barrier.h
checkpatch: add virt barriers
Minchan Kim (2):
virtio_balloon: fix race by fill and leak
virtio_balloon: fix race between migration and ballooning
Stefan Hajnoczi (1):
virtio: make find_vqs() checkpatch.pl-friendly
arch/arm/include/asm/barrier.h | 35 ++---------
arch/arm64/include/asm/barrier.h | 19 ++----
arch/blackfin/include/asm/barrier.h | 4 +-
arch/ia64/include/asm/barrier.h | 24 +++-----
arch/metag/include/asm/barrier.h | 55 +++++------------
arch/mips/include/asm/barrier.h | 51 +++++-----------
arch/powerpc/include/asm/barrier.h | 33 ++++------
arch/s390/include/asm/barrier.h | 23 ++++---
arch/sh/include/asm/barrier.h | 3 +-
arch/sh/include/asm/cmpxchg-grb.h | 22 +++++++
arch/sh/include/asm/cmpxchg-irq.h | 11 ++++
arch/sh/include/asm/cmpxchg-llsc.h | 25 +-------
arch/sh/include/asm/cmpxchg-xchg.h | 51 ++++++++++++++++
arch/sh/include/asm/cmpxchg.h | 3 +
arch/sparc/include/asm/barrier_32.h | 1 -
arch/sparc/include/asm/barrier_64.h | 29 ++-------
arch/sparc/include/asm/processor.h | 3 -
arch/tile/include/asm/barrier.h | 9 +--
arch/x86/include/asm/barrier.h | 36 +++++------
arch/x86/um/asm/barrier.h | 9 +--
arch/xtensa/include/asm/barrier.h | 4 +-
drivers/virtio/virtio_pci_common.h | 2 +-
include/asm-generic/barrier.h | 106 ++++++++++++++++++++++++++++++---
include/linux/virtio_config.h | 2 +-
include/linux/virtio_ring.h | 21 +++++--
include/xen/interface/io/ring.h | 16 ++---
arch/ia64/kernel/iosapic.c | 6 +-
drivers/gpu/drm/virtio/virtgpu_kms.c | 2 +-
drivers/misc/mic/card/mic_virtio.c | 2 +-
drivers/remoteproc/remoteproc_virtio.c | 2 +-
drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
drivers/s390/virtio/kvm_virtio.c | 2 +-
drivers/s390/virtio/virtio_ccw.c | 2 +-
drivers/virtio/virtio_balloon.c | 4 +-
drivers/virtio/virtio_input.c | 2 +-
drivers/virtio/virtio_mmio.c | 2 +-
drivers/virtio/virtio_pci_common.c | 4 +-
drivers/virtio/virtio_pci_modern.c | 2 +-
drivers/virtio/virtio_ring.c | 15 +++--
drivers/xen/events/events_fifo.c | 3 +-
drivers/xen/xenbus/xenbus_comms.c | 8 +--
mm/balloon_compaction.c | 4 +-
Documentation/memory-barriers.txt | 28 +++++++--
scripts/checkpatch.pl | 33 +++++++++-
44 files changed, 401 insertions(+), 319 deletions(-)
create mode 100644 arch/sh/include/asm/cmpxchg-xchg.h
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Borislav Petkov @ 2016-01-13 16:33 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
Andy Lutomirski, Andy Lutomirski, H. Peter Anvin,
Paul E. McKenney, Thomas Gleixner, virtualization, Linus Torvalds,
Ingo Molnar
In-Reply-To: <20160113182505-mutt-send-email-mst@redhat.com>
On Wed, Jan 13, 2016 at 06:25:21PM +0200, Michael S. Tsirkin wrote:
> Which flag do you refer to, exactly?
All the flags in rFLAGS which ADD modifies: OF,SF,ZF,AF,PF,CF
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Michael S. Tsirkin @ 2016-01-13 16:42 UTC (permalink / raw)
To: Borislav Petkov
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
Andy Lutomirski, Andy Lutomirski, H. Peter Anvin,
Paul E. McKenney, Thomas Gleixner, virtualization, Linus Torvalds,
Ingo Molnar
In-Reply-To: <20160113163331.GG12897@pd.tnic>
On Wed, Jan 13, 2016 at 05:33:31PM +0100, Borislav Petkov wrote:
> On Wed, Jan 13, 2016 at 06:25:21PM +0200, Michael S. Tsirkin wrote:
> > Which flag do you refer to, exactly?
>
> All the flags in rFLAGS which ADD modifies: OF,SF,ZF,AF,PF,CF
Oh, I think this means we need a "cc" clobber.
This also seems to be a bug on !XMM CPUs.
cmpxchg.h gets it right.
I'll send a patch.
> --
> Regards/Gruss,
> Boris.
>
> ECO tip #101: Trim your mails when you reply.
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Borislav Petkov @ 2016-01-13 16:53 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
Andy Lutomirski, Andy Lutomirski, H. Peter Anvin,
Paul E. McKenney, Thomas Gleixner, virtualization, Linus Torvalds,
Ingo Molnar
In-Reply-To: <20160113184003-mutt-send-email-mst@redhat.com>
On Wed, Jan 13, 2016 at 06:42:48PM +0200, Michael S. Tsirkin wrote:
> Oh, I think this means we need a "cc" clobber.
Btw, does your microbenchmark do it too?
Because, the "cc" clobber should cause additional handling of flags,
depending on the context. It won't matter if the context doesn't need
rFLAGS handling in the benchmark but if we start using LOCK; ADD in the
kernel, I can imagine some places where mb() is used and rFLAGS are
live, causing gcc to either reorder code or stash them away...
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Michael S. Tsirkin @ 2016-01-13 17:00 UTC (permalink / raw)
To: Borislav Petkov
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
Andy Lutomirski, Andy Lutomirski, H. Peter Anvin,
Paul E. McKenney, Thomas Gleixner, virtualization, Linus Torvalds,
Ingo Molnar
In-Reply-To: <20160113165319.GJ12897@pd.tnic>
On Wed, Jan 13, 2016 at 05:53:20PM +0100, Borislav Petkov wrote:
> On Wed, Jan 13, 2016 at 06:42:48PM +0200, Michael S. Tsirkin wrote:
> > Oh, I think this means we need a "cc" clobber.
>
> Btw, does your microbenchmark do it too?
Yes - I fixed it now, but it did not affect the result.
We'd need some code where gcc carries flags around though.
> Because, the "cc" clobber should cause additional handling of flags,
> depending on the context. It won't matter if the context doesn't need
> rFLAGS handling in the benchmark but if we start using LOCK; ADD in the
> kernel, I can imagine some places where mb() is used and rFLAGS are
> live, causing gcc to either reorder code or stash them away...
It will reorder code but not necessarily for the worse :)
Best I can do, I will add cc clobber to kernel and see whether
binary size grows.
> --
> Regards/Gruss,
> Boris.
>
> ECO tip #101: Trim your mails when you reply.
^ permalink raw reply
* [PATCH 0/2] vhost: cross-endian code cleanup
From: Greg Kurz @ 2016-01-13 17:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
This series is a respin of the following patch:
http://patchwork.ozlabs.org/patch/565921/
Patch 1 is preliminary work: it gives better names to the helpers that are
involved in cross-endian support.
Patch 2 is actually a v2 of the original patch. All devices now call a
helper in the generic code, which DTRT according to vq->private_data, as
suggested by Michael.
---
Greg Kurz (2):
vhost: helpers to enable/disable vring endianness
vhost: disentangle vring endianness stuff from the core code
drivers/vhost/net.c | 3 +++
drivers/vhost/scsi.c | 3 +++
drivers/vhost/test.c | 2 ++
drivers/vhost/vhost.c | 40 ++++++++++++++++++++++++++++------------
drivers/vhost/vhost.h | 1 +
5 files changed, 37 insertions(+), 12 deletions(-)
^ permalink raw reply
* [PATCH 1/2] vhost: helpers to enable/disable vring endianness
From: Greg Kurz @ 2016-01-13 17:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20160113170934.23705.62626.stgit@bahia.huguette.org>
The default use case for vhost is when the host and the vring have the
same endianness (default native endianness). But there are cases where
they differ and vhost should byteswap when accessing the vring:
- the host is big endian and the vring comes from a virtio 1.0 device
which is always little endian
- the architecture is bi-endian and the vring comes from a legacy virtio
device with a different endianness than the endianness of the host (aka
legacy cross-endian)
These cases are handled by the vq->is_le and the optional vq->user_be,
with the following logic:
- if none of the fields is enabled, vhost access the vring without byteswap
- if the vring is virtio 1.0 and the host is big endian, vq->is_le is
enabled to enforce little endian access to the vring
- if the vring is legacy cross-endian, userspace enables vq->user_be
to inform vhost about the vring endianness. This endianness is then
enforced for vring accesses through vq->is_le again
The logic is unclear in the current code.
This patch introduces helpers with explicit enable and disable semantics,
for better clarity.
No behaviour change.
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
drivers/vhost/vhost.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index ad2146a9ab2d..e02e06755ab7 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -43,11 +43,16 @@ enum {
#define vhost_avail_event(vq) ((__virtio16 __user *)&vq->used->ring[vq->num])
#ifdef CONFIG_VHOST_CROSS_ENDIAN_LEGACY
-static void vhost_vq_reset_user_be(struct vhost_virtqueue *vq)
+static void vhost_disable_user_be(struct vhost_virtqueue *vq)
{
vq->user_be = !virtio_legacy_is_little_endian();
}
+static void vhost_enable_user_be(struct vhost_virtqueue *vq, bool user_be)
+{
+ vq->user_be = user_be;
+}
+
static long vhost_set_vring_endian(struct vhost_virtqueue *vq, int __user *argp)
{
struct vhost_vring_state s;
@@ -62,7 +67,7 @@ static long vhost_set_vring_endian(struct vhost_virtqueue *vq, int __user *argp)
s.num != VHOST_VRING_BIG_ENDIAN)
return -EINVAL;
- vq->user_be = s.num;
+ vhost_enable_user_be(vq, !!s.num);
return 0;
}
@@ -81,7 +86,7 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx,
return 0;
}
-static void vhost_init_is_le(struct vhost_virtqueue *vq)
+static void vhost_enable_is_le(struct vhost_virtqueue *vq)
{
/* Note for legacy virtio: user_be is initialized at reset time
* according to the host endianness. If userspace does not set an
@@ -91,7 +96,7 @@ static void vhost_init_is_le(struct vhost_virtqueue *vq)
vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be;
}
#else
-static void vhost_vq_reset_user_be(struct vhost_virtqueue *vq)
+static void vhost_disable_user_be(struct vhost_virtqueue *vq)
{
}
@@ -106,13 +111,18 @@ static long vhost_get_vring_endian(struct vhost_virtqueue *vq, u32 idx,
return -ENOIOCTLCMD;
}
-static void vhost_init_is_le(struct vhost_virtqueue *vq)
+static void vhost_enable_is_le(struct vhost_virtqueue *vq)
{
if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
vq->is_le = true;
}
#endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
+static void vhost_disable_is_le(struct vhost_virtqueue *vq)
+{
+ vq->is_le = virtio_legacy_is_little_endian();
+}
+
static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
poll_table *pt)
{
@@ -276,8 +286,8 @@ static void vhost_vq_reset(struct vhost_dev *dev,
vq->call = NULL;
vq->log_ctx = NULL;
vq->memory = NULL;
- vq->is_le = virtio_legacy_is_little_endian();
- vhost_vq_reset_user_be(vq);
+ vhost_disable_is_le(vq);
+ vhost_disable_user_be(vq);
}
static int vhost_worker(void *data)
@@ -1157,11 +1167,11 @@ int vhost_init_used(struct vhost_virtqueue *vq)
__virtio16 last_used_idx;
int r;
if (!vq->private_data) {
- vq->is_le = virtio_legacy_is_little_endian();
+ vhost_disable_is_le(vq);
return 0;
}
- vhost_init_is_le(vq);
+ vhost_enable_is_le(vq);
r = vhost_update_used_flags(vq);
if (r)
^ permalink raw reply related
* [PATCH 2/2] vhost: disentangle vring endianness stuff from the core code
From: Greg Kurz @ 2016-01-13 17:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20160113170934.23705.62626.stgit@bahia.huguette.org>
The way vring endianness is being handled currently obfuscates
the code in vhost_init_used().
This patch tries to fix that by doing the following:
- move the the code that adjusts endianness to a dedicated helper
- export this helper so that backends explicitely call it
No behaviour change.
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
drivers/vhost/net.c | 3 +++
drivers/vhost/scsi.c | 3 +++
drivers/vhost/test.c | 2 ++
drivers/vhost/vhost.c | 16 +++++++++++-----
drivers/vhost/vhost.h | 1 +
5 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 9eda69e40678..df01c939cd00 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -917,6 +917,9 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
vhost_net_disable_vq(n, vq);
vq->private_data = sock;
+
+ vhost_adjust_vring_endian(vq);
+
r = vhost_init_used(vq);
if (r)
goto err_used;
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 29cfc57d496e..5a8363bfcb74 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1274,6 +1274,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
vq = &vs->vqs[i].vq;
mutex_lock(&vq->mutex);
vq->private_data = vs_tpg;
+
+ vhost_adjust_vring_endian(vq);
+
vhost_init_used(vq);
mutex_unlock(&vq->mutex);
}
diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
index f2882ac98726..75e3e0e9f5a8 100644
--- a/drivers/vhost/test.c
+++ b/drivers/vhost/test.c
@@ -196,6 +196,8 @@ static long vhost_test_run(struct vhost_test *n, int test)
oldpriv = vq->private_data;
vq->private_data = priv;
+ vhost_adjust_vring_endian(vq);
+
r = vhost_init_used(&n->vqs[index]);
mutex_unlock(&vq->mutex);
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index e02e06755ab7..b0a00340309e 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -123,6 +123,15 @@ static void vhost_disable_is_le(struct vhost_virtqueue *vq)
vq->is_le = virtio_legacy_is_little_endian();
}
+void vhost_adjust_vring_endian(struct vhost_virtqueue *vq)
+{
+ if (!vq->private_data)
+ vhost_disable_is_le(vq);
+ else
+ vhost_enable_is_le(vq);
+}
+EXPORT_SYMBOL_GPL(vhost_adjust_vring_endian);
+
static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
poll_table *pt)
{
@@ -1166,12 +1175,9 @@ int vhost_init_used(struct vhost_virtqueue *vq)
{
__virtio16 last_used_idx;
int r;
- if (!vq->private_data) {
- vhost_disable_is_le(vq);
- return 0;
- }
- vhost_enable_is_le(vq);
+ if (!vq->private_data)
+ return 0;
r = vhost_update_used_flags(vq);
if (r)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index d3f767448a72..88d86f45f756 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -162,6 +162,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len);
+void vhost_adjust_vring_endian(struct vhost_virtqueue *vq);
#define vq_err(vq, fmt, ...) do { \
pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
^ permalink raw reply related
* Re: [PATCH 3/4] x86,asm: Re-work smp_store_mb()
From: Linus Torvalds @ 2016-01-13 18:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, Linux Kernel Mailing List,
Andy Lutomirski, Borislav Petkov, Andy Lutomirski, H. Peter Anvin,
Paul E. McKenney, Thomas Gleixner, virtualization, Ingo Molnar
In-Reply-To: <20160113184003-mutt-send-email-mst@redhat.com>
On Wed, Jan 13, 2016 at 8:42 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> Oh, I think this means we need a "cc" clobber.
It's probably a good idea to add one.
Historically, gcc doesn't need one on x86, and always considers flags
clobbered. We are probably missing the cc clobber in a *lot* of places
for this reason.
But even if not necessary, it's probably a good thing to add for
documentation, and in case gcc semantcs ever change.
Linus
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-13 19:02 UTC (permalink / raw)
To: Will Deacon
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160113104516.GE25458@arm.com>
On 01/13/2016 02:45 AM, Will Deacon wrote:
> On Tue, Jan 12, 2016 at 12:45:14PM -0800, Leonid Yegoshin wrote:
>>
> I don't think the address dependency is enough on its own. By that
> reasoning, the following variant (WRC+addr+addr) would work too:
>
>
> P0:
> Wx = 1
>
> P1:
> Rx == 1
> <address dep>
> Wy = 1
>
> P2:
> Ry == 1
> <address dep>
> Rx = 0
>
>
> So are you saying that this is also forbidden?
> Imagine that P0 and P1 are two threads that share a store buffer. What
> then?
>
I ask HW team about it but I have a question - has it any relationship
with replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)? You use
any barrier or do not use it and I just voice an intention to use a more
efficient instruction instead of bold hummer (SYNC instruction). If you
don't use any barrier here then it is a different issue.
May be it has sense to return back to original issue?
- Leonid
^ permalink raw reply
* [PATCH v3 0/4] x86: faster mb()+documentation tweaks
From: Michael S. Tsirkin @ 2016-01-13 20:12 UTC (permalink / raw)
To: linux-kernel, Linus Torvalds
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, virtualization, Borislav Petkov,
H. Peter Anvin, Thomas Gleixner, Paul E. McKenney, Ingo Molnar
mb() typically uses mfence on modern x86, but a micro-benchmark shows that it's
2 to 3 times slower than lock; addl that we use on older CPUs.
So let's use the locked variant everywhere.
While I was at it, I found some inconsistencies in comments in
arch/x86/include/asm/barrier.h
The documentation fixes are included first - I verified that
they do not change the generated code at all. They should be
safe to apply directly.
The last patch changes mb() to lock addl. I was unable to
measure a speed difference on a macro benchmark,
but I noted that even doing
#define mb() barrier()
seems to make no difference for most benchmarks
(it causes hangs sometimes, of course).
HPA asked that the last patch is deferred until we hear back from
intel, which makes sense of course. So it needs HPA's ack.
I hope I'm not splitting this up too much - the reason is I wanted to isolate
the code changes (that people might want to test for performance)
from comment changes approved by Linus, from (so far unreviewed) changes
I came up with myself.
Changes from v2:
add patch adding cc clobber for addl
tweak commit log for patch 2
use addl at SP-4 (as opposed to SP) to reduce data dependencies
Michael S. Tsirkin (4):
x86: add cc clobber for addl
x86: drop a comment left over from X86_OOSTORE
x86: tweak the comment about use of wmb for IO
x86: drop mfence in favor of lock+addl
arch/x86/include/asm/barrier.h | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
--
MST
^ permalink raw reply
* [PATCH v3 1/4] x86: add cc clobber for addl
From: Michael S. Tsirkin @ 2016-01-13 20:12 UTC (permalink / raw)
To: linux-kernel, Linus Torvalds
Cc: Davidlohr Bueso, Arnd Bergmann, Davidlohr Bueso, Peter Zijlstra,
Andrey Konovalov, the arch/x86 maintainers, virtualization,
Ingo Molnar, Borislav Petkov, Borislav Petkov, Andy Lutomirski,
H. Peter Anvin, Thomas Gleixner, Paul E. McKenney, Ingo Molnar
In-Reply-To: <1452715911-12067-1-git-send-email-mst@redhat.com>
addl clobbers flags (such as CF) but barrier.h didn't tell this to gcc.
Historically, gcc doesn't need one on x86, and always considers flags
clobbered. We are probably missing the cc clobber in a *lot* of places
for this reason.
But even if not necessary, it's probably a good thing to add for
documentation, and in case gcc semantcs ever change.
Reported-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/x86/include/asm/barrier.h | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index a584e1c..a65bdb1 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -15,9 +15,12 @@
* Some non-Intel clones support out of order store. wmb() ceases to be a
* nop for these.
*/
-#define mb() alternative("lock; addl $0,0(%%esp)", "mfence", X86_FEATURE_XMM2)
-#define rmb() alternative("lock; addl $0,0(%%esp)", "lfence", X86_FEATURE_XMM2)
-#define wmb() alternative("lock; addl $0,0(%%esp)", "sfence", X86_FEATURE_XMM)
+#define mb() asm volatile(ALTERNATIVE("lock; addl $0,0(%%esp)", "mfence", \
+ X86_FEATURE_XMM2) ::: "memory", "cc")
+#define rmb() asm volatile(ALTERNATIVE("lock; addl $0,0(%%esp)", "lfence", \
+ X86_FEATURE_XMM2) ::: "memory", "cc")
+#define wmb() asm volatile(ALTERNATIVE("lock; addl $0,0(%%esp)", "sfence", \
+ X86_FEATURE_XMM2) ::: "memory", "cc")
#else
#define mb() asm volatile("mfence":::"memory")
#define rmb() asm volatile("lfence":::"memory")
--
MST
^ permalink raw reply related
* [PATCH v3 2/4] x86: drop a comment left over from X86_OOSTORE
From: Michael S. Tsirkin @ 2016-01-13 20:12 UTC (permalink / raw)
To: linux-kernel, Linus Torvalds
Cc: Davidlohr Bueso, Arnd Bergmann, Davidlohr Bueso, Peter Zijlstra,
Andrey Konovalov, the arch/x86 maintainers, virtualization,
Ingo Molnar, Borislav Petkov, Borislav Petkov, Andy Lutomirski,
H. Peter Anvin, Thomas Gleixner, Paul E. McKenney, Ingo Molnar
In-Reply-To: <1452715911-12067-1-git-send-email-mst@redhat.com>
The comment about wmb being non-nop to deal with non-intel CPUs is a
left over from before commit 09df7c4c8097 ("x86: Remove
CONFIG_X86_OOSTORE").
It makes no sense now: in particular, wmb is not a nop even for regular
intel CPUs because of weird use-cases e.g. dealing with WC memory.
Drop this comment.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/x86/include/asm/barrier.h | 4 ----
1 file changed, 4 deletions(-)
diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index a65bdb1..a291745 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -11,10 +11,6 @@
*/
#ifdef CONFIG_X86_32
-/*
- * Some non-Intel clones support out of order store. wmb() ceases to be a
- * nop for these.
- */
#define mb() asm volatile(ALTERNATIVE("lock; addl $0,0(%%esp)", "mfence", \
X86_FEATURE_XMM2) ::: "memory", "cc")
#define rmb() asm volatile(ALTERNATIVE("lock; addl $0,0(%%esp)", "lfence", \
--
MST
^ permalink raw reply related
* [PATCH v3 3/4] x86: tweak the comment about use of wmb for IO
From: Michael S. Tsirkin @ 2016-01-13 20:12 UTC (permalink / raw)
To: linux-kernel, Linus Torvalds
Cc: Davidlohr Bueso, Arnd Bergmann, Davidlohr Bueso, Peter Zijlstra,
Andrey Konovalov, the arch/x86 maintainers, virtualization,
Ingo Molnar, Borislav Petkov, Borislav Petkov, Andy Lutomirski,
H. Peter Anvin, Thomas Gleixner, Paul E. McKenney, Ingo Molnar
In-Reply-To: <1452715911-12067-1-git-send-email-mst@redhat.com>
On x86, we *do* still use the non-nop rmb/wmb for IO barriers, but even
that is generally questionable.
Leave them around as historial unless somebody can point to a case where
they care about the performance, but tweak the comment so people
don't think they are strictly required in all cases.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/x86/include/asm/barrier.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index a291745..bfb28ca 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -6,7 +6,7 @@
/*
* Force strict CPU ordering.
- * And yes, this is required on UP too when we're talking
+ * And yes, this might be required on UP too when we're talking
* to devices.
*/
--
MST
^ permalink raw reply related
* [PATCH v3 4/4] x86: drop mfence in favor of lock+addl
From: Michael S. Tsirkin @ 2016-01-13 20:12 UTC (permalink / raw)
To: linux-kernel, Linus Torvalds
Cc: Davidlohr Bueso, Arnd Bergmann, Davidlohr Bueso, Peter Zijlstra,
Andrey Konovalov, the arch/x86 maintainers, virtualization,
Andy Lutomirski, Borislav Petkov, Borislav Petkov,
Andy Lutomirski, H. Peter Anvin, Thomas Gleixner,
Paul E. McKenney, Ingo Molnar, Ingo Molnar
In-Reply-To: <1452715911-12067-1-git-send-email-mst@redhat.com>
mfence appears to be way slower than a locked instruction - let's use
lock+add unconditionally, as we always did on old 32-bit.
Just poking at SP would be the most natural, but if we
then read the value from SP, we get a false dependency
which will slow us down.
This was noted in this article:
http://shipilev.net/blog/2014/on-the-fence-with-dependencies/
And is easy to reproduce by sticking a barrier in a small non-inline
function.
So let's use a negative offset - which avoids this problem since we
build with the red zone disabled.
Update rmb/wmb on 32 bit to use the negative offset, too, for
consistency.
Suggested-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/x86/include/asm/barrier.h | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index bfb28ca..9a2d257 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -11,16 +11,15 @@
*/
#ifdef CONFIG_X86_32
-#define mb() asm volatile(ALTERNATIVE("lock; addl $0,0(%%esp)", "mfence", \
- X86_FEATURE_XMM2) ::: "memory", "cc")
-#define rmb() asm volatile(ALTERNATIVE("lock; addl $0,0(%%esp)", "lfence", \
+#define mb() asm volatile("lock; addl $0,-4(%%esp)" ::: "memory", "cc")
+#define rmb() asm volatile(ALTERNATIVE("lock; addl $0,-4(%%esp)", "lfence", \
X86_FEATURE_XMM2) ::: "memory", "cc")
-#define wmb() asm volatile(ALTERNATIVE("lock; addl $0,0(%%esp)", "sfence", \
+#define wmb() asm volatile(ALTERNATIVE("lock; addl $0,-4(%%esp)", "sfence", \
X86_FEATURE_XMM2) ::: "memory", "cc")
#else
-#define mb() asm volatile("mfence":::"memory")
-#define rmb() asm volatile("lfence":::"memory")
-#define wmb() asm volatile("sfence" ::: "memory")
+#define mb() asm volatile("lock; addl $0,-4(%%rsp)" ::: "memory", "cc")
+#define rmb() asm volatile("lfence" ::: "memory")
+#define wmb() asm volatile("sfence" ::: "memory")
#endif
#ifdef CONFIG_X86_PPRO_FENCE
--
MST
^ permalink raw reply related
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Peter Zijlstra @ 2016-01-13 20:48 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Will Deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <56969F4B.7070001@imgtec.com>
On Wed, Jan 13, 2016 at 11:02:35AM -0800, Leonid Yegoshin wrote:
> I ask HW team about it but I have a question - has it any relationship with
> replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)?
Of course. If you cannot explain the semantics of the primitives you
introduce, how can we judge the patch.
This barrier business is hard enough as it is, but magic unexplained
hardware makes it impossible.
Rest assured, you (MIPS) isn't the first (nor likely the last) to go
through all this. We've had these discussions (and to a certain extend
are still having them) for x86, PPC, Alpha, ARM, etc..
Any every time new barriers instructions get introduced we had better
have a full and comprehensive explanation to go along with them.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-13 20:58 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Will Deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160113204844.GV6357@twins.programming.kicks-ass.net>
On 01/13/2016 12:48 PM, Peter Zijlstra wrote:
> On Wed, Jan 13, 2016 at 11:02:35AM -0800, Leonid Yegoshin wrote:
>
>> I ask HW team about it but I have a question - has it any relationship with
>> replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)?
> Of course. If you cannot explain the semantics of the primitives you
> introduce, how can we judge the patch.
>
>
You missed a point - it is a question about replacement of SYNC with
lightweight primitives. It is NOT a question about multithread system
behavior without any SYNC. The answer on a latest Will's question lies
in different area.
- Leonid.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox