Re: perf: p6 PMU working by accident, should we fix it and KNC?

2012-10-19 Thread Peter Zijlstra 
On Wed, 2012-10-17 at 11:35 -0400, Vince Weaver wrote:
> 
> This is by accident; it looks like the code does 
>val |= ARCH_PERFMON_EVENTSEL_ENABLE;
> in p6_pmu_disable_event() so that events are never truly disabled
> (is this a bug?  should it be &=~ instead?).  

I think that's on purpose.. from what I can remember p6 only has a
single EN bit (on PMC0) that acts for both counters. So what I did was
treat that as a global enable/disable (which it is) and did the local
enable/disable by using the NOP events.

There really might be bugs in there, its not like I use this class of
hardware very frequently (nor do anybody much it seems).


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

perf: p6 PMU working by accident, should we fix it and KNC?

2012-10-17 Thread Vince Weaver 
Hello

quick summary: the p6 code looks to be buggy and is only currently
   working by luck.  I'm trying to work out how to best fix the KNC
   code which is based on the p6 PMU driver.

While working on the KNC PMU we ran into the following problem.

Between 2.6.33 and 2.6.34 the kernel was changed to have a much
more elaborate PMU initialization routine.

The new x86_pmu_enable() routine starts with cpuc->enabled set to 0.
  It then calls x86_pmu_start(event)
  Which then calls x86_pmu.enable(event)

On both p6 and KNC, x86_pmu.enable(event) is conditioned not to do
anything unless cpuc->enabled is set to 1 (the generic x86 enable does not 
have this limitation).

Only at the end of x86_pmu_enable() is cpuc->enabled set to 1.

On KNC this means that the counter setup does not happen properly.

This should also be the case on P6, but mysteriously things *do* work, as 
I just verified by brining my poor PII machine out of retirement.

This is by accident; it looks like the code does 
   val |= ARCH_PERFMON_EVENTSEL_ENABLE;
in p6_pmu_disable_event() so that events are never truly disabled
(is this a bug?  should it be &=~ instead?).  
This doesn't cause problems because P6_NOP_EVENT
is used in the disabled case so no spurious counts are generated.

So I have two questions:
  1.  Is it worth fixing the p6 code?
  2.  Is it OK to drop the cpuc->enabled test in KNC for event 
  enable/disable?  The driver works fine with that dropped, and
  the regular intel x86 driver doesn't have those tests.

Thanks,

Vince
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/