[PATCH] perf, powerpc: Fix power_pmu_event_init to not use event->ctx

[PATCH] perf, powerpc: Fix power_pmu_event_init to not use event->ctx

[Paul Mackerras]

Commit c3f00c70 ("perf: Separate find_get_context() from event
initialization") changed the generic perf_event code to call
perf_event_alloc, which calls the arch-specific event_init code,
before looking up the context for the new event.  Unfortunately,
power_pmu_event_init uses event->ctx->task to see whether the new
event is a per-task event or a system-wide event, and thus crashes
since event->ctx is NULL at the point where power_pmu_event_init gets
called.

(The reason it needs to know whether it is a per-task event is because
there are some hardware events on Power systems which only count when
the processor is not idle, and there are some fixed-function counters
which count such events.  For example, the "run cycles" event counts
cycles when the processor is not idle.  If the user asks to count
cycles, we can use "run cycles" if this is a per-task event, since the
processor is running when the task is running, by definition.  We
can't use "run cycles" if the user asks for "cycles" on a system-wide
counter.)

Fortunately the information we need is in the event->attach_state
field, so we just use that instead.

Signed-off-by: Paul Mackerras <paulus@samba.org>
Reported-by: Alexey Kardashevskiy <aik@au1.ibm.com>
---
 arch/powerpc/kernel/perf_event.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/perf_event.c b/arch/powerpc/kernel/perf_event.c
index 9cb4924..3129c85 100644
--- a/arch/powerpc/kernel/perf_event.c
+++ b/arch/powerpc/kernel/perf_event.c
@@ -1092,7 +1092,7 @@ static int power_pmu_event_init(struct perf_event *event)
 	 * XXX we should check if the task is an idle task.
 	 */
 	flags = 0;
-	if (event->ctx->task)
+	if (event->attach_state & PERF_ATTACH_TASK)
 		flags |= PPMU_ONLY_COUNT_RUN;
 
 	/*

Re: [PATCH] perf, powerpc: Fix power_pmu_event_init to not use event->ctx

[Peter Zijlstra]

On Tue, 2010-10-19 at 16:55 +1100, Paul Mackerras wrote:
> Commit c3f00c70 ("perf: Separate find_get_context() from event
> initialization") changed the generic perf_event code to call
> perf_event_alloc, which calls the arch-specific event_init code,
> before looking up the context for the new event.  Unfortunately,
> power_pmu_event_init uses event->ctx->task to see whether the new
> event is a per-task event or a system-wide event, and thus crashes
> since event->ctx is NULL at the point where power_pmu_event_init gets
> called.
> 
> (The reason it needs to know whether it is a per-task event is because
> there are some hardware events on Power systems which only count when
> the processor is not idle, and there are some fixed-function counters
> which count such events.  For example, the "run cycles" event counts
> cycles when the processor is not idle.  If the user asks to count
> cycles, we can use "run cycles" if this is a per-task event, since the
> processor is running when the task is running, by definition.  We
> can't use "run cycles" if the user asks for "cycles" on a system-wide
> counter.)
> 
> Fortunately the information we need is in the event->attach_state
> field, so we just use that instead.

Ah, sorry for the breakage.

> Signed-off-by: Paul Mackerras <paulus@samba.org>
> Reported-by: Alexey Kardashevskiy <aik@au1.ibm.com>

Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

> ---
>  arch/powerpc/kernel/perf_event.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/perf_event.c b/arch/powerpc/kernel/perf_event.c
> index 9cb4924..3129c85 100644
> --- a/arch/powerpc/kernel/perf_event.c
> +++ b/arch/powerpc/kernel/perf_event.c
> @@ -1092,7 +1092,7 @@ static int power_pmu_event_init(struct perf_event *event)
>  	 * XXX we should check if the task is an idle task.
>  	 */
>  	flags = 0;
> -	if (event->ctx->task)
> +	if (event->attach_state & PERF_ATTACH_TASK)
>  		flags |= PPMU_ONLY_COUNT_RUN;
>  
>  	/*

Re: [PATCH] perf, powerpc: Fix power_pmu_event_init to not use event->ctx

[Peter Zijlstra]

On Tue, 2010-10-19 at 10:28 +0200, Peter Zijlstra wrote:
 
> > (The reason it needs to know whether it is a per-task event is because
> > there are some hardware events on Power systems which only count when
> > the processor is not idle, and there are some fixed-function counters
> > which count such events.  For example, the "run cycles" event counts
> > cycles when the processor is not idle.  If the user asks to count
> > cycles, we can use "run cycles" if this is a per-task event, since the
> > processor is running when the task is running, by definition.  We
> > can't use "run cycles" if the user asks for "cycles" on a system-wide
> > counter.)

Right, so the problem comes from you using run-cycles as cycles? How
does that interact with things like cpu_relax() will that generate a
difference in run-cycles vs cycles?

Re: [PATCH] perf, powerpc: Fix power_pmu_event_init to not use event->ctx

[Paul Mackerras]

On Tue, Oct 19, 2010 at 01:50:30PM +0200, Peter Zijlstra wrote:

> On Tue, 2010-10-19 at 10:28 +0200, Peter Zijlstra wrote:
>  
> > > (The reason it needs to know whether it is a per-task event is because
> > > there are some hardware events on Power systems which only count when
> > > the processor is not idle, and there are some fixed-function counters
> > > which count such events.  For example, the "run cycles" event counts
> > > cycles when the processor is not idle.  If the user asks to count
> > > cycles, we can use "run cycles" if this is a per-task event, since the
> > > processor is running when the task is running, by definition.  We
> > > can't use "run cycles" if the user asks for "cycles" on a system-wide
> > > counter.)
> 
> Right, so the problem comes from you using run-cycles as cycles? How
> does that interact with things like cpu_relax() will that generate a
> difference in run-cycles vs cycles?

Cycles in cpu_relax() are still counted as run cycles.  Whether cycles
are considered "run" cycles or "idle" cycles is controlled by a bit in
an SPR that we set and clear in the idle loop and in the interrupt
entry code.  Only cycles when we're actually in the idle loop count as
"idle" cycles.

Paul.

[tip:perf/core] perf, powerpc: Fix power_pmu_event_init to not use event->ctx

[tip-bot for Paul Mackerras]

Commit-ID:  57fa7214330be2e292ddb1402834ff0b221ef29a
Gitweb:     http://git.kernel.org/tip/57fa7214330be2e292ddb1402834ff0b221ef29a
Author:     Paul Mackerras <paulus@samba.org>
AuthorDate: Tue, 19 Oct 2010 16:55:35 +1100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 19 Oct 2010 09:18:34 +0200

perf, powerpc: Fix power_pmu_event_init to not use event->ctx

Commit c3f00c70 ("perf: Separate find_get_context() from event
initialization") changed the generic perf_event code to call
perf_event_alloc, which calls the arch-specific event_init code,
before looking up the context for the new event.  Unfortunately,
power_pmu_event_init uses event->ctx->task to see whether the
new event is a per-task event or a system-wide event, and thus
crashes since event->ctx is NULL at the point where
power_pmu_event_init gets called.

(The reason it needs to know whether it is a per-task event is
because there are some hardware events on Power systems which
only count when the processor is not idle, and there are some
fixed-function counters which count such events.  For example,
the "run cycles" event counts cycles when the processor is not
idle.  If the user asks to count cycles, we can use "run cycles"
if this is a per-task event, since the processor is running when
the task is running, by definition.  We can't use "run cycles"
if the user asks for "cycles" on a system-wide counter.)

Fortunately the information we need is in the
event->attach_state field, so we just use that instead.

Signed-off-by: Paul Mackerras <paulus@samba.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <20101019055535.GA10398@drongo>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
Reported-by: Alexey Kardashevskiy <aik@au1.ibm.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/powerpc/kernel/perf_event.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/perf_event.c b/arch/powerpc/kernel/perf_event.c
index 9cb4924..3129c85 100644
--- a/arch/powerpc/kernel/perf_event.c
+++ b/arch/powerpc/kernel/perf_event.c
@@ -1092,7 +1092,7 @@ static int power_pmu_event_init(struct perf_event *event)
 	 * XXX we should check if the task is an idle task.
 	 */
 	flags = 0;
-	if (event->ctx->task)
+	if (event->attach_state & PERF_ATTACH_TASK)
 		flags |= PPMU_ONLY_COUNT_RUN;
 
 	/*