* Re: [PATCH] documentation: Add disclaimer
From: Will Deacon @ 2016-01-27 10:11 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, virtualization,
H. Peter Anvin, sparclinux, Ingo Molnar, linux-arch, linux-s390,
Russell King - ARM Linux, user-mode-linux-devel, linux-sh,
Michael Ellerman, x86, xen-devel, Ingo Molnar, Paul E. McKenney,
linux-xtensa, james.hogan, Arnd Bergmann, Stefano Stabellini,
adi-buildroot-devel, Leonid Yegoshin, ddaney.cavm,
Thomas Gleixner, linux-metag
In-Reply-To: <20160127083546.GJ6357@twins.programming.kicks-ass.net>
On Wed, Jan 27, 2016 at 09:35:46AM +0100, Peter Zijlstra wrote:
> On Tue, Jan 26, 2016 at 12:11:43PM -0800, Paul E. McKenney wrote:
> > So Peter, would you like to update your patch to include yourself
> > and Will as authors?
>
> Sure, here goes.
>
> ---
> Subject: documentation: Add disclaimer
>
> It appears people are reading this document as a requirements list for
> building hardware. This is not the intent of this document. Nor is it
> particularly suited for this purpose.
>
> The primary purpose of this document is our collective attempt to define
> a set of primitives that (hopefully) allow us to write correct code on
> the myriad of SMP platforms Linux supports.
>
> Its a definite work in progress as our understanding of these platforms,
> and memory ordering in general, progresses.
>
> Nor does being mentioned in this document mean we think its a
> particularly good idea; the data dependency barrier required by Alpha
> being a prime example. Yes we have it, no you're insane to require it
> when building new hardware.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> Documentation/memory-barriers.txt | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
Acked-by: Will Deacon <will.deacon@arm.com>
Will
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Peter Zijlstra @ 2016-01-27 8:39 UTC (permalink / raw)
To: Paul E. McKenney
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, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, Leonid Yegoshin,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160126201339.GW4503@linux.vnet.ibm.com>
On Tue, Jan 26, 2016 at 12:13:39PM -0800, Paul E. McKenney wrote:
> On Tue, Jan 26, 2016 at 11:19:27AM +0100, Peter Zijlstra wrote:
> > So isn't smp_mb__after_unlock_lock() exactly such a scenario? And would
> > not someone trying to implement RCsc locks using locally transitive
> > RELEASE/ACQUIRE operations need exactly this stuff?
> >
> > That is, I am afraid we need to cover the mix of local and global
> > transitive operations at least in overview.
>
> True, but we haven't gotten to locking yet.
The mythical smp_mb__after_release_acquire() then ;-)
(and yes, I know you're going to say we don't have that)
> That said, I would argue
> that smp_mb__after_unlock_lock() upgrades locks to transitive, and
> thus would not be an exception to the "no combining transitive and
> non-transitive steps in cycles" rule.
But But But ;-) It does that exactly by combining. I suspect this is
(partly) the source of your SC chains with one PC link example.
^ permalink raw reply
* [PATCH] documentation: Add disclaimer
From: Peter Zijlstra @ 2016-01-27 8:35 UTC (permalink / raw)
To: Paul E. McKenney
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, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, Leonid Yegoshin,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160126201143.GV4503@linux.vnet.ibm.com>
On Tue, Jan 26, 2016 at 12:11:43PM -0800, Paul E. McKenney wrote:
> So Peter, would you like to update your patch to include yourself
> and Will as authors?
Sure, here goes.
---
Subject: documentation: Add disclaimer
It appears people are reading this document as a requirements list for
building hardware. This is not the intent of this document. Nor is it
particularly suited for this purpose.
The primary purpose of this document is our collective attempt to define
a set of primitives that (hopefully) allow us to write correct code on
the myriad of SMP platforms Linux supports.
Its a definite work in progress as our understanding of these platforms,
and memory ordering in general, progresses.
Nor does being mentioned in this document mean we think its a
particularly good idea; the data dependency barrier required by Alpha
being a prime example. Yes we have it, no you're insane to require it
when building new hardware.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
Documentation/memory-barriers.txt | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index a61be39c7b51..98626125f484 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -4,8 +4,24 @@
By: David Howells <dhowells@redhat.com>
Paul E. McKenney <paulmck@linux.vnet.ibm.com>
+ Will Deacon <will.deacon@arm.com>
+ Peter Zijlstra <peterz@infradead.org>
-Contents:
+==========
+DISCLAIMER
+==========
+
+This document is not a specification; it is intentionally (for the sake of
+brevity) and unintentionally (due to being human) incomplete. This document is
+meant as a guide to using the various memory barriers provided by Linux, but
+in case of any doubt (and there are many) please ask.
+
+I repeat, this document is not a specification of what Linux expects from
+hardware.
+
+========
+CONTENTS
+========
(*) Abstract memory access model.
^ permalink raw reply related
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Linus Torvalds @ 2016-01-27 8:05 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-mips, linux-ia64@vger.kernel.org, linux-sh, Will Deacon,
virtualization, Peter Anvin, sparclinux, Ingo Molnar,
linux-arch@vger.kernel.org, linux-s390, Russell King - ARM Linux,
Herbert Xu, Michael Ellerman, the arch/x86 maintainers, xen-devel,
Ingo Molnar, Paul McKenney, Arnd Bergmann, linux-xtensa,
James Hogan, uml-devel, Stefano Stabellini <stefano.stabe>
In-Reply-To: <20160127075129.GH6357@twins.programming.kicks-ass.net>
[-- Attachment #1.1: Type: text/plain, Size: 832 bytes --]
On Jan 26, 2016 23:51, "Peter Zijlstra" <peterz@infradead.org> wrote:
>
> So for a moment it looked like MIPS wanted to equal or surpass Alpha in
> this respect.
If there is an architecture that I'd expect to try to take the "sucks more"
crown, MIPS would be it. They've already done the "worst cache award"
thing, and are proud members of the "stupid branch delay slot" crowd. MIPS
historically even did the "delayed load slot".
The only mistake they've never done, AFAIK, is to have a rotating register
file.
At the same time, the "load-to-dependent-store" thing you really have to
*work* at doing wrong. It's not enough to just have bad taste and
incompetent architects. You really have to spend real effort to screw up
that badly. It's really hard to do by mistake.
So I suspect that even MIPS can't get it wrong.
Linus
[-- Attachment #1.2: Type: text/html, Size: 1101 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Peter Zijlstra @ 2016-01-27 7:51 UTC (permalink / raw)
To: Linus Torvalds
Cc: linux-mips, linux-ia64@vger.kernel.org, Michael S. Tsirkin,
Will Deacon, virtualization, Peter Anvin, sparclinux, Ingo Molnar,
linux-arch@vger.kernel.org, linux-s390, Russell King - ARM Linux,
Herbert Xu, linux-sh, Michael Ellerman, the arch/x86 maintainers,
xen-devel, Ingo Molnar, Paul McKenney, linux-xtensa, James Hogan,
uml-devel, Stefano Stabellini <stefan>
In-Reply-To: <CA+55aFwxTJd+uibcxtZD3tGnj_n=LMwyAa0s8qyx_OF0OMWQkA@mail.gmail.com>
On Tue, Jan 26, 2016 at 02:33:40PM -0800, Linus Torvalds wrote:
> If it turns out that some architecture does actually need a barrier
> between a read and a dependent write, then that will mean that
>
> (a) we'll have to make up a _new_ barrier, because
> "smp_read_barrier_depends()" is not that barrier. We'll presumably
> then have to make that new barrier part of "rcu_derefence()" and
> friends.
>
> (b) we will have found an architecture with even worse memory
> ordering semantics than alpha, and we'll have to stop castigating
> alpha for being the worst memory ordering ever.
>
> but I sincerely hope that we'll never find that kind of broken architecture.
So for a moment it looked like MIPS wanted to equal or surpass Alpha in
this respect.
And Paul made the point that smp_read_barrier_depends() really should
be smp_aquire_barrier_depends() in that we rely on both dependent reads
and writes to be ordered against the initial pointer load.
Now, as you've made abundantly clear, Alpha does this, although it needs
the little extra help in the dependent read department.
The 'problem' is that someone seemed to have used our
Documentation/memory-barriers.txt as a specification for what hardware
is permitted and we require. And in that light Paul noted that
read_barrier_depends really should be considered an
acquire_barrier_depends and order both dependent reads and writes
against the (prior) read (if nothing else already does).
Now clearly, any sane architecture doesn't need anything like this, but
again our document doesn't seem to judge. That is, from reading the
document one can get the impression is a perfectly fine thing to do.
Nowhere does our disdain for this thing show.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Boqun Feng @ 2016-01-27 2:04 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-mips, linux-ia64@vger.kernel.org, Michael S. Tsirkin,
Peter Zijlstra, Will Deacon, virtualization, Peter Anvin,
sparclinux, Ingo Molnar, linux-arch@vger.kernel.org, linux-s390,
Russell King - ARM Linux, Herbert Xu, linux-sh, Michael Ellerman,
the arch/x86 maintainers, xen-devel, Ingo Molnar, linux-xtensa,
James Hogan, uml-devel, Stefano Stabellini <stefano.sta>
In-Reply-To: <20160126232921.GY4503@linux.vnet.ibm.com>
[-- Attachment #1.1: Type: text/plain, Size: 4479 bytes --]
On Tue, Jan 26, 2016 at 03:29:21PM -0800, Paul E. McKenney wrote:
> On Tue, Jan 26, 2016 at 02:33:40PM -0800, Linus Torvalds wrote:
> > On Tue, Jan 26, 2016 at 2:15 PM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > You might as well just write it as
> > >
> > > struct foo x = READ_ONCE(*ptr);
> > > x->bar = 5;
> > >
> > > because that "smp_read_barrier_depends()" does NOTHING wrt the second write.
> >
> > Just to clarify: on alpha it adds a memory barrier, but that memory
> > barrier is useless.
>
> No trailing data-dependent read, so agreed, no smp_read_barrier_depends()
> needed. That said, I believe that we should encourage rcu_dereference*()
> or lockless_dereference() instead of READ_ONCE() for documentation
> reasons, though.
>
> > On non-alpha, it is a no-op, and obviously does nothing simply because
> > it generates no code.
> >
> > So if anybody believes that the "smp_read_barrier_depends()" does
> > something, they are *wrong*.
>
> The other problem with smp_read_barrier_depends() is that it is often
> a pain figuring out which prior load it is supposed to apply to.
> Hence my preference for rcu_dereference*() and lockless_dereference().
>
Because semantically speaking, rcu_derefence*() and
lockless_dereference() are CONSUME(i.e. data/address dependent
read->read and read->write pairs are ordered), whereas
smp_read_barrier_depends() only guarantees read->read pairs with data
dependency are ordered, right?
If so, maybe we need to call it out in memory-barriers.txt, for example:
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 904ee42..6b262c2 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1703,8 +1703,8 @@ There are some more advanced barrier functions:
(*) lockless_dereference();
- This can be thought of as a pointer-fetch wrapper around the
- smp_read_barrier_depends() data-dependency barrier.
+ This is a load, and any load or store that has a data dependency on the
+ value returned by this load won't be reordered before this load.
This is also similar to rcu_dereference(), but in cases where
object lifetime is handled by some mechanism other than RCU, for
Regards,
Boqun
> > And if anybody sends out an email with that smp_read_barrier_depends()
> > in an example, they are actively just confusing other people, which is
> > even worse than just being wrong. Which is why I jumped in.
> >
> > So stop perpetuating the myth that smp_read_barrier_depends() does
> > something here. It does not. It's a bug, and it has become this "mind
> > virus" for some people that seem to believe that it does something.
>
> It looks like I should add words to memory-barriers.txt de-emphasizing
> smp_read_barrier_depends(). I will take a look at that.
>
> > I had to remove this crap once from the kernel already, see commit
> > 105ff3cbf225 ("atomic: remove all traces of READ_ONCE_CTRL() and
> > atomic*_read_ctrl()").
> >
> > I don't want to ever see that broken construct again. And I want to
> > make sure that everybody is educated about how broken it was. I'm
> > extremely unhappy that it came up again.
>
> Well, if it makes you feel better, that was control dependencies and this
> was data dependencies. So it was not -exactly- the same. ;-)
>
> (Sorry, couldn't resist...)
>
> > If it turns out that some architecture does actually need a barrier
> > between a read and a dependent write, then that will mean that
> >
> > (a) we'll have to make up a _new_ barrier, because
> > "smp_read_barrier_depends()" is not that barrier. We'll presumably
> > then have to make that new barrier part of "rcu_derefence()" and
> > friends.
>
> Agreed. We can worry about whether or not we replace the current
> smp_read_barrier_depends() with that new barrier when and if such
> hardware appears.
>
> > (b) we will have found an architecture with even worse memory
> > ordering semantics than alpha, and we'll have to stop castigating
> > alpha for being the worst memory ordering ever.
>
> ;-) ;-) ;-)
>
> > but I sincerely hope that we'll never find that kind of broken architecture.
>
> Apparently at least some hardware vendors are reading memory-barriers.txt,
> so perhaps the odds of that kind of breakage have reduced.
>
> Thanx, Paul
>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-27 0:57 UTC (permalink / raw)
To: Linus Torvalds
Cc: linux-mips, linux-ia64@vger.kernel.org, Michael S. Tsirkin,
Peter Zijlstra, Will Deacon, virtualization, Peter Anvin,
sparclinux, Ingo Molnar, linux-arch@vger.kernel.org, linux-s390,
Russell King - ARM Linux, Herbert Xu, linux-sh, Michael Ellerman,
the arch/x86 maintainers, xen-devel, Ingo Molnar, linux-xtensa,
James Hogan, uml-devel, Stefano Stabellini <stefano.sta>
In-Reply-To: <CA+55aFyD94yaA1QzXgfeO18T-czY3TVUi5n6r-9jOUObDeR-zQ@mail.gmail.com>
On Tue, Jan 26, 2016 at 03:45:23PM -0800, Linus Torvalds wrote:
> On Tue, Jan 26, 2016 at 3:29 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > No trailing data-dependent read, so agreed, no smp_read_barrier_depends()
> > needed. That said, I believe that we should encourage rcu_dereference*()
> > or lockless_dereference() instead of READ_ONCE() for documentation
> > reasons, though.
>
> I agree that that is likely the right thing to do in pretty much all situations.
>
> In theory, there might be performance situations where we'd want to
> actively avoid the smp_read_barrier_depends() inherent in those, but
> considering that it's only a performance issue on alpha, and we
> probably have all of two or three people using Linux on alpha, it's a
> pretty theoretical performance worry.
Agreed!
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Linus Torvalds @ 2016-01-26 23:45 UTC (permalink / raw)
To: Paul McKenney
Cc: linux-mips, linux-ia64@vger.kernel.org, Michael S. Tsirkin,
Peter Zijlstra, Will Deacon, virtualization, Peter Anvin,
sparclinux, Ingo Molnar, linux-arch@vger.kernel.org, linux-s390,
Russell King - ARM Linux, Herbert Xu, linux-sh, Michael Ellerman,
the arch/x86 maintainers, xen-devel, Ingo Molnar, linux-xtensa,
James Hogan, uml-devel, Stefano Stabellini <stefano.sta>
In-Reply-To: <20160126232921.GY4503@linux.vnet.ibm.com>
On Tue, Jan 26, 2016 at 3:29 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> No trailing data-dependent read, so agreed, no smp_read_barrier_depends()
> needed. That said, I believe that we should encourage rcu_dereference*()
> or lockless_dereference() instead of READ_ONCE() for documentation
> reasons, though.
I agree that that is likely the right thing to do in pretty much all situations.
In theory, there might be performance situations where we'd want to
actively avoid the smp_read_barrier_depends() inherent in those, but
considering that it's only a performance issue on alpha, and we
probably have all of two or three people using Linux on alpha, it's a
pretty theoretical performance worry.
Linus
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-26 23:37 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, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, Leonid Yegoshin,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160126121010.GD21553@arm.com>
On Tue, Jan 26, 2016 at 12:10:10PM +0000, Will Deacon wrote:
> On Mon, Jan 25, 2016 at 05:06:46PM -0800, Paul E. McKenney wrote:
> > On Mon, Jan 25, 2016 at 02:41:34PM +0000, Will Deacon wrote:
> > > On Fri, Jan 15, 2016 at 11:28:45AM -0800, Paul E. McKenney wrote:
> > > > On Fri, Jan 15, 2016 at 09:54:01AM -0800, Paul E. McKenney wrote:
> > > > > On Fri, Jan 15, 2016 at 10:24:32AM +0000, Will Deacon wrote:
> > > > > > See my earlier reply [1] (but also, your WRC Linux example looks more
> > > > > > like a variant on WWC and I couldn't really follow it).
> > > > >
> > > > > I will revisit my WRC Linux example. And yes, creating litmus tests
> > > > > that use non-fake dependencies is still a bit of an undertaking. :-/
> > > > > I am sure that it will seem more natural with time and experience...
> > > >
> > > > Hmmm... You are quite right, I did do WWC. I need to change cpu2()'s
> > > > last access from a store to a load to get WRC. Plus the levels of
> > > > indirection definitely didn't match up, did they?
> > >
> > > Nope, it was pretty baffling!
> >
> > "It is a service that I provide." ;-)
> >
> > > > struct foo {
> > > > struct foo *next;
> > > > };
> > > > struct foo a;
> > > > struct foo b;
> > > > struct foo c = { &a };
> > > > struct foo d = { &b };
> > > > struct foo x = { &c };
> > > > struct foo y = { &d };
> > > > struct foo *r1, *r2, *r3;
> > > >
> > > > void cpu0(void)
> > > > {
> > > > WRITE_ONCE(x.next, &y);
> > > > }
> > > >
> > > > void cpu1(void)
> > > > {
> > > > r1 = lockless_dereference(x.next);
> > > > WRITE_ONCE(r1->next, &x);
> > > > }
> > > >
> > > > void cpu2(void)
> > > > {
> > > > r2 = lockless_dereference(y.next);
> > > > r3 = READ_ONCE(r2->next);
> > > > }
> > > >
> > > > In this case, it is legal to end the run with:
> > > >
> > > > r1 == &y && r2 == &x && r3 == &c
> > > >
> > > > Please see below for a ppcmem litmus test.
> > > >
> > > > So, did I get it right this time? ;-)
> > >
> > > The code above looks correct to me (in that it matches WRC+addrs),
> > > but your litmus test:
> > >
> > > > PPC WRCnf+addrs
> > > > ""
> > > > {
> > > > 0:r2=x; 0:r3=y;
> > > > 1:r2=x; 1:r3=y;
> > > > 2:r2=x; 2:r3=y;
> > > > c=a; d=b; x=c; y=d;
> > > > }
> > > > P0 | P1 | P2 ;
> > > > stw r3,0(r2) | lwz r8,0(r2) | lwz r8,0(r3) ;
> > > > | stw r2,0(r3) | lwz r9,0(r8) ;
> > > > exists
> > > > (1:r8=y /\ 2:r8=x /\ 2:r9=c)
> > >
> > > Seems to be missing the address dependency on P1.
> >
> > You are quite correct! How about the following?
>
> I think that's it!
>
> > As before, both herd and ppcmem say that the cycle is allowed, as
> > expected, given non-transitive ordering. To prohibit the cycle, P1
> > needs a suitable memory-barrier instruction.
> >
> > ------------------------------------------------------------------------
> >
> > PPC WRCnf+addrs
> > ""
> > {
> > 0:r2=x; 0:r3=y;
> > 1:r2=x; 1:r3=y;
> > 2:r2=x; 2:r3=y;
> > c=a; d=b; x=c; y=d;
> > }
> > P0 | P1 | P2 ;
> > stw r3,0(r2) | lwz r8,0(r2) | lwz r8,0(r3) ;
> > | stw r2,0(r8) | lwz r9,0(r8) ;
> > exists
> > (1:r8=y /\ 2:r8=x /\ 2:r9=c)
>
> Agreed.
OK, thank you! Would you agree that it would be good to replace the
current xor-based fake-dependency litmus tests with tests having real
dependencies?
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-26 23:29 UTC (permalink / raw)
To: Linus Torvalds
Cc: linux-mips, linux-ia64@vger.kernel.org, Michael S. Tsirkin,
Peter Zijlstra, Will Deacon, virtualization, Peter Anvin,
sparclinux, Ingo Molnar, linux-arch@vger.kernel.org, linux-s390,
Russell King - ARM Linux, Herbert Xu, linux-sh, Michael Ellerman,
the arch/x86 maintainers, xen-devel, Ingo Molnar, linux-xtensa,
James Hogan, uml-devel, Stefano Stabellini <stefano.sta>
In-Reply-To: <CA+55aFwxTJd+uibcxtZD3tGnj_n=LMwyAa0s8qyx_OF0OMWQkA@mail.gmail.com>
On Tue, Jan 26, 2016 at 02:33:40PM -0800, Linus Torvalds wrote:
> On Tue, Jan 26, 2016 at 2:15 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > You might as well just write it as
> >
> > struct foo x = READ_ONCE(*ptr);
> > x->bar = 5;
> >
> > because that "smp_read_barrier_depends()" does NOTHING wrt the second write.
>
> Just to clarify: on alpha it adds a memory barrier, but that memory
> barrier is useless.
No trailing data-dependent read, so agreed, no smp_read_barrier_depends()
needed. That said, I believe that we should encourage rcu_dereference*()
or lockless_dereference() instead of READ_ONCE() for documentation
reasons, though.
> On non-alpha, it is a no-op, and obviously does nothing simply because
> it generates no code.
>
> So if anybody believes that the "smp_read_barrier_depends()" does
> something, they are *wrong*.
The other problem with smp_read_barrier_depends() is that it is often
a pain figuring out which prior load it is supposed to apply to.
Hence my preference for rcu_dereference*() and lockless_dereference().
> And if anybody sends out an email with that smp_read_barrier_depends()
> in an example, they are actively just confusing other people, which is
> even worse than just being wrong. Which is why I jumped in.
>
> So stop perpetuating the myth that smp_read_barrier_depends() does
> something here. It does not. It's a bug, and it has become this "mind
> virus" for some people that seem to believe that it does something.
It looks like I should add words to memory-barriers.txt de-emphasizing
smp_read_barrier_depends(). I will take a look at that.
> I had to remove this crap once from the kernel already, see commit
> 105ff3cbf225 ("atomic: remove all traces of READ_ONCE_CTRL() and
> atomic*_read_ctrl()").
>
> I don't want to ever see that broken construct again. And I want to
> make sure that everybody is educated about how broken it was. I'm
> extremely unhappy that it came up again.
Well, if it makes you feel better, that was control dependencies and this
was data dependencies. So it was not -exactly- the same. ;-)
(Sorry, couldn't resist...)
> If it turns out that some architecture does actually need a barrier
> between a read and a dependent write, then that will mean that
>
> (a) we'll have to make up a _new_ barrier, because
> "smp_read_barrier_depends()" is not that barrier. We'll presumably
> then have to make that new barrier part of "rcu_derefence()" and
> friends.
Agreed. We can worry about whether or not we replace the current
smp_read_barrier_depends() with that new barrier when and if such
hardware appears.
> (b) we will have found an architecture with even worse memory
> ordering semantics than alpha, and we'll have to stop castigating
> alpha for being the worst memory ordering ever.
;-) ;-) ;-)
> but I sincerely hope that we'll never find that kind of broken architecture.
Apparently at least some hardware vendors are reading memory-barriers.txt,
so perhaps the odds of that kind of breakage have reduced.
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Linus Torvalds @ 2016-01-26 22:33 UTC (permalink / raw)
To: Paul McKenney
Cc: linux-mips, linux-ia64@vger.kernel.org, Michael S. Tsirkin,
Peter Zijlstra, Will Deacon, virtualization, Peter Anvin,
sparclinux, Ingo Molnar, linux-arch@vger.kernel.org, linux-s390,
Russell King - ARM Linux, Herbert Xu, linux-sh, Michael Ellerman,
the arch/x86 maintainers, xen-devel, Ingo Molnar, linux-xtensa,
James Hogan, uml-devel, Stefano Stabellini <stefano.sta>
In-Reply-To: <CA+55aFxjb+2rs2wVHtiSCcOzgMrE8H=yDeNcjyujPQudDCtLgw@mail.gmail.com>
On Tue, Jan 26, 2016 at 2:15 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> You might as well just write it as
>
> struct foo x = READ_ONCE(*ptr);
> x->bar = 5;
>
> because that "smp_read_barrier_depends()" does NOTHING wrt the second write.
Just to clarify: on alpha it adds a memory barrier, but that memory
barrier is useless.
On non-alpha, it is a no-op, and obviously does nothing simply because
it generates no code.
So if anybody believes that the "smp_read_barrier_depends()" does
something, they are *wrong*.
And if anybody sends out an email with that smp_read_barrier_depends()
in an example, they are actively just confusing other people, which is
even worse than just being wrong. Which is why I jumped in.
So stop perpetuating the myth that smp_read_barrier_depends() does
something here. It does not. It's a bug, and it has become this "mind
virus" for some people that seem to believe that it does something.
I had to remove this crap once from the kernel already, see commit
105ff3cbf225 ("atomic: remove all traces of READ_ONCE_CTRL() and
atomic*_read_ctrl()").
I don't want to ever see that broken construct again. And I want to
make sure that everybody is educated about how broken it was. I'm
extremely unhappy that it came up again.
If it turns out that some architecture does actually need a barrier
between a read and a dependent write, then that will mean that
(a) we'll have to make up a _new_ barrier, because
"smp_read_barrier_depends()" is not that barrier. We'll presumably
then have to make that new barrier part of "rcu_derefence()" and
friends.
(b) we will have found an architecture with even worse memory
ordering semantics than alpha, and we'll have to stop castigating
alpha for being the worst memory ordering ever.
but I sincerely hope that we'll never find that kind of broken architecture.
Linus
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Linus Torvalds @ 2016-01-26 22:15 UTC (permalink / raw)
To: Paul McKenney
Cc: linux-mips, linux-ia64@vger.kernel.org, Michael S. Tsirkin,
Peter Zijlstra, Will Deacon, virtualization, Peter Anvin,
sparclinux, Ingo Molnar, linux-arch@vger.kernel.org, linux-s390,
Russell King - ARM Linux, Herbert Xu, linux-sh, Michael Ellerman,
the arch/x86 maintainers, xen-devel, Ingo Molnar, linux-xtensa,
James Hogan, uml-devel, Stefano Stabellini <stefano.sta>
In-Reply-To: <20160126201037.GU4503@linux.vnet.ibm.com>
On Tue, Jan 26, 2016 at 12:10 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Tue, Jan 26, 2016 at 11:44:46AM -0800, Linus Torvalds wrote:
>>
>> > struct foo *x = READ_ONCE(*ptr);
>> > smp_read_barrier_depends();
>> > x->bar = 5;
>>
>> This case is complete BS. Stop perpetuating it. I already removed a
>> number of bogus cases of it, and I removed the incorrect documentation
>> that had this crap.
>
> If I understand your objection correctly, you want the above pattern
> expressed either like this:
>
> struct foo *x = rcu_dereference(*ptr);
> x->bar = 5;
>
> Or like this:
>
> struct foo *x = lockless_dereference(*ptr);
> x->bar = 5;
>
> Or am I missing your point?
You are entirely missing the point.
You might as well just write it as
struct foo x = READ_ONCE(*ptr);
x->bar = 5;
because that "smp_read_barrier_depends()" does NOTHING wrt the second write.
So what I am saying is simple: anybody who writes that
"smp_read_barrier_depends()" in there is just ttoally and completely
WRONG, and the fact that Peter wrote it out after I removed several
instances of that bloody f*cking idiocy is disturbing.
Don't do it. It's BS. It's wrong. Don't make excuses for it.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/3] x86: faster mb()+other barrier.h tweaks
From: H. Peter Anvin @ 2016-01-26 21:37 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, linux-kernel, virtualization,
Borislav Petkov, Thomas Gleixner, Paul E. McKenney,
Linus Torvalds, Ingo Molnar
In-Reply-To: <20160126101921-mutt-send-email-mst@redhat.com>
On 01/26/16 00:20, Michael S. Tsirkin wrote:
> On Tue, Jan 12, 2016 at 02:25:24PM -0800, H. Peter Anvin wrote:
>
> Hi hpa,
> Any luck getting some feedback on this one?
>
Yes. What we know so far is that *most* cases it will work, but there
are apparently a few corner cases where MFENCE or a full-blown
serializing instruction is necessary. We are trying to characterize
those corner cases and see if any of them affect the kernel.
Even if they are, we can probably make those barriers explicitly
different, but we don't want to go ahead with the change until we know
where we need to care.
-hpa
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-26 20:13 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, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, Leonid Yegoshin,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160126101927.GD6357@twins.programming.kicks-ass.net>
On Tue, Jan 26, 2016 at 11:19:27AM +0100, Peter Zijlstra wrote:
> On Mon, Jan 25, 2016 at 10:03:22PM -0800, Paul E. McKenney wrote:
> > On Mon, Jan 25, 2016 at 04:42:43PM +0000, Will Deacon wrote:
> > > On Fri, Jan 15, 2016 at 01:58:53PM -0800, Paul E. McKenney wrote:
> > > > On Fri, Jan 15, 2016 at 10:27:14PM +0100, Peter Zijlstra wrote:
>
> > > > > Yes, that seems a good start. But yesterday you raised the 'fun' point
> > > > > of two globally ordered sequences connected by a single local link.
> > > >
> > > > The conclusion that I am slowly coming to is that litmus tests should
> > > > not be thought of as linear chains, but rather as cycles. If you think
> > > > of it as a cycle, then it doesn't matter where the local link is, just
> > > > how many of them and how they are connected.
> > >
> > > Do you have some examples of this? I'm struggling to make it work in my
> > > mind, or are you talking specifically in the context of the kernel
> > > memory model?
> >
> > Now that you mention it, maybe it would be best to keep the transitive
> > and non-transitive separate for the time being anyway. Just because it
> > might be possible to deal with does not necessarily mean that we should
> > be encouraging it. ;-)
>
> So isn't smp_mb__after_unlock_lock() exactly such a scenario? And would
> not someone trying to implement RCsc locks using locally transitive
> RELEASE/ACQUIRE operations need exactly this stuff?
>
> That is, I am afraid we need to cover the mix of local and global
> transitive operations at least in overview.
True, but we haven't gotten to locking yet. That said, I would argue
that smp_mb__after_unlock_lock() upgrades locks to transitive, and
thus would not be an exception to the "no combining transitive and
non-transitive steps in cycles" rule.
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-26 20:11 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, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, Leonid Yegoshin,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160126110053.GA21553@arm.com>
On Tue, Jan 26, 2016 at 11:09:27AM +0000, Will Deacon wrote:
> On Tue, Jan 26, 2016 at 11:32:00AM +0100, Peter Zijlstra wrote:
> > On Tue, Jan 26, 2016 at 11:24:02AM +0100, Peter Zijlstra wrote:
> >
> > > Yeah, this goes under the header: memory-barriers.txt is _NOT_ a
> > > specification (I seem to keep repeating this).
> >
> > Do we want this ?
Seems likely to me. ;-)
> > ---
> > Documentation/memory-barriers.txt | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> > index a61be39c7b51..433326ebdc26 100644
> > --- a/Documentation/memory-barriers.txt
> > +++ b/Documentation/memory-barriers.txt
> > @@ -1,3 +1,4 @@
> > +
> > ============================
> > LINUX KERNEL MEMORY BARRIERS
> > ============================
> > @@ -5,6 +6,22 @@
> > By: David Howells <dhowells@redhat.com>
> > Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >
> > +==========
> > +DISCLAIMER
> > +==========
> > +
> > +This document is not a specification; it is intentionally (for the sake of
> > +brevity) and unintentionally (due to being human) incomplete. This document is
> > +meant as a guide to using the various memory barriers provided by Linux, but
> > +in case of any doubt (and there are many) please ask.
>
> It might be worth adding you and me to the top of the file, to save Paul
> Cc'ing us on questions (get_maintainer.pl points at poor old Corbet for
> this file).
>
> But yes, it seems that something like this is required.
So Peter, would you like to update your patch to include yourself
and Will as authors?
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-26 20:10 UTC (permalink / raw)
To: Linus Torvalds
Cc: linux-mips, linux-ia64@vger.kernel.org, Michael S. Tsirkin,
Peter Zijlstra, Will Deacon, virtualization, Peter Anvin,
sparclinux, Ingo Molnar, linux-arch@vger.kernel.org, linux-s390,
Russell King - ARM Linux, Herbert Xu, linux-sh, Michael Ellerman,
the arch/x86 maintainers, xen-devel, Ingo Molnar, linux-xtensa,
James Hogan, uml-devel, Stefano Stabellini <stefano.sta>
In-Reply-To: <CA+55aFzcC6C8imPs5vk4yH1Y2YHjnAdFM9HCkVs04COxuDQH6w@mail.gmail.com>
On Tue, Jan 26, 2016 at 11:44:46AM -0800, Linus Torvalds wrote:
> On Tue, Jan 26, 2016 at 9:22 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > This is distinct from:
>
> That may be distinct, but:
>
> > struct foo *x = READ_ONCE(*ptr);
> > smp_read_barrier_depends();
> > x->bar = 5;
>
> This case is complete BS. Stop perpetuating it. I already removed a
> number of bogus cases of it, and I removed the incorrect documentation
> that had this crap.
If I understand your objection correctly, you want the above pattern
expressed either like this:
struct foo *x = rcu_dereference(*ptr);
x->bar = 5;
Or like this:
struct foo *x = lockless_dereference(*ptr);
x->bar = 5;
Or am I missing your point?
> It's called "smp_READ_barrier_depends()" for a reason.
>
> Alpha is the only one that needs it, and alpha needs it only for
> dependent READS.
>
> It's not called smp_read_write_barrier_depends(). It's not called
> "smp_mb_depends()". It's a weaker form of "smp_rmb()", nothing else.
>
> So alpha does have an implied dependency chain from a read to a
> subsequent dependent write, and does not need any extra barriers.
>
> Alpha does *not* have a dependency chain from a read to a subsequent
> read, which is why we need that horrible crappy
> smp_read_barrier_depends(). But it's the only reason.
>
> This is the alpha reference manual wrt read-to-write dependency:
>
> 5.6.1.7 Definition of Dependence Constraint
>
> The depends relation (DP) is defined as follows. Given u and v
> issued by processor Pi, where u
> is a read or an instruction fetch and v is a write, u precedes v
> in DP order (written u DP v, that
> is, v depends on u) in either of the following situations:
>
> • u determines the execution of v, the location accessed by v, or
> the value written by v.
> • u determines the execution or address or value of another
> memory access z that precedes
>
> v or might precede v (that is, would precede v in some execution
> path depending
> on the value read by u) by processor issue constraint (see Section 5.6.1.3).
>
> Note that the dependence barrier honors not only control flow, but
> address and data values too. This is a different syntax than we use,
> but 'u' is the READ_ONCE, and 'v' is the write. Any data, address or
> conditional dependency between the two implies an ordering.
>
> So no, "smp_read_barrier_depends()" is *ONLY* about two reads, where
> the second read is data-dependent on the first. Nothing else.
>
> So if you _ever_ see a "smp_read_barrier_depends()" that isn't about a
> barrier between two reads, then that is a bug.
And the smp_read_barrier_depends() in both rcu_dereference() and
in lockless_dereference() is ordering the read-to-read case and the
underlying hardware is ordering the read-to-write case on weakly ordered
hardware.
Or, again, am I missing your point?
Thanx, Paul
> The above code is crap. It's exactly as much crap as
>
> a = READ_ONCE(x);
> smp_rmb();
> WRITE_ONCE(b, y);
>
> because a "rmb()" simply doesn't have anything to do with
> read-vs-subsequent-write ordering.
>
> Linus
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-26 19:58 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, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, Leonid Yegoshin,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160126121608.GE21553@arm.com>
On Tue, Jan 26, 2016 at 12:16:09PM +0000, Will Deacon wrote:
> On Mon, Jan 25, 2016 at 10:03:22PM -0800, Paul E. McKenney wrote:
> > On Mon, Jan 25, 2016 at 04:42:43PM +0000, Will Deacon wrote:
> > > On Fri, Jan 15, 2016 at 01:58:53PM -0800, Paul E. McKenney wrote:
> > > > PPC Overlapping Group-B sets version 4
> > > > ""
> > > > (* When the Group-B sets from two different barriers involve instructions in
> > > > the same thread, within that thread one set must contain the other.
> > > >
> > > > P0 P1 P2
> > > > Rx=1 Wy=1 Wz=2
> > > > dep. lwsync lwsync
> > > > Ry=0 Wz=1 Wx=1
> > > > Rz=1
> > > >
> > > > assert(!(z=2))
> > > >
> > > > Forbidden by ppcmem, allowed by herd.
> > > > *)
> > > > {
> > > > 0:r1=x; 0:r2=y; 0:r3=z;
> > > > 1:r1=x; 1:r2=y; 1:r3=z; 1:r4=1;
> > > > 2:r1=x; 2:r2=y; 2:r3=z; 2:r4=1; 2:r5=2;
> > > > }
> > > > P0 | P1 | P2 ;
> > > > lwz r6,0(r1) | stw r4,0(r2) | stw r5,0(r3) ;
> > > > xor r7,r6,r6 | lwsync | lwsync ;
> > > > lwzx r7,r7,r2 | stw r4,0(r3) | stw r4,0(r1) ;
> > > > lwz r8,0(r3) | | ;
> > > >
> > > > exists
> > > > (z=2 /\ 0:r6=1 /\ 0:r7=0 /\ 0:r8=1)
> > >
> > > That really hurts. Assuming that the "assert(!(z=2))" is actually there
> > > to constrain the coherence order of z to be {0->1->2}, then I think that
> > > this test is forbidden on arm using dmb instead of lwsync. That said, I
> > > also don't think the Rz=1 in P0 changes anything.
> >
> > What about the smp_wmb() variant of dmb that orders only stores?
>
> Tricky, but I think it still works out if the coherence order of z is as
> I described above. The line of reasoning is weird though -- I ended up
> considering the two cases where P0 reads z before and after it reads x
> and what that means for the read of y.
By "works out" you mean that ARM prohibits the outcome?
BTW, I never have seen a real-world use for this case. At the moment
it is mostly a cautionary tale about memory-model corner cases and
tools.
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-26 19:51 UTC (permalink / raw)
To: Boqun Feng
Cc: linux-mips, linux-ia64, mst, peterz, will.deacon, virtualization,
hpa, sparclinux, mingo, linux-arch, linux-s390, linux, Herbert Xu,
linux-sh, mpe, x86, Linus Torvalds, xen-devel, mingo,
linux-xtensa, james.hogan, user-mode-linux-devel,
stefano.stabellini, adi-buildroot-devel, Leonid.Yegoshin,
ddaney.cavm, tglx, linux-metag, linux-arm-kernel, andrew.cooper3,
linux-kernel, ralf
In-Reply-To: <20160126165207.GB6029@fixme-laptop.cn.ibm.com>
On Wed, Jan 27, 2016 at 12:52:07AM +0800, Boqun Feng wrote:
> Hi Paul,
>
> On Mon, Jan 18, 2016 at 07:46:29AM -0800, Paul E. McKenney wrote:
> > On Mon, Jan 18, 2016 at 04:19:29PM +0800, Herbert Xu wrote:
> > > Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > > >
> > > > You could use SYNC_ACQUIRE() to implement read_barrier_depends() and
> > > > smp_read_barrier_depends(), but SYNC_RMB probably does not suffice.
> > > > The reason for this is that smp_read_barrier_depends() must order the
> > > > pointer load against any subsequent read or write through a dereference
> > > > of that pointer. For example:
> > > >
> > > > p = READ_ONCE(gp);
> > > > smp_rmb();
> > > > r1 = p->a; /* ordered by smp_rmb(). */
> > > > p->b = 42; /* NOT ordered by smp_rmb(), BUG!!! */
> > > > r2 = x; /* ordered by smp_rmb(), but doesn't need to be. */
> > > >
> > > > In contrast:
> > > >
> > > > p = READ_ONCE(gp);
> > > > smp_read_barrier_depends();
> > > > r1 = p->a; /* ordered by smp_read_barrier_depends(). */
> > > > p->b = 42; /* ordered by smp_read_barrier_depends(). */
> > > > r2 = x; /* not ordered by smp_read_barrier_depends(), which is OK. */
> > > >
> > > > Again, if your hardware maintains local ordering for address
> > > > and data dependencies, you can have read_barrier_depends() and
> > > > smp_read_barrier_depends() be no-ops like they are for most
> > > > architectures.
> > > >
> > > > Does that help?
> > >
> > > This is crazy! smp_rmb started out being strictly stronger than
> > > smp_read_barrier_depends, when did this stop being the case?
> >
> > Hello, Herbert!
> >
> > It is true that most Linux kernel code relies only on the read-read
> > properties of dependencies, but the read-write properties are useful.
> > Admittedly relatively rarely, but useful.
> >
> > The better comparison for smp_read_barrier_depends(), especially in
> > its rcu_dereference*() form, is smp_load_acquire().
>
> Confused..
>
> I recall that last time you and Linus came into a conclusion that even
> on Alpha, a barrier for read->write with data dependency is unnecessary:
>
> http://article.gmane.org/gmane.linux.kernel/2077661
>
> And in an earlier mail of that thread, Linus made his point that
> smp_read_barrier_depends() should only be used to order read->read.
Those examples involved read-to-write with conditionals, as in:
if (READ_ONCE(a))
WRITE_ONCE(b, 1);
Without the "if", no ordering is guaranteed on weakly ordered CPUs.
(The volatile accesses keep ordering within the compiler for once...
> So right now, are we going to extend the semantics of
> smp_read_barrier_depends()? Can we just make smp_read_barrier_depends()
> still only work for read->read, and assume all the architectures won't
> reorder read->write with data dependency, so that the code above having
> a smp_rmb() also works?
The semantics of smp_read_barrier_depends() has been both read-to-write
and read-to-read for some time now, this patch just catches the
documentation up with reality.
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Linus Torvalds @ 2016-01-26 19:44 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-mips, linux-ia64@vger.kernel.org, Michael S. Tsirkin,
Will Deacon, virtualization, Peter Anvin, sparclinux, Ingo Molnar,
linux-arch@vger.kernel.org, linux-s390, Russell King - ARM Linux,
Herbert Xu, linux-sh, Michael Ellerman, the arch/x86 maintainers,
xen-devel, Ingo Molnar, Paul E. McKenney, linux-xtensa,
James Hogan, uml-devel
In-Reply-To: <20160126172227.GG6357@twins.programming.kicks-ass.net>
On Tue, Jan 26, 2016 at 9:22 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> This is distinct from:
That may be distinct, but:
> struct foo *x = READ_ONCE(*ptr);
> smp_read_barrier_depends();
> x->bar = 5;
This case is complete BS. Stop perpetuating it. I already removed a
number of bogus cases of it, and I removed the incorrect documentation
that had this crap.
It's called "smp_READ_barrier_depends()" for a reason.
Alpha is the only one that needs it, and alpha needs it only for
dependent READS.
It's not called smp_read_write_barrier_depends(). It's not called
"smp_mb_depends()". It's a weaker form of "smp_rmb()", nothing else.
So alpha does have an implied dependency chain from a read to a
subsequent dependent write, and does not need any extra barriers.
Alpha does *not* have a dependency chain from a read to a subsequent
read, which is why we need that horrible crappy
smp_read_barrier_depends(). But it's the only reason.
This is the alpha reference manual wrt read-to-write dependency:
5.6.1.7 Definition of Dependence Constraint
The depends relation (DP) is defined as follows. Given u and v
issued by processor Pi, where u
is a read or an instruction fetch and v is a write, u precedes v
in DP order (written u DP v, that
is, v depends on u) in either of the following situations:
• u determines the execution of v, the location accessed by v, or
the value written by v.
• u determines the execution or address or value of another
memory access z that precedes
v or might precede v (that is, would precede v in some execution
path depending
on the value read by u) by processor issue constraint (see Section 5.6.1.3).
Note that the dependence barrier honors not only control flow, but
address and data values too. This is a different syntax than we use,
but 'u' is the READ_ONCE, and 'v' is the write. Any data, address or
conditional dependency between the two implies an ordering.
So no, "smp_read_barrier_depends()" is *ONLY* about two reads, where
the second read is data-dependent on the first. Nothing else.
So if you _ever_ see a "smp_read_barrier_depends()" that isn't about a
barrier between two reads, then that is a bug.
The above code is crap. It's exactly as much crap as
a = READ_ONCE(x);
smp_rmb();
WRITE_ONCE(b, y);
because a "rmb()" simply doesn't have anything to do with
read-vs-subsequent-write ordering.
Linus
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-26 19:44 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, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, Leonid Yegoshin,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160126102402.GE6357@twins.programming.kicks-ass.net>
On Tue, Jan 26, 2016 at 11:24:02AM +0100, Peter Zijlstra wrote:
> On Thu, Jan 14, 2016 at 02:20:46PM -0800, Paul E. McKenney wrote:
> > On Thu, Jan 14, 2016 at 01:24:34PM -0800, Leonid Yegoshin wrote:
> > > On 01/14/2016 12:48 PM, Paul E. McKenney wrote:
> > > >
> > > >So SYNC_RMB is intended to implement smp_rmb(), correct?
> > > Yes.
> > > >
> > > >You could use SYNC_ACQUIRE() to implement read_barrier_depends() and
> > > >smp_read_barrier_depends(), but SYNC_RMB probably does not suffice.
> > >
> > > If smp_read_barrier_depends() is used to separate not only two reads
> > > but read pointer and WRITE basing on that pointer (example below) -
> > > yes. I just doesn't see any example of this in famous
> > > Documentation/memory-barriers.txt and had no chance to know what you
> > > use it in this way too.
> >
> > Well, Documentation/memory-barriers.txt was intended as a guide for Linux
> > kernel hackers, and not for hardware architects.
>
> Yeah, this goes under the header: memory-barriers.txt is _NOT_ a
> specification (I seem to keep repeating this).
>
> > ------------------------------------------------------------------------
> >
> > commit 955720966e216b00613fcf60188d507c103f0e80
> > Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Date: Thu Jan 14 14:17:04 2016 -0800
> >
> > documentation: Subsequent writes ordered by rcu_dereference()
> >
> > The current memory-barriers.txt does not address the possibility of
> > a write to a dereferenced pointer. This should be rare,
>
> How are these rare? Isn't:
>
> rcu_read_lock()
> obj = rcu_dereference(ptr);
> if (!atomic_inc_not_zero(&obj->ref))
> obj = NULL;
> rcu_read_unlock();
>
> a _very_ common thing to do?
It is, but it provides its own barriers, so does not need to rely on
dependency ordering.
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Peter Zijlstra @ 2016-01-26 17:22 UTC (permalink / raw)
To: Boqun Feng
Cc: linux-mips, linux-ia64, mst, will.deacon, virtualization, hpa,
sparclinux, mingo, linux-arch, linux-s390, linux, Herbert Xu,
linux-sh, mpe, x86, Linus Torvalds, xen-devel, mingo,
Paul E. McKenney, linux-xtensa, james.hogan,
user-mode-linux-devel, stefano.stabellini, adi-buildroot-devel,
Leonid.Yegoshin, ddaney.cavm, tglx, linux-metag, linux-arm-kernel,
andrew.cooper3, linux-kernel
In-Reply-To: <20160126165207.GB6029@fixme-laptop.cn.ibm.com>
On Wed, Jan 27, 2016 at 12:52:07AM +0800, Boqun Feng wrote:
> I recall that last time you and Linus came into a conclusion that even
> on Alpha, a barrier for read->write with data dependency is unnecessary:
>
> http://article.gmane.org/gmane.linux.kernel/2077661
>
> And in an earlier mail of that thread, Linus made his point that
> smp_read_barrier_depends() should only be used to order read->read.
>
> So right now, are we going to extend the semantics of
> smp_read_barrier_depends()? Can we just make smp_read_barrier_depends()
> still only work for read->read, and assume all the architectures won't
> reorder read->write with data dependency, so that the code above having
> a smp_rmb() also works?
That discussions was about control dependencies. So writes that _depend_
on a prior read having an explicit value.
So something like:
struct foo *x = READ_ONCE(*ptr);
smp_read_barrier_depends()
if (x->val == 5)
x->bar = 5;
In that case, the load of x->val must be complete and its value
determined _before_ the store to x->bar can happen.
This is distinct from:
struct foo *x = READ_ONCE(*ptr);
smp_read_barrier_depends();
x->bar = 5;
And its the second case where smp_read_barrier_depends() read->write
order matters.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Boqun Feng @ 2016-01-26 16:52 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-mips, linux-ia64, mst, peterz, will.deacon, virtualization,
hpa, sparclinux, mingo, linux-arch, linux-s390, linux, Herbert Xu,
linux-sh, mpe, x86, Linus Torvalds, xen-devel, mingo,
linux-xtensa, james.hogan, user-mode-linux-devel,
stefano.stabellini, adi-buildroot-devel, Leonid.Yegoshin,
ddaney.cavm, tglx, linux-metag, linux-arm-kernel, andrew.cooper3,
linux-kernel, ralf
In-Reply-To: <20160118154629.GB3818@linux.vnet.ibm.com>
[-- Attachment #1.1: Type: text/plain, Size: 2574 bytes --]
Hi Paul,
On Mon, Jan 18, 2016 at 07:46:29AM -0800, Paul E. McKenney wrote:
> On Mon, Jan 18, 2016 at 04:19:29PM +0800, Herbert Xu wrote:
> > Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > >
> > > You could use SYNC_ACQUIRE() to implement read_barrier_depends() and
> > > smp_read_barrier_depends(), but SYNC_RMB probably does not suffice.
> > > The reason for this is that smp_read_barrier_depends() must order the
> > > pointer load against any subsequent read or write through a dereference
> > > of that pointer. For example:
> > >
> > > p = READ_ONCE(gp);
> > > smp_rmb();
> > > r1 = p->a; /* ordered by smp_rmb(). */
> > > p->b = 42; /* NOT ordered by smp_rmb(), BUG!!! */
> > > r2 = x; /* ordered by smp_rmb(), but doesn't need to be. */
> > >
> > > In contrast:
> > >
> > > p = READ_ONCE(gp);
> > > smp_read_barrier_depends();
> > > r1 = p->a; /* ordered by smp_read_barrier_depends(). */
> > > p->b = 42; /* ordered by smp_read_barrier_depends(). */
> > > r2 = x; /* not ordered by smp_read_barrier_depends(), which is OK. */
> > >
> > > Again, if your hardware maintains local ordering for address
> > > and data dependencies, you can have read_barrier_depends() and
> > > smp_read_barrier_depends() be no-ops like they are for most
> > > architectures.
> > >
> > > Does that help?
> >
> > This is crazy! smp_rmb started out being strictly stronger than
> > smp_read_barrier_depends, when did this stop being the case?
>
> Hello, Herbert!
>
> It is true that most Linux kernel code relies only on the read-read
> properties of dependencies, but the read-write properties are useful.
> Admittedly relatively rarely, but useful.
>
> The better comparison for smp_read_barrier_depends(), especially in
> its rcu_dereference*() form, is smp_load_acquire().
>
Confused..
I recall that last time you and Linus came into a conclusion that even
on Alpha, a barrier for read->write with data dependency is unnecessary:
http://article.gmane.org/gmane.linux.kernel/2077661
And in an earlier mail of that thread, Linus made his point that
smp_read_barrier_depends() should only be used to order read->read.
So right now, are we going to extend the semantics of
smp_read_barrier_depends()? Can we just make smp_read_barrier_depends()
still only work for read->read, and assume all the architectures won't
reorder read->write with data dependency, so that the code above having
a smp_rmb() also works?
Regards,
Boqun
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Boqun Feng @ 2016-01-26 14:35 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 E. McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
Leonid Yegoshin, ddaney.cavm, Thomas
In-Reply-To: <20160126121608.GE21553@arm.com>
[-- Attachment #1.1: Type: text/plain, Size: 2198 bytes --]
Hi Will,
On Tue, Jan 26, 2016 at 12:16:09PM +0000, Will Deacon wrote:
> On Mon, Jan 25, 2016 at 10:03:22PM -0800, Paul E. McKenney wrote:
> > On Mon, Jan 25, 2016 at 04:42:43PM +0000, Will Deacon wrote:
> > > On Fri, Jan 15, 2016 at 01:58:53PM -0800, Paul E. McKenney wrote:
> > > > PPC Overlapping Group-B sets version 4
> > > > ""
> > > > (* When the Group-B sets from two different barriers involve instructions in
> > > > the same thread, within that thread one set must contain the other.
> > > >
> > > > P0 P1 P2
> > > > Rx=1 Wy=1 Wz=2
> > > > dep. lwsync lwsync
> > > > Ry=0 Wz=1 Wx=1
> > > > Rz=1
> > > >
> > > > assert(!(z=2))
> > > >
> > > > Forbidden by ppcmem, allowed by herd.
> > > > *)
> > > > {
> > > > 0:r1=x; 0:r2=y; 0:r3=z;
> > > > 1:r1=x; 1:r2=y; 1:r3=z; 1:r4=1;
> > > > 2:r1=x; 2:r2=y; 2:r3=z; 2:r4=1; 2:r5=2;
> > > > }
> > > > P0 | P1 | P2 ;
> > > > lwz r6,0(r1) | stw r4,0(r2) | stw r5,0(r3) ;
> > > > xor r7,r6,r6 | lwsync | lwsync ;
> > > > lwzx r7,r7,r2 | stw r4,0(r3) | stw r4,0(r1) ;
> > > > lwz r8,0(r3) | | ;
> > > >
> > > > exists
> > > > (z=2 /\ 0:r6=1 /\ 0:r7=0 /\ 0:r8=1)
> > >
> > > That really hurts. Assuming that the "assert(!(z=2))" is actually there
> > > to constrain the coherence order of z to be {0->1->2}, then I think that
> > > this test is forbidden on arm using dmb instead of lwsync. That said, I
> > > also don't think the Rz=1 in P0 changes anything.
> >
> > What about the smp_wmb() variant of dmb that orders only stores?
>
> Tricky, but I think it still works out if the coherence order of z is as
> I described above. The line of reasoning is weird though -- I ended up
> considering the two cases where P0 reads z before and after it reads x
^^^^^^^^^^^^^^^
Because of the fact that two reads on the same processors can't be
executed simultaneously? I feel like this is exactly something herd
missed.
> and what that means for the read of y.
>
And the reasoning on PPC is similar, so looks like the read of z on P0
is a necessary condition for the exists clause to be forbidden.
Regards,
Boqun
> Will
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Will Deacon @ 2016-01-26 12:16 UTC (permalink / raw)
To: Paul E. McKenney
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, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, Leonid Yegoshin,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160126060322.GJ4503@linux.vnet.ibm.com>
On Mon, Jan 25, 2016 at 10:03:22PM -0800, Paul E. McKenney wrote:
> On Mon, Jan 25, 2016 at 04:42:43PM +0000, Will Deacon wrote:
> > On Fri, Jan 15, 2016 at 01:58:53PM -0800, Paul E. McKenney wrote:
> > > PPC Overlapping Group-B sets version 4
> > > ""
> > > (* When the Group-B sets from two different barriers involve instructions in
> > > the same thread, within that thread one set must contain the other.
> > >
> > > P0 P1 P2
> > > Rx=1 Wy=1 Wz=2
> > > dep. lwsync lwsync
> > > Ry=0 Wz=1 Wx=1
> > > Rz=1
> > >
> > > assert(!(z=2))
> > >
> > > Forbidden by ppcmem, allowed by herd.
> > > *)
> > > {
> > > 0:r1=x; 0:r2=y; 0:r3=z;
> > > 1:r1=x; 1:r2=y; 1:r3=z; 1:r4=1;
> > > 2:r1=x; 2:r2=y; 2:r3=z; 2:r4=1; 2:r5=2;
> > > }
> > > P0 | P1 | P2 ;
> > > lwz r6,0(r1) | stw r4,0(r2) | stw r5,0(r3) ;
> > > xor r7,r6,r6 | lwsync | lwsync ;
> > > lwzx r7,r7,r2 | stw r4,0(r3) | stw r4,0(r1) ;
> > > lwz r8,0(r3) | | ;
> > >
> > > exists
> > > (z=2 /\ 0:r6=1 /\ 0:r7=0 /\ 0:r8=1)
> >
> > That really hurts. Assuming that the "assert(!(z=2))" is actually there
> > to constrain the coherence order of z to be {0->1->2}, then I think that
> > this test is forbidden on arm using dmb instead of lwsync. That said, I
> > also don't think the Rz=1 in P0 changes anything.
>
> What about the smp_wmb() variant of dmb that orders only stores?
Tricky, but I think it still works out if the coherence order of z is as
I described above. The line of reasoning is weird though -- I ended up
considering the two cases where P0 reads z before and after it reads x
and what that means for the read of y.
Will
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Will Deacon @ 2016-01-26 12:10 UTC (permalink / raw)
To: Paul E. McKenney
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, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, Leonid Yegoshin,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160126010646.GH4503@linux.vnet.ibm.com>
On Mon, Jan 25, 2016 at 05:06:46PM -0800, Paul E. McKenney wrote:
> On Mon, Jan 25, 2016 at 02:41:34PM +0000, Will Deacon wrote:
> > On Fri, Jan 15, 2016 at 11:28:45AM -0800, Paul E. McKenney wrote:
> > > On Fri, Jan 15, 2016 at 09:54:01AM -0800, Paul E. McKenney wrote:
> > > > On Fri, Jan 15, 2016 at 10:24:32AM +0000, Will Deacon wrote:
> > > > > See my earlier reply [1] (but also, your WRC Linux example looks more
> > > > > like a variant on WWC and I couldn't really follow it).
> > > >
> > > > I will revisit my WRC Linux example. And yes, creating litmus tests
> > > > that use non-fake dependencies is still a bit of an undertaking. :-/
> > > > I am sure that it will seem more natural with time and experience...
> > >
> > > Hmmm... You are quite right, I did do WWC. I need to change cpu2()'s
> > > last access from a store to a load to get WRC. Plus the levels of
> > > indirection definitely didn't match up, did they?
> >
> > Nope, it was pretty baffling!
>
> "It is a service that I provide." ;-)
>
> > > struct foo {
> > > struct foo *next;
> > > };
> > > struct foo a;
> > > struct foo b;
> > > struct foo c = { &a };
> > > struct foo d = { &b };
> > > struct foo x = { &c };
> > > struct foo y = { &d };
> > > struct foo *r1, *r2, *r3;
> > >
> > > void cpu0(void)
> > > {
> > > WRITE_ONCE(x.next, &y);
> > > }
> > >
> > > void cpu1(void)
> > > {
> > > r1 = lockless_dereference(x.next);
> > > WRITE_ONCE(r1->next, &x);
> > > }
> > >
> > > void cpu2(void)
> > > {
> > > r2 = lockless_dereference(y.next);
> > > r3 = READ_ONCE(r2->next);
> > > }
> > >
> > > In this case, it is legal to end the run with:
> > >
> > > r1 == &y && r2 == &x && r3 == &c
> > >
> > > Please see below for a ppcmem litmus test.
> > >
> > > So, did I get it right this time? ;-)
> >
> > The code above looks correct to me (in that it matches WRC+addrs),
> > but your litmus test:
> >
> > > PPC WRCnf+addrs
> > > ""
> > > {
> > > 0:r2=x; 0:r3=y;
> > > 1:r2=x; 1:r3=y;
> > > 2:r2=x; 2:r3=y;
> > > c=a; d=b; x=c; y=d;
> > > }
> > > P0 | P1 | P2 ;
> > > stw r3,0(r2) | lwz r8,0(r2) | lwz r8,0(r3) ;
> > > | stw r2,0(r3) | lwz r9,0(r8) ;
> > > exists
> > > (1:r8=y /\ 2:r8=x /\ 2:r9=c)
> >
> > Seems to be missing the address dependency on P1.
>
> You are quite correct! How about the following?
I think that's it!
> As before, both herd and ppcmem say that the cycle is allowed, as
> expected, given non-transitive ordering. To prohibit the cycle, P1
> needs a suitable memory-barrier instruction.
>
> ------------------------------------------------------------------------
>
> PPC WRCnf+addrs
> ""
> {
> 0:r2=x; 0:r3=y;
> 1:r2=x; 1:r3=y;
> 2:r2=x; 2:r3=y;
> c=a; d=b; x=c; y=d;
> }
> P0 | P1 | P2 ;
> stw r3,0(r2) | lwz r8,0(r2) | lwz r8,0(r3) ;
> | stw r2,0(r8) | lwz r9,0(r8) ;
> exists
> (1:r8=y /\ 2:r8=x /\ 2:r9=c)
Agreed.
Will
^ 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