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

From: Vince Weaver
Date: Wed Oct 17 2012 - 11:34:12 EST


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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/