[RFC][PATCH] kmemcheck: divide and conquer

[RFC][PATCH] kmemcheck: divide and conquer

[Vegard Nossum]

Hi,

I've split the main x86/mm/kmemcheck.c file and made a subdirectory of many
files instead.

I have included the diffstat below, but I think the patch itself is too big
for the mailing list. It can instead be viewed at:

http://www.kernel.org/pub/linux/kernel/people/vegard/patches/0001-kmemcheck-divide-and-conquer.patch

The RFC part: Is this a good thing to do? I personally hate the 4000-line
files that are so commonly found in the kernel, and therefore prefer this
split-up. On the other hand, C lacks namespaces, which sometimes leads to
some really long and ugly names just to prevent clashes in the future. But
it's your call, I'll just do whatever it takes to get in... ;-)


Vegard


Author: Vegard Nossum <vegard.nossum@gmail.com>
Date:   Wed Jun 11 12:37:13 2008 +0200

    kmemcheck: divide and conquer

    This patch splits the kmemcheck.c into a subdirectory of many files. This
    makes it easier to get around the different functions because they're now
    grouped together by file.

    Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>

 arch/x86/mm/Makefile              |    2 +-
 arch/x86/mm/kmemcheck.c           | 1100 -------------------------------------
 arch/x86/mm/kmemcheck/Makefile    |    3 +
 arch/x86/mm/kmemcheck/error.c     |  215 ++++++++
 arch/x86/mm/kmemcheck/error.h     |   15 +
 arch/x86/mm/kmemcheck/kmemcheck.c |  477 ++++++++++++++++
 arch/x86/mm/kmemcheck/opcode.c    |   70 +++
 arch/x86/mm/kmemcheck/opcode.h    |    9 +
 arch/x86/mm/kmemcheck/pte.c       |   22 +
 arch/x86/mm/kmemcheck/pte.h       |   10 +
 arch/x86/mm/kmemcheck/shadow.c    |  174 ++++++
 arch/x86/mm/kmemcheck/shadow.h    |   16 +
 arch/x86/mm/kmemcheck/smp.c       |   79 +++
 arch/x86/mm/kmemcheck/smp.h       |   23 +
 arch/x86/mm/kmemcheck/string.c    |   91 +++
 include/linux/kmemcheck.h         |    3 +
 16 files changed, 1208 insertions(+), 1101 deletions(-)

Re: [RFC][PATCH] kmemcheck: divide and conquer

[Ingo Molnar]

* Vegard Nossum <vegard.nossum@gmail.com> wrote:

> Hi,
> 
> I've split the main x86/mm/kmemcheck.c file and made a subdirectory of 
> many files instead.
> 
> I have included the diffstat below, but I think the patch itself is 
> too big for the mailing list. It can instead be viewed at:
> 
> http://www.kernel.org/pub/linux/kernel/people/vegard/patches/0001-kmemcheck-divide-and-conquer.patch
> 
> The RFC part: Is this a good thing to do? I personally hate the 
> 4000-line files that are so commonly found in the kernel, and 
> therefore prefer this split-up. On the other hand, C lacks namespaces, 
> which sometimes leads to some really long and ugly names just to 
> prevent clashes in the future. But it's your call, I'll just do 
> whatever it takes to get in... ;-)

it's a very nice splitup! :-) [ Any Git coordinates to pick it up? ]

such a splitup opens up for future enhancements such as the sharing of 
opcode decoding between kmemcheck, mmiotrace and KVM. It also makes the 
code easier to maintain as there's less risk of patch merge conflicts. 
And not the least, it's easier to read as well if it's split up into 
logical modules.

	Ingo

Re: [RFC][PATCH] kmemcheck: divide and conquer

[Vegard Nossum]

On Sat, Jun 14, 2008 at 8:31 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Vegard Nossum <vegard.nossum@gmail.com> wrote:
>> The RFC part: Is this a good thing to do? I personally hate the
>> 4000-line files that are so commonly found in the kernel, and
>> therefore prefer this split-up. On the other hand, C lacks namespaces,
>> which sometimes leads to some really long and ugly names just to
>> prevent clashes in the future. But it's your call, I'll just do
>> whatever it takes to get in... ;-)
>
> it's a very nice splitup! :-) [ Any Git coordinates to pick it up? ]

Ah, thanks, that's just what I wanted to hear! :-D

This particular commit can be found in the "against-v2.6.26-rc4"
branch of kmemcheck.git:

http://git.kernel.org/?p=linux/kernel/git/vegard/kmemcheck.git;a=commit;h=8c67b9b8005a518ce003943a64c37a9fc2c84485

But for the newer versions, I've merged it into the "kmemcheck core"
commit. To pick up the whole series, you can either check out the
against-v2.6.26-rc5 branch (which is frozen), or you can check out the
"current" branch which is the most recent rebase (against v2.6.26-rc6,
but not yet frozen):

$ git remote add kmemcheck
git://git.kernel.org/pub/scm/linux/kernel/git/vegard/kmemcheck.git
$ git fetch kmemcheck
$ git checkout kmemcheck/current

Just a small disclaimer: It's not VERY well tested, so build breakage
may occur with this shuffle. It really shouldn't, though.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [RFC][PATCH] kmemcheck: divide and conquer

[Ingo Molnar]

* Vegard Nossum <vegard.nossum@gmail.com> wrote:

> http://git.kernel.org/?p=linux/kernel/git/vegard/kmemcheck.git;a=commit;h=8c67b9b8005a518ce003943a64c37a9fc2c84485
> 
> But for the newer versions, I've merged it into the "kmemcheck core" 
> commit. To pick up the whole series, you can either check out the 
> against-v2.6.26-rc5 branch (which is frozen), or you can check out the 
> "current" branch which is the most recent rebase (against v2.6.26-rc6, 
> but not yet frozen):

a small workflow request: could you please start adding append-only 
commits to that tree, if possible? For example right now as it looks the 
kmemcheck tree says:

 commit 385e31b9eae0528bada07d16a189f3f40df23961
 Author: Vegard Nossum <vegard.nossum@gmail.com>
 Date:   Fri Apr 4 00:51:41 2008 +0200

     kmemcheck: add the kmemcheck core

i had to look around for a few minutes to make sure i got the right 
branch until i realized that "Apr 4" is not the correct date - this 
commit got updated by you just today.

it would be much better if you stopped rebasing kmemcheck from now on, 
and did append-only updates only - that way i could pick up your updates 
into tip/kmemcheck by doing pulls. It does not matter if the result 
looks a bit messier - most of the fundamentals should be in place 
already.

	Ingo

Re: [RFC][PATCH] kmemcheck: divide and conquer

[Vegard Nossum]

On Sat, Jun 14, 2008 at 11:00 AM, Ingo Molnar <mingo@elte.hu> wrote:
> a small workflow request: could you please start adding append-only
> commits to that tree, if possible?

Yes, I will.

> it would be much better if you stopped rebasing kmemcheck from now on,
> and did append-only updates only - that way i could pick up your updates
> into tip/kmemcheck by doing pulls. It does not matter if the result
> looks a bit messier - most of the fundamentals should be in place
> already.

I've created the "for-tip" branch which is (for now) just a copy of
the current "current" branch. This branch ("for-tip") will henceforth
NEVER be rebased.

I won't guarantee this for the other branches, though; rebasing is
really handy when the patch series is to be reviewed since it cuts
down noise to the bare minimum, and avoids multiple changes to the
same area of code (which is confusing to reviewers).

I think I will append new commits to the "for-tip" branch, git-pull
new kernel -rc releases, and rebase the result into a new branch (e.g.
against-v2.6.26*). Now we have two heads which _should_ have exactly
the same content, but where one retains a strictly incremental
history, and the other is easy to read for reviewers. (And comparing
them for differences is trivial; that's just a git-diff.)


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [RFC][PATCH] kmemcheck: divide and conquer

[Ingo Molnar]

-tip testing found a build failure with kmemcheck:

include/asm/string_32.h:320:1: warning: "memset" redefined
include/asm/string_32.h:245:1: warning: this is the location of the previous definition

arch/x86/mm/kmemcheck/smp.c: In function 'kmemcheck_pause_allbutself':
arch/x86/mm/kmemcheck/smp.c:59: error: 'NMI_VECTOR' undeclared (first use in this function)
arch/x86/mm/kmemcheck/smp.c:59: error: (Each undeclared identifier is reported only once
arch/x86/mm/kmemcheck/smp.c:59: error: for each function it appears in.)

with this config:

  http://redhat.com/~mingo/misc/config-Mon_Jun_16_04_15_22_CEST_2008.bad

the patch below fixes the build failure, but note that there are lots of 
those memset redefined warnings with that config as well.

	Ingo

------------>
commit 36bdad27f868eba1d17e6d9dc1cbb6794361d541
Author: Ingo Molnar <mingo@elte.hu>
Date:   Mon Jun 16 07:11:37 2008 +0200

    kmemcheck: fix build error
    
    fix:
    
    arch/x86/mm/kmemcheck/smp.c: In function 'kmemcheck_pause_allbutself':
    arch/x86/mm/kmemcheck/smp.c:59: error: 'NMI_VECTOR' undeclared (first use in this function)
    arch/x86/mm/kmemcheck/smp.c:59: error: (Each undeclared identifier is reported only once
    arch/x86/mm/kmemcheck/smp.c:59: error: for each function it appears in.)
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/arch/x86/mm/kmemcheck/smp.c b/arch/x86/mm/kmemcheck/smp.c
index c4ff615..cd17ddf 100644
--- a/arch/x86/mm/kmemcheck/smp.c
+++ b/arch/x86/mm/kmemcheck/smp.c
@@ -5,6 +5,7 @@
 #include <mach_ipi.h>
 
 #include "smp.h"
+#include <asm/irq_vectors.h>
 
 static spinlock_t nmi_spinlock;
 

Re: [RFC][PATCH] kmemcheck: divide and conquer

[Vegard Nossum]

On Mon, Jun 16, 2008 at 7:15 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> -tip testing found a build failure with kmemcheck:
>
> include/asm/string_32.h:320:1: warning: "memset" redefined
> include/asm/string_32.h:245:1: warning: this is the location of the previous definition
>
> arch/x86/mm/kmemcheck/smp.c: In function 'kmemcheck_pause_allbutself':
> arch/x86/mm/kmemcheck/smp.c:59: error: 'NMI_VECTOR' undeclared (first use in this function)
> arch/x86/mm/kmemcheck/smp.c:59: error: (Each undeclared identifier is reported only once
> arch/x86/mm/kmemcheck/smp.c:59: error: for each function it appears in.)
>
> with this config:
>
>  http://redhat.com/~mingo/misc/config-Mon_Jun_16_04_15_22_CEST_2008.bad
>
> the patch below fixes the build failure, but note that there are lots of
> those memset redefined warnings with that config as well.

Thanks. I get the error with that config as well.

> @@ -5,6 +5,7 @@
>  #include <mach_ipi.h>
>
>  #include "smp.h"
> +#include <asm/irq_vectors.h>
>
>  static spinlock_t nmi_spinlock;

However, the real error is that I'm including <mach_ipi.h> which for
the default config resolves to
include/asm-x86/mach-default/mach_ipi.h. All the mach-*/mach_ipi.h
files exist, but only one of them define NMI_VECTOR.

But your config has CONFIG_X86_BIGSMP=y.

For some reason, there is no <asm/irq_vectors.h> in my repository, so
I assume the change came from another branch. In fact, I should have
included <asm/hw_irq.h>, but that doesn't contain the right
definitions in the tip/auto-test branch.

Yep, seems to be

commit 9b7dc567d03d74a1fbae84e88949b6a60d922d82
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Fri May 2 20:10:09 2008 +0200

    x86: unify interrupt vector defines

I also can't seem to get the redefined warnings, so I assume that's
also just an indirect conflict with other -tip changes.

Right now, I am reluctant to apply your fix because it means that
kmemcheck tree won't build as it is. In general, what's the way to
resolve these things? Do you have another branch, *-fixes, where these
fixlets can go until either of the conflicting changesets are merged
upstream? If so, it seems that that would be the right place for this
patch. Do you agree or do you have another solution? :-)

Thanks!


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [RFC][PATCH] kmemcheck: divide and conquer

[Ingo Molnar]

* Vegard Nossum <vegard.nossum@gmail.com> wrote:

> For some reason, there is no <asm/irq_vectors.h> in my repository, so 
> I assume the change came from another branch. In fact, I should have 
> included <asm/hw_irq.h>, but that doesn't contain the right 
> definitions in the tip/auto-test branch.
> 
> Yep, seems to be
> 
> commit 9b7dc567d03d74a1fbae84e88949b6a60d922d82
> Author: Thomas Gleixner <tglx@linutronix.de>
> Date:   Fri May 2 20:10:09 2008 +0200
> 
>     x86: unify interrupt vector defines
> 
> I also can't seem to get the redefined warnings, so I assume that's
> also just an indirect conflict with other -tip changes.
> 
> Right now, I am reluctant to apply your fix because it means that 
> kmemcheck tree won't build as it is. In general, what's the way to 
> resolve these things? Do you have another branch, *-fixes, where these 
> fixlets can go until either of the conflicting changesets are merged 
> upstream? If so, it seems that that would be the right place for this 
> patch. Do you agree or do you have another solution? :-)

yes, i solved it the following "Git way": i did a --no-commit merge of 
the tip/x86/irq tree into the tip/kmemcheck2 branch and then 
"git-cherry-pick --no-commit"-ed the fix and thus made it a part of that 
merge commit. This way the build failure is never visible during 
bisection either.

tip/kmemcheck2 is a temporary topic tree: it contains the pull from your 
tree and i'm testing it at the moment. It will be renamed to 
tip/kmemcheck once it has passed testing.

btw., due to kmemcheck not being rebased anymore, this will work out 
just fine in the future: i can pull fixes from your tree into 
tip/kmemcheck and it will just all get sorted out by Git, despite the 
cross-merge to tip/x86/irq.

If you rebased kmemcheck i'd have to do rather difficult (and 
error-prone and harder to trust) manual merges every time you put a 
kmemcheck fix into your tree, and any breakages introduced by new 
kmemcheck fixes would not be easily bisectable either.

	Ingo

Re: [RFC][PATCH] kmemcheck: divide and conquer

[Vegard Nossum]

On Mon, Jun 16, 2008 at 9:10 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Vegard Nossum <vegard.nossum@gmail.com> wrote:
>>
>> Right now, I am reluctant to apply your fix because it means that
>> kmemcheck tree won't build as it is. In general, what's the way to
>> resolve these things? Do you have another branch, *-fixes, where these
>> fixlets can go until either of the conflicting changesets are merged
>> upstream? If so, it seems that that would be the right place for this
>> patch. Do you agree or do you have another solution? :-)
>
> yes, i solved it the following "Git way": i did a --no-commit merge of
> the tip/x86/irq tree into the tip/kmemcheck2 branch and then
> "git-cherry-pick --no-commit"-ed the fix and thus made it a part of that
> merge commit. This way the build failure is never visible during
> bisection either.

Aha. This means that I don't need to do anything but ack your fix.
Sounds good to me, thanks!

(I guess I will still need to check out -tip to find the source of the
memset redefinition warning.)


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036