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 2 of 2 — ← Prev page 1 [2]

#1325871

On Wed, Feb 03, 2016 at 01:32:10PM +0000, Will Deacon wrote:
> On Wed, Feb 03, 2016 at 09:33:39AM +0100, Ingo Molnar wrote:
> > 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).

... so I actually got that up and running, believe it or not. Filthy stuff.

The good news is that you're right, and I'm now seeing ~1% difference
between the runs with ~0.3% noise for either of them. I still think
that's significant, but it's a lot more reassuring than 4%.

Will

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

#1324022

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.
> 
> So mixing ordering metaphors actually does make sense, and should be
> entirely well-defined.

I don't believe that anyone is arguing that this particular example
should not work the way that you want it to.

> 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.

The sorts of things I am really worried about are abominations like this
(and far worse):

	void thread0(void)
	{
		r1 = smp_load_acquire(&a);
		smp_store_release(&b, 1);
	}

	void thread1(void)
	{
		r2 = smp_load_acquire(&b);
		smp_store_release(&c, 1);
	}

	void thread2(void)
	{
		WRITE_ONCE(c, 2);
		smp_mb();
		r3 = READ_ONCE(d);
	}

	void thread3(void)
	{
		WRITE_ONCE(d, 1);
		smp_store_release(&a, 1);
	}

	r1 == 1 && r2 == 1 && c == 2 && r3 == 0 ???

I advise discouraging this sort of thing.  But it is your kernel, so
what is your preference?

							Thanx, Paul

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

#1324339

On Tue, Feb 2, 2016 at 4:02 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> The sorts of things I am really worried about are abominations like this
> (and far worse):

That one doesn't have any causal chain that I can see, so I agree that
it's an abomination, but it also doesn't act as an argument.

>  r1 == 1 && r2 == 1 && c == 2 && r3 == 0 ???

What do you see as the problem here? The above can happen in a
strictly ordered situation: thread2 runs first (c == 2, r3 = 0), then
thread3 runs (d = 1, a = 1) then thread0 runs (r1 = 1) and then
thread1 starts running but the store to c doesn't complete (now r2 =
1).

So there's no reason for your case to not happen, but the real issue
is that there is no causal relationship that your example describes,
so it's not even interesting.

Causality breaking is what really screws with peoples minds. The
reason transitivity is important (and why smp_read_barrier_depends()
is so annoying) is because causal breaks make peoples minds twist in
bad ways.

Sadly, memory orderings are very seldom described as honoring
causality, and instead people have the crazy litmus tests.

                 Linus

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

#1324696

On Tue, Feb 02, 2016 at 09:56:14AM -0800, Linus Torvalds wrote:
> On Tue, Feb 2, 2016 at 4:02 AM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > The sorts of things I am really worried about are abominations like this
> > (and far worse):
> 
> That one doesn't have any causal chain that I can see, so I agree that
> it's an abomination, but it also doesn't act as an argument.
> 
> >  r1 == 1 && r2 == 1 && c == 2 && r3 == 0 ???
> 
> What do you see as the problem here? The above can happen in a
> strictly ordered situation: thread2 runs first (c == 2, r3 = 0), then
> thread3 runs (d = 1, a = 1) then thread0 runs (r1 = 1) and then
> thread1 starts running but the store to c doesn't complete (now r2 =
> 1).

Apologies, I should have added that the condition does not get evaluated
until all the dust settles.  At that point both stores to c would have
completed, so that c == 1.

> So there's no reason for your case to not happen, but the real issue
> is that there is no causal relationship that your example describes,
> so it's not even interesting.

Because of the write-to-write relationship between thread1() and
thread2(), yes.  And I am very glad that you find this one uninteresting,
because including it would make things -really- complicated.

> Causality breaking is what really screws with peoples minds. The
> reason transitivity is important (and why smp_read_barrier_depends()
> is so annoying) is because causal breaks make peoples minds twist in
> bad ways.

Agreed, which means it is very important that the various flavors of
release-acquire chains be transitive.

In addition, smp_mb() is transitive, as are synchronize_rcu() and friends.

> Sadly, memory orderings are very seldom described as honoring
> causality, and instead people have the crazy litmus tests.

Indeed, a memory model defined solely by litmus tests would qualify as
an exotic form of torture.  What we do instead is use sets of litmus
tests as test cases for the prototype memory model under consideration.
It is all too easy to create a set of rules that look good and sound
good, but which mess something up.  The litmus tests help catch these
sorts of errors.

							Thanx, Paul

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

#1324140

On Tue, Feb 02, 2016 at 12:19:04AM -0800, Linus Torvalds wrote:

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

Confusing and the resulting bugs can be very hard to debug.

One of the problems I've experienced is that Linux does support liberal
memory ordering, even as extreme as the Alpha.  And every once in a
while a hardware guy is asking me if Linux their preferred variant of
weak ordering and answering honestly I have to say yes.  Even advising
against it in strong words against it, guess what will happen - another
weakly ordered core implementation.

To this point we've only fixed theoretical memory ordering issues on MIPS
but it's only a matter of time until we get bitten by a bug that does
actual damage.

SGI and a few others have managed to build large systems with significant
memory latencies yet managed to get decent performance with strong
ordering.

  Ralf

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

#1324172

Ralf Baechle <ralf@linux-mips.org> writes:

> On Tue, Feb 02, 2016 at 12:19:04AM -0800, Linus Torvalds wrote:
>
>> Memory ordering is confusing enough as it is. We should not make
>> people worry more than they already have to. Strong rules are good.
>
> Confusing and the resulting bugs can be very hard to debug.
>
> One of the problems I've experienced is that Linux does support liberal
> memory ordering, even as extreme as the Alpha.

We don't really have much choice given the reality of existing hardware.

-- 
Måns Rullgård

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

#1324174

On Tue, Feb 02, 2016 at 02:54:06PM +0000, Måns Rullgård wrote:

> We don't really have much choice given the reality of existing hardware.

No, of course not - but I want us discourage new weakly ordered
platforms as much as possible.

  Ralf

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

#1324218

Ralf Baechle <ralf@linux-mips.org> writes:

> On Tue, Feb 02, 2016 at 02:54:06PM +0000, Måns Rullgård wrote:
>
>> We don't really have much choice given the reality of existing hardware.
>
> No, of course not - but I want us discourage new weakly ordered
> platforms as much as possible.

Where do you draw the line though?  You can't exactly go and impose some
kind of global ordering in a multi-master system.

-- 
Måns Rullgård

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

#1324307

On 2 February 2016 07:44:33 CET, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

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


We rely on this with smp_cond_aquire() to extend the chain.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

#1324698

On Tue, Feb 02, 2016 at 06:23:37PM +0100, Peter Zijlstra wrote:
> 
> 
> On 2 February 2016 07:44:33 CET, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> >
> >> If so, do we also need to take the following pairing into
> >consideration?
> >> 
> >> o	smp_store_release() -> READ_ONCE(); if ;smp_rmb(); <ACCESS_ONCE()>
> 
> 
> We rely on this with smp_cond_aquire() to extend the chain.

We do indeed!  And it is not possible to use smp_load_acquire() given
this API becauseof the "!".  Hmmm...

If this one turns out to be problematic, there are some ways of dealing
with it.  But thank you for reminding me of it.  I think, anyway.  ;-)

							Thanx, Paul

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

#1324003

On Tue, Feb 02, 2016 at 01:19:04PM +0800, Boqun Feng wrote:
> 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:
> > > 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?

Well, the following ISA2 test is permitted on ARM:


P0:
Wx=1
WyRel=1	// rcu_assign_pointer

P1:
Ry=1	// rcu_dereference
WzRel=1 // rcu_assign_pointer

P2:
Rz=1	// rcu_dereference
<addr>
Rx=0


Make one of the rcu_dereferences an ACQUIRE and the behaviour is
forbidden.

Paul: what use-cases did you have in mind?

Will

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

#1324029

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

On Tue, Feb 02, 2016 at 11:45:59AM +0000, Will Deacon wrote:
> On Tue, Feb 02, 2016 at 01:19:04PM +0800, Boqun Feng wrote:
> > 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:
> > > > 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?
> 
> Well, the following ISA2 test is permitted on ARM:
> 
> 
> P0:
> Wx=1
> WyRel=1	// rcu_assign_pointer
> 
> P1:
> Ry=1	// rcu_dereference

What if a <addr> dependency is added here? Same result?

> WzRel=1 // rcu_assign_pointer
> 
> P2:
> Rz=1	// rcu_dereference
> <addr>
> Rx=0
> 
> 
> Make one of the rcu_dereferences an ACQUIRE and the behaviour is
> forbidden.
> 
> Paul: what use-cases did you have in mind?
> 
> Will

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

#1324034

On Tue, Feb 02, 2016 at 08:12:30PM +0800, Boqun Feng wrote:
> On Tue, Feb 02, 2016 at 11:45:59AM +0000, Will Deacon wrote:
> > On Tue, Feb 02, 2016 at 01:19:04PM +0800, Boqun Feng wrote:
> > > 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:
> > > > > 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?
> > 
> > Well, the following ISA2 test is permitted on ARM:
> > 
> > 
> > P0:
> > Wx=1
> > WyRel=1	// rcu_assign_pointer
> > 
> > P1:
> > Ry=1	// rcu_dereference
> 
> What if a <addr> dependency is added here? Same result?

Right, that fixes it. So if we're only considering things like:

  rcu_dereference
  <addr>
  RELEASE

then local transitivity should be preserved.

I think the same applies to <ctrl>, which seems to match your later
example.

Will

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

#1324070

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

On Tue, Feb 02, 2016 at 12:20:25PM +0000, Will Deacon wrote:
[...]
> > > > 
> > > > 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?
> > > 
> > > Well, the following ISA2 test is permitted on ARM:
> > > 
> > > 
> > > P0:
> > > Wx=1
> > > WyRel=1	// rcu_assign_pointer
> > > 
> > > P1:
> > > Ry=1	// rcu_dereference
> > 
> > What if a <addr> dependency is added here? Same result?
> 
> Right, that fixes it. So if we're only considering things like:
> 
>   rcu_dereference
>   <addr>
>   RELEASE
> 
> then local transitivity should be preserved.
> 
> I think the same applies to <ctrl>, which seems to match your later
> example.
> 

Thank you ;-)

Now I understand why you want thoses pairings to be locally transitive
chain terminations, they have more subtle requirements to extend locally
transitive chains and slight different behaviors on different
architectures. It's better for us to put them aside until we figure out
thoses subtle requirements and different behaviors.

Regards,
Boqun

> Will

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

#1324302

On Tue, Feb 02, 2016 at 12:20:25PM +0000, Will Deacon wrote:
> On Tue, Feb 02, 2016 at 08:12:30PM +0800, Boqun Feng wrote:
> > On Tue, Feb 02, 2016 at 11:45:59AM +0000, Will Deacon wrote:
> > > On Tue, Feb 02, 2016 at 01:19:04PM +0800, Boqun Feng wrote:
> > > > 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:
> > > > > > 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?
> > > 
> > > Well, the following ISA2 test is permitted on ARM:
> > > 
> > > 
> > > P0:
> > > Wx=1
> > > WyRel=1	// rcu_assign_pointer
> > > 
> > > P1:
> > > Ry=1	// rcu_dereference
> > 
> > What if a <addr> dependency is added here? Same result?
> 
> Right, that fixes it. So if we're only considering things like:
> 
>   rcu_dereference
>   <addr>
>   RELEASE
> 
> then local transitivity should be preserved.

Whew!!!  ;-)

> I think the same applies to <ctrl>, which seems to match your later
> example.

Could you please check?

							Thanx, Paul

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

#1324326

On Tue, Feb 02, 2016 at 09:12:37AM -0800, Paul E. McKenney wrote:
> On Tue, Feb 02, 2016 at 12:20:25PM +0000, Will Deacon wrote:
> > On Tue, Feb 02, 2016 at 08:12:30PM +0800, Boqun Feng wrote:
> > > On Tue, Feb 02, 2016 at 11:45:59AM +0000, Will Deacon wrote:
> > > > Well, the following ISA2 test is permitted on ARM:
> > > > 
> > > > 
> > > > P0:
> > > > Wx=1
> > > > WyRel=1	// rcu_assign_pointer
> > > > 
> > > > P1:
> > > > Ry=1	// rcu_dereference
> > > 
> > > What if a <addr> dependency is added here? Same result?
> > 
> > Right, that fixes it. So if we're only considering things like:
> > 
> >   rcu_dereference
> >   <addr>
> >   RELEASE
> > 
> > then local transitivity should be preserved.
> 
> Whew!!!  ;-)
> 
> > I think the same applies to <ctrl>, which seems to match your later
> > example.
> 
> Could you please check?

I should've been more concrete here: it does indeed apply if you replace
the <addr> with a <ctrl> in the snippet above, I just hadn't got Boqun's
example paged in and didn't want to commit on the examples being identical.

Will

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web