Various i386/x86-64 changes

All against recent git-x86

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.

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

Index: 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));
 			}
 		}
 	}

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

Only done for x86 currently.

---
 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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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);

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.

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

Index: 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)
 {

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

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

Index: 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

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.

---
 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
===================================================================
@@ -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
===================================================================
@@ -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

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.

---
 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
===================================================================
@@ -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/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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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);

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.

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

Index: 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);
 

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

Index: 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;
 

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

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

Index: 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

Add missing targets and missing options in x86 make help

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

Index: 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 \

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

May save a minor bit of compilation time.

---
 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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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 */

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.

---
 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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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

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

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

Index: 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"
 

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

Index: 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).

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.

---
 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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -712,7 +712,7 @@ void __init mtrr_bp_init(void)
 	}
 }
 
-void mtrr_ap_init(void)
+void __cpuinit mtrr_ap_init(void)
 {
 	unsigned long flags;
 

Otherwise

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

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

Index: 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();
 }

Good catch. I'll repost with that fixed.

-Andi

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

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

Won't help for UP at least.

-Andi

OK, I'll fix that up later.

Thanks,
Rafael

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

Ingo

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

good idea Andi, applied.

Ingo

agreed.

A style error slipped in, please fix it:

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

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

Ingo

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

Ingo

yeah - and KERN_DEBUG is rarely useful anyway.

Ingo

yep, added it.

yeah - fixed those.

Ingo

thanks, applied.

Ingo

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

Ingo

thanks, applied.

Ingo

Please run your patches through checkpatch.pl.

ERROR: use tabs not spaces
#48: FILE: arch/x86/kernel/alternative.c:360:

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

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 <[email protected]>
Ingo Molnar <[email protected]>

for genirq patches, please Cc:

Thomas Gleixner <[email protected]>
Ingo Molnar <[email protected]>

for timer/clocksource/clockevents patches please Cc:

Thomas Gleixner <[email protected]>
John Stultz <[email protected]>
Ingo Molnar <[email protected]>

for x86 patches, please Cc:

Thomas Gleixner <[email protected]>
"H. Peter Anvin" <[email protected]>
Ingo Molnar <[email protected]>

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

Ingo

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.

Don't think so, smp_init() seems to use it one off.

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

-Andi

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

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

-Andi

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

-Andi

[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: [email protected]
| 
| Signed-off-by: Andi Kleen <[email protected]>
| 
| ---
|  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 [email protected]
| More majordomo info at  http://vger.kernel.org/majordomo-info.html
| Please read the FAQ at  http://www.tux.org/lkml/
| 

- Cyrill -

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

-Andi

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.

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".

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

tglx

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.

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

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:

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

Hi Andi,

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

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.

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.

Yes I fixed that now.

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 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.

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.

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

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

-Andi

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.

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

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_*

It's not -- checkpatch is often wrong.

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

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

Fortunately it's easily fixed.

I wrote one now.

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

As you can see it was trivial.

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

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.

Documentation/SubmitChecklist

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

I need to fix up your sloppiness ?

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.

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

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.

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.

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

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

That says

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

I think I did that.

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.

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 fixed the patch instead. But you're right I was wrong on that.

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.

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

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

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

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

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.

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.

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

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.

Rate is certainly higher here.

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

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.

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

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

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.

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

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

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

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.

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

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

-Andi

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?

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

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.

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

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

-Andi

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

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

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

x86.git would be fine.

Thanks,
Rafael

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

Thanks,
Rafael

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

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.

---
 arch/x86/Kconfig        |    2 +-
 arch/x86/power/Makefile |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Index: 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
===================================================================
@@ -1,2 +1,2 @@
-obj-$(CONFIG_PM)		+= cpu.o
+obj-$(CONFIG_PM_SLEEP)		+= cpu.o
 obj-$(CONFIG_HIBERNATION)	+= swsusp.o suspend.o

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 <[email protected]>

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

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

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.

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

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

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

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')

Definitely.

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

But _how_ can we get this automated at all?

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

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.

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

Please send the .config.

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

'make allyesconfig' and disable CONFIG_HOTPLUG=y.

Ingo

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?

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

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.

---

index e1028fa..b2838c2 100644
@@ -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;
 

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

Sam

The correct figure is 112.

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

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

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.

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

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

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.

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

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 <[email protected]>

turn the build warning into a build error.

---
 scripts/mod/modpost.c |   63 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 24 deletions(-)

Index: 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;

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

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 was told future gcc versions would remove that. Why do you want it?

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

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.

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

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

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

Are there any better way to tell gcc no to inline so agressively?

Sam

Inlining should not be random. And how does inlining cause such a warning?

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.

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

-Andi

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.

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

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

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

Andrew: a v2.6.24 mm fix i think.

Ingo

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

Ingo

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.

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