Messages in this thread | | | Subject | Re: [PATCH 1/4] tracing: Use the perf recursion protection from trace event | From | Peter Zijlstra <> | Date | Sun, 22 Nov 2009 12:00:14 +0100 |
| |
On Sun, 2009-11-22 at 05:21 +0100, Frederic Weisbecker wrote: > diff --git a/kernel/perf_event.c b/kernel/perf_event.c > index 718fa93..aba8227 100644 > --- a/kernel/perf_event.c > +++ b/kernel/perf_event.c > @@ -3880,34 +3880,42 @@ static void perf_swevent_ctx_event(struct perf_event_context *ctx, > } > } > > -static int *perf_swevent_recursion_context(struct perf_cpu_context *cpuctx) > +/* > + * Must be called with preemption disabled > + */ > +int perf_swevent_get_recursion_context(int **recursion) > { > + struct perf_cpu_context *cpuctx = &__get_cpu_var(perf_cpu_context); > + > if (in_nmi()) > - return &cpuctx->recursion[3]; > + *recursion = &cpuctx->recursion[3]; > + else if (in_irq()) > + *recursion = &cpuctx->recursion[2]; > + else if (in_softirq()) > + *recursion = &cpuctx->recursion[1]; > + else > + *recursion = &cpuctx->recursion[0]; > > - if (in_irq()) > - return &cpuctx->recursion[2]; > + if (**recursion) > + return -1; > > - if (in_softirq()) > - return &cpuctx->recursion[1]; > + (**recursion)++; > > - return &cpuctx->recursion[0]; > + return 0; > }
You lost the barrier();
> -static void do_perf_sw_event(enum perf_type_id type, u32 event_id, > - u64 nr, int nmi, > - struct perf_sample_data *data, > - struct pt_regs *regs) > +void perf_swevent_put_recursion_context(int *recursion) > { > - struct perf_cpu_context *cpuctx = &get_cpu_var(perf_cpu_context); > - int *recursion = perf_swevent_recursion_context(cpuctx); > - struct perf_event_context *ctx; > - > - if (*recursion) > - goto out; > + (*recursion)--; > +}
And here as well.
Furthermore, its much cleaner if you simply use get_cpu_var(perf_cpu_context) in get_recursion_context, and put_cpu_var() in put_recursion_context.
That way you put the preempt_disable where it belongs, instead of:
> +static void do_perf_sw_event(enum perf_type_id type, u32 event_id, > + u64 nr, int nmi, > + struct perf_sample_data *data, > + struct pt_regs *regs) > +{ > + int *recursion; > + > + preempt_disable(); > + > + if (perf_swevent_get_recursion_context(&recursion)) > + goto out; > + > + __do_perf_sw_event(type, event_id, nr, nmi, data, regs); > > + perf_swevent_put_recursion_context(recursion); > out: > - put_cpu_var(perf_cpu_context); > + preempt_enable(); > }
mucking about like here, also it cleans up the code in that you don't have to drag that recursion variable around like so.
| |