[PATCH x86] [0/16] Various i386/x86-64 changes

[PATCH x86] [0/16] Various i386/x86-64 changes

[Andi Kleen]

All against recent git-x86

[PATCH x86] [1/16] Make clocksource watchdog cycle through online CPUs

[Andi Kleen]

This way it checks if the clocks are synchronized between CPUs too.
This might be able to detect slowly drifting TSCs which only
go wrong over longer time.

Cc: tglx@linutronix.de

Signed-off-by: Andi Kleen <ak@suse.de>

---
 kernel/time/clocksource.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Index: linux/kernel/time/clocksource.c
===================================================================
--- linux.orig/kernel/time/clocksource.c
+++ linux/kernel/time/clocksource.c
@@ -142,8 +142,13 @@ static void clocksource_watchdog(unsigne
 	}
 
 	if (!list_empty(&watchdog_list)) {
-		__mod_timer(&watchdog_timer,
-			    watchdog_timer.expires + WATCHDOG_INTERVAL);
+		/* Cycle through CPUs to check if the CPUs stay synchronized to
+		 * each other. */
+		int next_cpu = next_cpu(raw_smp_processor_id(), cpu_online_map);
+		if (next_cpu >= NR_CPUS)
+			next_cpu = first_cpu(cpu_online_map);
+		watchdog_timer.expires += WATCHDOG_INTERVAL;
+		add_timer_on(&watchdog_timer, next_cpu);
 	}
 	spin_unlock(&watchdog_lock);
 }
@@ -165,7 +170,7 @@ static void clocksource_check_watchdog(s
 		if (!started && watchdog) {
 			watchdog_last = watchdog->read();
 			watchdog_timer.expires = jiffies + WATCHDOG_INTERVAL;
-			add_timer(&watchdog_timer);
+			add_timer_on(&watchdog_timer, first_cpu(cpu_online_map));
 		}
 	} else {
 		if (cs->flags & CLOCK_SOURCE_IS_CONTINUOUS)
@@ -186,7 +191,8 @@ static void clocksource_check_watchdog(s
 				watchdog_last = watchdog->read();
 				watchdog_timer.expires =
 					jiffies + WATCHDOG_INTERVAL;
-				add_timer(&watchdog_timer);
+				add_timer_on(&watchdog_timer,
+						first_cpu(cpu_online_map));
 			}
 		}
 	}

[PATCH x86] [2/16] Add a counter for per cpu clocksource watchdog checks and report them in /proc/interrupts

[Andi Kleen]

While infrequent it's better if all per CPU events have a counter.

Only done for x86 currently.

Cc: tglx@linutronix.de

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/kernel/irq_32.c    |    7 +++++++
 arch/x86/kernel/irq_64.c    |    7 +++++++
 include/linux/clocksource.h |    3 +++
 kernel/time/clocksource.c   |    5 +++++
 4 files changed, 22 insertions(+)

Index: linux/arch/x86/kernel/irq_32.c
===================================================================
--- linux.orig/arch/x86/kernel/irq_32.c
+++ linux/arch/x86/kernel/irq_32.c
@@ -15,6 +15,7 @@
 #include <linux/notifier.h>
 #include <linux/cpu.h>
 #include <linux/delay.h>
+#include <linux/clocksource.h>
 
 #include <asm/apic.h>
 #include <asm/uaccess.h>
@@ -315,6 +316,12 @@ skip:
 				per_cpu(irq_stat,j).irq_tlb_count);
 		seq_printf(p, "  TLB shootdowns\n");
 #endif
+#ifdef CONFIG_CLOCKSOURCE_WATCHDOG
+		seq_printf(p, "CDG: ");
+		for_each_online_cpu(j)
+			seq_printf(p, "%10u ", per_cpu(clock_wdog_count,j));
+		seq_printf(p, "  Clocksource Watchdog checks\n");
+#endif
 		seq_printf(p, "TRM: ");
 		for_each_online_cpu(j)
 			seq_printf(p, "%10u ",
Index: linux/include/linux/clocksource.h
===================================================================
--- linux.orig/include/linux/clocksource.h
+++ linux/include/linux/clocksource.h
@@ -14,6 +14,7 @@
 #include <linux/list.h>
 #include <linux/cache.h>
 #include <linux/timer.h>
+#include <linux/percpu.h>
 #include <asm/div64.h>
 #include <asm/io.h>
 
@@ -232,4 +233,6 @@ static inline void update_vsyscall_tz(vo
 }
 #endif
 
+DECLARE_PER_CPU(unsigned, clock_wdog_count);
+
 #endif /* _LINUX_CLOCKSOURCE_H */
Index: linux/kernel/time/clocksource.c
===================================================================
--- linux.orig/kernel/time/clocksource.c
+++ linux/kernel/time/clocksource.c
@@ -76,6 +76,8 @@ static DEFINE_SPINLOCK(watchdog_lock);
 static cycle_t watchdog_last;
 static unsigned long watchdog_resumed;
 
+DEFINE_PER_CPU(unsigned, clock_wdog_count);
+
 /*
  * Interval: 0.5sec Threshold: 0.0625s
  */
@@ -102,6 +104,9 @@ static void clocksource_watchdog(unsigne
 	int64_t wd_nsec, cs_nsec;
 	int resumed;
 
+	per_cpu(clock_wdog_count, get_cpu())++;
+	put_cpu();
+
 	spin_lock(&watchdog_lock);
 
 	resumed = test_and_clear_bit(0, &watchdog_resumed);
Index: linux/arch/x86/kernel/irq_64.c
===================================================================
--- linux.orig/arch/x86/kernel/irq_64.c
+++ linux/arch/x86/kernel/irq_64.c
@@ -13,6 +13,7 @@
 #include <linux/seq_file.h>
 #include <linux/module.h>
 #include <linux/delay.h>
+#include <linux/clocksource.h>
 #include <asm/uaccess.h>
 #include <asm/io_apic.h>
 #include <asm/idle.h>
@@ -139,6 +140,12 @@ skip:
 		for_each_online_cpu(j)
 			seq_printf(p, "%10u ", cpu_pda(j)->irq_thermal_count);
 		seq_printf(p, "  Thermal event interrupts\n");
+#ifdef CONFIG_CLOCKSOURCE_WATCHDOG
+		seq_printf(p, "CDG: ");
+		for_each_online_cpu(j)
+			seq_printf(p, "%10u ", per_cpu(clock_wdog_count,j));
+		seq_printf(p, "  Clocksource Watchdog checks\n");
+#endif
 		seq_printf(p, "THR: ");
 		for_each_online_cpu(j)
 			seq_printf(p, "%10u ", cpu_pda(j)->irq_threshold_count);

[PATCH x86] [3/16] Turn irq debugging options into module params

[Andi Kleen]

This allows to change them at runtime using sysfs. No need to 
reboot to set them.

I only added aliases (kernel.noirqdebug etc.) so the old options 
still work.

Signed-off-by: Andi Kleen <ak@suse.de>

---
 kernel/irq/spurious.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux/kernel/irq/spurious.c
===================================================================
--- linux.orig/kernel/irq/spurious.c
+++ linux/kernel/irq/spurious.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/kallsyms.h>
 #include <linux/interrupt.h>
+#include <linux/moduleparam.h>
 
 static int irqfixup __read_mostly;
 
@@ -225,6 +226,8 @@ int noirqdebug_setup(char *str)
 }
 
 __setup("noirqdebug", noirqdebug_setup);
+module_param(noirqdebug, bool, 0644);
+MODULE_PARM_DESC(noirqdebug, "Disable irq lockup detection when true");
 
 static int __init irqfixup_setup(char *str)
 {
@@ -236,6 +239,8 @@ static int __init irqfixup_setup(char *s
 }
 
 __setup("irqfixup", irqfixup_setup);
+module_param(irqfixup, int, 0644);
+MODULE_PARM_DESC("irqfixup", "0: No fixup, 1: irqfixup mode 2: irqpoll mode");
 
 static int __init irqpoll_setup(char *str)
 {

[PATCH x86] [4/16] Add /proc/irq/*/spurious to dump the spurious irq debugging state

[Andi Kleen]

This is useful to debug problems with interrupt handlers that return
sometimes IRQ_NONE.

Signed-off-by: Andi Kleen <ak@suse.de>

---
 kernel/irq/proc.c |   21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Index: linux/kernel/irq/proc.c
===================================================================
--- linux.orig/kernel/irq/proc.c
+++ linux/kernel/irq/proc.c
@@ -75,6 +75,18 @@ static int irq_affinity_write_proc(struc
 
 #endif
 
+static int irq_spurious_read(char *page, char **start, off_t off,
+				  int count, int *eof, void *data)
+{
+	struct irq_desc *d = &irq_desc[(long) data];
+	return sprintf(page, "count %u\n"
+			     "unhandled %u\n"
+			     "last_unhandled %u ms\n",
+			d->irq_count,
+			d->irqs_unhandled,
+			jiffies_to_msecs(d->last_unhandled));
+}
+
 #define MAX_NAMELEN 128
 
 static int name_unique(unsigned int irq, struct irqaction *new_action)
@@ -118,6 +130,7 @@ void register_handler_proc(unsigned int 
 void register_irq_proc(unsigned int irq)
 {
 	char name [MAX_NAMELEN];
+	struct proc_dir_entry *entry;
 
 	if (!root_irq_dir ||
 		(irq_desc[irq].chip == &no_irq_chip) ||
@@ -132,8 +145,6 @@ void register_irq_proc(unsigned int irq)
 
 #ifdef CONFIG_SMP
 	{
-		struct proc_dir_entry *entry;
-
 		/* create /proc/irq/<irq>/smp_affinity */
 		entry = create_proc_entry("smp_affinity", 0600, irq_desc[irq].dir);
 
@@ -144,6 +155,12 @@ void register_irq_proc(unsigned int irq)
 		}
 	}
 #endif
+
+	entry = create_proc_entry("spurious", 0444, irq_desc[irq].dir);
+	if (entry) {
+		entry->data = (void *)(long)irq;
+		entry->read_proc = irq_spurious_read;
+	}
 }
 
 #undef MAX_NAMELEN

[PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table

[Andi Kleen]

On x86-64 there are several memory allocations before bootmem. To avoid
them stomping on each other they used to be all hard coded in bad_area().
Replace this with an array that is filled as needed.

This cleans up the code considerably and allows to expand its use.

Cc: peterz@infradead.org

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/kernel/e820_64.c  |   97 ++++++++++++++++++++++++---------------------
 arch/x86/kernel/head64.c   |   48 ++++++++++++++++++++++
 arch/x86/kernel/setup_64.c |   67 +------------------------------
 arch/x86/mm/init_64.c      |    5 +-
 arch/x86/mm/numa_64.c      |    1 
 include/asm-x86/e820_64.h  |    5 +-
 include/asm-x86/proto.h    |    2 
 7 files changed, 112 insertions(+), 113 deletions(-)

Index: linux/arch/x86/kernel/e820_64.c
===================================================================
--- linux.orig/arch/x86/kernel/e820_64.c
+++ linux/arch/x86/kernel/e820_64.c
@@ -47,56 +47,65 @@ unsigned long end_pfn_map;
  */
 static unsigned long __initdata end_user_pfn = MAXMEM>>PAGE_SHIFT;
 
-/* Check for some hardcoded bad areas that early boot is not allowed to touch */
-static inline int bad_addr(unsigned long *addrp, unsigned long size)
-{
-	unsigned long addr = *addrp, last = addr + size;
+/*
+ * Early reserved memory areas.
+ */
+#define MAX_EARLY_RES 20
 
-	/* various gunk below that needed for SMP startup */
-	if (addr < 0x8000) {
-		*addrp = PAGE_ALIGN(0x8000);
-		return 1;
-	}
+struct early_res {
+	unsigned long start, end;
+};
+static struct early_res early_res[MAX_EARLY_RES] __initdata = {
+	{ 0, PAGE_SIZE },			/* BIOS data page */
+#ifdef CONFIG_SMP
+	{ SMP_TRAMPOLINE_BASE, SMP_TRAMPOLINE_BASE + 2*PAGE_SIZE },
+#endif
+	{}
+};
 
-	/* direct mapping tables of the kernel */
-	if (last >= table_start<<PAGE_SHIFT && addr < table_end<<PAGE_SHIFT) {
-		*addrp = PAGE_ALIGN(table_end << PAGE_SHIFT);
-		return 1;
+void __init reserve_early(unsigned long start, unsigned long end)
+{
+	int i;
+	struct early_res *r;
+	for (i = 0; i < MAX_EARLY_RES && early_res[i].end; i++) {
+		r = &early_res[i];
+		if (end > r->start && start < r->end)
+			panic("Duplicated early reservation %lx-%lx\n",
+			      start, end);
 	}
+	if (i >= MAX_EARLY_RES)
+		panic("Too many early reservations");
+	r = &early_res[i];
+	r->start = start;
+	r->end = end;
+}
 
-	/* initrd */
-#ifdef CONFIG_BLK_DEV_INITRD
-	if (boot_params.hdr.type_of_loader && boot_params.hdr.ramdisk_image) {
-		unsigned long ramdisk_image = boot_params.hdr.ramdisk_image;
-		unsigned long ramdisk_size  = boot_params.hdr.ramdisk_size;
-		unsigned long ramdisk_end   = ramdisk_image+ramdisk_size;
+void __init early_res_to_bootmem(void)
+{
+	int i;
+	for (i = 0; i < MAX_EARLY_RES && early_res[i].end; i++) {
+	struct early_res *r = &early_res[i];
+		reserve_bootmem_generic(r->start, r->end - r->start);
+ 	}
+}
 
-		if (last >= ramdisk_image && addr < ramdisk_end) {
-			*addrp = PAGE_ALIGN(ramdisk_end);
-			return 1;
+/* Check for already reserved areas */
+static inline int bad_addr(unsigned long *addrp, unsigned long size)
+{
+	int i;
+	unsigned long addr = *addrp, last;
+	int changed = 0;
+again:
+	last = addr + size;
+	for (i = 0; i < MAX_EARLY_RES && early_res[i].end; i++) {
+		struct early_res *r = &early_res[i];
+		if (last >= r->start && addr < r->end) {
+			*addrp = addr = r->end;
+			changed = 1;
+			goto again;
 		}
-	}
-#endif
-	/* kernel code */
-	if (last >= __pa_symbol(&_text) && addr < __pa_symbol(&_end)) {
-		*addrp = PAGE_ALIGN(__pa_symbol(&_end));
-		return 1;
-	}
-
-	if (last >= ebda_addr && addr < ebda_addr + ebda_size) {
-		*addrp = PAGE_ALIGN(ebda_addr + ebda_size);
-		return 1;
-	}
-
-#ifdef CONFIG_NUMA
-	/* NUMA memory to node map */
-	if (last >= nodemap_addr && addr < nodemap_addr + nodemap_size) {
-		*addrp = nodemap_addr + nodemap_size;
-		return 1;
-	}
-#endif
-	/* XXX ramdisk image here? */
-	return 0;
+ 	}
+	return changed;
 }
 
 /*
Index: linux/arch/x86/kernel/head64.c
===================================================================
--- linux.orig/arch/x86/kernel/head64.c
+++ linux/arch/x86/kernel/head64.c
@@ -21,6 +21,7 @@
 #include <asm/tlbflush.h>
 #include <asm/sections.h>
 #include <asm/kdebug.h>
+#include <asm/e820.h>
 
 static void __init zap_identity_mappings(void)
 {
@@ -48,6 +49,35 @@ static void __init copy_bootdata(char *r
 	}
 }
 
+#define EBDA_ADDR_POINTER 0x40E
+
+static __init void reserve_ebda(void)
+{
+	unsigned ebda_addr, ebda_size;
+
+	/*
+	 * there is a real-mode segmented pointer pointing to the
+	 * 4K EBDA area at 0x40E
+	 */
+	ebda_addr = *(unsigned short *)__va(EBDA_ADDR_POINTER);
+	ebda_addr <<= 4;
+
+	if (!ebda_addr)
+		return;
+
+	ebda_size = *(unsigned short *)__va(ebda_addr);
+
+	/* Round EBDA up to pages */
+	if (ebda_size == 0)
+		ebda_size = 1;
+	ebda_size <<= 10;
+	ebda_size = round_up(ebda_size + (ebda_addr & ~PAGE_MASK), PAGE_SIZE);
+	if (ebda_size > 64*1024)
+		ebda_size = 64*1024;
+
+	reserve_early(ebda_addr, ebda_addr + ebda_size);
+}
+
 void __init x86_64_start_kernel(char * real_mode_data)
 {
 	int i;
@@ -70,5 +100,23 @@ void __init x86_64_start_kernel(char * r
 	pda_init(0);
 	copy_bootdata(__va(real_mode_data));
 
+	reserve_early(__pa_symbol(&_text), __pa_symbol(&_end));
+
+	/* Reserve INITRD */
+	if (boot_params.hdr.type_of_loader && boot_params.hdr.ramdisk_image) {
+		unsigned long ramdisk_image = boot_params.hdr.ramdisk_image;
+		unsigned long ramdisk_size  = boot_params.hdr.ramdisk_size;
+		unsigned long ramdisk_end   = ramdisk_image + ramdisk_size;
+		reserve_early(ramdisk_image, ramdisk_end);
+	}
+
+	reserve_ebda();
+
+	/*
+	 * At this point everything still needed from the boot loader
+	 * or BIOS or kernel text should be early reserved or marked not
+	 * RAM in e820. All other memory is free game.
+	 */
+
 	start_kernel();
 }
Index: linux/arch/x86/kernel/setup_64.c
===================================================================
--- linux.orig/arch/x86/kernel/setup_64.c
+++ linux/arch/x86/kernel/setup_64.c
@@ -243,41 +243,6 @@ static inline void __init reserve_crashk
 {}
 #endif
 
-#define EBDA_ADDR_POINTER 0x40E
-
-unsigned __initdata ebda_addr;
-unsigned __initdata ebda_size;
-
-static void discover_ebda(void)
-{
-	/*
-	 * there is a real-mode segmented pointer pointing to the
-	 * 4K EBDA area at 0x40E
-	 */
-	ebda_addr = *(unsigned short *)__va(EBDA_ADDR_POINTER);
-	/*
-	 * There can be some situations, like paravirtualized guests,
-	 * in which there is no available ebda information. In such
-	 * case, just skip it
-	 */
-	if (!ebda_addr) {
-		ebda_size = 0;
-		return;
-	}
-
-	ebda_addr <<= 4;
-
-	ebda_size = *(unsigned short *)__va(ebda_addr);
-
-	/* Round EBDA up to pages */
-	if (ebda_size == 0)
-		ebda_size = 1;
-	ebda_size <<= 10;
-	ebda_size = round_up(ebda_size + (ebda_addr & ~PAGE_MASK), PAGE_SIZE);
-	if (ebda_size > 64*1024)
-		ebda_size = 64*1024;
-}
-
 /* Overridden in paravirt.c if CONFIG_PARAVIRT */
 void __attribute__((weak)) memory_setup(void)
 {
@@ -355,8 +320,6 @@ void __init setup_arch(char **cmdline_p)
 
 	check_efer();
 
-	discover_ebda();
-
 	init_memory_mapping(0, (end_pfn_map << PAGE_SHIFT));
 	if (efi_enabled)
 		efi_init();
@@ -399,33 +362,7 @@ void __init setup_arch(char **cmdline_p)
 	contig_initmem_init(0, end_pfn);
 #endif
 
-	/* Reserve direct mapping */
-	reserve_bootmem_generic(table_start << PAGE_SHIFT,
-				(table_end - table_start) << PAGE_SHIFT);
-
-	/* reserve kernel */
-	reserve_bootmem_generic(__pa_symbol(&_text),
-				__pa_symbol(&_end) - __pa_symbol(&_text));
-
-	/*
-	 * reserve physical page 0 - it's a special BIOS page on many boxes,
-	 * enabling clean reboots, SMP operation, laptop functions.
-	 */
-	reserve_bootmem_generic(0, PAGE_SIZE);
-
-	/* reserve ebda region */
-	if (ebda_addr)
-		reserve_bootmem_generic(ebda_addr, ebda_size);
-#ifdef CONFIG_NUMA
-	/* reserve nodemap region */
-	if (nodemap_addr)
-		reserve_bootmem_generic(nodemap_addr, nodemap_size);
-#endif
-
-#ifdef CONFIG_SMP
-	/* Reserve SMP trampoline */
-	reserve_bootmem_generic(SMP_TRAMPOLINE_BASE, 2*PAGE_SIZE);
-#endif
+	early_res_to_bootmem();
 
 #ifdef CONFIG_ACPI_SLEEP
 	/*
@@ -455,6 +392,8 @@ void __init setup_arch(char **cmdline_p)
 			initrd_start = ramdisk_image + PAGE_OFFSET;
 			initrd_end = initrd_start+ramdisk_size;
 		} else {
+			/* Assumes everything on node 0 */
+			free_bootmem(ramdisk_image, ramdisk_size);
 			printk(KERN_ERR "initrd extends beyond end of memory "
 			       "(0x%08lx > 0x%08lx)\ndisabling initrd\n",
 			       ramdisk_end, end_of_mem);
Index: linux/arch/x86/mm/numa_64.c
===================================================================
--- linux.orig/arch/x86/mm/numa_64.c
+++ linux/arch/x86/mm/numa_64.c
@@ -99,6 +99,7 @@ static int __init allocate_cachealigned_
 	}
 	pad_addr = (nodemap_addr + pad) & ~pad;
 	memnodemap = phys_to_virt(pad_addr);
+	reserve_early(nodemap_addr, nodemap_addr + nodemap_size);
 
 	printk(KERN_DEBUG "NUMA: Allocated memnodemap from %lx - %lx\n",
 	       nodemap_addr, nodemap_addr + nodemap_size);
Index: linux/include/asm-x86/e820_64.h
===================================================================
--- linux.orig/include/asm-x86/e820_64.h
+++ linux/include/asm-x86/e820_64.h
@@ -36,8 +36,9 @@ extern void finish_e820_parsing(void);
 
 extern struct e820map e820;
 
-extern unsigned ebda_addr, ebda_size;
-extern unsigned long nodemap_addr, nodemap_size;
+extern void reserve_early(unsigned long start, unsigned long end);
+extern void early_res_to_bootmem(void);
+
 #endif/*!__ASSEMBLY__*/
 
 #endif/*__E820_HEADER*/
Index: linux/arch/x86/mm/init_64.c
===================================================================
--- linux.orig/arch/x86/mm/init_64.c
+++ linux/arch/x86/mm/init_64.c
@@ -176,7 +176,8 @@ __set_fixmap (enum fixed_addresses idx, 
 	set_pte_phys(address, phys, prot);
 }
 
-unsigned long __meminitdata table_start, table_end;
+static unsigned long __initdata table_start;
+static unsigned long __meminitdata table_end;
 
 static __meminit void *alloc_low_page(unsigned long *phys)
 { 
@@ -387,6 +388,8 @@ void __init_refok init_memory_mapping(un
 	if (!after_bootmem)
 		mmu_cr4_features = read_cr4();
 	__flush_tlb_all();
+
+	reserve_early(table_start << PAGE_SHIFT, table_end << PAGE_SHIFT);
 }
 
 #ifndef CONFIG_NUMA
Index: linux/include/asm-x86/proto.h
===================================================================
--- linux.orig/include/asm-x86/proto.h
+++ linux/include/asm-x86/proto.h
@@ -22,8 +22,6 @@ extern void syscall32_cpu_init(void);
 
 extern void check_efer(void);
 
-extern unsigned long table_start, table_end;
-
 extern int reboot_force;
 
 long do_arch_prctl(struct task_struct *task, int code, unsigned long addr);

[PATCH x86] [6/16] Add a new arch_early_alloc() interface for x86-64

[Andi Kleen]

This allows to allocate memory really early before bootmem is setup.

And a symbol that can be tested by the preprocessor.

pgtable.h is probably not the best include for it, but also not the worst.

Cc: peterz@infradead.org

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/kernel/e820_64.c    |   14 ++++++++++++++
 include/asm-x86/pgtable_64.h |    3 +++
 2 files changed, 17 insertions(+)

Index: linux/arch/x86/kernel/e820_64.c
===================================================================
--- linux.orig/arch/x86/kernel/e820_64.c
+++ linux/arch/x86/kernel/e820_64.c
@@ -819,3 +819,17 @@ int __init arch_get_ram_range(int slot, 
 		max_pfn << PAGE_SHIFT) - *addr;
 	return i + 1;
 }
+
+#define EARLY_ALLOC_START (32<<20)
+__init void *arch_early_alloc(unsigned long size)
+{
+	unsigned long p = find_e820_area(EARLY_ALLOC_START, -1UL, size);
+	if (p == -1ULL) {
+		/* Risk filling the DMA zone */
+		p = find_e820_area(EARLY_ALLOC_START, -1UL, size);
+		if (p == -1ULL)
+			panic("arch_early_alloc %lu failed", size);
+	}
+	reserve_early(p, p + size);
+	return __va(p);
+}
Index: linux/include/asm-x86/pgtable_64.h
===================================================================
--- linux.orig/include/asm-x86/pgtable_64.h
+++ linux/include/asm-x86/pgtable_64.h
@@ -436,6 +436,9 @@ pte_t *lookup_address(unsigned long addr
 #define	kc_offset_to_vaddr(o) \
    (((o) & (1UL << (__VIRTUAL_MASK_SHIFT-1))) ? ((o) | (~__VIRTUAL_MASK)) : (o))
 
+#define ARCH_HAS_EARLY_ALLOC
+extern void *arch_early_alloc(unsigned long size);
+
 #define __HAVE_ARCH_PTEP_TEST_AND_CLEAR_YOUNG
 #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
 #define __HAVE_ARCH_PTEP_GET_AND_CLEAR_FULL

[PATCH x86] [7/16] Convert lockdep to use arch_early_alloc() if available for its large arrays

[Andi Kleen]

The static arrays in lockdep can be quite big. Upto several megabytes. 
We ran into problems with 64bit kernels becoming so big that the kernel
image overlapped into the 16MB area reserved for a kdump kernel.

This patch converts lockdep to use arch_early_alloc() if available
to avoid this problem.

Cc: peterz@infradead.org

Signed-off-by: Andi Kleen <ak@suse.de>

---
 kernel/lockdep.c |   33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

Index: linux/kernel/lockdep.c
===================================================================
--- linux.orig/kernel/lockdep.c
+++ linux/kernel/lockdep.c
@@ -109,7 +109,7 @@ static inline int debug_locks_off_graph_
 static int lockdep_initialized;
 
 unsigned long nr_list_entries;
-static struct lock_list list_entries[MAX_LOCKDEP_ENTRIES];
+static struct lock_list *list_entries;
 
 /*
  * All data structures here are protected by the global debug_lock.
@@ -118,7 +118,7 @@ static struct lock_list list_entries[MAX
  * get freed - this significantly simplifies the debugging code.
  */
 unsigned long nr_lock_classes;
-static struct lock_class lock_classes[MAX_LOCKDEP_KEYS];
+static struct lock_class *lock_classes;
 
 #ifdef CONFIG_LOCK_STAT
 static DEFINE_PER_CPU(struct lock_class_stats[MAX_LOCKDEP_KEYS], lock_stats);
@@ -257,7 +257,7 @@ static struct list_head classhash_table[
 #define __chainhashfn(chain)	hash_long(chain, CHAINHASH_BITS)
 #define chainhashentry(chain)	(chainhash_table + __chainhashfn((chain)))
 
-static struct list_head chainhash_table[CHAINHASH_SIZE];
+static struct list_head *chainhash_table;
 
 /*
  * The hash key of the lock dependency chains is a hash itself too:
@@ -332,7 +332,7 @@ static int verbose(struct lock_class *cl
  * addresses. Protected by the graph_lock.
  */
 unsigned long nr_stack_trace_entries;
-static unsigned long stack_trace[MAX_STACK_TRACE_ENTRIES];
+static unsigned long *stack_trace;
 
 static int save_trace(struct stack_trace *trace)
 {
@@ -1454,7 +1454,7 @@ out_bug:
 }
 
 unsigned long nr_lock_chains;
-static struct lock_chain lock_chains[MAX_LOCKDEP_CHAINS];
+static struct lock_chain *lock_chains;
 
 /*
  * Look up a dependency chain. If the key is not present yet then
@@ -3010,6 +3010,27 @@ out_restore:
 	raw_local_irq_restore(flags);
 }
 
+/*
+ * The large arrays can bloat the kernel image too much causing problems
+ * because it needs an continuous area in memory. Allocate them
+ * using a special allocator if possible. This is before bootmem and only
+ * works on some architectures.
+ */
+
+#ifndef ARCH_HAS_EARLY_ALLOC
+#define LARGEVAR(x,y) { static typeof(*x) __ ## x[y];  x = __ ## x; }
+#else
+#define LARGEVAR(x,y) x = arch_early_alloc(sizeof(*x) * y)
+#endif
+
+void lockdep_init_mem(void)
+{
+	LARGEVAR(stack_trace, MAX_STACK_TRACE_ENTRIES);
+	LARGEVAR(list_entries, MAX_LOCKDEP_ENTRIES);
+	LARGEVAR(lock_chains, MAX_LOCKDEP_CHAINS);
+	LARGEVAR(lock_classes, MAX_LOCKDEP_KEYS);
+	LARGEVAR(chainhash_table, CHAINHASH_SIZE);
+}
 void lockdep_init(void)
 {
 	int i;
@@ -3023,6 +3044,8 @@ void lockdep_init(void)
 	if (lockdep_initialized)
 		return;
 
+	lockdep_init_mem();
+
 	for (i = 0; i < CLASSHASH_SIZE; i++)
 		INIT_LIST_HEAD(classhash_table + i);
 

[PATCH x86] [8/16] Make lockdep_init __init

[Andi Kleen]

Cc: peterz@infradead.org
Signed-off-by: Andi Kleen <ak@suse.de>

---
 kernel/lockdep.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux/kernel/lockdep.c
===================================================================
--- linux.orig/kernel/lockdep.c
+++ linux/kernel/lockdep.c
@@ -3023,7 +3023,7 @@ out_restore:
 #define LARGEVAR(x,y) x = arch_early_alloc(sizeof(*x) * y)
 #endif
 
-void lockdep_init_mem(void)
+void __init lockdep_init_mem(void)
 {
 	LARGEVAR(stack_trace, MAX_STACK_TRACE_ENTRIES);
 	LARGEVAR(list_entries, MAX_LOCKDEP_ENTRIES);
@@ -3031,7 +3031,8 @@ void lockdep_init_mem(void)
 	LARGEVAR(lock_classes, MAX_LOCKDEP_KEYS);
 	LARGEVAR(chainhash_table, CHAINHASH_SIZE);
 }
-void lockdep_init(void)
+
+void __init lockdep_init(void)
 {
 	int i;
 

[PATCH x86] [9/16] Remove CPU capabitilites printks on i386

[Andi Kleen]

I don't know of any case where they have been useful and they look ugly.

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/kernel/cpu/common.c |   20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

Index: linux/arch/x86/kernel/cpu/common.c
===================================================================
--- linux.orig/arch/x86/kernel/cpu/common.c
+++ linux/arch/x86/kernel/cpu/common.c
@@ -432,20 +432,9 @@ void __cpuinit identify_cpu(struct cpuin
 
 	generic_identify(c);
 
-	printk(KERN_DEBUG "CPU: After generic identify, caps:");
-	for (i = 0; i < NCAPINTS; i++)
-		printk(" %08x", c->x86_capability[i]);
-	printk("\n");
-
-	if (this_cpu->c_identify) {
+	if (this_cpu->c_identify)
 		this_cpu->c_identify(c);
 
-		printk(KERN_DEBUG "CPU: After vendor identify, caps:");
-		for (i = 0; i < NCAPINTS; i++)
-			printk(" %08x", c->x86_capability[i]);
-		printk("\n");
-	}
-
 	/*
 	 * Vendor-specific initialization.  In this section we
 	 * canonicalize the feature flags, meaning if there are
@@ -496,13 +485,6 @@ void __cpuinit identify_cpu(struct cpuin
 				c->x86, c->x86_model);
 	}
 
-	/* Now the feature flags better reflect actual CPU features! */
-
-	printk(KERN_DEBUG "CPU: After all inits, caps:");
-	for (i = 0; i < NCAPINTS; i++)
-		printk(" %08x", c->x86_capability[i]);
-	printk("\n");
-
 	/*
 	 * On SMP, boot_cpu_data holds the common feature set between
 	 * all CPUs; so make sure that we indicate which features are

[PATCH x86] [10/16] Document fdimage/isoimage completely in make help

[Andi Kleen]

Add missing targets and missing options in x86 make help

Cc: hpa@rpath.com
Cc: sam@ravnborg.org

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/Makefile |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Index: linux/arch/x86/Makefile
===================================================================
--- linux.orig/arch/x86/Makefile
+++ linux/arch/x86/Makefile
@@ -247,9 +247,13 @@ define archhelp
   echo  '		   (your) ~/bin/installkernel or'
   echo  '		   (distribution) /sbin/installkernel or'
   echo  '		   install to $$(INSTALL_PATH) and run lilo'
-  echo  '  bzdisk       - Create a boot floppy in /dev/fd0'
-  echo  '  fdimage      - Create a boot floppy image'
-  echo  '  isoimage     - Create a boot CD-ROM image'
+  echo  '  fdimage      - Create 1.4MB boot floppy image (arch/x86/boot/fdimage)'
+  echo  '  fdimage144	- Create 1.4MB boot floppy image (arch/x86/boot/fdimage)'
+  echo  '  fdimage288	- Create 2.8MB boot floppy image (arch/x86/boot/fdimage)'
+  echo  '  isoimage     - Create a boot CD-ROM image (arch/x86/boot/image.iso)'
+  echo  '                  bzdisk/fdimage*/isoimage also accept:'
+  echo  '                  FDARGS="..."  arguments for the booted kernel'
+  echo  '                  FDINITRD=file initrd for the booted kernel'
 endef
 
 CLEAN_FILES += arch/x86/boot/fdimage \

[PATCH x86] [11/16] Compile apm and voyager module only when selected in Kconfig

[Andi Kleen]

Previously the complete files were #ifdef'ed, but now handle that in the 
Makefile.

May save a minor bit of compilation time.

Cc: hpa@rpath.com

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/Kconfig        |    5 +++++
 arch/x86/boot/Makefile  |    6 ++++--
 arch/x86/boot/apm.c     |    3 ---
 arch/x86/boot/voyager.c |    4 ----
 4 files changed, 9 insertions(+), 9 deletions(-)

Index: linux/arch/x86/Kconfig
===================================================================
--- linux.orig/arch/x86/Kconfig
+++ linux/arch/x86/Kconfig
@@ -1219,6 +1219,11 @@ source "kernel/power/Kconfig"
 
 source "drivers/acpi/Kconfig"
 
+config X86_APM_BOOT
+	bool
+	default y
+	depends on APM
+
 menuconfig APM
 	tristate "APM (Advanced Power Management) BIOS support"
 	depends on X86_32 && PM_SLEEP && !X86_VISWS
Index: linux/arch/x86/boot/Makefile
===================================================================
--- linux.orig/arch/x86/boot/Makefile
+++ linux/arch/x86/boot/Makefile
@@ -28,9 +28,11 @@ SVGA_MODE := -DSVGA_MODE=NORMAL_VGA
 targets		:= vmlinux.bin setup.bin setup.elf zImage bzImage
 subdir- 	:= compressed
 
-setup-y		+= a20.o apm.o cmdline.o copy.o cpu.o cpucheck.o edd.o
+setup-y		+= a20.o cmdline.o copy.o cpu.o cpucheck.o edd.o
 setup-y		+= header.o main.o mca.o memory.o pm.o pmjump.o
-setup-y		+= printf.o string.o tty.o video.o version.o voyager.o
+setup-y		+= printf.o string.o tty.o video.o version.o
+setup-$(CONFIG_X86_APM_BOOT) += apm.o
+setup-$(CONFIG_X86_VOYAGER) += voyager.o
 
 # The link order of the video-*.o modules can matter.  In particular,
 # video-vga.o *must* be listed first, followed by video-vesa.o.
Index: linux/arch/x86/boot/apm.c
===================================================================
--- linux.orig/arch/x86/boot/apm.c
+++ linux/arch/x86/boot/apm.c
@@ -19,8 +19,6 @@
 
 #include "boot.h"
 
-#if defined(CONFIG_APM) || defined(CONFIG_APM_MODULE)
-
 int query_apm_bios(void)
 {
 	u16 ax, bx, cx, dx, di;
@@ -95,4 +93,3 @@ int query_apm_bios(void)
 	return 0;
 }
 
-#endif
Index: linux/arch/x86/boot/voyager.c
===================================================================
--- linux.orig/arch/x86/boot/voyager.c
+++ linux/arch/x86/boot/voyager.c
@@ -16,8 +16,6 @@
 
 #include "boot.h"
 
-#ifdef CONFIG_X86_VOYAGER
-
 int query_voyager(void)
 {
 	u8 err;
@@ -42,5 +40,3 @@ int query_voyager(void)
 	copy_from_fs(data_ptr, di, 7);	/* Table is 7 bytes apparently */
 	return 0;
 }
-
-#endif /* CONFIG_X86_VOYAGER */

[PATCH x86] [12/16] Optimize lock prefix switching to run less frequently

[Andi Kleen]

On VMs implemented using JITs that cache translated code changing the lock 
prefixes is a quite costly operation that forces the JIT to throw away and 
retranslate a lot of code. 

Previously a SMP kernel would rewrite the locks once for each CPU which
is quite unnecessary. This patch changes the code to never switch at boot in
the normal case (SMP kernel booting with >1 CPU) or only once for SMP kernel 
on UP.

This makes a significant difference in boot up performance on AMD SimNow!
Also I expect it to be a little faster on native systems too because a smp 
switch does a lot of text_poke()s which each synchronize the pipeline.

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/kernel/alternative.c |   16 ++++++++++++++--
 include/linux/smp.h           |    2 ++
 init/main.c                   |    2 +-
 3 files changed, 17 insertions(+), 3 deletions(-)

Index: linux/arch/x86/kernel/alternative.c
===================================================================
--- linux.orig/arch/x86/kernel/alternative.c
+++ linux/arch/x86/kernel/alternative.c
@@ -273,6 +273,7 @@ struct smp_alt_module {
 };
 static LIST_HEAD(smp_alt_modules);
 static DEFINE_SPINLOCK(smp_alt);
+static int smp_mode = 1;	/* protected by smp_alt */
 
 void alternatives_smp_module_add(struct module *mod, char *name,
 				 void *locks, void *locks_end,
@@ -354,7 +355,14 @@ void alternatives_smp_switch(int smp)
 	BUG_ON(!smp && (num_online_cpus() > 1));
 
 	spin_lock_irqsave(&smp_alt, flags);
-	if (smp) {
+
+	/*
+ 	 * Avoid unnecessary switches because it forces JIT based VMs to
+	 * throw away all cached translations, which can be quite costly.
+	 */
+	if (smp == smp_mode) {
+		/* nothing */
+	} else if (smp) {
 		printk(KERN_INFO "SMP alternatives: switching to SMP code\n");
 		clear_cpu_cap(&boot_cpu_data, X86_FEATURE_UP);
 		clear_cpu_cap(&cpu_data(0), X86_FEATURE_UP);
@@ -369,6 +377,7 @@ void alternatives_smp_switch(int smp)
 			alternatives_smp_unlock(mod->locks, mod->locks_end,
 						mod->text, mod->text_end);
 	}
+	smp_mode = smp;
 	spin_unlock_irqrestore(&smp_alt, flags);
 }
 
@@ -441,7 +450,10 @@ void __init alternative_instructions(voi
 		alternatives_smp_module_add(NULL, "core kernel",
 					    __smp_locks, __smp_locks_end,
 					    _text, _etext);
-		alternatives_smp_switch(0);
+
+		/* Only switch to UP mode if we don't immediately boot others */
+		if (num_possible_cpus() == 1 || max_cpus == 0)
+			alternatives_smp_switch(0);
 	}
 #endif
  	apply_paravirt(__parainstructions, __parainstructions_end);
Index: linux/include/linux/smp.h
===================================================================
--- linux.orig/include/linux/smp.h
+++ linux/include/linux/smp.h
@@ -78,6 +78,8 @@ int on_each_cpu(void (*func) (void *info
  */
 void smp_prepare_boot_cpu(void);
 
+extern unsigned int max_cpus;
+
 #else /* !SMP */
 
 /*
Index: linux/init/main.c
===================================================================
--- linux.orig/init/main.c
+++ linux/init/main.c
@@ -128,7 +128,7 @@ static char *ramdisk_execute_command;
 
 #ifdef CONFIG_SMP
 /* Setup configured maximum number of CPUs to activate */
-static unsigned int __initdata max_cpus = NR_CPUS;
+unsigned int __initdata max_cpus = NR_CPUS;
 
 /*
  * Setup routine for controlling SMP activation

[PATCH x86] [13/16] i386: Set CFQ as default in i386 defconfig

[Andi Kleen]

Someone complained that the i386 defconfig contains AS as default IO
scheduler. Change that to CFQ.

Cc: axboe@kernel.dk

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/configs/i386_defconfig |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux/arch/x86/configs/i386_defconfig
===================================================================
--- linux.orig/arch/x86/configs/i386_defconfig
+++ linux/arch/x86/configs/i386_defconfig
@@ -99,9 +99,9 @@ CONFIG_IOSCHED_NOOP=y
 CONFIG_IOSCHED_AS=y
 CONFIG_IOSCHED_DEADLINE=y
 CONFIG_IOSCHED_CFQ=y
-CONFIG_DEFAULT_AS=y
+# CONFIG_DEFAULT_AS is not set
 # CONFIG_DEFAULT_DEADLINE is not set
-# CONFIG_DEFAULT_CFQ is not set
+CONFIG_DEFAULT_CFQ=y
 # CONFIG_DEFAULT_NOOP is not set
 CONFIG_DEFAULT_IOSCHED="anticipatory"
 

[PATCH x86] [14/16] Enable RDC321X subarch only on 32bit

[Andi Kleen]

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/Kconfig |    1 +
 1 file changed, 1 insertion(+)

Index: linux/arch/x86/Kconfig
===================================================================
--- linux.orig/arch/x86/Kconfig
+++ linux/arch/x86/Kconfig
@@ -295,6 +295,7 @@ config X86_RDC321X
 	select X86_REBOOTFIXUPS
 	select GENERIC_GPIO
 	select LEDS_GPIO
+	depends on X86_32
 	help
 	  This option is needed for RDC R-321x system-on-chip, also known
 	  as R-8610-(G).

[PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Andi Kleen]

This avoids the requirement to mark a lot of initialization functions not 
__cpuinit just for resume from RAM.

More functions could be converted now, didn't do all.

Cc: rjw@sisk.pl
Cc: pavel@suse.cz

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/Kconfig                |    5 +++++
 arch/x86/kernel/cpu/mtrr/main.c |    2 +-
 arch/x86/kernel/trampoline_64.S |    2 +-
 include/linux/init.h            |    2 +-
 4 files changed, 8 insertions(+), 3 deletions(-)

Index: linux/arch/x86/kernel/trampoline_64.S
===================================================================
--- linux.orig/arch/x86/kernel/trampoline_64.S
+++ linux/arch/x86/kernel/trampoline_64.S
@@ -34,7 +34,7 @@
 #include <asm/segment.h>
 
 /* We can free up trampoline after bootup if cpu hotplug is not supported. */
-#ifndef CONFIG_HOTPLUG_CPU
+#if !defined(CONFIG_HOTPLUG_CPU) && !defined(CONFIG_PM_CPUINIT)
 .section .init.data, "aw", @progbits
 #else
 .section .rodata, "a", @progbits
Index: linux/include/linux/init.h
===================================================================
--- linux.orig/include/linux/init.h
+++ linux/include/linux/init.h
@@ -266,7 +266,7 @@ void __init parse_early_param(void);
 #define __devexitdata __exitdata
 #endif
 
-#ifdef CONFIG_HOTPLUG_CPU
+#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_PM_CPUINIT)
 #define __cpuinit
 #define __cpuinitdata
 #define __cpuexit
Index: linux/arch/x86/Kconfig
===================================================================
--- linux.orig/arch/x86/Kconfig
+++ linux/arch/x86/Kconfig
@@ -130,6 +130,11 @@ config GENERIC_PENDING_IRQ
 	depends on GENERIC_HARDIRQS && SMP
 	default y
 
+config PM_CPUINIT
+	bool
+	depends on PM
+	default y
+
 config X86_SMP
 	bool
 	depends on SMP && ((X86_32 && !X86_VOYAGER) || X86_64)
Index: linux/arch/x86/kernel/cpu/mtrr/main.c
===================================================================
--- linux.orig/arch/x86/kernel/cpu/mtrr/main.c
+++ linux/arch/x86/kernel/cpu/mtrr/main.c
@@ -712,7 +712,7 @@ void __init mtrr_bp_init(void)
 	}
 }
 
-void mtrr_ap_init(void)
+void __cpuinit mtrr_ap_init(void)
 {
 	unsigned long flags;
 

[PATCH x86] [16/16] Mark memory_setup __init

[Andi Kleen]

Otherwise

WARNING: vmlinux.o(.text+0x64a9): Section mismatch: reference to .init.text:machine_specific_memory_setup (between 'memory_setup' and 'show_cpuinfo')

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/x86/kernel/setup_64.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/arch/x86/kernel/setup_64.c
===================================================================
--- linux.orig/arch/x86/kernel/setup_64.c
+++ linux/arch/x86/kernel/setup_64.c
@@ -244,7 +244,7 @@ static inline void __init reserve_crashk
 #endif
 
 /* Overridden in paravirt.c if CONFIG_PARAVIRT */
-void __attribute__((weak)) memory_setup(void)
+void __attribute__((weak)) __init memory_setup(void)
 {
        machine_specific_memory_setup();
 }

Re: [PATCH x86] [6/16] Add a new arch_early_alloc() interface for x86-64

[Eric Dumazet]

On Thu,  3 Jan 2008 16:42:20 +0100 (CET)
Andi Kleen <ak@suse.de> wrote:

> 
> This allows to allocate memory really early before bootmem is setup.
> 
> And a symbol that can be tested by the preprocessor.
> 
> pgtable.h is probably not the best include for it, but also not the worst.
> 
> Cc: peterz@infradead.org
> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> 
> ---
>  arch/x86/kernel/e820_64.c    |   14 ++++++++++++++
>  include/asm-x86/pgtable_64.h |    3 +++
>  2 files changed, 17 insertions(+)
> 
> Index: linux/arch/x86/kernel/e820_64.c
> ===================================================================
> --- linux.orig/arch/x86/kernel/e820_64.c
> +++ linux/arch/x86/kernel/e820_64.c
> @@ -819,3 +819,17 @@ int __init arch_get_ram_range(int slot, 
>  		max_pfn << PAGE_SHIFT) - *addr;
>  	return i + 1;
>  }
> +
> +#define EARLY_ALLOC_START (32<<20)
> +__init void *arch_early_alloc(unsigned long size)
> +{
> +	unsigned long p = find_e820_area(EARLY_ALLOC_START, -1UL, size);
> +	if (p == -1ULL) {
> +		/* Risk filling the DMA zone */
> +		p = find_e820_area(EARLY_ALLOC_START, -1UL, size);

Hum... Are you sure of this EARLY_ALLOC_START here ?

> +		if (p == -1ULL)
> +			panic("arch_early_alloc %lu failed", size);
> +	}
> +	reserve_early(p, p + size);
> +	return __va(p);
> +}

Re: [PATCH x86] [6/16] Add a new arch_early_alloc() interface for x86-64

[Andi Kleen]

> > +		/* Risk filling the DMA zone */
> > +		p = find_e820_area(EARLY_ALLOC_START, -1UL, size);
> 
> Hum... Are you sure of this EARLY_ALLOC_START here ?


Good catch. I'll repost with that fixed.

-Andi

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Rafael J. Wysocki]

On Thursday, 3 of January 2008, Andi Kleen wrote:
> 
> This avoids the requirement to mark a lot of initialization functions not 
> __cpuinit just for resume from RAM.
> 
> More functions could be converted now, didn't do all.
> 
> Cc: rjw@sisk.pl
> Cc: pavel@suse.cz
> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> 
> ---
>  arch/x86/Kconfig                |    5 +++++
>  arch/x86/kernel/cpu/mtrr/main.c |    2 +-
>  arch/x86/kernel/trampoline_64.S |    2 +-
>  include/linux/init.h            |    2 +-
>  4 files changed, 8 insertions(+), 3 deletions(-)
> 
> Index: linux/arch/x86/kernel/trampoline_64.S
> ===================================================================
> --- linux.orig/arch/x86/kernel/trampoline_64.S
> +++ linux/arch/x86/kernel/trampoline_64.S
> @@ -34,7 +34,7 @@
>  #include <asm/segment.h>
>  
>  /* We can free up trampoline after bootup if cpu hotplug is not supported. */
> -#ifndef CONFIG_HOTPLUG_CPU
> +#if !defined(CONFIG_HOTPLUG_CPU) && !defined(CONFIG_PM_CPUINIT)
>  .section .init.data, "aw", @progbits
>  #else
>  .section .rodata, "a", @progbits
> Index: linux/include/linux/init.h
> ===================================================================
> --- linux.orig/include/linux/init.h
> +++ linux/include/linux/init.h
> @@ -266,7 +266,7 @@ void __init parse_early_param(void);
>  #define __devexitdata __exitdata
>  #endif
>  
> -#ifdef CONFIG_HOTPLUG_CPU
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_PM_CPUINIT)
>  #define __cpuinit
>  #define __cpuinitdata
>  #define __cpuexit
> Index: linux/arch/x86/Kconfig
> ===================================================================
> --- linux.orig/arch/x86/Kconfig
> +++ linux/arch/x86/Kconfig
> @@ -130,6 +130,11 @@ config GENERIC_PENDING_IRQ
>  	depends on GENERIC_HARDIRQS && SMP
>  	default y
>  
> +config PM_CPUINIT
> +	bool
> +	depends on PM

Please make it PM_SLEEP (PM is more than suspend/hibernation).

> +	default y
> +
>  config X86_SMP
>  	bool
>  	depends on SMP && ((X86_32 && !X86_VOYAGER) || X86_64)
> Index: linux/arch/x86/kernel/cpu/mtrr/main.c
> ===================================================================
> --- linux.orig/arch/x86/kernel/cpu/mtrr/main.c
> +++ linux/arch/x86/kernel/cpu/mtrr/main.c
> @@ -712,7 +712,7 @@ void __init mtrr_bp_init(void)
>  	}
>  }
>  
> -void mtrr_ap_init(void)
> +void __cpuinit mtrr_ap_init(void)
>  {
>  	unsigned long flags;

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Andi Kleen]

> > +config PM_CPUINIT
> > +	bool
> > +	depends on PM
> 
> Please make it PM_SLEEP (PM is more than suspend/hibernation).

That was something that irritated me too while writing the patch, but the functions I 
am interested in with this  are referenced from arch/x86/power/cpu.c and that is

obj-$(CONFIG_PM)                += cpu.o

So you would need to fix that first. Would be fine for me, but is out of scope for
my patch.

-Andi

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Thu, Jan 03, 2008 at 04:42:29PM +0100, Andi Kleen wrote:
> 
> This avoids the requirement to mark a lot of initialization functions not 
> __cpuinit just for resume from RAM.
> 
> More functions could be converted now, didn't do all.
>...

Shouldn't this aready be handled by the following?

config PM_SLEEP_SMP
        bool
        depends on SUSPEND_SMP_POSSIBLE || HIBERNATION_SMP_POSSIBLE
        depends on PM_SLEEP
        select HOTPLUG_CPU
        default y


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Andi Kleen]

On Thursday 03 January 2008 19:14:38 Adrian Bunk wrote:
> On Thu, Jan 03, 2008 at 04:42:29PM +0100, Andi Kleen wrote:
> > 
> > This avoids the requirement to mark a lot of initialization functions not 
> > __cpuinit just for resume from RAM.
> > 
> > More functions could be converted now, didn't do all.
> >...
> 
> Shouldn't this aready be handled by the following?
> 
> config PM_SLEEP_SMP
>         bool
>         depends on SUSPEND_SMP_POSSIBLE || HIBERNATION_SMP_POSSIBLE
>         depends on PM_SLEEP
>         select HOTPLUG_CPU
>         default y

Won't help for UP at least. 

-Andi

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Rafael J. Wysocki]

On Thursday, 3 of January 2008, Andi Kleen wrote:
> 
> > > +config PM_CPUINIT
> > > +	bool
> > > +	depends on PM
> > 
> > Please make it PM_SLEEP (PM is more than suspend/hibernation).
> 
> That was something that irritated me too while writing the patch, but the functions I 
> am interested in with this  are referenced from arch/x86/power/cpu.c and that is
> 
> obj-$(CONFIG_PM)                += cpu.o
> 
> So you would need to fix that first. Would be fine for me, but is out of scope for
> my patch.

OK, I'll fix that up later.

Thanks,
Rafael

Re: [PATCH x86] [11/16] Compile apm and voyager module only when selected in Kconfig

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 585 bytes --]

On Thu,  3 Jan 2008 16:42:25 +0100 (CET) Andi Kleen <ak@suse.de> wrote:
>
> +++ linux/arch/x86/Kconfig
> @@ -1219,6 +1219,11 @@ source "kernel/power/Kconfig"
>  
>  source "drivers/acpi/Kconfig"
>  
> +config X86_APM_BOOT
> +	bool
> +	default y
> +	depends on APM

Does this want to be
	depends on APM || APM_MODULE

> +++ linux/arch/x86/boot/apm.c
> @@ -19,8 +19,6 @@
>  
>  #include "boot.h"
>  
> -#if defined(CONFIG_APM) || defined(CONFIG_APM_MODULE)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH x86] [1/16] Make clocksource watchdog cycle through online CPUs

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> This way it checks if the clocks are synchronized between CPUs too. 
> This might be able to detect slowly drifting TSCs which only go wrong 
> over longer time.

ok, makes sense - applied. Have you seen this trigger in practice?

	Ingo

Re: [PATCH x86] [2/16] Add a counter for per cpu clocksource watchdog checks and report them in /proc/interrupts

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> While infrequent it's better if all per CPU events have a counter.
> 
> Only done for x86 currently.

while i agree with exposing this counter, /proc/interrupts is the wrong 
place for this - please add it to /proc/timer_list instead, that's where 
we put all the time related debug info - it already has per cpu printout 
sections. And that way it's much simpler as well. (no need to modify all 
the architectures)

	Ingo

Re: [PATCH x86] [3/16] Turn irq debugging options into module params

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> This allows to change them at runtime using sysfs. No need to reboot 
> to set them.
> 
> I only added aliases (kernel.noirqdebug etc.) so the old options still 
> work.

good idea Andi, applied.

	Ingo

Re: [PATCH x86] [4/16] Add /proc/irq/*/spurious to dump the spurious irq debugging state

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> This is useful to debug problems with interrupt handlers that return 
> sometimes IRQ_NONE.

agreed.

A style error slipped in, please fix it:

> +static int irq_spurious_read(char *page, char **start, off_t off,
> +				  int count, int *eof, void *data)
> +{
> +	struct irq_desc *d = &irq_desc[(long) data];
> +	return sprintf(page, "count %u\n"
> +			     "unhandled %u\n"

	Ingo

Re: [PATCH x86] [10/16] Document fdimage/isoimage completely in make help

[Sam Ravnborg]

On Thu, Jan 03, 2008 at 04:42:24PM +0100, Andi Kleen wrote:
> 
> Add missing targets and missing options in x86 make help
> 
> Cc: hpa@rpath.com
> Cc: sam@ravnborg.org

Ingo - I assume you add this to x86.git.

PS. It looks like we have a tab embeded in the echo statements -
    we should kill the remaining uses if thats correct.

	Sam


> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> 
> ---
>  arch/x86/Makefile |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> Index: linux/arch/x86/Makefile
> ===================================================================
> --- linux.orig/arch/x86/Makefile
> +++ linux/arch/x86/Makefile
> @@ -247,9 +247,13 @@ define archhelp
>    echo  '		   (your) ~/bin/installkernel or'
>    echo  '		   (distribution) /sbin/installkernel or'
>    echo  '		   install to $$(INSTALL_PATH) and run lilo'
> -  echo  '  bzdisk       - Create a boot floppy in /dev/fd0'
> -  echo  '  fdimage      - Create a boot floppy image'
> -  echo  '  isoimage     - Create a boot CD-ROM image'
> +  echo  '  fdimage      - Create 1.4MB boot floppy image (arch/x86/boot/fdimage)'
> +  echo  '  fdimage144	- Create 1.4MB boot floppy image (arch/x86/boot/fdimage)'
> +  echo  '  fdimage288	- Create 2.8MB boot floppy image (arch/x86/boot/fdimage)'
> +  echo  '  isoimage     - Create a boot CD-ROM image (arch/x86/boot/image.iso)'
> +  echo  '                  bzdisk/fdimage*/isoimage also accept:'
> +  echo  '                  FDARGS="..."  arguments for the booted kernel'
> +  echo  '                  FDINITRD=file initrd for the booted kernel'
>  endef
>  
>  CLEAN_FILES += arch/x86/boot/fdimage \
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> On x86-64 there are several memory allocations before bootmem. To 
> avoid them stomping on each other they used to be all hard coded in 
> bad_area(). Replace this with an array that is filled as needed.
> 
> This cleans up the code considerably and allows to expand its use.

this code introduces a number of style errors that make the code harder 
to read and follow. Please fix them.

	Ingo

Re: [PATCH x86] [7/16] Convert lockdep to use arch_early_alloc() if available for its large arrays

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> +#ifndef ARCH_HAS_EARLY_ALLOC
> +#define LARGEVAR(x,y) { static typeof(*x) __ ## x[y];  x = __ ## x; }
> +#else
> +#define LARGEVAR(x,y) x = arch_early_alloc(sizeof(*x) * y)
> +#endif

your patch makes sense in principle, but please do this via the Kconfig 
space - we dont introduce new ARCH_HAS flags anymore and try to get rid 
of the existing ones as well.

	Ingo

Re: [PATCH x86] [8/16] Make lockdep_init __init

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> Make lockdep_init __init
> 
> Cc: peterz@infradead.org
> Signed-off-by: Andi Kleen <ak@suse.de>

thanks, applied the bits that did not depend on the early-reservations 
changes.

	Ingo

Re: [PATCH x86] [9/16] Remove CPU capabitilites printks on i386

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> I don't know of any case where they have been useful and they look 
> ugly.

yeah - and KERN_DEBUG is rarely useful anyway.

	Ingo

Re: [PATCH x86] [10/16] Document fdimage/isoimage completely in make help

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> On Thu, Jan 03, 2008 at 04:42:24PM +0100, Andi Kleen wrote:
> > 
> > Add missing targets and missing options in x86 make help
> > 
> > Cc: hpa@rpath.com
> > Cc: sam@ravnborg.org
> 
> Ingo - I assume you add this to x86.git.

yep, added it.

> PS. It looks like we have a tab embeded in the echo statements -
>     we should kill the remaining uses if thats correct.

yeah - fixed those.

	Ingo

Re: [PATCH x86] [13/16] i386: Set CFQ as default in i386 defconfig

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> Someone complained that the i386 defconfig contains AS as default IO 
> scheduler. Change that to CFQ.

thanks, applied.

	Ingo

Re: [PATCH x86] [14/16] Enable RDC321X subarch only on 32bit

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> @@ -295,6 +295,7 @@ config X86_RDC321X
>  	select X86_REBOOTFIXUPS
>  	select GENERIC_GPIO
>  	select LEDS_GPIO
> +	depends on X86_32

not applied - the RDC321X subarch already has that dependency:

config X86_RDC321X
        bool "RDC R-321x SoC"
        depends on X86_32

	Ingo

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> > So you would need to fix that first. Would be fine for me, but is 
> > out of scope for my patch.
> 
> OK, I'll fix that up later.

i'll apply Andi's patch to x86.git - or would like to maintain that 
patch?

	Ingo

Re: [PATCH x86] [16/16] Mark memory_setup __init

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> Otherwise
> 
> WARNING: vmlinux.o(.text+0x64a9): Section mismatch: reference to 
> .init.text:machine_specific_memory_setup (between 'memory_setup' and 
> 'show_cpuinfo')

thanks, applied.

	Ingo

Re: [PATCH x86] [12/16] Optimize lock prefix switching to run less frequently

[Thomas Gleixner]

On Thu, 3 Jan 2008, Andi Kleen wrote:
> This makes a significant difference in boot up performance on AMD SimNow!
> Also I expect it to be a little faster on native systems too because a smp 
> switch does a lot of text_poke()s which each synchronize the pipeline.

Please run your patches through checkpatch.pl.

ERROR: use tabs not spaces
#48: FILE: arch/x86/kernel/alternative.c:360:
 
> +
> +		/* Only switch to UP mode if we don't immediately boot others */
> +		if (num_possible_cpus() == 1 || max_cpus == 0)

Shouldn't this be max_cpus <= 1 ?

> +extern unsigned int max_cpus;

I'm a bit wary about making max_cpus global. max_cpus is used all over
the place as a local variable name. Can we please rename it to
setup_max_cpus or something like that?

Thanks,

	tglx

Re: [PATCH x86] [16/16] Mark memory_setup __init

[Ingo Molnar]

btw., as a sidenote, all of your recent patches to lkml had one or two 
missing Cc:s.

for lockdep patches, please Cc:

   Peter Zijlstra <a.p.zijlstra@chello.nl>
   Ingo Molnar <mingo@elte.hu>

for genirq patches, please Cc:

   Thomas Gleixner <tglx@linutronix.de>
   Ingo Molnar <mingo@elte.hu>

for timer/clocksource/clockevents patches please Cc:

   Thomas Gleixner <tglx@linutronix.de>
   John Stultz <johnstul@us.ibm.com>
   Ingo Molnar <mingo@elte.hu>

for x86 patches, please Cc:

   Thomas Gleixner <tglx@linutronix.de>
   "H. Peter Anvin" <hpa@zytor.com>
   Ingo Molnar <mingo@elte.hu>

having a complete Cc: line makes it easier to coordinate patches - 
especially for larger patchbombs. Thanks,

	Ingo

Re: [PATCH x86] [12/16] Optimize lock prefix switching to run less frequently

[Andi Kleen]

On Friday 04 January 2008 10:42:17 Thomas Gleixner wrote:
> On Thu, 3 Jan 2008, Andi Kleen wrote:
> > This makes a significant difference in boot up performance on AMD SimNow!
> > Also I expect it to be a little faster on native systems too because a smp 
> > switch does a lot of text_poke()s which each synchronize the pipeline.
> 
> Please run your patches through checkpatch.pl.
> 
> ERROR: use tabs not spaces
> #48: FILE: arch/x86/kernel/alternative.c:360:


I saw a lot of these warnings, but disregarded them as obviously silly. I don't
have plans to redo all the patches for that.
  
> > +
> > +		/* Only switch to UP mode if we don't immediately boot others */
> > +		if (num_possible_cpus() == 1 || max_cpus == 0)
> 
> Shouldn't this be max_cpus <= 1 ?

Don't think so, smp_init() seems to use it one off.
 
> > +extern unsigned int max_cpus;
> 
> I'm a bit wary about making max_cpus global. max_cpus is used all over
> the place as a local variable name. Can we please rename it to
> setup_max_cpus or something like that?

Hmm, I didn't see any warnings from this so surely it's not a big issue?

-Andi

Re: [PATCH x86] [4/16] Add /proc/irq/*/spurious to dump the spurious irq debugging state

[Andi Kleen]

ix it:

You mean the new line after local variables?

I never liked this rule. Newlines may make sense on very large functions, but
on short functions they just obscure more. And I actually checked CodingStyle now 
and it says nothing about that. So please apply the patch anyways.

-Andi

> > +static int irq_spurious_read(char *page, char **start, off_t off,
> > +				  int count, int *eof, void *data)
> > +{
> > +	struct irq_desc *d = &irq_desc[(long) data];
> > +	return sprintf(page, "count %u\n"
> > +			     "unhandled %u\n"
> 
> 	Ingo
> 

Re: [PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table

[Andi Kleen]

On Friday 04 January 2008 10:00:52 Ingo Molnar wrote:
> 
> * Andi Kleen <ak@suse.de> wrote:
> 
> > On x86-64 there are several memory allocations before bootmem. To 
> > avoid them stomping on each other they used to be all hard coded in 
> > bad_area(). Replace this with an array that is filled as needed.
> > 
> > This cleans up the code considerably and allows to expand its use.
> 
> this code introduces a number of style errors that make the code harder 
> to read and follow. Please fix them.

I don't see any style errors. Also I didn't realize that white space
was more important than seriously cleaner code now.

-Andi

Re: [PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table II

[Andi Kleen]

On Friday 04 January 2008 10:00:52 Ingo Molnar wrote:
> 
> * Andi Kleen <ak@suse.de> wrote:
> 
> > On x86-64 there are several memory allocations before bootmem. To 
> > avoid them stomping on each other they used to be all hard coded in 
> > bad_area(). Replace this with an array that is filled as needed.
> > 
> > This cleans up the code considerably and allows to expand its use.
> 
> this code introduces a number of style errors that make the code harder 
> to read and follow. Please fix them.

I reviewed the code now and I honestly don't see any "style errors" so I don't
know how to change it.

-Andi 

Re: [PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table

[Cyrill Gorcunov]

[Andi Kleen - Thu, Jan 03, 2008 at 04:42:18PM +0100]
| 
| On x86-64 there are several memory allocations before bootmem. To avoid
| them stomping on each other they used to be all hard coded in bad_area().
| Replace this with an array that is filled as needed.
| 
| This cleans up the code considerably and allows to expand its use.
| 
| Cc: peterz@infradead.org
| 
| Signed-off-by: Andi Kleen <ak@suse.de>
| 
| ---
|  arch/x86/kernel/e820_64.c  |   97 ++++++++++++++++++++++++---------------------
|  arch/x86/kernel/head64.c   |   48 ++++++++++++++++++++++
|  arch/x86/kernel/setup_64.c |   67 +------------------------------
|  arch/x86/mm/init_64.c      |    5 +-
|  arch/x86/mm/numa_64.c      |    1 
|  include/asm-x86/e820_64.h  |    5 +-
|  include/asm-x86/proto.h    |    2 
|  7 files changed, 112 insertions(+), 113 deletions(-)
| 
| Index: linux/arch/x86/kernel/e820_64.c
| ===================================================================
| --- linux.orig/arch/x86/kernel/e820_64.c
| +++ linux/arch/x86/kernel/e820_64.c
| @@ -47,56 +47,65 @@ unsigned long end_pfn_map;
|   */
|  static unsigned long __initdata end_user_pfn = MAXMEM>>PAGE_SHIFT;
|  
| -/* Check for some hardcoded bad areas that early boot is not allowed to touch */
| -static inline int bad_addr(unsigned long *addrp, unsigned long size)
| -{
| -	unsigned long addr = *addrp, last = addr + size;
| +/*
| + * Early reserved memory areas.
| + */
| +#define MAX_EARLY_RES 20
|  
| -	/* various gunk below that needed for SMP startup */
| -	if (addr < 0x8000) {
| -		*addrp = PAGE_ALIGN(0x8000);
| -		return 1;
| -	}
| +struct early_res {
| +	unsigned long start, end;
| +};
| +static struct early_res early_res[MAX_EARLY_RES] __initdata = {
| +	{ 0, PAGE_SIZE },			/* BIOS data page */
| +#ifdef CONFIG_SMP
| +	{ SMP_TRAMPOLINE_BASE, SMP_TRAMPOLINE_BASE + 2*PAGE_SIZE },
| +#endif
| +	{}
| +};
|  
| -	/* direct mapping tables of the kernel */
| -	if (last >= table_start<<PAGE_SHIFT && addr < table_end<<PAGE_SHIFT) {
| -		*addrp = PAGE_ALIGN(table_end << PAGE_SHIFT);
| -		return 1;
| +void __init reserve_early(unsigned long start, unsigned long end)
| +{
| +	int i;
| +	struct early_res *r;
| +	for (i = 0; i < MAX_EARLY_RES && early_res[i].end; i++) {
| +		r = &early_res[i];
| +		if (end > r->start && start < r->end)
| +			panic("Duplicated early reservation %lx-%lx\n",
| +			      start, end);
|  	}
| +	if (i >= MAX_EARLY_RES)
| +		panic("Too many early reservations");
| +	r = &early_res[i];
| +	r->start = start;
| +	r->end = end;
| +}
|  
| -	/* initrd */
| -#ifdef CONFIG_BLK_DEV_INITRD
| -	if (boot_params.hdr.type_of_loader && boot_params.hdr.ramdisk_image) {
| -		unsigned long ramdisk_image = boot_params.hdr.ramdisk_image;
| -		unsigned long ramdisk_size  = boot_params.hdr.ramdisk_size;
| -		unsigned long ramdisk_end   = ramdisk_image+ramdisk_size;
| +void __init early_res_to_bootmem(void)
| +{
| +	int i;
| +	for (i = 0; i < MAX_EARLY_RES && early_res[i].end; i++) {
| +	struct early_res *r = &early_res[i];
	^^^^^^
--->	just one tab used?

	Andi, it seems that is a point Ingo complained about?

| +		reserve_bootmem_generic(r->start, r->end - r->start);
| + 	}
| +}
|  
| -		if (last >= ramdisk_image && addr < ramdisk_end) {
| -			*addrp = PAGE_ALIGN(ramdisk_end);
| -			return 1;
| +/* Check for already reserved areas */
| +static inline int bad_addr(unsigned long *addrp, unsigned long size)
| +{
| +	int i;
| +	unsigned long addr = *addrp, last;
| +	int changed = 0;
| +again:
| +	last = addr + size;
| +	for (i = 0; i < MAX_EARLY_RES && early_res[i].end; i++) {
| +		struct early_res *r = &early_res[i];
| +		if (last >= r->start && addr < r->end) {
| +			*addrp = addr = r->end;
| +			changed = 1;
| +			goto again;
|  		}
| -	}
| -#endif
| -	/* kernel code */
| -	if (last >= __pa_symbol(&_text) && addr < __pa_symbol(&_end)) {
| -		*addrp = PAGE_ALIGN(__pa_symbol(&_end));
| -		return 1;
| -	}
| -
| -	if (last >= ebda_addr && addr < ebda_addr + ebda_size) {
| -		*addrp = PAGE_ALIGN(ebda_addr + ebda_size);
| -		return 1;
| -	}
| -
| -#ifdef CONFIG_NUMA
| -	/* NUMA memory to node map */
| -	if (last >= nodemap_addr && addr < nodemap_addr + nodemap_size) {
| -		*addrp = nodemap_addr + nodemap_size;
| -		return 1;
| -	}
| -#endif
| -	/* XXX ramdisk image here? */
| -	return 0;
| + 	}
| +	return changed;
|  }
|  
|  /*
| Index: linux/arch/x86/kernel/head64.c
| ===================================================================
| --- linux.orig/arch/x86/kernel/head64.c
| +++ linux/arch/x86/kernel/head64.c
| @@ -21,6 +21,7 @@
|  #include <asm/tlbflush.h>
|  #include <asm/sections.h>
|  #include <asm/kdebug.h>
| +#include <asm/e820.h>
|  
|  static void __init zap_identity_mappings(void)
|  {
| @@ -48,6 +49,35 @@ static void __init copy_bootdata(char *r
|  	}
|  }
|  
| +#define EBDA_ADDR_POINTER 0x40E
| +
| +static __init void reserve_ebda(void)
| +{
| +	unsigned ebda_addr, ebda_size;
| +
| +	/*
| +	 * there is a real-mode segmented pointer pointing to the
| +	 * 4K EBDA area at 0x40E
| +	 */
| +	ebda_addr = *(unsigned short *)__va(EBDA_ADDR_POINTER);
| +	ebda_addr <<= 4;
| +
| +	if (!ebda_addr)
| +		return;
| +
| +	ebda_size = *(unsigned short *)__va(ebda_addr);
| +
| +	/* Round EBDA up to pages */
| +	if (ebda_size == 0)
| +		ebda_size = 1;
| +	ebda_size <<= 10;
| +	ebda_size = round_up(ebda_size + (ebda_addr & ~PAGE_MASK), PAGE_SIZE);
| +	if (ebda_size > 64*1024)
| +		ebda_size = 64*1024;
| +
| +	reserve_early(ebda_addr, ebda_addr + ebda_size);
| +}
| +
|  void __init x86_64_start_kernel(char * real_mode_data)
|  {
|  	int i;
| @@ -70,5 +100,23 @@ void __init x86_64_start_kernel(char * r
|  	pda_init(0);
|  	copy_bootdata(__va(real_mode_data));
|  
| +	reserve_early(__pa_symbol(&_text), __pa_symbol(&_end));
| +
| +	/* Reserve INITRD */
| +	if (boot_params.hdr.type_of_loader && boot_params.hdr.ramdisk_image) {
| +		unsigned long ramdisk_image = boot_params.hdr.ramdisk_image;
| +		unsigned long ramdisk_size  = boot_params.hdr.ramdisk_size;
| +		unsigned long ramdisk_end   = ramdisk_image + ramdisk_size;
| +		reserve_early(ramdisk_image, ramdisk_end);
| +	}
| +
| +	reserve_ebda();
| +
| +	/*
| +	 * At this point everything still needed from the boot loader
| +	 * or BIOS or kernel text should be early reserved or marked not
| +	 * RAM in e820. All other memory is free game.
| +	 */
| +
|  	start_kernel();
|  }
| Index: linux/arch/x86/kernel/setup_64.c
| ===================================================================
| --- linux.orig/arch/x86/kernel/setup_64.c
| +++ linux/arch/x86/kernel/setup_64.c
| @@ -243,41 +243,6 @@ static inline void __init reserve_crashk
|  {}
|  #endif
|  
| -#define EBDA_ADDR_POINTER 0x40E
| -
| -unsigned __initdata ebda_addr;
| -unsigned __initdata ebda_size;
| -
| -static void discover_ebda(void)
| -{
| -	/*
| -	 * there is a real-mode segmented pointer pointing to the
| -	 * 4K EBDA area at 0x40E
| -	 */
| -	ebda_addr = *(unsigned short *)__va(EBDA_ADDR_POINTER);
| -	/*
| -	 * There can be some situations, like paravirtualized guests,
| -	 * in which there is no available ebda information. In such
| -	 * case, just skip it
| -	 */
| -	if (!ebda_addr) {
| -		ebda_size = 0;
| -		return;
| -	}
| -
| -	ebda_addr <<= 4;
| -
| -	ebda_size = *(unsigned short *)__va(ebda_addr);
| -
| -	/* Round EBDA up to pages */
| -	if (ebda_size == 0)
| -		ebda_size = 1;
| -	ebda_size <<= 10;
| -	ebda_size = round_up(ebda_size + (ebda_addr & ~PAGE_MASK), PAGE_SIZE);
| -	if (ebda_size > 64*1024)
| -		ebda_size = 64*1024;
| -}
| -
|  /* Overridden in paravirt.c if CONFIG_PARAVIRT */
|  void __attribute__((weak)) memory_setup(void)
|  {
| @@ -355,8 +320,6 @@ void __init setup_arch(char **cmdline_p)
|  
|  	check_efer();
|  
| -	discover_ebda();
| -
|  	init_memory_mapping(0, (end_pfn_map << PAGE_SHIFT));
|  	if (efi_enabled)
|  		efi_init();
| @@ -399,33 +362,7 @@ void __init setup_arch(char **cmdline_p)
|  	contig_initmem_init(0, end_pfn);
|  #endif
|  
| -	/* Reserve direct mapping */
| -	reserve_bootmem_generic(table_start << PAGE_SHIFT,
| -				(table_end - table_start) << PAGE_SHIFT);
| -
| -	/* reserve kernel */
| -	reserve_bootmem_generic(__pa_symbol(&_text),
| -				__pa_symbol(&_end) - __pa_symbol(&_text));
| -
| -	/*
| -	 * reserve physical page 0 - it's a special BIOS page on many boxes,
| -	 * enabling clean reboots, SMP operation, laptop functions.
| -	 */
| -	reserve_bootmem_generic(0, PAGE_SIZE);
| -
| -	/* reserve ebda region */
| -	if (ebda_addr)
| -		reserve_bootmem_generic(ebda_addr, ebda_size);
| -#ifdef CONFIG_NUMA
| -	/* reserve nodemap region */
| -	if (nodemap_addr)
| -		reserve_bootmem_generic(nodemap_addr, nodemap_size);
| -#endif
| -
| -#ifdef CONFIG_SMP
| -	/* Reserve SMP trampoline */
| -	reserve_bootmem_generic(SMP_TRAMPOLINE_BASE, 2*PAGE_SIZE);
| -#endif
| +	early_res_to_bootmem();
|  
|  #ifdef CONFIG_ACPI_SLEEP
|  	/*
| @@ -455,6 +392,8 @@ void __init setup_arch(char **cmdline_p)
|  			initrd_start = ramdisk_image + PAGE_OFFSET;
|  			initrd_end = initrd_start+ramdisk_size;
|  		} else {
| +			/* Assumes everything on node 0 */
| +			free_bootmem(ramdisk_image, ramdisk_size);
|  			printk(KERN_ERR "initrd extends beyond end of memory "
|  			       "(0x%08lx > 0x%08lx)\ndisabling initrd\n",
|  			       ramdisk_end, end_of_mem);
| Index: linux/arch/x86/mm/numa_64.c
| ===================================================================
| --- linux.orig/arch/x86/mm/numa_64.c
| +++ linux/arch/x86/mm/numa_64.c
| @@ -99,6 +99,7 @@ static int __init allocate_cachealigned_
|  	}
|  	pad_addr = (nodemap_addr + pad) & ~pad;
|  	memnodemap = phys_to_virt(pad_addr);
| +	reserve_early(nodemap_addr, nodemap_addr + nodemap_size);
|  
|  	printk(KERN_DEBUG "NUMA: Allocated memnodemap from %lx - %lx\n",
|  	       nodemap_addr, nodemap_addr + nodemap_size);
| Index: linux/include/asm-x86/e820_64.h
| ===================================================================
| --- linux.orig/include/asm-x86/e820_64.h
| +++ linux/include/asm-x86/e820_64.h
| @@ -36,8 +36,9 @@ extern void finish_e820_parsing(void);
|  
|  extern struct e820map e820;
|  
| -extern unsigned ebda_addr, ebda_size;
| -extern unsigned long nodemap_addr, nodemap_size;
| +extern void reserve_early(unsigned long start, unsigned long end);
| +extern void early_res_to_bootmem(void);
| +
|  #endif/*!__ASSEMBLY__*/
|  
|  #endif/*__E820_HEADER*/
| Index: linux/arch/x86/mm/init_64.c
| ===================================================================
| --- linux.orig/arch/x86/mm/init_64.c
| +++ linux/arch/x86/mm/init_64.c
| @@ -176,7 +176,8 @@ __set_fixmap (enum fixed_addresses idx, 
|  	set_pte_phys(address, phys, prot);
|  }
|  
| -unsigned long __meminitdata table_start, table_end;
| +static unsigned long __initdata table_start;
| +static unsigned long __meminitdata table_end;
|  
|  static __meminit void *alloc_low_page(unsigned long *phys)
|  { 
| @@ -387,6 +388,8 @@ void __init_refok init_memory_mapping(un
|  	if (!after_bootmem)
|  		mmu_cr4_features = read_cr4();
|  	__flush_tlb_all();
| +
| +	reserve_early(table_start << PAGE_SHIFT, table_end << PAGE_SHIFT);
|  }
|  
|  #ifndef CONFIG_NUMA
| Index: linux/include/asm-x86/proto.h
| ===================================================================
| --- linux.orig/include/asm-x86/proto.h
| +++ linux/include/asm-x86/proto.h
| @@ -22,8 +22,6 @@ extern void syscall32_cpu_init(void);
|  
|  extern void check_efer(void);
|  
| -extern unsigned long table_start, table_end;
| -
|  extern int reboot_force;
|  
|  long do_arch_prctl(struct task_struct *task, int code, unsigned long addr);
| --
| To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
| the body of a message to majordomo@vger.kernel.org
| More majordomo info at  http://vger.kernel.org/majordomo-info.html
| Please read the FAQ at  http://www.tux.org/lkml/
| 
		- Cyrill -

Re: [PATCH x86] [1/16] Make clocksource watchdog cycle through online CPUs

[Andi Kleen]

On Friday 04 January 2008 09:51:24 Ingo Molnar wrote:
> 
> * Andi Kleen <ak@suse.de> wrote:
> 
> > This way it checks if the clocks are synchronized between CPUs too. 
> > This might be able to detect slowly drifting TSCs which only go wrong 
> > over longer time.
> 
> ok, makes sense - applied. Have you seen this trigger in practice?

No, obviously because there are no per CPU timers currently.

-Andi

Re: [PATCH x86] [12/16] Optimize lock prefix switching to run less frequently

[Thomas Gleixner]

On Fri, 4 Jan 2008, Andi Kleen wrote:
> On Friday 04 January 2008 10:42:17 Thomas Gleixner wrote:
> > On Thu, 3 Jan 2008, Andi Kleen wrote:
> > > This makes a significant difference in boot up performance on AMD SimNow!
> > > Also I expect it to be a little faster on native systems too because a smp 
> > > switch does a lot of text_poke()s which each synchronize the pipeline.
> > 
> > Please run your patches through checkpatch.pl.
> > 
> > ERROR: use tabs not spaces
> > #48: FILE: arch/x86/kernel/alternative.c:360:
> 
> 
> I saw a lot of these warnings, but disregarded them as obviously silly. I don't
> have plans to redo all the patches for that.

Andi, this behaviour is obviously silly. 

I know that you do not care about white space and consistent coding
style, but others do.

The kernel process request that _all_ contributors run their patches
through checkpath.pl and fix the problems. The review process is the
same for _all_ contributors and I'm not going to add an extra Andi
attitude mode to it.

> > > +
> > > +		/* Only switch to UP mode if we don't immediately boot others */
> > > +		if (num_possible_cpus() == 1 || max_cpus == 0)
> > 
> > Shouldn't this be max_cpus <= 1 ?
> 
> Don't think so, smp_init() seems to use it one off.

It's not a question of what you think. It's a question of what the
code does and what the meaning of the command line parameter is:

/*
 * Setup routine for controlling SMP activation
 *
 * Command-line option of "nosmp" or "maxcpus=0" will disable SMP
 * activation entirely (the MPS table probe still happens, though).
 *
 * Command-line option of "maxcpus=<NUM>", where <NUM> is an integer
 * greater than 0, limits the maximum number of CPUs activated in
 * SMP mode to <NUM>.
 */

There is no "one off" use in smp_init():

	for_each_present_cpu(cpu) {
		if (num_online_cpus() >= max_cpus)
			break;

so the above check needs to be "max_cpus <= 1".

> > > +extern unsigned int max_cpus;
> > 
> > I'm a bit wary about making max_cpus global. max_cpus is used all over
> > the place as a local variable name. Can we please rename it to
> > setup_max_cpus or something like that?
> 
> Hmm, I didn't see any warnings from this so surely it's not a big issue?

It's not about warnings. It's about name spaces and it makes the
purpose of the global variable clear to the reader.

     tglx

Re: [PATCH x86] [12/16] Optimize lock prefix switching to run less frequently

[Andi Kleen]

> The kernel process request that _all_ contributors run their patches 
> through checkpath.pl and fix the problems.

That's new. When and by whom was that rule been introduced? And with what
rationale?

Even checkpatch.pl itself says to only fix the warnings that make sense.
This one didn't. And it has so many false positives in my experiences
that a lot of its warnings are useless.

So even if you require checkpatch.pl compliance it will be always
subjective because checkpatch.pl is not always correct.

Anyways if you care so much about space<->tabs why don't you just write
a filter that converts spaces to tabs for incoming patches? Like it is
traditionally done for trailing white spaces? Would be a trivial perl script.

Bothering humans with such a silly mindless thing is beyond ridiculous.

> The review process is the 
> same for _all_ contributors and I'm not going to add an extra Andi
> attitude mode to it.
 
I wish you guys would care more about actually broken logic in patches than just 
checkpatch.pl output. When I occasionally check git-x86 I always tend to find some 
obviously broken patch that should have been caught during review before
merge.

-Andi

Re: [PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table II

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> On Friday 04 January 2008 10:00:52 Ingo Molnar wrote:
> > 
> > * Andi Kleen <ak@suse.de> wrote:
> > 
> > > On x86-64 there are several memory allocations before bootmem. To 
> > > avoid them stomping on each other they used to be all hard coded in 
> > > bad_area(). Replace this with an array that is filled as needed.
> > > 
> > > This cleans up the code considerably and allows to expand its use.
> > 
> > this code introduces a number of style errors that make the code harder 
> > to read and follow. Please fix them.
> 
> I reviewed the code now and I honestly don't see any "style errors" so 
> I don't know how to change it.
> 
> -Andi 

there are 7 style problems in the patch. Checkpatch.pl gives two:

 ERROR: use tabs not spaces
 #92: FILE: arch/x86/kernel/e820_64.c:89:
 + ^I}$

 ERROR: use tabs not spaces
 #135: FILE: arch/x86/kernel/e820_64.c:107:
 + ^I}$

 total: 2 errors, 0 warnings, 308 lines checked

i'm not sure how you manage patches - if you have some 'refresh patch' 
command then you might want to stick a call to scripts/checkpatch.pl in 
there. I have such a check included in my quilt refresh shortcut.

#3:

 +       int i;
 +       struct early_res *r;
 +       for (i = 0; i < MAX_EARLY_RES && early_res[i].end; i++) {
 +               r = &early_res[i];

newline needed after the variables.

#4:

  void __init early_res_to_bootmem(void)
  {
          int i;
          for (i = 0; i < MAX_EARLY_RES && early_res[i].end; i++) {
          struct early_res *r = &early_res[i];
                  reserve_bootmem_generic(r->start, r->end - r->start);

the variables definition here is totally misaligned.

#5:

+       int changed = 0;
+again:
+       last = addr + size;

newline needed before the 'again:'.

#6:

+               struct early_res *r = &early_res[i];
+               if (last >= r->start && addr < r->end) {

newline needed after the variables.

#7:

+               unsigned long ramdisk_end   = ramdisk_image + ramdisk_size;
+               reserve_early(ramdisk_image, ramdisk_end);

newline needed after the variables.

from your other mail:

> [...] Also I didn't realize that white space was more important than 
> seriously cleaner code now.

here's my opinion about this topic:

- style cleanliness is an admittedly secondary concept but it 
  strengthens the most important, primary aspect of any code: 
  readability and maintainability.

- sloppy style is also often a statistical indicator for sloppy quality: 
  code written in rush, not reviewed and not read well enough. I'm not 
  implying anything like that about this patch of yours, but it's a 
  general policy and you'd sure agree that writing 100% clean code is 
  not that much harder than just writing good code.

- cleanup is generally pushed back to patch submitters - this 
  distributes better as it removes load from maintainers (and subsequent 
  patch developers).

- it's also a matter of visual taste. People are more likely to fix and 
  improve clean _looking_ code than messy looking code. It's often the 
  first stepping stone towards reaching structural cleanliness. And 
  frankly, rarely have i seen any genuinely clean code that had a messy 
  style - messy visual output is usually the mirror image of lack of 
  interest or lack of time. (i'd be glad if anyone could point me 
  towards any genuinely clean code within the kernel that nevertheless 
  has messy style.)

So please dont take this personal in any way - you'll see many other 
checkpatch.pl related patch rejections on lkml, from me and from others 
as well. Sloppy style does mount up step by step and results in harder 
to read and harder to fix/maintain code.

White space is not at all more important than seriously cleaner code, 
but 'even more seriously clean code' is more important than just pure 
seriously clean code ;-)

	Ingo

Re: [PATCH x86] [12/16] Optimize lock prefix switching to run less frequently

[Pekka Enberg]

Hi Andi,

On Jan 4, 2008 4:39 PM, Andi Kleen <ak@suse.de> wrote:
> > The kernel process request that _all_ contributors run their patches
> > through checkpath.pl and fix the problems.
>
> That's new. When and by whom was that rule been introduced? And with what
> rationale?

Not be new to anyone trying to sneak a patch past Andrew for the past
few months...

Re: [PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table II

[Andi Kleen]

>  ERROR: use tabs not spaces
>  #92: FILE: arch/x86/kernel/e820_64.c:89:
>  + ^I}$

Can't you just convert them using a script on new if you care so much about those? 

Ok I can write such a script too, but then every new contributor would need too which
will be a collective waste of time.
 
>  ERROR: use tabs not spaces
>  #135: FILE: arch/x86/kernel/e820_64.c:107:
>  + ^I}$
> 
>  total: 2 errors, 0 warnings, 308 lines checked
> 
> i'm not sure how you manage patches - if you have some 'refresh patch' 
> command then you might want to stick a call to scripts/checkpatch.pl in 
> there. I have such a check included in my quilt refresh shortcut.
> 
> #3:
> 
>  +       int i;
>  +       struct early_res *r;
>  +       for (i = 0; i < MAX_EARLY_RES && early_res[i].end; i++) {
>  +               r = &early_res[i];
> 
> newline needed after the variables.

That's an Ingo only rule I disagree with and is actually not in CodingStyle
When checkpatch.pl warns about it it is wrong and I would lobby for removing it.


> #4:
> 
>   void __init early_res_to_bootmem(void)
>   {
>           int i;
>           for (i = 0; i < MAX_EARLY_RES && early_res[i].end; i++) {
>           struct early_res *r = &early_res[i];
>                   reserve_bootmem_generic(r->start, r->end - r->start);
> 
> the variables definition here is totally misaligned.

Yes I fixed that now.

>
> - style cleanliness is an admittedly secondary concept but it 
>   strengthens the most important, primary aspect of any code: 
>   readability and maintainability.

In a minor way -- it is far less important than let's say good high level
comments or clear code flow. 

e.g. this patch imho is a major improvement in code clarity and logic
and makes previously quite fragile code much simpler and more approachable
and easier changeable (I can say this because I was responsible for the 
original mess -- only excuse is that it has grown over time). And then when 
people don't see these advantages and just talk about tabs<->spaces that is 
quite annoying.
 
> - sloppy style is also often a statistical indicator for sloppy quality: 
>   code written in rush, not reviewed and not read well enough. I'm not 
>   implying anything like that about this patch of yours, but it's a 
>   general policy and you'd sure agree that writing 100% clean code is 
>   not that much harder than just writing good code.

It depends. e.g. I personally consider the newline after variables rule
ugly for small functions and writing code for what I consider ugly 
style is harder. Also it was always my personal style to put
a variable near its first use in the same paragraph (imho that makes it clearer) so

	int foo;
	for (foo = ...; foo ; foo) { 
	} 

makes perfect sense to me.  And I don't really see a reason to change
that. 

I think all the cases you complained about were like this. Which means
you didn't actually look at the code logic, just at some abstract rule
that had nothing to do with the code flow. Which is bad.

I guess it's ok to add these newlines for larger functions and on those
you should have paragraphs anyways to separate logically separate
code blocks; but again that's not really something that can 

Same for tabs versus spaces. That is something no human should
need to care about. And it is something a machine can handle fine too 
anyways.

> - cleanup is generally pushed back to patch submitters - this 
>   distributes better as it removes load from maintainers (and subsequent 
>   patch developers).

When it makes sense. e.g. for trailing white space at least Andrew (and I,
don't know about you) always just removed those automatically on merge. No need to 
push a patch back for such a silly thing. Tabs versus Spaces is similar --
something a machine can do fine without any human assistance.

If you really feel strongly about those the right way is to remove
it from checkpatch.pl and just let the major maintainers run a script
to convert those on all new lines automatically.

Also I really dislike the recent flurry of checkpatch.pl
only patches on linux-kernel. Adding the whole file mode to it was a 
really bad idea. Style is only a minor incredient to the real
goal of good linux code and when people convert whole files everybody
who has outstanding patching against these files is forced to redo them
which is a large waste of time. Fixing style when something is changed
anyways is ok on the other hand because then other patches will reject
anyways. To borrow one of your favourite expressions it is is pissing
on the work of people who actually do non trivial changes to the tree.
 
I wish that disease would stop.

> - it's also a matter of visual taste. People are more likely to fix and 
>   improve clean _looking_ code than messy looking code.

Even more likely they are to improve code which is logically clean.
The best coding style doesn't help if that's not the case. Ok it's not totally
unimportant, but definitely not as much as you claim it to be. Also 
some minor coding style variants in the tree are not the end of the world
for anybody.

-Andi

Re: [PATCH x86] [12/16] Optimize lock prefix switching to run less frequently

[Andi Kleen]

On Friday 04 January 2008 16:02:37 Pekka Enberg wrote:
> Hi Andi,
> 
> On Jan 4, 2008 4:39 PM, Andi Kleen <ak@suse.de> wrote:
> > > The kernel process request that _all_ contributors run their patches
> > > through checkpath.pl and fix the problems.
> >
> > That's new. When and by whom was that rule been introduced? And with what
> > rationale?
> 
> Not be new to anyone trying to sneak a patch past Andrew for the past
> few months...

I got several in and I don't remember anybody complaining about this
before Ingo/Thomas.

-Andi

Re: [PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table II

[Sam Ravnborg]

> 
> In a minor way -- it is far less important than let's say good high level
> comments or clear code flow. 
> 
> e.g. this patch imho is a major improvement in code clarity and logic
> and makes previously quite fragile code much simpler and more approachable
> and easier changeable (I can say this because I was responsible for the 
> original mess -- only excuse is that it has grown over time). And then when 
> people don't see these advantages and just talk about tabs<->spaces that is 
> quite annoying.

It is great to have all the obvious style issues fixed no matter how much
the code clarity and logic is improved. For the simple reason that with
obvious coding style issues fixed you do not get distracted.

And I do not understand why people has any problem with that when we talk
about patches.

What is then considered "obvious style issues" are today implemented
as errors / warnings in checkpatch for the most part. Not everyone
agrees with what checkpatch says are errors/warnings but this is at
least an more or less unambigious way to express these obvious cases.
And this is far better than the maintainers choice of the day style.
 
> Same for tabs versus spaces. That is something no human should
> need to care about. And it is something a machine can handle fine too 
> anyways.
Things turn red in my vi if no-one cares - and I have no magic tool
to fix it here.
No-one should submit patches with white-space errors - thank you.

	Sam

Re: [PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table II

[Andi Kleen]

> What is then considered "obvious style issues" are today implemented
> as errors / warnings in checkpatch for the most part. Not everyone

In my experience checkpatch has a lot of false positives. e.g. when 
I run it over my pile 20+% of the warnings are just bogus. And in
a lot of other cases it is that the rules of thumbs it applies
do not apply to that specific case. 

e.g. example: when you have self test debugging code it is perfectly
fine to not use KERN_*

> agrees with what checkpatch says are errors/warnings but this is at
> least an more or less unambigious way to express these obvious cases.

It's not -- checkpatch is often wrong. 

> And this is far better than the maintainers choice of the day style.

You mean the one from Ingo with the random new lines? @) Perhaps.
I just ask that people use some common sense.

> > Same for tabs versus spaces. That is something no human should
> > need to care about. And it is something a machine can handle fine too 
> > anyways.
> Things turn red in my vi if no-one cares 

That's a problem of your vi. I'm sure it can be turned off.

Fortunately it's easily fixed.

>- and I have no magic tool
> to fix it here.

I wrote one now.

ftp://ftp.firstfloor.org/pub/ak/perl/converttabs

As you can see it was trivial.

> No-one should submit patches with white-space errors - thank you.

No I disagree. If white space is suddenly that important -- it is still
unclear to me why that should be -- then the right way to handle that is 
at the maintainer level while merging patches. But rejecting patches
for reasons that a trivial script can fix up is just unnecessary friction
in the merging process and unnecessary bureaucracy.

Unnecessary bureaucracy defined as in rituals that do not 
actually improve any code or prevent bugs (or even style). I must say I find 
it a little sad that Linux is dissolving into that now. Ok perhaps
it is the eventual fate of anything that has been around for quite a long time,
but it seems to get worse much quicker recently.

Anyways I have the script above included now here in my patch setup
scripts so future patches of mine will have tabs.

But the problem with your "rule" is that every of the hundreds
of Linux kernel contributors will need to do the same. And that's just 
ineffecient, waste of valuable human work and just doesn't make much sense.

It certainly won't improve Linux in any way.

-Andi

Re: [PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table II

[Joe Perches]

On Fri, 2008-01-04 at 19:06 +0100, Andi Kleen wrote:
> But the problem with your "rule" is that every of the hundreds
> of Linux kernel contributors will need to do the same. And that's just 
> ineffecient, waste of valuable human work and just doesn't make much sense.
> 
> It certainly won't improve Linux in any way.

What I would prefer is a git option that supports
check-in/outs in a coding style of a user choice.

Commits should be white space ignorant.

Re: [PATCH x86] [12/16] Optimize lock prefix switching to run less frequently

[Thomas Gleixner]

On Fri, 4 Jan 2008, Andi Kleen wrote:
> > The kernel process request that _all_ contributors run their patches 
> > through checkpath.pl and fix the problems.
> 
> That's new. When and by whom was that rule been introduced? And with what
> rationale?

Documentation/SubmitChecklist

> Even checkpatch.pl itself says to only fix the warnings that make sense.

This applies to warnings and not to errors and what I pointed out to
you was an error.

> Anyways if you care so much about space<->tabs why don't you just write
> a filter that converts spaces to tabs for incoming patches? Like it is
> traditionally done for trailing white spaces? Would be a trivial perl script.

I need to fix up your sloppiness ?

> Bothering humans with such a silly mindless thing is beyond ridiculous.

Sorry, but all the 65 other contributors to x86.git (and the other
1000 upstream contributors) are following the same rules. Most of them
just use the tools and provide clean patches upfront and none of them
ever complained about a polite request to fix things up which slipped
through.

One real reason why you should be special ? (Aside the reverse logic
of your argument: you are the only human and all others are silly
mindless hackers.)

The only thing which is beyond ridiculous is your absolute refusal to
play by the rules.

> > The review process is the 
> > same for _all_ contributors and I'm not going to add an extra Andi
> > attitude mode to it.
>  
> I wish you guys would care more about actually broken logic in patches than just 
> checkpatch.pl output. When I occasionally check git-x86 I always tend to find some 
> obviously broken patch that should have been caught during review before
> merge.

I just pointed out broken logic in your patch (vs. max_cpus) and your
answer is just sloppy hand waving ignoring my completely valid
point. After I pointed it out to you, you do not even have the modesty
of acknowledging your error. No, a second later you claim that we care
only about coding style and not the patch content itself.

Your findings of broken patches in the x86.git tree just prove that we
are all human beings, but at least the people responsible for the
x86.git tree are able to admit their mistakes and fix them without
arguing. On the other side I'm waiting since month for any response
from you on a review for a pile of obviously broken patches which you
wanted to push into 2.6.24.

Your hyprocritical crusade is annoying the hell out of me, but feel
free to ridicule yourself further.

    tglx

Re: [PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table II

[Sam Ravnborg]

On Fri, Jan 04, 2008 at 07:06:50PM +0100, Andi Kleen wrote:
> > What is then considered "obvious style issues" are today implemented
> > as errors / warnings in checkpatch for the most part. Not everyone
> 
> In my experience checkpatch has a lot of false positives. e.g. when 
> I run it over my pile 20+% of the warnings are just bogus. And in
> a lot of other cases it is that the rules of thumbs it applies
> do not apply to that specific case. 

Tried it on my pile of patches here.
ERROR: "foo * bar" should be "foo *bar"
#33: FILE: scripts/basic/docproc.c:61:
+FILELINE * docsection;

Legitimate error.

And a bunch of > 80 chars WARNINGS.
The warnings I ignore because I prefer readability on my 22" widescreen
but the ERROR should never had hit me.

But thats me that does not adhere to CodingStyle and checkpatch is correct.

> > No-one should submit patches with white-space errors - thank you.
> 
> No I disagree. If white space is suddenly that important -- it is still
> unclear to me why that should be -- then the right way to handle that is 
> at the maintainer level while merging patches. But rejecting patches
> for reasons that a trivial script can fix up is just unnecessary friction
> in the merging process and unnecessary bureaucracy.

Disagree. This is about a distributed model where the individual
submitters shall make life as easy as possible for the maintainer.
And the maintainer shall not be distracted by imporlerly formatted source
code that looks wird. It simple does not scale.
And no tool will help here because first review happens at the diff level
before applying any patch to any code base and the diff has thus never
seen a tool on the maintainer side.

> Unnecessary bureaucracy defined as in rituals that do not 
> actually improve any code or prevent bugs (or even style). I must say I find 
> it a little sad that Linux is dissolving into that now. Ok perhaps
> it is the eventual fate of anything that has been around for quite a long time,
> but it seems to get worse much quicker recently.
Several reasons why it has accelerated:
1) Ingo cares about checkpatch compliance
2) Ingo accepts patches fixing checkpatch compliance
3) We have a codebase that has lower quality than we usually praise us to have

I think it is fine to fix style issue when we do some major surgery in a file,
like unification.
But starting to randomly attack files all over the kernel is IMO to overdue it
and this is happening at the moment. And only carefull reviewing prevents bugs.

	Sam

Re: [PATCH x86] [12/16] Optimize lock prefix switching to run less frequently

[Andi Kleen]

On Friday 04 January 2008 21:34:15 Thomas Gleixner wrote:
> On Fri, 4 Jan 2008, Andi Kleen wrote:
> > > The kernel process request that _all_ contributors run their patches 
> > > through checkpath.pl and fix the problems.
> > 
> > That's new. When and by whom was that rule been introduced? And with what
> > rationale?
> 
> Documentation/SubmitChecklist

That says

" You should be able to justify all violations that remain in your patch."

I think I did that.

> > Anyways if you care so much about space<->tabs why don't you just write
> > a filter that converts spaces to tabs for incoming patches? Like it is
> > traditionally done for trailing white spaces? Would be a trivial perl script.
> 
> I need to fix up your sloppiness ?

That's the wrong way to see it.  

See it this way: Is forcing humans to convert spaces to tabs an useful activity?
Is rejecting patches for that and requiring repost a useful activity that 
improves Linux in any way? Will it help Linux to let people spend time
to convert spaces to tabs instead of writing patches or testing?

I don't think it is.  Arguably having tabs is nicer than spaces (I wont'
disagree with that).

But since it's something a simple script can do it's 
best to just handle it automatically. 
Then everybody can forget about that and it won't bother anymore again.

ftp://ftp.firstfloor.org/pub/ak/perl/converttabs

Anyways I could run converttabs over my pile and repost if you want,
but as pointed out elsewhere I think it would be better to just integrate
it into the merge flow -- then people could just forget about this forever.

Anyways my future patches will be always run through that.

> The only thing which is beyond ridiculous is your absolute refusal to
> play by the rules.

I question newly introduced rules that don't make sense to me. e.g the absolute
requirement of 100% checkpatch.pl compliance is certainly new.
 
> I just pointed out broken logic in your patch (vs. max_cpus) and your
> answer is just sloppy hand waving ignoring my completely valid
> point. After I pointed it out to you, you do not even have the modesty
> of acknowledging your error. 

I just fixed the patch instead. But you're right I was wrong on that.

> No, a second later you claim that we care 
> only about coding style and not the patch content itself.
> 
> Your findings of broken patches in the x86.git tree just prove that we
> are all human beings, 

Sure that's expected and I missed issues too when I was reviewing x86 patches
(the sentence came out perhaps a more harsh than originally intend) 

But my impression from reading the reviews was that a lot of the rejections
were based only on checkpatch.pl and not on logic checks and there were certainly
a few patches that clearly weren't logic reviewed.  If you do logic checks then that's 
fine of course -- feel free to prove me wrong on that front in the future.

To be honest I was also a little frustrated for patches that I consider
important cleanups (like the early-reserve code) I just get some checkpatch.pl 
complaints.

> but at least the people responsible for the 
> x86.git tree are able to admit their mistakes and fix them without
> arguing.

I fixed all objections that made sense, haven't reposted everything changed yet though.

> On the other side I'm waiting since month for any response 
> from you on a review for a pile of obviously broken patches which you
> wanted to push into 2.6.24.

What patches do you mean? I'm not of anything pending for .24.
 
> Your hyprocritical crusade 

I would prefer calling it "trying to improve inefficient newly introduced procedures
by constructive criticism" :-)

> is annoying the hell out of me, but feel 
> free to ridicule yourself further.

I'm sorry that you don't see my points. I guess I'll need to do a better job
at explaining them, but probably not tonight.

-Andi
 

Re: [PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table II

[Andi Kleen]

On Friday 04 January 2008 19:47:33 Joe Perches wrote:
> On Fri, 2008-01-04 at 19:06 +0100, Andi Kleen wrote:
> > But the problem with your "rule" is that every of the hundreds
> > of Linux kernel contributors will need to do the same. And that's just 
> > ineffecient, waste of valuable human work and just doesn't make much sense.
> > 
> > It certainly won't improve Linux in any way.
> 
> What I would prefer is a git option that supports
> check-in/outs in a coding style of a user choice.

I'm not very deep into git specifics, but for the spaces<->tab problem
I suspect some kind of commit hook script would be the correct solution.
i.e. always convert them in newly changed lines as comitted.

-Andi

Re: [PATCH x86] [5/16] Replace hard coded reservations in x86-64 early boot code with dynamic table II

[J. Bruce Fields]

On Fri, Jan 04, 2008 at 11:13:14PM +0100, Andi Kleen wrote:
> On Friday 04 January 2008 19:47:33 Joe Perches wrote:
> > On Fri, 2008-01-04 at 19:06 +0100, Andi Kleen wrote:
> > > But the problem with your "rule" is that every of the hundreds
> > > of Linux kernel contributors will need to do the same. And that's just 
> > > ineffecient, waste of valuable human work and just doesn't make much sense.
> > > 
> > > It certainly won't improve Linux in any way.
> > 
> > What I would prefer is a git option that supports
> > check-in/outs in a coding style of a user choice.
> 
> I'm not very deep into git specifics, but for the spaces<->tab problem
> I suspect some kind of commit hook script would be the correct solution.
> i.e. always convert them in newly changed lines as comitted.

If you've got the very latest git, after

	cd linux-2.6/
	echo "* whitespace" >.gitattributes

it'll treat any spaces in initial whitespace that could have been tabs
as bad whitespace, for the purposes of commands like git-am.  (So e.g.

	git-am --whitespace=fix mbox

will rewrite such stuff with tabs.)  For details search for whitespace
in the gitattributes, git-config, and git-apply man pages....

--b.

Re: [PATCH x86] [12/16] Optimize lock prefix switching to run less frequently

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> > > > The kernel process request that _all_ contributors run their 
> > > > patches through checkpath.pl and fix the problems.
> > > 
> > > That's new. When and by whom was that rule been introduced? And 
> > > with what rationale?
> > 
> > Documentation/SubmitChecklist
> 
> That says
> 
> " You should be able to justify all violations that remain in your patch."
> 
> I think I did that.

i dont think you did. A good number of bona fide style problems were 
pointed out in your patch and you werent even aware of them even after 
the fact was pointed out to you. In fact we are at silly mail #10 
already that argues about this.

I said in the very first mail that your patch looks good to me in 
principle, just please clean it up a bit. I also applied a lot of your 
other, clean patches. I dont know why you make such a big fuss about it, 
have you never been requested to clean up your code before? I too make 
cleanliness errors quite often and get asked to fix them up. That's what 
review cycles are about and it serves us well.

> > > Anyways if you care so much about space<->tabs why don't you just 
> > > write a filter that converts spaces to tabs for incoming patches? 
> > > Like it is traditionally done for trailing white spaces? Would be 
> > > a trivial perl script.
> > 
> > I need to fix up your sloppiness ?
> 
> That's the wrong way to see it.
> 
> See it this way: Is forcing humans to convert spaces to tabs an useful 
> activity? Is rejecting patches for that and requiring repost a useful 
> activity that improves Linux in any way? Will it help Linux to let 
> people spend time to convert spaces to tabs instead of writing patches 
> or testing?

the half dozen style problems i listed in your patch are very much not 
just about spaces to tabs, and the answer is a resounding: YES, fixing 
them helps Linux in general.

Why? Because the most important asset an outstanding programmer can have 
(besides the ability to abstract) is a good eye for small details.

It's _very_ easy for humans to slip over small details. Like the sign of 
a variable. Or parentheses in the wrong place. Or a missing check for 
NULL. Or implicit type conversions. Small details that a different 
system - like a computer - will _never_ get wrong.

And having a good eye for details is a VERY non-human ability - just 
like mathematics. Millions of years of evolution have taught our brain 
that all that matters is statistics and fuzzy rules - momentary details 
rarely matter in the physical world. Being able to pick an apple 80% of 
the time is plenty good as a survival tactic in the jungle. On the other 
hand, being correct 99.9% of the time in a computer program makes it 
virtually unusable most of the time.

Getting code alignment wrong, introducing various visible whitespace 
problems, getting convention wrong and thus making it harder for kernel 
developers to read each other's code shows a lack for attention to 
details - be that temporary or permanent inability. And lack of 
attention to details - even if it's only for a seemingly unimportant 
thing like coding style - is a disease that easily spreads to other 
areas such as the code's logic itself. Sloppy style is often an early 
indicator for sloppy logic.

There are many details that reviewers have to check, and if you sprinkle 
your code with other, irrelevant style mistakes then they will distract 
from the primary task: to understand and maintain the logic changes you 
do via your patch. Style errors can easily distract the human eye and 
can cause us to miss a much more serious error. In other words: style 
errors add random noise to the code and they thus make review of code 
more complex.

Sloppy and inconsistent code also leaves a lasting negative impression 
on newbie developers. I've heard quite a few opinions about Linux's code 
base being a lot less consistent (and hence harder to understand) than 
that of other OSs. I dont think it's a good policy for us to 
intentionally obfuscate our code and make it harder to read for 
everyone.

Frankly, if you dont understand the necessity of clean code, and the 
negative effect that a large number of small uncleanlinesses can have on 
a large system then you wont be getting the big picture either.

checkpatch.pl is not perfect (and it cannot be), and we dont use it as a 
rigid criterium (and dont want to), but it's quite conservative (its 
false positive rate for code that i maintain is below 1% - which is 
phenomenal for such a tool) and it is a pretty good tool at flagging 
sloppy patches. It gives us a way to improve the kernel's code quality 
gradually. We change about 10% of the kernel codebase in every kernel 
release, that is about 30% of the kernel every year. With checkpatch.pl 
we'll have a very clean kernel in just a few years.

And yes, the hardest task as usual is not to convince newbies and casual 
kernel developers, but to convince well-established kernel hackers who 
think they are perfect and write the cleanest code on the planet ;-) 
[and that list included me too] And yes, this too is a fundamentally 
human weakness ;-)

	Ingo

Re: [PATCH x86] [12/16] Optimize lock prefix switching to run less frequently

[Andi Kleen]

> Sloppy and inconsistent code also leaves a lasting negative impression 
> on newbie developers. I've heard quite a few opinions about Linux's code 
> base being a lot less consistent (and hence harder to understand) than 

Do you think they did refer to code style (as in indentation etc.) here? 
When I was talking to such people my impression was that their complaints
usually came from other issues.

e.g. I know from talking to a few people that they consider the BSD
kernels cleaner just because they have a very consistent printk
style for driver probing so it looks cleaner at booting. Sounds silly
(although I can sympathize a little bit) but true.

Besides the kernel indentation is not that inconsistent anyways.

> Frankly, if you dont understand the necessity of clean code, and the 
> negative effect that a large number of small uncleanlinesses can have on 
> a large system then you wont be getting the big picture either.
> 
> checkpatch.pl is not perfect (and it cannot be), and we dont use it as a 
> rigid criterium (and dont want to), but it's quite conservative (its 
> false positive rate for code that i maintain is below 1% - which is 

Rate is certainly higher here.

> sloppy patches. It gives us a way to improve the kernel's code quality 
> gradually. We change about 10% of the kernel codebase in every kernel 
> release, that is about 30% of the kernel every year. With checkpatch.pl 
> we'll have a very clean kernel in just a few years.

That's the problem I have with your position. You equate quality
with coding style or checkpatch.pl output. I think it's only a small
part of it.

If we follow your description above to the logical conclusion then we could 
get the perfect kernel(tm) very quickly just by running everything 
through Lindent or recruit a few hundred people who run all files 
through checkpatch.pl and fix all warnings and then we end up with the 
perfect kernel with "improved code quality" 

Great idea! And is it ok for me to refer to that plan as "the Ingo Molnar
plan" (TIMP) in the future? I'm sure a short term for it will come handy 
in future discussions ;-)

-Andi

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Thu, Jan 03, 2008 at 07:43:43PM +0100, Andi Kleen wrote:
> On Thursday 03 January 2008 19:14:38 Adrian Bunk wrote:
> > On Thu, Jan 03, 2008 at 04:42:29PM +0100, Andi Kleen wrote:
> > > 
> > > This avoids the requirement to mark a lot of initialization functions not 
> > > __cpuinit just for resume from RAM.
> > > 
> > > More functions could be converted now, didn't do all.
> > >...
> > 
> > Shouldn't this aready be handled by the following?
> > 
> > config PM_SLEEP_SMP
> >         bool
> >         depends on SUSPEND_SMP_POSSIBLE || HIBERNATION_SMP_POSSIBLE
> >         depends on PM_SLEEP
> >         select HOTPLUG_CPU
> >         default y
> 
> Won't help for UP at least. 

I know that it's not popular to care about the kernel size, but your 
patch will cost precious memory in the common case of UP embedded 
systems with CONFIG_PM=y.

It seems the correct solution would be not to hijack __cpuinit
(as your patch does), but to create a new annotation.

> -Andi

cu
Adrian

BTW: Is there any good reason why your patch is x86 only?
     No matter how this gets handled, it should be an architecture 
     independent issue.

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Andi Kleen]

> It seems the correct solution would be not to hijack __cpuinit
> (as your patch does), but to create a new annotation.

The rationale is that after suspend the CPU has to be reinitialized.
That is because it is essentially like a reboot. All the previous
CPU state is gone.

It doesn't need to touch state in memory only -- if you want
you could create a new annotation for functions that only touch
memory; but I'm not sure it would buy all that much.

-Andi

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Thu, Jan 10, 2008 at 10:58:57AM +0100, Andi Kleen wrote:
> > It seems the correct solution would be not to hijack __cpuinit
> > (as your patch does), but to create a new annotation.
> 
> The rationale is that after suspend the CPU has to be reinitialized.
> That is because it is essentially like a reboot. All the previous
> CPU state is gone.
>...

But your patch does:

+config PM_CPUINIT
+       bool
+       depends on PM
+       default y

As an example, even plain ACPI support without any suspend support in 
the kernel at all requires CONFIG_PM and therefore forces all __cpuinit 
code to be non-__init after your patch.

And if the dependency was corrected to PM_SLEEP it will still make 
the UP kernel use more memory since we currently have __cpuinit code 
that gets discarded after boot but suspend/resume is apparently working.

Plus my other point that it seems to be wrong to do whatever change only 
for x86.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Andi Kleen]

> But your patch does:
> 
> +config PM_CPUINIT
> +       bool
> +       depends on PM

That is because arch/x86/power/cpu.c where this happens is currently

obj-$(CONFIG_PM)                += cpu.o

If it was changed to CONFIG_something else then yes that dependency
should be changed too.

-Andi

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Thu, Jan 10, 2008 at 12:15:15PM +0100, Andi Kleen wrote:
> > But your patch does:
> > 
> > +config PM_CPUINIT
> > +       bool
> > +       depends on PM
> 
> That is because arch/x86/power/cpu.c where this happens is currently
> 
> obj-$(CONFIG_PM)                += cpu.o
> 
> If it was changed to CONFIG_something else then yes that dependency
> should be changed too.


Then fix this first.


And the following other points you didn't bother to reply to also still 
stand even after this fix:
- already __cpuinit code will waste memory with CONFIG_PM_SLEEP=y
- change shouldn't be x86 specific


> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Andi Kleen]

On Thursday 10 January 2008 12:26:07 Adrian Bunk wrote:
> On Thu, Jan 10, 2008 at 12:15:15PM +0100, Andi Kleen wrote:
> > > But your patch does:
> > > 
> > > +config PM_CPUINIT
> > > +       bool
> > > +       depends on PM
> > 
> > That is because arch/x86/power/cpu.c where this happens is currently
> > 
> > obj-$(CONFIG_PM)                += cpu.o
> > 
> > If it was changed to CONFIG_something else then yes that dependency
> > should be changed too.
> 
> 
> Then fix this first.

Rafael indicated he would do that, but it is really outside the scope
of my patch. I was just interested in fixing a linker warning.

> 
> And the following other points you didn't bother to reply to also still 
> stand even after this fix:
> - already __cpuinit code will waste memory with CONFIG_PM_SLEEP=y

Don't know what your point is. Anyways if you think there is a problem
somewhere please feel free to write patches.

> - change shouldn't be x86 specific

CPU initialization is deeply architecture specific. I don't see much use
in generalizing that.

-Andi

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Thu, Jan 10, 2008 at 12:42:53PM +0100, Andi Kleen wrote:
> On Thursday 10 January 2008 12:26:07 Adrian Bunk wrote:
> > On Thu, Jan 10, 2008 at 12:15:15PM +0100, Andi Kleen wrote:
> > > > But your patch does:
> > > > 
> > > > +config PM_CPUINIT
> > > > +       bool
> > > > +       depends on PM
> > > 
> > > That is because arch/x86/power/cpu.c where this happens is currently
> > > 
> > > obj-$(CONFIG_PM)                += cpu.o
> > > 
> > > If it was changed to CONFIG_something else then yes that dependency
> > > should be changed too.
> > 
> > 
> > Then fix this first.
> 
> Rafael indicated he would do that, but it is really outside the scope
> of my patch. I was just interested in fixing a linker warning.

Your patch description doesn't mention any linker warning.

Can you send the linker warning so that we can see the problem and not 
only the patch you wrote for fixing the undisclosed problem?

> > And the following other points you didn't bother to reply to also still 
> > stand even after this fix:
> > - already __cpuinit code will waste memory with CONFIG_PM_SLEEP=y
> 
> Don't know what your point is. Anyways if you think there is a problem
> somewhere please feel free to write patches.

Technically you are the one who has to deal with problems in your 
patches, not the people pointing at the problems.

> > - change shouldn't be x86 specific
> 
> CPU initialization is deeply architecture specific. I don't see much use
> in generalizing that.

That the code is architecture specific is clear.

But how to best annotate suspend and CPU hotplug code is a problem that 
is shared between many architectures and whose solution should not be 
architecture specific.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Andi Kleen]

Adrian Bunk <bunk@kernel.org> writes:
>
> Technically you are the one who has to deal with problems in your 
> patches, not the people pointing at the problems.

If you believe that my patch adds a new problem then please describe
it clearly so that I can understand it.

-Andi

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Thu, Jan 10, 2008 at 02:12:59PM +0100, Andi Kleen wrote:
> Adrian Bunk <bunk@kernel.org> writes:
> >
> > Technically you are the one who has to deal with problems in your 
> > patches, not the people pointing at the problems.
> 
> If you believe that my patch adds a new problem then please describe
> it clearly so that I can understand it.

Description:
- there are already __cpuinit* annotations in the kernel
- on UP kernels supporting suspend/resume, such annotated code
  currently gets freed after booting (and this works)
- with your patch applied, this code no longer gets freed

Whether or not this is a problem depends on whether you care about the 
memory used by the kernel or not...

My other issues with your patch were:
- whatever warning this patch was supposed to fixed was not shown
  (the patch description didn't mention any warning at all) - and
  understanding a fix gets easier when you know the problem it's
  supposed to fix
- this patch shouldn't be x86 specific since the issue how to 
  annotate suspend and CPU hotplug code isn't x86 specific

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Rafael J. Wysocki]

[Sorry for not replying earlier, I missed your message.]

On Friday, 4 of January 2008, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > > So you would need to fix that first. Would be fine for me, but is 
> > > out of scope for my patch.
> > 
> > OK, I'll fix that up later.
> 
> i'll apply Andi's patch to x86.git - or would like to maintain that 
> patch?

x86.git would be fine.

Thanks,
Rafael

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Rafael J. Wysocki]

On Thursday, 10 of January 2008, Andi Kleen wrote:
> On Thursday 10 January 2008 12:26:07 Adrian Bunk wrote:
> > On Thu, Jan 10, 2008 at 12:15:15PM +0100, Andi Kleen wrote:
> > > > But your patch does:
> > > > 
> > > > +config PM_CPUINIT
> > > > +       bool
> > > > +       depends on PM
> > > 
> > > That is because arch/x86/power/cpu.c where this happens is currently
> > > 
> > > obj-$(CONFIG_PM)                += cpu.o
> > > 
> > > If it was changed to CONFIG_something else then yes that dependency
> > > should be changed too.
> > 
> > 
> > Then fix this first.
> 
> Rafael indicated he would do that,

Yes, and I'm still going to do that. :-)

Thanks,
Rafael

[PATCH] x86: Change unnecessary dependencies on CONFIG_PM

[Rafael J. Wysocki]

[Ingo, this patch applies on top of the mm branch, please add.]
---
From: Rafael J. Wysocki <rjw@sisk.pl>

CONFIG_PM_CPUINIT should depend on CONFIG_PM_SLEEP rather than
on CONFIG_PM, because it only is needed for suspend and
hibernation.  Also, it's not necessary to compile
arch/x86/power/cpu.c if CONFIG_PM_SLEEP is not set, for the same
reason.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 arch/x86/Kconfig        |    2 +-
 arch/x86/power/Makefile |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/Kconfig
===================================================================
--- linux-2.6.orig/arch/x86/Kconfig
+++ linux-2.6/arch/x86/Kconfig
@@ -150,7 +150,7 @@ config GENERIC_PENDING_IRQ
 
 config PM_CPUINIT
 	bool
-	depends on PM
+	depends on PM_SLEEP
 	default y
 
 config X86_SMP
Index: linux-2.6/arch/x86/power/Makefile
===================================================================
--- linux-2.6.orig/arch/x86/power/Makefile
+++ linux-2.6/arch/x86/power/Makefile
@@ -1,2 +1,2 @@
-obj-$(CONFIG_PM)		+= cpu.o
+obj-$(CONFIG_PM_SLEEP)		+= cpu.o
 obj-$(CONFIG_HIBERNATION)	+= swsusp.o suspend.o

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Ingo Molnar]

* Adrian Bunk <bunk@kernel.org> wrote:

> On Thu, Jan 10, 2008 at 02:12:59PM +0100, Andi Kleen wrote:
> > Adrian Bunk <bunk@kernel.org> writes:
> > >
> > > Technically you are the one who has to deal with problems in your 
> > > patches, not the people pointing at the problems.
> > 
> > If you believe that my patch adds a new problem then please describe
> > it clearly so that I can understand it.
> 
> Description:
> - there are already __cpuinit* annotations in the kernel
> - on UP kernels supporting suspend/resume, such annotated code
>   currently gets freed after booting (and this works)
> - with your patch applied, this code no longer gets freed

ok, i've dropped this patch from x86.git for now:

 Subject: x86: force __cpuinit on for CONFIG_PM without HOTPLUG_CPU
 From: Andi Kleen <ak@suse.de>

but longer-term, shouldnt these annotations be automated? We'll see a 
constant stream of them, all around the clock as people regularly get it 
wrong (because it's not intuitive).

	Ingo

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Sam Ravnborg]

On Mon, Jan 14, 2008 at 02:52:40PM +0100, Ingo Molnar wrote:
> 
> * Adrian Bunk <bunk@kernel.org> wrote:
> 
> > On Thu, Jan 10, 2008 at 02:12:59PM +0100, Andi Kleen wrote:
> > > Adrian Bunk <bunk@kernel.org> writes:
> > > >
> > > > Technically you are the one who has to deal with problems in your 
> > > > patches, not the people pointing at the problems.
> > > 
> > > If you believe that my patch adds a new problem then please describe
> > > it clearly so that I can understand it.
> > 
> > Description:
> > - there are already __cpuinit* annotations in the kernel
> > - on UP kernels supporting suspend/resume, such annotated code
> >   currently gets freed after booting (and this works)
> > - with your patch applied, this code no longer gets freed
> 
> ok, i've dropped this patch from x86.git for now:
> 
>  Subject: x86: force __cpuinit on for CONFIG_PM without HOTPLUG_CPU
>  From: Andi Kleen <ak@suse.de>
> 
> but longer-term, shouldnt these annotations be automated? We'll see a 
> constant stream of them, all around the clock as people regularly get it 
> wrong (because it's not intuitive).
Would be great to have them automated - just dunno how to do it.
Do you see a feasible way to do it?

Short term modpost etc. will be enhanced to be less dependent on the actual
configuration when performing the checks.
It should not matter if CONFIG_HOTPLUG_CPU is enabled or not when we check
the __cpuint annotations but this is how we see it today so far
too many faults slip through.

	Sam

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> > ok, i've dropped this patch from x86.git for now:
> > 
> >  Subject: x86: force __cpuinit on for CONFIG_PM without HOTPLUG_CPU
> >  From: Andi Kleen <ak@suse.de>
> > 
> > but longer-term, shouldnt these annotations be automated? We'll see 
> > a constant stream of them, all around the clock as people regularly 
> > get it wrong (because it's not intuitive).
>
> Would be great to have them automated - just dunno how to do it. Do 
> you see a feasible way to do it?

a good starting point would be to make the warnings a lot more 
self-explanatory. Right now it's often non-obvious trying to figure out 
how the dependencies are structured and which one should be changed to 
get rid of the bug.

> Short term modpost etc. will be enhanced to be less dependent on the 
> actual configuration when performing the checks. It should not matter 
> if CONFIG_HOTPLUG_CPU is enabled or not when we check the __cpuint 
> annotations but this is how we see it today so far too many faults 
> slip through.

a good way i see is to:

 - improve the debug output so that it's obvious at a glance what the 
   problem is.

 - quickly reach a close-to-100%-perfect stage, brute-force. Drop 
   __init* annotations en masse if they are not perfectly layered. 
   Whoever reintroduces them will then have to do it perfectly.

 - then turn these into hard failures (which they _are_ in a significant 
   percentage of the situations).

once it's a hard build failure, people will fix them quickly and 
automated tests will pick them up as well.

	Ingo

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > Would be great to have them automated - just dunno how to do it. Do 
> > you see a feasible way to do it?
> 
> a good starting point would be to make the warnings a lot more 
> self-explanatory. Right now it's often non-obvious trying to figure 
> out how the dependencies are structured and which one should be 
> changed to get rid of the bug.

for example, in current -git, could you tell me why this triggers:

 WARNING: vmlinux.o(.text+0x87e2a): Section mismatch: reference to 
          .init.text: (between 'process_zones' and 'setup_per_cpu_pageset')

and how to resolve it? I had a quick look and it was not obvious to me.

	Ingo

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > Would be great to have them automated - just dunno how to do it. Do 
> > you see a feasible way to do it?
> 
> a good starting point would be to make the warnings a lot more 
> self-explanatory. Right now it's often non-obvious trying to figure 
> out how the dependencies are structured and which one should be 
> changed to get rid of the bug.

also, in theory we've got a pretty reliable set of the following 
information:

  function X references symbol Y

and we know what type of sections they are in, right?

could -ffunction-sections be used to delay the categorization of 
functions to the link stage, and in essence remove the need to mark 
functions via any of the __init markers?

	Ingo

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> also, in theory we've got a pretty reliable set of the following 
> information:
> 
>   function X references symbol Y
> 
> and we know what type of sections they are in, right?
> 
> could -ffunction-sections be used to delay the categorization of 
> functions to the link stage, and in essence remove the need to mark 
> functions via any of the __init markers?

find below the current set of warnings on -git. There are 62.

for example, instead of the rather cryptic:

  WARNING: vmlinux.o(.text+0x1815e): Section mismatch: reference to 
  .init.text:calibrate_delay (between 'smp_callin' and '__cpu_up')

the following output would be more informative:

--------------------------------->
| function:
|
|  ./init/calibrate.c:void __devinit calibrate_delay(void)
|
| calls:
|
|  ./arch/x86/kernel/smpboot_32.c:static void __cpuinit smp_callin(void)
|
| but calibrate_delay() is __devinit while smp_callin() is __cpuinit. 
| Change smp_callin() to __devinit to resolve this warning.
|-------------------------------->

would result in a lot faster cycles of fixing this.

do we have all the info to print this?

	Ingo

.init.text: calibrate_delay ('smp_callin' <-> '__cpu_up')
.init.text: register_cpu ('arch_register_cpu' <-> 'in_gate_area_no_task')
.init.text: idle_regs ('fork_idle' <-> 'get_task_mm')
.init.text: ('process_zones' <-> 'pageset_cpuup_callback')
.init.text: pcibios_fixup_bus ('pci_scan_child_bus' <-> 'pci_scan_bus_parented')
.init.data: mtrr ('param_set_scroll' <-> 'uvesafb_cn_callback')
.init.data: mtrr ('param_set_scroll' <-> 'uvesafb_cn_callback')
.init.data: pmi_setpal ('param_set_scroll' <-> 'uvesafb_cn_callback')
.init.data: pmi_setpal ('param_set_scroll' <-> 'uvesafb_cn_callback')
.init.text: pci_acpi_scan_root ('acpi_pci_root_add' <-> 'acpi_pci_unregister_driver')
.init.text: ('olympic_open' <-> 'olympic_interrupt')
.init.text: setup_teles3 ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_s0box ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_telespci ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_avm_pcipnp ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_elsa ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_diva ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_sedlbauer ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_netjet_s ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_hfcpci ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_hfcsx ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_niccy ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_bkm_a4t ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_sct_quadro ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_gazel ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_w6692 ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_netjet_u ('checkcard' <-> 'hisax_sched_event')
.init.text: setup_enternow_pci ('checkcard' <-> 'hisax_sched_event')
.init.data: ISACVer ('ISACVersion' <-> 'DC_Close_isac')
.init.text: clear_pending_isac_ints ('inithscxisac' <-> 'HscxVersion')
.init.text: initisac ('inithscxisac' <-> 'HscxVersion')
.init.text: clear_pending_isac_ints ('AVM_card_msg' <-> 'setup_avm_a1_pcmcia')
.init.text: setup_isac ('setup_avm_a1_pcmcia' <-> 'avm_a1p_interrupt')
.init.text: clear_pending_isac_ints ('AVM_card_msg' <-> 'ReadISACfifo_IPAC')
.init.text: initisac ('AVM_card_msg' <-> 'ReadISACfifo_IPAC')
.init.text: clear_pending_isac_ints ('Sedl_card_msg' <-> 'WriteISACfifo')
.init.text: initisac ('Sedl_card_msg' <-> 'WriteISACfifo')
.init.text: initisar ('Sedl_card_msg' <-> 'WriteISACfifo')
.init.text: clear_pending_isac_ints ('NETjet_S_card_msg' <-> 'netjet_s_interrupt')
.init.text: initisac ('NETjet_S_card_msg' <-> 'netjet_s_interrupt')
.init.text: clear_pending_isac_ints ('BKM_card_msg' <-> 'ReadISAC')
.init.text: initisac ('BKM_card_msg' <-> 'ReadISAC')
.init.text: Amd7930_init ('enpci_card_msg' <-> 'ReadWordAmd7930')
.init.text: Amd7930_init ('enpci_card_msg' <-> 'ReadWordAmd7930')
.init.text: snd_usb_caiaq_audio_init ('snd_probe' <-> 'usb_ep1_command_reply_dispatch')
.init.text: snd_usb_caiaq_midi_init ('snd_probe' <-> 'usb_ep1_command_reply_dispatch')
.init.data: _asc_def_iop_base ('advansys_isa_remove' <-> 'advansys_exit')
.init.text: suni_init ('__ksymtab_suni_init' <-> '__ksymtab_scsi_device_lookup')
.init.text: profile_cpu_callback ('profile_cpu_callback_nb.19579' <-> 'prof_cpu_mask')
.init.text: workqueue_cpu_callback ('workqueue_cpu_callback_nb.12756' <-> 'workqueue_mutex')
.init.text: cpu_callback ('cpu_callback_nb.23833' <-> 'shrinker_rwsem')
.init.text: tpm_inf_pnp_probe ('tpm_inf_pnp' <-> 'inf_attr_grp')
.init.data: prism2_plx_id_table ('prism2_plx_drv_id' <-> 'prism2_plx_funcs')
.init.text: asd_aic9410_setup ('asd_pcidev_data' <-> 'dev_attr_revision')
.init.text: asd_aic9410_setup ('asd_pcidev_data' <-> 'dev_attr_revision')
.init.text: asd_aic9405_setup ('asd_pcidev_data' <-> 'dev_attr_revision')
.init.text: megaraid_probe_one ('megaraid_pci_driver_g' <-> 'megaraid_template_g')
.init.text: snd_ad1889_probe ('ad1889_pci' <-> 'ops.23552')
.exit.text: ('qla2xxx_pci_error_detected' <-> 'qla2x00_stop_timer')
.exit.text: ('sym2_io_error_detected' <-> 'sym_set_cam_result_error')
.exit.text: tpm_nsc_remove ('init_nsc' <-> 'init_atmel')
.exit.text: ('asd_pci_probe' <-> 'ips_insert_device')

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Mon, Jan 14, 2008 at 02:52:40PM +0100, Ingo Molnar wrote:
>...
> but longer-term, shouldnt these annotations be automated? We'll see a 
> constant stream of them, all around the clock as people regularly get it 
> wrong (because it's not intuitive).

Definitely.

Even more when you looking at what Maciej Rozycki suggested regarding 
discardable strings. [1]

But _how_ can we get this automated at all?

> 	Ingo

cu
Adrian

[1] http://lkml.org/lkml/2007/10/12/297

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Mon, Jan 14, 2008 at 03:58:54PM +0100, Ingo Molnar wrote:
>...
> a good way i see is to:
>...
>  - quickly reach a close-to-100%-perfect stage, brute-force. Drop 
>    __init* annotations en masse if they are not perfectly layered. 
>    Whoever reintroduces them will then have to do it perfectly.
>...

First of all, this would really hurt space limited systems.

And "whoever reintroduces them" will most likely mean in practice that 
some janitors will go through the code and you as a maintainer will 
anyway be busy with getting them in shape.

Let's get them rightonce and we have a reasonable status quo.

> 	Ingo

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Mon, Jan 14, 2008 at 04:01:03PM +0100, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > > Would be great to have them automated - just dunno how to do it. Do 
> > > you see a feasible way to do it?
> > 
> > a good starting point would be to make the warnings a lot more 
> > self-explanatory. Right now it's often non-obvious trying to figure 
> > out how the dependencies are structured and which one should be 
> > changed to get rid of the bug.
> 
> for example, in current -git, could you tell me why this triggers:
> 
>  WARNING: vmlinux.o(.text+0x87e2a): Section mismatch: reference to 
>           .init.text: (between 'process_zones' and 'setup_per_cpu_pageset')
> 
> and how to resolve it? I had a quick look and it was not obvious to me.

Please send the .config.

> 	Ingo

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Ingo Molnar]

* Adrian Bunk <bunk@kernel.org> wrote:

> > for example, in current -git, could you tell me why this triggers:
> > 
> >  WARNING: vmlinux.o(.text+0x87e2a): Section mismatch: reference to 
> >           .init.text: (between 'process_zones' and 'setup_per_cpu_pageset')
> > 
> > and how to resolve it? I had a quick look and it was not obvious to me.
> 
> Please send the .config.

'make allyesconfig' and disable CONFIG_HOTPLUG=y.

	Ingo

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Mon, Jan 14, 2008 at 08:10:09PM +0100, Ingo Molnar wrote:
> 
> * Adrian Bunk <bunk@kernel.org> wrote:
> 
> > > for example, in current -git, could you tell me why this triggers:
> > > 
> > >  WARNING: vmlinux.o(.text+0x87e2a): Section mismatch: reference to 
> > >           .init.text: (between 'process_zones' and 'setup_per_cpu_pageset')
> > > 
> > > and how to resolve it? I had a quick look and it was not obvious to me.
> > 
> > Please send the .config.
> 
> 'make allyesconfig' and disable CONFIG_HOTPLUG=y.

Works for me without the warning.

Wait, I remember something @#$%#@$!!!:
'make allyesconfig' on x86 became ambiguously.

If you have by chance a 64bit CPU that would explain why I didn't get it.

@Sam:
Can you fix the bug that menuconfig does not let me enable CONFIG_64BIT?

> 	Ingo

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Sam Ravnborg]

On Mon, Jan 14, 2008 at 04:01:03PM +0100, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > > Would be great to have them automated - just dunno how to do it. Do 
> > > you see a feasible way to do it?
> > 
> > a good starting point would be to make the warnings a lot more 
> > self-explanatory. Right now it's often non-obvious trying to figure 
> > out how the dependencies are structured and which one should be 
> > changed to get rid of the bug.
> 
> for example, in current -git, could you tell me why this triggers:
> 
>  WARNING: vmlinux.o(.text+0x87e2a): Section mismatch: reference to 
>           .init.text: (between 'process_zones' and 'setup_per_cpu_pageset')
> 
> and how to resolve it? I had a quick look and it was not obvious to me.
I was confused by your error message - it looked all wrong.

process_zones is .text but setup_per_cpu_pageset is __init. I expect you
had some local changes.

So I tried myself and got this warning:
WARNING: mm/built-in.o(.text+0x6864): Section mismatch: reference to .init.text: (between 'process_zones' and 'pageset_cpuup_callback')

This made much more sense.
So I looked closely at process_zones() and the first
thing I always do is to check all the local functions.

I noticed that we use the function zone_batchsize() which is marked __devinit.
A function marked __cpuinit may use other functions marked __cpuinit, data marked
__cpuinitdata and .text/.data. But references to __devinit is not ok.

I furthermore noticed that zone_batchsize() were used in anohter
function marked __meminit.

So the simple fix for this warning is to remove the annotation of zone_batchsize.
It looks like a real opps candidate to me..

Why modpost did not pick up the zone_batchsize symbol is anohter matter.
It is present in the file:
$ objdump --syms vmlinux.o | grep zone_batchsize
0000000000016929 l     F .init.text     0000000000000053 zone_batchsize

Debugging modpost I could see that we had an addend value of 695,
but the addr of the symbol is 699. So somehow we point 4 bytes wrong.

Strange...

Anyway - here follows the patch.

	Sam

[PATCH] mm: fix section mismatch warning in page_alloc.c

With CONFIG_HOTPLUG=n and CONFIG_HOTPLUG_CPU=y we saw
following warning:
WARNING: mm/built-in.o(.text+0x6864): Section mismatch: reference to .init.text: (between 'process_zones' and 'pageset_cpuup_callback')

The culprint was zone_batchsize() which were annotated
__devinit but used from process_zones() which is annotated __cpuinit.
zone_batchsize() are used from another function annotated __meminit
so the only valid option is to drop the annotation of
zone_batchsize() so we know it is always valid to use it.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---


diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index e1028fa..b2838c2 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2566,7 +2566,7 @@ static void __meminit zone_init_free_lists(struct pglist_data *pgdat,
 	memmap_init_zone((size), (nid), (zone), (start_pfn), MEMMAP_EARLY)
 #endif
 
-static int __devinit zone_batchsize(struct zone *zone)
+static int zone_batchsize(struct zone *zone)
 {
 	int batch;
 

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Sam Ravnborg]

On Mon, Jan 14, 2008 at 09:52:58PM +0200, Adrian Bunk wrote:
> On Mon, Jan 14, 2008 at 08:10:09PM +0100, Ingo Molnar wrote:
> > 
> > * Adrian Bunk <bunk@kernel.org> wrote:
> > 
> > > > for example, in current -git, could you tell me why this triggers:
> > > > 
> > > >  WARNING: vmlinux.o(.text+0x87e2a): Section mismatch: reference to 
> > > >           .init.text: (between 'process_zones' and 'setup_per_cpu_pageset')
> > > > 
> > > > and how to resolve it? I had a quick look and it was not obvious to me.
> > > 
> > > Please send the .config.
> > 
> > 'make allyesconfig' and disable CONFIG_HOTPLUG=y.
> 
> Works for me without the warning.
> 
> Wait, I remember something @#$%#@$!!!:
> 'make allyesconfig' on x86 became ambiguously.
> 
> If you have by chance a 64bit CPU that would explain why I didn't get it.
> 
> @Sam:
> Can you fix the bug that menuconfig does not let me enable CONFIG_64BIT?

make ARCH=x86_64 allyesconfig
will set CONFIG_64BIT for you - no?

	Sam

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Sam Ravnborg]

On Mon, Jan 14, 2008 at 04:24:40PM +0100, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > also, in theory we've got a pretty reliable set of the following 
> > information:
> > 
> >   function X references symbol Y
> > 
> > and we know what type of sections they are in, right?
> > 
> > could -ffunction-sections be used to delay the categorization of 
> > functions to the link stage, and in essence remove the need to mark 
> > functions via any of the __init markers?
> 
> find below the current set of warnings on -git. There are 62.

The correct figure is 112.

You need to do a:
make KCFLAGS=-fno-unit-at-a-time
build to see them all.

> 
> for example, instead of the rather cryptic:
> 
>   WARNING: vmlinux.o(.text+0x1815e): Section mismatch: reference to 
>   .init.text:calibrate_delay (between 'smp_callin' and '__cpu_up')
> 
> the following output would be more informative:
> 
> --------------------------------->
> | function:
> |
> |  ./init/calibrate.c:void __devinit calibrate_delay(void)
> |
> | calls:
> |
> |  ./arch/x86/kernel/smpboot_32.c:static void __cpuinit smp_callin(void)
> |
> | but calibrate_delay() is __devinit while smp_callin() is __cpuinit. 
> | Change smp_callin() to __devinit to resolve this warning.
> |-------------------------------->
> 
> would result in a lot faster cycles of fixing this.
> 
> do we have all the info to print this?

Not yet. The patch I posted on lkml bring us one step further.
At modpost time today we only see .init.text and .text but
with the suggested patch we will be able to see what section
a function belong to and we are several steps closer to better
error messages.

So let me get that into shape and I will revisit the messages.

In this case I would not know if calibrate_delay() should be
__cpuinit or smp_callin() should be __devinit looking at the
information modpost has available without additional digging.
But a quick grep tells me that the right fix is to annotate
calibrate_delay() with __cpuinit because is is used from
either __cpuinit annotated or __init annotated functions.

	Sam

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Mon, Jan 14, 2008 at 09:01:26PM +0100, Sam Ravnborg wrote:
> On Mon, Jan 14, 2008 at 09:52:58PM +0200, Adrian Bunk wrote:
> > On Mon, Jan 14, 2008 at 08:10:09PM +0100, Ingo Molnar wrote:
> > > 
> > > * Adrian Bunk <bunk@kernel.org> wrote:
> > > 
> > > > > for example, in current -git, could you tell me why this triggers:
> > > > > 
> > > > >  WARNING: vmlinux.o(.text+0x87e2a): Section mismatch: reference to 
> > > > >           .init.text: (between 'process_zones' and 'setup_per_cpu_pageset')
> > > > > 
> > > > > and how to resolve it? I had a quick look and it was not obvious to me.
> > > > 
> > > > Please send the .config.
> > > 
> > > 'make allyesconfig' and disable CONFIG_HOTPLUG=y.
> > 
> > Works for me without the warning.
> > 
> > Wait, I remember something @#$%#@$!!!:
> > 'make allyesconfig' on x86 became ambiguously.
> > 
> > If you have by chance a 64bit CPU that would explain why I didn't get it.
> > 
> > @Sam:
> > Can you fix the bug that menuconfig does not let me enable CONFIG_64BIT?
> 
> make ARCH=x86_64 allyesconfig
> will set CONFIG_64BIT for you - no?

Yes.

But this still leaves the fact that when someone says 'allyesconfig' 
it's no longer clear which configuration he has. And no, I do not 
consider it funny that this now implies two hours of compilation twice 
just for seeing one bug.

And the fact that I have to set ARCH only for seeing the 32/64bit 
Kconfig choice is clearly bullshit.

> 	Sam

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Sam Ravnborg]

> > > Can you fix the bug that menuconfig does not let me enable CONFIG_64BIT?
> > 
> > make ARCH=x86_64 allyesconfig
> > will set CONFIG_64BIT for you - no?
> 
> Yes.
> 
> But this still leaves the fact that when someone says 'allyesconfig' 
> it's no longer clear which configuration he has.
Assuming you are on an x86 box...

make allyesconfig will give you a config for same OS bit-size as you run.
make ARCH=i386 allyesconfig gives you a 32 bit config
make ARCH=x86_64 allyesconfig gives you a 64 bit config

make ARCH=x86 allyesconfig gives you a 32 bit config where you can select 64 bit.

It was seen as the best solution when unifying 32 and 64 bit stuff
and introducing support for ARCH=x86.

No-one has complained until now so most people seems to get it
or maybe they are just silent.

	Sam

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Mon, Jan 14, 2008 at 09:27:40PM +0100, Sam Ravnborg wrote:
> > > > Can you fix the bug that menuconfig does not let me enable CONFIG_64BIT?
> > > 
> > > make ARCH=x86_64 allyesconfig
> > > will set CONFIG_64BIT for you - no?
> > 
> > Yes.
> > 
> > But this still leaves the fact that when someone says 'allyesconfig' 
> > it's no longer clear which configuration he has.
> Assuming you are on an x86 box...
> 
> make allyesconfig will give you a config for same OS bit-size as you run.
> make ARCH=i386 allyesconfig gives you a 32 bit config
> make ARCH=x86_64 allyesconfig gives you a 64 bit config
> 
> make ARCH=x86 allyesconfig gives you a 32 bit config where you can select 64 bit.
> 
> It was seen as the best solution when unifying 32 and 64 bit stuff
> and introducing support for ARCH=x86.
> 
> No-one has complained until now so most people seems to get it
> or maybe they are just silent.

Or I'm the only person doing kernel development on a 32bit x86 machine?

Even oldconfig is busted since the Kconfig choice is not available 
without explicitely setting ARCH.

> 	Sam

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> > find below the current set of warnings on -git. There are 62.
> 
> The correct figure is 112.
> 
> You need to do a:
> make KCFLAGS=-fno-unit-at-a-time
> build to see them all.

btw., please add a .config option to trigger the -fno-unit-at-a-time 
flags. Something like CONFIG_SECTION_ERRORS=y - plus perhaps combine it 
with the patch below that turns such section bugs into detectable build 
errors. A distro does not want to build a kernel that could potentially 
corrupt kernel memory. (it's a security risk as well.) If we make the 
err=1 dependent on CONFIG_SECTION_ERRORS then we'll have this 
configurable.

	Ingo

---------------->
Subject: x86: link mismatch error
From: Ingo Molnar <mingo@elte.hu>

turn the build warning into a build error.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 scripts/mod/modpost.c |   63 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 24 deletions(-)

Index: linux/scripts/mod/modpost.c
===================================================================
--- linux.orig/scripts/mod/modpost.c
+++ linux/scripts/mod/modpost.c
@@ -863,8 +863,8 @@ static void find_symbols_between(struct 
  * Try to find symbols near it so user can find it.
  * Check whitelist before warning - it may be a false positive.
  **/
-static void warn_sec_mismatch(const char *modname, const char *fromsec,
-			      struct elf_info *elf, Elf_Sym *sym, Elf_Rela r)
+static int error_sec_mismatch(const char *modname, const char *fromsec,
+			     struct elf_info *elf, Elf_Sym *sym, Elf_Rela r)
 {
 	const char *refsymname = "";
 	Elf_Sym *before, *after;
@@ -874,6 +874,7 @@ static void warn_sec_mismatch(const char
 	const char *secstrings = (void *)hdr +
 				 sechdrs[hdr->e_shstrndx].sh_offset;
 	const char *secname = secstrings + sechdrs[sym->st_shndx].sh_name;
+	int err = 0;
 
 	find_symbols_between(elf, r.r_offset, fromsec, &before, &after);
 
@@ -885,32 +886,38 @@ static void warn_sec_mismatch(const char
 	if (secref_whitelist(modname, secname, fromsec,
 			     before ? elf->strtab + before->st_name : "",
 	                     refsymname))
-		return;
+		goto out;
 
 	if (before && after) {
-		warn("%s(%s+0x%llx): Section mismatch: reference to %s:%s "
+		merror("%s(%s+0x%llx): Section mismatch: reference to %s:%s "
 		     "(between '%s' and '%s')\n",
 		     modname, fromsec, (unsigned long long)r.r_offset,
 		     secname, refsymname,
 		     elf->strtab + before->st_name,
 		     elf->strtab + after->st_name);
+		err = 1;
 	} else if (before) {
-		warn("%s(%s+0x%llx): Section mismatch: reference to %s:%s "
+		merror("%s(%s+0x%llx): Section mismatch: reference to %s:%s "
 		     "(after '%s')\n",
 		     modname, fromsec, (unsigned long long)r.r_offset,
 		     secname, refsymname,
 		     elf->strtab + before->st_name);
+		err = 1;
 	} else if (after) {
-		warn("%s(%s+0x%llx): Section mismatch: reference to %s:%s "
+		merror("%s(%s+0x%llx): Section mismatch: reference to %s:%s "
 		     "before '%s' (at offset -0x%llx)\n",
 		     modname, fromsec, (unsigned long long)r.r_offset,
 		     secname, refsymname,
 		     elf->strtab + after->st_name);
+		err = 1;
 	} else {
-		warn("%s(%s+0x%llx): Section mismatch: reference to %s:%s\n",
+		merror("%s(%s+0x%llx): Section mismatch: reference to %s:%s\n",
 		     modname, fromsec, (unsigned long long)r.r_offset,
 		     secname, refsymname);
+		err = 1;
 	}
+out:
+	return err;
 }
 
 static unsigned int *reloc_location(struct elf_info *elf,
@@ -997,10 +1004,10 @@ static int addend_mips_rel(struct elf_in
  * to find all references to a section that reference a section that will
  * be discarded and warns about it.
  **/
-static void check_sec_ref(struct module *mod, const char *modname,
-			  struct elf_info *elf,
-			  int section(const char*),
-			  int section_ref_ok(const char *))
+static int check_sec_ref(struct module *mod, const char *modname,
+			 struct elf_info *elf,
+			 int section(const char*),
+			 int section_ref_ok(const char *))
 {
 	int i;
 	Elf_Sym  *sym;
@@ -1049,9 +1056,11 @@ static void check_sec_ref(struct module 
 
 				secname = secstrings +
 					sechdrs[sym->st_shndx].sh_name;
-				if (section(secname))
-					warn_sec_mismatch(modname, name,
-							  elf, sym, r);
+				if (section(secname)) {
+					if (error_sec_mismatch(modname, name,
+							  elf, sym, r))
+						return 1;
+				}
 			}
 		} else if (sechdrs[i].sh_type == SHT_REL) {
 			Elf_Rel *rel;
@@ -1100,12 +1109,15 @@ static void check_sec_ref(struct module 
 
 				secname = secstrings +
 					sechdrs[sym->st_shndx].sh_name;
-				if (section(secname))
-					warn_sec_mismatch(modname, name,
-							  elf, sym, r);
+				if (section(secname)) {
+					if (error_sec_mismatch(modname, name,
+							  elf, sym, r))
+						return 1;
+				}
 			}
 		}
 	}
+	return 0;
 }
 
 /*
@@ -1249,7 +1261,7 @@ static int exit_section_ref_ok(const cha
 	return 0;
 }
 
-static void read_symbols(char *modname)
+static int read_symbols(char *modname)
 {
 	const char *symname;
 	char *version;
@@ -1257,9 +1269,10 @@ static void read_symbols(char *modname)
 	struct module *mod;
 	struct elf_info info = { };
 	Elf_Sym *sym;
+	int err = 0;
 
 	if (!parse_elf(&info, modname))
-		return;
+		goto out;
 
 	mod = new_module(modname);
 
@@ -1289,8 +1302,8 @@ static void read_symbols(char *modname)
 		handle_moddevtable(mod, &info, sym, symname);
 	}
 	if (is_vmlinux(modname) && vmlinux_section_warnings) {
-		check_sec_ref(mod, modname, &info, init_section, init_section_ref_ok);
-		check_sec_ref(mod, modname, &info, exit_section, exit_section_ref_ok);
+		err |= check_sec_ref(mod, modname, &info, init_section, init_section_ref_ok);
+		err |= check_sec_ref(mod, modname, &info, exit_section, exit_section_ref_ok);
 	}
 
 	version = get_modinfo(info.modinfo, info.modinfo_len, "version");
@@ -1309,6 +1322,8 @@ static void read_symbols(char *modname)
 	 * important anyhow */
 	if (modversions)
 		mod->unres = alloc_symbol("struct_module", 0, mod->unres);
+out:
+	return err;
 }
 
 #define SZ 500
@@ -1693,8 +1708,10 @@ int main(int argc, char **argv)
 	if (module_read)
 		read_dump(module_read, 0);
 
+	err = 0;
+
 	while (optind < argc) {
-		read_symbols(argv[optind++]);
+		err |= read_symbols(argv[optind++]);
 	}
 
 	for (mod = modules; mod; mod = mod->next) {
@@ -1703,8 +1720,6 @@ int main(int argc, char **argv)
 		check_exports(mod);
 	}
 
-	err = 0;
-
 	for (mod = modules; mod; mod = mod->next) {
 		if (mod->skip)
 			continue;

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Sam Ravnborg]

On Tue, Jan 15, 2008 at 04:17:42PM +0100, Ingo Molnar wrote:
> 
> * Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > > find below the current set of warnings on -git. There are 62.
> > 
> > The correct figure is 112.
> > 
> > You need to do a:
> > make KCFLAGS=-fno-unit-at-a-time
> > build to see them all.
> 
> btw., please add a .config option to trigger the -fno-unit-at-a-time 
> flags. Something like CONFIG_SECTION_ERRORS=y - plus perhaps combine it 
> with the patch below that turns such section bugs into detectable build 
> errors. A distro does not want to build a kernel that could potentially 
> corrupt kernel memory. (it's a security risk as well.) If we make the 
> err=1 dependent on CONFIG_SECTION_ERRORS then we'll have this 
> configurable.

The plan is to let section mismatch warnings become errors 
after the merge window - so we hit -mm first.
And I will add a config option to:
- set -fno-unit-at-a-time
- add no-inline to all functions marked __init*
- and maybe disable __inline if that gives additional errors

Slowly getting there.
Need to beat modpost in shape to get much less configuration
dependent warnings/errors first.

	Sam

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Andi Kleen]

On Tue, Jan 15, 2008 at 05:25:13PM +0100, Sam Ravnborg wrote:
> On Tue, Jan 15, 2008 at 04:17:42PM +0100, Ingo Molnar wrote:
> > 
> > * Sam Ravnborg <sam@ravnborg.org> wrote:
> > 
> > > > find below the current set of warnings on -git. There are 62.
> > > 
> > > The correct figure is 112.
> > > 
> > > You need to do a:
> > > make KCFLAGS=-fno-unit-at-a-time
> > > build to see them all.
> > 
> > btw., please add a .config option to trigger the -fno-unit-at-a-time 
> > flags. Something like CONFIG_SECTION_ERRORS=y - plus perhaps combine it 
> > with the patch below that turns such section bugs into detectable build 
> > errors. A distro does not want to build a kernel that could potentially 
> > corrupt kernel memory. (it's a security risk as well.) If we make the 
> > err=1 dependent on CONFIG_SECTION_ERRORS then we'll have this 
> > configurable.
> 
> The plan is to let section mismatch warnings become errors 
> after the merge window - so we hit -mm first.

A lot of those I look at seem to be not really bugs; also my
impression is that they sometimes crop up randomly. e.g. you
change something completely unrelated and suddenly you get
a section warning somewhere else.

> And I will add a config option to:
> - set -fno-unit-at-a-time

I was told future gcc versions would remove that. Why do you
want it?

> - add no-inline to all functions marked __init*
> - and maybe disable __inline if that gives additional errors

I sometimes do that for debugging "define static noinline" 
in specific files or similar because it's easier to make sense of oopses
when functions not inlined.  Not sure it would work
as a global option though because if you do it globally
then all the inlines in all .hs would be affected and that might
lead to immense code bloat.

-Andi

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Tue, Jan 15, 2008 at 06:11:46PM +0100, Andi Kleen wrote:
> On Tue, Jan 15, 2008 at 05:25:13PM +0100, Sam Ravnborg wrote:
> > On Tue, Jan 15, 2008 at 04:17:42PM +0100, Ingo Molnar wrote:
> > > 
> > > * Sam Ravnborg <sam@ravnborg.org> wrote:
> > > 
> > > > > find below the current set of warnings on -git. There are 62.
> > > > 
> > > > The correct figure is 112.
> > > > 
> > > > You need to do a:
> > > > make KCFLAGS=-fno-unit-at-a-time
> > > > build to see them all.
> > > 
> > > btw., please add a .config option to trigger the -fno-unit-at-a-time 
> > > flags. Something like CONFIG_SECTION_ERRORS=y - plus perhaps combine it 
> > > with the patch below that turns such section bugs into detectable build 
> > > errors. A distro does not want to build a kernel that could potentially 
> > > corrupt kernel memory. (it's a security risk as well.) If we make the 
> > > err=1 dependent on CONFIG_SECTION_ERRORS then we'll have this 
> > > configurable.
> > 
> > The plan is to let section mismatch warnings become errors 
> > after the merge window - so we hit -mm first.
> 
> A lot of those I look at seem to be not really bugs;

Half of them are possible Oopses, and the other half are about wasted 
memory.

The warnings in the kernel that are not really bugs you can count with 
your fingers.

> also my
> impression is that they sometimes crop up randomly. e.g. you
> change something completely unrelated and suddenly you get
> a section warning somewhere else.
>...

Not all errors are always visible, they might depend on CONFIG_ 
options, and sometimes gcc optimizes such bugs away.

> -Andi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Sam Ravnborg]

> > 
> > The plan is to let section mismatch warnings become errors 
> > after the merge window - so we hit -mm first.
> 
> A lot of those I look at seem to be not really bugs; also my
> impression is that they sometimes crop up randomly. e.g. you
> change something completely unrelated and suddenly you get
> a section warning somewhere else.
I have fixed a lot of these warnings.
And when I look closer at them they are explainable.

The warnings are today very dependent on the configuration
and the inlining that gcc uses.
With default options to gcc my .config produces ~65 warnings
but with -fno-unit-a-time I get 112 warnings.
Solely due to less inlining done by gcc.

So there are two sources for the 'randomization':
a) The actual config
b) The sometimes agressive inlining

a) will be addressed by having separate sections for each
__init* type that is at link time combined where it belongs.

b) is addressed by a Kernel Hacking option which
   1) uses -fno-unit-at-a-time to get less gcc inlining
   2) maybe make all __*init function no-inline
   3) maybe disable inlining globally

> > And I will add a config option to:
> > - set -fno-unit-at-a-time
> 
> I was told future gcc versions would remove that. Why do you
> want it?
Are there any better way to tell gcc no to inline so agressively?

	Sam

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Andi Kleen]

On Tue, Jan 15, 2008 at 07:21:42PM +0100, Sam Ravnborg wrote:
> With default options to gcc my .config produces ~65 warnings
> but with -fno-unit-a-time I get 112 warnings.
> Solely due to less inlining done by gcc.
> 
> So there are two sources for the 'randomization':
> a) The actual config
> b) The sometimes agressive inlining

Inlining should not be random.  And how does inlining cause such a warning?
> 
> a) will be addressed by having separate sections for each
> __init* type that is at link time combined where it belongs.

One problem I ran into the past was that older binutils seem
to have some exponential behaviour with a lot of named sections
and run very slowly.

> 
> b) is addressed by a Kernel Hacking option which
>    1) uses -fno-unit-at-a-time to get less gcc inlining
>    2) maybe make all __*init function no-inline
>    3) maybe disable inlining globally
> 
> > > And I will add a config option to:
> > > - set -fno-unit-at-a-time
> > 
> > I was told future gcc versions would remove that. Why do you
> > want it?
> Are there any better way to tell gcc no to inline so agressively?

You can either sprinkle noinlines or set specific --params to throttle
back the inliner. The later is very gcc version specific unfortunately.

-Andi

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Sam Ravnborg]

On Tue, Jan 15, 2008 at 07:29:04PM +0100, Andi Kleen wrote:
> On Tue, Jan 15, 2008 at 07:21:42PM +0100, Sam Ravnborg wrote:
> > With default options to gcc my .config produces ~65 warnings
> > but with -fno-unit-a-time I get 112 warnings.
> > Solely due to less inlining done by gcc.
> > 
> > So there are two sources for the 'randomization':
> > a) The actual config
> > b) The sometimes agressive inlining
> 
> Inlining should not be random.  And how does inlining cause such a warning?
Consider:

static int __init foo()
{
	// ...
}

static int bar()
{
	// ...
	if (foo())
		// ...
}

gcc will often inline foo into bar - and then all code are suddenly
part of .text and no section mismatch.
But you add anohter call to foo() somewhere so gcc decide no longer
to inline foo() and we then have a reference from .text to .init.text.

> > 
> > a) will be addressed by having separate sections for each
> > __init* type that is at link time combined where it belongs.
> 
> One problem I ran into the past was that older binutils seem
> to have some exponential behaviour with a lot of named sections
> and run very slowly.
This is more the -ffunction-section issue I guess.
What we are dealig with here is ~20 more sections and the kernel has
~100 section today (or more). So not a huge increase.

	Sam

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Tue, Jan 15, 2008 at 07:21:42PM +0100, Sam Ravnborg wrote:
>...
> > > And I will add a config option to:
> > > - set -fno-unit-at-a-time
> > 
> > I was told future gcc versions would remove that. Why do you
> > want it?
> Are there any better way to tell gcc no to inline so agressively?

I haven't tested it, but -fno-inline-functions-called-once alone 
_might_ be enough.

> 	Sam

cu
Adrian

BTW: I hope it's clear for everyone that disabling such optimizations
     should only be used for debugging section mismatches, not for
     production kernels.

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

[patch for v2.6.24] fix section mismatch warning in page_alloc.c

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> [PATCH] mm: fix section mismatch warning in page_alloc.c
> 
> With CONFIG_HOTPLUG=n and CONFIG_HOTPLUG_CPU=y we saw following 
> warning: WARNING: mm/built-in.o(.text+0x6864): Section mismatch: 
> reference to .init.text: (between 'process_zones' and 
> 'pageset_cpuup_callback')
> 
> The culprint was zone_batchsize() which were annotated __devinit but 
> used from process_zones() which is annotated __cpuinit. 
> zone_batchsize() are used from another function annotated __meminit so 
> the only valid option is to drop the annotation of zone_batchsize() so 
> we know it is always valid to use it.

Andrew: a v2.6.24 mm fix i think.

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Ingo Molnar]

* Adrian Bunk <bunk@kernel.org> wrote:

> > make ARCH=x86_64 allyesconfig
> > will set CONFIG_64BIT for you - no?
> 
> Yes.
> 
> But this still leaves the fact that when someone says 'allyesconfig' 
> it's no longer clear which configuration he has. And no, I do not 
> consider it funny that this now implies two hours of compilation twice 
> just for seeing one bug.

sorry - i should have mentioned that it's 64-bit. (and i even 
mis-remembered having seen it on 32-bit as well)

	Ingo

Re: [PATCH x86] [15/16] Force __cpuinit on for CONFIG_PM without HOTPLUG_CPU

[Adrian Bunk]

On Tue, Jan 15, 2008 at 11:14:09PM +0100, Ingo Molnar wrote:
> 
> * Adrian Bunk <bunk@kernel.org> wrote:
> 
> > > make ARCH=x86_64 allyesconfig
> > > will set CONFIG_64BIT for you - no?
> > 
> > Yes.
> > 
> > But this still leaves the fact that when someone says 'allyesconfig' 
> > it's no longer clear which configuration he has. And no, I do not 
> > consider it funny that this now implies two hours of compilation twice 
> > just for seeing one bug.
> 
> sorry - i should have mentioned that it's 64-bit. (and i even 
> mis-remembered having seen it on 32-bit as well)

This bug seems to also be present on 32bit, but allyesconfig minus 
CONFIG_HOTPLUG is not among the configurations where it's present
on 32bit.

> 	Ingo

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed