Re: [RFC PATCH] x86, oprofile: fix P4 oprofile CPU setup bug

[Ingo Molnar]

* Ingo Molnar <mingo@xxxxxxx> wrote:

> Hm, you only seem to be looking at solving that warning message, 
> and your interest does not seem to go outside of that scope at 
> all. Your first fix was to make the message go away, your second 
> fix minimally (tried to address) the review from Linus and did a 
> mechanic change to make the message go away.
> 
> This second change was actually subtly worse than the first one: 
> because it spread the uncleanliness elsewhere. The getting of 
> 'stagger' is in no way central to the code, it does not denote 
> critical sections at all.

btw. - and this is a pet peeve of mine - in the x86 tree we never 
ever "fix" warnings.

Warnings can only be dealt with in two ways correctly:

 - Fix _false positive_ warnings - carefully analyzing and 
   explaining why the warning is a false positive.

 - .. or by fixing _real bugs_, which bugs some warning mechanism
   exposed. The warning there does not need to be 'fixed' - it was
   the canary for a real bug and we fix the _bug_, not the warning. 
   ( the warning does go away but only as a side-effect of the bug
     going away. )

The moment you start 'fixing warnings' (and i note here that you 
seem to be interested in doing many such changes and cleanups, so we 
better talk about this fine distinction), you risk the exact pattern 
that happened above:

... something subtle happened somewhere - and you sent a patch that 
you denoted as a 'fix' in the subject line. Your commit log did not 
show at all whether you understood the full context, and you did not 
analyze the _reason_ for the warning, at all.

I routinely reject such patches, just based on that pattern, even if 
i happen to agree with the change.

They are in fact _actively harmful_, because a poor overworked 
maintainer like me might apply it in a weak moment. [ except Linus, 
who is overworked and still notices such patches but what's new ;-) ]

Instead, if you see such warnings, and dont know why they trigger - 
please send in the message denoted as a bug report. Or if you have a 
guess of a patch but are unsure about a change in any way send in an 
RFC patch and _declare_ the bits you are sure about and the bits you 
are unsure about, and ask specific questions and show how far you 
got in the analysis. It is not a problem to declare the area you 
dont understand - it is useful information to those reading your 
emails. Conversely, the _lack_ of such information is harmful. Ok?

Please dont _ever_ send in patches without a meaningful changelog.

Take this as a warning! ;-)

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