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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 09 Jul 2010 17:52:14 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Will Deacon <will.deacon@....com>
Cc:	paulus <paulus@...ba.org>,
	stephane eranian <eranian@...glemail.com>,
	Robert Richter <robert.richter@....com>,
	Paul Mundt <lethal@...ux-sh.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Cyrill Gorcunov <gorcunov@...il.com>,
	Lin Ming <ming.m.lin@...el.com>,
	Yanmin <yanmin_zhang@...ux.intel.com>,
	Deng-Cheng Zhu <dengcheng.zhu@...il.com>,
	David Miller <davem@...emloft.net>,
	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH 00/13] perf pmu interface changes -v3

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