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

Groups > linux.kernel > #1261973 > unrolled thread

Re: [GIT PULL] locking changes for v4.4

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: [GIT PULL] locking changes for v4.4 Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-04 01:00 +0100
    Re: [GIT PULL] locking changes for v4.4 Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-04 02:40 +0100
      Re: [GIT PULL] locking changes for v4.4 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-04 05:20 +0100
        Re: [GIT PULL] locking changes for v4.4 Dmitry Vyukov <dvyukov@google.com> - 2015-11-04 16:00 +0100
          Re: [GIT PULL] locking changes for v4.4 Dmitry Vyukov <dvyukov@google.com> - 2015-11-04 17:10 +0100
          Re: [GIT PULL] locking changes for v4.4 Peter Zijlstra <peterz@infradead.org> - 2015-11-04 17:10 +0100
      Re: [GIT PULL] locking changes for v4.4 Peter Zijlstra <peterz@infradead.org> - 2015-11-04 12:40 +0100
        Re: [GIT PULL] locking changes for v4.4 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-04 15:00 +0100
      Re: [GIT PULL] locking changes for v4.4 Ingo Molnar <mingo@kernel.org> - 2015-11-04 12:50 +0100
        Re: [GIT PULL] locking changes for v4.4 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-11-04 15:00 +0100
          Re: [GIT PULL] locking changes for v4.4 Ingo Molnar <mingo@kernel.org> - 2015-11-04 16:00 +0100

#1261973 — Re: [GIT PULL] locking changes for v4.4

On Tue, Nov 3, 2015 at 3:54 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Nov 3, 2015 at 1:16 AM, Ingo Molnar <mingo@kernel.org> wrote:
>>
>>   - More gradual enhancements to atomic ops: new atomic*_read_ctrl() ops,
>>     synchronize atomic_{read,set}() ordering requirements between architectures,
>>     add atomic_long_t bitops. (Peter Zijlstra)
>
> From another thread: those new "atomic*_read_ctrl()" operations are
> complete voodoo programming, and should never ever be used.

Sadly, that commit seems to be in the middle of the series.

I think I'll pull this, but then just make a separate commit to remove
all the bogus games with "control" dependencies that seem to have no
basis is reality.

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1262020

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

On Tue, Nov 3, 2015 at 3:58 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I think I'll pull this, but then just make a separate commit to remove
> all the bogus games with "control" dependencies that seem to have no
> basis is reality.

So the attached is what I committed in my tree. It took much longer to
try to write the rationale than it took to actually remove the
atomic_read_ctrl() functions, and even so I'm not sure how good that
commit message is. But at least it tries to explain what's going on.

Note the final part of the rationale:

    I may have to eat my words at some point, but in the absense of clear
    proof that alpha actually needs this, or indeed even an explanation of
    how alpha could _possibly_ need it, I do not believe these functions are
    called for.

    And if it turns out that alpha really _does_ need a barrier for this
    case, that barrier still should not be "smp_read_barrier_depends()".
    We'd have to make up some new speciality barrier just for alpha, along
    with the documentation for why it really is necessary.

so it's possible that we'll have to re-introduce these things, even if
I am obviously doubtful. However, if we do that, I think we need much
more explanations for why they would be necessary, and we'd want to
make another "smp_read_to_write_ctrl_barrier()" or something that
makes this particular ordering explicit. Because I don't think
"smp_read_barrier_depends()" is that barrier, even if it might have a
very similar implementation.

From everything I have seen in the alpha architecture manual, it is
really just read-to-dependent-read that is special (and in fact alpha
there is "consistent": it doesn't matter whether there's a data
dependency or a control dependency between the two reads). While
"read-to-dependent-write" actually seems to be documented to be
ordered, and act like other architectures (and again, it doesn't
matter whether it's a control or data dependency between the read and
the write).

That said, I sure did *not* enjoy reading that crazy memory ordering
documentation again. People who say that x86 memory ordering isn't
clearly defined have clearly not read the alpha manual. Gods, I hope I
will never have to try to read that ever again...

Added Dmitry Vyukov to the cc, since he seems to be one of the people
who wanted to use the atomic_read_ctrl() thing. So at least for now,
it's just something we assume is always valid for READ_ONCE() and for
atomic*_read(): doing a dependent write is just "ordered" with the
read it depends on (whether that's a control dependency or a data
one).

                          Linus

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

#1262065

On Tue, Nov 03, 2015 at 05:30:29PM -0800, Linus Torvalds wrote:
> On Tue, Nov 3, 2015 at 3:58 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > I think I'll pull this, but then just make a separate commit to remove
> > all the bogus games with "control" dependencies that seem to have no
> > basis is reality.
> 
> So the attached is what I committed in my tree. It took much longer to
> try to write the rationale than it took to actually remove the
> atomic_read_ctrl() functions, and even so I'm not sure how good that
> commit message is. But at least it tries to explain what's going on.
> 
> Note the final part of the rationale:
> 
>     I may have to eat my words at some point, but in the absense of clear
>     proof that alpha actually needs this, or indeed even an explanation of
>     how alpha could _possibly_ need it, I do not believe these functions are
>     called for.
> 
>     And if it turns out that alpha really _does_ need a barrier for this
>     case, that barrier still should not be "smp_read_barrier_depends()".
>     We'd have to make up some new speciality barrier just for alpha, along
>     with the documentation for why it really is necessary.

For whatever it is worth, the patch looks good to me.  The reasons I
could imagine why we might want to mark control dependencies are things
like documentation and tooling, but given that we currently only have a
very small number of them, it is hard to argue that this is of immediate
concern, if it is ever of concern.

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1262323

On Wed, Nov 4, 2015 at 5:16 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Tue, Nov 03, 2015 at 05:30:29PM -0800, Linus Torvalds wrote:
>> On Tue, Nov 3, 2015 at 3:58 PM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>> >
>> > I think I'll pull this, but then just make a separate commit to remove
>> > all the bogus games with "control" dependencies that seem to have no
>> > basis is reality.
>>
>> So the attached is what I committed in my tree. It took much longer to
>> try to write the rationale than it took to actually remove the
>> atomic_read_ctrl() functions, and even so I'm not sure how good that
>> commit message is. But at least it tries to explain what's going on.
>>
>> Note the final part of the rationale:
>>
>>     I may have to eat my words at some point, but in the absense of clear
>>     proof that alpha actually needs this, or indeed even an explanation of
>>     how alpha could _possibly_ need it, I do not believe these functions are
>>     called for.
>>
>>     And if it turns out that alpha really _does_ need a barrier for this
>>     case, that barrier still should not be "smp_read_barrier_depends()".
>>     We'd have to make up some new speciality barrier just for alpha, along
>>     with the documentation for why it really is necessary.
>
> For whatever it is worth, the patch looks good to me.  The reasons I
> could imagine why we might want to mark control dependencies are things
> like documentation and tooling, but given that we currently only have a
> very small number of them, it is hard to argue that this is of immediate
> concern, if it is ever of concern.


To clarify, yes, documentation and tooling was my main motivation.
It is usually helpful to see acquire/release, rmb/wmb pairs, and so it
is useful to know that something below is ordered wrt this load by
means of a control dependency (which effectively becomes an acquire,
and there must be a pairing release somewhere).
As for the tooling, as you may know, we are working on
KernelThreadSanitizer, which is dynamic happens-before-based race
detector (dynamically tracks acquire/release pairs to establish
synchronizes-with relation). As shown by memory_order_consume tracking
dependencies in compiler is practically impossible, and so the tool
needs some hint for these.
But we don't need to sort it out right now. And I agree we need a
clear idea as to why we are doing this. And we definitely must not
hide it under the alpha excuse. And our main headache for now is
"benign" data races anyway (missing READ/WRITE_ONCE).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1262400

On Wed, Nov 4, 2015 at 5:01 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Nov 04, 2015 at 03:51:01PM +0100, Dmitry Vyukov wrote:
>> To clarify, yes, documentation and tooling was my main motivation.
>
> Right; I don't object to having _ctrl() methods purely for documentation
> purposes, I keep finding places we rely on them. Having them stand out
> better might be useful.
>
>> It is usually helpful to see acquire/release, rmb/wmb pairs, and so it
>> is useful to know that something below is ordered wrt this load by
>> means of a control dependency (which effectively becomes an acquire,
>> and there must be a pairing release somewhere).
>
> You need at least a trailing smp_rmb() before you cover the ACQUIRE
> semantics -- or have no trailing reads at all of course.

Yes, I know, but is the best we can do. Episodic false negatives are
OK, while false positives are unacceptable. So we considered
READ_ONCE_CTRL as acquire.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1262402

On Wed, Nov 04, 2015 at 03:51:01PM +0100, Dmitry Vyukov wrote:
> To clarify, yes, documentation and tooling was my main motivation.

Right; I don't object to having _ctrl() methods purely for documentation
purposes, I keep finding places we rely on them. Having them stand out
better might be useful.

> It is usually helpful to see acquire/release, rmb/wmb pairs, and so it
> is useful to know that something below is ordered wrt this load by
> means of a control dependency (which effectively becomes an acquire,
> and there must be a pairing release somewhere).

You need at least a trailing smp_rmb() before you cover the ACQUIRE
semantics -- or have no trailing reads at all of course.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1262229

On Tue, Nov 03, 2015 at 05:30:29PM -0800, Linus Torvalds wrote:
> From 105ff3cbf225036b75a6a46c96d1ddce8e7bdc66 Mon Sep 17 00:00:00 2001
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Tue, 3 Nov 2015 17:22:17 -0800
> Subject: [PATCH] atomic: remove all traces of READ_ONCE_CTRL() and atomic*_read_ctrl()

> So I do not see how these "x_ctrl()" functions can currently be necessary.

> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Paul E McKenney <paulmck@us.ibm.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Hooray!

Now if only we could convince the C/C++ people that write speculation is
a bad idea :-)

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1262294

On Wed, Nov 04, 2015 at 12:37:01PM +0100, Peter Zijlstra wrote:
> On Tue, Nov 03, 2015 at 05:30:29PM -0800, Linus Torvalds wrote:
> > From 105ff3cbf225036b75a6a46c96d1ddce8e7bdc66 Mon Sep 17 00:00:00 2001
> > From: Linus Torvalds <torvalds@linux-foundation.org>
> > Date: Tue, 3 Nov 2015 17:22:17 -0800
> > Subject: [PATCH] atomic: remove all traces of READ_ONCE_CTRL() and atomic*_read_ctrl()
> 
> > So I do not see how these "x_ctrl()" functions can currently be necessary.
> 
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Paul E McKenney <paulmck@us.ibm.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> Hooray!
> 
> Now if only we could convince the C/C++ people that write speculation is
> a bad idea :-)

Sigh...  Working on it...

							Thanx, Paul

> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1262232

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, Nov 3, 2015 at 3:58 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > I think I'll pull this, but then just make a separate commit to remove

Thanks!

> > all the bogus games with "control" dependencies that seem to have no basis is 
> > reality.
> 
> So the attached is what I committed in my tree. [...]

Acked-by: Ingo Molnar <mingo@kernel.org>

> +      dependency into nonexistence.  Careful use of READ_ONCE() or
> +      atomic{,64}_read() can help to preserve your control dependency.
> +      Please see the Compiler Barrier section for more information.

So technically it's the "COMPILER BARRIER" section, but this is a pre-existing 
formulation and the document doesn't use such references consistently so I guess 
it doesn't matter much.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1262295

On Wed, Nov 04, 2015 at 12:48:50PM +0100, Ingo Molnar wrote:
> 
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Tue, Nov 3, 2015 at 3:58 PM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > I think I'll pull this, but then just make a separate commit to remove
> 
> Thanks!
> 
> > > all the bogus games with "control" dependencies that seem to have no basis is 
> > > reality.
> > 
> > So the attached is what I committed in my tree. [...]
> 
> Acked-by: Ingo Molnar <mingo@kernel.org>
> 
> > +      dependency into nonexistence.  Careful use of READ_ONCE() or
> > +      atomic{,64}_read() can help to preserve your control dependency.
> > +      Please see the Compiler Barrier section for more information.
> 
> So technically it's the "COMPILER BARRIER" section, but this is a pre-existing 
> formulation and the document doesn't use such references consistently so I guess 
> it doesn't matter much.

I will take a pass through and fix these up.  I don't see this as urgent,
so will add it to my patches for the next merge window.

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1262322

* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> On Wed, Nov 04, 2015 at 12:48:50PM +0100, Ingo Molnar wrote:
> > 
> > * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > 
> > > On Tue, Nov 3, 2015 at 3:58 PM, Linus Torvalds
> > > <torvalds@linux-foundation.org> wrote:
> > > >
> > > > I think I'll pull this, but then just make a separate commit to remove
> > 
> > Thanks!
> > 
> > > > all the bogus games with "control" dependencies that seem to have no basis is 
> > > > reality.
> > > 
> > > So the attached is what I committed in my tree. [...]
> > 
> > Acked-by: Ingo Molnar <mingo@kernel.org>
> > 
> > > +      dependency into nonexistence.  Careful use of READ_ONCE() or
> > > +      atomic{,64}_read() can help to preserve your control dependency.
> > > +      Please see the Compiler Barrier section for more information.
> > 
> > So technically it's the "COMPILER BARRIER" section, but this is a pre-existing 
> > formulation and the document doesn't use such references consistently so I guess 
> > it doesn't matter much.
> 
> I will take a pass through and fix these up.  I don't see this as urgent,
> so will add it to my patches for the next merge window.

Sounds good to me!

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web