[PATCH 0/2] jump label updates

[PATCH 0/2] jump label updates

[Jason Baron]

Hi,

So we finally tracked down the jump label crashes and it turned out that there
was a a bug in the compiler in the implementation of 'asm goto'. As the fix has
not yet been committed yet to gcc, and I don't have a simple test case to
check if the bug has been fixed, we are going to disable jump labels pending
a better test to detect that gcc has been fixed.

The second patch fixes a hang on Geode LX, b/c the no-op probing was done too
early - before the exception tables had even been setup.

thanks,

-Jason

Jason Baron (2):
  move arch_init_ideal_nop5 later
  jump label: disable due to compiler bug

 arch/x86/include/asm/alternative.h |    1 -
 arch/x86/kernel/alternative.c      |  132 ++++++++++++++++++------------------
 arch/x86/kernel/setup.c            |    6 --
 include/linux/jump_label.h         |    9 +++
 4 files changed, 76 insertions(+), 72 deletions(-)

[PATCH 1/2] move arch_init_ideal_nop5 later

[Jason Baron]

arch_init_ideal_nop5() was being called from setup_arch() before
the exception table was setup. Move it later into
alternative_instructions().

Fixes a boot hang on OLPC's XO-1 laptop based on Geode LX
processor.


Reported-by: Daniel Drake <dsd@laptop.org>
Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 arch/x86/include/asm/alternative.h |    1 -
 arch/x86/kernel/alternative.c      |  132 ++++++++++++++++++------------------
 arch/x86/kernel/setup.c            |    6 --
 3 files changed, 67 insertions(+), 72 deletions(-)

diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
index 76561d2..2a7f618 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -186,7 +186,6 @@ extern void *text_poke_smp(void *addr, const void *opcode, size_t len);
 #if defined(CONFIG_DYNAMIC_FTRACE) || defined(HAVE_JUMP_LABEL)
 #define IDEAL_NOP_SIZE_5 5
 extern unsigned char ideal_nop5[IDEAL_NOP_SIZE_5];
-extern void arch_init_ideal_nop5(void);
 #else
 static inline void arch_init_ideal_nop5(void) {}
 #endif
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index a36bb90..9f39a1c 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -452,6 +452,71 @@ extern struct paravirt_patch_site __start_parainstructions[],
 	__stop_parainstructions[];
 #endif	/* CONFIG_PARAVIRT */
 
+#if defined(CONFIG_DYNAMIC_FTRACE) || defined(HAVE_JUMP_LABEL)
+
+unsigned char ideal_nop5[IDEAL_NOP_SIZE_5];
+
+static void __init arch_init_ideal_nop5(void)
+{
+	extern const unsigned char ftrace_test_p6nop[];
+	extern const unsigned char ftrace_test_nop5[];
+	extern const unsigned char ftrace_test_jmp[];
+	int faulted = 0;
+
+	/*
+	 * There is no good nop for all x86 archs.
+	 * We will default to using the P6_NOP5, but first we
+	 * will test to make sure that the nop will actually
+	 * work on this CPU. If it faults, we will then
+	 * go to a lesser efficient 5 byte nop. If that fails
+	 * we then just use a jmp as our nop. This isn't the most
+	 * efficient nop, but we can not use a multi part nop
+	 * since we would then risk being preempted in the middle
+	 * of that nop, and if we enabled tracing then, it might
+	 * cause a system crash.
+	 *
+	 * TODO: check the cpuid to determine the best nop.
+	 */
+	asm volatile (
+		"ftrace_test_jmp:"
+		"jmp ftrace_test_p6nop\n"
+		"nop\n"
+		"nop\n"
+		"nop\n"  /* 2 byte jmp + 3 bytes */
+		"ftrace_test_p6nop:"
+		P6_NOP5
+		"jmp 1f\n"
+		"ftrace_test_nop5:"
+		".byte 0x66,0x66,0x66,0x66,0x90\n"
+		"1:"
+		".section .fixup, \"ax\"\n"
+		"2:	movl $1, %0\n"
+		"	jmp ftrace_test_nop5\n"
+		"3:	movl $2, %0\n"
+		"	jmp 1b\n"
+		".previous\n"
+		_ASM_EXTABLE(ftrace_test_p6nop, 2b)
+		_ASM_EXTABLE(ftrace_test_nop5, 3b)
+		: "=r"(faulted) : "0" (faulted));
+
+	switch (faulted) {
+	case 0:
+		pr_info("converting mcount calls to 0f 1f 44 00 00\n");
+		memcpy(ideal_nop5, ftrace_test_p6nop, IDEAL_NOP_SIZE_5);
+		break;
+	case 1:
+		pr_info("converting mcount calls to 66 66 66 66 90\n");
+		memcpy(ideal_nop5, ftrace_test_nop5, IDEAL_NOP_SIZE_5);
+		break;
+	case 2:
+		pr_info("converting mcount calls to jmp . + 5\n");
+		memcpy(ideal_nop5, ftrace_test_jmp, IDEAL_NOP_SIZE_5);
+		break;
+	}
+
+}
+#endif
+
 void __init alternative_instructions(void)
 {
 	/* The patching is not fully atomic, so try to avoid local interruptions
@@ -508,6 +573,8 @@ void __init alternative_instructions(void)
 				(unsigned long)__smp_locks_end);
 
 	restart_nmi();
+
+	arch_init_ideal_nop5();
 }
 
 /**
@@ -641,68 +708,3 @@ void *__kprobes text_poke_smp(void *addr, const void *opcode, size_t len)
 	__stop_machine(stop_machine_text_poke, (void *)&tpp, NULL);
 	return addr;
 }
-
-#if defined(CONFIG_DYNAMIC_FTRACE) || defined(HAVE_JUMP_LABEL)
-
-unsigned char ideal_nop5[IDEAL_NOP_SIZE_5];
-
-void __init arch_init_ideal_nop5(void)
-{
-	extern const unsigned char ftrace_test_p6nop[];
-	extern const unsigned char ftrace_test_nop5[];
-	extern const unsigned char ftrace_test_jmp[];
-	int faulted = 0;
-
-	/*
-	 * There is no good nop for all x86 archs.
-	 * We will default to using the P6_NOP5, but first we
-	 * will test to make sure that the nop will actually
-	 * work on this CPU. If it faults, we will then
-	 * go to a lesser efficient 5 byte nop. If that fails
-	 * we then just use a jmp as our nop. This isn't the most
-	 * efficient nop, but we can not use a multi part nop
-	 * since we would then risk being preempted in the middle
-	 * of that nop, and if we enabled tracing then, it might
-	 * cause a system crash.
-	 *
-	 * TODO: check the cpuid to determine the best nop.
-	 */
-	asm volatile (
-		"ftrace_test_jmp:"
-		"jmp ftrace_test_p6nop\n"
-		"nop\n"
-		"nop\n"
-		"nop\n"  /* 2 byte jmp + 3 bytes */
-		"ftrace_test_p6nop:"
-		P6_NOP5
-		"jmp 1f\n"
-		"ftrace_test_nop5:"
-		".byte 0x66,0x66,0x66,0x66,0x90\n"
-		"1:"
-		".section .fixup, \"ax\"\n"
-		"2:	movl $1, %0\n"
-		"	jmp ftrace_test_nop5\n"
-		"3:	movl $2, %0\n"
-		"	jmp 1b\n"
-		".previous\n"
-		_ASM_EXTABLE(ftrace_test_p6nop, 2b)
-		_ASM_EXTABLE(ftrace_test_nop5, 3b)
-		: "=r"(faulted) : "0" (faulted));
-
-	switch (faulted) {
-	case 0:
-		pr_info("converting mcount calls to 0f 1f 44 00 00\n");
-		memcpy(ideal_nop5, ftrace_test_p6nop, IDEAL_NOP_SIZE_5);
-		break;
-	case 1:
-		pr_info("converting mcount calls to 66 66 66 66 90\n");
-		memcpy(ideal_nop5, ftrace_test_nop5, IDEAL_NOP_SIZE_5);
-		break;
-	case 2:
-		pr_info("converting mcount calls to jmp . + 5\n");
-		memcpy(ideal_nop5, ftrace_test_jmp, IDEAL_NOP_SIZE_5);
-		break;
-	}
-
-}
-#endif
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 0ac571d..850059d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -112,7 +112,6 @@
 #include <asm/numa_64.h>
 #endif
 #include <asm/mce.h>
-#include <asm/alternative.h>
 
 /*
  * end_pfn only includes RAM, while max_pfn_mapped includes all e820 entries.
@@ -695,7 +694,6 @@ void __init setup_arch(char **cmdline_p)
 {
 	int acpi = 0;
 	int k8 = 0;
-	unsigned long flags;
 
 #ifdef CONFIG_X86_32
 	memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data));
@@ -1055,10 +1053,6 @@ void __init setup_arch(char **cmdline_p)
 	x86_init.oem.banner();
 
 	mcheck_init();
-
-	local_irq_save(flags);
-	arch_init_ideal_nop5();
-	local_irq_restore(flags);
 }
 
 #ifdef CONFIG_X86_32
-- 
1.7.1

[PATCH 2/2] jump label: disable due to compiler bug

[Jason Baron]

Unfortunately, we found a compiler bug in the implementation of
'asm goto'. The bug can cause the kernel to crash.

For now, we are disabling jump labels with a big hammer. When the
gcc fix is committed, we will update the kernel with a better check
for either the version number it's fix in, or some detection of
whether gcc has the fix in place.

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 include/linux/jump_label.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index b67cb18..954462a 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -1,11 +1,20 @@
 #ifndef _LINUX_JUMP_LABEL_H
 #define _LINUX_JUMP_LABEL_H
 
+/*
+ * A compiler bug was found in the implementation of 'asm goto'. Thus,
+ * we are disabling it for now pending a better check for compiler version
+ * that fixes it.
+ */
+#if 0
+
 #if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL)
 # include <asm/jump_label.h>
 # define HAVE_JUMP_LABEL
 #endif
 
+#endif
+
 enum jump_label_type {
 	JUMP_LABEL_ENABLE,
 	JUMP_LABEL_DISABLE
-- 
1.7.1

Re: [PATCH 0/2] jump label updates

[David Miller]

From: Jason Baron <jbaron@redhat.com>
Date: Wed, 27 Oct 2010 17:07:08 -0400

> So we finally tracked down the jump label crashes and it turned out that there
> was a a bug in the compiler in the implementation of 'asm goto'. As the fix has
> not yet been committed yet to gcc, and I don't have a simple test case to
> check if the bug has been fixed, we are going to disable jump labels pending
> a better test to detect that gcc has been fixed.
> 
> The second patch fixes a hang on Geode LX, b/c the no-op probing was done too
> early - before the exception tables had even been setup.

Please don't lose the sparc section alignment bug fix I posted recently.

Re: [PATCH 2/2] jump label: disable due to compiler bug

[H. Peter Anvin]

Could you detail the bug, please?  static_cpu_has() also uses asm goto.

"Jason Baron" <jbaron@redhat.com> wrote:

>Unfortunately, we found a compiler bug in the implementation of
>'asm goto'. The bug can cause the kernel to crash.
>
>For now, we are disabling jump labels with a big hammer. When the
>gcc fix is committed, we will update the kernel with a better check
>for either the version number it's fix in, or some detection of
>whether gcc has the fix in place.
>
>Signed-off-by: Jason Baron <jbaron@redhat.com>
>---
> include/linux/jump_label.h |    9 +++++++++
> 1 files changed, 9 insertions(+), 0 deletions(-)
>
>diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
>index b67cb18..954462a 100644
>--- a/include/linux/jump_label.h
>+++ b/include/linux/jump_label.h
>@@ -1,11 +1,20 @@
> #ifndef _LINUX_JUMP_LABEL_H
> #define _LINUX_JUMP_LABEL_H
> 
>+/*
>+ * A compiler bug was found in the implementation of 'asm goto'. Thus,
>+ * we are disabling it for now pending a better check for compiler
>version
>+ * that fixes it.
>+ */
>+#if 0
>+
> #if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL)
> # include <asm/jump_label.h>
> # define HAVE_JUMP_LABEL
> #endif
> 
>+#endif
>+
> enum jump_label_type {
> 	JUMP_LABEL_ENABLE,
> 	JUMP_LABEL_DISABLE
>-- 
>1.7.1

-- 
Sent from my mobile phone.  Please pardon any lack of formatting.

Re: [PATCH 1/2] move arch_init_ideal_nop5 later

[H. Peter Anvin]

The exception test is broken anyway for reasons I already explained, so it would be better to just drop it.

"Jason Baron" <jbaron@redhat.com> wrote:

>arch_init_ideal_nop5() was being called from setup_arch() before
>the exception table was setup. Move it later into
>alternative_instructions().
>
>Fixes a boot hang on OLPC's XO-1 laptop based on Geode LX
>processor.
>
>
>Reported-by: Daniel Drake <dsd@laptop.org>
>Signed-off-by: Jason Baron <jbaron@redhat.com>
>---
> arch/x86/include/asm/alternative.h |    1 -
>arch/x86/kernel/alternative.c      |  132
>++++++++++++++++++------------------
> arch/x86/kernel/setup.c            |    6 --
> 3 files changed, 67 insertions(+), 72 deletions(-)
>
>diff --git a/arch/x86/include/asm/alternative.h
>b/arch/x86/include/asm/alternative.h
>index 76561d2..2a7f618 100644
>--- a/arch/x86/include/asm/alternative.h
>+++ b/arch/x86/include/asm/alternative.h
>@@ -186,7 +186,6 @@ extern void *text_poke_smp(void *addr, const void
>*opcode, size_t len);
> #if defined(CONFIG_DYNAMIC_FTRACE) || defined(HAVE_JUMP_LABEL)
> #define IDEAL_NOP_SIZE_5 5
> extern unsigned char ideal_nop5[IDEAL_NOP_SIZE_5];
>-extern void arch_init_ideal_nop5(void);
> #else
> static inline void arch_init_ideal_nop5(void) {}
> #endif
>diff --git a/arch/x86/kernel/alternative.c
>b/arch/x86/kernel/alternative.c
>index a36bb90..9f39a1c 100644
>--- a/arch/x86/kernel/alternative.c
>+++ b/arch/x86/kernel/alternative.c
>@@ -452,6 +452,71 @@ extern struct paravirt_patch_site
>__start_parainstructions[],
> 	__stop_parainstructions[];
> #endif	/* CONFIG_PARAVIRT */
> 
>+#if defined(CONFIG_DYNAMIC_FTRACE) || defined(HAVE_JUMP_LABEL)
>+
>+unsigned char ideal_nop5[IDEAL_NOP_SIZE_5];
>+
>+static void __init arch_init_ideal_nop5(void)
>+{
>+	extern const unsigned char ftrace_test_p6nop[];
>+	extern const unsigned char ftrace_test_nop5[];
>+	extern const unsigned char ftrace_test_jmp[];
>+	int faulted = 0;
>+
>+	/*
>+	 * There is no good nop for all x86 archs.
>+	 * We will default to using the P6_NOP5, but first we
>+	 * will test to make sure that the nop will actually
>+	 * work on this CPU. If it faults, we will then
>+	 * go to a lesser efficient 5 byte nop. If that fails
>+	 * we then just use a jmp as our nop. This isn't the most
>+	 * efficient nop, but we can not use a multi part nop
>+	 * since we would then risk being preempted in the middle
>+	 * of that nop, and if we enabled tracing then, it might
>+	 * cause a system crash.
>+	 *
>+	 * TODO: check the cpuid to determine the best nop.
>+	 */
>+	asm volatile (
>+		"ftrace_test_jmp:"
>+		"jmp ftrace_test_p6nop\n"
>+		"nop\n"
>+		"nop\n"
>+		"nop\n"  /* 2 byte jmp + 3 bytes */
>+		"ftrace_test_p6nop:"
>+		P6_NOP5
>+		"jmp 1f\n"
>+		"ftrace_test_nop5:"
>+		".byte 0x66,0x66,0x66,0x66,0x90\n"
>+		"1:"
>+		".section .fixup, \"ax\"\n"
>+		"2:	movl $1, %0\n"
>+		"	jmp ftrace_test_nop5\n"
>+		"3:	movl $2, %0\n"
>+		"	jmp 1b\n"
>+		".previous\n"
>+		_ASM_EXTABLE(ftrace_test_p6nop, 2b)
>+		_ASM_EXTABLE(ftrace_test_nop5, 3b)
>+		: "=r"(faulted) : "0" (faulted));
>+
>+	switch (faulted) {
>+	case 0:
>+		pr_info("converting mcount calls to 0f 1f 44 00 00\n");
>+		memcpy(ideal_nop5, ftrace_test_p6nop, IDEAL_NOP_SIZE_5);
>+		break;
>+	case 1:
>+		pr_info("converting mcount calls to 66 66 66 66 90\n");
>+		memcpy(ideal_nop5, ftrace_test_nop5, IDEAL_NOP_SIZE_5);
>+		break;
>+	case 2:
>+		pr_info("converting mcount calls to jmp . + 5\n");
>+		memcpy(ideal_nop5, ftrace_test_jmp, IDEAL_NOP_SIZE_5);
>+		break;
>+	}
>+
>+}
>+#endif
>+
> void __init alternative_instructions(void)
> {
>	/* The patching is not fully atomic, so try to avoid local
>interruptions
>@@ -508,6 +573,8 @@ void __init alternative_instructions(void)
> 				(unsigned long)__smp_locks_end);
> 
> 	restart_nmi();
>+
>+	arch_init_ideal_nop5();
> }
> 
> /**
>@@ -641,68 +708,3 @@ void *__kprobes text_poke_smp(void *addr, const
>void *opcode, size_t len)
> 	__stop_machine(stop_machine_text_poke, (void *)&tpp, NULL);
> 	return addr;
> }
>-
>-#if defined(CONFIG_DYNAMIC_FTRACE) || defined(HAVE_JUMP_LABEL)
>-
>-unsigned char ideal_nop5[IDEAL_NOP_SIZE_5];
>-
>-void __init arch_init_ideal_nop5(void)
>-{
>-	extern const unsigned char ftrace_test_p6nop[];
>-	extern const unsigned char ftrace_test_nop5[];
>-	extern const unsigned char ftrace_test_jmp[];
>-	int faulted = 0;
>-
>-	/*
>-	 * There is no good nop for all x86 archs.
>-	 * We will default to using the P6_NOP5, but first we
>-	 * will test to make sure that the nop will actually
>-	 * work on this CPU. If it faults, we will then
>-	 * go to a lesser efficient 5 byte nop. If that fails
>-	 * we then just use a jmp as our nop. This isn't the most
>-	 * efficient nop, but we can not use a multi part nop
>-	 * since we would then risk being preempted in the middle
>-	 * of that nop, and if we enabled tracing then, it might
>-	 * cause a system crash.
>-	 *
>-	 * TODO: check the cpuid to determine the best nop.
>-	 */
>-	asm volatile (
>-		"ftrace_test_jmp:"
>-		"jmp ftrace_test_p6nop\n"
>-		"nop\n"
>-		"nop\n"
>-		"nop\n"  /* 2 byte jmp + 3 bytes */
>-		"ftrace_test_p6nop:"
>-		P6_NOP5
>-		"jmp 1f\n"
>-		"ftrace_test_nop5:"
>-		".byte 0x66,0x66,0x66,0x66,0x90\n"
>-		"1:"
>-		".section .fixup, \"ax\"\n"
>-		"2:	movl $1, %0\n"
>-		"	jmp ftrace_test_nop5\n"
>-		"3:	movl $2, %0\n"
>-		"	jmp 1b\n"
>-		".previous\n"
>-		_ASM_EXTABLE(ftrace_test_p6nop, 2b)
>-		_ASM_EXTABLE(ftrace_test_nop5, 3b)
>-		: "=r"(faulted) : "0" (faulted));
>-
>-	switch (faulted) {
>-	case 0:
>-		pr_info("converting mcount calls to 0f 1f 44 00 00\n");
>-		memcpy(ideal_nop5, ftrace_test_p6nop, IDEAL_NOP_SIZE_5);
>-		break;
>-	case 1:
>-		pr_info("converting mcount calls to 66 66 66 66 90\n");
>-		memcpy(ideal_nop5, ftrace_test_nop5, IDEAL_NOP_SIZE_5);
>-		break;
>-	case 2:
>-		pr_info("converting mcount calls to jmp . + 5\n");
>-		memcpy(ideal_nop5, ftrace_test_jmp, IDEAL_NOP_SIZE_5);
>-		break;
>-	}
>-
>-}
>-#endif
>diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>index 0ac571d..850059d 100644
>--- a/arch/x86/kernel/setup.c
>+++ b/arch/x86/kernel/setup.c
>@@ -112,7 +112,6 @@
> #include <asm/numa_64.h>
> #endif
> #include <asm/mce.h>
>-#include <asm/alternative.h>
> 
> /*
>* end_pfn only includes RAM, while max_pfn_mapped includes all e820
>entries.
>@@ -695,7 +694,6 @@ void __init setup_arch(char **cmdline_p)
> {
> 	int acpi = 0;
> 	int k8 = 0;
>-	unsigned long flags;
> 
> #ifdef CONFIG_X86_32
> 	memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data));
>@@ -1055,10 +1053,6 @@ void __init setup_arch(char **cmdline_p)
> 	x86_init.oem.banner();
> 
> 	mcheck_init();
>-
>-	local_irq_save(flags);
>-	arch_init_ideal_nop5();
>-	local_irq_restore(flags);
> }
> 
> #ifdef CONFIG_X86_32
>-- 
>1.7.1

-- 
Sent from my mobile phone.  Please pardon any lack of formatting.

Re: [PATCH 0/2] jump label updates

[Steven Rostedt]

On Wed, 2010-10-27 at 14:15 -0700, David Miller wrote:
> From: Jason Baron <jbaron@redhat.com>
> Date: Wed, 27 Oct 2010 17:07:08 -0400
> 
> > So we finally tracked down the jump label crashes and it turned out that there
> > was a a bug in the compiler in the implementation of 'asm goto'. As the fix has
> > not yet been committed yet to gcc, and I don't have a simple test case to
> > check if the bug has been fixed, we are going to disable jump labels pending
> > a better test to detect that gcc has been fixed.
> > 
> > The second patch fixes a hang on Geode LX, b/c the no-op probing was done too
> > early - before the exception tables had even been setup.
> 
> Please don't lose the sparc section alignment bug fix I posted recently.

I wont, I'm currently busy doing my RH job ;-) I'll have it already
queued up, and will post later tonight.

-- Steve

Re: [PATCH 1/2] move arch_init_ideal_nop5 later

[Steven Rostedt]

On Wed, 2010-10-27 at 15:24 -0700, H. Peter Anvin wrote:
> The exception test is broken anyway for reasons I already explained, so it would be better to just drop it.
> 

Would you be OK if we nuke it for the next merge window? It's been in
there since 2.6.28, I'd hate to drop it now to discover that it breaks
something this late in the merge window.

-- Steve

Re: [PATCH 1/2] move arch_init_ideal_nop5 later

[H. Peter Anvin]

On 10/27/2010 07:11 PM, Steven Rostedt wrote:
> On Wed, 2010-10-27 at 15:24 -0700, H. Peter Anvin wrote:
>> The exception test is broken anyway for reasons I already explained, so it would be better to just drop it.
> 
> Would you be OK if we nuke it for the next merge window? It's been in
> there since 2.6.28, I'd hate to drop it now to discover that it breaks
> something this late in the merge window.
> 

I'd be much happier nuking it now than moving the invocation.  The
latter is much more likely to break things.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Jason Baron, rth]

On Wed, Oct 27, 2010 at 03:21:55PM -0700, H. Peter Anvin wrote:
> Could you detail the bug, please?  static_cpu_has() also uses asm goto.
> 

It seems that gcc can leave the stack in an inconsistent state.

I only saw this in the 'jmp' enabled case, I'm not sure if it can occur in the
straight-line code path as well.

thanks,

-Jason

> "Jason Baron" <jbaron@redhat.com> wrote:
> 
> >Unfortunately, we found a compiler bug in the implementation of
> >'asm goto'. The bug can cause the kernel to crash.
> >
> >For now, we are disabling jump labels with a big hammer. When the
> >gcc fix is committed, we will update the kernel with a better check
> >for either the version number it's fix in, or some detection of
> >whether gcc has the fix in place.
> >
> >Signed-off-by: Jason Baron <jbaron@redhat.com>
> >---
> > include/linux/jump_label.h |    9 +++++++++
> > 1 files changed, 9 insertions(+), 0 deletions(-)
> >
> >diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> >index b67cb18..954462a 100644
> >--- a/include/linux/jump_label.h
> >+++ b/include/linux/jump_label.h
> >@@ -1,11 +1,20 @@
> > #ifndef _LINUX_JUMP_LABEL_H
> > #define _LINUX_JUMP_LABEL_H
> > 
> >+/*
> >+ * A compiler bug was found in the implementation of 'asm goto'. Thus,
> >+ * we are disabling it for now pending a better check for compiler
> >version
> >+ * that fixes it.
> >+ */
> >+#if 0
> >+
> > #if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL)
> > # include <asm/jump_label.h>
> > # define HAVE_JUMP_LABEL
> > #endif
> > 
> >+#endif
> >+
> > enum jump_label_type {
> > 	JUMP_LABEL_ENABLE,
> > 	JUMP_LABEL_DISABLE
> >-- 
> >1.7.1
> 
> -- 
> Sent from my mobile phone.  Please pardon any lack of formatting.

Re: [PATCH 2/2] jump label: disable due to compiler bug

[David Daney]

On 10/28/2010 07:17 AM, Jason Baron wrote:
> On Wed, Oct 27, 2010 at 03:21:55PM -0700, H. Peter Anvin wrote:
>> Could you detail the bug, please?  static_cpu_has() also uses asm goto.
>>
>
> It seems that gcc can leave the stack in an inconsistent state.
>
> I only saw this in the 'jmp' enabled case, I'm not sure if it can occur in the
> straight-line code path as well.
>

Jason,

Were you going to file a GCC bug on this issue?  Someone should.

It would be a shame to have the problem persist due to lack of awareness 
by the GCC developers.

David Daney

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Jason Baron]

On Thu, Oct 28, 2010 at 11:55:32AM -0700, David Daney wrote:
> On 10/28/2010 07:17 AM, Jason Baron wrote:
>> On Wed, Oct 27, 2010 at 03:21:55PM -0700, H. Peter Anvin wrote:
>>> Could you detail the bug, please?  static_cpu_has() also uses asm goto.
>>>
>>
>> It seems that gcc can leave the stack in an inconsistent state.
>>
>> I only saw this in the 'jmp' enabled case, I'm not sure if it can occur in the
>> straight-line code path as well.
>>
>
> Jason,
>
> Were you going to file a GCC bug on this issue?  Someone should.
>

I will file one. I did inform Richard Henderson about the bug, and he
has already developed a fix. I'm not sure yet, when the fix will be
included in gcc.

Also, Richard said that -maccumulate-outgoing-args flag avoids this bug.
That flag is already always set for x86_64 (see arch/x86/Makefile), but not
for x86. This is also consistent with our testing. We have not seen the
crash on x86_64, only x86.

Thus, I'm going to propose a patch, which makes jump label conditional
on x86_64 for now (adding -maccumulate-outgoing-args to x86 increases
the text size, so I'm not proposing adding the flag there).

I'm not sure whether other architectures are exposed to this bug or not.
I'm trying to find out.

thanks,

-Jason

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Ingo Molnar]

* Jason Baron <jbaron@redhat.com> wrote:

> Thus, I'm going to propose a patch, which makes jump label conditional on x86_64 
> for now [...]

No, we dont do 64-bit only features really.

> [...] (adding -maccumulate-outgoing-args to x86 increases the text size, so I'm 
> not proposing adding the flag there).

Please make jump labels configurable instead. (dependent on EMBEDDED)

Also, could you please send the fix to me ASAP? We dont need confirmation and 
re-confirmation and testing, as this thing is crashing on me several times every 
single day ...

I also know that 64-bit never crashed this way, nor did the 32-bit kernel with 
function tracer enabled - so -maccumulate-outgoing-args will likely fix it.

Lets get the ball rolling so we can get it to Linus today ...

Thanks,

	Ingo

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Steven Rostedt]

On Fri, 2010-10-29 at 08:34 +0200, Ingo Molnar wrote:
> * Jason Baron <jbaron@redhat.com> wrote:
> 
> > Thus, I'm going to propose a patch, which makes jump label conditional on x86_64 
> > for now [...]
> 
> No, we dont do 64-bit only features really.
> 
> > [...] (adding -maccumulate-outgoing-args to x86 increases the text size, so I'm 
> > not proposing adding the flag there).
> 
> Please make jump labels configurable instead. (dependent on EMBEDDED)

Do you mean to have it dependent on !EMBEDDED, or perhaps 
!CC_OPTIMIZE_FOR_SIZE ?

I can see people making a big stink about adding back a gcc option that
makes the kernel bigger.

-- Steve

> 
> Also, could you please send the fix to me ASAP? We dont need confirmation and 
> re-confirmation and testing, as this thing is crashing on me several times every 
> single day ...
> 
> I also know that 64-bit never crashed this way, nor did the 32-bit kernel with 
> function tracer enabled - so -maccumulate-outgoing-args will likely fix it.
> 
> Lets get the ball rolling so we can get it to Linus today ...

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 2010-10-29 at 08:34 +0200, Ingo Molnar wrote:
> > * Jason Baron <jbaron@redhat.com> wrote:
> > 
> > > Thus, I'm going to propose a patch, which makes jump label conditional on x86_64 
> > > for now [...]
> > 
> > No, we dont do 64-bit only features really.
> > 
> > > [...] (adding -maccumulate-outgoing-args to x86 increases the text size, so I'm 
> > > not proposing adding the flag there).
> > 
> > Please make jump labels configurable instead. (dependent on EMBEDDED)
> 
> Do you mean to have it dependent on !EMBEDDED, or perhaps !CC_OPTIMIZE_FOR_SIZE ?

CC_OPTIMIZE_FOR_SIZE dependency probably not. Might even leave out !EMBEDDED, to 
make it generally configurable.

Also, we should make it default off - we need to see whether there are any compiler 
bugs lurking.

> I can see people making a big stink about adding back a gcc option that makes the 
> kernel bigger.

Depends on exactly how much .text we are talking about here. Plus it would be 
default-off.

Thanks,

	Ingo

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Steven Rostedt]

On Fri, 2010-10-29 at 14:22 +0200, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 2010-10-29 at 08:34 +0200, Ingo Molnar wrote:

> > Do you mean to have it dependent on !EMBEDDED, or perhaps !CC_OPTIMIZE_FOR_SIZE ?
> 
> CC_OPTIMIZE_FOR_SIZE dependency probably not. Might even leave out !EMBEDDED, to 
> make it generally configurable.
> 
> Also, we should make it default off - we need to see whether there are any compiler 
> bugs lurking.

Ah, so we should then add a:

config JUMP_LABEL
	prompt "Optimize trace point call sites"
	depends on HAVE_ARCH_JUMP_LABEL
	help
	  If it is detected that the compiler has support for
	  "asm goto", the kernel will compile trace points locations 
	  with just a nop instruction. When trace points are enabled
	  the nop will be converted to a jump to the trace function.
	  This technique lowers overhead and stress on the branch
	  prediction of the processor.

	  On i386, the options added to make this work may increase
	  the size of the kernel slightly.


Then in the code have:

-#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL)
+#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL) \
+	&& defined(JUMP_LABEL)
 # include <asm/jump_label.h>
 # define HAVE_JUMP_LABEL
 #endif


If you agree, I can write up a patch.

-- Steve
	  
> 
> > I can see people making a big stink about adding back a gcc option that makes the 
> > kernel bigger.
> 
> Depends on exactly how much .text we are talking about here. Plus it would be 
> default-off.
> 
> Thanks,
> 
> 	Ingo

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 2010-10-29 at 14:22 +0200, Ingo Molnar wrote:
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > On Fri, 2010-10-29 at 08:34 +0200, Ingo Molnar wrote:
> 
> > > Do you mean to have it dependent on !EMBEDDED, or perhaps !CC_OPTIMIZE_FOR_SIZE ?
> > 
> > CC_OPTIMIZE_FOR_SIZE dependency probably not. Might even leave out !EMBEDDED, to 
> > make it generally configurable.
> > 
> > Also, we should make it default off - we need to see whether there are any compiler 
> > bugs lurking.
> 
> Ah, so we should then add a:
> 
> config JUMP_LABEL
> 	prompt "Optimize trace point call sites"
> 	depends on HAVE_ARCH_JUMP_LABEL
> 	help
> 	  If it is detected that the compiler has support for
> 	  "asm goto", the kernel will compile trace points locations 
> 	  with just a nop instruction. When trace points are enabled
> 	  the nop will be converted to a jump to the trace function.
> 	  This technique lowers overhead and stress on the branch
> 	  prediction of the processor.
> 
> 	  On i386, the options added to make this work may increase
> 	  the size of the kernel slightly.

Yeah.

> Then in the code have:
> 
> -#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL)
> +#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL) \
> +	&& defined(JUMP_LABEL)
>  # include <asm/jump_label.h>
>  # define HAVE_JUMP_LABEL
>  #endif
> 
> 
> If you agree, I can write up a patch.

Yeah. Thanks!

	Ingo

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Fri, 2010-10-29 at 14:22 +0200, Ingo Molnar wrote:
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > On Fri, 2010-10-29 at 08:34 +0200, Ingo Molnar wrote:
> 
> > > Do you mean to have it dependent on !EMBEDDED, or perhaps !CC_OPTIMIZE_FOR_SIZE ?
> > 
> > CC_OPTIMIZE_FOR_SIZE dependency probably not. Might even leave out !EMBEDDED, to 
> > make it generally configurable.
> > 
> > Also, we should make it default off - we need to see whether there are any compiler 
> > bugs lurking.
> 
> Ah, so we should then add a:
> 
> config JUMP_LABEL
> 	prompt "Optimize trace point call sites"
> 	depends on HAVE_ARCH_JUMP_LABEL
> 	help
> 	  If it is detected that the compiler has support for
> 	  "asm goto", the kernel will compile trace points locations 
> 	  with just a nop instruction. When trace points are enabled
> 	  the nop will be converted to a jump to the trace function.
> 	  This technique lowers overhead and stress on the branch
> 	  prediction of the processor.
> 
> 	  On i386, the options added to make this work may increase
> 	  the size of the kernel slightly.
> 
> 
> Then in the code have:
> 
> -#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL)
> +#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL) \

Technically, you could remove the dependency on
"defined(CONFIG_HAVE_ARCH_JUMP_LABEL)", because it is now dealt with at
the Kconfig level (CONFIG_JUMP_LABEL depends on it).

> +	&& defined(JUMP_LABEL)

this would be defined(CONFIG_JUMP_LABEL)

The rest looks fine,

Thanks,

Mathieu

>  # include <asm/jump_label.h>
>  # define HAVE_JUMP_LABEL
>  #endif
> 
> 
> If you agree, I can write up a patch.
> 
> -- Steve
> 	  
> > 
> > > I can see people making a big stink about adding back a gcc option that makes the 
> > > kernel bigger.
> > 
> > Depends on exactly how much .text we are talking about here. Plus it would be 
> > default-off.
> > 
> > Thanks,
> > 
> > 	Ingo
> 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Richard Henderson]

On 10/28/2010 01:11 PM, Jason Baron wrote:
> I will file one. I did inform Richard Henderson about the bug, and he
> has already developed a fix. I'm not sure yet, when the fix will be
> included in gcc.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226

Which also includes a reduced test case.  I'll be checking in the fix
to the relevant branches in a moment.


r~

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Steven Rostedt]

On Fri, 2010-10-29 at 09:35 -0400, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
 
> > Then in the code have:
> > 
> > -#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL)
> > +#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL) \
> 
> Technically, you could remove the dependency on
> "defined(CONFIG_HAVE_ARCH_JUMP_LABEL)", because it is now dealt with at
> the Kconfig level (CONFIG_JUMP_LABEL depends on it).

Yeah, I noticed that just after hitting send.

> 
> > +	&& defined(JUMP_LABEL)
> 
> this would be defined(CONFIG_JUMP_LABEL)

Yep, this too, but thanks for pointing it out.


> 
> The rest looks fine,

Great, can I add your acked-by then?

-- Steve

Re: [PATCH 2/2] jump label: disable due to compiler bug

[David Daney]

On 10/29/2010 09:18 AM, Steven Rostedt wrote:
> On Fri, 2010-10-29 at 09:35 -0400, Mathieu Desnoyers wrote:
>> * Steven Rostedt (rostedt@goodmis.org) wrote:
>
>>> Then in the code have:
>>>
>>> -#if defined(CC_HAVE_ASM_GOTO)&&  defined(CONFIG_HAVE_ARCH_JUMP_LABEL)
>>> +#if defined(CC_HAVE_ASM_GOTO)&&  defined(CONFIG_HAVE_ARCH_JUMP_LABEL) \
>>
>> Technically, you could remove the dependency on
>> "defined(CONFIG_HAVE_ARCH_JUMP_LABEL)", because it is now dealt with at
>> the Kconfig level (CONFIG_JUMP_LABEL depends on it).
>
> Yeah, I noticed that just after hitting send.
>
>>
>>> +	&&  defined(JUMP_LABEL)
>>
>> this would be defined(CONFIG_JUMP_LABEL)
>
> Yep, this too, but thanks for pointing it out.
>
>
>>
>> The rest looks fine,
>
> Great, can I add your acked-by then?
>

I hate to continue beating a dead horse, but RTH just committed the GCC 
patch, and it will be part of GCC-4.5.2 and later.

Should this knowledge be builtin to the jump label enabling calculus?

David Daney

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Steven Rostedt]

On Fri, 2010-10-29 at 10:18 -0700, David Daney wrote:

> I hate to continue beating a dead horse, but RTH just committed the GCC 
> patch, and it will be part of GCC-4.5.2 and later.

Great!

> 
> Should this knowledge be builtin to the jump label enabling calculus?

No, because we can't trust versions. We never know what home grown gcc a
kernel developer is using (and what has been backported or not). Thus
the only option is to have a builtin test we can do at compile time to
determine if the bug exists or not and decide then.

Note, I'm currently running my last set of patches through ktest. When
it finishes (presumably with no issues), I'll post a pull request.

-- Steve

Re: [PATCH 2/2] jump label: disable due to compiler bug

[David Daney]

On 10/29/2010 10:33 AM, Steven Rostedt wrote:
> On Fri, 2010-10-29 at 10:18 -0700, David Daney wrote:
>
>> I hate to continue beating a dead horse, but RTH just committed the GCC
>> patch, and it will be part of GCC-4.5.2 and later.
>
> Great!
>
>>
>> Should this knowledge be builtin to the jump label enabling calculus?
>
> No, because we can't trust versions. We never know what home grown gcc a
> kernel developer is using (and what has been backported or not). Thus
> the only option is to have a builtin test we can do at compile time to
> determine if the bug exists or not and decide then.
>

The test case is in the GCC bug report, and is quite small:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226

Perhaps it could be integrated with the current checks for the presence 
of 'asm goto'

> Note, I'm currently running my last set of patches through ktest. When
> it finishes (presumably with no issues), I'll post a pull request.
>
> -- Steve
>
>

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Steven Rostedt]

On Fri, 2010-10-29 at 10:48 -0700, David Daney wrote:

> The test case is in the GCC bug report, and is quite small:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226
> 
> Perhaps it could be integrated with the current checks for the presence 
> of 'asm goto'

Yeah, but it's too late in the merge window to do this much hackery.
Lets slow down a bit and do it right. This test case shows that gcc is
broken, but do we really know if it wont produce a false positive
(saying gcc works when it does not)?

Adding a manual option for this release is the right thing to do. This
will let things settle down. Users that want it can enable it
themselves, and then they choose to be our testers ;-)

Once we have a better idea on things and have a test case we know shows
gcc works when it really does work and that it fails when it really does
fail, then we can add that in. Perhaps for 38.

-- Steve

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Richard Henderson]

On 10/29/2010 10:48 AM, David Daney wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226
> 
> Perhaps it could be integrated with the current checks for the presence of 'asm goto'

If you're looking for a cross-compile-able test, you can strip the
testcase down to 

extern void abort(void);
void g(int x, ...);
void f(int x)
{
  if (x == 0)
    g(0);
  g(1, 2, 3, 4, 5, 6, 7);
  asm goto ("XYZZY" : : : : label);
  abort ();
 label:
  return;
}

and use perl to notice the adjustment to esp before or after XYZZY.
It should be fairly obvious looking at the assembly before and after
the patch.

I don't know what sort of embedded processors supported by linux
actually use ACCUMULATE_OUTGOING_ARGS.  Almost all ports don't, so
I'd be pretty comfortable making this a 32-bit x86 only test, and
let other targets just check that the test compiles.


r~

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Steven Rostedt]

On Fri, 2010-10-29 at 11:13 -0700, Richard Henderson wrote:
> On 10/29/2010 10:48 AM, David Daney wrote:
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226
> > 
> > Perhaps it could be integrated with the current checks for the presence of 'asm goto'
> 
> If you're looking for a cross-compile-able test, you can strip the
> testcase down to 
> 
> extern void abort(void);
> void g(int x, ...);
> void f(int x)
> {
>   if (x == 0)
>     g(0);
>   g(1, 2, 3, 4, 5, 6, 7);
>   asm goto ("XYZZY" : : : : label);
>   abort ();
>  label:
>   return;
> }
> 
> and use perl to notice the adjustment to esp before or after XYZZY.
> It should be fairly obvious looking at the assembly before and after
> the patch.
> 
> I don't know what sort of embedded processors supported by linux
> actually use ACCUMULATE_OUTGOING_ARGS.  Almost all ports don't, so
> I'd be pretty comfortable making this a 32-bit x86 only test, and
> let other targets just check that the test compiles.
> 

Thanks! I'll make a patch with this check (for x86_32) and queue it up
for 2.6.38. I think we are way past doing this for 2.6.37.

-- Steve

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Fri, 2010-10-29 at 09:35 -0400, Mathieu Desnoyers wrote:
> > * Steven Rostedt (rostedt@goodmis.org) wrote:
>  
> > > Then in the code have:
> > > 
> > > -#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL)
> > > +#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_HAVE_ARCH_JUMP_LABEL) \
> > 
> > Technically, you could remove the dependency on
> > "defined(CONFIG_HAVE_ARCH_JUMP_LABEL)", because it is now dealt with at
> > the Kconfig level (CONFIG_JUMP_LABEL depends on it).
> 
> Yeah, I noticed that just after hitting send.
> 
> > 
> > > +	&& defined(JUMP_LABEL)
> > 
> > this would be defined(CONFIG_JUMP_LABEL)
> 
> Yep, this too, but thanks for pointing it out.
> 
> 
> > 
> > The rest looks fine,
> 
> Great, can I add your acked-by then?

As noted in my other email, please do :)

Thanks,

Mathieu

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/2] jump label: disable due to compiler bug

[H. Peter Anvin]

On 10/29/2010 10:33 AM, Steven Rostedt wrote:
>>
>> Should this knowledge be builtin to the jump label enabling calculus?
> 
> No, because we can't trust versions. We never know what home grown gcc a
> kernel developer is using (and what has been backported or not). Thus
> the only option is to have a builtin test we can do at compile time to
> determine if the bug exists or not and decide then.
> 
> Note, I'm currently running my last set of patches through ktest. When
> it finishes (presumably with no issues), I'll post a pull request.
> 

I disagree with that assessment.

We know that if version >= 4.5.2 the problem has been fixed, and that
for earlier versions we can't know if it's there, so just disable it for
gcc < 4.5.2.  The fix might have been backported, but it's not a big
deal if the users of backported compilers don't see the full benefit --
it's only a problem during a limited time window anyway.

Admittedly it would be nice to have a header file or even a
configuration file where the gcc version is tested and workarounds are
centralized; then the backporters could put their own overrides in there.

	-hpa

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Mathieu Desnoyers]

* H. Peter Anvin (hpa@zytor.com) wrote:
> On 10/29/2010 10:33 AM, Steven Rostedt wrote:
> >>
> >> Should this knowledge be builtin to the jump label enabling calculus?
> > 
> > No, because we can't trust versions. We never know what home grown gcc a
> > kernel developer is using (and what has been backported or not). Thus
> > the only option is to have a builtin test we can do at compile time to
> > determine if the bug exists or not and decide then.
> > 
> > Note, I'm currently running my last set of patches through ktest. When
> > it finishes (presumably with no issues), I'll post a pull request.
> > 
> 
> I disagree with that assessment.
> 
> We know that if version >= 4.5.2 the problem has been fixed, and that
> for earlier versions we can't know if it's there, so just disable it for
> gcc < 4.5.2.  The fix might have been backported, but it's not a big
> deal if the users of backported compilers don't see the full benefit --
> it's only a problem during a limited time window anyway.

I agree with HPA here. Moreover, we have no hard guarantee that the
automated assembly inspection proposed won't be breaked in the future
caused by changes in gcc. Compiling and running a user-level runtime
test at kernel build time does not work with cross-compilers, and
matching the exact options used for kernel builds might be hard.

I would tend to argue that the added longer-term maintainance burden of
adding such a test is far worse than leaving gcc < 4.5.2 users without
static jump patching, especially given that we have a fallback.

For now, we know that x86_64 works, but the standard setup for x86_32
kernel is broken. What do we do if testing shows that some other
architecture has similar issues ? Add more work-arounds ? At this early
stage, I would simply bump the gcc version dependency forward for all
architectures. Later on, after things have settled and as more and more
architectures are supported, we could deal with arch-specific breakages
on a per-architecture basis.

If this was a showstopper feature, my position would be different. But
the fact that we have been able to wait for gcc to integrate asm gotos
while adding tracepoints shows that it's not crucial in the short term.
We still have to be careful when adding new tracepoints anyway as long
as gcc < 4.5 is still widely used.

Thanks,

Mathieu

> 
> Admittedly it would be nice to have a header file or even a
> configuration file where the gcc version is tested and workarounds are
> centralized; then the backporters could put their own overrides in there.
> 
> 	-hpa

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Steven Rostedt]

On Fri, 2010-10-29 at 16:05 -0400, Mathieu Desnoyers wrote:

> > > The rest looks fine,
> > 
> > Great, can I add your acked-by then?
> 
> As noted in my other email, please do :)

Crap, I pushed it out already. Well, I have you Cc'd.

-- Steve

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Steven Rostedt]

On Fri, 2010-10-29 at 13:15 -0700, H. Peter Anvin wrote:

> I disagree with that assessment.
> 
> We know that if version >= 4.5.2 the problem has been fixed, and that
> for earlier versions we can't know if it's there, so just disable it for
> gcc < 4.5.2.  The fix might have been backported, but it's not a big
> deal if the users of backported compilers don't see the full benefit --
> it's only a problem during a limited time window anyway.
> 
> Admittedly it would be nice to have a header file or even a
> configuration file where the gcc version is tested and workarounds are
> centralized; then the backporters could put their own overrides in there.

Fine, but we can wait till 2.6.38 for that. For now we have a manual
config option. I'm not changing it until I have 4.5.2 in my distcc farm
and testing it out.

If things turn out fine, we can have > 4.5.2 || run check option. Or
just stick with 4.5.2. But we can debate this later (or at KS if you
want ;-)

-- Steve

Re: [PATCH 2/2] jump label: disable due to compiler bug

[H. Peter Anvin]

On 10/29/2010 01:47 PM, Steven Rostedt wrote:
> 
> Fine, but we can wait till 2.6.38 for that. For now we have a manual
> config option. I'm not changing it until I have 4.5.2 in my distcc farm
> and testing it out.
> 
> If things turn out fine, we can have > 4.5.2 || run check option. Or
> just stick with 4.5.2. But we can debate this later (or at KS if you
> want ;-)
> 

KS would be a good place to talk about toolchain dependencies in
general.  It's a big problem.

	-hpa

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Steven Rostedt]

On Fri, 2010-10-29 at 13:51 -0700, H. Peter Anvin wrote:

> 
> KS would be a good place to talk about toolchain dependencies in
> general.  It's a big problem.

Yeah, lets finally decide to implement kcc and put it in the tools
directory!

/me runs

-- Steve

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Andi Kleen]

> Fine, but we can wait till 2.6.38 for that. For now we have a manual
> config option. I'm not changing it until I have 4.5.2 in my distcc farm
> and testing it out.

It's probably obvious, but a manual config option still breaks
the allyes|modconfig crowd.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 2/2] jump label: disable due to compiler bug

[Steven Rostedt]

On Sat, 2010-10-30 at 09:47 +0200, Andi Kleen wrote:
> > Fine, but we can wait till 2.6.38 for that. For now we have a manual
> > config option. I'm not changing it until I have 4.5.2 in my distcc farm
> > and testing it out.
> 
> It's probably obvious, but a manual config option still breaks
> the allyes|modconfig crowd.

Enabling the config only adds the workaround which causes the kernel to
be a bit bigger (adds -maccumulate-outgoing-args) and does not break the
kernel. When we have the test, we wont need to add this workaround if
the test passes.

This is still a new feature, and until we are sure its safe, we should
have it as an option.

There's lots of things in the kernel that will break with
all{yes,mod}config. I know, because I keep hitting them with my
randconfig tests.


-- Steve