Skip to contentSkip to conversation

Add noinline attribute

PATCH In review Andi Kleen 6 participants 14 messages Jan 14, 2004 – Jan 19, 2004 Sparse checker

lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Add noinline attribute

do_test_wp_bit cannot be inlined, otherwise the kernel doesn't boot because the exception tables get reordered.

Add a new noinline attribute to compiler.h and use it.

This patch is needed for the next unit-at-a-time patch.

-Andi

arch/i386/mm/init.c renamed from arch/i386/mm/init.c-o +1 -1
diff -u linux-34/arch/i386/mm/init.c-o linux-34/arch/i386/mm/init.c
@@ -555,7 +555,7 @@
  * This function cannot be __init, since exceptions don't work in that
  * section.  Put this after the callers, so that it cannot be inlined.
  */
-static int do_test_wp_bit(void)
+static int noinline do_test_wp_bit(void)
 {
 	char tmp_reg;
 	int flag;
include/linux/compiler.h renamed from include/linux/compiler.h-o +4 -0
diff -u linux-34/include/linux/compiler.h-o linux-34/include/linux/compiler.h
@@ -76,6 +76,10 @@
 # define __attribute_pure__	/* unimplemented */
 #endif
 
+#ifndef noinline
+#define noinline
+#endif
+
 /* Optimization barrier */
 #ifndef barrier
 # define barrier() __memory_barrier()
include/linux/compiler-gcc3.h renamed from include/linux/compiler-gcc3.h-o +4 -0
diff -u linux-34/include/linux/compiler-gcc3.h-o linux-34/include/linux/compiler-gcc3.h
@@ -20,3 +22,7 @@
 #endif
 
 #define __attribute_pure__	__attribute__((pure))
+
+#if __GNUC_MINOR__ >= 1
+#define  noinline __attribute__((noinline))
+#endif
#1 lore raw
Quoted 3 lines · Andi Kleen

do_test_wp_bit cannot be inlined, otherwise the kernel doesn't boot because the exception tables get reordered.

This patch seems to just hide the _real_ bug, which is that the exception table gets confused.

If the exception table is confused, you'll get oopses on bad user system call pointers, but since that is uncommon, you'll never see it under normal circumstances. This is the only exception that you'll always see.

So it sounds like you have a compiler combination that breaks the exception table totally for _any_ function called from any non-regular segment, and this just happens to be the only one you actually saw.

How about just fixing the exception table instead? A bogo-sort at boot time?

Linus

#2 lore raw
Quoted 7 lines · Linus Torvalds
Quoted 3 lines · Andi Kleen

do_test_wp_bit cannot be inlined, otherwise the kernel doesn't boot because the exception tables get reordered.

Quoted 6 lines

This patch seems to just hide the _real_ bug, which is that the exception table gets confused.

If the exception table is confused, you'll get oopses on bad user system call pointers, but since that is uncommon, you'll never see it under normal circumstances. This is the only exception that you'll always see.

Actually you would get a non booting system because the broken mount ABI does a stress test of EFAULT on every boot.

The only problem is using the exception table from __init functions. It is long known that this doesn't work because the exception table setup relies on the linker generating functions in order and __init violates that.

This has broken various things over time, but so far nothing in mainline i386 yet. I think kdb did this sorting forever for example.

Quoted 6 lines

So it sounds like you have a compiler combination that breaks the exception table totally for _any_ function called from any non-regular segment, and this just happens to be the only one you actually saw.

How about just fixing the exception table instead? A bogo-sort at boot time?

That's fine for me. In fact I did this some time ago on x86-64 when I ran into similar problems. Here's a port of the x86-64 sort function.

-AndI

arch/i386/mm/extable.c renamed from arch/i386/mm/extable.c-o +26 -0
diff -u linux-34/arch/i386/mm/extable.c-o linux-34/arch/i386/mm/extable.c
@@ -5,6 +5,7 @@
 #include <linux/config.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>
+#include <linux/init.h>
 #include <asm/uaccess.h>
 
 /* Simple binary search */

@@ -56,3 +57,28 @@
 
 	return 0;
 }
+
+/* When an exception handler is in an non standard section (like __init)
+   the fixup table can end up unordered. Fix that here. */
+__init int check_extable(void)
+{
+	extern struct exception_table_entry __start___ex_table[];
+	extern struct exception_table_entry  __stop___ex_table[];
+	struct exception_table_entry *e;
+	int change;
+
+	/* The input is near completely presorted, which makes bubble sort the
+	   best (and simplest) sort algorithm. */
+	do {
+		change = 0;
+		for (e = __start___ex_table+1; e < __stop___ex_table; e++) {
+			if (e->insn < e[-1].insn) {
+				struct exception_table_entry tmp = e[-1];
+				e[-1] = e[0];
+				e[0] = tmp;
+				change = 1;
+			}
+		}
+	} while (change != 0);
+	return 0;
+}
arch/i386/kernel/setup.c renamed from arch/i386/kernel/setup.c-o +3 -0
diff -u linux-34/arch/i386/kernel/setup.c-o linux-34/arch/i386/kernel/setup.c
@@ -119,6 +119,7 @@
 extern void generic_apic_probe(char *);
 extern int root_mountflags;
 extern char _end[];
+extern int check_extable(void);
 
 unsigned long saved_videomode;
 

@@ -1114,6 +1115,8 @@
 #endif
 	paging_init();
 
+	check_extable();
+
 	dmi_scan_machine();
 
 #ifdef CONFIG_X86_GENERICARCH
#3 lore raw
Quoted 3 lines · Andi Kleen

Actually you would get a non booting system because the broken mount ABI does a stress test of EFAULT on every boot.

There is somethinglike _three_ exceptions that get any kind of testing:
the WP bit, the mount interface, and the FP fault check.

That's three out of several thousand, so coverage really sucks. That's why I'd rather just sort the damn thing once and for all, and not have that issue pop up every once in a while.

Quoted 2 lines

That's fine for me. In fact I did this some time ago on x86-64 when I ran into similar problems. Here's a port of the x86-64 sort function.

Ugh. Can't we just make this be generic code (and that means calling it in the module loading code too..)?

As to bubble sort (which is fine for something that is 99% sorted anyway), isn't it better to continue pushing the entry down when you find something out of order? That way you don't have to repeat the whole scan, you just continue with the next entry once the unsorted entry has percolated to its place (ie keep entries "0..n-1" sorted at all times). That should make the code cleaner too.

Linus

#4 lore raw
Quoted 8 lines · Linus Torvalds
Quoted 3 lines · Andi Kleen

That's fine for me. In fact I did this some time ago on x86-64 when I ran into similar problems. Here's a port of the x86-64 sort function.

Ugh. Can't we just make this be generic code (and that means calling it in the module loading code too..)?

Quoted 6 lines

As to bubble sort (which is fine for something that is 99% sorted anyway), isn't it better to continue pushing the entry down when you find something out of order? That way you don't have to repeat the whole scan, you just continue with the next entry once the unsorted entry has percolated to its place (ie keep entries "0..n-1" sorted at all times). That should make the code cleaner too.

Yes, that algorithm is called insertion sort and is already implemented in the ppc arch.

Signature · 2 lines
Peter Osterlund - [email protected]
http://w1.894.telia.com/~u89404340
#5 lore raw
In reply to Linus Torvalds
Quoted 2 lines

Ugh. Can't we just make this be generic code (and that means calling it in the module loading code too..)?

Ok, here is a new patch that does the whole thing in generic code and for modules too. I didn't bother to change the sort algorithm because the existing one works well enough.

-Andi

------------------------------------------------------------------

Sort exception tables at runtime. This avoids problems with out of order sections like __init. Needed for gcc 3.4 or 3.3-hammer with -funit-at-a-time.

init/main.c renamed from init/main.c-o +27 -0
diff -u linux-34/init/main.c-o linux-34/init/main.c
@@ -289,6 +289,32 @@
 }
 __setup("init=", init_setup);
 
+extern struct exception_table_entry __start___ex_table[];
+extern struct exception_table_entry  __stop___ex_table[];
+
+/* When an exception handler is in an non standard section (like __init)
+   the fixup table can end up unordered. Fix that here. */
+__init void sort_extable(struct exception_table_entry *start,
+			 struct exception_table_entry *end)
+{
+	struct exception_table_entry *e;
+	int change;
+
+	/* The input is near completely presorted, which makes bubble sort the
+	   best (and simplest) sort algorithm. */
+	do {
+		change = 0;
+		for (e = start+1; e < end; e++) {
+			if (e->insn < e[-1].insn) {
+				struct exception_table_entry tmp = e[-1];
+				e[-1] = e[0];
+				e[0] = tmp;
+				change = 1;
+			}
+		}
+	} while (change != 0);
+}
+
 extern void setup_arch(char **);
 extern void cpu_idle(void);
 

@@ -394,6 +420,7 @@
 	lock_kernel();
 	printk(linux_banner);
 	setup_arch(&command_line);
+	sort_extable(__start___ex_table, __stop___ex_table);
 	setup_per_cpu_areas();
 
 	/*
kernel/module.c renamed from kernel/module.c-o +4 -0
diff -u linux-34/kernel/module.c-o linux-34/kernel/module.c
@@ -37,6 +37,9 @@
 #include <asm/pgalloc.h>
 #include <asm/cacheflush.h>
 
+extern void sort_extable(const struct exception_table_entry *start,
+			 const struct exception_table_entry *end);
+
 #if 0
 #define DEBUGP printk
 #else

@@ -1614,6 +1617,7 @@
   	/* Set up exception table */
 	mod->num_exentries = sechdrs[exindex].sh_size / sizeof(*mod->extable);
 	mod->extable = (void *)sechdrs[exindex].sh_addr;
+	sort_extable(mod->extable, mod->extable+mod->num_exentries); 
 
 	/* Now do relocations. */
 	for (i = 1; i < hdr->e_shnum; i++) {
#6 lore raw
Quoted 3 lines · Andi Kleen

Ok, here is a new patch that does the whole thing in generic code and for modules too. I didn't bother to change the sort algorithm because the existing one works well enough.

One, you've still got the function marked __init.

Two, the format of struct exception_table_entry is arch specific. That comparison won't work on Alpha, because "insn" is encoded pc-relative.

r~

#7 lore raw
Quoted 6 lines · Richard Henderson
Quoted 3 lines · Andi Kleen

Ok, here is a new patch that does the whole thing in generic code and for modules too. I didn't bother to change the sort algorithm because the existing one works well enough.

One, you've still got the function marked __init.

Already fixed.

Quoted 4 lines

Two, the format of struct exception_table_entry is arch specific. That comparison won't work on Alpha, because "insn" is encoded pc-relative.

Hmpf. Would an extable_compare() function in an asm-*/ file work ?

-Andi

#8 lore raw
Quoted 1 line · Andi Kleen

Hmpf. Would an extable_compare() function in an asm-*/ file work ?

I'd need a swap function too, since the data itself must change when it gets moved.

r~

#9 lore raw

Re: [PATCH] Add noinline attribute - new extable sort patch

In reply to Richard Henderson
Quoted 10 lines · Richard Henderson
Quoted 3 lines · Andi Kleen

Ok, here is a new patch that does the whole thing in generic code and for modules too. I didn't bother to change the sort algorithm because the existing one works well enough.

One, you've still got the function marked __init.

Two, the format of struct exception_table_entry is arch specific. That comparison won't work on Alpha, because "insn" is encoded pc-relative.

I looked at it more closely now. Alpha (and IA64 which uses the same format) would be relatively easy to do. But sparc and sparc64 have a very strange different format which would be too complicated to handle.

I withdraw the patch that does the sort in init/main.c. Instead just let's do it in arch/i386 like in the original patch
Also it doesn't do it for modules because there are no __init sections there (avoids some ifdefs)

Andrew, please replace the previous patch with that one.

Thanks,

-Andi

arch/i386/mm/extable.c renamed from arch/i386/mm/extable.c-o +26 -0
diff -u linux-34/arch/i386/mm/extable.c-o linux-34/arch/i386/mm/extable.c
@@ -5,6 +5,7 @@
 #include <linux/config.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>
+#include <linux/init.h>
 #include <asm/uaccess.h>
 
 /* Simple binary search */

@@ -56,3 +57,28 @@
 
 	return 0;
 }
+
+/* When an exception handler is in an non standard section (like __init)
+   the fixup table can end up unordered. Fix that here. */
+__init int check_extable(void)
+{
+	extern struct exception_table_entry __start___ex_table[];
+	extern struct exception_table_entry  __stop___ex_table[];
+	struct exception_table_entry *e;
+	int change;
+
+	/* The input is near completely presorted, which makes bubble sort the
+	   best (and simplest) sort algorithm. */
+	do {
+		change = 0;
+		for (e = __start___ex_table+1; e < __stop___ex_table; e++) {
+			if (e->insn < e[-1].insn) {
+				struct exception_table_entry tmp = e[-1];
+				e[-1] = e[0];
+				e[0] = tmp;
+				change = 1;
+			}
+		}
+	} while (change != 0);
+	return 0;
+}
arch/i386/kernel/setup.c renamed from arch/i386/kernel/setup.c-o +3 -0
diff -u linux-34/arch/i386/kernel/setup.c-o linux-34/arch/i386/kernel/setup.c
@@ -119,6 +119,7 @@
 extern void generic_apic_probe(char *);
 extern int root_mountflags;
 extern char _end[];
+extern int check_extable(void);
 
 unsigned long saved_videomode;
 

@@ -1114,6 +1115,8 @@
 #endif
 	paging_init();
 
+	check_extable();
+
 	dmi_scan_machine();
 
 #ifdef CONFIG_X86_GENERICARCH
#10 lore raw

Re: [PATCH] Add noinline attribute - new extable sort patch

Quoted 3 lines · Andi Kleen

I looked at it more closely now. Alpha (and IA64 which uses the same format) would be relatively easy to do. But sparc and sparc64 have a very strange different format which would be too complicated to handle.

I don't think that's true. Yes, sparc and sparc64 have paired entries, but they should still sort consecutive. If there were an entry that, after sorting, came between them, something would be Very Wrong.

r~

#11 lore raw

Re: [PATCH] Add noinline attribute - new extable sort patch

Quoted 9 lines · Richard Henderson
Quoted 3 lines · Andi Kleen

I looked at it more closely now. Alpha (and IA64 which uses the same format) would be relatively easy to do. But sparc and sparc64 have a very strange different format which would be too complicated to handle.

I don't think that's true. Yes, sparc and sparc64 have paired entries, but they should still sort consecutive. If there were an entry that, after sorting, came between them, something would be Very Wrong.

Hmm, are they really just paired? The description in arch/sparc64/mm/extable.c looked differently to me. Anyways - given all these complexities doing the sort in arch code is probably better. It wasn't my idea anyways to move it into generic code ;-)

It's probably not very critical for the other architectures anyways because they likely don't depend on exception tables working in __init functions (and if they do they likely already have an own sort function)

-Andi

#12 lore raw

Re: [PATCH] Add noinline attribute - new extable sort patch

Quoted 10 lines · On 19 Jan 2004 02:01:33 +0100 Andi Kleen <[email protected]> wrote:
Quoted 4 lines · Richard Henderson

I don't think that's true. Yes, sparc and sparc64 have paired entries, but they should still sort consecutive. If there were an entry that, after sorting, came between them, something would be Very Wrong.

Hmm, are they really just paired? The description in arch/sparc64/mm/extable.c looked differently to me. Anyways - given all these complexities doing the sort in arch code is probably better. It wasn't my idea anyways to move it into generic code ;-)

When I started the 2.5 extable consolidation, I stopped where you see today, becuase I realized that we'd need to move "range" extable entries to a separate section (empty on most archs) and every arch would need to supply a cmp function for sorting each one. Add in rth's point about needing a swap fn, I think that it's simpler to leave it as is, maybe with a module.c call to extable_sort() for archs which care to implement.

Cheers,
Rusty.

Signature · 2 lines
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy
#13 lore raw
In reply to Andi Kleen
Quoted 2 lines · On Wed, 14 Jan 2004 09:31:14 +0100 Andi Kleen <[email protected]> wrote:

do_test_wp_bit cannot be inlined, otherwise the kernel doesn't boot because the exception tables get reordered.

Maybe you should sort the exception table as PPC does. See FIXME in i386/kernel/module.c too.

Cheers,
Rusty.

Signature · 2 lines
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy