Re: [RFC][PATCH 00/13] perf pmu interface changes -v3

[Will Deacon <will.deacon@arm.com>]

Hi Peter,

On Fri, 2010-07-09 at 09:21 +0100, Peter Zijlstra wrote:
> The patches are boot tested on x86_64 and compile tested on: powerpc,
> ppc-fsl,
> sparc and arm and SH (by courtesy of Matt Fleming).
> 
> X86 -- appears to be fully functional.
> FSL -- should, after the initial few fixes, at least compile again.
> ARM -- is known to have grown some issues, Will was looking into that.

I've been looking at this and after fixing atomic64 operations and a
sign-extension bug, I'm getting closer to finding the *real* cause of
the problem! It appears that the PMU isn't being enabled for pinned
events. The following patch fixes that, but I'm not sure that it's the
correct thing to do:

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index c440ae1..a946a77 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -304,6 +304,8 @@ armpmu_add(struct perf_event *event, int flags)
        int idx;
        int err = 0;
 
+       perf_pmu_disable(event->pmu);
+
        /* If we don't have a space for the counter then finish early.
*/
        idx = armpmu->get_event_idx(cpuc, hwc);
        if (idx < 0) {
@@ -328,6 +330,7 @@ armpmu_add(struct perf_event *event, int flags)
        perf_event_update_userpage(event);
 
 out:
+       perf_pmu_enable(event->pmu);
        return err;
 }

Because only pinned events seem to be broken, I'm worried that this is
just hiding the problem... or is pmu->add(...) supposed to enable the
PMU as well as installing the event?

The function names aren't especially helpful here either:
armpmu_{start,stop} call armpmu->{enable,disable} and
armpmu_{enable,disable} call armpmu->{start,stop}!

Any thoughts?

Will