Mailing List Archive: Linux: Kernel

[GIT PULL -tip][PATCH 0/9] MTRR fix trivial style patches

Index | Next | Previous | View Threaded

jaswinder at kernel

Jul 3, 2009, 7:18 PM

Post #1 of 3 (103 views)
Permalink

[GIT PULL -tip][PATCH 0/9] MTRR fix trivial style patches

On Fri, 2009-07-03 at 18:42 +0200, Ingo Molnar wrote:
> * Alan Cox <alan[at]lxorguk.ukuu.org.uk> wrote:
>
> > A good example of what happens if you don't is
> >
> > scripts/checkpatch.pl --file arch/x86/kernel/cpu/mtrr/*c
> >
> > where the core part of the code isn't changed very much even
> > though the interfaces completely get reworked. Thus it never gets
> > cleaned up and reviewed as a whole. The code works, will probably
> > work for years but never gets looked at as a whole and tidied.
>
> I think even the MTRR code (which is indeed one of the few x86
> places still not fully cleaned up) supports my arguments.
>
> Look at the averages:
>
> errors lines of code errors/KLOC
> arch/x86/kernel/cpu/mtrr/amd.c 2 120 16.6
> arch/x86/kernel/cpu/mtrr/centaur.c 8 225 35.5
> arch/x86/kernel/cpu/mtrr/cleanup.c 0 1102 0
> arch/x86/kernel/cpu/mtrr/cyrix.c 10 275 36.3
> arch/x86/kernel/cpu/mtrr/generic.c 16 730 21.9
> arch/x86/kernel/cpu/mtrr/if.c 11 428 25.7
> arch/x86/kernel/cpu/mtrr/main.c 50 751 66.5
> arch/x86/kernel/cpu/mtrr/state.c 0 83 0
>

By these trivial mtrr cleanup patches:
errors
arch/x86/kernel/cpu/mtrr/amd.c 0
arch/x86/kernel/cpu/mtrr/centaur.c 0
arch/x86/kernel/cpu/mtrr/cleanup.c 0
arch/x86/kernel/cpu/mtrr/cyrix.c 0
arch/x86/kernel/cpu/mtrr/generic.c 0
arch/x86/kernel/cpu/mtrr/if.c 0
arch/x86/kernel/cpu/mtrr/main.c 0
arch/x86/kernel/cpu/mtrr/state.c 0

The following changes since commit 2137f10fd78ce8540ec4305f4dcd559039444544:
Ingo Molnar (1):
Merge branch 'perfcounters/urgent'

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-top.git master

Jaswinder Singh Rajput (9):
x86: mtrr/amd.c fix trivial style problems
x86: mtrr/centaur.c fix style problems
x86: mtrr/cleanup.c fix trivial style problems
x86: mtrr/cyrix.c fix trivial style problems
x86: mtrr/generic.c fix style problems
x86: mtrr/if.c fix trivial style problems
x86: mtrr/mtrr.h fix trivial style problems
x86: mtrr/state.c fix trivial style problems
x86: mtrr/main.c fix style problems

arch/x86/kernel/cpu/mtrr/amd.c | 7 +-
arch/x86/kernel/cpu/mtrr/centaur.c | 127 +++----------------------
arch/x86/kernel/cpu/mtrr/cleanup.c | 18 ++--
arch/x86/kernel/cpu/mtrr/cyrix.c | 30 +++---
arch/x86/kernel/cpu/mtrr/generic.c | 147 +++++++++++++++-------------
arch/x86/kernel/cpu/mtrr/if.c | 51 +++++++----
arch/x86/kernel/cpu/mtrr/main.c | 185 +++++++++++++++++++-----------------
arch/x86/kernel/cpu/mtrr/mtrr.h | 7 +-
arch/x86/kernel/cpu/mtrr/state.c | 20 +++--
9 files changed, 263 insertions(+), 329 deletions(-)

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

mingo at elte

Jul 5, 2009, 11:21 AM

Post #2 of 3 (78 views)
Permalink

Re: [GIT PULL -tip][PATCH 0/9] MTRR fix trivial style patches [In reply to]

* Jaswinder Singh Rajput <jaswinder[at]kernel.org> wrote:

> > I think even the MTRR code (which is indeed one of the few x86
> > places still not fully cleaned up) supports my arguments.
> >
> > Look at the averages:
> >
> > errors lines of code errors/KLOC
> > arch/x86/kernel/cpu/mtrr/amd.c 2 120 16.6
> > arch/x86/kernel/cpu/mtrr/centaur.c 8 225 35.5
> > arch/x86/kernel/cpu/mtrr/cleanup.c 0 1102 0
> > arch/x86/kernel/cpu/mtrr/cyrix.c 10 275 36.3
> > arch/x86/kernel/cpu/mtrr/generic.c 16 730 21.9
> > arch/x86/kernel/cpu/mtrr/if.c 11 428 25.7
> > arch/x86/kernel/cpu/mtrr/main.c 50 751 66.5
> > arch/x86/kernel/cpu/mtrr/state.c 0 83 0
> >
>
> By these trivial mtrr cleanup patches:
> errors
> arch/x86/kernel/cpu/mtrr/amd.c 0
> arch/x86/kernel/cpu/mtrr/centaur.c 0
> arch/x86/kernel/cpu/mtrr/cleanup.c 0
> arch/x86/kernel/cpu/mtrr/cyrix.c 0
> arch/x86/kernel/cpu/mtrr/generic.c 0
> arch/x86/kernel/cpu/mtrr/if.c 0
> arch/x86/kernel/cpu/mtrr/main.c 0
> arch/x86/kernel/cpu/mtrr/state.c 0
>
> The following changes since commit 2137f10fd78ce8540ec4305f4dcd559039444544:
> Ingo Molnar (1):
> Merge branch 'perfcounters/urgent'
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-top.git master
>
> Jaswinder Singh Rajput (9):
> x86: mtrr/amd.c fix trivial style problems
> x86: mtrr/centaur.c fix style problems
> x86: mtrr/cleanup.c fix trivial style problems
> x86: mtrr/cyrix.c fix trivial style problems
> x86: mtrr/generic.c fix style problems
> x86: mtrr/if.c fix trivial style problems
> x86: mtrr/mtrr.h fix trivial style problems
> x86: mtrr/state.c fix trivial style problems
> x86: mtrr/main.c fix style problems
>
> arch/x86/kernel/cpu/mtrr/amd.c | 7 +-
> arch/x86/kernel/cpu/mtrr/centaur.c | 127 +++----------------------
> arch/x86/kernel/cpu/mtrr/cleanup.c | 18 ++--
> arch/x86/kernel/cpu/mtrr/cyrix.c | 30 +++---
> arch/x86/kernel/cpu/mtrr/generic.c | 147 +++++++++++++++-------------
> arch/x86/kernel/cpu/mtrr/if.c | 51 +++++++----
> arch/x86/kernel/cpu/mtrr/main.c | 185 +++++++++++++++++++-----------------
> arch/x86/kernel/cpu/mtrr/mtrr.h | 7 +-
> arch/x86/kernel/cpu/mtrr/state.c | 20 +++--
> 9 files changed, 263 insertions(+), 329 deletions(-)

Jaswinder, i'm going to ignore such pull requests from you in the
future, because your changes are exceedingly painful and you just
dont seem to learn.

This is the Nth incident, and there were a countless number of prior
incidents where i warned you about various classes of avoidable
problems, and you are not showing signs of significant improvements.

To get more specific: in this case too it's the same old problem of
low quality of patches from you again:

- You managed to put _3_ separate bugs into these 'trivial'
cleanups. That is not acceptable. If you cannot make trivial
patches be truly trivial, you should not be doing them really.

- i had to remove/undo/fix some bogus 'fixes' you did in those
patches where you just blindly followed checkpatch instead of
thinking about it for a minute. We dont want checkpatch warriors
- we want people with taste who use it as a tool to keep their
new patches tidy, or who use it as a tool to aid cleanups.

- i had to complete the patches and do all the cleanups you missed
- sometimes on the next time to a change you did. You apparently
only checked checkpatch output - you didnt even look at all the
files for how it all looks like and whether there are other
things in those files that checkpatch missed. You made the files
'checkpatch clean' - but you didnt actually look at whether the
files got cleaned up for good really. You left a half-done
jumbled mess behind you with files that still had inconsistent
looking style in them. Check the deltas of the patches i
committed versus the ones you sent to see the countless cases you
missed. And you cannot even claim to have done the 'trivial'
ones safely - because you did it in a buggy way and because the
final cleanups i did are all trivial too and can be validated.

- i had to double check each commit on the assembly level to prove
that the patches are true cleanups. The new commit logs, size
checks, md5 sums are proof of that due diligence process. You
obviously didnt do any of that - you'd have noticed the bugs you
have put in had you done that.

- i had to edit _all_ your changelogs. Again. You _still_ dont
think about your changelogs, they still suck 90% of the time.

All things considered it took me 6 hours to fix up and complete your
'trivial' patches into which you have put only 3 hours of work:

- your buggy and incomplete cleanups did:

9 files changed, 263 insertions(+), 329 deletions(-)

- the proper, fixed, rounded up, checked, validated and working
cleanups i committed with 6 extra hours of work:

9 files changed, 868 insertions(+), 862 deletions(-)

The MTRR code is a highly critical piece of x86 code and i had to
check assembly dumps manually and compared them to make sure the
changes dont introduce subtle regressions.

The fact is, there is no other way to establish the trust level of
trivial cleanups but to invest this depth of review and this level
of testing and checking. I cannot just review until i find the first
bug or problem and bounce it back to you as a review failure - i
cannot know whether i can trust your next version of the patch and i
cannot check the same trivial cleanups again and again as a machine,
until you manage to submit the correct one.

So i've tested the trustworthyness of your changes for a final time
and you failed this test.

I still committed it all under your name because i kept a fair
amount of your changes so it's all v2 versions of your original
patches - and per kernel convention i noted it in the changelog that
i modified it further (and i didnt want to create 9 more commits,
especially as some of your patches were broken so not bisection
safe) - but this was the last time i went through such an effort
with your patches.

As i warned you _repeatedly_ in the past that you should insert a
lot more effort into patches before sending any patches and before
sending any pull request. Other x86 maintainers warned you about
that too. You seem to prefer five patches a day (a few of which are
inevitably broken) instead of one good patch every five days.

This low level of patch quality just does not scale for maintainers
of a busy tree which has more than a hundred contributors in every
cycle. If i cannot trust a contributor i cannot apply such patches
directly.

All in one, you were making the same kinds of mistakes again and
again, over a many month time-span, and you are causing a
considerable amount of maintainer overhead that is just not
sustainable really.

So this simply does not work in this form. I can work with pretty
much anyone, but there's a final limit to the energy i can invest
into this. Please find some other Linux maintainer who can work with
you and who is willing to apply your patches. If you want to work on
x86 bits please find an x86 developer or maintainer who is willing
to apply your patches or who is willing to give you Reviewed-by tags
before sending them to me.

Thanks,

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

jaswinder at kernel

Jul 5, 2009, 3:09 PM

Post #3 of 3 (76 views)
Permalink

Re: [GIT PULL -tip][PATCH 0/9] MTRR fix trivial style patches [In reply to]

On Sun, 2009-07-05 at 20:21 +0200, Ingo Molnar wrote:
> * Jaswinder Singh Rajput <jaswinder[at]kernel.org> wrote:
>

> - You managed to put _3_ separate bugs into these 'trivial'
> cleanups. That is not acceptable. If you cannot make trivial
> patches be truly trivial, you should not be doing them really.
>

Where is the list of _3_ bugs. If there was bugs you should report to
me. If there are bugs show to me, why are you hiding the details.

So it is your mistake.

> - i had to remove/undo/fix some bogus 'fixes' you did in those
> patches where you just blindly followed checkpatch instead of
> thinking about it for a minute. We dont want checkpatch warriors
> - we want people with taste who use it as a tool to keep their
> new patches tidy, or who use it as a tool to aid cleanups.
>

If there was bogus 'fixes', then you should ignore that patch. Or let me
know I will fix that.

So it is your mistake.

> - i had to complete the patches and do all the cleanups you missed
> - sometimes on the next time to a change you did. You apparently
> only checked checkpatch output - you didnt even look at all the
> files for how it all looks like and whether there are other
> things in those files that checkpatch missed. You made the files
> 'checkpatch clean' - but you didnt actually look at whether the
> files got cleaned up for good really. You left a half-done
> jumbled mess behind you with files that still had inconsistent
> looking style in them. Check the deltas of the patches i
> committed versus the ones you sent to see the countless cases you
> missed. And you cannot even claim to have done the 'trivial'
> ones safely - because you did it in a buggy way and because the
> final cleanups i did are all trivial too and can be validated.
>

If you want more cleanup, you should do in separate patch, why you keep
on changing my patch.

This is a never ending task I can do further clean-ups on your patches
also.

So again it is your mistake.

> - i had to double check each commit on the assembly level to prove
> that the patches are true cleanups. The new commit logs, size
> checks, md5 sums are proof of that due diligence process. You
> obviously didnt do any of that - you'd have noticed the bugs you
> have put in had you done that.
>

Why you did this, you never told me to do so.

So again it is your mistake.

> - i had to edit _all_ your changelogs. Again. You _still_ dont
> think about your changelogs, they still suck 90% of the time.
>

Again this is never ending task, even Linus and Andrew can further
change changelog made by you, it is a personnel flavor.

> All things considered it took me 6 hours to fix up and complete your
> 'trivial' patches into which you have put only 3 hours of work:
>
> - your buggy and incomplete cleanups did:
>
> 9 files changed, 263 insertions(+), 329 deletions(-)
>
> - the proper, fixed, rounded up, checked, validated and working
> cleanups i committed with 6 extra hours of work:
>
> 9 files changed, 868 insertions(+), 862 deletions(-)
>
> The MTRR code is a highly critical piece of x86 code and i had to
> check assembly dumps manually and compared them to make sure the
> changes dont introduce subtle regressions.
>

If you want to do it, you should do in separate patches.

It is again your mistake.

> The fact is, there is no other way to establish the trust level of
> trivial cleanups but to invest this depth of review and this level
> of testing and checking. I cannot just review until i find the first
> bug or problem and bounce it back to you as a review failure - i
> cannot know whether i can trust your next version of the patch and i
> cannot check the same trivial cleanups again and again as a machine,
> until you manage to submit the correct one.
>
> So i've tested the trustworthyness of your changes for a final time
> and you failed this test.
>

You never told me to test in this manner.

> I still committed it all under your name because i kept a fair
> amount of your changes so it's all v2 versions of your original
> patches - and per kernel convention i noted it in the changelog that
> i modified it further (and i didnt want to create 9 more commits,
> especially as some of your patches were broken so not bisection
> safe) - but this was the last time i went through such an effort
> with your patches.
>

Why you committed under my name, I do not need your such help if you
again shout at me.

If there was issue then you should ignore my patches and start from
scratch.

This is again your mistake.

> As i warned you _repeatedly_ in the past that you should insert a
> lot more effort into patches before sending any patches and before
> sending any pull request. Other x86 maintainers warned you about
> that too. You seem to prefer five patches a day (a few of which are
> inevitably broken) instead of one good patch every five days.
>
> This low level of patch quality just does not scale for maintainers
> of a busy tree which has more than a hundred contributors in every
> cycle. If i cannot trust a contributor i cannot apply such patches
> directly.
>
> All in one, you were making the same kinds of mistakes again and
> again, over a many month time-span, and you are causing a
> considerable amount of maintainer overhead that is just not
> sustainable really.
>

If I can spend 10-16 hours in a day and no one is paying to me, I am
spending from my pocket. And I send 5 patches in a day.

For you it will not take more than 5-10 minutes to review my 5 patches.

This is your job, and your are getting salary for it.

You should be thankful to me instead of blaming me.

So again it is your fault.

> So this simply does not work in this form. I can work with pretty
> much anyone, but there's a final limit to the energy i can invest
> into this. Please find some other Linux maintainer who can work with
> you and who is willing to apply your patches. If you want to work on
> x86 bits please find an x86 developer or maintainer who is willing
> to apply your patches or who is willing to give you Reviewed-by tags
> before sending them to me.
>

So you are blaming me for your faults.

I am contributing to open-source from my pocket, because I thought that
open-source people are open-heart (big heart), but now it seems I was
wrong. You are having very little heart and very low tolerance.

Thanks,
--
JSR

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

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact lists@gossamer-threads.com