perf pmu interface changes -v3
in [Kernel]
|
Prev: Solo6010 staging driver - kernel panic
Next: vmscan: stop meaningless loop iteration when no reclaimable slab
From: Peter Zijlstra on 9 Jul 2010 06:50 On Fri, 2010-07-09 at 10:21 +0200, Peter Zijlstra wrote: Because I'm full of fail today and again had wandering hunks: > Patches also available in git format for easy testing (tip/master + patches): > > git://git.kernel.org/pub/scm/linux/kernel/git/peterz/linux-2.6-perf.git perf-pmu The new HEAD should be 6aa2867187 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Will Deacon on 9 Jul 2010 11:20 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 9 Jul 2010 12:00 On Fri, 2010-07-09 at 16:11 +0100, Will Deacon wrote: > > 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: > 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? Ah yes.. Silly me.. We used to only call ->enable() [ now called ->add() ] inside perf_disable()/perf_enable() regions. But due to patch 9/13 that is no longer the case. I did move it inside several ->enable methods, but not for ARM (and SH) as it wasn't evident they needed it (that'll teach me to assume things). Once we get context per pmu we can move it outside again. So yes, your patch looks good and I added the SH bit as well. I'll push it out to the git tree as and worry about rebasing the whole series once I get back. Thanks! (new HEAD 0a20dff8474e5738f086e9acc38649f56938c0ad) --- arch/arm/kernel/perf_event.c | 3 +++ arch/sh/kernel/perf_event.c | 10 ++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) Index: linux-2.6/arch/arm/kernel/perf_event.c =================================================================== --- linux-2.6.orig/arch/arm/kernel/perf_event.c +++ linux-2.6/arch/arm/kernel/perf_event.c @@ -304,6 +304,8 @@ armpmu_add(struct perf_event *event, int 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 perf_event_update_userpage(event); out: + perf_pmu_enable(event->pmu); return err; } Index: linux-2.6/arch/sh/kernel/perf_event.c =================================================================== --- linux-2.6.orig/arch/sh/kernel/perf_event.c +++ linux-2.6/arch/sh/kernel/perf_event.c @@ -256,11 +256,14 @@ static int sh_pmu_add(struct perf_event struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events); struct hw_perf_event *hwc = &event->hw; int idx = hwc->idx; + int ret = -EAGAIN; + + perf_pmu_disable(event->pmu); if (__test_and_set_bit(idx, cpuc->used_mask)) { idx = find_first_zero_bit(cpuc->used_mask, sh_pmu->num_events); if (idx == sh_pmu->num_events) - return -EAGAIN; + goto ret; __set_bit(idx, cpuc->used_mask); hwc->idx = idx; @@ -273,8 +276,11 @@ static int sh_pmu_add(struct perf_event sh_pmu_start(event, PERF_EF_RELOAD); perf_event_update_userpage(event); + ret = 0; - return 0; +out: + perf_pmu_enable(event->pmu); + return ret; } static void sh_pmu_read(struct perf_event *event) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 9 Jul 2010 12:20 On Fri, 2010-07-09 at 16:11 +0100, Will Deacon wrote: > > The function names aren't especially helpful here either: > armpmu_{start,stop} call armpmu->{enable,disable} and > armpmu_{enable,disable} call armpmu->{start,stop}! Yes, that is rather unfortunate. Maybe we can clean that up once the dust settles a bit. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Matt Fleming on 9 Jul 2010 19:40
On Fri, 09 Jul 2010 17:52:14 +0200, Peter Zijlstra <peterz(a)infradead.org> wrote: > Index: linux-2.6/arch/sh/kernel/perf_event.c > =================================================================== > --- linux-2.6.orig/arch/sh/kernel/perf_event.c > +++ linux-2.6/arch/sh/kernel/perf_event.c > @@ -256,11 +256,14 @@ static int sh_pmu_add(struct perf_event > struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events); > struct hw_perf_event *hwc = &event->hw; > int idx = hwc->idx; > + int ret = -EAGAIN; > + > + perf_pmu_disable(event->pmu); > > if (__test_and_set_bit(idx, cpuc->used_mask)) { > idx = find_first_zero_bit(cpuc->used_mask, sh_pmu->num_events); > if (idx == sh_pmu->num_events) > - return -EAGAIN; > + goto ret; > > __set_bit(idx, cpuc->used_mask); > hwc->idx = idx; > @@ -273,8 +276,11 @@ static int sh_pmu_add(struct perf_event > sh_pmu_start(event, PERF_EF_RELOAD); > > perf_event_update_userpage(event); > + ret = 0; > > - return 0; > +out: > + perf_pmu_enable(event->pmu); > + return ret; > } > > static void sh_pmu_read(struct perf_event *event) This patch results in the following compilation error, CC arch/sh/kernel/perf_event.o cc1: warnings being treated as errors arch/sh/kernel/perf_event.c: In function 'sh_pmu_add': arch/sh/kernel/perf_event.c:281: error: label 'out' defined but not used arch/sh/kernel/perf_event.c:266: error: label 'ret' used but not defined arch/sh/kernel/perf_event.c: In function 'sh_pmu_setup': arch/sh/kernel/perf_event.c:343: error: parameter 'cpuhw' is initialized arch/sh/kernel/perf_event.c:343: error: 'cpu' undeclared (first use in this function) arch/sh/kernel/perf_event.c:343: error: (Each undeclared identifier is reported only once arch/sh/kernel/perf_event.c:343: error: for each function it appears in.) arch/sh/kernel/perf_event.c:343: error: left-hand operand of comma expression has no effect arch/sh/kernel/perf_event.c:345: error: expected declaration specifiers before 'memset' arch/sh/kernel/perf_event.c:346: error: expected declaration specifiers before '}' token arch/sh/kernel/perf_event.c:350: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token arch/sh/kernel/perf_event.c:366: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token arch/sh/kernel/perf_event.c:378: error: old-style parameter declarations in prototyped function definition arch/sh/kernel/perf_event.c:378: error: expected '{' at end of input make[1]: *** [arch/sh/kernel/perf_event.o] Error 1 make: *** [arch/sh/kernel/perf_event.o] Error 2 You can pick up a CodeSourcery SH toolchain from http://www.codesourcery.com/sgpp/lite/superh/portal/release1298 in case you wanted to build any further changes. Does this patch look OK? It fixes the above compilation error for me. diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c index 3bfd70b..bcfb325 100644 --- a/arch/sh/kernel/perf_event.c +++ b/arch/sh/kernel/perf_event.c @@ -263,7 +263,7 @@ static int sh_pmu_add(struct perf_event *event, int flags) if (__test_and_set_bit(idx, cpuc->used_mask)) { idx = find_first_zero_bit(cpuc->used_mask, sh_pmu->num_events); if (idx == sh_pmu->num_events) - goto ret; + goto out; __set_bit(idx, cpuc->used_mask); hwc->idx = idx; @@ -339,7 +339,7 @@ static struct pmu pmu = { }; static void sh_pmu_setup(int cpu) - +{ struct cpu_hw_events *cpuhw = &per_cpu(cpu_hw_events, cpu); memset(cpuhw, 0, sizeof(struct cpu_hw_events)); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |