[PATCH] x86: !x & y typo in mtrr code

[PATCH] x86: !x & y typo in mtrr code

[Harvey Harrison]

As written, this can never be true.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 arch/x86/kernel/cpu/mtrr/generic.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 353efe4..5d241ce 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -90,7 +90,7 @@ u8 mtrr_type_lookup(u64 start, u64 end)
 	 * Look of multiple ranges matching this address and pick type
 	 * as per MTRR precedence
 	 */
-	if (!mtrr_state.enabled & 2) {
+	if (!(mtrr_state.enabled & 2)) {
 		return mtrr_state.def_type;
 	}
 
-- 
1.5.5.1.270.g89765

Re: [PATCH] x86: !x & y typo in mtrr code

[Al Viro]

On Sat, Apr 26, 2008 at 09:00:17PM -0700, Harvey Harrison wrote:
> As written, this can never be true.

... and introduced through x86.git, of all things...  May I suggest that
in addition to all-important whitespace hunting something that catches
lowly real bugs would also be run over that tree?  sparse *does* catch
that...

Re: [PATCH] x86: !x & y typo in mtrr code

[Ingo Molnar]

* Harvey Harrison <harvey.harrison@gmail.com> wrote:

> As written, this can never be true.

thanks Harvey, applied. Found via Sparse?

	Ingo

Re: [PATCH] x86: !x & y typo in mtrr code

[Harvey Harrison]

On Mon, 2008-04-28 at 13:27 +0200, Ingo Molnar wrote:
> * Harvey Harrison <harvey.harrison@gmail.com> wrote:
> 
> > As written, this can never be true.
> 
> thanks Harvey, applied. Found via Sparse?
> 

Yeah.

Harvey

RE: [PATCH] x86: !x & y typo in mtrr code

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: linux-kernel-owner@vger.kernel.org 
>[mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of 
>Harvey Harrison
>Sent: Saturday, April 26, 2008 9:00 PM
>To: Ingo Molnar
>Cc: Thomas Gleixner; H. Peter Anvin; LKML
>Subject: [PATCH] x86: !x & y typo in mtrr code
>
>As written, this can never be true.
>

My bad. I introduced this ugly and nasty bug.

Harvey: Thanks for catching the bug and the patch.

Thanks,
Venki

>Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
>---
> arch/x86/kernel/cpu/mtrr/generic.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
>diff --git a/arch/x86/kernel/cpu/mtrr/generic.c 
>b/arch/x86/kernel/cpu/mtrr/generic.c
>index 353efe4..5d241ce 100644
>--- a/arch/x86/kernel/cpu/mtrr/generic.c
>+++ b/arch/x86/kernel/cpu/mtrr/generic.c
>@@ -90,7 +90,7 @@ u8 mtrr_type_lookup(u64 start, u64 end)
> 	 * Look of multiple ranges matching this address and pick type
> 	 * as per MTRR precedence
> 	 */
>-	if (!mtrr_state.enabled & 2) {
>+	if (!(mtrr_state.enabled & 2)) {
> 		return mtrr_state.def_type;
> 	}
> 
>-- 
>1.5.5.1.270.g89765
>
>
>
>--
>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/
>

Re: [PATCH] x86: !x & y typo in mtrr code

[Ingo Molnar]

* Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Sat, Apr 26, 2008 at 09:00:17PM -0700, Harvey Harrison wrote:
> > As written, this can never be true.
> 
> ... and introduced through x86.git, of all things...  May I suggest 
> that in addition to all-important whitespace hunting something that 
> catches lowly real bugs would also be run over that tree?  sparse 
> *does* catch that...

firstly, it _was_ caught via Sparse, secondly, if you check the code 
flow, the bug - while indeed ugly looking - was harmless and had no 
side-effects. That's why it didnt trigger in testing.

	Ingo

Re: [PATCH] x86: !x & y typo in mtrr code

[Björn Steinbrink]

On 2008.04.29 23:04:35 +0200, Ingo Molnar wrote:
> 
> * Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > On Sat, Apr 26, 2008 at 09:00:17PM -0700, Harvey Harrison wrote:
> > > As written, this can never be true.
> > 
> > ... and introduced through x86.git, of all things...  May I suggest 
> > that in addition to all-important whitespace hunting something that 
> > catches lowly real bugs would also be run over that tree?  sparse 
> > *does* catch that...
> 
> firstly, it _was_ caught via Sparse, ...

But that is exactly Al's point. Had you run sparse over your tree
_before_ it got pulled by Linus, that bug should have never made it into
mainline.

Björn

Re: [PATCH] x86: !x & y typo in mtrr code

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 839 bytes --]

On Wed, 30 Apr 2008, Björn Steinbrink wrote:
> On 2008.04.29 23:04:35 +0200, Ingo Molnar wrote:
> > 
> > * Al Viro <viro@ZenIV.linux.org.uk> wrote:
> > 
> > > On Sat, Apr 26, 2008 at 09:00:17PM -0700, Harvey Harrison wrote:
> > > > As written, this can never be true.
> > > 
> > > ... and introduced through x86.git, of all things...  May I suggest 
> > > that in addition to all-important whitespace hunting something that 
> > > catches lowly real bugs would also be run over that tree?  sparse 
> > > *does* catch that...
> > 
> > firstly, it _was_ caught via Sparse, ...
> 
> But that is exactly Al's point. Had you run sparse over your tree
> _before_ it got pulled by Linus, that bug should have never made it into
> mainline.

sparse builds are on the way to the automated build framework
already. won't happen again.

Thanks,
	tglx

Re: [PATCH] x86: !x & y typo in mtrr code

[Ingo Molnar]

* Bj?rn Steinbrink <B.Steinbrink@gmx.de> wrote:

> > firstly, it _was_ caught via Sparse, ...
> 
> But that is exactly Al's point. Had you run sparse over your tree 
> _before_ it got pulled by Linus, that bug should have never made it 
> into mainline.

It is clear from your argument that you have never attempted to do this 
yourself for any significant size of code. Sparse output is way too 
verbose, the false positive rate is well above 50% and way too few 
subsystems care about it.

What you have also missed is the plain fact that x86 _is_ amongst the 
very few subsystems that _do_ regular Sparse runs.  The person who does 
this regular (and not trivial) manual Sparse sweep work for arch/x86 is 
... Harvey, the person who submitted this fix! Most of the time Harvey's 
Sparse fixes hit x86.git before we push it upstream. Sometimes, as in 
this case, after. As per the analysis of this specific bug no puppies 
were hurt - and that's typical of most Sparse fixes.

Sparse analysis is not just a matter of typing "make C=1" and fixing 
whatever it prints... FYI, today's -git has the following Sparse 
warnings histogram (sparse-0.4.1-2.fc9):

      2  error: incompatible types in comparison expression (different address spaces)
      3  error: dubious one-bit signed bitfield
      5  error: subtraction of different types can't work (different address spaces)
     10  error: incompatible types for operation (*)
     10  error: invalid assignment
     24  error: cannot size expression
     97  error: bad constant expression
    157  error: bad integer constant expression
  86817  error: attribute '__cold__': unknown attribute

even ignoring those nearly one hundred thousand '__cold__' warnings 
(which came upstream many months ago via commit v2.6.24-2245-gf3fe866), 
that's still more than 300 warnings to sift through.

The reality is that nearly nobody runs Sparse for every patch and nearly 
nobody cares about making the default C=1 output meaningful and 
interesting to newbies. It's a frustrating experience at the moment, and 
its (non-) uptake is reflecting that reality.

And as anyone can see it from the verbosity of the histogram above, 
Sparse validation is a deeply manual work today, for something as 
complex as general x86. Al has delta analysis automation around it but 
those tools are not part of the upstream kernel yet and it's not usable 
for daily patch-management.

Automating Sparse runs so that it can become part of our daily 
patch-flow is _not_ trivial and it's not just a matter of typing make 
C=1. Thomas is working on integrating Sparse delta analysis into our 
daily patch workflow nevertheless.

But to suggest that we somehow should have done this before shows deep 
ignorance of the problem space, at best.

	Ingo

Re: [PATCH] x86: !x & y typo in mtrr code

[Björn Steinbrink]

On 2008.04.30 11:32:33 +0200, Ingo Molnar wrote:
> Automating Sparse runs so that it can become part of our daily 
> patch-flow is _not_ trivial and it's not just a matter of typing make 
> C=1. Thomas is working on integrating Sparse delta analysis into our 
> daily patch workflow nevertheless.

Who said daily? More often can be better, but just before the pull
request is still better than nothing. And even some very basic and
stupid filtering can make the sparse output more feasible without losing
"too much" information (my stupid approach here can fail for example
when you remove an error of one class while introducing another one of
the same class).

Given the merge of x86.git that introduced this specific bug we have:
9e9abecf - "merge x86.git"

Now we can get:
$ git merge-base 9e9abecf^1 9e9abecf^2
4b119e21d0c66c22e8ca03df05d9de623d0eb50f

So you branched at 4b119e21... (your history is linear, that helps here)

To get the same information before the merge happened upstream, in your
local repo, you can do:
git merge-base for-linus origin/master
(or whatever your branches are called)

# So we get the old upstream version:
git checkout 4b119e21d0c
# setup config, whatever
make
make C=2 2>old-sparse

# Then the state of your tree right before the merge:
git checkout 9e9abecf^2
make
make C=2 2>new-sparse

# And now some stupid sorting and cleaning:
sort old-sparse | sed -e 's/^\([^:]*\):[^:]*:[^:]*:/\1:/' > old-sparsef
sort new-sparse | sed -e 's/^\([^:]*\):[^:]*:[^:]*:/\1:/' > new-sparsef

# How many old sparse warnings are gone now (approx.)?
$ diff old-sparsef new-sparsef | grep -c '^< [^ ]*:'
33

# And how many new do we have (approx.)?
$ diff old-sparsef new-sparsef | grep -c '^> [^ ]*:'
36


Depending on the order of the sed and sort commands, you get slightly
different output, due to code moving around or whatever, of course to
get that "mostly" right it takes more than the 5 minutes of thinking
that I spent on it.

In those cases in which there is more than 1 line per warning, the
output is obivously messed up here, so the actual number of real
warnings from sparse that are left is even lower than the number above
(26 for gone and new warnings in this case). And you can also only use
the filtered messages to navigate the original sparse log, because the
line numbers are stripped and the messed up ordering of multi-line
warnings, but well, shouldn't be too bad.

Slap another level of filtering on it that can ignore warnings that are
known to be bogus or in which you're simply not interested or whatever
and you're down to a pretty manageable set of warnings I guess.

Finally, yeah, I know, this approach is far from perfect, but in my
nobody-should-care-about-it opinion, it's better than nothing and it's
probably not too hard for you (or Thomas) to come up with a less stupid
filtering strategy, that's less error-prone than my hack.

Björn

Re: [PATCH] x86: !x & y typo in mtrr code

[Ingo Molnar]

* Björn Steinbrink <B.Steinbrink@gmx.de> wrote:

> On 2008.04.30 11:32:33 +0200, Ingo Molnar wrote:
> > Automating Sparse runs so that it can become part of our daily 
> > patch-flow is _not_ trivial and it's not just a matter of typing 
> > make C=1. Thomas is working on integrating Sparse delta analysis 
> > into our daily patch workflow nevertheless.
> 
> Who said daily? More often can be better, but just before the pull 
> request is still better than nothing. [...]

Suffice to say that the number of large subsystems that actually run 
Sparse before each and every pull request is extremely low - this simply 
matches the practical difficulties involved.

You are suggesting ad-hoc solutions without having tried it all 
yourself. And i'd very much agree with doing an upstream kernel Sparse 
sweep shortly before the final release. Do you volunteer for that?

We for one simply concentrate on the the things that we think makes it 
more likely to find bugs. For example we build and boot x86.git with 
many different configs before every pull request. In practice that 
catches far more tester-critical bugs than Sparse - and we know that 
simply from the fact because we use both methods and have a good 
comparison of the results. We also work on automating Sparse checks in 
the future but as i said it, it's not easy.

	Ingo

Re: [PATCH] x86: !x & y typo in mtrr code

[Harvey Harrison]

On Wed, 2008-04-30 at 14:35 +0200, Ingo Molnar wrote:
> We for one simply concentrate on the the things that we think makes it 
> more likely to find bugs. For example we build and boot x86.git with 
> many different configs before every pull request. In practice that 
> catches far more tester-critical bugs than Sparse - and we know that 
> simply from the fact because we use both methods and have a good 
> comparison of the results. We also work on automating Sparse checks in 
> the future but as i said it, it's not easy.
> 

That's precisely why I've been sending all of the sparse cleanup patches
lately, to try and get the output down to something more managable.  You
may want to upgrade your sparse to one that understands __cold though.

Harvey

Re: [PATCH] x86: !x & y typo in mtrr code

[Ingo Molnar]

* Harvey Harrison <harvey.harrison@gmail.com> wrote:

> That's precisely why I've been sending all of the sparse cleanup 
> patches lately, to try and get the output down to something more 
> managable.  You may want to upgrade your sparse to one that 
> understands __cold though.

yep, and i wholeheartedly agree with those efforts of bringing the 
Sparse noise baseline back to near zero. That way it will be useful for 
the casual human look and normal patch workflow as well, not just 
(expensive to maintain and acquire) delta statistics.

	Ingo

Re: [PATCH] x86: !x & y typo in mtrr code

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]

On Wed, 30 Apr 2008 08:35:34 PDT, Harvey Harrison said:
> On Wed, 2008-04-30 at 14:35 +0200, Ingo Molnar wrote:
> > We for one simply concentrate on the the things that we think makes it 
> > more likely to find bugs. For example we build and boot x86.git with 
> > many different configs before every pull request. In practice that 
> > catches far more tester-critical bugs than Sparse - and we know that 
> > simply from the fact because we use both methods and have a good 
> > comparison of the results. We also work on automating Sparse checks in 
> > the future but as i said it, it's not easy.
> > 
> 
> That's precisely why I've been sending all of the sparse cleanup patches
> lately, to try and get the output down to something more managable.  You
> may want to upgrade your sparse to one that understands __cold though.

What release of sparse would that be?  Fedora ships one tagged as 0.4.1-2.fc9,
but that says:

include/linux/kernel.h:135:52: error: attribute '__cold__': unknown attribute

But you go look at Documentation/sparse.txt and it points you to:

"You can get latest released versions from the Sparse homepage at
http://www.kernel.org/pub/linux/kernel/people/josh/sparse/ "

which says 0.4.1 is the latest released version.  So is this a git-only feature?

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]