Re: [patch 8/9] F00F bug fixup for i386 - use conditional calls

Re: [patch 8/9] F00F bug fixup for i386 - use conditional calls

[Mikael Pettersson]

Andrew Morton wrote:
> Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> 
> > * Andrew Morton (akpm@linux-foundation.org) wrote:
> >  
> > > > Use the faster conditional calls for F00F bug handling in do_page_fault.
> > > > 
> > > 
> > > I guess this means that CONDCALL will be enabled on pretty much all i386,
> > > in which case making the whole feature Kconfigurable is starting to look
> > > marginal.
> > > 
> > > Perhaps a better approach would have to made this change dependent upon
> > > CONDCALL, rather than forcing it on.
> > > 
> > 
> > Do you mean making X86_F00F_BUG depend on COND_CALL instead of selecting
> > it ?
> 
> yup

X86_F00F_BUG needs to be enabled in all kernels capable of booting on
P5 class machines, whether or not some obscure CONFIG_COND_CALL thingy
is enabled or not. X86_F00F_BUG is not some optional optimisation, it's
an essential workaround for a serious hardware bug.

Therefore it seems select rather than depend is called for.

Re: [patch 8/9] F00F bug fixup for i386 - use conditional calls

[Andrew Morton]

On Fri, 1 Jun 2007 02:14:53 +0200 (MEST)
Mikael Pettersson <mikpe@it.uu.se> wrote:

> Andrew Morton wrote:
> > Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> > 
> > > * Andrew Morton (akpm@linux-foundation.org) wrote:
> > >  
> > > > > Use the faster conditional calls for F00F bug handling in do_page_fault.
> > > > > 
> > > > 
> > > > I guess this means that CONDCALL will be enabled on pretty much all i386,
> > > > in which case making the whole feature Kconfigurable is starting to look
> > > > marginal.
> > > > 
> > > > Perhaps a better approach would have to made this change dependent upon
> > > > CONDCALL, rather than forcing it on.
> > > > 
> > > 
> > > Do you mean making X86_F00F_BUG depend on COND_CALL instead of selecting
> > > it ?
> > 
> > yup
> 
> X86_F00F_BUG needs to be enabled in all kernels capable of booting on
> P5 class machines, whether or not some obscure CONFIG_COND_CALL thingy
> is enabled or not. X86_F00F_BUG is not some optional optimisation, it's
> an essential workaround for a serious hardware bug.
> 
> Therefore it seems select rather than depend is called for.

Nope.

CONFIG_COND_CALL=n	-> 	do f00f handling the present way
CONFIG_COND_CALL=y	-> 	do f00f handling the new, fast-n-fancy way


Because I don't think everyone will want to drag all this cond_call stuff
into their kernel just for slightly faster f00f handling.

Re: [patch 8/9] F00F bug fixup for i386 - use conditional calls

[Mathieu Desnoyers]

* Andrew Morton (akpm@linux-foundation.org) wrote:
> On Fri, 1 Jun 2007 02:14:53 +0200 (MEST)
> Mikael Pettersson <mikpe@it.uu.se> wrote:
> 
> > Andrew Morton wrote:
> > > Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> > > 
> > > > * Andrew Morton (akpm@linux-foundation.org) wrote:
> > > >  
> > > > > > Use the faster conditional calls for F00F bug handling in do_page_fault.
> > > > > > 
> > > > > 
> > > > > I guess this means that CONDCALL will be enabled on pretty much all i386,
> > > > > in which case making the whole feature Kconfigurable is starting to look
> > > > > marginal.
> > > > > 
> > > > > Perhaps a better approach would have to made this change dependent upon
> > > > > CONDCALL, rather than forcing it on.
> > > > > 
> > > > 
> > > > Do you mean making X86_F00F_BUG depend on COND_CALL instead of selecting
> > > > it ?
> > > 
> > > yup
> > 
> > X86_F00F_BUG needs to be enabled in all kernels capable of booting on
> > P5 class machines, whether or not some obscure CONFIG_COND_CALL thingy
> > is enabled or not. X86_F00F_BUG is not some optional optimisation, it's
> > an essential workaround for a serious hardware bug.
> > 
> > Therefore it seems select rather than depend is called for.
> 
> Nope.
> 
> CONFIG_COND_CALL=n	-> 	do f00f handling the present way
> CONFIG_COND_CALL=y	-> 	do f00f handling the new, fast-n-fancy way
> 
> 
> Because I don't think everyone will want to drag all this cond_call stuff
> into their kernel just for slightly faster f00f handling.
> 

Then I should probably leave the redundant 
if (boot_cpu_data.f00f_bug)
test in do_f00f_workaround so that configuration where cond_calls are
not enabled still behave the same way as before. The redundant test will
make the case when cond calls are present a little slower, but we still
have the improvement when the architecture does not need the workaround.
I could also wrap this test in a #ifndef CONFIG_COND_CALL, but it seems
ugly.

Also, since the default is not to set the CF_STATIC_ENABLE flag, the
workaround won't be compiled in the kernel if the conditional calls are
configured out.

The question that arises is : what should be the default behavior of a
cond_call when the cond_calls are configured out? Active, active only
if the CF_STATIC_ENABLE flags is set or inactive?


-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

[patch 0/9] Conditional Calls - for 2.6.22-rc2-mm1

[Mathieu Desnoyers]

Hi,

Here is the port of the conditional calls to 2.6.22-rc2-mm1.

Quoting my last post:

Following Andi Kleen's advice, I splitted the jump-over-call into a speparate
piece of infrastructure so it can be used more widely.

It also use a hash table to permit cond_call enable/disable both when the
cond_call is armed and when a module containing an armed cond_call is loaded.

Please add at the end of the 2.6.22-rc2-mm1 series,

conditional-calls-architecture-independent-code.patch
conditional-calls-hash-table.patch
conditional-calls-non-optimized-architectures.patch
conditional-calls-kconfig-menus.patch
conditional-calls-i386-optimization.patch
conditional-calls-powerpc-optimization.patch
conditional-calls-documentation.patch
#
f00f-bug-use-cond-calls.patch
profiling-use-cond-calls.patch

Mathieu


-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

[patch 8/9] F00F bug fixup for i386 - use conditional calls

[Mathieu Desnoyers]

[-- Attachment #1: f00f-bug-use-cond-calls.patch --]
[-- Type: text/plain, Size: 2891 bytes --]

Use the faster conditional calls for F00F bug handling in do_page_fault.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

---
 arch/i386/Kconfig.cpu    |    1 +
 arch/i386/kernel/traps.c |    2 ++
 arch/i386/mm/fault.c     |   35 ++++++++++++++++++++++-------------
 3 files changed, 25 insertions(+), 13 deletions(-)

Index: linux-2.6-lttng/arch/i386/kernel/traps.c
===================================================================
--- linux-2.6-lttng.orig/arch/i386/kernel/traps.c	2007-05-29 11:05:30.000000000 -0400
+++ linux-2.6-lttng/arch/i386/kernel/traps.c	2007-05-29 11:07:07.000000000 -0400
@@ -31,6 +31,7 @@
 #include <linux/uaccess.h>
 #include <linux/nmi.h>
 #include <linux/bug.h>
+#include <linux/condcall.h>
 
 #ifdef CONFIG_EISA
 #include <linux/ioport.h>
@@ -1084,6 +1085,7 @@
 	 */
 	idt_descr.address = fix_to_virt(FIX_F00F_IDT);
 	load_idt(&idt_descr);
+	BUG_ON(cond_call_arm("fix_f00f"));
 }
 #endif
 
Index: linux-2.6-lttng/arch/i386/mm/fault.c
===================================================================
--- linux-2.6-lttng.orig/arch/i386/mm/fault.c	2007-05-29 11:05:48.000000000 -0400
+++ linux-2.6-lttng/arch/i386/mm/fault.c	2007-05-29 11:13:16.000000000 -0400
@@ -25,6 +25,7 @@
 #include <linux/kprobes.h>
 #include <linux/uaccess.h>
 #include <linux/kdebug.h>
+#include <linux/condcall.h>
 
 #include <asm/system.h>
 #include <asm/desc.h>
@@ -221,6 +222,25 @@
 
 fastcall void do_invalid_op(struct pt_regs *, unsigned long);
 
+#ifdef CONFIG_X86_F00F_BUG
+/*
+ * Pentium F0 0F C7 C8 bug workaround.
+ */
+static inline int do_f00f_workaround(struct pt_regs *regs,
+		unsigned long address)
+{
+	unsigned long nr;
+
+	nr = (address - idt_descr.address) >> 3;
+
+	if (nr == 6) {
+		do_invalid_op(regs, 0);
+		return 1;
+	}
+	return 0;
+}
+#endif
+
 static inline pmd_t *vmalloc_sync_one(pgd_t *pgd, unsigned long address)
 {
 	unsigned index = pgd_index(address);
@@ -474,19 +494,8 @@
 	}
 
 #ifdef CONFIG_X86_F00F_BUG
-	/*
-	 * Pentium F0 0F C7 C8 bug workaround.
-	 */
-	if (boot_cpu_data.f00f_bug) {
-		unsigned long nr;
-		
-		nr = (address - idt_descr.address) >> 3;
-
-		if (nr == 6) {
-			do_invalid_op(regs, 0);
-			return;
-		}
-	}
+	if (cond_call(fix_f00f, do_f00f_workaround(regs, address)))
+		return;
 #endif
 
 no_context:
Index: linux-2.6-lttng/arch/i386/Kconfig.cpu
===================================================================
--- linux-2.6-lttng.orig/arch/i386/Kconfig.cpu	2007-05-29 11:51:46.000000000 -0400
+++ linux-2.6-lttng/arch/i386/Kconfig.cpu	2007-05-29 11:52:08.000000000 -0400
@@ -275,6 +275,7 @@
 config X86_F00F_BUG
 	bool
 	depends on M586MMX || M586TSC || M586 || M486 || M386
+	select COND_CALL
 	default y
 
 config X86_WP_WORKS_OK

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [patch 8/9] F00F bug fixup for i386 - use conditional calls

[Andrew Morton]

On Wed, 30 May 2007 10:00:33 -0400
Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:

> Use the faster conditional calls for F00F bug handling in do_page_fault.
> 

I guess this means that CONDCALL will be enabled on pretty much all i386,
in which case making the whole feature Kconfigurable is starting to look
marginal.

Perhaps a better approach would have to made this change dependent upon
CONDCALL, rather than forcing it on.

> @@ -1084,6 +1085,7 @@
>  	 */
>  	idt_descr.address = fix_to_virt(FIX_F00F_IDT);
>  	load_idt(&idt_descr);
> +	BUG_ON(cond_call_arm("fix_f00f"));

It is generally poor C style to do

	assert(something_which_has_side_effects())

because people can legitimately expect to do

#define assert() /*nothing*/

for production code.

The kernel doesn't actually do the right thing here when CONFIG_BUG=n,
sadly.  But still, children might be watching, so the better and preferred
style is

	if (cond_call_arm("fix_f00f"))
		BUG();

>  }
>  #endif
>  
> Index: linux-2.6-lttng/arch/i386/mm/fault.c
> ===================================================================
> --- linux-2.6-lttng.orig/arch/i386/mm/fault.c	2007-05-29 11:05:48.000000000 -0400
> +++ linux-2.6-lttng/arch/i386/mm/fault.c	2007-05-29 11:13:16.000000000 -0400
> @@ -25,6 +25,7 @@
>  #include <linux/kprobes.h>
>  #include <linux/uaccess.h>
>  #include <linux/kdebug.h>
> +#include <linux/condcall.h>
>  
>  #include <asm/system.h>
>  #include <asm/desc.h>
> @@ -221,6 +222,25 @@
>  
>  fastcall void do_invalid_op(struct pt_regs *, unsigned long);
>  
> +#ifdef CONFIG_X86_F00F_BUG
> +/*
> + * Pentium F0 0F C7 C8 bug workaround.
> + */
> +static inline int do_f00f_workaround(struct pt_regs *regs,
> +		unsigned long address)
> +{
> +	unsigned long nr;
> +
> +	nr = (address - idt_descr.address) >> 3;
> +
> +	if (nr == 6) {
> +		do_invalid_op(regs, 0);
> +		return 1;
> +	}
> +	return 0;
> +}
> +#endif
> +
>  static inline pmd_t *vmalloc_sync_one(pgd_t *pgd, unsigned long address)
>  {
>  	unsigned index = pgd_index(address);
> @@ -474,19 +494,8 @@
>  	}
>  
>  #ifdef CONFIG_X86_F00F_BUG
> -	/*
> -	 * Pentium F0 0F C7 C8 bug workaround.
> -	 */
> -	if (boot_cpu_data.f00f_bug) {
> -		unsigned long nr;
> -		
> -		nr = (address - idt_descr.address) >> 3;
> -
> -		if (nr == 6) {
> -			do_invalid_op(regs, 0);
> -			return;
> -		}
> -	}
> +	if (cond_call(fix_f00f, do_f00f_workaround(regs, address)))
> +		return;

We do a cond_call() to an inlined function?  That's a bit weird, isn't it?

>  #endif
>  
>  no_context:
> Index: linux-2.6-lttng/arch/i386/Kconfig.cpu
> ===================================================================
> --- linux-2.6-lttng.orig/arch/i386/Kconfig.cpu	2007-05-29 11:51:46.000000000 -0400
> +++ linux-2.6-lttng/arch/i386/Kconfig.cpu	2007-05-29 11:52:08.000000000 -0400
> @@ -275,6 +275,7 @@
>  config X86_F00F_BUG
>  	bool
>  	depends on M586MMX || M586TSC || M586 || M486 || M386
> +	select COND_CALL

That hurts.

Re: [patch 8/9] F00F bug fixup for i386 - use conditional calls

[Mathieu Desnoyers]

* Andrew Morton (akpm@linux-foundation.org) wrote:
 
> > Use the faster conditional calls for F00F bug handling in do_page_fault.
> > 
> 
> I guess this means that CONDCALL will be enabled on pretty much all i386,
> in which case making the whole feature Kconfigurable is starting to look
> marginal.
> 
> Perhaps a better approach would have to made this change dependent upon
> CONDCALL, rather than forcing it on.
> 

Do you mean making X86_F00F_BUG depend on COND_CALL instead of selecting
it ?

> > +	if (cond_call(fix_f00f, do_f00f_workaround(regs, address)))
> > +		return;
> 
> We do a cond_call() to an inlined function?  That's a bit weird, isn't it?
> 

Yes, but it works :) I will add this information to the documentation.

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [patch 8/9] F00F bug fixup for i386 - use conditional calls

[Andrew Morton]

On Thu, 31 May 2007 17:07:55 -0400
Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:

> * Andrew Morton (akpm@linux-foundation.org) wrote:
>  
> > > Use the faster conditional calls for F00F bug handling in do_page_fault.
> > > 
> > 
> > I guess this means that CONDCALL will be enabled on pretty much all i386,
> > in which case making the whole feature Kconfigurable is starting to look
> > marginal.
> > 
> > Perhaps a better approach would have to made this change dependent upon
> > CONDCALL, rather than forcing it on.
> > 
> 
> Do you mean making X86_F00F_BUG depend on COND_CALL instead of selecting
> it ?

yup

> > > +	if (cond_call(fix_f00f, do_f00f_workaround(regs, address)))
> > > +		return;
> > 
> > We do a cond_call() to an inlined function?  That's a bit weird, isn't it?
> > 
> 
> Yes, but it works :) I will add this information to the documentation.

But why does it work?  Did the compiler generate an out-of-line copy
of the function?  If so, we'll end up with multiple copies of the function if
there are other callers.  If not, the `inline' was pointless.

Re: [patch 8/9] F00F bug fixup for i386 - use conditional calls

[Mathieu Desnoyers]

* Andrew Morton (akpm@linux-foundation.org) wrote:
> On Thu, 31 May 2007 17:07:55 -0400
> Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> 
> > * Andrew Morton (akpm@linux-foundation.org) wrote:
> >  
> > > > Use the faster conditional calls for F00F bug handling in do_page_fault.
> > > > 
> > > 
> > > I guess this means that CONDCALL will be enabled on pretty much all i386,
> > > in which case making the whole feature Kconfigurable is starting to look
> > > marginal.
> > > 
> > > Perhaps a better approach would have to made this change dependent upon
> > > CONDCALL, rather than forcing it on.
> > > 
> > 
> > Do you mean making X86_F00F_BUG depend on COND_CALL instead of selecting
> > it ?
> 
> yup
> 
> > > > +	if (cond_call(fix_f00f, do_f00f_workaround(regs, address)))
> > > > +		return;
> > > 
> > > We do a cond_call() to an inlined function?  That's a bit weird, isn't it?
> > > 
> > 
> > Yes, but it works :) I will add this information to the documentation.
> 
> But why does it work?  Did the compiler generate an out-of-line copy
> of the function?  If so, we'll end up with multiple copies of the function if
> there are other callers.  If not, the `inline' was pointless.

Yes, it's doing an out-of-line copy. Just like the compiler would have
done if we place a call to an inline function within a if() { } block.

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

[patch 0/9] Conditional Calls

[Mathieu Desnoyers]

Hi,

Following Andi Kleen's advice, I splitted the jump-over-call into a speparate
piece of infrastructure so it can be used more widely.

It also use a hash table to permit cond_call enable/disable both when the
cond_call is armed and when a module containing an armed cond_call is loaded.

Please add at the end of the 2.6.22-mm1 series,

conditional-calls-architecture-independent-code.patch
conditional-calls-hash-table.patch
conditional-calls-non-optimized-architectures.patch
conditional-calls-kconfig-menus.patch
conditional-calls-i386-optimization.patch
conditional-calls-powerpc-optimization.patch
conditional-calls-documentation.patch
#
f00f-bug-use-cond-calls.patch
profiling-use-cond-calls.patch

Mathieu

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

[patch 8/9] F00F bug fixup for i386 - use conditional calls

[Mathieu Desnoyers]

[-- Attachment #1: f00f-bug-use-cond-calls.patch --]
[-- Type: text/plain, Size: 2891 bytes --]

Use the faster conditional calls for F00F bug handling in do_page_fault.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

---
 arch/i386/Kconfig.cpu    |    1 +
 arch/i386/kernel/traps.c |    2 ++
 arch/i386/mm/fault.c     |   35 ++++++++++++++++++++++-------------
 3 files changed, 25 insertions(+), 13 deletions(-)

Index: linux-2.6-lttng/arch/i386/kernel/traps.c
===================================================================
--- linux-2.6-lttng.orig/arch/i386/kernel/traps.c	2007-05-29 11:05:30.000000000 -0400
+++ linux-2.6-lttng/arch/i386/kernel/traps.c	2007-05-29 11:07:07.000000000 -0400
@@ -31,6 +31,7 @@
 #include <linux/uaccess.h>
 #include <linux/nmi.h>
 #include <linux/bug.h>
+#include <linux/condcall.h>
 
 #ifdef CONFIG_EISA
 #include <linux/ioport.h>
@@ -1084,6 +1085,7 @@
 	 */
 	idt_descr.address = fix_to_virt(FIX_F00F_IDT);
 	load_idt(&idt_descr);
+	BUG_ON(cond_call_arm("fix_f00f"));
 }
 #endif
 
Index: linux-2.6-lttng/arch/i386/mm/fault.c
===================================================================
--- linux-2.6-lttng.orig/arch/i386/mm/fault.c	2007-05-29 11:05:48.000000000 -0400
+++ linux-2.6-lttng/arch/i386/mm/fault.c	2007-05-29 11:13:16.000000000 -0400
@@ -25,6 +25,7 @@
 #include <linux/kprobes.h>
 #include <linux/uaccess.h>
 #include <linux/kdebug.h>
+#include <linux/condcall.h>
 
 #include <asm/system.h>
 #include <asm/desc.h>
@@ -221,6 +222,25 @@
 
 fastcall void do_invalid_op(struct pt_regs *, unsigned long);
 
+#ifdef CONFIG_X86_F00F_BUG
+/*
+ * Pentium F0 0F C7 C8 bug workaround.
+ */
+static inline int do_f00f_workaround(struct pt_regs *regs,
+		unsigned long address)
+{
+	unsigned long nr;
+
+	nr = (address - idt_descr.address) >> 3;
+
+	if (nr == 6) {
+		do_invalid_op(regs, 0);
+		return 1;
+	}
+	return 0;
+}
+#endif
+
 static inline pmd_t *vmalloc_sync_one(pgd_t *pgd, unsigned long address)
 {
 	unsigned index = pgd_index(address);
@@ -474,19 +494,8 @@
 	}
 
 #ifdef CONFIG_X86_F00F_BUG
-	/*
-	 * Pentium F0 0F C7 C8 bug workaround.
-	 */
-	if (boot_cpu_data.f00f_bug) {
-		unsigned long nr;
-		
-		nr = (address - idt_descr.address) >> 3;
-
-		if (nr == 6) {
-			do_invalid_op(regs, 0);
-			return;
-		}
-	}
+	if (cond_call(fix_f00f, do_f00f_workaround(regs, address)))
+		return;
 #endif
 
 no_context:
Index: linux-2.6-lttng/arch/i386/Kconfig.cpu
===================================================================
--- linux-2.6-lttng.orig/arch/i386/Kconfig.cpu	2007-05-29 11:51:46.000000000 -0400
+++ linux-2.6-lttng/arch/i386/Kconfig.cpu	2007-05-29 11:52:08.000000000 -0400
@@ -275,6 +275,7 @@
 config X86_F00F_BUG
 	bool
 	depends on M586MMX || M586TSC || M586 || M486 || M386
+	select COND_CALL
 	default y
 
 config X86_WP_WORKS_OK

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68