[PATCH -tip] perf_counter/x86: Fix incorrect default branch

[PATCH -tip] perf_counter/x86: Fix incorrect default branch

[Yong Wang]

The event selector and UMASK values of Nehalem do not apply to all Intel processors.

Signed-off-by: Yong Wang <yong.y.wang@intel.com>

---
 arch/x86/kernel/cpu/perf_counter.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/perf_counter.c b/arch/x86/kernel/cpu/perf_counter.c
index 56001fe..c74b6d1 100644
--- a/arch/x86/kernel/cpu/perf_counter.c
+++ b/arch/x86/kernel/cpu/perf_counter.c
@@ -1413,7 +1413,6 @@ static int intel_pmu_init(void)
 
 		pr_cont("Core2 events, ");
 		break;
-	default:
 	case 26:
 		memcpy(hw_cache_event_ids, nehalem_hw_cache_event_ids,
 		       sizeof(hw_cache_event_ids));
@@ -1426,6 +1425,8 @@ static int intel_pmu_init(void)
 
 		pr_cont("Atom events, ");
 		break;
+	default:
+		break;
 	}
 	return 0;
 }

Re: [PATCH -tip] perf_counter/x86: Fix incorrect default branch

[Ingo Molnar]

* Yong Wang <yong.y.wang@linux.intel.com> wrote:

> The event selector and UMASK values of Nehalem do not apply to all 
> Intel processors.

The idea was to offer _some_ sort of table on new CPUs, instead of 
nothing...

Eventually Intel stops changing those model specific index values in 
future CPUs and puts them into architectural perfmon, for 
fundamental stats like L1/LLC cache statistics.

In that case defaulting to the latest (known) enumeration might work 
out to be just the thing used by all future CPUs.

So this is a subtle hint ;-)

	Ingo

Re: [PATCH -tip] perf_counter/x86: Fix incorrect default branch

[Yong Wang]

On Tue, Jun 09, 2009 at 03:23:13PM +0200, Ingo Molnar wrote:
> 
> * Yong Wang <yong.y.wang@linux.intel.com> wrote:
> 
> > The event selector and UMASK values of Nehalem do not apply to all 
> > Intel processors.
> 
> The idea was to offer _some_ sort of table on new CPUs, instead of 
> nothing...
> 
> Eventually Intel stops changing those model specific index values in 
> future CPUs and puts them into architectural perfmon, for 
> fundamental stats like L1/LLC cache statistics.
> 
> In that case defaulting to the latest (known) enumeration might work 
> out to be just the thing used by all future CPUs.
> 
> So this is a subtle hint ;-)
> 

I see what you mean. You talked about future cpus. But what about those
old ones that are not atom, core2 or nehalem? Forget about them?

Thanks
-Yong

Re: [PATCH -tip] perf_counter/x86: Fix incorrect default branch

[Ingo Molnar]

* Yong Wang <yong.y.wang@linux.intel.com> wrote:

> On Tue, Jun 09, 2009 at 03:23:13PM +0200, Ingo Molnar wrote:
> > 
> > * Yong Wang <yong.y.wang@linux.intel.com> wrote:
> > 
> > > The event selector and UMASK values of Nehalem do not apply to all 
> > > Intel processors.
> > 
> > The idea was to offer _some_ sort of table on new CPUs, instead of 
> > nothing...
> > 
> > Eventually Intel stops changing those model specific index values in 
> > future CPUs and puts them into architectural perfmon, for 
> > fundamental stats like L1/LLC cache statistics.
> > 
> > In that case defaulting to the latest (known) enumeration might work 
> > out to be just the thing used by all future CPUs.
> > 
> > So this is a subtle hint ;-)
> > 
> 
> I see what you mean. You talked about future cpus. But what about 
> those old ones that are not atom, core2 or nehalem? Forget about 
> them?

They dont have X86_FEATURE_ARCH_PERFMON set, right?

I made the switch statement under the assumption that it covers all 
existing arch-perfmon CPU models. If not, the 'default:' placement 
would indeed be buggy.

	Ingo

Re: [PATCH -tip] perf_counter/x86: Fix incorrect default branch

[Yong Wang]

On Tue, Jun 09, 2009 at 04:25:06PM +0200, Ingo Molnar wrote:
> 
> They dont have X86_FEATURE_ARCH_PERFMON set, right?
> 
> I made the switch statement under the assumption that it covers all 
> existing arch-perfmon CPU models. If not, the 'default:' placement 
> would indeed be buggy.
> 

OK, you are right. X86_FEATURE_ARCH_PERFMON eliminates all the old CPUs.
However, the logic still seems not correct. Both the Core2 CPUs I'm
using are recognized as Nehalem/Corei7. /proc/cpuinfo and cpuid outputs
are given below.

The first one:
vendor_id       : GenuineIntel
cpu family      : 6
model           : 23
model name      : Intel(R) Core(TM)2 Duo CPU     P8400  @ 2.26GHz
stepping        : 6

CPUID.0x00000001: eax=0x00010676

The second one:
vendor_id       : GenuineIntel
cpu family      : 6
model           : 15
model name      : Intel(R) Core(TM)2 CPU          6600  @ 2.40GHz
stepping        : 6

CPUID.0x00000001: eax=0x000006f6

Looks like it's not enough to just look at boot_cpu_data.x86_model.

-Yong

Re: [PATCH -tip] perf_counter/x86: Fix incorrect default branch

[Ingo Molnar]

* Yong Wang <yong.y.wang@linux.intel.com> wrote:

> On Tue, Jun 09, 2009 at 04:25:06PM +0200, Ingo Molnar wrote:
> > 
> > They dont have X86_FEATURE_ARCH_PERFMON set, right?
> > 
> > I made the switch statement under the assumption that it covers all 
> > existing arch-perfmon CPU models. If not, the 'default:' placement 
> > would indeed be buggy.
> > 
> 
> OK, you are right. X86_FEATURE_ARCH_PERFMON eliminates all the old CPUs.
> However, the logic still seems not correct. Both the Core2 CPUs I'm
> using are recognized as Nehalem/Corei7. /proc/cpuinfo and cpuid outputs
> are given below.
> 
> The first one:
> vendor_id       : GenuineIntel
> cpu family      : 6
> model           : 23
> model name      : Intel(R) Core(TM)2 Duo CPU     P8400  @ 2.26GHz
> stepping        : 6
> 
> CPUID.0x00000001: eax=0x00010676
> 
> The second one:
> vendor_id       : GenuineIntel
> cpu family      : 6
> model           : 15
> model name      : Intel(R) Core(TM)2 CPU          6600  @ 2.40GHz
> stepping        : 6
> 
> CPUID.0x00000001: eax=0x000006f6
> 
> Looks like it's not enough to just look at boot_cpu_data.x86_model.

Yeah - i've applied the patch you sent, thanks. I'm mostly working 
on NHM so i didnt notice.

	Ingo