Re: [PATCH v6 02/10] tracing: add basic event trigger framework

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

Hi Tom,

This patch basically good for me.

Unfortunately, I've found a small build issue on this patch.

  CC      kernel/trace/trace_events_filter.o
/home/mhiramat/ksrc/linux-3/kernel/trace/trace_events.c:1864:1: error: static declaration of 'find_event_file' follows non-static declaration
/home/mhiramat/ksrc/linux-3/kernel/trace/trace.h:1019:34: note: previous declaration of 'find_event_file' was here
make[3]: *** [kernel/trace/trace_events.o] Error 1


> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index b1227b9..1733ac9 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1016,9 +1016,184 @@ extern void trace_event_enable_cmd_record(bool enable);
>  extern int event_trace_add_tracer(struct dentry *parent, struct trace_array *tr);
>  extern int event_trace_del_tracer(struct trace_array *tr);
>
> +extern struct ftrace_event_file *find_event_file(struct trace_array *tr,
> +						 const char *system,
> +						 const char *event);
> +

Here, you exposed find_event_file, but forgot to remove static from the definition
in kernel/trace/trace_events.c.
And even if it solved, I also hit an error and many warnings.

  LD      arch/x86/built-in.o
/home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c: In function 'event_trigger_callback':
/home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:510:14: error: 'struct event_trigger_data' has no member named 'post_trigger'
/home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c: At top level:
/home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:242:19: warning: 'register_event_command' defined but not used [-Wunused-function]
/home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:267:19: warning: 'unregister_event_command' defined but not used [-Wunused-function]
/home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:297:1: warning: 'event_trigger_print' defined but not used [-Wunused-function]
/home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:326:1: warning: 'event_trigger_init' defined but not used [-Wunused-function]
/home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:344:1: warning: 'event_trigger_free' defined but not used [-Wunused-function]
/home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:412:12: warning: 'register_trigger' defined but not used [-Wunused-function]
/home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:451:13: warning: 'unregister_trigger' defined but not used [-Wunused-function]
/home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:482:1: warning: 'event_trigger_callback' defined but not used [-Wunused-function]
make[3]: *** [kernel/trace/trace_events_trigger.o] Error 1

The post_trigger member is introduced in 7/10, and all warning helper functions
are used in the next (3/10) patch. So, I recommend you to move all post_trigger
related code into 7/10 or merge it into this patch, and move all helper functions
into 3/10 for fixing these warnings and an error. :)

(2013/08/23 8:27), Tom Zanussi wrote:
> +/**
> + * struct event_command - callbacks and data members for event commands
> + *
[...]
> + * @post_trigger: A flag that says whether or not this command needs
[...]
> +	trigger_data->count = -1;
> +	trigger_data->ops = trigger_ops;
> +	trigger_data->cmd_ops = cmd_ops;
> +	trigger_data->mode = cmd_ops->trigger_mode;
> +	trigger_data->post_trigger = cmd_ops->post_trigger;

BTW, why the trigger_data has a copy of trigger_mode and post_trigger?
It seems redundant... :(
Also, the "trigger_mode" name is not good for me, because we already has
FTRACE_EVENT_FL_TRIGGER_MODE_BIT bitflag. Perhaps, trigger_group or just
group_id may be better, I think.

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com