LinuxLists.cc - [git-pull -tip] x86: include inverse Xmas tree patches

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sat, 2009-03-28 at 18:58 +0300, Alexey Dobriyan wrote:
> On Sat, Mar 28, 2009 at 03:55:49PM +0000, Alan Cox wrote:
> > > kenel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-Xmas.git x86/core
> > > >
> > > > Jaswinder Singh Rajput (49):
> > > > x86: process_32.c include inverse Xmas tree effect
> > >
> > > What the heck is this?
> >
> > Does inverse christmas tree effect not translate well. It could perhaps
> > have been more clearly explained.
> >
> > Getting the includes under control is a good thing and saves everyone
> > time.
>
> Moving #includes up and down is not getting them under control.

Here is Ingo's response about this effect:

On Sun, 2009-03-15 at 05:54 +0100, Ingo Molnar wrote:
> * Li Zefan <[email protected]> wrote:
>
> > > #include <linux/debugfs.h>
> > > #include <linux/uaccess.h>
> > > -#include <linux/stat.h>
> > > +#include <linux/module.h>
> > > #include <linux/init.h>
> > > +#include <linux/stat.h>
> > > #include <linux/io.h>
> > > #include <linux/mm.h>
> > > -#include <linux/module.h>
> > >
> >
> > Just curious about the rule to sort those includes, and why they need
> > to be rearranged.
>
> Such includes (the 'reverse christmas tree'):
>
> #include <linux/interrupt.h>
> #include <linux/mmiotrace.h>
> #include <linux/bootmem.h>
> #include <linux/compiler.h>
> #include <linux/highmem.h>
> #include <linux/kprobes.h>
> #include <linux/uaccess.h>
> #include <linux/vmalloc.h>
> #include <linux/vt_kern.h>
> #include <linux/signal.h>
> #include <linux/kernel.h>
> #include <linux/ptrace.h>
> #include <linux/string.h>
> #include <linux/module.h>
> #include <linux/kdebug.h>
> #include <linux/errno.h>
> #include <linux/magic.h>
> #include <linux/sched.h>
> #include <linux/types.h>
> #include <linux/init.h>
> #include <linux/mman.h>
> #include <linux/tty.h>
> #include <linux/smp.h>
> #include <linux/mm.h>
>
> #include <asm/kmemcheck.h>
> #include <asm/tlbflush.h>
> #include <asm/pgalloc.h>
> #include <asm/segment.h>
> #include <asm/system.h>
> #include <asm/proto.h>
> #include <asm/traps.h>
> #include <asm/desc.h>
>
> are used by x86 architecture code (and some other subsystems) to
> reduce the likelyhood of patch conflicts in commonly modified
> kernel files.
>
> Without such ordering developers typically append to the
> existing list of include files when introducing a new header -
> creating an almost certain patch conflict. Via the above
> ordering, new headers get distributed roughly evenly amongst the
> full range - and thus the chance of patch conflicts is much
> smaller.
>
> This way it also looks a bit more structured and bit less messy.
> It looks unprofessional and sloppy if a .c file starts with a
> big block of thrown-together include files.
>
> Ingo

2009-03-28 16:24:25

by Alan

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

> kenel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-Xmas.git x86/core
> >
> > Jaswinder Singh Rajput (49):
> > x86: process_32.c include inverse Xmas tree effect
>
> What the heck is this?

Does inverse christmas tree effect not translate well. It could perhaps
have been more clearly explained.

Getting the includes under control is a good thing and saves everyone
time.

Alan

2009-03-28 16:49:16

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sat, 2009-03-28 at 16:07 +0100, Joerg Roedel wrote:
> On Sat, Mar 28, 2009 at 08:18:50PM +0530, Jaswinder Singh Rajput wrote:
> > The following changes since commit 29219683c46cb89edf5c58418b5305b14646d030:
> > Ingo Molnar (1):
> > Merge branches 'x86/apic', 'x86/cleanups', 'x86/mm', 'x86/pat', 'x86/setup' and 'x86/signal'; commit 'v2.6.29' into x86/core
> >
> > are available in the git repository at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-Xmas.git x86/core
> >
> > Jaswinder Singh Rajput (49):
> > x86: process_32.c include inverse Xmas tree effect
> > x86: signal.c include inverse Xmas tree effect
> > x86: entry_32.S include inverse Xmas tree effect
> > x86: traps.c include inverse Xmas tree effect
> > x86: irq.c include inverse Xmas tree effect
> > x86: irq_32.c include inverse Xmas tree effect
> > x86: dumpstack.c include inverse Xmas tree effect
> > x86: time_32.c include inverse Xmas tree effect
> > x86: ioport.c include inverse Xmas tree effect
> > x86: ldt.c include inverse Xmas tree effect
> > x86: dumpstack_32.c include inverse Xmas tree effect
> > x86: setup.c include inverse Xmas tree effect
> > x86: i8259.c include inverse Xmas tree effect
> > x86: irqinit_32.c include inverse Xmas tree effect
> > x86: probe_roms_32.c include inverse Xmas tree effect
> > x86: sys_i386_32.c include inverse Xmas tree effect
> > x86: i386_ksyms_32.c include inverse Xmas tree effect
> > x86: bootflag.c include inverse Xmas tree effect
> > x86: e820.c include inverse Xmas tree effect
> > x86: pci-dma.c include inverse Xmas tree effect
> > x86: quirks.c include inverse Xmas tree effect
> > x86: i8237.c include inverse Xmas tree effect
> > x86: alternative.c include inverse Xmas tree effect
> > x86: tsc.c include inverse Xmas tree effect
> > x86: process.c include inverse Xmas tree effect
> > x86: xsave.c include inverse Xmas tree effect
> > x86: ptrace.c include inverse Xmas tree effect
> > x86: ds.c include inverse Xmas tree effect
> > x86: tls.c include inverse Xmas tree effect
> > x86: step.c include inverse Xmas tree effect
> > x86: stacktrace.c include inverse Xmas tree effect
> > x86: reboot.c include inverse Xmas tree effect
> > x86: msr.c include inverse Xmas tree effect
> > x86: cpuid.c include inverse Xmas tree effect
> > x86: early-quirks.c include inverse Xmas tree effect
> > x86: smp.c include inverse Xmas tree effect
> > x86: smpboot.c include inverse Xmas tree effect
> > x86: tsc_sync.c include inverse Xmas tree effect
> > x86: setup_percpu.c include inverse Xmas tree effect
> > x86: trampoline_32.S include inverse Xmas tree effect
> > x86: mpparse.c include inverse Xmas tree effect
> > x86: module_32.c include inverse Xmas tree effect
> > x86: doublefault_32.c include inverse Xmas tree effect
> > x86: vm86_32.c include inverse Xmas tree effect
> > ix86: early_printk.c include inverse Xmas tree effect
> > x86: pcspeaker.c include inverse Xmas tree effect
> > x86: head_32.c include inverse Xmas tree effect
> > x86: head.c include inverse Xmas tree effect
> > x86: init_task.c include inverse Xmas tree effect
>
> Where have you sent these patches for review?
>

-tip's master.

> > Complete diff:
>
> No. Complete diffs are not easy to review. At least not in this size.

46 of 49 are clean patches, and only 3 are doubtful which you pointed. I
am attaching those patches for feedback.

>
> > diff --git a/arch/x86/include/asm/proto.h b/arch/x86/include/asm/proto.h
> > index 49fb3ec..a8d63be 100644
> > --- a/arch/x86/include/asm/proto.h
> > +++ b/arch/x86/include/asm/proto.h
> > @@ -7,7 +7,9 @@
> >
> > extern void early_idt_handler(void);
> >
> > +#ifdef CONFIG_X86_64
> > extern void system_call(void);
> > +#endif
> > extern void syscall_init(void);
> >
> > extern void ia32_syscall(void);
> > diff --git a/arch/x86/include/asm/sigcontext32.h b/arch/x86/include/asm/sigcontext32.h
> > index ad1478c..f80df2e 100644
> > --- a/arch/x86/include/asm/sigcontext32.h
> > +++ b/arch/x86/include/asm/sigcontext32.h
> > @@ -1,6 +1,7 @@
> > #ifndef _ASM_X86_SIGCONTEXT32_H
> > #define _ASM_X86_SIGCONTEXT32_H
> >
> > +#ifdef CONFIG_IA32_EMULATION
> > #include <linux/types.h>
> >
> > /* signal context for 32bit programs. */
> > @@ -73,5 +74,5 @@ struct sigcontext_ia32 {
> > unsigned int oldmask;
> > unsigned int cr2;
> > };
> > -
> > +#endif
> > #endif /* _ASM_X86_SIGCONTEXT32_H */
>
> This doesn't seem to be related to your header include rearangements.
>

Please check this patch then you will find the relation:

From: Jaswinder Singh Rajput <[email protected]>
Date: Sat, 28 Mar 2009 16:02:42 +0530
Subject: [PATCH] x86: xsave.c include inverse Xmas tree effect

Impact: Xmas tree effect

Signed-off-by: Jaswinder Singh Rajput <[email protected]>
---
arch/x86/include/asm/sigcontext32.h | 3 ++-
arch/x86/kernel/xsave.c | 5 ++---
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/sigcontext32.h b/arch/x86/include/asm/sigcontext32.h
index ad1478c..f80df2e 100644
--- a/arch/x86/include/asm/sigcontext32.h
+++ b/arch/x86/include/asm/sigcontext32.h
@@ -1,6 +1,7 @@
#ifndef _ASM_X86_SIGCONTEXT32_H
#define _ASM_X86_SIGCONTEXT32_H

+#ifdef CONFIG_IA32_EMULATION
#include <linux/types.h>

/* signal context for 32bit programs. */
@@ -73,5 +74,5 @@ struct sigcontext_ia32 {
unsigned int oldmask;
unsigned int cr2;
};
-
+#endif
#endif /* _ASM_X86_SIGCONTEXT32_H */
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 2b54fe0..778ed67 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -3,12 +3,11 @@
*
* Author: Suresh Siddha <[email protected]>
*/
+
+#include <asm/sigcontext32.h>
#include <linux/bootmem.h>
#include <linux/compat.h>
#include <asm/i387.h>
-#ifdef CONFIG_IA32_EMULATION
-#include <asm/sigcontext32.h>
-#endif
#include <asm/xcr.h>

/*
--
1.6.0.6

> > diff --git a/arch/x86/include/asm/smpboot_hooks.h b/arch/x86/include/asm/smpboot_hooks.h
> > index 1def601..dff6e11 100644
> > --- a/arch/x86/include/asm/smpboot_hooks.h
> > +++ b/arch/x86/include/asm/smpboot_hooks.h
> > @@ -1,6 +1,8 @@
> > /* two abstractions specific to kernel/smpboot.c, mainly to cater to visws
> > * which needs to alter them. */
> >
> > +#include <asm/tlbflush.h>
> > +
> > static inline void smpboot_clear_io_apic_irqs(void)
> > {
> > #ifdef CONFIG_X86_IO_APIC

From: Jaswinder Singh Rajput <[email protected]>
Date: Sat, 28 Mar 2009 16:46:42 +0530
Subject: [PATCH] x86: smpboot.c include inverse Xmas tree effect

Impact: Xmas tree effect

Signed-off-by: Jaswinder Singh Rajput <[email protected]>
---
arch/x86/include/asm/smpboot_hooks.h | 2 ++
arch/x86/kernel/smpboot.c | 33 ++++++++++++++++-----------------
2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/smpboot_hooks.h b/arch/x86/include/asm/smpboot_hooks.h
index 1def601..dff6e11 100644
--- a/arch/x86/include/asm/smpboot_hooks.h
+++ b/arch/x86/include/asm/smpboot_hooks.h
@@ -1,6 +1,8 @@
/* two abstractions specific to kernel/smpboot.c, mainly to cater to visws
* which needs to alter them. */

+#include <asm/tlbflush.h>
+
static inline void smpboot_clear_io_apic_irqs(void)
{
#ifdef CONFIG_X86_IO_APIC
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ef7d101..60f6dde 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -39,33 +39,32 @@
* Glauber Costa : i386 and x86_64 integration
*/

-#include <linux/init.h>
-#include <linux/smp.h>
+#include <linux/mc146818rtc.h>
+#include <linux/bootmem.h>
#include <linux/module.h>
-#include <linux/sched.h>
#include <linux/percpu.h>
-#include <linux/bootmem.h>
+#include <linux/sched.h>
+#include <linux/init.h>
#include <linux/err.h>
#include <linux/nmi.h>
+#include <linux/smp.h>

+#include <asm/smpboot_hooks.h>
+#include <asm/trampoline.h>
+#include <asm/tlbflush.h>
+#include <asm/pgtable.h>
+#include <asm/setup.h>
+#include <asm/uv/uv.h>
#include <asm/acpi.h>
+#include <asm/apic.h>
#include <asm/desc.h>
-#include <asm/nmi.h>
-#include <asm/irq.h>
#include <asm/idle.h>
-#include <asm/trampoline.h>
-#include <asm/cpu.h>
-#include <asm/numa.h>
-#include <asm/pgtable.h>
-#include <asm/tlbflush.h>
#include <asm/mtrr.h>
+#include <asm/numa.h>
+#include <asm/cpu.h>
+#include <asm/irq.h>
+#include <asm/nmi.h>
#include <asm/vmi.h>
-#include <asm/apic.h>
-#include <asm/setup.h>
-#include <asm/uv/uv.h>
-#include <linux/mc146818rtc.h>
-
-#include <asm/smpboot_hooks.h>

#ifdef CONFIG_X86_32
u8 apicid_2_node[MAX_APICID];
--
1.6.0.6

> > diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
> > index 77cfb2c..1f23e58 100644
> > --- a/arch/x86/include/asm/topology.h
> > +++ b/arch/x86/include/asm/topology.h
> > @@ -194,10 +194,12 @@ extern int __node_distance(int, int);
> >
> > #else /* !CONFIG_NUMA */
> >
> > +#if 0
> > static inline int numa_node_id(void)
> > {
> > return 0;
> > }
> > +#endif
>
> "#if 0" ???
>

Hmm, this is really funny and doubtful. numa_node_id is already defined.
Rusty :-)

From: Jaswinder Singh Rajput <[email protected]>
Date: Sat, 28 Mar 2009 17:11:43 +0530
Subject: [PATCH] x86: setup_percpu.c include inverse Xmas tree effect

Impact: Xmas tree effect

Signed-off-by: Jaswinder Singh Rajput <[email protected]>
---
arch/x86/include/asm/topology.h | 2 ++
arch/x86/kernel/setup_percpu.c | 21 +++++++++++----------
2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index 77cfb2c..1f23e58 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -194,10 +194,12 @@ extern int __node_distance(int, int);

#else /* !CONFIG_NUMA */

+#if 0
static inline int numa_node_id(void)
{
return 0;
}
+#endif

static inline int cpu_to_node(int cpu)
{
diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index 400331b..85e84b4 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -1,23 +1,24 @@
+#include <linux/crash_dump.h>
+#include <linux/topology.h>
+#include <linux/bootmem.h>
#include <linux/kernel.h>
#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/bootmem.h>
#include <linux/percpu.h>
#include <linux/kexec.h>
-#include <linux/crash_dump.h>
-#include <linux/smp.h>
-#include <linux/topology.h>
+#include <linux/init.h>
#include <linux/pfn.h>
-#include <asm/sections.h>
+#include <linux/smp.h>
+
+#include <asm/stackprotector.h>
#include <asm/processor.h>
-#include <asm/setup.h>
-#include <asm/mpspec.h>
+#include <asm/sections.h>
#include <asm/apicdef.h>
+#include <asm/cpumask.h>
#include <asm/highmem.h>
+#include <asm/mpspec.h>
#include <asm/proto.h>
-#include <asm/cpumask.h>
+#include <asm/setup.h>
#include <asm/cpu.h>
-#include <asm/stackprotector.h>

#ifdef CONFIG_DEBUG_PER_CPU_MAPS
# define DBG(x...) printk(KERN_DEBUG x)
--
1.6.0.6

--
JSR

2009-03-28 17:17:20

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sat, 2009-03-28 at 22:18 +0530, Jaswinder Singh Rajput wrote:
> On Sat, 2009-03-28 at 16:07 +0100, Joerg Roedel wrote:
> > On Sat, Mar 28, 2009 at 08:18:50PM +0530, Jaswinder Singh Rajput wrote:
> > > The following changes since commit 29219683c46cb89edf5c58418b5305b14646d030:
> > > Ingo Molnar (1):
> > > Merge branches 'x86/apic', 'x86/cleanups', 'x86/mm', 'x86/pat', 'x86/setup' and 'x86/signal'; commit 'v2.6.29' into x86/core
> > >
> > > are available in the git repository at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-Xmas.git x86/core
> > >
> > > Jaswinder Singh Rajput (49):
> > > x86: process_32.c include inverse Xmas tree effect
> > > x86: signal.c include inverse Xmas tree effect
> > > x86: entry_32.S include inverse Xmas tree effect
> > > x86: traps.c include inverse Xmas tree effect
> > > x86: irq.c include inverse Xmas tree effect
> > > x86: irq_32.c include inverse Xmas tree effect
> > > x86: dumpstack.c include inverse Xmas tree effect
> > > x86: time_32.c include inverse Xmas tree effect
> > > x86: ioport.c include inverse Xmas tree effect
> > > x86: ldt.c include inverse Xmas tree effect
> > > x86: dumpstack_32.c include inverse Xmas tree effect
> > > x86: setup.c include inverse Xmas tree effect
> > > x86: i8259.c include inverse Xmas tree effect
> > > x86: irqinit_32.c include inverse Xmas tree effect
> > > x86: probe_roms_32.c include inverse Xmas tree effect
> > > x86: sys_i386_32.c include inverse Xmas tree effect
> > > x86: i386_ksyms_32.c include inverse Xmas tree effect
> > > x86: bootflag.c include inverse Xmas tree effect
> > > x86: e820.c include inverse Xmas tree effect
> > > x86: pci-dma.c include inverse Xmas tree effect
> > > x86: quirks.c include inverse Xmas tree effect
> > > x86: i8237.c include inverse Xmas tree effect
> > > x86: alternative.c include inverse Xmas tree effect
> > > x86: tsc.c include inverse Xmas tree effect
> > > x86: process.c include inverse Xmas tree effect
> > > x86: xsave.c include inverse Xmas tree effect
> > > x86: ptrace.c include inverse Xmas tree effect
> > > x86: ds.c include inverse Xmas tree effect
> > > x86: tls.c include inverse Xmas tree effect
> > > x86: step.c include inverse Xmas tree effect
> > > x86: stacktrace.c include inverse Xmas tree effect
> > > x86: reboot.c include inverse Xmas tree effect
> > > x86: msr.c include inverse Xmas tree effect
> > > x86: cpuid.c include inverse Xmas tree effect
> > > x86: early-quirks.c include inverse Xmas tree effect
> > > x86: smp.c include inverse Xmas tree effect
> > > x86: smpboot.c include inverse Xmas tree effect
> > > x86: tsc_sync.c include inverse Xmas tree effect
> > > x86: setup_percpu.c include inverse Xmas tree effect
> > > x86: trampoline_32.S include inverse Xmas tree effect
> > > x86: mpparse.c include inverse Xmas tree effect
> > > x86: module_32.c include inverse Xmas tree effect
> > > x86: doublefault_32.c include inverse Xmas tree effect
> > > x86: vm86_32.c include inverse Xmas tree effect
> > > ix86: early_printk.c include inverse Xmas tree effect
> > > x86: pcspeaker.c include inverse Xmas tree effect
> > > x86: head_32.c include inverse Xmas tree effect
> > > x86: head.c include inverse Xmas tree effect
> > > x86: init_task.c include inverse Xmas tree effect
> >
> > Where have you sent these patches for review?
> >
>
> -tip's master.
>
> > > Complete diff:
> >
> > No. Complete diffs are not easy to review. At least not in this size.
>
> 46 of 49 are clean patches, and only 3 are doubtful which you pointed. I
> am attaching those patches for feedback.
>

Ahh here is one more:

> >
> > > diff --git a/arch/x86/include/asm/proto.h b/arch/x86/include/asm/proto.h
> > > index 49fb3ec..a8d63be 100644
> > > --- a/arch/x86/include/asm/proto.h
> > > +++ b/arch/x86/include/asm/proto.h
> > > @@ -7,7 +7,9 @@
> > >
> > > extern void early_idt_handler(void);
> > >
> > > +#ifdef CONFIG_X86_64
> > > extern void system_call(void);
> > > +#endif
> > > extern void syscall_init(void);
> > >
> > > extern void ia32_syscall(void);

From: Jaswinder Singh Rajput <[email protected]>
Date: Sat, 28 Mar 2009 14:12:34 +0530
Subject: [PATCH] x86: traps.c include inverse Xmas tree effect

Impact: Xmas tree effect

Signed-off-by: Jaswinder Singh Rajput <[email protected]>
---
arch/x86/include/asm/proto.h | 2 ++
arch/x86/kernel/traps.c | 34 +++++++++++-----------------------
2 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/proto.h b/arch/x86/include/asm/proto.h
index 49fb3ec..a8d63be 100644
--- a/arch/x86/include/asm/proto.h
+++ b/arch/x86/include/asm/proto.h
@@ -7,7 +7,9 @@

extern void early_idt_handler(void);

+#ifdef CONFIG_X86_64
extern void system_call(void);
+#endif
extern void syscall_init(void);

extern void ia32_syscall(void);
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index a1d2883..4f1e6cf 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -15,6 +15,7 @@
#include <linux/kprobes.h>
#include <linux/uaccess.h>
#include <linux/utsname.h>
+#include <linux/ioport.h>
#include <linux/kdebug.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -25,47 +26,34 @@
#include <linux/kexec.h>
#include <linux/sched.h>
#include <linux/timer.h>
+#include <linux/edac.h>
+#include <linux/eisa.h>
#include <linux/init.h>
#include <linux/bug.h>
+#include <linux/mca.h>
#include <linux/nmi.h>
#include <linux/mm.h>
#include <linux/smp.h>
#include <linux/io.h>

-#ifdef CONFIG_EISA
-#include <linux/ioport.h>
-#include <linux/eisa.h>
-#endif
-
-#ifdef CONFIG_MCA
-#include <linux/mca.h>
-#endif
-
-#if defined(CONFIG_EDAC)
-#include <linux/edac.h>
-#endif
-
+#include <asm/processor-flags.h>
+#include <asm/mach_traps.h>
#include <asm/stacktrace.h>
#include <asm/processor.h>
#include <asm/debugreg.h>
+#include <asm/pgalloc.h>
#include <asm/atomic.h>
#include <asm/system.h>
-#include <asm/traps.h>
-#include <asm/desc.h>
-#include <asm/i387.h>
-
-#include <asm/mach_traps.h>
-
-#ifdef CONFIG_X86_64
-#include <asm/pgalloc.h>
#include <asm/proto.h>
-#else
-#include <asm/processor-flags.h>
#include <asm/setup.h>
#include <asm/traps.h>
+#include <asm/desc.h>
+#include <asm/i387.h>

#include "cpu/mcheck/mce.h"

+#ifndef CONFIG_X86_64
+
asmlinkage int system_call(void);

/* Do we ignore FPU interrupts ? */
--
1.6.0.6

> > > diff --git a/arch/x86/include/asm/sigcontext32.h b/arch/x86/include/asm/sigcontext32.h
> > > index ad1478c..f80df2e 100644
> > > --- a/arch/x86/include/asm/sigcontext32.h
> > > +++ b/arch/x86/include/asm/sigcontext32.h
> > > @@ -1,6 +1,7 @@
> > > #ifndef _ASM_X86_SIGCONTEXT32_H
> > > #define _ASM_X86_SIGCONTEXT32_H
> > >
> > > +#ifdef CONFIG_IA32_EMULATION
> > > #include <linux/types.h>
> > >
> > > /* signal context for 32bit programs. */
> > > @@ -73,5 +74,5 @@ struct sigcontext_ia32 {
> > > unsigned int oldmask;
> > > unsigned int cr2;
> > > };
> > > -
> > > +#endif
> > > #endif /* _ASM_X86_SIGCONTEXT32_H */
> >
> > This doesn't seem to be related to your header include rearangements.
> >
>
> Please check this patch then you will find the relation:
>
> From: Jaswinder Singh Rajput <[email protected]>
> Date: Sat, 28 Mar 2009 16:02:42 +0530
> Subject: [PATCH] x86: xsave.c include inverse Xmas tree effect
>
> Impact: Xmas tree effect
>
> Signed-off-by: Jaswinder Singh Rajput <[email protected]>
> ---
> arch/x86/include/asm/sigcontext32.h | 3 ++-
> arch/x86/kernel/xsave.c | 5 ++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/sigcontext32.h b/arch/x86/include/asm/sigcontext32.h
> index ad1478c..f80df2e 100644
> --- a/arch/x86/include/asm/sigcontext32.h
> +++ b/arch/x86/include/asm/sigcontext32.h
> @@ -1,6 +1,7 @@
> #ifndef _ASM_X86_SIGCONTEXT32_H
> #define _ASM_X86_SIGCONTEXT32_H
>
> +#ifdef CONFIG_IA32_EMULATION
> #include <linux/types.h>
>
> /* signal context for 32bit programs. */
> @@ -73,5 +74,5 @@ struct sigcontext_ia32 {
> unsigned int oldmask;
> unsigned int cr2;
> };
> -
> +#endif
> #endif /* _ASM_X86_SIGCONTEXT32_H */
> diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
> index 2b54fe0..778ed67 100644
> --- a/arch/x86/kernel/xsave.c
> +++ b/arch/x86/kernel/xsave.c
> @@ -3,12 +3,11 @@
> *
> * Author: Suresh Siddha <[email protected]>
> */
> +
> +#include <asm/sigcontext32.h>
> #include <linux/bootmem.h>
> #include <linux/compat.h>
> #include <asm/i387.h>
> -#ifdef CONFIG_IA32_EMULATION
> -#include <asm/sigcontext32.h>
> -#endif
> #include <asm/xcr.h>
>
> /*
> --
> 1.6.0.6

this one looks much better:

From: Jaswinder Singh Rajput <[email protected]>
Date: Sat, 28 Mar 2009 16:02:42 +0530
Subject: [PATCH] x86: xsave.c include inverse Xmas tree effect

Impact: Xmas tree effect

Signed-off-by: Jaswinder Singh Rajput <[email protected]>
---
arch/x86/include/asm/sigcontext32.h | 3 ++-
arch/x86/kernel/xsave.c | 6 +++---
2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/sigcontext32.h b/arch/x86/include/asm/sigcontext32.h
index ad1478c..f80df2e 100644
--- a/arch/x86/include/asm/sigcontext32.h
+++ b/arch/x86/include/asm/sigcontext32.h
@@ -1,6 +1,7 @@
#ifndef _ASM_X86_SIGCONTEXT32_H
#define _ASM_X86_SIGCONTEXT32_H

+#ifdef CONFIG_IA32_EMULATION
#include <linux/types.h>

/* signal context for 32bit programs. */
@@ -73,5 +74,5 @@ struct sigcontext_ia32 {
unsigned int oldmask;
unsigned int cr2;
};
-
+#endif
#endif /* _ASM_X86_SIGCONTEXT32_H */
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 2b54fe0..f3ce183 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -3,12 +3,12 @@
*
* Author: Suresh Siddha <[email protected]>
*/
+
#include <linux/bootmem.h>
#include <linux/compat.h>
-#include <asm/i387.h>
-#ifdef CONFIG_IA32_EMULATION
+
#include <asm/sigcontext32.h>
-#endif
+#include <asm/i387.h>
#include <asm/xcr.h>

/*
--
1.6.0.6

2009-03-28 19:02:00

by Sam Ravnborg

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sat, Mar 28, 2009 at 06:07:46PM +0300, Alexey Dobriyan wrote:
> On Sat, Mar 28, 2009 at 08:18:50PM +0530, Jaswinder Singh Rajput wrote:
> > The following changes since commit 29219683c46cb89edf5c58418b5305b14646d030:
> > Ingo Molnar (1):
> > Merge branches 'x86/apic', 'x86/cleanups', 'x86/mm', 'x86/pat', 'x86/setup' and 'x86/signal'; commit 'v2.6.29' into x86/core
> >
> > are available in the git repository at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-Xmas.git x86/core
> >
> > Jaswinder Singh Rajput (49):
> > x86: process_32.c include inverse Xmas tree effect
>
> What the heck is this?

Ordering include based on length of line minimize the
risk of merge conflicts.
If people just add new includes in the bottom of the list you
are certain that a merge conflit happens.

This scheme is starting to be used in several places with the
primary advocates being David Miller and Ingo.

It is getting used both for includes _and_ for local variables.

Sam

2009-03-28 22:26:49

by H. Peter Anvin

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

Sam Ravnborg wrote:
>
> Ordering include based on length of line minimize the
> risk of merge conflicts.
> If people just add new includes in the bottom of the list you
> are certain that a merge conflit happens.
>
> This scheme is starting to be used in several places with the
> primary advocates being David Miller and Ingo.
>
> It is getting used both for includes _and_ for local variables.
>

Personally I'd prefer alphabetic order, sorting based on length isn't a
complete ordering. Nearly all editors can sort alphabetically at the
push of a key.

-hpa

2009-03-28 22:38:27

by Al Viro

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sat, Mar 28, 2009 at 03:25:17PM -0700, H. Peter Anvin wrote:
> Sam Ravnborg wrote:
> >
> > Ordering include based on length of line minimize the
> > risk of merge conflicts.
> > If people just add new includes in the bottom of the list you
> > are certain that a merge conflit happens.
> >
> > This scheme is starting to be used in several places with the
> > primary advocates being David Miller and Ingo.
> >
> > It is getting used both for includes _and_ for local variables.
> >
>
> Personally I'd prefer alphabetic order, sorting based on length isn't a
> complete ordering. Nearly all editors can sort alphabetically at the
> push of a key.

*shrug*

FWIW, the real problem is that we have far too many includes in a typical
file; the ordering wouldn't matter if there would be 4-5 #include in
foo.c. Inventing elaborate policies to cope with that crap instead of
addressing the root cause (namely, cut'n'paste approach to includes) is
rather pointless...

2009-03-28 23:35:25

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

* Thomas Gleixner <[email protected]> wrote:

> > Personally I'd prefer alphabetic order, sorting based on length
> > isn't a complete ordering. Nearly all editors can sort
> > alphabetically at the push of a key.
>
> I'd prefer if somebody would sit down and write a tool to analyse
> the include hell instead of manually shuffling crap around to
> avoid trivial merge conflicts. I have cleaned up enough stuff in
> the x86 merger myself where I was able to cut the number of
> includes at least in half just by staring at the gcc intermediate
> files. We could do better and automate the analysis so we get down
> to a handful of includes instead of including the world and more.

I do not disagree with include file cleanups (we've done many of
them in this cycle and in previous cycles), but note that the
reduction in include files at the top of .c files actually increases
the chance of patch conflicts: when a new include file is added by
two patches to the same .c file.

Ingo

2009-03-28 23:36:04

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

* Jaswinder Singh Rajput <[email protected]> wrote:

> The following changes since commit 29219683c46cb89edf5c58418b5305b14646d030:
> Ingo Molnar (1):
> Merge branches 'x86/apic', 'x86/cleanups', 'x86/mm', 'x86/pat', 'x86/setup' and 'x86/signal'; commit 'v2.6.29' into x86/core
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-Xmas.git x86/core
>
> Jaswinder Singh Rajput (49):

Looks nice.

Have you written a script for this? If yes, would you mind to share
it with us? Thanks,

Ingo

2009-03-28 23:37:51

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sat, 28 Mar 2009, H. Peter Anvin wrote:
> Sam Ravnborg wrote:
> >
> > Ordering include based on length of line minimize the
> > risk of merge conflicts.
> > If people just add new includes in the bottom of the list you
> > are certain that a merge conflit happens.
> >
> > This scheme is starting to be used in several places with the
> > primary advocates being David Miller and Ingo.
> >
> > It is getting used both for includes _and_ for local variables.
> >
>
> Personally I'd prefer alphabetic order, sorting based on length isn't a
> complete ordering. Nearly all editors can sort alphabetically at the
> push of a key.

I'd prefer if somebody would sit down and write a tool to analyse the
include hell instead of manually shuffling crap around to avoid
trivial merge conflicts. I have cleaned up enough stuff in the x86
merger myself where I was able to cut the number of includes at least
in half just by staring at the gcc intermediate files. We could do
better and automate the analysis so we get down to a handful of
includes instead of including the world and more.

Also there are lots of occasions where includes in header files can be
avoided completely by a single line forward declaration of struct foo
instead of adding the include, which drags in another five.

Thanks,

tglx

2009-03-28 23:40:34

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

* Hugh Dickins <[email protected]> wrote:

> On Sat, 28 Mar 2009, Jaswinder Singh Rajput wrote:
> > The following changes since commit 29219683c46cb89edf5c58418b5305b14646d030:
> > Ingo Molnar (1):
> > Merge branches 'x86/apic', 'x86/cleanups', 'x86/mm', 'x86/pat', 'x86/setup' and 'x86/signal'; commit 'v2.6.29' into x86/core
> >
> > are available in the git repository at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-Xmas.git x86/core
> >
> > Jaswinder Singh Rajput (49):
> > x86: process_32.c include inverse Xmas tree effect
> > x86: signal.c include inverse Xmas tree effect
> > x86: entry_32.S include inverse Xmas tree effect
> > x86: traps.c include inverse Xmas tree effect
> > x86: irq.c include inverse Xmas tree effect
> > x86: irq_32.c include inverse Xmas tree effect
> > x86: dumpstack.c include inverse Xmas tree effect
> > x86: time_32.c include inverse Xmas tree effect
> > x86: ioport.c include inverse Xmas tree effect
> > x86: ldt.c include inverse Xmas tree effect
> > x86: dumpstack_32.c include inverse Xmas tree effect
> > x86: setup.c include inverse Xmas tree effect
> > x86: i8259.c include inverse Xmas tree effect
> > x86: irqinit_32.c include inverse Xmas tree effect
> > x86: probe_roms_32.c include inverse Xmas tree effect
> > x86: sys_i386_32.c include inverse Xmas tree effect
> > x86: i386_ksyms_32.c include inverse Xmas tree effect
> > x86: bootflag.c include inverse Xmas tree effect
> > x86: e820.c include inverse Xmas tree effect
> > x86: pci-dma.c include inverse Xmas tree effect
> > x86: quirks.c include inverse Xmas tree effect
> > x86: i8237.c include inverse Xmas tree effect
> > x86: alternative.c include inverse Xmas tree effect
> > x86: tsc.c include inverse Xmas tree effect
> > x86: process.c include inverse Xmas tree effect
> > x86: xsave.c include inverse Xmas tree effect
> > x86: ptrace.c include inverse Xmas tree effect
> > x86: ds.c include inverse Xmas tree effect
> > x86: tls.c include inverse Xmas tree effect
> > x86: step.c include inverse Xmas tree effect
> > x86: stacktrace.c include inverse Xmas tree effect
> > x86: reboot.c include inverse Xmas tree effect
> > x86: msr.c include inverse Xmas tree effect
> > x86: cpuid.c include inverse Xmas tree effect
> > x86: early-quirks.c include inverse Xmas tree effect
> > x86: smp.c include inverse Xmas tree effect
> > x86: smpboot.c include inverse Xmas tree effect
> > x86: tsc_sync.c include inverse Xmas tree effect
> > x86: setup_percpu.c include inverse Xmas tree effect
> > x86: trampoline_32.S include inverse Xmas tree effect
> > x86: mpparse.c include inverse Xmas tree effect
> > x86: module_32.c include inverse Xmas tree effect
> > x86: doublefault_32.c include inverse Xmas tree effect
> > x86: vm86_32.c include inverse Xmas tree effect
> > ix86: early_printk.c include inverse Xmas tree effect
> > x86: pcspeaker.c include inverse Xmas tree effect
> > x86: head_32.c include inverse Xmas tree effect
> > x86: head.c include inverse Xmas tree effect
> > x86: init_task.c include inverse Xmas tree effect
>
> Gasp.
>
> Please find a better use for your time.
>
> Imagine, there might be developers out there with real work
> to do in these files: all you are doing is throwing silly
> little obstacles in their way.

Actually, now is the right time to do such changes as we've just
pushed all pending x86 changes upstream. And since these changes
reduce the chance of future patch conflicts, they do help
development. (in a small way)

Nevertheless i'd suggest to make this a single commit, to not
clobber the shortlog with it.

Ingo

2009-03-28 23:46:16

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sun, 29 Mar 2009, Ingo Molnar wrote:
> * Thomas Gleixner <[email protected]> wrote:
>
> > > Personally I'd prefer alphabetic order, sorting based on length
> > > isn't a complete ordering. Nearly all editors can sort
> > > alphabetically at the push of a key.
> >
> > I'd prefer if somebody would sit down and write a tool to analyse
> > the include hell instead of manually shuffling crap around to
> > avoid trivial merge conflicts. I have cleaned up enough stuff in
> > the x86 merger myself where I was able to cut the number of
> > includes at least in half just by staring at the gcc intermediate
> > files. We could do better and automate the analysis so we get down
> > to a handful of includes instead of including the world and more.
>
> I do not disagree with include file cleanups (we've done many of
> them in this cycle and in previous cycles), but note that the
> reduction in include files at the top of .c files actually increases
> the chance of patch conflicts: when a new include file is added by
> two patches to the same .c file.

Those conflicts are trivial and if we have a mechanism to anlyse
include dependencies then we can avoid such conflicts often at all.

Go through some of the include madness and watch the compiler reading
the same header file ten times for a single source file compile.

Thanks,

tglx

2009-03-29 00:01:06

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

* Thomas Gleixner <[email protected]> wrote:

> On Sun, 29 Mar 2009, Ingo Molnar wrote:
> > * Thomas Gleixner <[email protected]> wrote:
> >
> > > > Personally I'd prefer alphabetic order, sorting based on length
> > > > isn't a complete ordering. Nearly all editors can sort
> > > > alphabetically at the push of a key.
> > >
> > > I'd prefer if somebody would sit down and write a tool to analyse
> > > the include hell instead of manually shuffling crap around to
> > > avoid trivial merge conflicts. I have cleaned up enough stuff in
> > > the x86 merger myself where I was able to cut the number of
> > > includes at least in half just by staring at the gcc intermediate
> > > files. We could do better and automate the analysis so we get down
> > > to a handful of includes instead of including the world and more.
> >
> > I do not disagree with include file cleanups (we've done many of
> > them in this cycle and in previous cycles), but note that the
> > reduction in include files at the top of .c files actually increases
> > the chance of patch conflicts: when a new include file is added by
> > two patches to the same .c file.
>
> Those conflicts are trivial and if we have a mechanism to anlyse
> include dependencies then we can avoid such conflicts often at
> all.

Yeah, i agree that least 50% of the existing #include's are not
needed.

That's not the point though. When two new files both append to the
#include file section, they'll clash. Those _new_ includes tend to
be justified - people dont add #includes needlessly - it's years
long bitrot that causes #includes to bloat up slowly.

> Go through some of the include madness and watch the compiler
> reading the same header file ten times for a single source file
> compile.

Yes, we need both efforts. That section needs to look professional
(unordered lines look amateurishly random, arbitrary and confusing),
and it needs to be minimalized as well.

And the xmas-tree helps the minimizing effort - as it trivially
eliminates duplicate #includes (which do happen frequently as well),
and they eliminate related includes as well because they get moved
together and get noticed.

Most importantly, such code needs to look like as if someone
actually cared about it. If code looks tidy, people assume that it's
supposed to be perfect, and will notice and fix more subtle
imperfections in it. If code looks totally messy on the other hand,
_nobody_ will care because it's visibly broken. So these changes are
a small step forward.

Ingo

2009-03-29 00:24:36

by Al Viro

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sun, Mar 29, 2009 at 01:00:26AM +0100, Ingo Molnar wrote:

> Yes, we need both efforts. That section needs to look professional
> (unordered lines look amateurishly random, arbitrary and confusing),
> and it needs to be minimalized as well.

You are _begging_ for basic training jokes, along the lines of "every
blade of grass shall be painted regulation green"...

2009-03-29 00:29:52

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sun, 29 Mar 2009, Thomas Gleixner wrote:

> On Sun, 29 Mar 2009, Ingo Molnar wrote:
> > * Thomas Gleixner <[email protected]> wrote:
> >
> > > > Personally I'd prefer alphabetic order, sorting based on length
> > > > isn't a complete ordering. Nearly all editors can sort
> > > > alphabetically at the push of a key.
> > >
> > > I'd prefer if somebody would sit down and write a tool to analyse
> > > the include hell instead of manually shuffling crap around to
> > > avoid trivial merge conflicts. I have cleaned up enough stuff in
> > > the x86 merger myself where I was able to cut the number of
> > > includes at least in half just by staring at the gcc intermediate
> > > files. We could do better and automate the analysis so we get down
> > > to a handful of includes instead of including the world and more.
> >
> > I do not disagree with include file cleanups (we've done many of
> > them in this cycle and in previous cycles), but note that the
> > reduction in include files at the top of .c files actually increases
> > the chance of patch conflicts: when a new include file is added by
> > two patches to the same .c file.
>
> Those conflicts are trivial and if we have a mechanism to anlyse
> include dependencies then we can avoid such conflicts often at all.
>
> Go through some of the include madness and watch the compiler reading
> the same header file ten times for a single source file compile.

I'm not against sorting the headers at all. I personally have no
preference whether it's alphabetical or length, but...

I just run the following on current git:

# FS=`find -type f`; for F in $FS; do grep -c "#include" $F; done | sort -n | uniq -c

The top 5 are:

lib/locking-selftest.c: 108 (justified as this includes all the test cases)
fs/compat_ioctl.c 88 (maybe ok as it is the all in one kitchen sink)
arch/x86/kernel/setup.c: 76 (completely nuts)
init/main.c: 62 (needs to be looked at)
security/selinux/hooks.c 61 (needs to be looked at, spotted at least 5 which can be removed)

1400 files have more than 20 includes. 5375 more than 10.

I think it's well justified to sit down and work on a tool which helps
us analyse and distangle that mess.

Thanks,

tglx

2009-03-29 01:58:34

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

* Thomas Gleixner <[email protected]> wrote:

> I just run the following on current git:
>
> # FS=`find -type f`; for F in $FS; do grep -c "#include" $F; done | sort -n | uniq -c
>
> The top 5 are:
>
> lib/locking-selftest.c: 108 (justified as this includes all the test cases)
> fs/compat_ioctl.c 88 (maybe ok as it is the all in one kitchen sink)
> arch/x86/kernel/setup.c: 76 (completely nuts)

[yep]

> init/main.c: 62 (needs to be looked at)
> security/selinux/hooks.c 61 (needs to be looked at, spotted at least 5 which can be removed)
>
> 1400 files have more than 20 includes. 5375 more than 10.
>
> I think it's well justified to sit down and work on a tool which
> helps us analyse and distangle that mess.

yeah.

Ingo

2009-03-29 05:54:55

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sun, 2009-03-29 at 00:35 +0100, Ingo Molnar wrote:
> * Jaswinder Singh Rajput <[email protected]> wrote:
>
> > The following changes since commit 29219683c46cb89edf5c58418b5305b14646d030:
> > Ingo Molnar (1):
> > Merge branches 'x86/apic', 'x86/cleanups', 'x86/mm', 'x86/pat', 'x86/setup' and 'x86/signal'; commit 'v2.6.29' into x86/core
> >
> > are available in the git repository at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-Xmas.git x86/core
> >
> > Jaswinder Singh Rajput (49):
>
> Looks nice.
>
> Have you written a script for this? If yes, would you mind to share
> it with us? Thanks,

I have done this manually.

Scripts can be helpful in ordering but will not be useful in many cases
like:
1. Duplication of asm files (include/linux/xyz.h call asm/xyz.h file
without any case or this case is always true in that file)
2. Remove unwanted files
3. Handle various ifdefs in include file
4. handling errors which can be generated by this ordering
5. do some more beautification where required
6. ordering of variables
7. and many more issues which I have not faced so far ..

So because of above cases even you made scripts with AI, they will fail
in many cases.

--
JSR

2009-03-29 06:39:59

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sun, 2009-03-29 at 00:40 +0100, Ingo Molnar wrote:
> * Hugh Dickins <[email protected]> wrote:
> >
> > Please find a better use for your time.
> >
> > Imagine, there might be developers out there with real work
> > to do in these files: all you are doing is throwing silly
> > little obstacles in their way.
>
> Actually, now is the right time to do such changes as we've just
> pushed all pending x86 changes upstream. And since these changes
> reduce the chance of future patch conflicts, they do help
> development. (in a small way)
>
> Nevertheless i'd suggest to make this a single commit, to not
> clobber the shortlog with it.
>

I think individual patches will be better option, in case of unavoidable
situation we can simply revert that particular patch.

--
JSR

2009-03-29 07:58:43

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sat, 2009-03-28 at 15:25 -0700, H. Peter Anvin wrote:
> Sam Ravnborg wrote:
> >
> > Ordering include based on length of line minimize the
> > risk of merge conflicts.
> > If people just add new includes in the bottom of the list you
> > are certain that a merge conflit happens.
> >
> > This scheme is starting to be used in several places with the
> > primary advocates being David Miller and Ingo.
> >
> > It is getting used both for includes _and_ for local variables.
> >
>
> Personally I'd prefer alphabetic order, sorting based on length isn't a
> complete ordering. Nearly all editors can sort alphabetically at the
> push of a key.
>

This sorting is based on Length + alphabetic.

--
JSR

2009-03-29 10:18:32

by Alexey Dobriyan

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sun, Mar 29, 2009 at 01:00:26AM +0100, Ingo Molnar wrote:
>
> * Thomas Gleixner <[email protected]> wrote:
>
> > On Sun, 29 Mar 2009, Ingo Molnar wrote:
> > > * Thomas Gleixner <[email protected]> wrote:
> > >
> > > > > Personally I'd prefer alphabetic order, sorting based on length
> > > > > isn't a complete ordering. Nearly all editors can sort
> > > > > alphabetically at the push of a key.
> > > >
> > > > I'd prefer if somebody would sit down and write a tool to analyse
> > > > the include hell instead of manually shuffling crap around to
> > > > avoid trivial merge conflicts. I have cleaned up enough stuff in
> > > > the x86 merger myself where I was able to cut the number of
> > > > includes at least in half just by staring at the gcc intermediate
> > > > files. We could do better and automate the analysis so we get down
> > > > to a handful of includes instead of including the world and more.
> > >
> > > I do not disagree with include file cleanups (we've done many of
> > > them in this cycle and in previous cycles), but note that the
> > > reduction in include files at the top of .c files actually increases
> > > the chance of patch conflicts: when a new include file is added by
> > > two patches to the same .c file.
> >
> > Those conflicts are trivial and if we have a mechanism to anlyse
> > include dependencies then we can avoid such conflicts often at
> > all.
>
> Yeah, i agree that least 50% of the existing #include's are not
> needed.
>
> That's not the point though. When two new files both append to the
> #include file section, they'll clash. Those _new_ includes tend to
> be justified - people dont add #includes needlessly - it's years
> long bitrot that causes #includes to bloat up slowly.
>
> > Go through some of the include madness and watch the compiler
> > reading the same header file ten times for a single source file
> > compile.

Care to read strace(1) output? Include file is not even opened second
time.

[reads strace(1) output himself]

Correction!

Except, e. g., <asm/atomic.h> because i386/x86_64 unification dropped
ifndef guards in the right place and added them in the wrong one.

> Yes, we need both efforts. That section needs to look professional
> (unordered lines look amateurishly random, arbitrary and confusing),
> and it needs to be minimalized as well.

These are #include directives, how else should they look like.
As for confusing, as if you're looking for the first time at C code.

> And the xmas-tree helps the minimizing effort - as it trivially
> eliminates duplicate #includes (which do happen frequently as well),

yes, 5-10-20 for the whole tree per release and people periodically
removing them regardless of -tip, Xmas and so on.

> and they eliminate related includes as well because they get moved
> together and get noticed.

I'll answer on this later.

> Most importantly, such code needs to look like as if someone
> actually cared about it.

Reminder, this is include directives, not real code.

> If code looks tidy, people assume that it's
> supposed to be perfect, and will notice and fix more subtle
> imperfections in it. If code looks totally messy on the other hand,
> _nobody_ will care because it's visibly broken. So these changes are
> a small step forward.

People probably forgot the real problem with includes and start
inventing new non-problems with all the consequences.

So here is my countersuggestion for small and useful improvements:

1. Plug ifndef guargs back on x86.
Check with strace(1) for past behaviour re non-opened includes.
Remove current ifdef guards (need much more thinking for consequences)

2. Do #1 for other arches which exercised similar transformation
(sparc/sparc64?)

3. For headers with small and let's say bounded area like <asm/unistd.h>,
go check for every file including them and remove it if header genuinely
unneeded.

In case of <asm/unistd.h> this would check for copy_from_user/copy_to_user,
put_user/get_user, clear_user and not much more.

This will reduce compile time _and_ size of DEBUG_INFO=y kernels.

2009-03-29 11:48:20

[permalink] [raw]

Subject: Re: [git-pull -tip] x86: include inverse Xmas tree patches

On Sun, 29 Mar 2009, Jaswinder Singh Rajput wrote:
> On Sun, 2009-03-29 at 00:40 +0100, Ingo Molnar wrote:
> > * Hugh Dickins <[email protected]> wrote:
> > >
> > > Please find a better use for your time.
> > >
> > > Imagine, there might be developers out there with real work
> > > to do in these files: all you are doing is throwing silly
> > > little obstacles in their way.
> >
> > Actually, now is the right time to do such changes as we've just
> > pushed all pending x86 changes upstream. And since these changes
> > reduce the chance of future patch conflicts, they do help
> > development. (in a small way)
> >
> > Nevertheless i'd suggest to make this a single commit, to not
> > clobber the shortlog with it.
> >
>
> I think individual patches will be better option, in case of unavoidable
> situation we can simply revert that particular patch.

What unavoidable situation ? That you broke stuff with your changes ?
If you are so unsure about the quality of your changes, then the best
option is to not pull that stuff at all.

Thanks,

tglx

2009-03-29 14:39:36