Re: [PATCH] tracing: Don't display trigger file for events that don't have register func

From: Chunyu Hu
Date: Tue May 03 2016 - 07:29:46 EST




----- Original Message -----
> From: "Steven Rostedt" <rostedt@xxxxxxxxxxx>
> To: "Chunyu Hu" <chuhu@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Sent: Tuesday, May 3, 2016 10:42:44 AM
> Subject: Re: [PATCH] tracing: Don't display trigger file for events that don't have register func
>
> On Mon, 2 May 2016 22:14:14 +0800
> Chunyu Hu <chuhu@xxxxxxxxxx> wrote:
>
> > Currently register function of the event will be called
> > through the 'reg' field of event class directly without
> > any check when seting up triggers.
> >
> > Triggers for events that don't support register through
> > debug fs (events under events/ftrace are for perf to
>
> Actually, they were created for trace-cmd. I'm not even sure if perf
> uses the ftrace event formats.

Thanks for correcting me. I misunderstood it.

>
> > read event format, and most of them don't have regisgter
> > function except events/ftrace/function.) can't be enabled
> > at all, and an oops will be hit when setting up trigger
> > for those events, so just not showing them is an easy way
> > to avoid the oops.
> >
> > Signed-off-by: Chunyu Hu <chuhu@xxxxxxxxxx>
> > ---
> > kernel/trace/trace_events.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index da1eeb6..9fb99fd 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -2138,9 +2138,10 @@ event_create_dir(struct dentry *parent, struct
> > trace_event_file *file)
> > trace_create_file("filter", 0644, file->dir, file,
> > &ftrace_event_filter_fops);
> >
> > - trace_create_file("trigger", 0644, file->dir, file,
> > - &event_trigger_fops);
> > -
> > + if (call->class->reg) {
> > + trace_create_file("trigger", 0644, file->dir, file,
> > + &event_trigger_fops);
> > + }
>
> As you stated, reg is there for function tracing, and is not a good
> value to use as a check. Use the following check instead:
>
> if (!(call->flags & TRACE_EVENT_FL_IGNORE_ENABLE))
> trace_create_file("trigger", 0644, file->dir, file,
> &event_trigger_fops);

Thanks. Agree, will prepare v2 for this.

It looks like that checking both is safest way, like other parts
did in trace_events.c. but i didn't see other events using
TRACE_EVENT_FL_IGNORE_ENABLE, ftrace event sub system is the only user now.
and didn't see other events that don't have register func.
so i think it's safe to use only this flag to check it.

> And add a comment that states that only event directories that can be
> enabled should have triggers.

Ok.

> -- Steve
>
>
>
> > #ifdef CONFIG_HIST_TRIGGERS
> > trace_create_file("hist", 0444, file->dir, file,
> > &event_hist_fops);
>
>

--
Regards,
Chunyu Hu