[PATCH] Add noinline attribute

[PATCH] Add noinline attribute

[Andi Kleen]

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

diff -u linux-34/arch/i386/mm/init.c-o linux-34/arch/i386/mm/init.c
--- linux-34/arch/i386/mm/init.c-o	2004-01-09 09:27:09.000000000 +0100
+++ linux-34/arch/i386/mm/init.c	2004-01-13 22:19:19.000000000 +0100
@@ -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;
diff -u linux-34/include/linux/compiler.h-o linux-34/include/linux/compiler.h
--- linux-34/include/linux/compiler.h-o	2003-11-24 04:46:36.000000000 +0100
+++ linux-34/include/linux/compiler.h	2004-01-13 22:17:26.000000000 +0100
@@ -76,6 +76,10 @@
 # define __attribute_pure__	/* unimplemented */
 #endif
 
+#ifndef noinline
+#define noinline
+#endif
+
 /* Optimization barrier */
 #ifndef barrier
 # define barrier() __memory_barrier()
diff -u linux-34/include/linux/compiler-gcc3.h-o linux-34/include/linux/compiler-gcc3.h
--- linux-34/include/linux/compiler-gcc3.h-o	2003-09-28 10:53:23.000000000 +0200
+++ linux-34/include/linux/compiler-gcc3.h	2004-01-13 22:36:22.000000000 +0100
@@ -20,3 +22,7 @@
 #endif
 
 #define __attribute_pure__	__attribute__((pure))
+
+#if __GNUC_MINOR__ >= 1
+#define  noinline __attribute__((noinline))
+#endif

Re: [PATCH] Add noinline attribute

[Linus Torvalds]

On Wed, 14 Jan 2004, Andi Kleen wrote:
> 
> 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

Re: [PATCH] Add noinline attribute

[Rusty Russell]

On Wed, 14 Jan 2004 09:31:14 +0100
Andi Kleen <ak@muc.de> 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.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

Re: [PATCH] Add noinline attribute

[Andi Kleen]

On Wed, Jan 14, 2004 at 03:23:35PM -0800, Linus Torvalds wrote:
> 
> 
> On Wed, 14 Jan 2004, Andi Kleen wrote:
> > 
> > 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.

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.


> 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


diff -u linux-34/arch/i386/mm/extable.c-o linux-34/arch/i386/mm/extable.c
--- linux-34/arch/i386/mm/extable.c-o	2003-05-27 03:01:00.000000000 +0200
+++ linux-34/arch/i386/mm/extable.c	2004-01-15 08:39:31.657013864 +0100
@@ -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;
+}
diff -u linux-34/arch/i386/kernel/setup.c-o linux-34/arch/i386/kernel/setup.c
--- linux-34/arch/i386/kernel/setup.c-o	2004-01-09 09:27:09.000000000 +0100
+++ linux-34/arch/i386/kernel/setup.c	2004-01-15 08:39:30.438199152 +0100
@@ -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

Re: [PATCH] Add noinline attribute

[Linus Torvalds]

On Wed, 15 Jan 2004, Andi Kleen wrote:
> 
> 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.

> 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

Re: [PATCH] Add noinline attribute

[Peter Osterlund]

Linus Torvalds <torvalds@osdl.org> writes:

> On Wed, 15 Jan 2004, Andi Kleen wrote:
> > 
> > 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.

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

-- 
Peter Osterlund - petero2@telia.com
http://w1.894.telia.com/~u89404340

Re: [PATCH] Add noinline attribute

[Andi Kleen]

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

diff -u linux-34/init/main.c-o linux-34/init/main.c
--- linux-34/init/main.c-o	2004-01-09 09:27:20.000000000 +0100
+++ linux-34/init/main.c	2004-01-16 10:07:00.699618728 +0100
@@ -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();
 
 	/*
diff -u linux-34/kernel/module.c-o linux-34/kernel/module.c
--- linux-34/kernel/module.c-o	2004-01-09 09:27:20.000000000 +0100
+++ linux-34/kernel/module.c	2004-01-16 10:06:14.945574400 +0100
@@ -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++) {

Re: [PATCH] Add noinline attribute

[Richard Henderson]

On Fri, Jan 16, 2004 at 11:13:45AM +0100, Andi Kleen wrote:
> 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~

Re: [PATCH] Add noinline attribute

[Andi Kleen]

On Sun, Jan 18, 2004 at 12:47:00PM -0800, Richard Henderson wrote:
> On Fri, Jan 16, 2004 at 11:13:45AM +0100, Andi Kleen wrote:
> > 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.

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

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

[Andi Kleen]

On Sun, Jan 18, 2004 at 12:47:00PM -0800, Richard Henderson wrote:
> On Fri, Jan 16, 2004 at 11:13:45AM +0100, Andi Kleen wrote:
> > 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


diff -u linux-34/arch/i386/mm/extable.c-o linux-34/arch/i386/mm/extable.c
--- linux-34/arch/i386/mm/extable.c-o	2003-05-27 03:01:00.000000000 +0200
+++ linux-34/arch/i386/mm/extable.c	2004-01-15 08:39:31.657013864 +0100
@@ -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;
+}
diff -u linux-34/arch/i386/kernel/setup.c-o linux-34/arch/i386/kernel/setup.c
--- linux-34/arch/i386/kernel/setup.c-o	2004-01-09 09:27:09.000000000 +0100
+++ linux-34/arch/i386/kernel/setup.c	2004-01-15 08:39:30.438199152 +0100
@@ -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

Re: [PATCH] Add noinline attribute

[Richard Henderson]

On Sun, Jan 18, 2004 at 09:58:00PM +0100, Andi Kleen wrote:
> 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~

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

[Richard Henderson]

On Mon, Jan 19, 2004 at 12:07:43AM +0100, Andi Kleen wrote:
> 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~

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

[Andi Kleen]

On Sun, Jan 18, 2004 at 04:52:44PM -0800, Richard Henderson wrote:
> On Mon, Jan 19, 2004 at 12:07:43AM +0100, Andi Kleen wrote:
> > 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

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

[Rusty Russell]

On 19 Jan 2004 02:01:33 +0100
Andi Kleen <ak@colin2.muc.de> wrote:

> On Sun, Jan 18, 2004 at 04:52:44PM -0800, Richard Henderson wrote:
> > 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.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy