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

[Peter Zijlstra]

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