On Tue, Oct 10, 2017 at 03:43:59PM -0400, Steven Rostedt wrote:
> On Tue, 10 Oct 2017 18:31:26 +0200
> Peter Zijlstra <pet...@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).
