Re: [PATCH 1/5] perf, x86: Don't assume the alternative cycles encoding is architectural

[Andi Kleen <ak@linux.intel.com>]

On Wed, Jun 06, 2012 at 04:14:21PM +0200, Peter Zijlstra wrote:
> On Wed, 2012-06-06 at 07:12 -0700, Andi Kleen wrote:
> > On Wed, Jun 06, 2012 at 12:39:48PM +0200, Peter Zijlstra wrote:
> > > On Tue, 2012-06-05 at 17:56 -0700, Andi Kleen wrote:
> > > > From: Andi Kleen <ak@linux.intel.com>
> > > > 
> > > > cycles:p uses an special cycles encoding by default. However that is not
> > > > architectural, so it can only be used when the CPU is known
> > > > (it already caused problems on Sandy Bridge). It may or may not work
> > > > on future CPUs.
> > > > 
> > > > So make it opt-in only. Right now I enabled it on Core2, Nehalem, Westmere
> > > > and not on Sandy-Bridge or Atom.
> > > 
> > > No. 
> > 
> > What do you mean? Are you claiming it's architectural?
> 
> I mean this patch is shite.
> 
> You don't disable it because it doesn't work some place, you fix it.

I disable it because it's non architectural, like the description
says.

This code is not behind model numbers. You cannot do random model specific 
hacks like this without a model number check. And you already got burned
for it. It may well break again in the future because there's no
guarantee for these things working.

Arch perfmon just means you can use the parts guaranteed in
arch perfmon and nothing more.

Now on what model numbers exactly it should be on can be debated.
I was very conservative, but the set could likely be larger. 

But it's very clear this cannot be done without a model check.
I don't understand how you can even argue against that.

-Andi

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