lkml.org 
[lkml]   [2009]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] x86, oprofile: fix P4 oprofile CPU setup bug

* Ingo Molnar <mingo@elte.hu> 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


\
 
 \ /
  Last update: 2009-04-06 00:03    [W:0.057 / U:0.120 seconds]
©2003-2014 Jasper Spaans. Advertise on this site