Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753960Ab3H1QiX (ORCPT ); Wed, 28 Aug 2013 12:38:23 -0400 Received: from hrndva-omtalb.mail.rr.com ([71.74.56.122]:23771 "EHLO hrndva-omtalb.mail.rr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753390Ab3H1QiV (ORCPT ); Wed, 28 Aug 2013 12:38:21 -0400 X-Authority-Analysis: v=2.0 cv=ddwCLAre c=1 sm=0 a=Sro2XwOs0tJUSHxCKfOySw==:17 a=Drc5e87SC40A:10 a=F6HMqF90r98A:10 a=5SG0PmZfjMsA:10 a=kj9zAlcOel0A:10 a=meVymXHHAAAA:8 a=KGjhK52YXX0A:10 a=5aXPCKecBmUA:10 a=QyXUC8HyAAAA:8 a=7z68rMhqe0dOtBY_z-kA:9 a=CjuIK1q_8ugA:10 a=Sro2XwOs0tJUSHxCKfOySw==:117 X-Cloudmark-Score: 0 X-Authenticated-User: X-Originating-IP: 67.255.60.225 Date: Wed, 28 Aug 2013 12:38:19 -0400 From: Steven Rostedt To: Tom Zanussi Cc: masami.hiramatsu.pt@hitachi.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 07/10] tracing: add and use generic set_trigger_filter() implementation Message-ID: <20130828123819.4dd50c1e@gandalf.local.home> In-Reply-To: <354e6cfb2edbf9c78e01ec31374e5e599c9bcdd9.1377631572.git.tom.zanussi@linux.intel.com> References: <354e6cfb2edbf9c78e01ec31374e5e599c9bcdd9.1377631572.git.tom.zanussi@linux.intel.com> X-Mailer: Claws Mail 3.9.2 (GTK+ 2.24.20; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15424 Lines: 446 On Tue, 27 Aug 2013 14:40:19 -0500 Tom Zanussi wrote: > enum { > FILTER_OTHER = 0, > diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h > index 326ba32..6c701c3 100644 > --- a/include/trace/ftrace.h > +++ b/include/trace/ftrace.h > @@ -412,13 +412,15 @@ static inline notrace int ftrace_get_offsets_##call( \ > * struct ftrace_data_offsets_ __maybe_unused __data_offsets; > * struct ring_buffer_event *event; > * struct ftrace_raw_ *entry; <-- defined in stage 1 > + * enum event_trigger_type __tt = ETT_NONE; > * struct ring_buffer *buffer; > * unsigned long irq_flags; > * int __data_size; > * int pc; > * > - * if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, > - * &ftrace_file->flags)) > + * if ((ftrace_file->flags & (FTRACE_EVENT_FL_SOFT_DISABLED | > + * FTRACE_EVENT_FL_TRIGGER_MODE)) == > + * FTRACE_EVENT_FL_SOFT_DISABLED) Don't worry too much about 80 character limit here. Move the FL_SOFT_DISABLED up. > * return; > * > * local_save_flags(irq_flags); > @@ -437,9 +439,19 @@ static inline notrace int ftrace_get_offsets_##call( \ > * { ; } <-- Here we assign the entries by the __field and > * __array macros. > * > - * if (!filter_current_check_discard(buffer, event_call, entry, event)) > - * trace_nowake_buffer_unlock_commit(buffer, > - * event, irq_flags, pc); > + * if (test_bit(FTRACE_EVENT_FL_TRIGGER_MODE_BIT, > + * &ftrace_file->flags)) > + * __tt = event_triggers_call(ftrace_file, entry); > + * > + * if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, > + * &ftrace_file->flags)) > + * ring_buffer_discard_commit(buffer, event); > + * else if (!filter_current_check_discard(buffer, event_call, > + * entry, event)) > + * trace_buffer_unlock_commit(buffer, event, irq_flags, pc); > + * > + * if (__tt) > + * event_triggers_post_call(ftrace_file, __tt); > * } > * > * static struct trace_event ftrace_event_type_ = { > @@ -521,17 +533,15 @@ ftrace_raw_event_##call(void *__data, proto) \ > struct ftrace_data_offsets_##call __maybe_unused __data_offsets;\ > struct ring_buffer_event *event; \ > struct ftrace_raw_##call *entry; \ > + enum event_trigger_type __tt = ETT_NONE; \ > struct ring_buffer *buffer; \ > unsigned long irq_flags; \ > int __data_size; \ > int pc; \ > \ > - if (test_bit(FTRACE_EVENT_FL_TRIGGER_MODE_BIT, \ > - &ftrace_file->flags)) \ > - event_triggers_call(ftrace_file); \ > - \ > - if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, \ > - &ftrace_file->flags)) \ > + if ((ftrace_file->flags & (FTRACE_EVENT_FL_SOFT_DISABLED | \ > + FTRACE_EVENT_FL_TRIGGER_MODE)) == \ > + FTRACE_EVENT_FL_SOFT_DISABLED) \ Ditto. Also, I don't think we need to worry about the flags changing, so we should be able to just save it. unsigned long eflags = ftrace_file_flags; And then we can also only delay the event triggers if it has a condition. What about adding a FTRACE_EVENT_FL_TRIGGER_COND_BIT, and do: if (!(eflags & FTRACE_EVENT_FL_TRIGGER_COND)) if (eflags & FTRACE_EVENT_FL_TRIGGER_MODE) event_triggers_call(ftrace_file, NULL); if (eflags & FTRACE_EVENT_FL_SOFT_DISABLED) return; } > return; \ > \ > local_save_flags(irq_flags); \ > @@ -551,8 +561,19 @@ ftrace_raw_event_##call(void *__data, proto) \ > \ > { assign; } \ > \ > - if (!filter_current_check_discard(buffer, event_call, entry, event)) \ > + if (test_bit(FTRACE_EVENT_FL_TRIGGER_MODE_BIT, \ > + &ftrace_file->flags)) \ > + __tt = event_triggers_call(ftrace_file, entry); \ Then here we test just the TRIGGER_COND bit. > + \ > + if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, \ > + &ftrace_file->flags)) \ > + ring_buffer_discard_commit(buffer, event); \ > + else if (!filter_current_check_discard(buffer, event_call, \ > + entry, event)) \ > trace_buffer_unlock_commit(buffer, event, irq_flags, pc); \ > + \ > + if (__tt) \ > + event_triggers_post_call(ftrace_file, __tt); \ This part is fine. > } > /* > * The ftrace_test_probe is compiled out, it is only here as a build time check > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > index 3cb846e..af5f3b6 100644 > --- a/kernel/trace/trace.h > +++ b/kernel/trace/trace.h > @@ -994,6 +994,10 @@ extern int apply_subsystem_event_filter(struct ftrace_subsystem_dir *dir, > extern void print_subsystem_event_filter(struct event_subsystem *system, > struct trace_seq *s); > extern int filter_assign_type(const char *type); > +extern int create_event_filter(struct ftrace_event_call *call, > + char *filter_str, bool set_str, > + struct event_filter **filterp); > +extern void free_event_filter(struct event_filter *filter); > > struct ftrace_event_field * > trace_find_event_field(struct ftrace_event_call *call, char *name); > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c > index 97daa8c..0c45aa1 100644 > --- a/kernel/trace/trace_events_filter.c > +++ b/kernel/trace/trace_events_filter.c > @@ -781,6 +781,11 @@ static void __free_filter(struct event_filter *filter) > kfree(filter); > } > > +void free_event_filter(struct event_filter *filter) > +{ > + __free_filter(filter); > +} > + > /* > * Called when destroying the ftrace_event_call. > * The call is being freed, so we do not need to worry about > @@ -1806,6 +1811,14 @@ static int create_filter(struct ftrace_event_call *call, > return err; > } > > +int create_event_filter(struct ftrace_event_call *call, > + char *filter_str, bool set_str, > + struct event_filter **filterp) > +{ > + return create_filter(call, filter_str, set_str, filterp); > +} > + > + > /** > * create_system_filter - create a filter for an event_subsystem > * @system: event_subsystem to create a filter for > diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c > index 54678e2..b5e7ca7 100644 > --- a/kernel/trace/trace_events_trigger.c > +++ b/kernel/trace/trace_events_trigger.c > @@ -43,24 +43,53 @@ struct event_trigger_data { > static void > trigger_data_free(struct event_trigger_data *data) > { > + if (data->cmd_ops->set_filter) > + data->cmd_ops->set_filter(NULL, data, NULL); > + > synchronize_sched(); /* make sure current triggers exit before free */ > kfree(data); > } > > -void event_triggers_call(struct ftrace_event_file *file) > +enum event_trigger_type > +event_triggers_call(struct ftrace_event_file *file, void *rec) > { > struct event_trigger_data *data; > + enum event_trigger_type tt = ETT_NONE; > > if (list_empty(&file->triggers)) > - return; > + return tt; > > preempt_disable_notrace(); > - list_for_each_entry_rcu(data, &file->triggers, list) > + list_for_each_entry_rcu(data, &file->triggers, list) { > + if (data->filter && !filter_match_preds(data->filter, rec)) We would need a check for !rec, just to be safe (with the mods I talked about). > + continue; > + if (data->cmd_ops->post_trigger) { > + tt |= data->cmd_ops->trigger_type; > + continue; > + } > data->ops->func((void **)&data); > + } > preempt_enable_notrace(); > + > + return tt; > } > EXPORT_SYMBOL_GPL(event_triggers_call); > > +void > +event_triggers_post_call(struct ftrace_event_file *file, > + enum event_trigger_type tt) > +{ > + struct event_trigger_data *data; > + > + preempt_disable_notrace(); > + list_for_each_entry_rcu(data, &file->triggers, list) { > + if (data->cmd_ops->trigger_type & tt) > + data->ops->func((void **)&data); > + } > + preempt_enable_notrace(); > +} > +EXPORT_SYMBOL_GPL(event_triggers_post_call); > + > static void *trigger_next(struct seq_file *m, void *t, loff_t *pos) > { > struct ftrace_event_file *event_file = event_file_data(m->private); > @@ -561,6 +590,66 @@ event_trigger_callback(struct event_command *cmd_ops, > goto out; > } > > +/** > + * set_trigger_filter - generic event_command @set_filter > + * implementation > + * > + * Common implementation for event command filter parsing and filter > + * instantiation. > + * > + * Usually used directly as the @set_filter method in event command > + * implementations. > + * > + * Also used to remove a filter (if filter_str = NULL). > + */ > +static int set_trigger_filter(char *filter_str, void *trigger_data, > + struct ftrace_event_file *file) > +{ > + struct event_trigger_data *data = trigger_data; > + struct event_filter *filter = NULL, *tmp; > + int ret = -EINVAL; > + char *s; > + > + if (!filter_str) /* clear the current filter */ > + goto assign; > + > + s = strsep(&filter_str, " \t"); > + > + if (!strlen(s) || strcmp(s, "if") != 0) > + goto out; > + > + if (!filter_str) > + goto out; > + > + /* The filter is for the 'trigger' event, not the triggered event */ > + ret = create_event_filter(file->event_call, filter_str, false, &filter); > + if (ret) > + goto out; > + assign: > + tmp = data->filter; > + > + rcu_assign_pointer(data->filter, filter); > + > + if (tmp) { > + /* Make sure the call is done with the filter */ > + synchronize_sched(); > + free_event_filter(tmp); > + } > + > + kfree(data->filter_str); > + > + if (filter_str) { > + data->filter_str = kstrdup(filter_str, GFP_KERNEL); > + if (!data->filter_str) { > + free_event_filter(data->filter); > + data->filter = NULL; > + ret = -ENOMEM; > + } > + } > + out: > + return ret; > +} > + > static void > traceon_trigger(void **_data) > { > @@ -698,6 +787,7 @@ static struct event_command trigger_traceon_cmd = { > .reg = register_trigger, > .unreg = unregister_trigger, > .get_trigger_ops = onoff_get_trigger_ops, > + .set_filter = set_trigger_filter, > }; > > static struct event_command trigger_traceoff_cmd = { > @@ -707,6 +797,7 @@ static struct event_command trigger_traceoff_cmd = { > .reg = register_trigger, > .unreg = unregister_trigger, > .get_trigger_ops = onoff_get_trigger_ops, > + .set_filter = set_trigger_filter, > }; > > static void > @@ -788,6 +879,7 @@ static struct event_command trigger_snapshot_cmd = { > .reg = register_snapshot_trigger, > .unreg = unregister_trigger, > .get_trigger_ops = snapshot_get_trigger_ops, > + .set_filter = set_trigger_filter, > }; > > /* > @@ -867,6 +959,7 @@ static struct event_command trigger_stacktrace_cmd = { > .reg = register_trigger, > .unreg = unregister_trigger, > .get_trigger_ops = stacktrace_get_trigger_ops, > + .set_filter = set_trigger_filter, > }; > > static __init void unregister_trigger_traceon_traceoff_cmds(void) > @@ -1194,6 +1287,7 @@ static struct event_command trigger_enable_cmd = { > .reg = event_enable_register_trigger, > .unreg = event_enable_unregister_trigger, > .get_trigger_ops = event_enable_get_trigger_ops, > + .set_filter = set_trigger_filter, > }; > > static struct event_command trigger_disable_cmd = { > @@ -1203,6 +1297,7 @@ static struct event_command trigger_disable_cmd = { > .reg = event_enable_register_trigger, > .unreg = event_enable_unregister_trigger, > .get_trigger_ops = event_enable_get_trigger_ops, > + .set_filter = set_trigger_filter, > }; > > static __init void unregister_trigger_enable_disable_cmds(void) > diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c > index 4f56d54..84cdbce 100644 > --- a/kernel/trace/trace_syscalls.c > +++ b/kernel/trace/trace_syscalls.c > @@ -306,6 +306,7 @@ static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id) > struct syscall_trace_enter *entry; > struct syscall_metadata *sys_data; > struct ring_buffer_event *event; > + enum event_trigger_type __tt = ETT_NONE; > struct ring_buffer *buffer; > unsigned long irq_flags; > int pc; > @@ -321,9 +322,9 @@ static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id) > if (!ftrace_file) > return; > > - if (test_bit(FTRACE_EVENT_FL_TRIGGER_MODE_BIT, &ftrace_file->flags)) > - event_triggers_call(ftrace_file); > - if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &ftrace_file->flags)) > + if ((ftrace_file->flags & > + (FTRACE_EVENT_FL_SOFT_DISABLED | FTRACE_EVENT_FL_TRIGGER_MODE)) == > + FTRACE_EVENT_FL_SOFT_DISABLED) We would need the same changes here too. -- Steve > return; > > sys_data = syscall_nr_to_meta(syscall_nr); > @@ -345,10 +346,17 @@ static void ftrace_syscall_enter(void *data, struct pt_regs *regs, long id) > entry->nr = syscall_nr; > syscall_get_arguments(current, regs, 0, sys_data->nb_args, entry->args); > > - if (!filter_current_check_discard(buffer, sys_data->enter_event, > - entry, event)) > + if (test_bit(FTRACE_EVENT_FL_TRIGGER_MODE_BIT, &ftrace_file->flags)) > + __tt = event_triggers_call(ftrace_file, entry); > + > + if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &ftrace_file->flags)) > + ring_buffer_discard_commit(buffer, event); > + else if (!filter_current_check_discard(buffer, sys_data->enter_event, > + entry, event)) > trace_current_buffer_unlock_commit(buffer, event, > irq_flags, pc); > + if (__tt) > + event_triggers_post_call(ftrace_file, __tt); > } > > static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret) > @@ -358,6 +366,7 @@ static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret) > struct syscall_trace_exit *entry; > struct syscall_metadata *sys_data; > struct ring_buffer_event *event; > + enum event_trigger_type __tt = ETT_NONE; > struct ring_buffer *buffer; > unsigned long irq_flags; > int pc; > @@ -372,9 +381,9 @@ static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret) > if (!ftrace_file) > return; > > - if (test_bit(FTRACE_EVENT_FL_TRIGGER_MODE_BIT, &ftrace_file->flags)) > - event_triggers_call(ftrace_file); > - if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &ftrace_file->flags)) > + if ((ftrace_file->flags & > + (FTRACE_EVENT_FL_SOFT_DISABLED | FTRACE_EVENT_FL_TRIGGER_MODE)) == > + FTRACE_EVENT_FL_SOFT_DISABLED) > return; > > sys_data = syscall_nr_to_meta(syscall_nr); > @@ -395,10 +404,17 @@ static void ftrace_syscall_exit(void *data, struct pt_regs *regs, long ret) > entry->nr = syscall_nr; > entry->ret = syscall_get_return_value(current, regs); > > - if (!filter_current_check_discard(buffer, sys_data->exit_event, > - entry, event)) > + if (test_bit(FTRACE_EVENT_FL_TRIGGER_MODE_BIT, &ftrace_file->flags)) > + __tt = event_triggers_call(ftrace_file, entry); > + > + if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &ftrace_file->flags)) > + ring_buffer_discard_commit(buffer, event); > + else if (!filter_current_check_discard(buffer, sys_data->exit_event, > + entry, event)) > trace_current_buffer_unlock_commit(buffer, event, > irq_flags, pc); > + if (__tt) > + event_triggers_post_call(ftrace_file, __tt); > } > > static int reg_event_syscall_enter(struct ftrace_event_file *file, -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/