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

[Peter Zijlstra <peterz@infradead.org>]

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);