[PATCH 0/2] i386: MCE updates

[PATCH 0/2] i386: MCE updates

[Joerg Roedel]

This patchset includes two patches. 

1. Checks for the MCA fetures as early as possible.
2. Coding style cleanup.

[PATCH 1/2] i386: mce cleanup part1: functional change

[Joerg Roedel]

MCG_CAP never reports a negative count of available error-reporting banks.
Therefore, make it unsigned.
Check for MCA/MCE feature bits as early as possible.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/i386/kernel/cpu/mcheck/mce.c |   13 ++++++++++++-
 arch/i386/kernel/cpu/mcheck/mce.h |    2 +-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/i386/kernel/cpu/mcheck/mce.c b/arch/i386/kernel/cpu/mcheck/mce.c
index 34c781e..bad2c64 100644
--- a/arch/i386/kernel/cpu/mcheck/mce.c
+++ b/arch/i386/kernel/cpu/mcheck/mce.c
@@ -17,7 +17,7 @@
 #include "mce.h"
 
 int mce_disabled = 0;
-int nr_mce_banks;
+unsigned int nr_mce_banks;
 
 EXPORT_SYMBOL_GPL(nr_mce_banks);	/* non-fatal.o */
 
@@ -33,9 +33,20 @@ void fastcall (*machine_check_vector)(struct pt_regs *, long error_code) = unexp
 /* This has to be run for each processor */
 void mcheck_init(struct cpuinfo_x86 *c)
 {
+	uint32_t mca, mce;
+
 	if (mce_disabled==1)
 		return;
 
+	mca = cpu_has(c, X86_FEATURE_MCA);
+	mce = cpu_has(c, X86_FEATURE_MCE);
+
+	if (!mca || !mce) {
+		printk(KERN_INFO "CPU%i: No machine check support available\n",
+			smp_processor_id());
+		return;
+	}
+
 	switch (c->x86_vendor) {
 		case X86_VENDOR_AMD:
 			amd_mcheck_init(c);
diff --git a/arch/i386/kernel/cpu/mcheck/mce.h b/arch/i386/kernel/cpu/mcheck/mce.h
index 81fb6e2..9cbe812 100644
--- a/arch/i386/kernel/cpu/mcheck/mce.h
+++ b/arch/i386/kernel/cpu/mcheck/mce.h
@@ -10,5 +10,5 @@ void winchip_mcheck_init(struct cpuinfo_x86 *c);
 /* Call the installed machine check handler for this CPU setup. */
 extern fastcall void (*machine_check_vector)(struct pt_regs *, long error_code);
 
-extern int nr_mce_banks;
+extern unsigned int nr_mce_banks;
 
-- 
1.5.2.5

Re: [PATCH 1/2] i386: mce cleanup part1: functional change

[Oleg Verych]

* Tue,  9 Oct 2007 14:49:55 +0200

[]
> @@ -33,9 +33,20 @@ void fastcall (*machine_check_vector)(struct pt_regs *, long error_code) = unexp
>  /* This has to be run for each processor */
>  void mcheck_init(struct cpuinfo_x86 *c)
>  {
> +	uint32_t mca, mce;
> +
>  	if (mce_disabled==1)
>  		return;
>  
> +	mca = cpu_has(c, X86_FEATURE_MCA);
> +	mce = cpu_has(c, X86_FEATURE_MCE);
> +
> +	if (!mca || !mce) {
> +		printk(KERN_INFO "CPU%i: No machine check support available\n",
> +			smp_processor_id());
> +		return;
> +	}
> +

cpu_has() returns int,
but would it be better to have something like

  	if (!mce_disabled &&
	    !(c->x86_capability & (X86_FEATURE_MCA | X86_FEATURE_MCE)) {
		printk(KERN_INFO "CPU%i: No machine check support available\n",
			smp_processor_id());
		return;
	} else
		return;
?
____

Re: [PATCH 1/2] i386: mce cleanup part1: functional change

[Joerg Roedel]

On Tue, Oct 09, 2007 at 06:04:48PM +0200, Oleg Verych wrote:
> * Tue,  9 Oct 2007 14:49:55 +0200
> 
> []
> > @@ -33,9 +33,20 @@ void fastcall (*machine_check_vector)(struct pt_regs *, long error_code) = unexp
> >  /* This has to be run for each processor */
> >  void mcheck_init(struct cpuinfo_x86 *c)
> >  {
> > +	uint32_t mca, mce;
> > +
> >  	if (mce_disabled==1)
> >  		return;
> >  
> > +	mca = cpu_has(c, X86_FEATURE_MCA);
> > +	mce = cpu_has(c, X86_FEATURE_MCE);
> > +
> > +	if (!mca || !mce) {
> > +		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > +			smp_processor_id());
> > +		return;
> > +	}
> > +
> 
> cpu_has() returns int,
> but would it be better to have something like
> 
>   	if (!mce_disabled &&
> 	    !(c->x86_capability & (X86_FEATURE_MCA | X86_FEATURE_MCE)) {
> 		printk(KERN_INFO "CPU%i: No machine check support available\n",
> 			smp_processor_id());

This looks complicated and is harder to read. Its exactly the purpose of the
cpu_has() macro to avoid such constructs.

> 		return;
> 	} else
> 		return;

Return unconditionaly here?

-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

Re: [PATCH 1/2] i386: mce cleanup part1: functional change

[Oleg Verych]

On Tue, Oct 09, 2007 at 06:06:05PM +0200, Joerg Roedel wrote:
> > cpu_has() returns int,
> > but would it be better to have something like
> > 
> >   	if (!mce_disabled &&
> > 	    !(c->x86_capability & (X86_FEATURE_MCA | X86_FEATURE_MCE)) {
> > 		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > 			smp_processor_id());
> 
> This looks complicated and is harder to read. Its exactly the purpose of the
> cpu_has() macro to avoid such constructs.

It is done via test_bit(), which is designed for IO access with all that
`const volatile' stuff, 2 x unnecessary, can't be optimized here (IMHO).
____

Re: [PATCH 1/2] i386: mce cleanup part1: functional change

[Joerg Roedel]

On Tue, Oct 09, 2007 at 06:32:30PM +0200, Oleg Verych wrote:
> On Tue, Oct 09, 2007 at 06:06:05PM +0200, Joerg Roedel wrote:
> > > cpu_has() returns int,
> > > but would it be better to have something like
> > > 
> > >   	if (!mce_disabled &&
> > > 	    !(c->x86_capability & (X86_FEATURE_MCA | X86_FEATURE_MCE)) {
> > > 		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > > 			smp_processor_id());
> > 
> > This looks complicated and is harder to read. Its exactly the purpose of the
> > cpu_has() macro to avoid such constructs.
> 
> It is done via test_bit(), which is designed for IO access with all that
> `const volatile' stuff, 2 x unnecessary, can't be optimized here (IMHO).

This code runs only on bootup and once per cpu. I think readable code
is more important here than make it a few cycles faster.

-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

Re: [PATCH 1/2] i386: mce cleanup part1: functional change

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

On Tue, 09 Oct 2007 18:32:30 +0200, Oleg Verych said:
> On Tue, Oct 09, 2007 at 06:06:05PM +0200, Joerg Roedel wrote:
> > > cpu_has() returns int,
> > > but would it be better to have something like
> > > 
> > >   	if (!mce_disabled &&
> > > 	    !(c->x86_capability & (X86_FEATURE_MCA | X86_FEATURE_MCE)) {
> > > 		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > > 			smp_processor_id());
> > 
> > This looks complicated and is harder to read. Its exactly the purpose of the
> > cpu_has() macro to avoid such constructs.
> 
> It is done via test_bit(), which is designed for IO access with all that
> `const volatile' stuff, 2 x unnecessary, can't be optimized here (IMHO).

If this code is getting called often enough that optimization matters, you
got *bigger* issues to worry about than optimization.  Looks like it should
only happen once at boot time.

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH 1/2] i386: mce cleanup part1: functional change

[Oleg Verych]

On Tue, Oct 09, 2007 at 04:46:46PM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Tue, 09 Oct 2007 18:32:30 +0200, Oleg Verych said:
> > On Tue, Oct 09, 2007 at 06:06:05PM +0200, Joerg Roedel wrote:
> > > > cpu_has() returns int,
> > > > but would it be better to have something like
> > > > 
> > > >   	if (!mce_disabled &&
> > > > 	    !(c->x86_capability & (X86_FEATURE_MCA | X86_FEATURE_MCE)) {
> > > > 		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > > > 			smp_processor_id());
> > > 
> > > This looks complicated and is harder to read. Its exactly the purpose of the
> > > cpu_has() macro to avoid such constructs.
> > 
> > It is done via test_bit(), which is designed for IO access with all that
> > `const volatile' stuff, 2 x unnecessary, can't be optimized here (IMHO).
> 
> If this code is getting called often enough that optimization matters, you
> got *bigger* issues to worry about than optimization.  Looks like it should
> only happen once at boot time.

Text size matters even on static storages. A Linuxbios image not fitting
to the 2M flash, etc.

Thanks.
____

coding for optimizations (Re: [PATCH 1/2] i386: mce cleanup part1: functional change)

[Oleg Verych]

On Tue, Oct 09, 2007 at 06:06:05PM +0200, Joerg Roedel wrote:
> > >  void mcheck_init(struct cpuinfo_x86 *c)
> > >  {
> > > +	uint32_t mca, mce;
> > > +
> > >  	if (mce_disabled==1)
> > >  		return;
> > >  
> > > +	mca = cpu_has(c, X86_FEATURE_MCA);
> > > +	mce = cpu_has(c, X86_FEATURE_MCE);
> > > +
> > > +	if (!mca || !mce) {
> > > +		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > > +			smp_processor_id());
> > > +		return;
> > > +	}
> > > +
> > 
> > cpu_has() returns int,
> > but would it be better to have something like
> > 
> >   	if (!mce_disabled &&
> > 	    !(c->x86_capability & (X86_FEATURE_MCA | X86_FEATURE_MCE)) {
> > 		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > 			smp_processor_id());
> 
> This looks complicated and is harder to read. Its exactly the purpose of the
> cpu_has() macro to avoid such constructs.
> 
> > 		return;
> > 	} else
> > 		return;
> 
> Return unconditionaly here?

Kind of typo.

Due to arch code, i think, it must be coded in non-gcc optimistic way. As
practice and Linus shows, gcc's optimizations sometimes produce very
unexpected results. People do spaghetti-like coding, without thinking
about text size / run time.

Compile time payment was noted by Andrew many years ago:

"As you know, I'd be more concerned about moves to drop support for the
older and much faster gcc versions.  If you're not using egcs-1.1.2,
you're already a very patient person." <http://lkml.org/lkml/2001/12/29/163>

So, i actually wanted to write this:

	if (!mce_disabled) {
	if (!(c->x86_capability & (X86_FEATURE_MCA | X86_FEATURE_MCE)) {
		printk(KERN_INFO "CPU%i: No machine check support available\n",
 			smp_processor_id());
		return;
	}
	/* function code */
	}

(ugly, because `mce_disabled == 1' check is even in gcc is likely by
default). But changed my mind, due to violation of the coding style.
Of course this my be no appropriate at all, but i'd like to bring
this, if anyone would be like to discuss this.
____

Re: coding for optimizations (Re: [PATCH 1/2] i386: mce cleanup part1: functional change)

[Joerg Roedel]

On Tue, Oct 09, 2007 at 07:33:17PM +0200, olecom@flower.upol.cz wrote:
> 	if (!mce_disabled) {
> 	if (!(c->x86_capability & (X86_FEATURE_MCA | X86_FEATURE_MCE)) {
> 		printk(KERN_INFO "CPU%i: No machine check support available\n",
>  			smp_processor_id());
> 		return;
> 	}
> 	/* function code */
> 	}

I have 2 problems with this way:

1) It is totally broken because c->x86_capability is not an integer but
   an *array* of integers. This is also the reason why test_bit() is used
   in cpu_has() and not plain logical operators.

2) It is still hard to read and breaks the kernel coding style.

But you are right with the redundant mca and mce variables. They are not
needed and I will inline the cpu_has() checks into the condition check.
I'll resubmit tomorrow.

-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

Re: coding for optimizations (Re: [PATCH 1/2] i386: mce cleanup part1: functional change)

[Adrian Bunk]

On Tue, Oct 09, 2007 at 08:30:11PM +0200, Joerg Roedel wrote:
>...
> But you are right with the redundant mca and mce variables. They are not
> needed and I will inline the cpu_has() checks into the condition check.
> I'll resubmit tomorrow.

Please don't let Oley bring you away from the right path.

It is your job to write readable C code, and it's the compiler's job to 
transform this C code into efficient machine code.

And non-ancient gcc versions are usually quite good in optimizing code.

And saving code in a variable before using it might even result in the 
same assembler code.

It is personal preference whether you use variables or not in this case 
(both seem to be equally readable) so you can do it any way you like, 
but don't try to guess what gcc might make out of it.

We have problems with the size of the kernel image, but we need to solve 
them with the bigger knobs that have measurable effects, not by wasting 
our time on guessing how gcc might handle a single if.

It's also a quite ill idea to think about whether gcc might produce a 
few bytes more or less code at the if when there's such a long printk() 
in the middle...

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: coding for optimizations (Re: [PATCH 1/2] i386: mce cleanup part1: functional change)

[Oleg Verych]

On Thu, Oct 11, 2007 at 01:14:29AM +0200, Adrian Bunk wrote:
[] 
> It's also a quite ill idea to think about whether gcc might produce a 
> few bytes more or less code at the if when there's such a long printk() 
> in the middle...

printk() problem was discussed with proper banana userspace replacement
proposition by me, so i don't care much.

Though, why MCE can't be enabled/disabled by config option? Native
engineers from proper company should know what processors have this
feature... If kconfig isn't flexible for this, i'm glad to hear
opinions. If it's needed anyway, then sorry.
____

Re: coding for optimizations (Re: [PATCH 1/2] i386: mce cleanup part1: functional change)

[Adrian Bunk]

On Thu, Oct 11, 2007 at 05:26:09PM +0200, Oleg Verych wrote:
> On Thu, Oct 11, 2007 at 01:14:29AM +0200, Adrian Bunk wrote:
> [] 
> > It's also a quite ill idea to think about whether gcc might produce a 
> > few bytes more or less code at the if when there's such a long printk() 
> > in the middle...
> 
> printk() problem was discussed with proper banana userspace replacement
> proposition by me, so i don't care much.
> 
> Though, why MCE can't be enabled/disabled by config option? Native
> engineers from proper company should know what processors have this
> feature... If kconfig isn't flexible for this, i'm glad to hear
> opinions. If it's needed anyway, then sorry.

You should better say sorry for not having checked that it can already 
be disabled with a kconfig option...

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: coding for optimizations (Re: [PATCH 1/2] i386: mce cleanup part1: functional change)

[Oleg Verych]

On Thu, Oct 11, 2007 at 05:21:30PM +0200, Adrian Bunk wrote:
> On Thu, Oct 11, 2007 at 05:26:09PM +0200, Oleg Verych wrote:
> > On Thu, Oct 11, 2007 at 01:14:29AM +0200, Adrian Bunk wrote:
> > [] 
> > > It's also a quite ill idea to think about whether gcc might produce a 
> > > few bytes more or less code at the if when there's such a long printk() 
> > > in the middle...
> > 
> > printk() problem was discussed with proper banana userspace replacement
> > proposition by me, so i don't care much.
> > 
> > Though, why MCE can't be enabled/disabled by config option? Native
> > engineers from proper company should know what processors have this
> > feature... If kconfig isn't flexible for this, i'm glad to hear
> > opinions. If it's needed anyway, then sorry.
> 
> You should better say sorry for not having checked that it can already 
> be disabled with a kconfig option...

Talk is not about wana-build-MCE-in, given to the user's sake. But about
flexible selection of this code, if it is really supported by CPU, when
user have it enabled (see winchip/preventium sub-thread). More work, it
is not such quick patch, certainly, but this is needed for famous i386.

I wonder, why such patch wasn't introduced long time ago. Modern
(cheap) chips have no MCE?
____

[PATCH 2/2] i386: mce cleanup part2: conding style cleanups

[Joerg Roedel]

Remove one indent level

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/i386/kernel/cpu/mcheck/mce.c |   40 ++++++++++++++++++------------------
 1 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/arch/i386/kernel/cpu/mcheck/mce.c b/arch/i386/kernel/cpu/mcheck/mce.c
index bad2c64..fd743c3 100644
--- a/arch/i386/kernel/cpu/mcheck/mce.c
+++ b/arch/i386/kernel/cpu/mcheck/mce.c
@@ -48,26 +48,26 @@ void mcheck_init(struct cpuinfo_x86 *c)
 	}
 
 	switch (c->x86_vendor) {
-		case X86_VENDOR_AMD:
-			amd_mcheck_init(c);
-			break;
-
-		case X86_VENDOR_INTEL:
-			if (c->x86==5)
-				intel_p5_mcheck_init(c);
-			if (c->x86==6)
-				intel_p6_mcheck_init(c);
-			if (c->x86==15)
-				intel_p4_mcheck_init(c);
-			break;
-
-		case X86_VENDOR_CENTAUR:
-			if (c->x86==5)
-				winchip_mcheck_init(c);
-			break;
-
-		default:
-			break;
+	case X86_VENDOR_AMD:
+		amd_mcheck_init(c);
+		break;
+
+	case X86_VENDOR_INTEL:
+		if (c->x86==5)
+			intel_p5_mcheck_init(c);
+		if (c->x86==6)
+			intel_p6_mcheck_init(c);
+		if (c->x86==15)
+			intel_p4_mcheck_init(c);
+		break;
+
+	case X86_VENDOR_CENTAUR:
+		if (c->x86==5)
+			winchip_mcheck_init(c);
+		break;
+
+	default:
+		break;
 	}
 }
 
-- 
1.5.2.5