Re: [GIT PULL] locking fix

[pr-tracker-bot]

The pull request you sent on Sun, 28 Mar 2021 12:28:43 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 
> locking-urgent-2021-03-28

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/47fbbc94dab61a1385f21a0a209c61b5d6b0a215

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] locking fix

[pr-tracker-bot]

The pull request you sent on Sun, 14 Jul 2019 13:36:21 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 
> locking-urgent-for-linus

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/0c85ce135456a3927f552e738f730c47ac905ac3

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

Re: [GIT PULL] locking fix

[Greg KH]

On Thu, May 16, 2019 at 07:55:53PM -0400, Sasha Levin wrote:
> On Thu, May 16, 2019 at 11:42:58AM -0700, Linus Torvalds wrote:
> > On Thu, May 16, 2019 at 11:39 AM Greg KH  wrote:
> > > 
> > > Thanks, I'll work on that later tonight...
> > 
> > Note that it probably is almost entirely impossible to trigger the
> > problem in practice, so it's not like this is a particularly important
> > stable back-port.
> > 
> > I just happened to look at it and go "hmm, it's not _marked_ for stable".
> 
> I've addressed a missing a8654596f03 ("locking/rwsem: Enable lock event
> counting") and queued up a backport for 5.1-4.9.

Thanks for doing this.

greg k-h

Re: [GIT PULL] locking fix

[Sasha Levin]

On Thu, May 16, 2019 at 11:42:58AM -0700, Linus Torvalds wrote:

On Thu, May 16, 2019 at 11:39 AM Greg KH  wrote:


Thanks, I'll work on that later tonight...


Note that it probably is almost entirely impossible to trigger the
problem in practice, so it's not like this is a particularly important
stable back-port.

I just happened to look at it and go "hmm, it's not _marked_ for stable".


I've addressed a missing a8654596f03 ("locking/rwsem: Enable lock event
counting") and queued up a backport for 5.1-4.9.

--
Thanks,
Sasha

Re: [GIT PULL] locking fix

[Linus Torvalds]

On Thu, May 16, 2019 at 11:39 AM Greg KH  wrote:
>
> Thanks, I'll work on that later tonight...

Note that it probably is almost entirely impossible to trigger the
problem in practice, so it's not like this is a particularly important
stable back-port.

I just happened to look at it and go "hmm, it's not _marked_ for stable".

  Linus

Re: [GIT PULL] locking fix

[Greg KH]

On Thu, May 16, 2019 at 10:57:53AM -0700, Linus Torvalds wrote:
> On Thu, May 16, 2019 at 9:01 AM Ingo Molnar  wrote:
> >
> > A single rwsem fix.
> 
> Side note, this one isn't marked for stable, but I'm hoping stable
> picks it up just from the "Fixes" tag.
> 
> Stable people, we're talking about
> 
> a9e9bcb45b15 ("locking/rwsem: Prevent decrement of reader count
> before increment")
> 
> that I just pulled into my tree, and needs to go in 4.9 and later.

Thanks, I'll work on that later tonight...


greg k-h

Re: [GIT PULL] locking fix

[pr-tracker-bot]

The pull request you sent on Thu, 16 May 2019 18:01:35 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 
> locking-urgent-for-linus

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/f57d7715d7645b7c3d1e7b7cb79ac7690fe2d260

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

Re: [GIT PULL] locking fix

[Linus Torvalds]

On Thu, May 16, 2019 at 9:01 AM Ingo Molnar  wrote:
>
> A single rwsem fix.

Side note, this one isn't marked for stable, but I'm hoping stable
picks it up just from the "Fixes" tag.

Stable people, we're talking about

a9e9bcb45b15 ("locking/rwsem: Prevent decrement of reader count
before increment")

that I just pulled into my tree, and needs to go in 4.9 and later.

  Linus

Re: [GIT PULL] locking fix

[pr-tracker-bot]

The pull request you sent on Fri, 12 Apr 2019 13:53:43 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 
> locking-urgent-for-linus

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/26e2b81977bb1ad70ff9b2acb4d4cb13c23facfd

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

Re: [GIT pull] locking fix for 4.9

[Steven Rostedt]

On Wed, 12 Oct 2016 10:22:59 -0700
Linus Torvalds  wrote:

> On Wed, Oct 12, 2016 at 1:28 AM, Steven Rostedt  wrote:
> >
> > But I agree. We have lived a long time without the need for this
> > warning. I'm not strongly advocating keeping the warning around and
> > just disabling it totally. But it all comes down to how much we
> > trust those that inherit this after we are gone ;-)  
> 
> I'll take that as an ack for just moving it back to being globally
> disabled, and will do a commit doing that.
> 

Fine with me:

Acked-by: Steven Rostedt 

-- Steve

Re: [GIT pull] locking fix for 4.9

[Steven Rostedt]

On Wed, 12 Oct 2016 10:22:59 -0700
Linus Torvalds  wrote:

> On Wed, Oct 12, 2016 at 1:28 AM, Steven Rostedt  wrote:
> >
> > But I agree. We have lived a long time without the need for this
> > warning. I'm not strongly advocating keeping the warning around and
> > just disabling it totally. But it all comes down to how much we
> > trust those that inherit this after we are gone ;-)  
> 
> I'll take that as an ack for just moving it back to being globally
> disabled, and will do a commit doing that.
> 

Fine with me:

Acked-by: Steven Rostedt 

-- Steve

Re: [GIT pull] locking fix for 4.9

[Linus Torvalds]

On Wed, Oct 12, 2016 at 1:28 AM, Steven Rostedt  wrote:
>
> But I agree. We have lived a long time without the need for this
> warning. I'm not strongly advocating keeping the warning around and
> just disabling it totally. But it all comes down to how much we
> trust those that inherit this after we are gone ;-)

I'll take that as an ack for just moving it back to being globally
disabled, and will do a commit doing that.

  Linus

Re: [GIT pull] locking fix for 4.9

[Linus Torvalds]

On Wed, Oct 12, 2016 at 1:28 AM, Steven Rostedt  wrote:
>
> But I agree. We have lived a long time without the need for this
> warning. I'm not strongly advocating keeping the warning around and
> just disabling it totally. But it all comes down to how much we
> trust those that inherit this after we are gone ;-)

I'll take that as an ack for just moving it back to being globally
disabled, and will do a commit doing that.

  Linus

Re: [GIT pull] locking fix for 4.9

[Steven Rostedt]

On Mon, 10 Oct 2016 10:29:27 -0700
Linus Torvalds  wrote:

> On Sat, Oct 8, 2016 at 5:47 AM, Thomas Gleixner  wrote:
> >
> > A single fix which prevents newer GCCs from spamming the build output with
> > overly eager warnings about __builtin_return_address() uses which are
> > correct.  
> 
> Ugh. This feels over-engineered to me.
> 
> We already disable that warning unconditionally for the trace
> subdirectory, and for mm/usercopy.c.
> 
> I feel that the simpler solution is to just disable the warning
> globally, and not worry about "when this config option is enabled we
> need to disable it".
> 
> Basically, we disable the warning every time we ever use
> __builtin_return_address(), so maybe we should just disable it once
> and for all.

The only advantage of doing this is to make it a pain to use
__builtin_return_address(n) with n > 0, so that we don't accidentally
use it without knowing what we are doing.

> 
> It's not like the __builtin_return_address() warning is so incredibly
> useful anyway.
> 

But I agree. We have lived a long time without the need for this
warning. I'm not strongly advocating keeping the warning around and
just disabling it totally. But it all comes down to how much we
trust those that inherit this after we are gone ;-)

/me is feeling his age.

-- Steve

Re: [GIT pull] locking fix for 4.9

[Steven Rostedt]

On Mon, 10 Oct 2016 10:29:27 -0700
Linus Torvalds  wrote:

> On Sat, Oct 8, 2016 at 5:47 AM, Thomas Gleixner  wrote:
> >
> > A single fix which prevents newer GCCs from spamming the build output with
> > overly eager warnings about __builtin_return_address() uses which are
> > correct.  
> 
> Ugh. This feels over-engineered to me.
> 
> We already disable that warning unconditionally for the trace
> subdirectory, and for mm/usercopy.c.
> 
> I feel that the simpler solution is to just disable the warning
> globally, and not worry about "when this config option is enabled we
> need to disable it".
> 
> Basically, we disable the warning every time we ever use
> __builtin_return_address(), so maybe we should just disable it once
> and for all.

The only advantage of doing this is to make it a pain to use
__builtin_return_address(n) with n > 0, so that we don't accidentally
use it without knowing what we are doing.

> 
> It's not like the __builtin_return_address() warning is so incredibly
> useful anyway.
> 

But I agree. We have lived a long time without the need for this
warning. I'm not strongly advocating keeping the warning around and
just disabling it totally. But it all comes down to how much we
trust those that inherit this after we are gone ;-)

/me is feeling his age.

-- Steve

Re: [GIT pull] locking fix for 4.9

[Linus Torvalds]

On Sat, Oct 8, 2016 at 5:47 AM, Thomas Gleixner  wrote:
>
> A single fix which prevents newer GCCs from spamming the build output with
> overly eager warnings about __builtin_return_address() uses which are
> correct.

Ugh. This feels over-engineered to me.

We already disable that warning unconditionally for the trace
subdirectory, and for mm/usercopy.c.

I feel that the simpler solution is to just disable the warning
globally, and not worry about "when this config option is enabled we
need to disable it".

Basically, we disable the warning every time we ever use
__builtin_return_address(), so maybe we should just disable it once
and for all.

It's not like the __builtin_return_address() warning is so incredibly
useful anyway.

Hmm?

Linus

Re: [GIT pull] locking fix for 4.9

[Linus Torvalds]

On Sat, Oct 8, 2016 at 5:47 AM, Thomas Gleixner  wrote:
>
> A single fix which prevents newer GCCs from spamming the build output with
> overly eager warnings about __builtin_return_address() uses which are
> correct.

Ugh. This feels over-engineered to me.

We already disable that warning unconditionally for the trace
subdirectory, and for mm/usercopy.c.

I feel that the simpler solution is to just disable the warning
globally, and not worry about "when this config option is enabled we
need to disable it".

Basically, we disable the warning every time we ever use
__builtin_return_address(), so maybe we should just disable it once
and for all.

It's not like the __builtin_return_address() warning is so incredibly
useful anyway.

Hmm?

Linus

Re: [GIT PULL] locking fix

[Ingo Molnar]

* Linus Torvalds  wrote:

> On Sun, Oct 27, 2013 at 12:56 PM, Maarten Lankhorst
>  wrote:
> >
> > And this is why ww_ctx == NULL is now passed as an inline 
> > argument. :)
> 
> Well, more than that - the "optimization" has been done at the 
> source code level, so that the behavior is no longer a matter 
> about how well the compiler optimizes it any more.
> 
> I'm not complaining about the fix. I'm complaining about how the 
> fix was claimed to be due to a compiler bug. The "documentation" 
> for the fix (ie the commit message) was actively misleading.

Agreed, there was quite a bit of back and forth and I genuinely got 
confused and thought it's purely about a compiler bug (hence the 
misleading pull request) - will watch out for that pattern better 
next time around.

Thanks,

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

Re: [GIT PULL] locking fix

[Ingo Molnar]

* Linus Torvalds torva...@linux-foundation.org wrote:

 On Sun, Oct 27, 2013 at 12:56 PM, Maarten Lankhorst
 maarten.lankho...@canonical.com wrote:
 
  And this is why ww_ctx == NULL is now passed as an inline 
  argument. :)
 
 Well, more than that - the optimization has been done at the 
 source code level, so that the behavior is no longer a matter 
 about how well the compiler optimizes it any more.
 
 I'm not complaining about the fix. I'm complaining about how the 
 fix was claimed to be due to a compiler bug. The documentation 
 for the fix (ie the commit message) was actively misleading.

Agreed, there was quite a bit of back and forth and I genuinely got 
confused and thought it's purely about a compiler bug (hence the 
misleading pull request) - will watch out for that pattern better 
next time around.

Thanks,

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

Re: [GIT PULL] locking fix

[Linus Torvalds]

On Sun, Oct 27, 2013 at 12:56 PM, Maarten Lankhorst
 wrote:
>
> And this is why ww_ctx == NULL is now passed as an inline argument. :)

Well, more than that - the "optimization" has been done at the source
code level, so that the behavior is no longer a matter about how well
the compiler optimizes it any more.

I'm not complaining about the fix. I'm complaining about how the fix
was claimed to be due to a compiler bug. The "documentation" for the
fix (ie the commit message) was actively misleading.

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

Re: [GIT PULL] locking fix

[Maarten Lankhorst]

op 27-10-13 20:51, Linus Torvalds schreef:
> On Sun, Oct 27, 2013 at 12:37 PM, Maarten Lankhorst
>  wrote:
>> I would love for a compiler to become that smart though, but I do not think 
>> it's likely.
> Dammit, even if that is true, then write the conditional *correctly*.
>
> As mentioned, the conditional
>
> __builtin_constant_p(ww_ctx) && ww_ctx == NULL
>
> is actually sensible, in a way the original one was *not*. It actually
> tests what you apparently intended to test, and is more readable to
> humans to boot.
Yeah that mail arrived after I sent mine, I agree that this would have been 
more sensible.
> And no, it still isn't actually guaranteed to do what you want it to
> do. Historically, in gcc, __builtin_constant_p() really only ever
> worked in macros, because by the time you use it in inline functions,
> a constant NULL in the caller will have been turned into a argument
> variable in the inline function, and __builtin_constant_p() would be
> done before that was optimized away. Over the years, gcc has pushed
> some of the builtin evaluation deeper down, and these days it actually
> works within inline functions, but my point that
> __builtin_constant_p() is about a certain level of compiler
> optimization is very much true: you're actually testing for a compiler
> optimization detail.
>
> I know the LLVM people had similar issues with this comparison, so
> these days it's not even just about gcc versions. We may never have
> cared very much about icc, but llvm is actually an interesting target
> compiler.
>
And this is why ww_ctx == NULL is now passed as an inline argument. :)

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

Re: [GIT PULL] locking fix

[Linus Torvalds]

On Sun, Oct 27, 2013 at 12:37 PM, Maarten Lankhorst
 wrote:
>
> I would love for a compiler to become that smart though, but I do not think 
> it's likely.

Dammit, even if that is true, then write the conditional *correctly*.

As mentioned, the conditional

__builtin_constant_p(ww_ctx) && ww_ctx == NULL

is actually sensible, in a way the original one was *not*. It actually
tests what you apparently intended to test, and is more readable to
humans to boot.

And no, it still isn't actually guaranteed to do what you want it to
do. Historically, in gcc, __builtin_constant_p() really only ever
worked in macros, because by the time you use it in inline functions,
a constant NULL in the caller will have been turned into a argument
variable in the inline function, and __builtin_constant_p() would be
done before that was optimized away. Over the years, gcc has pushed
some of the builtin evaluation deeper down, and these days it actually
works within inline functions, but my point that
__builtin_constant_p() is about a certain level of compiler
optimization is very much true: you're actually testing for a compiler
optimization detail.

I know the LLVM people had similar issues with this comparison, so
these days it's not even just about gcc versions. We may never have
cared very much about icc, but llvm is actually an interesting target
compiler.

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

Re: [GIT PULL] locking fix

[Maarten Lankhorst]

op 27-10-13 20:23, Linus Torvalds schreef:
> On Sun, Oct 27, 2013 at 12:00 PM, Maarten Lankhorst
>  wrote:
>> op 27-10-13 18:28, Linus Torvalds schreef:
>>> That expression is largely equivalent to
>>> "__builtin_constant_p(ww_ctx)" (because iff ww_ctx is constant, then
>>> the comparison to NULL is constant), which is actually much easier to
>>> read, while carrying a totally different semantic meaning. Making
>>> things worse, the comparison to NULL *may* be marked constant under
>>> some very random situations (ie the compiler could turn a "taking an
>>> address of a variable is never NULL" kind of knowledge and combining
>>> it with other knowledge, and turn a complicated "ctx" expression into
>>> a "I know this cannot be NULL" thing, and thus the "== NULL" is a
>>> constant, even though ctx itself is some dynamic calculation).
>>>
>>> Whoever wrote the original should be shot. And this commit shouldn't
>>> have been marked as being somehow about gcc-version dependence, but
>>> about removing completely crap code.
>>>
>> Unfortunately gcc disagreed there, which was another compiler bug.
> Stop this idiotic "blame gcc bug" crap. Which part of my explanation
> for why it was *NOT* a compiler bug did you not understand?
>
>> __builtin_constant_p(ww_ctx) was NOT equal to __builtin_constant_p(ww_ctx == 
>> NULL), iirc.
> See my "largely equivalent" comment, with the *EXTRA* logic that gcc
> may actually find cases where the comparison is a constant even if the
> ww_ctx thing itself isn't a constant.
Sure in the theoretical case it's possible.
>>  __builtin_constant_p(ww_ctx == NULL) is equal to 
>> __builtin_constant_p(ww_ctx != NULL), but
>> the former is more readable, since it shows we expect ww_ctx to be null.
> Stop the f*cking around already! The  whole "we expect ww_ctx to be
> null" thing shows that YOU DO NOT SEEM TO UNDERSTAND WHAT THE TEST
> ACTUALLY IS!
>
> The expression
>
>__builtin_constant_p(ww_ctx == NULL)
>
> has ABSOLUTELY NOTHING to do with whether ww_ctx is NULL or not!
> Christ, can you really not understand that?
I'm fully aware, I just think the compiler cannot know that the address is 
always non-null for a generic function that takes an argument and isn't inlined.

> For example, ww_ctx could be "_variable", and the compiler can
> - and some compiles _will_ - say that ww_ctx clearly cannot be NULL,
> so "ww_ctx == NULL" is 0, which is a constant, so the
> __builtin_constant_p() expression returns true. See? That expression
> has absolutely NOTHING to do with whether you passed in NULL or not.
> NOTHING.
but __ww_mutex_lock isn't inlined..
> That __builtin_constant_p() tests whether the comparison is
> *CONSTANT*. And "0" is just as much a constant as "1" is. Really. So
> the whole f*cking expression is total and utter crap, because it is
> entirely and utterly senseless. It lacks all meaning. It's not
> actually testing for NULL at all. Never was, never will.
>
> The *ONLY* thing it is testing for is "how much can the compiler
> optimize this", and as such the *ONLY* thing it tests for is compiler
> differences.
>
> Really. Seriously. If you start blaming the compiler for different
> compilers giving different results, the only thing *that* shows is
> that you didn't understand the expression to begin with.
>
>> But yeah I guess it was too broken in gcc after all, so that's why it had to 
>> be killed altogether.
> NO NO NO NO. No a f*cking thousand times. It's not "too broken in
> gcc". It's too broken in the source code, and the fact that you don't
> even understand that is sad. You wrote the code, and you seem to be
> unable to admit that *your* code was buggy.
>
> It's not a compiler bug. It's your bug. Stand up like a man, instead
> of trying to flail around and blame anything else but yourself.
>
> So guys, get your act together, and stop blaming the compiler already.
I never denied my original code didn't contain bugs, which is why I wrote that 
fix. I just don't believe gcc
will ever be smart enough to determine that ww_ctx is a non-null argument in 
all calls to __ww_mutex_lock,
and then determine for that reason ww_ctx != NULL would be an invariant.

I would love for a compiler to become that smart though, but I do not think 
it's likely.

But hey it was a bug, my code was buggy and I helped by suggesting how to write 
the correct fix.

~Maarten

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

Re: [GIT PULL] locking fix

[Linus Torvalds]

On Sun, Oct 27, 2013 at 12:23 PM, Linus Torvalds
 wrote:
>
> The *ONLY* thing it is testing for is "how much can the compiler
> optimize this", and as such the *ONLY* thing it tests for is compiler
> differences.

Side note: testing "can the compiler optimize this expression at
compile time" is actually sometimes an interesting question, so it can
be a valid thing to test.

But people should understand that the question is literally about THAT
(ie visibility into compiler optimization) rather than about the value
itself.

So generally, the only thing that a __builtin_constant_p() test can be
used for is in *conjunction* with having an actual test for an actual
value, and then having special-case logic that pertains to that value.

So for example, *this* is a valid test:

if (__builtin_constant_p(ww_ctx) && ww_ctx == NULL) {
... special compile-time shortcut for the NULL value ..
} else {
... generic code that *also* handles the NULL value ..
}

and it's useful for triggering a compile-time optimized code-sequence
that is only true for NULL. But because __builtin_constant_p() is
about "how well can the compiler optimize this", that "else" statement
had better be able to handle the generic case too.

And yes, there are a few places where we do expect a certain minimal
set of optimizations. So in some cases we *might* have the rule that
the only valid use of NULL in a case like the above is when the
pointer passed in is passed in as a constant. And then we might say
"we rely on the compiler always returning true for
__builtin_constant_p(NULL)", and then we might say "so the "generic"
version of the code is guaranteed to never see NULL".

But notice how *different* that

__builtin_constant_p(ww_ctx) && ww_ctx == NULL

test is from

__builtin_constant_p(ww_ctx == NULL)

and really, the two tests are *fundamentally* really really different.
The first one can make sense. While the second one is pure and utter
garbage.

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

Re: [GIT PULL] locking fix

[Linus Torvalds]

On Sun, Oct 27, 2013 at 12:00 PM, Maarten Lankhorst
 wrote:
> op 27-10-13 18:28, Linus Torvalds schreef:
>>
>> That expression is largely equivalent to
>> "__builtin_constant_p(ww_ctx)" (because iff ww_ctx is constant, then
>> the comparison to NULL is constant), which is actually much easier to
>> read, while carrying a totally different semantic meaning. Making
>> things worse, the comparison to NULL *may* be marked constant under
>> some very random situations (ie the compiler could turn a "taking an
>> address of a variable is never NULL" kind of knowledge and combining
>> it with other knowledge, and turn a complicated "ctx" expression into
>> a "I know this cannot be NULL" thing, and thus the "== NULL" is a
>> constant, even though ctx itself is some dynamic calculation).
>>
>> Whoever wrote the original should be shot. And this commit shouldn't
>> have been marked as being somehow about gcc-version dependence, but
>> about removing completely crap code.
>>
> Unfortunately gcc disagreed there, which was another compiler bug.

Stop this idiotic "blame gcc bug" crap. Which part of my explanation
for why it was *NOT* a compiler bug did you not understand?

> __builtin_constant_p(ww_ctx) was NOT equal to __builtin_constant_p(ww_ctx == 
> NULL), iirc.

See my "largely equivalent" comment, with the *EXTRA* logic that gcc
may actually find cases where the comparison is a constant even if the
ww_ctx thing itself isn't a constant.

> __builtin_constant_p(ww_ctx == NULL) is equal to __builtin_constant_p(ww_ctx 
> != NULL), but
> the former is more readable, since it shows we expect ww_ctx to be null.

Stop the f*cking around already! The  whole "we expect ww_ctx to be
null" thing shows that YOU DO NOT SEEM TO UNDERSTAND WHAT THE TEST
ACTUALLY IS!

The expression

   __builtin_constant_p(ww_ctx == NULL)

has ABSOLUTELY NOTHING to do with whether ww_ctx is NULL or not!
Christ, can you really not understand that?

For example, ww_ctx could be "_variable", and the compiler can
- and some compiles _will_ - say that ww_ctx clearly cannot be NULL,
so "ww_ctx == NULL" is 0, which is a constant, so the
__builtin_constant_p() expression returns true. See? That expression
has absolutely NOTHING to do with whether you passed in NULL or not.
NOTHING.

That __builtin_constant_p() tests whether the comparison is
*CONSTANT*. And "0" is just as much a constant as "1" is. Really. So
the whole f*cking expression is total and utter crap, because it is
entirely and utterly senseless. It lacks all meaning. It's not
actually testing for NULL at all. Never was, never will.

The *ONLY* thing it is testing for is "how much can the compiler
optimize this", and as such the *ONLY* thing it tests for is compiler
differences.

Really. Seriously. If you start blaming the compiler for different
compilers giving different results, the only thing *that* shows is
that you didn't understand the expression to begin with.

> But yeah I guess it was too broken in gcc after all, so that's why it had to 
> be killed altogether.

NO NO NO NO. No a f*cking thousand times. It's not "too broken in
gcc". It's too broken in the source code, and the fact that you don't
even understand that is sad. You wrote the code, and you seem to be
unable to admit that *your* code was buggy.

It's not a compiler bug. It's your bug. Stand up like a man, instead
of trying to flail around and blame anything else but yourself.

So guys, get your act together, and stop blaming the compiler already.

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

Re: [GIT PULL] locking fix

[Maarten Lankhorst]

op 27-10-13 18:28, Linus Torvalds schreef:
> On Sat, Oct 26, 2013 at 5:19 AM, Ingo Molnar  wrote:
>> This tree fixes a boot crash in CONFIG_DEBUG_MUTEXES=y kernels, on
>> kernels built with GCC 3.x. (There are still such distros.)
> Btw, it's really not just gcc 3.x. That code was (a) incomprehensible,
> (b) wrong and (c) caused problems for LLVM too.
>
> It was wrong because "__builtin_constant_p(ww_ctx == NULL)" simply
> makes no sense.
>
> Why?
>
> That expression is largely equivalent to
> "__builtin_constant_p(ww_ctx)" (because iff ww_ctx is constant, then
> the comparison to NULL is constant), which is actually much easier to
> read, while carrying a totally different semantic meaning. Making
> things worse, the comparison to NULL *may* be marked constant under
> some very random situations (ie the compiler could turn a "taking an
> address of a variable is never NULL" kind of knowledge and combining
> it with other knowledge, and turn a complicated "ctx" expression into
> a "I know this cannot be NULL" thing, and thus the "== NULL" is a
> constant, even though ctx itself is some dynamic calculation).
>
> Whoever wrote the original should be shot. And this commit shouldn't
> have been marked as being somehow about gcc-version dependence, but
> about removing completely crap code.
>
Unfortunately gcc disagreed there, which was another compiler bug.
__builtin_constant_p(ww_ctx) was NOT equal to __builtin_constant_p(ww_ctx == 
NULL), iirc.
__builtin_constant_p(ww_ctx == NULL) is equal to __builtin_constant_p(ww_ctx != 
NULL), but
the former is more readable, since it shows we expect ww_ctx to be null.

But yeah I guess it was too broken in gcc after all, so that's why it had to be 
killed altogether.

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

Re: [GIT PULL] locking fix

[Linus Torvalds]

On Sat, Oct 26, 2013 at 5:19 AM, Ingo Molnar  wrote:
>
> This tree fixes a boot crash in CONFIG_DEBUG_MUTEXES=y kernels, on
> kernels built with GCC 3.x. (There are still such distros.)

Btw, it's really not just gcc 3.x. That code was (a) incomprehensible,
(b) wrong and (c) caused problems for LLVM too.

It was wrong because "__builtin_constant_p(ww_ctx == NULL)" simply
makes no sense.

Why?

That expression is largely equivalent to
"__builtin_constant_p(ww_ctx)" (because iff ww_ctx is constant, then
the comparison to NULL is constant), which is actually much easier to
read, while carrying a totally different semantic meaning. Making
things worse, the comparison to NULL *may* be marked constant under
some very random situations (ie the compiler could turn a "taking an
address of a variable is never NULL" kind of knowledge and combining
it with other knowledge, and turn a complicated "ctx" expression into
a "I know this cannot be NULL" thing, and thus the "== NULL" is a
constant, even though ctx itself is some dynamic calculation).

Whoever wrote the original should be shot. And this commit shouldn't
have been marked as being somehow about gcc-version dependence, but
about removing completely crap code.

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

Re: [GIT PULL] locking fix

[Linus Torvalds]

On Sat, Oct 26, 2013 at 5:19 AM, Ingo Molnar mi...@kernel.org wrote:

 This tree fixes a boot crash in CONFIG_DEBUG_MUTEXES=y kernels, on
 kernels built with GCC 3.x. (There are still such distros.)

Btw, it's really not just gcc 3.x. That code was (a) incomprehensible,
(b) wrong and (c) caused problems for LLVM too.

It was wrong because __builtin_constant_p(ww_ctx == NULL) simply
makes no sense.

Why?

That expression is largely equivalent to
__builtin_constant_p(ww_ctx) (because iff ww_ctx is constant, then
the comparison to NULL is constant), which is actually much easier to
read, while carrying a totally different semantic meaning. Making
things worse, the comparison to NULL *may* be marked constant under
some very random situations (ie the compiler could turn a taking an
address of a variable is never NULL kind of knowledge and combining
it with other knowledge, and turn a complicated ctx expression into
a I know this cannot be NULL thing, and thus the == NULL is a
constant, even though ctx itself is some dynamic calculation).

Whoever wrote the original should be shot. And this commit shouldn't
have been marked as being somehow about gcc-version dependence, but
about removing completely crap code.

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

Re: [GIT PULL] locking fix

[Maarten Lankhorst]

op 27-10-13 18:28, Linus Torvalds schreef:
 On Sat, Oct 26, 2013 at 5:19 AM, Ingo Molnar mi...@kernel.org wrote:
 This tree fixes a boot crash in CONFIG_DEBUG_MUTEXES=y kernels, on
 kernels built with GCC 3.x. (There are still such distros.)
 Btw, it's really not just gcc 3.x. That code was (a) incomprehensible,
 (b) wrong and (c) caused problems for LLVM too.

 It was wrong because __builtin_constant_p(ww_ctx == NULL) simply
 makes no sense.

 Why?

 That expression is largely equivalent to
 __builtin_constant_p(ww_ctx) (because iff ww_ctx is constant, then
 the comparison to NULL is constant), which is actually much easier to
 read, while carrying a totally different semantic meaning. Making
 things worse, the comparison to NULL *may* be marked constant under
 some very random situations (ie the compiler could turn a taking an
 address of a variable is never NULL kind of knowledge and combining
 it with other knowledge, and turn a complicated ctx expression into
 a I know this cannot be NULL thing, and thus the == NULL is a
 constant, even though ctx itself is some dynamic calculation).

 Whoever wrote the original should be shot. And this commit shouldn't
 have been marked as being somehow about gcc-version dependence, but
 about removing completely crap code.

Unfortunately gcc disagreed there, which was another compiler bug.
__builtin_constant_p(ww_ctx) was NOT equal to __builtin_constant_p(ww_ctx == 
NULL), iirc.
__builtin_constant_p(ww_ctx == NULL) is equal to __builtin_constant_p(ww_ctx != 
NULL), but
the former is more readable, since it shows we expect ww_ctx to be null.

But yeah I guess it was too broken in gcc after all, so that's why it had to be 
killed altogether.

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

Re: [GIT PULL] locking fix

[Linus Torvalds]

On Sun, Oct 27, 2013 at 12:00 PM, Maarten Lankhorst
maarten.lankho...@canonical.com wrote:
 op 27-10-13 18:28, Linus Torvalds schreef:

 That expression is largely equivalent to
 __builtin_constant_p(ww_ctx) (because iff ww_ctx is constant, then
 the comparison to NULL is constant), which is actually much easier to
 read, while carrying a totally different semantic meaning. Making
 things worse, the comparison to NULL *may* be marked constant under
 some very random situations (ie the compiler could turn a taking an
 address of a variable is never NULL kind of knowledge and combining
 it with other knowledge, and turn a complicated ctx expression into
 a I know this cannot be NULL thing, and thus the == NULL is a
 constant, even though ctx itself is some dynamic calculation).

 Whoever wrote the original should be shot. And this commit shouldn't
 have been marked as being somehow about gcc-version dependence, but
 about removing completely crap code.

 Unfortunately gcc disagreed there, which was another compiler bug.

Stop this idiotic blame gcc bug crap. Which part of my explanation
for why it was *NOT* a compiler bug did you not understand?

 __builtin_constant_p(ww_ctx) was NOT equal to __builtin_constant_p(ww_ctx == 
 NULL), iirc.

See my largely equivalent comment, with the *EXTRA* logic that gcc
may actually find cases where the comparison is a constant even if the
ww_ctx thing itself isn't a constant.

 __builtin_constant_p(ww_ctx == NULL) is equal to __builtin_constant_p(ww_ctx 
 != NULL), but
 the former is more readable, since it shows we expect ww_ctx to be null.

Stop the f*cking around already! The  whole we expect ww_ctx to be
null thing shows that YOU DO NOT SEEM TO UNDERSTAND WHAT THE TEST
ACTUALLY IS!

The expression

   __builtin_constant_p(ww_ctx == NULL)

has ABSOLUTELY NOTHING to do with whether ww_ctx is NULL or not!
Christ, can you really not understand that?

For example, ww_ctx could be static_variable, and the compiler can
- and some compiles _will_ - say that ww_ctx clearly cannot be NULL,
so ww_ctx == NULL is 0, which is a constant, so the
__builtin_constant_p() expression returns true. See? That expression
has absolutely NOTHING to do with whether you passed in NULL or not.
NOTHING.

That __builtin_constant_p() tests whether the comparison is
*CONSTANT*. And 0 is just as much a constant as 1 is. Really. So
the whole f*cking expression is total and utter crap, because it is
entirely and utterly senseless. It lacks all meaning. It's not
actually testing for NULL at all. Never was, never will.

The *ONLY* thing it is testing for is how much can the compiler
optimize this, and as such the *ONLY* thing it tests for is compiler
differences.

Really. Seriously. If you start blaming the compiler for different
compilers giving different results, the only thing *that* shows is
that you didn't understand the expression to begin with.

 But yeah I guess it was too broken in gcc after all, so that's why it had to 
 be killed altogether.

NO NO NO NO. No a f*cking thousand times. It's not too broken in
gcc. It's too broken in the source code, and the fact that you don't
even understand that is sad. You wrote the code, and you seem to be
unable to admit that *your* code was buggy.

It's not a compiler bug. It's your bug. Stand up like a man, instead
of trying to flail around and blame anything else but yourself.

So guys, get your act together, and stop blaming the compiler already.

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

Re: [GIT PULL] locking fix

[Linus Torvalds]

On Sun, Oct 27, 2013 at 12:23 PM, Linus Torvalds
torva...@linux-foundation.org wrote:

 The *ONLY* thing it is testing for is how much can the compiler
 optimize this, and as such the *ONLY* thing it tests for is compiler
 differences.

Side note: testing can the compiler optimize this expression at
compile time is actually sometimes an interesting question, so it can
be a valid thing to test.

But people should understand that the question is literally about THAT
(ie visibility into compiler optimization) rather than about the value
itself.

So generally, the only thing that a __builtin_constant_p() test can be
used for is in *conjunction* with having an actual test for an actual
value, and then having special-case logic that pertains to that value.

So for example, *this* is a valid test:

if (__builtin_constant_p(ww_ctx)  ww_ctx == NULL) {
... special compile-time shortcut for the NULL value ..
} else {
... generic code that *also* handles the NULL value ..
}

and it's useful for triggering a compile-time optimized code-sequence
that is only true for NULL. But because __builtin_constant_p() is
about how well can the compiler optimize this, that else statement
had better be able to handle the generic case too.

And yes, there are a few places where we do expect a certain minimal
set of optimizations. So in some cases we *might* have the rule that
the only valid use of NULL in a case like the above is when the
pointer passed in is passed in as a constant. And then we might say
we rely on the compiler always returning true for
__builtin_constant_p(NULL), and then we might say so the generic
version of the code is guaranteed to never see NULL.

But notice how *different* that

__builtin_constant_p(ww_ctx)  ww_ctx == NULL

test is from

__builtin_constant_p(ww_ctx == NULL)

and really, the two tests are *fundamentally* really really different.
The first one can make sense. While the second one is pure and utter
garbage.

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

Re: [GIT PULL] locking fix

[Maarten Lankhorst]

op 27-10-13 20:23, Linus Torvalds schreef:
 On Sun, Oct 27, 2013 at 12:00 PM, Maarten Lankhorst
 maarten.lankho...@canonical.com wrote:
 op 27-10-13 18:28, Linus Torvalds schreef:
 That expression is largely equivalent to
 __builtin_constant_p(ww_ctx) (because iff ww_ctx is constant, then
 the comparison to NULL is constant), which is actually much easier to
 read, while carrying a totally different semantic meaning. Making
 things worse, the comparison to NULL *may* be marked constant under
 some very random situations (ie the compiler could turn a taking an
 address of a variable is never NULL kind of knowledge and combining
 it with other knowledge, and turn a complicated ctx expression into
 a I know this cannot be NULL thing, and thus the == NULL is a
 constant, even though ctx itself is some dynamic calculation).

 Whoever wrote the original should be shot. And this commit shouldn't
 have been marked as being somehow about gcc-version dependence, but
 about removing completely crap code.

 Unfortunately gcc disagreed there, which was another compiler bug.
 Stop this idiotic blame gcc bug crap. Which part of my explanation
 for why it was *NOT* a compiler bug did you not understand?

 __builtin_constant_p(ww_ctx) was NOT equal to __builtin_constant_p(ww_ctx == 
 NULL), iirc.
 See my largely equivalent comment, with the *EXTRA* logic that gcc
 may actually find cases where the comparison is a constant even if the
 ww_ctx thing itself isn't a constant.
Sure in the theoretical case it's possible.
  __builtin_constant_p(ww_ctx == NULL) is equal to 
 __builtin_constant_p(ww_ctx != NULL), but
 the former is more readable, since it shows we expect ww_ctx to be null.
 Stop the f*cking around already! The  whole we expect ww_ctx to be
 null thing shows that YOU DO NOT SEEM TO UNDERSTAND WHAT THE TEST
 ACTUALLY IS!

 The expression

__builtin_constant_p(ww_ctx == NULL)

 has ABSOLUTELY NOTHING to do with whether ww_ctx is NULL or not!
 Christ, can you really not understand that?
I'm fully aware, I just think the compiler cannot know that the address is 
always non-null for a generic function that takes an argument and isn't inlined.

 For example, ww_ctx could be static_variable, and the compiler can
 - and some compiles _will_ - say that ww_ctx clearly cannot be NULL,
 so ww_ctx == NULL is 0, which is a constant, so the
 __builtin_constant_p() expression returns true. See? That expression
 has absolutely NOTHING to do with whether you passed in NULL or not.
 NOTHING.
but __ww_mutex_lock isn't inlined..
 That __builtin_constant_p() tests whether the comparison is
 *CONSTANT*. And 0 is just as much a constant as 1 is. Really. So
 the whole f*cking expression is total and utter crap, because it is
 entirely and utterly senseless. It lacks all meaning. It's not
 actually testing for NULL at all. Never was, never will.

 The *ONLY* thing it is testing for is how much can the compiler
 optimize this, and as such the *ONLY* thing it tests for is compiler
 differences.

 Really. Seriously. If you start blaming the compiler for different
 compilers giving different results, the only thing *that* shows is
 that you didn't understand the expression to begin with.

 But yeah I guess it was too broken in gcc after all, so that's why it had to 
 be killed altogether.
 NO NO NO NO. No a f*cking thousand times. It's not too broken in
 gcc. It's too broken in the source code, and the fact that you don't
 even understand that is sad. You wrote the code, and you seem to be
 unable to admit that *your* code was buggy.

 It's not a compiler bug. It's your bug. Stand up like a man, instead
 of trying to flail around and blame anything else but yourself.

 So guys, get your act together, and stop blaming the compiler already.
I never denied my original code didn't contain bugs, which is why I wrote that 
fix. I just don't believe gcc
will ever be smart enough to determine that ww_ctx is a non-null argument in 
all calls to __ww_mutex_lock,
and then determine for that reason ww_ctx != NULL would be an invariant.

I would love for a compiler to become that smart though, but I do not think 
it's likely.

But hey it was a bug, my code was buggy and I helped by suggesting how to write 
the correct fix.

~Maarten

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

Re: [GIT PULL] locking fix

[Linus Torvalds]

On Sun, Oct 27, 2013 at 12:37 PM, Maarten Lankhorst
maarten.lankho...@canonical.com wrote:

 I would love for a compiler to become that smart though, but I do not think 
 it's likely.

Dammit, even if that is true, then write the conditional *correctly*.

As mentioned, the conditional

__builtin_constant_p(ww_ctx)  ww_ctx == NULL

is actually sensible, in a way the original one was *not*. It actually
tests what you apparently intended to test, and is more readable to
humans to boot.

And no, it still isn't actually guaranteed to do what you want it to
do. Historically, in gcc, __builtin_constant_p() really only ever
worked in macros, because by the time you use it in inline functions,
a constant NULL in the caller will have been turned into a argument
variable in the inline function, and __builtin_constant_p() would be
done before that was optimized away. Over the years, gcc has pushed
some of the builtin evaluation deeper down, and these days it actually
works within inline functions, but my point that
__builtin_constant_p() is about a certain level of compiler
optimization is very much true: you're actually testing for a compiler
optimization detail.

I know the LLVM people had similar issues with this comparison, so
these days it's not even just about gcc versions. We may never have
cared very much about icc, but llvm is actually an interesting target
compiler.

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

Re: [GIT PULL] locking fix

[Maarten Lankhorst]

op 27-10-13 20:51, Linus Torvalds schreef:
 On Sun, Oct 27, 2013 at 12:37 PM, Maarten Lankhorst
 maarten.lankho...@canonical.com wrote:
 I would love for a compiler to become that smart though, but I do not think 
 it's likely.
 Dammit, even if that is true, then write the conditional *correctly*.

 As mentioned, the conditional

 __builtin_constant_p(ww_ctx)  ww_ctx == NULL

 is actually sensible, in a way the original one was *not*. It actually
 tests what you apparently intended to test, and is more readable to
 humans to boot.
Yeah that mail arrived after I sent mine, I agree that this would have been 
more sensible.
 And no, it still isn't actually guaranteed to do what you want it to
 do. Historically, in gcc, __builtin_constant_p() really only ever
 worked in macros, because by the time you use it in inline functions,
 a constant NULL in the caller will have been turned into a argument
 variable in the inline function, and __builtin_constant_p() would be
 done before that was optimized away. Over the years, gcc has pushed
 some of the builtin evaluation deeper down, and these days it actually
 works within inline functions, but my point that
 __builtin_constant_p() is about a certain level of compiler
 optimization is very much true: you're actually testing for a compiler
 optimization detail.

 I know the LLVM people had similar issues with this comparison, so
 these days it's not even just about gcc versions. We may never have
 cared very much about icc, but llvm is actually an interesting target
 compiler.

And this is why ww_ctx == NULL is now passed as an inline argument. :)

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

Re: [GIT PULL] locking fix

[Linus Torvalds]

On Sun, Oct 27, 2013 at 12:56 PM, Maarten Lankhorst
maarten.lankho...@canonical.com wrote:

 And this is why ww_ctx == NULL is now passed as an inline argument. :)

Well, more than that - the optimization has been done at the source
code level, so that the behavior is no longer a matter about how well
the compiler optimizes it any more.

I'm not complaining about the fix. I'm complaining about how the fix
was claimed to be due to a compiler bug. The documentation for the
fix (ie the commit message) was actively misleading.

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