Re: [tip:perf/core] perf: Rework the PMU methods

Re: [tip:perf/core] perf: Rework the PMU methods

[Michael Cree]

On 10/09/10 07:50, tip-bot for Peter Zijlstra wrote:
> Commit-ID:  a4eaf7f14675cb512d69f0c928055e73d0c6d252
> Gitweb:     http://git.kernel.org/tip/a4eaf7f14675cb512d69f0c928055e73d0c6d252
> Author:     Peter Zijlstra<a.p.zijlstra@chello.nl>
> AuthorDate: Wed, 16 Jun 2010 14:37:10 +0200
> Committer:  Ingo Molnar<mingo@elte.hu>
> CommitDate: Thu, 9 Sep 2010 20:46:30 +0200
>
> perf: Rework the PMU methods
>
> Replace pmu::{enable,disable,start,stop,unthrottle} with
> pmu::{add,del,start,stop}, all of which take a flags argument.

Regarding the new function alpha_pmu_stop() in 
arch/alpha/kernel/perf_event.c:

> -static void alpha_pmu_unthrottle(struct perf_event *event)
> +static void alpha_pmu_stop(struct perf_event *event, int flags)
>   {
>   	struct hw_perf_event *hwc =&event->hw;
>   	struct cpu_hw_events *cpuc =&__get_cpu_var(cpu_hw_events);
>
> +	if (!(hwc->state&  PERF_HES_STOPPED)) {
> +		cpuc->idx_mask&= !(1UL<<hwc->idx);
                                  ^
Presumably ones complement (rather than logical not) is meant.

> +		hwc->state |= PERF_HES_STOPPED;
> +	}
> +
> +	if ((flags&  PERF_EF_UPDATE)&&  !(hwc->state&  PERF_HES_UPTODATE)) {
> +		alpha_perf_event_update(event, hwc, hwc->idx, 0);
> +		hwc->state |= PERF_HES_UPTODATE;
> +	}
> +
> +	if (cpuc->enabled)
> +		wrperfmon(PERFMON_CMD_ENABLE, (1UL<<hwc->idx));

By the name of the function (alpha_pmu_stop) I assume that the intent is 
to stop the specific PMC here.  The above fails to do that.  When 
wrperfmon() is used with PERFMON_CMD_ENABLE it enables the PMCs with set 
bits in the second argument.  It does not stop the others.  To do that 
wrperfmon() must be called with PERFMON_CMD_DISABLE and the 
corresponding PMC bits set to disable the PMC.

Cheers
Michael.

Re: [tip:perf/core] perf: Rework the PMU methods

[Peter Zijlstra]

On Sat, 2010-09-11 at 20:16 +1200, Michael Cree wrote:
> On 10/09/10 07:50, tip-bot for Peter Zijlstra wrote:
> > Commit-ID:  a4eaf7f14675cb512d69f0c928055e73d0c6d252
> > Gitweb:     http://git.kernel.org/tip/a4eaf7f14675cb512d69f0c928055e73d0c6d252
> > Author:     Peter Zijlstra<a.p.zijlstra@chello.nl>
> > AuthorDate: Wed, 16 Jun 2010 14:37:10 +0200
> > Committer:  Ingo Molnar<mingo@elte.hu>
> > CommitDate: Thu, 9 Sep 2010 20:46:30 +0200
> >
> > perf: Rework the PMU methods
> >
> > Replace pmu::{enable,disable,start,stop,unthrottle} with
> > pmu::{add,del,start,stop}, all of which take a flags argument.
> 
> Regarding the new function alpha_pmu_stop() in 
> arch/alpha/kernel/perf_event.c:
> 
> > -static void alpha_pmu_unthrottle(struct perf_event *event)
> > +static void alpha_pmu_stop(struct perf_event *event, int flags)
> >   {
> >   	struct hw_perf_event *hwc =&event->hw;
> >   	struct cpu_hw_events *cpuc =&__get_cpu_var(cpu_hw_events);
> >
> > +	if (!(hwc->state&  PERF_HES_STOPPED)) {
> > +		cpuc->idx_mask&= !(1UL<<hwc->idx);
>                                   ^
> Presumably ones complement (rather than logical not) is meant.

Yes, typo that, sorry.

> 
> > +		hwc->state |= PERF_HES_STOPPED;
> > +	}
> > +
> > +	if ((flags&  PERF_EF_UPDATE)&&  !(hwc->state&  PERF_HES_UPTODATE)) {
> > +		alpha_perf_event_update(event, hwc, hwc->idx, 0);
> > +		hwc->state |= PERF_HES_UPTODATE;
> > +	}
> > +
> > +	if (cpuc->enabled)
> > +		wrperfmon(PERFMON_CMD_ENABLE, (1UL<<hwc->idx));
> 
> By the name of the function (alpha_pmu_stop) I assume that the intent is 
> to stop the specific PMC here.  The above fails to do that.  When 
> wrperfmon() is used with PERFMON_CMD_ENABLE it enables the PMCs with set 
> bits in the second argument.  It does not stop the others.  To do that 
> wrperfmon() must be called with PERFMON_CMD_DISABLE and the 
> corresponding PMC bits set to disable the PMC.

Right, so ->add()/->del() schedule the event onto the pmu and deal with
any resource issues where needed. ->stop()/->start() simply leave the
event on the pmu with all resources in tact, but ensure it doesn't
actually count.

Depending on the PMU there's various ways of achieving that, some PMUs
can't disable counter, for those we simply take a counter reading and
disable the interrupt, and reset the counter to the previous value on
->start again and enable the interrupt.

Or when we can't even disable the interrupt, we program it to the
longest possible period, etc..

Apparently ALPHA can nicely disable things, except I seem to have
misunderstood the way how, I assumed it had an enable register, and
writing a 0 to the idx position would stop it.

Could you provide a patch that makes ALPHA work again, or would you like
me to take another stab at it?

Re: [tip:perf/core] perf: Rework the PMU methods

[Michael Cree]

On 11/09/10 21:40, Peter Zijlstra wrote:
> On Sat, 2010-09-11 at 20:16 +1200, Michael Cree wrote:
>> On 10/09/10 07:50, tip-bot for Peter Zijlstra wrote:
>>> Commit-ID:  a4eaf7f14675cb512d69f0c928055e73d0c6d252
>>> Gitweb:     http://git.kernel.org/tip/a4eaf7f14675cb512d69f0c928055e73d0c6d252
>>> Author:     Peter Zijlstra<a.p.zijlstra@chello.nl>
>>> AuthorDate: Wed, 16 Jun 2010 14:37:10 +0200
>>> Committer:  Ingo Molnar<mingo@elte.hu>
>>> CommitDate: Thu, 9 Sep 2010 20:46:30 +0200
>>>
>>> perf: Rework the PMU methods
>>>
>>> Replace pmu::{enable,disable,start,stop,unthrottle} with
>>> pmu::{add,del,start,stop}, all of which take a flags argument.
>>
>> Regarding the new function alpha_pmu_stop() in
>> arch/alpha/kernel/perf_event.c:
>>

> Could you provide a patch that makes ALPHA work again, or would you like
> me to take another stab at it?

Yes, done.  I also took the liberty to fix an undefined variable and 
multiple defined variable errors that were exposed by compilation.  Will 
reply to this with the patch.

I've also tested it on a UP alpha.  It worked well for a little while 
but after running 'perf top' for a number of seconds I got the following 
warning:

[  287.952977] WARNING: at arch/alpha/kernel/perf_event.c:546 
alpha_pmu_start+0xd0/0x120()
[  287.952977] Modules linked in: radeon ttm drm_kms_helper hwmon 
cfbcopyarea cfbimgblt cfbfillrect parport_pc parport nfsd exportfs nfs 
lockd sunrpc ipv6 loop snd_hda_codec_atihdmi snd_hda_intel snd_hda_codec 
snd_pcm snd_seq snd_timer snd_seq_device ftdi_sio sg usbserial sr_mod 
snd cdrom r8169 ohci1394 soundcore ohci_hcd ehci_hcd ieee1394 mii 
snd_page_alloc ata_generic pcspkr tulip evdev usbcore pata_cypress
[  287.952977] fffffc000078fb80 fffffc00007961c8 fffffc000031dbb0 
fffffc002780f400
[  287.952977]        0000000000000000 0000000000000400 00000000000ee6b3 
0000000000000000
[  287.952977]        fffffc000036cf28 fffffc002780f400 fffffc003f006240 
fffffc003f343538
[  287.952977]        fffffc000036d124 0000000000000001 fffffc003f006200 
0000000000000000
[  287.952977]        fffffc003f0062c8 0000000000000000 fffffc002780f410 
fffffc003f0062c8
[  287.952977]        fffffc000034d118 fffffc003f0062c8 fffffc0000795b28 
fffffc0000795b40
[  287.952977] Trace:
[  287.952977] [<fffffc000031dbb0>] alpha_pmu_start+0xd0/0x120
[  287.952977] [<fffffc000036cf28>] perf_ctx_adjust_freq+0x138/0x150
[  287.952977] [<fffffc000036d124>] perf_event_context_tick+0x1e4/0x210
[  287.952977] [<fffffc000034d118>] hrtimer_run_queues+0x1b8/0x2f0
[  287.952977] [<fffffc0000338a48>] run_local_timers+0x18/0x40
[  287.952977] [<fffffc0000338aac>] update_process_times+0x3c/0xb0
[  287.952977] [<fffffc000031895c>] timer_interrupt+0x9c/0x100
[  287.952977] [<fffffc0000361450>] handle_IRQ_event+0x70/0x1a0
[  287.952977] [<fffffc0000361648>] __do_IRQ+0xc8/0x160
[  287.952977] [<fffffc0000315374>] handle_irq+0x54/0xb0
[  287.952977] [<fffffc0000315b04>] do_entInt+0xc4/0x1e0
[  287.952977] [<fffffc0000310c40>] ret_from_sys_call+0x0/0x10
[  287.952977] [<fffffc00003ab414>] file_free_rcu+0x54/0x70
[  287.952977] [<fffffc00003bd0ec>] estimate_accuracy+0xdc/0x120
[  287.952977] [<fffffc0000313108>] cpu_idle+0x48/0x60
[  287.952977] [<fffffc0000367e90>] perf_event_task_sched_in+0x0/0x60
[  287.952977] [<fffffc00003130f0>] cpu_idle+0x30/0x60
[  287.952977] [<fffffc00006338d0>] rest_init+0xb0/0xd0
[  287.952977] [<fffffc000031001c>] _stext+0x1c/0x20
[  287.952977]
[  287.952977] ---[ end trace 6f10adce9e4129fa ]---

which is from the line in alpha_pmu_start() that checks that 
PERF_HES_STOPPED is set.

I see that the backtrace is from the Alpha timer_interrupt() code which 
goes something like this:

[do some stuff updating timer deltas then...]

#ifndef CONFIG_SMP
         while (nticks--)
                 update_process_times(user_mode(get_irq_regs()));
#endif

         if (test_perf_event_pending()) {
                 clear_perf_event_pending();
                 perf_event_do_pending();
         }

         return IRQ_HANDLED;
}


When I added the code for handle pending events to the timer interrupt I 
hadn't realised that update_process_times() could call back into the 
perf code.  I'm speculating here, but could it be that there is pending 
work to stop the HW counter, but the call to re-start it is beating the 
call to stop it?

Cheers
Michael.

[PATCH] alpha: Fix HW performance counters to be stopped properly.

[Michael Cree]

Also fix a few compile errors due to undefined and duplicated
variables.

Signed-off-by: Michael Cree <mcree@orcon.net.nz>
---
 arch/alpha/kernel/perf_event.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/alpha/kernel/perf_event.c b/arch/alpha/kernel/perf_event.c
index a25fe9e..41f204f 100644
--- a/arch/alpha/kernel/perf_event.c
+++ b/arch/alpha/kernel/perf_event.c
@@ -422,9 +422,10 @@ static void maybe_change_configuration(struct cpu_hw_events *cpuc)
 static int alpha_pmu_add(struct perf_event *event, int flags)
 {
 	struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
+	struct hw_perf_event *hwc = &event->hw;
 	int n0;
 	int ret;
-	unsigned long flags;
+	unsigned long irq_flags;
 
 	/*
 	 * The Sparc code has the IRQ disable first followed by the perf
@@ -435,7 +436,7 @@ static int alpha_pmu_add(struct perf_event *event, int flags)
 	 * final PMI to occur before we disable interrupts.
 	 */
 	perf_pmu_disable(event->pmu);
-	local_irq_save(flags);
+	local_irq_save(irq_flags);
 
 	/* Default to error to be returned */
 	ret = -EAGAIN;
@@ -458,7 +459,7 @@ static int alpha_pmu_add(struct perf_event *event, int flags)
 	if (!(flags & PERF_EF_START))
 		hwc->state |= PERF_HES_STOPPED;
 
-	local_irq_restore(flags);
+	local_irq_restore(irq_flags);
 	perf_pmu_enable(event->pmu);
 
 	return ret;
@@ -474,11 +475,11 @@ static void alpha_pmu_del(struct perf_event *event, int flags)
 {
 	struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
 	struct hw_perf_event *hwc = &event->hw;
-	unsigned long flags;
+	unsigned long irq_flags;
 	int j;
 
 	perf_pmu_disable(event->pmu);
-	local_irq_save(flags);
+	local_irq_save(irq_flags);
 
 	for (j = 0; j < cpuc->n_events; j++) {
 		if (event == cpuc->event[j]) {
@@ -504,7 +505,7 @@ static void alpha_pmu_del(struct perf_event *event, int flags)
 		}
 	}
 
-	local_irq_restore(flags);
+	local_irq_restore(irq_flags);
 	perf_pmu_enable(event->pmu);
 }
 
@@ -523,7 +524,7 @@ static void alpha_pmu_stop(struct perf_event *event, int flags)
 	struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
 
 	if (!(hwc->state & PERF_HES_STOPPED)) {
-		cpuc->idx_mask &= !(1UL<<hwc->idx);
+		cpuc->idx_mask &= ~(1UL<<hwc->idx);
 		hwc->state |= PERF_HES_STOPPED;
 	}
 
@@ -533,7 +534,7 @@ static void alpha_pmu_stop(struct perf_event *event, int flags)
 	}
 
 	if (cpuc->enabled)
-		wrperfmon(PERFMON_CMD_ENABLE, (1UL<<hwc->idx));
+		wrperfmon(PERFMON_CMD_DISABLE, (1UL<<hwc->idx));
 }
 
 
-- 
1.7.1

Re: [tip:perf/core] perf: Rework the PMU methods

[Peter Zijlstra]

On Sun, 2010-09-12 at 17:33 +1200, Michael Cree wrote:

> Yes, done.  I also took the liberty to fix an undefined variable and 
> multiple defined variable errors that were exposed by compilation.  Will 
> reply to this with the patch.

Thanks, and sorry for messing up Alpha that bad.. I have an alpha
compiler and I really through I compile tested it :/

> I've also tested it on a UP alpha.  It worked well for a little while 
> but after running 'perf top' for a number of seconds I got the following 
> warning:

<snip warn>

> which is from the line in alpha_pmu_start() that checks that 
> PERF_HES_STOPPED is set.
> 
> I see that the backtrace is from the Alpha timer_interrupt() code which 
> goes something like this:
> 
> [do some stuff updating timer deltas then...]
> 
> #ifndef CONFIG_SMP
>          while (nticks--)
>                  update_process_times(user_mode(get_irq_regs()));
> #endif
> 
>          if (test_perf_event_pending()) {
>                  clear_perf_event_pending();
>                  perf_event_do_pending();
>          }
> 
>          return IRQ_HANDLED;
> }
> 
> 
> When I added the code for handle pending events to the timer interrupt I 
> hadn't realised that update_process_times() could call back into the 
> perf code.  I'm speculating here, but could it be that there is pending 
> work to stop the HW counter, but the call to re-start it is beating the 
> call to stop it?

Right, so the ->start() call came from perf_ctx_adjust_freq(), which
depending on whether perf_adjust_period() gets inlined, can have two
such calls.

Assuming it didn't inline (there's two callsites, which should defeat
the inline static functions with a single callsite heuristic), you hit
the unthrottle() call.

Ahh, the alpha throttle call should be using the fancy new stop function
too (will fold into your earlier patch if it indeed works):

As to the point you raised above, yes, I think it would be prudent to
call perf_event_do_pending() before update_process_times().


Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
Index: linux-2.6/arch/alpha/kernel/perf_event.c
===================================================================
--- linux-2.6.orig/arch/alpha/kernel/perf_event.c
+++ linux-2.6/arch/alpha/kernel/perf_event.c
@@ -825,14 +825,14 @@ static void alpha_perf_event_irq_handler
 			break;
 	}
 
+	event = cpuc->event[j];
+
 	if (unlikely(j == cpuc->n_events)) {
 		/* This can occur if the event is disabled right on a PMC overflow. */
-		wrperfmon(PERFMON_CMD_ENABLE, cpuc->idx_mask);
+		alpha_pmu_stop(event, 0);
 		return;
 	}
 
-	event = cpuc->event[j];
-
 	if (unlikely(!event)) {
 		/* This should never occur! */
 		irq_err_count++;

Re: [tip:perf/core] perf: Rework the PMU methods

[Peter Zijlstra]

On Mon, 2010-09-13 at 14:15 +0200, Peter Zijlstra wrote:
> On Sun, 2010-09-12 at 17:33 +1200, Michael Cree wrote:
> 
> > Yes, done.  I also took the liberty to fix an undefined variable and 
> > multiple defined variable errors that were exposed by compilation.  Will 
> > reply to this with the patch.
> 
> Thanks, and sorry for messing up Alpha that bad.. I have an alpha
> compiler and I really through I compile tested it :/
> 
> > I've also tested it on a UP alpha.  It worked well for a little while 
> > but after running 'perf top' for a number of seconds I got the following 
> > warning:
> 
> <snip warn>
> 
> > which is from the line in alpha_pmu_start() that checks that 
> > PERF_HES_STOPPED is set.
> > 
> > I see that the backtrace is from the Alpha timer_interrupt() code which 
> > goes something like this:
> > 
> > [do some stuff updating timer deltas then...]
> > 
> > #ifndef CONFIG_SMP
> >          while (nticks--)
> >                  update_process_times(user_mode(get_irq_regs()));
> > #endif
> > 
> >          if (test_perf_event_pending()) {
> >                  clear_perf_event_pending();
> >                  perf_event_do_pending();
> >          }
> > 
> >          return IRQ_HANDLED;
> > }
> > 
> > 
> > When I added the code for handle pending events to the timer interrupt I 
> > hadn't realised that update_process_times() could call back into the 
> > perf code.  I'm speculating here, but could it be that there is pending 
> > work to stop the HW counter, but the call to re-start it is beating the 
> > call to stop it?
> 
> Right, so the ->start() call came from perf_ctx_adjust_freq(), which
> depending on whether perf_adjust_period() gets inlined, can have two
> such calls.
> 
> Assuming it didn't inline (there's two callsites, which should defeat
> the inline static functions with a single callsite heuristic), you hit
> the unthrottle() call.
> 
> Ahh, the alpha throttle call should be using the fancy new stop function
> too (will fold into your earlier patch if it indeed works):
> 
> As to the point you raised above, yes, I think it would be prudent to
> call perf_event_do_pending() before update_process_times().
> 
> 
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

Damn I suck.. Please try this one.

---
Index: linux-2.6/arch/alpha/kernel/perf_event.c
===================================================================
--- linux-2.6.orig/arch/alpha/kernel/perf_event.c
+++ linux-2.6/arch/alpha/kernel/perf_event.c
@@ -850,7 +850,7 @@ static void alpha_perf_event_irq_handler
 			/* Interrupts coming too quickly; "throttle" the
 			 * counter, i.e., disable it for a little while.
 			 */
-			cpuc->idx_mask &= ~(1UL<<idx);
+			alpha_pmu_stop(event, 0);
 		}
 	}
 	wrperfmon(PERFMON_CMD_ENABLE, cpuc->idx_mask);

Re: [tip:perf/core] perf: Rework the PMU methods

[Michael Cree]

On 14/09/10 01:18, Peter Zijlstra wrote:
> On Mon, 2010-09-13 at 14:15 +0200, Peter Zijlstra wrote:
>> On Sun, 2010-09-12 at 17:33 +1200, Michael Cree wrote:
>>
>>> I've also tested it on a UP alpha.  It worked well for a little while
>>> but after running 'perf top' for a number of seconds I got the following
>>> warning:

>> Ahh, the alpha throttle call should be using the fancy new stop function
>> too (will fold into your earlier patch if it indeed works):
>>
>> As to the point you raised above, yes, I think it would be prudent to
>> call perf_event_do_pending() before update_process_times().
>>
>>
>> Signed-off-by: Peter Zijlstra<a.p.zijlstra@chello.nl>
>
> Damn I suck.. Please try this one.
>
> ---
> Index: linux-2.6/arch/alpha/kernel/perf_event.c
> ===================================================================
> --- linux-2.6.orig/arch/alpha/kernel/perf_event.c
> +++ linux-2.6/arch/alpha/kernel/perf_event.c
> @@ -850,7 +850,7 @@ static void alpha_perf_event_irq_handler
>   			/* Interrupts coming too quickly; "throttle" the
>   			 * counter, i.e., disable it for a little while.
>   			 */
> -			cpuc->idx_mask&= ~(1UL<<idx);
> +			alpha_pmu_stop(event, 0);
>   		}
>   	}
>   	wrperfmon(PERFMON_CMD_ENABLE, cpuc->idx_mask);

Thanks, that does the trick.  I have had it running for a while without 
any problems... Well, except for a new problem which I describe below. 
Anyway the above patch has fixed the original problem so can be rolled 
into my earlier one as you suggest.

I haven't shifted perf_event_do_pending() before update_process_times() 
in the timer interrupt yet.  When I get around to it I'll send a patch 
through the Alpha maintainer as this is independent of the work on the 
core perf event code.

Now to the new problem I saw:  I accidently reran 'perf top' while it 
was already running, and the second instance of 'perf top' seg-faulted 
and the kernel OOPSed with the following:

[  818.575752] Unable to handle kernel paging request at virtual address 
0000000000000060
[  818.575752] perf(4935): Oops 0
[  818.575752] pc = [<fffffc000036b8b8>]  ra = [<fffffc000036ba3c>]  ps 
= 0000    Not tainted
[  818.575752] pc is at put_ctx+0x18/0xb0
[  818.575752] ra is at free_event+0xec/0x220
[  818.575752] v0 = 0000000000000000  t0 = 0000000000000000  t1 = 
0000000000000002
[  818.575752] t2 = 0000000000000000  t3 = 0000000000000000  t4 = 
fffffc000063e4b0
[  818.575752] t5 = fffffc0034837000  t6 = fffffc0034837000  t7 = 
fffffc003abe0000
[  818.575752] s0 = 0000000000000000  s1 = 0000000000000000  s2 = 
0000000000000000
[  818.575752] s3 = ffffffffffffffff  s4 = 0000000000000000  s5 = 
0000000000000053
[  818.575752] s6 = fffffc0034836c00
[  818.575752] a0 = 0000000000000000  a1 = fffffc00311a6980  a2 = 
0000000000000000
[  818.575752] a3 = 0000000000000001  a4 = fffffc0000797110  a5 = 
00000001200055f8
[  818.575752] t8 = 0000000000000001  t9 = 00000001200396f4  t10= 
0000000000000000
[  818.575752] t11= 000000000000000a  pv = fffffc000031d650  at = 
fffffc000036f8bc
[  818.575752] gp = fffffc00007d6e40  sp = fffffc003abe3e18
[  818.575752] Disabling lock debugging due to kernel taint
[  818.575752] Trace:
[  818.575752] [<fffffc000036ba3c>] free_event+0xec/0x220
[  818.575752] [<fffffc000036fa24>] SyS_perf_event_open+0x2a4/0x6f0
[  818.575752] [<fffffc0000310c24>] entSys+0xa4/0xc0
[  818.575752]
[  818.575752] Code: 27bb0047  23bdb5a0  23defff0  b53e0008  b75e0000 
47f00409 <a8500060> 40403121

The first instance of 'perf top' kept running happily.

Cheers
Michael.

Re: [tip:perf/core] perf: Rework the PMU methods

[Peter Zijlstra]

On Tue, 2010-09-14 at 22:11 +1200, Michael Cree wrote:
> 
> [  818.575752] Unable to handle kernel paging request at virtual address 
> 0000000000000060
> [  818.575752] perf(4935): Oops 0
> [  818.575752] pc = [<fffffc000036b8b8>]  ra = [<fffffc000036ba3c>]  ps 
> = 0000    Not tainted
> [  818.575752] pc is at put_ctx+0x18/0xb0
> [  818.575752] ra is at free_event+0xec/0x220
> [  818.575752] v0 = 0000000000000000  t0 = 0000000000000000  t1 = 
> 0000000000000002
> [  818.575752] t2 = 0000000000000000  t3 = 0000000000000000  t4 = 
> fffffc000063e4b0
> [  818.575752] t5 = fffffc0034837000  t6 = fffffc0034837000  t7 = 
> fffffc003abe0000
> [  818.575752] s0 = 0000000000000000  s1 = 0000000000000000  s2 = 
> 0000000000000000
> [  818.575752] s3 = ffffffffffffffff  s4 = 0000000000000000  s5 = 
> 0000000000000053
> [  818.575752] s6 = fffffc0034836c00
> [  818.575752] a0 = 0000000000000000  a1 = fffffc00311a6980  a2 = 
> 0000000000000000
> [  818.575752] a3 = 0000000000000001  a4 = fffffc0000797110  a5 = 
> 00000001200055f8
> [  818.575752] t8 = 0000000000000001  t9 = 00000001200396f4  t10= 
> 0000000000000000
> [  818.575752] t11= 000000000000000a  pv = fffffc000031d650  at = 
> fffffc000036f8bc
> [  818.575752] gp = fffffc00007d6e40  sp = fffffc003abe3e18
> [  818.575752] Disabling lock debugging due to kernel taint
> [  818.575752] Trace:
> [  818.575752] [<fffffc000036ba3c>] free_event+0xec/0x220
> [  818.575752] [<fffffc000036fa24>] SyS_perf_event_open+0x2a4/0x6f0
> [  818.575752] [<fffffc0000310c24>] entSys+0xa4/0xc0
> [  818.575752]
> [  818.575752] Code: 27bb0047  23bdb5a0  23defff0  b53e0008  b75e0000 
> 47f00409 <a8500060> 40403121 

Does the below cure that?

---
commit 0c67b40872326a5340cab51d79a192a5fbaeb484
Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date:   Mon Sep 13 11:15:58 2010 +0200

    perf: Fix free_event()
    
    With the context rework stuff we can actually end up freeing an event
    before it gets attached to a context.
    
    Reported-by: Cyrill Gorcunov <gorcunov@gmail.com>
    Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
    LKML-Reference: <new-submission>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index bc46bff..440f9ca 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -2150,7 +2150,9 @@ static void free_event(struct perf_event *event)
 	if (event->destroy)
 		event->destroy(event);
 
-	put_ctx(event->ctx);
+	if (event->ctx)
+		put_ctx(event->ctx);
+
 	call_rcu(&event->rcu_head, free_event_rcu);
 }
 

Re: [tip:perf/core] perf: Rework the PMU methods

[Michael Cree]

On 15/09/2010, at 2:07 AM, Peter Zijlstra wrote:
> On Tue, 2010-09-14 at 22:11 +1200, Michael Cree wrote:
>>
>> [  818.575752] Unable to handle kernel paging request at virtual  
>> address
>> 0000000000000060
>> [  818.575752] perf(4935): Oops 0
>
> Does the below cure that?
>
> ---
> commit 0c67b40872326a5340cab51d79a192a5fbaeb484
> Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Date:   Mon Sep 13 11:15:58 2010 +0200
>
>    perf: Fix free_event()

Yes, thanks, that's fixed it.

Cheers
Michael.