Messages in this thread | | | Date | Tue, 10 Oct 2017 23:07:07 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH 2/3] perf/ftrace: Fix function trace events |
| |
On Tue, Oct 10, 2017 at 03:43:59PM -0400, Steven Rostedt wrote: > On Tue, 10 Oct 2017 18:31:26 +0200 > Peter Zijlstra <peterz@infradead.org> wrote:
> > --- a/kernel/trace/trace_event_perf.c > > +++ b/kernel/trace/trace_event_perf.c > > @@ -240,27 +240,31 @@ void perf_trace_destroy(struct perf_even > > int perf_trace_add(struct perf_event *p_event, int flags) > > { > > struct trace_event_call *tp_event = p_event->tp_event; > > - struct hlist_head __percpu *pcpu_list; > > - struct hlist_head *list; > > > > - pcpu_list = tp_event->perf_events; > > - if (WARN_ON_ONCE(!pcpu_list)) > > - return -EINVAL; > > You're going to add a comment here on v2? > > Also, this is highly subtle, that ftrace reg returns non-zero, and all > others return zero. It may be good to add a comment in > include/linux/trace_events.h by the TRACE_REG_PERF_ADD and DEL enums, > stating what is expected when they are passed in.
Yeah, I was going to add a comment; your earlier email came in just after I send out these ;-)
> > perf_ftrace_function_call(unsigned long ip, unsigned long parent_ip, > > struct ftrace_ops *ops, struct pt_regs *pt_regs) > > { > > + struct hlist_head head = HLIST_HEAD_INIT; > > struct ftrace_entry *entry; > > - struct hlist_head *head; > > + struct perf_event *event; > > struct pt_regs regs; > > int rctx; > > > > - head = this_cpu_ptr(event_function.perf_events); > > - if (hlist_empty(head)) > > + event = container_of(ops, struct perf_event, ftrace_ops); > > + > > + if ((unsigned long)event->ftrace_ops.private != smp_processor_id()) > > Could you also just do: > > if ((unsigned long)ops->private != smp_processor_id()) > > ?
Ah yes of course.
> > return; > > > > + hlist_add_head(&event->hlist_entry, &head); > > + > > #define ENTRY_SIZE (ALIGN(sizeof(struct ftrace_entry) + sizeof(u32), \ > > sizeof(u64)) - sizeof(u32)) > > > > @@ -330,17 +338,21 @@ perf_ftrace_function_call(unsigned long > > entry->ip = ip; > > entry->parent_ip = parent_ip; > > perf_trace_buf_submit(entry, ENTRY_SIZE, rctx, TRACE_FN, > > - 1, ®s, head, NULL); > > + 1, ®s, &head, NULL); > > > > #undef ENTRY_SIZE > > + > > + hlist_del_init(&event->hlist_entry); > > Hmm, is there a better way to do this? It adds and removes the entry at > each function trace.
Hmm, hlist_add_head() doesn't seem to use anything from @n if !@h->first, in which case I could simply remove this. I should probably add a comment to clarify this.
> > } > > > > static int perf_ftrace_function_register(struct perf_event *event) > > { > > struct ftrace_ops *ops = &event->ftrace_ops; > > > > - ops->flags |= FTRACE_OPS_FL_PER_CPU | FTRACE_OPS_FL_RCU; > > - ops->func = perf_ftrace_function_call; > > + ops->flags |= FTRACE_OPS_FL_RCU; > > I know this doesn't change the patch, but I don't believe that "|=" is > necessary. "=" should work too. But you can make that a separate > patch, in case it does break something, and we have a nice bisect to > see this was the cause.
Will add a 4th patch to that effect.
> > @@ -377,11 +381,11 @@ int perf_ftrace_event_register(struct tr > > case TRACE_REG_PERF_CLOSE: > > return perf_ftrace_function_unregister(data); > > case TRACE_REG_PERF_ADD: > > - perf_ftrace_function_enable(data); > > - return 0; > > + event->ftrace_ops.private = (void *)(unsigned long)smp_processor_id(); > > I curious, is there a per CPU event, or is this called during > sched_switch or something?
Yes (the latter).
| |