x86: !x & y typo in mtrr code

As written, this can never be true.

---
 arch/x86/kernel/cpu/mtrr/generic.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 353efe4..5d241ce 100644
@@ -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

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

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

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

Thanks,
tglx

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

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

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

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

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

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?

thanks Harvey, applied. Found via Sparse?

Ingo

Yeah.

Harvey

My bad. I introduced this ugly and nasty bug.

Harvey: Thanks for catching the bug and the patch.

Thanks,
Venki