Re: [PATCH -tip] x86,perf: P4 PMU -- protect sensible procedures from preemption

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 2010-05-06 at 19:26 +0400, Cyrill Gorcunov wrote:
> On Thu, May 06, 2010 at 06:48:54PM +0400, Cyrill Gorcunov wrote:
> > On Thu, May 06, 2010 at 09:45:24AM -0400, Steven Rostedt wrote:
> > ...
> > > > > We want the one with the least runtime overhead. These are instrumentation
> > > > > routines, so we want to optimize them as much as possible.
> > > 
> > > 
> > > Yeah, my point was either disable preemption or keep the checks. In
> > > other words, if you don't disable preemption, do not use
> > > raw_smp_procesor_id(), because then we will not catch it if it changes
> > > in the future.
> > > 
> > > > ok, Ingo, dont apply this patch then for a while.
> > > 
> > > Send another patch, I'll test it again ;-)
> > > 
> > > -- Steve
> > > 
> > > 
> > 
> > Ingo, Steven, it seems we have potential preemtion available
> > in perf_event.c:validate_group:x86_pmu.schedule_events() which
> > is reached via syscall from userspace perf_event_open() call,
> > so get_cpu is still needed. But I'm a bit messed with call
> > graph at the moment :(
> > 
> > 	-- Cyrill
> 
> Steve, while I'm diving through call graph could you give this
> patch a shot? If preemtion happens -- it'll trigger it fast.

This one works too.

Tested-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> 
> 	-- Cyrill
> ---
>  arch/x86/kernel/cpu/perf_event_p4.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c
> =====================================================================
> --- linux-2.6.git.orig/arch/x86/kernel/cpu/perf_event_p4.c
> +++ linux-2.6.git/arch/x86/kernel/cpu/perf_event_p4.c
> @@ -421,7 +421,7 @@ static u64 p4_pmu_event_map(int hw_event
>  
>  static int p4_hw_config(struct perf_event *event)
>  {
> -	int cpu = raw_smp_processor_id();
> +	int cpu = get_cpu();
>  	u32 escr, cccr;
>  
>  	/*
> @@ -440,7 +440,7 @@ static int p4_hw_config(struct perf_even
>  		event->hw.config = p4_set_ht_bit(event->hw.config);
>  
>  	if (event->attr.type != PERF_TYPE_RAW)
> -		return 0;
> +		goto out;
>  
>  	/*
>  	 * We don't control raw events so it's up to the caller
> @@ -455,6 +455,8 @@ static int p4_hw_config(struct perf_even
>  		(p4_config_pack_escr(P4_ESCR_MASK_HT) |
>  		 p4_config_pack_cccr(P4_CCCR_MASK_HT));
>  
> +out:
> +	put_cpu();
>  	return 0;
>  }
>  
> @@ -741,7 +743,7 @@ static int p4_pmu_schedule_events(struct
>  {
>  	unsigned long used_mask[BITS_TO_LONGS(X86_PMC_IDX_MAX)];
>  	unsigned long escr_mask[BITS_TO_LONGS(ARCH_P4_TOTAL_ESCR)];
> -	int cpu = raw_smp_processor_id();
> +	int cpu = smp_processor_id();
>  	struct hw_perf_event *hwc;
>  	struct p4_event_bind *bind;
>  	unsigned int i, thread, num;