Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320171 > unrolled thread

Re: [RFC][PATCH] mips: Fix arch_spin_unlock()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-28 01:40 +0100
    Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-01-28 11:00 +0100
      Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-28 23:40 +0100
        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-01-29 11:10 +0100
          Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-29 23:40 +0100
            Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-01 15:00 +0100
              Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-02 05:00 +0100
                Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Boqun Feng <boqun.feng@gmail.com> - 2016-02-02 06:20 +0100
                  Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-02 07:50 +0100
                    Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-02 09:10 +0100
                      Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-02 09:20 +0100
                        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Boqun Feng <boqun.feng@gmail.com> - 2016-02-02 10:40 +0100
                          Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-02 18:40 +0100
                            Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-02 19:00 +0100
                              Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-02 19:10 +0100
                                Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-02 20:40 +0100
                                  Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-02 21:00 +0100
                                    Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-03 20:20 +0100
                                  Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Ingo Molnar <mingo@kernel.org> - 2016-02-03 09:40 +0100
                                    Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-03 14:40 +0100
                                      Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-03 20:10 +0100
                        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-02 13:10 +0100
                          Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-02 19:00 +0100
                            Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-02 23:40 +0100
                        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Ralf Baechle <ralf@linux-mips.org> - 2016-02-02 15:50 +0100
                          Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Måns Rullgård <mans@mansr.com> - 2016-02-02 16:00 +0100
                            Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Ralf Baechle <ralf@linux-mips.org> - 2016-02-02 16:00 +0100
                              Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Måns Rullgård <mans@mansr.com> - 2016-02-02 17:00 +0100
                    Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Peter Zijlstra <peterz@infradead.org> - 2016-02-02 18:30 +0100
                      Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-02 23:40 +0100
                  Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-02 12:50 +0100
                    Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Boqun Feng <boqun.feng@gmail.com> - 2016-02-02 13:20 +0100
                      Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-02 13:30 +0100
                        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Boqun Feng <boqun.feng@gmail.com> - 2016-02-02 14:20 +0100
                        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-02 18:20 +0100
                          Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-02 18:40 +0100

Page 1 of 2  [1] 2  Next page →

#1320171 — Re: [RFC][PATCH] mips: Fix arch_spin_unlock()

On Wed, Jan 27, 2016 at 03:21:58PM +0000, Will Deacon wrote:
> On Wed, Jan 27, 2016 at 03:54:21PM +0100, Peter Zijlstra wrote:
> > On Wed, Jan 27, 2016 at 11:43:48AM +0000, Will Deacon wrote:
> > > Do you know whether a SYNC 18 (RELEASE) followed in program order by a
> > > SYNC 17 (ACQUIRE) creates a full barrier (i.e. something like SYNC 16)?
> > > 
> > > If not, you may need to implement smp_mb__after_unlock_lock for RCU
> > > to ensure globally transitive unlock->lock ordering should you decide
> > > to relax your locking barriers.
> > 
> > You know that is a tricky question. Maybe its easier if you give the 3
> > cpu litmus test that goes with it.
> 
> Sure, I was building up to that. I just wanted to make sure the basics
> were there (program-order, so same CPU) before we go any further. It
> sounds like they are, so that's promising.
> 
> > Maciej, the tricky point is what, if any, effect the
> > SYNC_RELEASE+SYNC_ACQUIRE pair has on an unrelated CPU. Please review
> > the TRANSITIVITY section in Documentation/memory-barriers.txt and
> > replace <general barrier> with the RELEASE+ACQUIRE pair.
> > 
> > We've all (Will, Paul and me) had much 'fun' trying to decipher the
> > MIPS64r6 manual but failed to reach a conclusion on this.
> 
> For the inter-thread case, Paul had a previous example along the lines
> of:
> 
> 
> Wx=1
> WyRel=1
> 
> RyAcq=1
> Rz=0
> 
> Wz=1
> smp_mb()
> Rx=0

Each paragraph being a separate thread, correct?  If so, agreed.

> and I suppose a variant of that:
> 
> 
> Wx=1
> WyRel=1
> 
> RyAcq=1
> Wz=1
> 
> Rz=1
> <address dependency>
> Rx=0

Agreed, this would be needed as well, along with the read-read and
read-write variants.  I picked the write-read version (Will's first
test above) because write-read reordering is the most likely on
hardware that I am aware of.

							Thanx, Paul

[toc] | [next] | [standalone]

#1320488

On Wed, Jan 27, 2016 at 03:38:36PM -0800, Paul E. McKenney wrote:
> On Wed, Jan 27, 2016 at 03:21:58PM +0000, Will Deacon wrote:
> > On Wed, Jan 27, 2016 at 03:54:21PM +0100, Peter Zijlstra wrote:
> > > On Wed, Jan 27, 2016 at 11:43:48AM +0000, Will Deacon wrote:
> > > > Do you know whether a SYNC 18 (RELEASE) followed in program order by a
> > > > SYNC 17 (ACQUIRE) creates a full barrier (i.e. something like SYNC 16)?
> > > > 
> > > > If not, you may need to implement smp_mb__after_unlock_lock for RCU
> > > > to ensure globally transitive unlock->lock ordering should you decide
> > > > to relax your locking barriers.
> > > 
> > > You know that is a tricky question. Maybe its easier if you give the 3
> > > cpu litmus test that goes with it.
> > 
> > Sure, I was building up to that. I just wanted to make sure the basics
> > were there (program-order, so same CPU) before we go any further. It
> > sounds like they are, so that's promising.
> > 
> > > Maciej, the tricky point is what, if any, effect the
> > > SYNC_RELEASE+SYNC_ACQUIRE pair has on an unrelated CPU. Please review
> > > the TRANSITIVITY section in Documentation/memory-barriers.txt and
> > > replace <general barrier> with the RELEASE+ACQUIRE pair.
> > > 
> > > We've all (Will, Paul and me) had much 'fun' trying to decipher the
> > > MIPS64r6 manual but failed to reach a conclusion on this.
> > 
> > For the inter-thread case, Paul had a previous example along the lines
> > of:
> > 
> > 
> > Wx=1
> > WyRel=1
> > 
> > RyAcq=1
> > Rz=0
> > 
> > Wz=1
> > smp_mb()
> > Rx=0
> 
> Each paragraph being a separate thread, correct?  If so, agreed.

Yes, sorry for the shorthand:

  - Each paragraph is a separate thread
  - Wx=1 means WRITE_ONCE(x, 1), Rx=1 means READ_ONCE(x) returns 1
  - WxRel means smp_store_release(x,1), RxAcq=1 means smp_load_acquire(x)
    returns 1
  - Everything is initially zero

> > and I suppose a variant of that:
> > 
> > 
> > Wx=1
> > WyRel=1
> > 
> > RyAcq=1
> > Wz=1
> > 
> > Rz=1
> > <address dependency>
> > Rx=0
> 
> Agreed, this would be needed as well, along with the read-read and
> read-write variants.  I picked the write-read version (Will's first
> test above) because write-read reordering is the most likely on
> hardware that I am aware of.

Question: if you replaced "Wz=1" with "WzRel=1" in my second test, would
it then be forbidden?

Will

[toc] | [prev] | [next] | [standalone]

#1321115

On Thu, Jan 28, 2016 at 09:57:19AM +0000, Will Deacon wrote:
> On Wed, Jan 27, 2016 at 03:38:36PM -0800, Paul E. McKenney wrote:
> > On Wed, Jan 27, 2016 at 03:21:58PM +0000, Will Deacon wrote:
> > > On Wed, Jan 27, 2016 at 03:54:21PM +0100, Peter Zijlstra wrote:
> > > > On Wed, Jan 27, 2016 at 11:43:48AM +0000, Will Deacon wrote:
> > > > > Do you know whether a SYNC 18 (RELEASE) followed in program order by a
> > > > > SYNC 17 (ACQUIRE) creates a full barrier (i.e. something like SYNC 16)?
> > > > > 
> > > > > If not, you may need to implement smp_mb__after_unlock_lock for RCU
> > > > > to ensure globally transitive unlock->lock ordering should you decide
> > > > > to relax your locking barriers.
> > > > 
> > > > You know that is a tricky question. Maybe its easier if you give the 3
> > > > cpu litmus test that goes with it.
> > > 
> > > Sure, I was building up to that. I just wanted to make sure the basics
> > > were there (program-order, so same CPU) before we go any further. It
> > > sounds like they are, so that's promising.
> > > 
> > > > Maciej, the tricky point is what, if any, effect the
> > > > SYNC_RELEASE+SYNC_ACQUIRE pair has on an unrelated CPU. Please review
> > > > the TRANSITIVITY section in Documentation/memory-barriers.txt and
> > > > replace <general barrier> with the RELEASE+ACQUIRE pair.
> > > > 
> > > > We've all (Will, Paul and me) had much 'fun' trying to decipher the
> > > > MIPS64r6 manual but failed to reach a conclusion on this.
> > > 
> > > For the inter-thread case, Paul had a previous example along the lines
> > > of:
> > > 
> > > 
> > > Wx=1
> > > WyRel=1
> > > 
> > > RyAcq=1
> > > Rz=0
> > > 
> > > Wz=1
> > > smp_mb()
> > > Rx=0
> > 
> > Each paragraph being a separate thread, correct?  If so, agreed.
> 
> Yes, sorry for the shorthand:
> 
>   - Each paragraph is a separate thread
>   - Wx=1 means WRITE_ONCE(x, 1), Rx=1 means READ_ONCE(x) returns 1
>   - WxRel means smp_store_release(x,1), RxAcq=1 means smp_load_acquire(x)
>     returns 1
>   - Everything is initially zero
> 
> > > and I suppose a variant of that:
> > > 
> > > 
> > > Wx=1
> > > WyRel=1
> > > 
> > > RyAcq=1
> > > Wz=1
> > > 
> > > Rz=1
> > > <address dependency>
> > > Rx=0
> > 
> > Agreed, this would be needed as well, along with the read-read and
> > read-write variants.  I picked the write-read version (Will's first
> > test above) because write-read reordering is the most likely on
> > hardware that I am aware of.
> 
> Question: if you replaced "Wz=1" with "WzRel=1" in my second test, would
> it then be forbidden?

On Power, yes.  I would guess on ARM as well.

For Linux in general, this is a question: How strict do we want to be
about matching the type of write with the corresponding read?  My
default approach is to initially be quite strict and loosen as needed.
Here "quite strict" might mean requiring an rcu_assign_pointer() for
the write and rcu_dereference() for the read, as opposed to (say)
ACCESS_ONCE() for the read.  (I am guessing that this would be too
tight, but it makes a good example.)

Thoughts?

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1321624

Hi Paul,

On Thu, Jan 28, 2016 at 02:31:31PM -0800, Paul E. McKenney wrote:
> On Thu, Jan 28, 2016 at 09:57:19AM +0000, Will Deacon wrote:
> > On Wed, Jan 27, 2016 at 03:38:36PM -0800, Paul E. McKenney wrote:
> > > On Wed, Jan 27, 2016 at 03:21:58PM +0000, Will Deacon wrote:
> > Yes, sorry for the shorthand:
> > 
> >   - Each paragraph is a separate thread
> >   - Wx=1 means WRITE_ONCE(x, 1), Rx=1 means READ_ONCE(x) returns 1
> >   - WxRel means smp_store_release(x,1), RxAcq=1 means smp_load_acquire(x)
> >     returns 1
> >   - Everything is initially zero
> > 
> > > > and I suppose a variant of that:
> > > > 
> > > > 
> > > > Wx=1
> > > > WyRel=1
> > > > 
> > > > RyAcq=1
> > > > Wz=1
> > > > 
> > > > Rz=1
> > > > <address dependency>
> > > > Rx=0
> > > 
> > > Agreed, this would be needed as well, along with the read-read and
> > > read-write variants.  I picked the write-read version (Will's first
> > > test above) because write-read reordering is the most likely on
> > > hardware that I am aware of.
> > 
> > Question: if you replaced "Wz=1" with "WzRel=1" in my second test, would
> > it then be forbidden?
> 
> On Power, yes.  I would guess on ARM as well.

Indeed.

> For Linux in general, this is a question: How strict do we want to be
> about matching the type of write with the corresponding read?  My
> default approach is to initially be quite strict and loosen as needed.
> Here "quite strict" might mean requiring an rcu_assign_pointer() for
> the write and rcu_dereference() for the read, as opposed to (say)
> ACCESS_ONCE() for the read.  (I am guessing that this would be too
> tight, but it makes a good example.)
> 
> Thoughts?

That sounds broadly sensible to me and allows rcu_assign_pointer and
rcu_dereference to be used as drop-in replacements for release/acquire
where local transitivity isn't required. However, I don't think we can
rule out READ_ONCE/WRITE_ONCE interactions as they seem to be used
already in things like the osq_lock (albeit without the address
dependency).

Will

[toc] | [prev] | [next] | [standalone]

#1322142

On Fri, Jan 29, 2016 at 09:59:59AM +0000, Will Deacon wrote:
> On Thu, Jan 28, 2016 at 02:31:31PM -0800, Paul E. McKenney wrote:

[ . . . ]

> > For Linux in general, this is a question: How strict do we want to be
> > about matching the type of write with the corresponding read?  My
> > default approach is to initially be quite strict and loosen as needed.
> > Here "quite strict" might mean requiring an rcu_assign_pointer() for
> > the write and rcu_dereference() for the read, as opposed to (say)
> > ACCESS_ONCE() for the read.  (I am guessing that this would be too
> > tight, but it makes a good example.)
> > 
> > Thoughts?
> 
> That sounds broadly sensible to me and allows rcu_assign_pointer and
> rcu_dereference to be used as drop-in replacements for release/acquire
> where local transitivity isn't required. However, I don't think we can
> rule out READ_ONCE/WRITE_ONCE interactions as they seem to be used
> already in things like the osq_lock (albeit without the address
> dependency).

Agreed.  So in the most strict case that I can imagine anyone putting
up with, we have the following pairings:

o	smp_store_release() -> smp_load_acquire() (locally transitive)

o	smp_store_release() -> lockless_dereference() (???)

o	smp_store_release() -> READ_ONCE(); if

o	rcu_assign_pointer() -> rcu_dereference()

o	smp_mb(); WRITE_ONCE() -> READ_ONCE(); (globally transitive)

o	synchronize_rcu(); WRITE_ONCE() -> READ_ONCE(); (globally transitive)

o	synchronize_rcu(); WRITE_ONCE() -> rcu_read_lock(); READ_ONCE()
		(strange and wonderful properties)

Seem reasonable, or am I missing some?

							Thanx, Paul

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

[toc] | [prev] | [next] | [standalone]

#1323140

On Fri, Jan 29, 2016 at 02:22:53AM -0800, Paul E. McKenney wrote:
> On Fri, Jan 29, 2016 at 09:59:59AM +0000, Will Deacon wrote:
> > On Thu, Jan 28, 2016 at 02:31:31PM -0800, Paul E. McKenney wrote:
> 
> [ . . . ]
> 
> > > For Linux in general, this is a question: How strict do we want to be
> > > about matching the type of write with the corresponding read?  My
> > > default approach is to initially be quite strict and loosen as needed.
> > > Here "quite strict" might mean requiring an rcu_assign_pointer() for
> > > the write and rcu_dereference() for the read, as opposed to (say)
> > > ACCESS_ONCE() for the read.  (I am guessing that this would be too
> > > tight, but it makes a good example.)
> > > 
> > > Thoughts?
> > 
> > That sounds broadly sensible to me and allows rcu_assign_pointer and
> > rcu_dereference to be used as drop-in replacements for release/acquire
> > where local transitivity isn't required. However, I don't think we can
> > rule out READ_ONCE/WRITE_ONCE interactions as they seem to be used
> > already in things like the osq_lock (albeit without the address
> > dependency).
> 
> Agreed.  So in the most strict case that I can imagine anyone putting
> up with, we have the following pairings:

I think we can group these up:

Locally transitive:

> o	smp_store_release() -> smp_load_acquire() (locally transitive)

Locally transitive chain termination:

(i.e. these can't be used to extend a chain)

> o	smp_store_release() -> lockless_dereference() (???)
> o	rcu_assign_pointer() -> rcu_dereference()
> o	smp_store_release() -> READ_ONCE(); if

Globally transitive:

> o	smp_mb(); WRITE_ONCE() -> READ_ONCE(); (globally transitive)
> o	synchronize_rcu(); WRITE_ONCE() -> READ_ONCE(); (globally transitive)

RCU:

> o	synchronize_rcu(); WRITE_ONCE() -> rcu_read_lock(); READ_ONCE()
> 		(strange and wonderful properties)
> 
> Seem reasonable, or am I missing some?

Looks alright to me.

Will

[toc] | [prev] | [next] | [standalone]

#1323777

On Mon, Feb 01, 2016 at 01:56:22PM +0000, Will Deacon wrote:
> On Fri, Jan 29, 2016 at 02:22:53AM -0800, Paul E. McKenney wrote:
> > On Fri, Jan 29, 2016 at 09:59:59AM +0000, Will Deacon wrote:
> > > On Thu, Jan 28, 2016 at 02:31:31PM -0800, Paul E. McKenney wrote:
> > 
> > [ . . . ]
> > 
> > > > For Linux in general, this is a question: How strict do we want to be
> > > > about matching the type of write with the corresponding read?  My
> > > > default approach is to initially be quite strict and loosen as needed.
> > > > Here "quite strict" might mean requiring an rcu_assign_pointer() for
> > > > the write and rcu_dereference() for the read, as opposed to (say)
> > > > ACCESS_ONCE() for the read.  (I am guessing that this would be too
> > > > tight, but it makes a good example.)
> > > > 
> > > > Thoughts?
> > > 
> > > That sounds broadly sensible to me and allows rcu_assign_pointer and
> > > rcu_dereference to be used as drop-in replacements for release/acquire
> > > where local transitivity isn't required. However, I don't think we can
> > > rule out READ_ONCE/WRITE_ONCE interactions as they seem to be used
> > > already in things like the osq_lock (albeit without the address
> > > dependency).
> > 
> > Agreed.  So in the most strict case that I can imagine anyone putting
> > up with, we have the following pairings:
> 
> I think we can group these up:
> 
> Locally transitive:
> 
> > o	smp_store_release() -> smp_load_acquire() (locally transitive)
> 
> Locally transitive chain termination:
> 
> (i.e. these can't be used to extend a chain)

Agreed.

> > o	smp_store_release() -> lockless_dereference() (???)
> > o	rcu_assign_pointer() -> rcu_dereference()
> > o	smp_store_release() -> READ_ONCE(); if

I am OK with the first and last, but I believe that the middle one
has real use cases.  So the rcu_assign_pointer() -> rcu_dereference()
case needs to be locally transitive.

> Globally transitive:
> 
> > o	smp_mb(); WRITE_ONCE() -> READ_ONCE(); (globally transitive)
> > o	synchronize_rcu(); WRITE_ONCE() -> READ_ONCE(); (globally transitive)
> 
> RCU:
> 
> > o	synchronize_rcu(); WRITE_ONCE() -> rcu_read_lock(); READ_ONCE()
> > 		(strange and wonderful properties)

Agreed.

> > Seem reasonable, or am I missing some?
> 
> Looks alright to me.

So I have some litmus tests to generate.  ;-)

							Thnax, Paul

[toc] | [prev] | [next] | [standalone]

#1323798

[Multipart message — attachments visible in raw view] — view raw

Hi Paul,

On Mon, Feb 01, 2016 at 07:54:58PM -0800, Paul E. McKenney wrote:
> On Mon, Feb 01, 2016 at 01:56:22PM +0000, Will Deacon wrote:
> > On Fri, Jan 29, 2016 at 02:22:53AM -0800, Paul E. McKenney wrote:
> > > On Fri, Jan 29, 2016 at 09:59:59AM +0000, Will Deacon wrote:
> > > > On Thu, Jan 28, 2016 at 02:31:31PM -0800, Paul E. McKenney wrote:
> > > 
> > > [ . . . ]
> > > 
> > > > > For Linux in general, this is a question: How strict do we want to be
> > > > > about matching the type of write with the corresponding read?  My
> > > > > default approach is to initially be quite strict and loosen as needed.
> > > > > Here "quite strict" might mean requiring an rcu_assign_pointer() for
> > > > > the write and rcu_dereference() for the read, as opposed to (say)
> > > > > ACCESS_ONCE() for the read.  (I am guessing that this would be too
> > > > > tight, but it makes a good example.)
> > > > > 
> > > > > Thoughts?
> > > > 
> > > > That sounds broadly sensible to me and allows rcu_assign_pointer and
> > > > rcu_dereference to be used as drop-in replacements for release/acquire
> > > > where local transitivity isn't required. However, I don't think we can
> > > > rule out READ_ONCE/WRITE_ONCE interactions as they seem to be used
> > > > already in things like the osq_lock (albeit without the address
> > > > dependency).
> > > 
> > > Agreed.  So in the most strict case that I can imagine anyone putting
> > > up with, we have the following pairings:
> > 
> > I think we can group these up:
> > 
> > Locally transitive:
> > 
> > > o	smp_store_release() -> smp_load_acquire() (locally transitive)
> > 
> > Locally transitive chain termination:
> > 
> > (i.e. these can't be used to extend a chain)
> 
> Agreed.
> 
> > > o	smp_store_release() -> lockless_dereference() (???)
> > > o	rcu_assign_pointer() -> rcu_dereference()
> > > o	smp_store_release() -> READ_ONCE(); if

Just want to make sure, this one is actually:

o	smp_store_release() -> READ_ONCE(); if ;<WRITE_ONCE()>

right? Because control dependency only orders READ->WRITE.

If so, do we also need to take the following pairing into consideration?

o	smp_store_release() -> READ_ONCE(); if ;smp_rmb(); <ACCESS_ONCE()>

> 
> I am OK with the first and last, but I believe that the middle one
> has real use cases.  So the rcu_assign_pointer() -> rcu_dereference()
> case needs to be locally transitive.
> 

Hmm... I don't think we should differ rcu_dereference() and
lockless_dereference(). One reason: list_for_each_entry_rcu() are using
lockless_dereference() right now, which means we used to think
rcu_dereference() and lockless_dereference() are interchangeable, right?

Besides, Will, what's the reason of having a locally transitive chain
termination? Because on some architectures RELEASE->DEPENDENCY pairs may
not be locally transitive?

Regards,
Boqun

> > Globally transitive:
> > 
> > > o	smp_mb(); WRITE_ONCE() -> READ_ONCE(); (globally transitive)
> > > o	synchronize_rcu(); WRITE_ONCE() -> READ_ONCE(); (globally transitive)
> > 
> > RCU:
> > 
> > > o	synchronize_rcu(); WRITE_ONCE() -> rcu_read_lock(); READ_ONCE()
> > > 		(strange and wonderful properties)
> 
> Agreed.
> 
> > > Seem reasonable, or am I missing some?
> > 
> > Looks alright to me.
> 
> So I have some litmus tests to generate.  ;-)
> 
> 							Thnax, Paul
> 

[toc] | [prev] | [next] | [standalone]

#1323835

On Tue, Feb 02, 2016 at 01:19:04PM +0800, Boqun Feng wrote:
> Hi Paul,
> 
> On Mon, Feb 01, 2016 at 07:54:58PM -0800, Paul E. McKenney wrote:
> > On Mon, Feb 01, 2016 at 01:56:22PM +0000, Will Deacon wrote:
> > > On Fri, Jan 29, 2016 at 02:22:53AM -0800, Paul E. McKenney wrote:
> > > > On Fri, Jan 29, 2016 at 09:59:59AM +0000, Will Deacon wrote:
> > > > > On Thu, Jan 28, 2016 at 02:31:31PM -0800, Paul E. McKenney wrote:
> > > > 
> > > > [ . . . ]
> > > > 
> > > > > > For Linux in general, this is a question: How strict do we want to be
> > > > > > about matching the type of write with the corresponding read?  My
> > > > > > default approach is to initially be quite strict and loosen as needed.
> > > > > > Here "quite strict" might mean requiring an rcu_assign_pointer() for
> > > > > > the write and rcu_dereference() for the read, as opposed to (say)
> > > > > > ACCESS_ONCE() for the read.  (I am guessing that this would be too
> > > > > > tight, but it makes a good example.)
> > > > > > 
> > > > > > Thoughts?
> > > > > 
> > > > > That sounds broadly sensible to me and allows rcu_assign_pointer and
> > > > > rcu_dereference to be used as drop-in replacements for release/acquire
> > > > > where local transitivity isn't required. However, I don't think we can
> > > > > rule out READ_ONCE/WRITE_ONCE interactions as they seem to be used
> > > > > already in things like the osq_lock (albeit without the address
> > > > > dependency).
> > > > 
> > > > Agreed.  So in the most strict case that I can imagine anyone putting
> > > > up with, we have the following pairings:
> > > 
> > > I think we can group these up:
> > > 
> > > Locally transitive:
> > > 
> > > > o	smp_store_release() -> smp_load_acquire() (locally transitive)
> > > 
> > > Locally transitive chain termination:
> > > 
> > > (i.e. these can't be used to extend a chain)
> > 
> > Agreed.
> > 
> > > > o	smp_store_release() -> lockless_dereference() (???)
> > > > o	rcu_assign_pointer() -> rcu_dereference()
> > > > o	smp_store_release() -> READ_ONCE(); if
> 
> Just want to make sure, this one is actually:
> 
> o	smp_store_release() -> READ_ONCE(); if ;<WRITE_ONCE()>
> 
> right? Because control dependency only orders READ->WRITE.

Yep!

> If so, do we also need to take the following pairing into consideration?
> 
> o	smp_store_release() -> READ_ONCE(); if ;smp_rmb(); <ACCESS_ONCE()>

It looks like we will be restricing smp_rmb() and smp_wmb() to pairwise
scenarios only.  So no transitivity in any scenarios involving these
two primitives.

> > I am OK with the first and last, but I believe that the middle one
> > has real use cases.  So the rcu_assign_pointer() -> rcu_dereference()
> > case needs to be locally transitive.
> 
> Hmm... I don't think we should differ rcu_dereference() and
> lockless_dereference(). One reason: list_for_each_entry_rcu() are using
> lockless_dereference() right now, which means we used to think
> rcu_dereference() and lockless_dereference() are interchangeable, right?

They use the same instructions, if that is what you mean, so intuitively
they should behave the same.  I don't feel all that strongly either way.
But where there is uncertainty, we should -not- assume ordering.  It is
easy to tighten the rules later, but hard to loosen them.

That might change if tools that automatically analyze usage patterns
in raw code, but we do not yet have such tools.

							Thanx, Paul

> Besides, Will, what's the reason of having a locally transitive chain
> termination? Because on some architectures RELEASE->DEPENDENCY pairs may
> not be locally transitive?
> 
> Regards,
> Boqun
> 
> > > Globally transitive:
> > > 
> > > > o	smp_mb(); WRITE_ONCE() -> READ_ONCE(); (globally transitive)
> > > > o	synchronize_rcu(); WRITE_ONCE() -> READ_ONCE(); (globally transitive)
> > > 
> > > RCU:
> > > 
> > > > o	synchronize_rcu(); WRITE_ONCE() -> rcu_read_lock(); READ_ONCE()
> > > > 		(strange and wonderful properties)
> > 
> > Agreed.
> > 
> > > > Seem reasonable, or am I missing some?
> > > 
> > > Looks alright to me.
> > 
> > So I have some litmus tests to generate.  ;-)
> > 
> > 							Thnax, Paul
> > 

[toc] | [prev] | [next] | [standalone]

#1323867

On Mon, Feb 1, 2016 at 10:44 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Tue, Feb 02, 2016 at 01:19:04PM +0800, Boqun Feng wrote:
>> Hi Paul,
>> If so, do we also need to take the following pairing into consideration?
>>
>> o     smp_store_release() -> READ_ONCE(); if ;smp_rmb(); <ACCESS_ONCE()>
>
> It looks like we will be restricing smp_rmb() and smp_wmb() to pairwise
> scenarios only.  So no transitivity in any scenarios involving these
> two primitives.

NO!

Stop this idiocy now, Paul.

We are not trying to make the kernel memory ordering rules pander to shit.

> But where there is uncertainty, we should -not- assume ordering.  It is
> easy to tighten the rules later, but hard to loosen them.

Bullshit.

The rules should absolutely be as tight as at all humanly possible,
given real hardware constraints.

It's also entirely wrong to say that "we can tighten the rules later".
No way. Because before those rules get tightened, we'll see confused
people who then try to "fix" code that doesn't want fixing, and adding
new odd constructs. We already had that with the whole bogus "control
dependency" shit. That crap came exactly from the fact that people
thought it was a good idea to make weak rules.

So you had better re-think the whole thing. I refuse to pander to the
idiotic and wrongheaded "weak memory ordering is good" braindamage.

Weak memory ordering is *not* good. It's shit.  It's hard to think
about, and people won't even realize that the assumptions they make
(unconsciously!) may be wrong.

So the memory ordering rules should be as strong as at all possible,
and should be as intuitive as possible, and follow peoples
expectations.

And quite frankly, if some crazy shit-for-brains architecture gets its
memory ordering wrong (like alpha did), then that crazy architecture
should pay the price.

There is absolutely _zero_ valid reason why smp_store_release should
not pair fine with a smp_rmb() on the other side. Can you actually
point to a relevant architecture that doesn't do that? I can't even
imagine how you'd make that pairing not work. If the releasing store
doesn't imply that it is ordered wrt previous stores, then it damn
well isn't a "release". And if the other side does a "smp-rmb()", then
the loads are ordered on the other side, so it had damn well just
work.

How could it not work?

So I can't even see how your "we won't guarantee" that wouldn't work.
It sounds insane.

But more importantly, your whole notion of "let's make things as weak
as possible" is *wrong*. That is now AT ALL what we should strive for.
We should strive for the strictest possible memory ordering that we
can get away with, and have as few "undefined" cases as at all
possible.

So we *absolutely* should say that *OF COURSE* these things work:

 - CPU A:

   .. initialize data structure -> smp_wmb() -> WRITE_ONCE(ptr);

 - CPU B:

   smp_load_acquire(ptr)  - we can rely on things behind "ptr" being initialized

and that the above mirror of that (ie smp_store_release paired with
READ_ONCE+smp_rmb) also works.

There are often reasons to prefer one model over the other, and
sometimes the reasons might argue for mixed access models.

For example, we might decide that the producer uses "smp_wmb()",
because that is universally fairly fast, and avoids a full memory
barrier on those architectures that don't really have "release"
semantics natively.  But at the same time the _consumer_ side might
want a "smp_load_acquire()" simply because it requires subsequent
loads and stores to be ordered if it sees that state (see for example
our "sk_state_store/load()" cases in networking - right now those pair
with release/acquire, but I suspect that "release" really could be a
"smp_wmb() + WRITE_ONCE()" instead, which could be faster on some
architectures)..

Because I really don't think there is any sane reason why that kind of
mixed access shouldn't work.

If those pairings don't work, there's something wrong with the
architecture, and the architecture will need to do whatever barriers
it needs to make it work in its store-release/load-acquire so that it
pairs with smp_wmb/rmb.

And if there against all sanity really is some crazy reason why that
pairing can be problematic, then that insane reason needs to be
extensively documented. Because that reason is so insane that it needs
a big honking description, so that people are aware of it.

But at no point should the logic be "let's leave it weakly defined".
Because at that point the documentation is actively _worse_ than not
having documentation at all, and just letting sanity prevail.

                     Linus

[toc] | [prev] | [next] | [standalone]

#1323874

On Tue, Feb 2, 2016 at 12:07 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So we *absolutely* should say that *OF COURSE* these things work:
>
>  - CPU A:
>
>    .. initialize data structure -> smp_wmb() -> WRITE_ONCE(ptr);
>
>  - CPU B:
>
>    smp_load_acquire(ptr)  - we can rely on things behind "ptr" being initialized

That's a bad example, btw. I shouldn't have made it be a "pointer",
because then we get the whole address dependency chain ordering
anyway.

So instead of "ptr", read "state flag". It might just be an "int" that
says "data has been initialized".

So

    .. initialize memory ..
    smp_wmb();
    WRITE_ONCE(&is_initialized, 1);

should pair with

    if (smp_load_acquire(&is_initialized))
        ... we can read and write the data, knowing it has been initialized ..

exactly because "smp_wmb()" (cheap write barrier) might be cheaper
than "smp_store_release()" (expensive full barrier) and thus
preferred.

So mixing ordering metaphors actually does make sense, and should be
entirely well-defined.

There's likely less reason to do it the other way (ie
"smp_store_release()" on one side pairing with "LOAD_ONCE() +
smp_rmb()" on the other) since there likely isn't the same kind of
performance reason for that pairing. But even if we would never
necessarily want to do it, I think our memory ordering rules would be
*much* better for strongly stating that it has to work, than being
timid and trying to make the rules weak.

Memory ordering is confusing enough as it is. We should not make
people worry more than they already have to. Strong rules are good.

                    Linus

[toc] | [prev] | [next] | [standalone]

#1323910

[Multipart message — attachments visible in raw view] — view raw

Hello Linus,

On Tue, Feb 02, 2016 at 12:19:04AM -0800, Linus Torvalds wrote:
> On Tue, Feb 2, 2016 at 12:07 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > So we *absolutely* should say that *OF COURSE* these things work:
> >
> >  - CPU A:
> >
> >    .. initialize data structure -> smp_wmb() -> WRITE_ONCE(ptr);
> >
> >  - CPU B:
> >
> >    smp_load_acquire(ptr)  - we can rely on things behind "ptr" being initialized
> 
> That's a bad example, btw. I shouldn't have made it be a "pointer",
> because then we get the whole address dependency chain ordering
> anyway.
> 
> So instead of "ptr", read "state flag". It might just be an "int" that
> says "data has been initialized".
> 
> So
> 
>     .. initialize memory ..
>     smp_wmb();
>     WRITE_ONCE(&is_initialized, 1);
> 
> should pair with
> 
>     if (smp_load_acquire(&is_initialized))
>         ... we can read and write the data, knowing it has been initialized ..
> 
> exactly because "smp_wmb()" (cheap write barrier) might be cheaper
> than "smp_store_release()" (expensive full barrier) and thus
> preferred.
> 

Just to be clear, what Will, Paul and I are discussing here is about
local transitivity, which refers to something like this following
example:

(a, b and is_initialized are all initially zero)

P0:

	WRITE_ONCE(a, 1);
	smp_store_release(&is_initialized, 1);

P1:
	r1 = smp_load_acquire(&is_initialized);
	smp_store_release(&b, 1);


P2:
	r2 = smp_load_acquire(&b);
	r3 = READ_ONCE(a);

, in which case, r1 == 1 && r2 == 1 && r3 == 0 can not happen because
RELEASE+ACQUIRE pairs guarantee local transitivity.

More on local transitvity:

http://article.gmane.org/gmane.linux.kernel.virtualization/26856


And what I'm asking here is something like this following example:

(a, b and is_initialized are all initially zero)

P0:

	WRITE_ONCE(a, 1);
	smp_store_release(&is_initialized, 1);

P1:
	if (r1 = READ_ONCE(is_initialized))
		smp_store_release(&b, 1);


P2:
	if (r2 = READ_ONCE(b)) {
		smp_rmb();	
		r3 = READ_ONCE(a);
	}

, in which case, can r1 == 1 && r2 == 1 && r3 == 0 happen?


Please note this example is about two questions on local transitivity:

1.	Could "READ_ONCE(); if" extend a locally transitive chain.

2.	Could "READ_ONCE(); if; smp_rmb()" at least be a locally
	transitive chain termination?

> So mixing ordering metaphors actually does make sense, and should be
> entirely well-defined.
> 

I think Paul does agree that smp_{r,w}mb() with applicative memory
operations around could pair with smp_store_release() or
smp_load_acquire().

Hope I didn't misunderstand any of you or make you misunderstood with
each other..

Regards,
Boqun

> There's likely less reason to do it the other way (ie
> "smp_store_release()" on one side pairing with "LOAD_ONCE() +
> smp_rmb()" on the other) since there likely isn't the same kind of
> performance reason for that pairing. But even if we would never
> necessarily want to do it, I think our memory ordering rules would be
> *much* better for strongly stating that it has to work, than being
> timid and trying to make the rules weak.
> 
> Memory ordering is confusing enough as it is. We should not make
> people worry more than they already have to. Strong rules are good.
> 
>                     Linus

[toc] | [prev] | [next] | [standalone]

#1324321

On Tue, Feb 2, 2016 at 1:34 AM, Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Just to be clear, what Will, Paul and I are discussing here is about
> local transitivity,

I really don't think that changes the picture.

Given that

 (a) we already mix ordering methods and there are good reasons for
it, and I'd expect transitivity only makes that more likely

 (b) we expect transitivity from the individual ordering methods

 (c) I don't think that there are any relevant CPU's that violate this anyway

I really think that not expecting that to hold for mixed accesses
would be a complete disaster. It will confuse the hell out of people.

And the basic argument really stands: we should make the memory
ordering expectations as strong as we can, given the existing relevant
architecture constraints (ie x86/arm/power).

If that then means that some other architecture might need to add
extra serialization that that architecture doesn't _want_ to add,
tough luck. I absolutely hate the fact that alpha forced us to add
that crazy read-depends barrier, and I want to discourage that a lot.

In fact, I'd be willing to strengthen our existing orderings just in
the name of sanity, and say that "rcu_dereference()" should just be an
acquire, and say that if the architecture makes that more expensive,
then who the hell cares? I have not been very happy with the "consume"
memory ordering discussions for C++. Yes, it would hurt pre-lwsync
power a bit, and it would hurt 32-bit arm, but enough that we should
have the headache of the existing semantics?

So I think our current memory orderings are potentially too _weak_,
and we sure as hell shouldn't strive to weaken them further.

I think doing "smp_read_barrier_depends()" was a mistake, but it was a
mistake driven by the fact that our memory ordering _used_ to be
barrier-centric. If alpha were to have happened today, I would say
that rather than have smp_read_barrier_depends(), we should just say
that anything that requires it should use smp_load_acquire() (or
rcu_dereference - which I think could have the same semantics), and be
done with it.

And if I would make that choice today, why isn't the right thing to
just get rid of that weak nasty thing, and convert the existing (few -
there really aren't that many) smp_read_barriers() away from that
model.

See what I'm saying? We've been pandering to weak memory ordering
before. We may have had our reasons to do so, but I think it's a
mistake. It results in code that is hard to think about.

So the fact that people worry about "rcu_dereference()" really makes
me think that one is just too weak, and we should just bite the bullet
and say it's an acquire. I'd much rather take a few barriers than make
our programming model hard to understand.

                    Linus

[toc] | [prev] | [next] | [standalone]

#1324340

On Tue, Feb 02, 2016 at 09:30:26AM -0800, Linus Torvalds wrote:
> On Tue, Feb 2, 2016 at 1:34 AM, Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Just to be clear, what Will, Paul and I are discussing here is about
> > local transitivity,
> 
> I really don't think that changes the picture.

For the general point about mixed methods, perhaps not, but it does
mean that we can't describe all of the issues using fewer than three
processors.

> Given that
> 
>  (a) we already mix ordering methods and there are good reasons for
> it, and I'd expect transitivity only makes that more likely
> 
>  (b) we expect transitivity from the individual ordering methods
> 
>  (c) I don't think that there are any relevant CPU's that violate this anyway
> 
> I really think that not expecting that to hold for mixed accesses
> would be a complete disaster. It will confuse the hell out of people.
> 
> And the basic argument really stands: we should make the memory
> ordering expectations as strong as we can, given the existing relevant
> architecture constraints (ie x86/arm/power).
> 
> If that then means that some other architecture might need to add
> extra serialization that that architecture doesn't _want_ to add,
> tough luck. I absolutely hate the fact that alpha forced us to add
> that crazy read-depends barrier, and I want to discourage that a lot.
> 
> In fact, I'd be willing to strengthen our existing orderings just in
> the name of sanity, and say that "rcu_dereference()" should just be an
> acquire, and say that if the architecture makes that more expensive,
> then who the hell cares? I have not been very happy with the "consume"
> memory ordering discussions for C++. Yes, it would hurt pre-lwsync
> power a bit, and it would hurt 32-bit arm, but enough that we should
> have the headache of the existing semantics?

Given that the vast majority of weakly ordered architectures respect
address dependencies, I would expect all of them to be hurt if they
were forced to use barrier instructions instead, even those where the
microarchitecture is fairly strongly ordered in practice.

Even load-acquire on ARMv8 has more work to do than a plain old address
dependency, so I'd be sad to see us upgrading rcu_dereference like this,
particularly when its a relatively uncontentious, easy to understand
part of the kernel memory model.

As far as I understand it, the problems with "consume" have centred
largely around compiler and specification issues, which we don't have
with rcu_dereference (i.e. we ignore thin-air and use volatile casts
/barrier() to keep the optimizer at bay).

Will

[toc] | [prev] | [next] | [standalone]

#1324348

On Tue, Feb 2, 2016 at 9:51 AM, Will Deacon <will.deacon@arm.com> wrote:
>
> Given that the vast majority of weakly ordered architectures respect
> address dependencies, I would expect all of them to be hurt if they
> were forced to use barrier instructions instead, even those where the
> microarchitecture is fairly strongly ordered in practice.

I do wonder if it would be all that noticeable, though. I don't think
we've really had benchmarks.

For example, most of the RCU list traversal shows up on x86 - where
loads are already acquires. But they show up not because of that, but
because a RCU list traversal is pretty much always going to take the
cache miss.

So it would actually be interesting to just try it - what happens to
kernel-centric benchmarks (which are already fairly rare) on arm if we
change the rcu_dereference() to be a smp_load_acquire()?

Because maybe nothing happens at all. I don't think we've ever tried it.

> As far as I understand it, the problems with "consume" have centred
> largely around compiler and specification issues, which we don't have
> with rcu_dereference (i.e. we ignore thin-air and use volatile casts
> /barrier() to keep the optimizer at bay).

Oh, I agree. The C++ consume orderings have been different from the
kernel worries.

But if it turns out that we have situations where we lose transitivity
because of rcu_dereference not being an acquire, then we have kernel
problems.

I do see that your later email said that the pointer dependency (which
we assume in rcu) should retain the transitivity, so maybe there is no
real reason to strengthen things.

But I _would_ argue that transitivity is so important (because of the
whole "individual orderings make sense and are causal, so the end
result must make sense and be causal") that if that were to break,
then such an architecture really should just strengthen the orderings.

Because the *worst* kinds of bugs are exactly the ones where the code
makes sense and works locally, but then the combination of two or
three pieces of code that are individually sensible ends up not
working for some crazy non-transitivity reason. That really is not
something that people can cope with.

And maybe we will one day have widely available automated ordering
proofs that work across the whole kernel, and we don't even need to
worry about "this breaks peoples minds", but I don't think we are
there yet.

                  Linus

[toc] | [prev] | [next] | [standalone]

#1324559

On Tue, Feb 02, 2016 at 10:06:36AM -0800, Linus Torvalds wrote:
> On Tue, Feb 2, 2016 at 9:51 AM, Will Deacon <will.deacon@arm.com> wrote:
> >
> > Given that the vast majority of weakly ordered architectures respect
> > address dependencies, I would expect all of them to be hurt if they
> > were forced to use barrier instructions instead, even those where the
> > microarchitecture is fairly strongly ordered in practice.
> 
> I do wonder if it would be all that noticeable, though. I don't think
> we've really had benchmarks.
> 
> For example, most of the RCU list traversal shows up on x86 - where
> loads are already acquires. But they show up not because of that, but
> because a RCU list traversal is pretty much always going to take the
> cache miss.
> 
> So it would actually be interesting to just try it - what happens to
> kernel-centric benchmarks (which are already fairly rare) on arm if we
> change the rcu_dereference() to be a smp_load_acquire()?
> 
> Because maybe nothing happens at all. I don't think we've ever tried it.

FWIW, and this is by no means conclusive, I hacked that up quickly and
ran hackbench a few times on the nearest idle arm64 system. The results
were consistently ~4% slower using acquire for rcu_dereference.

Will

[toc] | [prev] | [next] | [standalone]

#1324578

On Tue, Feb 2, 2016 at 11:30 AM, Will Deacon <will.deacon@arm.com> wrote:
>
> FWIW, and this is by no means conclusive, I hacked that up quickly and
> ran hackbench a few times on the nearest idle arm64 system. The results
> were consistently ~4% slower using acquire for rcu_dereference.

Ok, that's *much* more noticeable than I would have expected. I take
it that load-acquire is really really slow on current arm64
implementations.

That, btw, is one reason why I despise weak memory ordering. In most
cases I've ever seen, the hardware designers have said "barriers don't
much matter" and just made them crazy slow. My old alpha was the worst
of the worst.

It's shades of my least favorite x86 microarchitecture ever: netburst.
Make common cases fast, but make exceptional cases _so_ slow that it's
actually a noticeable pain.

Just out of interest, is store-release slow too? Because that should
be easy to make fast.

                Linus

[toc] | [prev] | [next] | [standalone]

#1325882

On Tue, Feb 02, 2016 at 11:55:57AM -0800, Linus Torvalds wrote:
> On Tue, Feb 2, 2016 at 11:30 AM, Will Deacon <will.deacon@arm.com> wrote:
> >
> > FWIW, and this is by no means conclusive, I hacked that up quickly and
> > ran hackbench a few times on the nearest idle arm64 system. The results
> > were consistently ~4% slower using acquire for rcu_dereference.
> 
> Ok, that's *much* more noticeable than I would have expected. I take
> it that load-acquire is really really slow on current arm64
> implementations.

See my reply to Ingo, but it seems a bunch of this was down to rebooting
the system between runs and hackbench being particularly susceptible to
that.

> Just out of interest, is store-release slow too? Because that should
> be easy to make fast.

There's a slight gotcha with arm64's store-release instruction in that
it's RCsc and therefore orders against a subsequent load-acquire. That's
not to say you can't make it fast, but it's potentially more involved
than posting a flag in a store buffer (or whatever you were envisaging :)

Measuring store-release is much more difficult, because you can't replace
it with a dependency or the like, only other barrier constructs.

Will

[toc] | [prev] | [next] | [standalone]

#1325044

* Will Deacon <will.deacon@arm.com> wrote:

> On Tue, Feb 02, 2016 at 10:06:36AM -0800, Linus Torvalds wrote:
> > On Tue, Feb 2, 2016 at 9:51 AM, Will Deacon <will.deacon@arm.com> wrote:
> > >
> > > Given that the vast majority of weakly ordered architectures respect
> > > address dependencies, I would expect all of them to be hurt if they
> > > were forced to use barrier instructions instead, even those where the
> > > microarchitecture is fairly strongly ordered in practice.
> > 
> > I do wonder if it would be all that noticeable, though. I don't think
> > we've really had benchmarks.
> > 
> > For example, most of the RCU list traversal shows up on x86 - where
> > loads are already acquires. But they show up not because of that, but
> > because a RCU list traversal is pretty much always going to take the
> > cache miss.
> > 
> > So it would actually be interesting to just try it - what happens to
> > kernel-centric benchmarks (which are already fairly rare) on arm if we
> > change the rcu_dereference() to be a smp_load_acquire()?
> > 
> > Because maybe nothing happens at all. I don't think we've ever tried it.
> 
> FWIW, and this is by no means conclusive, I hacked that up quickly and ran 
> hackbench a few times on the nearest idle arm64 system. The results were 
> consistently ~4% slower using acquire for rcu_dereference.

Could you please double check that? The thing is that hackbench is a _notoriously_ 
unstable workload and very dependent on various small details such as kernel image 
layout and random per-bootup cache/memory layouts details.

In fact I'd suggest to test this via a quick runtime hack like this in rcupdate.h:

	extern int panic_timeout;

	...

	if (panic_timeout)
		smp_load_acquire(p);
	else
		typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p);

(or so)

and then you can start a loop of hackbench runs, and in another terminal change 
the ordering primitive via:

   echo 1 > /proc/sys/kernel/panic		# smpload_acquire()
   echo 0 > /proc/sys/kernel/panic		# smp_read_barrier_depends()

without having to reboot the kernel.

Also, instead of using hackbench which has a too short runtime that makes it 
sensitive to scheduling micro-details, you could try the perf-bench hackbench 
work-alike where the number of loops is parametric:

 triton:~/tip> perf bench sched messaging -l 10000
 # Running 'sched/messaging' benchmark:
 # 20 sender and receiver processes per group
 # 10 groups == 400 processes run

     Total time: 4.532 [sec]

and you could get specific numbers of noise estimations via:

 triton:~/tip> perf stat --null --repeat 10 perf bench sched messaging -l 10000

 [...]

 Performance counter stats for 'perf bench sched messaging -l 10000' (10 runs):

       4.616404309 seconds time elapsed                                          ( +-  1.67% )

note that even with a repeat count of 10 runs and a loop count 100 times larger 
than the hackbench default, the intrinsic noise of this workload was still 1.6% - 
and that does not include boot-to-boot systematic noise.

It's very easy to get systemic noise with hackbench workloads and go down the 
entirely wrong road.

Of course, the numbers might also confirm your 4% figure!

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1325446

On Wed, Feb 03, 2016 at 09:33:39AM +0100, Ingo Molnar wrote:
> * Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Feb 02, 2016 at 10:06:36AM -0800, Linus Torvalds wrote:
> > > On Tue, Feb 2, 2016 at 9:51 AM, Will Deacon <will.deacon@arm.com> wrote:
> > > >
> > > > Given that the vast majority of weakly ordered architectures respect
> > > > address dependencies, I would expect all of them to be hurt if they
> > > > were forced to use barrier instructions instead, even those where the
> > > > microarchitecture is fairly strongly ordered in practice.
> > > 
> > > I do wonder if it would be all that noticeable, though. I don't think
> > > we've really had benchmarks.
> > > 
> > > For example, most of the RCU list traversal shows up on x86 - where
> > > loads are already acquires. But they show up not because of that, but
> > > because a RCU list traversal is pretty much always going to take the
> > > cache miss.
> > > 
> > > So it would actually be interesting to just try it - what happens to
> > > kernel-centric benchmarks (which are already fairly rare) on arm if we
> > > change the rcu_dereference() to be a smp_load_acquire()?
> > > 
> > > Because maybe nothing happens at all. I don't think we've ever tried it.
> > 
> > FWIW, and this is by no means conclusive, I hacked that up quickly and ran 
> > hackbench a few times on the nearest idle arm64 system. The results were 
> > consistently ~4% slower using acquire for rcu_dereference.
> 
> Could you please double check that? The thing is that hackbench is a _notoriously_ 
> unstable workload and very dependent on various small details such as kernel image 
> layout and random per-bootup cache/memory layouts details.

Yup. FWIW, I did try across a few reboots to arrive at the 4% figure, but
it's not conclusive (as I said :).

> In fact I'd suggest to test this via a quick runtime hack like this in rcupdate.h:
> 
> 	extern int panic_timeout;
> 
> 	...
> 
> 	if (panic_timeout)
> 		smp_load_acquire(p);
> 	else
> 		typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p);
> 
> (or so)

So the problem with this is that a LOAD <ctrl> LOAD sequence isn't an
ordering hazard on ARM, so you're potentially at the mercy of the branch
predictor as to whether you get an acquire. That's not to say it won't
be discarded as soon as the conditional is resolved, but it could
screw up the benchmarking.

I'd be better off doing some runtime patching, but that's not something
I can knock up in a couple of minutes (so I'll add it to my list).

> and you could get specific numbers of noise estimations via:
> 
>  triton:~/tip> perf stat --null --repeat 10 perf bench sched messaging -l 10000
> 
>  [...]
> 
>  Performance counter stats for 'perf bench sched messaging -l 10000' (10 runs):
> 
>        4.616404309 seconds time elapsed                                          ( +-  1.67% )
> 
> note that even with a repeat count of 10 runs and a loop count 100 times larger 
> than the hackbench default, the intrinsic noise of this workload was still 1.6% - 
> and that does not include boot-to-boot systematic noise.

That's helpful, thanks.

Will

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web