* [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable @ 2009-06-17 8:46 Li Zefan 2009-06-17 8:47 ` [PATCH 2/2] tracing/filters: remove error messages Li Zefan 2009-06-18 5:58 ` [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable Tom Zanussi 0 siblings, 2 replies; 10+ messages in thread From: Li Zefan @ 2009-06-17 8:46 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar; +Cc: Frederic Weisbecker, Tom Zanussi, LKML | commit 7ce7e4249921d5073e764f7ff7ad83cfa9894bd7 | Author: Tom Zanussi <tzanussi@gmail.com> | Date: Sun Mar 22 03:31:04 2009 -0500 | | tracing: add per-event filtering | ... | | Filters can also be set or cleared for a complete subsystem by writing | the same filter as would be written to an individual event to the | filter file at the root of the subsytem. Note however, that if any | event in the subsystem lacks a field specified in the filter being | set, the set will fail and all filters in the subsytem are | automatically cleared. This change from the previous version was made | because using only the fields that happen to exist for a given event | would most likely result in a meaningless filter. I really don't like this change. It is inconvenient, and makes subsystem filter much less useful: # echo 'vec == 1' > irq/softirq_entry/filter # echo 'irq == 5' > irq/filter bash: echo: write error: Invalid argument # cat irq/softirq_entry/filter none I'd expect this will set the filter for irq_handler_entry and irq_handler_exit, and won't touch softirq_entry and softirq_exit. But it just failed, and what's worse, each event's filter was cleared. The basic idea of this patch is, save the filter before applying the new one, and fall back to it if the new one can't be applied: # echo 'vec == 1' > softirq_entry/filter # echo 'irq == 5' > filter # cat irq_handler_entry/filter irq == 5 # cat softirq_entry/filter vec == 1 [ Impact: allow subsystem filter be applied to parts of the subsys ] Signed-off-by: Li Zefan <lizf@cn.fujitsu.com> --- kernel/trace/trace.h | 2 + kernel/trace/trace_events_filter.c | 138 +++++++++++++++++++++++++++++------- 2 files changed, 114 insertions(+), 26 deletions(-) diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h index 7d5cc37..b5153e9 100644 --- a/kernel/trace/trace.h +++ b/kernel/trace/trace.h @@ -763,6 +763,8 @@ struct event_filter { int n_preds; struct filter_pred **preds; char *filter_string; + char *old_filter; + bool need_reset; }; struct event_subsystem { diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c index 936c621..2f706e5 100644 --- a/kernel/trace/trace_events_filter.c +++ b/kernel/trace/trace_events_filter.c @@ -246,6 +246,29 @@ static int replace_filter_string(struct event_filter *filter, return 0; } +static int save_old_filter(struct event_filter *filter) +{ + char *old = filter->old_filter; + + kfree(old); + + if (filter->filter_string) + old = kstrdup(filter->filter_string, GFP_KERNEL); + else + old = kstrdup("0", GFP_KERNEL); + filter->old_filter = old; + + if (!old) + return -ENOMEM; + return 0; +} + +static void free_old_filter(struct event_filter *filter) +{ + kfree(filter->old_filter); + filter->old_filter = NULL; +} + static int append_filter_string(struct event_filter *filter, char *string) { @@ -374,6 +397,8 @@ void destroy_preds(struct ftrace_event_call *call) struct event_filter *filter = call->filter; int i; + WARN_ON(filter->old_filter); + for (i = 0; i < MAX_FILTER_PRED; i++) { if (filter->preds[i]) filter_free_pred(filter->preds[i]); @@ -418,10 +443,9 @@ oom: } EXPORT_SYMBOL_GPL(init_preds); -static void filter_free_subsystem_preds(struct event_subsystem *system) +static void __filter_free_subsystem_preds(struct event_subsystem *system) { struct event_filter *filter = system->filter; - struct ftrace_event_call *call; int i; if (filter->n_preds) { @@ -431,11 +455,22 @@ static void filter_free_subsystem_preds(struct event_subsystem *system) filter->preds = NULL; filter->n_preds = 0; } +} + +static void filter_free_subsystem_preds(struct event_subsystem *system, + bool save_old) +{ + struct ftrace_event_call *call; + + __filter_free_subsystem_preds(system); list_for_each_entry(call, &ftrace_events, list) { if (!call->define_fields) continue; + if (save_old) + save_old_filter(call->filter); + if (!strcmp(call->system, system->name)) { filter_disable_preds(call); remove_filter_string(call->filter); @@ -608,8 +643,9 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps, char *filter_string) { struct event_filter *filter = system->filter; + struct event_filter *call_filter; struct ftrace_event_call *call; - int err = 0; + int err; if (!filter->preds) { filter->preds = kzalloc(MAX_FILTER_PRED * sizeof(pred), @@ -629,22 +665,23 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps, list_for_each_entry(call, &ftrace_events, list) { + call_filter = call->filter; + if (!call->define_fields) continue; if (strcmp(call->system, system->name)) continue; + if (call_filter->need_reset) + continue; + err = filter_add_pred(ps, call, pred); - if (err) { - filter_free_subsystem_preds(system); - parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0); - goto out; - } - replace_filter_string(call->filter, filter_string); + if (err) + call_filter->need_reset = true; } -out: - return err; + + return 0; } static void parse_init(struct filter_parse_state *ps, @@ -1063,25 +1100,21 @@ static int replace_preds(struct event_subsystem *system, return 0; } -int apply_event_filter(struct ftrace_event_call *call, char *filter_string) +int __apply_event_filter(struct ftrace_event_call *call, char *filter_string) { int err; struct filter_parse_state *ps; - mutex_lock(&event_mutex); - if (!strcmp(strstrip(filter_string), "0")) { filter_disable_preds(call); remove_filter_string(call->filter); - mutex_unlock(&event_mutex); return 0; } - err = -ENOMEM; ps = kzalloc(sizeof(*ps), GFP_KERNEL); if (!ps) - goto out_unlock; + return -ENOMEM; filter_disable_preds(call); replace_filter_string(call->filter, filter_string); @@ -1101,23 +1134,52 @@ out: filter_opstack_clear(ps); postfix_clear(ps); kfree(ps); -out_unlock: - mutex_unlock(&event_mutex); return err; } +void __apply_event_old_filter(struct ftrace_event_call *call) +{ + struct event_filter *filter = call->filter; + int ret; + + ret = __apply_event_filter(call, filter->old_filter); + if (ret) { + ret = __apply_event_filter(call, "0"); + WARN_ON(ret); + } +} + +int apply_event_filter(struct ftrace_event_call *call, char *filter_string) +{ + int ret; + + ret = save_old_filter(call->filter); + if (ret) + return ret; + + mutex_lock(&event_mutex); + ret = __apply_event_filter(call, filter_string); + if (ret) + __apply_event_old_filter(call); + mutex_unlock(&event_mutex); + + free_old_filter(call->filter); + return ret; +} + int apply_subsystem_event_filter(struct event_subsystem *system, char *filter_string) { - int err; - + struct event_filter *filter; + struct ftrace_event_call *call; struct filter_parse_state *ps; + int err; mutex_lock(&event_mutex); if (!strcmp(strstrip(filter_string), "0")) { - filter_free_subsystem_preds(system); + filter_free_subsystem_preds(system, false); remove_filter_string(system->filter); mutex_unlock(&event_mutex); return 0; @@ -1128,24 +1190,48 @@ int apply_subsystem_event_filter(struct event_subsystem *system, if (!ps) goto out_unlock; - filter_free_subsystem_preds(system); - replace_filter_string(system->filter, filter_string); + filter = system->filter; + filter_free_subsystem_preds(system, true); + save_old_filter(filter); + replace_filter_string(filter, filter_string); parse_init(ps, filter_ops, filter_string); err = filter_parse(ps); if (err) { - append_filter_err(ps, system->filter); + replace_filter_string(filter, filter->old_filter); goto out; } err = replace_preds(system, NULL, ps, filter_string); if (err) - append_filter_err(ps, system->filter); + replace_filter_string(filter, filter->old_filter); + + /* + * For those events that the filter can't be applied to, + * reset them with their original filters. + */ + list_for_each_entry(call, &ftrace_events, list) { + if (!call->define_fields) + continue; + + if (strcmp(call->system, system->name) != 0) + continue; + + filter = call->filter; + if (filter->need_reset || err) { + filter->need_reset = false; + __apply_event_old_filter(call); + } else + replace_filter_string(filter, filter_string); + + free_old_filter(filter); + } out: filter_opstack_clear(ps); postfix_clear(ps); kfree(ps); + free_old_filter(system->filter); out_unlock: mutex_unlock(&event_mutex); -- 1.5.4.rc3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] tracing/filters: remove error messages 2009-06-17 8:46 [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable Li Zefan @ 2009-06-17 8:47 ` Li Zefan 2009-06-17 12:02 ` Frederic Weisbecker 2009-06-18 5:58 ` [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable Tom Zanussi 1 sibling, 1 reply; 10+ messages in thread From: Li Zefan @ 2009-06-17 8:47 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar; +Cc: Frederic Weisbecker, Tom Zanussi, LKML Now we restore original filter is the new one can't be applied, and no long show error messages, so we can remove them totally. Another reason is, I don't think it's good to show error messages when reading a control file. Signed-off-by: Li Zefan <lizf@cn.fujitsu.com> --- kernel/trace/trace_events_filter.c | 168 ++++++------------------------------ 1 files changed, 27 insertions(+), 141 deletions(-) diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c index 2f706e5..cd7a928 100644 --- a/kernel/trace/trace_events_filter.c +++ b/kernel/trace/trace_events_filter.c @@ -60,36 +60,6 @@ static struct filter_op filter_ops[] = { { OP_OPEN_PAREN, "(", 0 }, }; -enum { - FILT_ERR_NONE, - FILT_ERR_INVALID_OP, - FILT_ERR_UNBALANCED_PAREN, - FILT_ERR_TOO_MANY_OPERANDS, - FILT_ERR_OPERAND_TOO_LONG, - FILT_ERR_FIELD_NOT_FOUND, - FILT_ERR_ILLEGAL_FIELD_OP, - FILT_ERR_ILLEGAL_INTVAL, - FILT_ERR_BAD_SUBSYS_FILTER, - FILT_ERR_TOO_MANY_PREDS, - FILT_ERR_MISSING_FIELD, - FILT_ERR_INVALID_FILTER, -}; - -static char *err_text[] = { - "No error", - "Invalid operator", - "Unbalanced parens", - "Too many operands", - "Operand too long", - "Field not found", - "Illegal operation for field type", - "Illegal integer value", - "Couldn't find or set field in one of a subsystem's events", - "Too many terms in predicate expression", - "Missing field name and/or value", - "Meaningless filter expression", -}; - struct opstack_op { int op; struct list_head list; @@ -105,8 +75,6 @@ struct filter_parse_state { struct filter_op *ops; struct list_head opstack; struct list_head postfix; - int lasterr; - int lasterr_pos; struct { char *string; @@ -223,12 +191,6 @@ int filter_match_preds(struct ftrace_event_call *call, void *rec) } EXPORT_SYMBOL_GPL(filter_match_preds); -static void parse_error(struct filter_parse_state *ps, int err, int pos) -{ - ps->lasterr = err; - ps->lasterr_pos = pos; -} - static void remove_filter_string(struct event_filter *filter) { kfree(filter->filter_string); @@ -269,48 +231,6 @@ static void free_old_filter(struct event_filter *filter) filter->old_filter = NULL; } -static int append_filter_string(struct event_filter *filter, - char *string) -{ - int newlen; - char *new_filter_string; - - BUG_ON(!filter->filter_string); - newlen = strlen(filter->filter_string) + strlen(string) + 1; - new_filter_string = kmalloc(newlen, GFP_KERNEL); - if (!new_filter_string) - return -ENOMEM; - - strcpy(new_filter_string, filter->filter_string); - strcat(new_filter_string, string); - kfree(filter->filter_string); - filter->filter_string = new_filter_string; - - return 0; -} - -static void append_filter_err(struct filter_parse_state *ps, - struct event_filter *filter) -{ - int pos = ps->lasterr_pos; - char *buf, *pbuf; - - buf = (char *)__get_free_page(GFP_TEMPORARY); - if (!buf) - return; - - append_filter_string(filter, "\n"); - memset(buf, ' ', PAGE_SIZE); - if (pos > PAGE_SIZE - 128) - pos = 0; - buf[pos] = '^'; - pbuf = &buf[pos] + 1; - - sprintf(pbuf, "\nparse_error: %s\n", err_text[ps->lasterr]); - append_filter_string(filter, buf); - free_page((unsigned long) buf); -} - void print_event_filter(struct ftrace_event_call *call, struct trace_seq *s) { struct event_filter *filter = call->filter; @@ -478,18 +398,15 @@ static void filter_free_subsystem_preds(struct event_subsystem *system, } } -static int filter_add_pred_fn(struct filter_parse_state *ps, - struct ftrace_event_call *call, +static int filter_add_pred_fn(struct ftrace_event_call *call, struct filter_pred *pred, filter_pred_fn_t fn) { struct event_filter *filter = call->filter; int idx, err; - if (filter->n_preds == MAX_FILTER_PRED) { - parse_error(ps, FILT_ERR_TOO_MANY_PREDS, 0); + if (filter->n_preds == MAX_FILTER_PRED) return -ENOSPC; - } idx = filter->n_preds; filter_clear_pred(filter->preds[idx]); @@ -570,8 +487,7 @@ static filter_pred_fn_t select_comparison_fn(int op, int field_size, return fn; } -static int filter_add_pred(struct filter_parse_state *ps, - struct ftrace_event_call *call, +static int filter_add_pred(struct ftrace_event_call *call, struct filter_pred *pred) { struct ftrace_event_field *field; @@ -584,24 +500,20 @@ static int filter_add_pred(struct filter_parse_state *ps, if (pred->op == OP_AND) { pred->pop_n = 2; - return filter_add_pred_fn(ps, call, pred, filter_pred_and); + return filter_add_pred_fn(call, pred, filter_pred_and); } else if (pred->op == OP_OR) { pred->pop_n = 2; - return filter_add_pred_fn(ps, call, pred, filter_pred_or); + return filter_add_pred_fn(call, pred, filter_pred_or); } field = find_event_field(call, pred->field_name); - if (!field) { - parse_error(ps, FILT_ERR_FIELD_NOT_FOUND, 0); + if (!field) return -EINVAL; - } pred->offset = field->offset; - if (!is_legal_op(field, pred->op)) { - parse_error(ps, FILT_ERR_ILLEGAL_FIELD_OP, 0); + if (!is_legal_op(field, pred->op)) return -EINVAL; - } string_type = is_string_field(field->type); if (string_type) { @@ -612,33 +524,28 @@ static int filter_add_pred(struct filter_parse_state *ps, pred->str_len = field->size; if (pred->op == OP_NE) pred->not = 1; - return filter_add_pred_fn(ps, call, pred, fn); + return filter_add_pred_fn(call, pred, fn); } else { if (field->is_signed) ret = strict_strtoll(pred->str_val, 0, &val); else ret = strict_strtoull(pred->str_val, 0, &val); - if (ret) { - parse_error(ps, FILT_ERR_ILLEGAL_INTVAL, 0); + if (ret) return -EINVAL; - } pred->val = val; } fn = select_comparison_fn(pred->op, field->size, field->is_signed); - if (!fn) { - parse_error(ps, FILT_ERR_INVALID_OP, 0); + if (!fn) return -EINVAL; - } if (pred->op == OP_NE) pred->not = 1; - return filter_add_pred_fn(ps, call, pred, fn); + return filter_add_pred_fn(call, pred, fn); } -static int filter_add_subsystem_pred(struct filter_parse_state *ps, - struct event_subsystem *system, +static int filter_add_subsystem_pred(struct event_subsystem *system, struct filter_pred *pred, char *filter_string) { @@ -655,10 +562,8 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps, return -ENOMEM; } - if (filter->n_preds == MAX_FILTER_PRED) { - parse_error(ps, FILT_ERR_TOO_MANY_PREDS, 0); + if (filter->n_preds == MAX_FILTER_PRED) return -ENOSPC; - } filter->preds[filter->n_preds] = pred; filter->n_preds++; @@ -676,7 +581,7 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps, if (call_filter->need_reset) continue; - err = filter_add_pred(ps, call, pred); + err = filter_add_pred(call, pred); if (err) call_filter->need_reset = true; } @@ -906,10 +811,8 @@ static int filter_parse(struct filter_parse_state *ps) if (is_op_char(ps, ch)) { op = infix_get_op(ps, ch); - if (op == OP_NONE) { - parse_error(ps, FILT_ERR_INVALID_OP, 0); + if (op == OP_NONE) return -EINVAL; - } if (strlen(curr_operand(ps))) { postfix_append_operand(ps, curr_operand(ps)); @@ -948,17 +851,13 @@ static int filter_parse(struct filter_parse_state *ps) postfix_append_op(ps, top_op); top_op = filter_opstack_pop(ps); } - if (top_op == OP_NONE) { - parse_error(ps, FILT_ERR_UNBALANCED_PAREN, 0); + if (top_op == OP_NONE) return -EINVAL; - } continue; } parse_operand: - if (append_operand_char(ps, ch)) { - parse_error(ps, FILT_ERR_OPERAND_TOO_LONG, 0); + if (append_operand_char(ps, ch)) return -EINVAL; - } } if (strlen(curr_operand(ps))) @@ -968,10 +867,8 @@ parse_operand: top_op = filter_opstack_pop(ps); if (top_op == OP_NONE) break; - if (top_op == OP_OPEN_PAREN) { - parse_error(ps, FILT_ERR_UNBALANCED_PAREN, 0); + if (top_op == OP_OPEN_PAREN) return -EINVAL; - } postfix_append_op(ps, top_op); } @@ -1029,10 +926,8 @@ static int check_preds(struct filter_parse_state *ps) n_normal_preds++; } - if (!n_normal_preds || n_logical_preds >= n_normal_preds) { - parse_error(ps, FILT_ERR_INVALID_FILTER, 0); + if (!n_normal_preds || n_logical_preds >= n_normal_preds) return -EINVAL; - } return 0; } @@ -1057,21 +952,19 @@ static int replace_preds(struct event_subsystem *system, operand1 = elt->operand; else if (!operand2) operand2 = elt->operand; - else { - parse_error(ps, FILT_ERR_TOO_MANY_OPERANDS, 0); + else return -EINVAL; - } continue; } if (elt->op == OP_AND || elt->op == OP_OR) { pred = create_logical_pred(elt->op); if (call) { - err = filter_add_pred(ps, call, pred); + err = filter_add_pred(call, pred); filter_free_pred(pred); } else - err = filter_add_subsystem_pred(ps, system, - pred, filter_string); + err = filter_add_subsystem_pred(system, pred, + filter_string); if (err) return err; @@ -1079,17 +972,15 @@ static int replace_preds(struct event_subsystem *system, continue; } - if (!operand1 || !operand2) { - parse_error(ps, FILT_ERR_MISSING_FIELD, 0); + if (!operand1 || !operand2) return -EINVAL; - } pred = create_pred(elt->op, operand1, operand2); if (call) { - err = filter_add_pred(ps, call, pred); + err = filter_add_pred(call, pred); filter_free_pred(pred); } else - err = filter_add_subsystem_pred(ps, system, pred, + err = filter_add_subsystem_pred(system, pred, filter_string); if (err) return err; @@ -1121,15 +1012,10 @@ int __apply_event_filter(struct ftrace_event_call *call, char *filter_string) parse_init(ps, filter_ops, filter_string); err = filter_parse(ps); - if (err) { - append_filter_err(ps, call->filter); + if (err) goto out; - } err = replace_preds(NULL, call, ps, filter_string); - if (err) - append_filter_err(ps, call->filter); - out: filter_opstack_clear(ps); postfix_clear(ps); -- 1.5.4.rc3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] tracing/filters: remove error messages 2009-06-17 8:47 ` [PATCH 2/2] tracing/filters: remove error messages Li Zefan @ 2009-06-17 12:02 ` Frederic Weisbecker 2009-06-18 1:17 ` Li Zefan 0 siblings, 1 reply; 10+ messages in thread From: Frederic Weisbecker @ 2009-06-17 12:02 UTC (permalink / raw) To: Li Zefan; +Cc: Steven Rostedt, Ingo Molnar, Tom Zanussi, LKML On Wed, Jun 17, 2009 at 04:47:15PM +0800, Li Zefan wrote: > Now we restore original filter is the new one can't be applied, > and no long show error messages, so we can remove them totally. Why? These messages are very powerful to point a user to its mistakes in filters syntaxes or semantics. I really think we are removing a very useful feature in this patch. May be we can keep the previous filter in case of new filter string inserting failure, though if the user wanted to insert a new one, there are few chances that the previous one is still relevant for him. I don't know. > Another reason is, I don't think it's good to show error messages > when reading a control file. So, why not create a filter_error file in this case? One for each event and subsys that would print the last error? Frederic. > > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com> > --- > kernel/trace/trace_events_filter.c | 168 ++++++------------------------------ > 1 files changed, 27 insertions(+), 141 deletions(-) > > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c > index 2f706e5..cd7a928 100644 > --- a/kernel/trace/trace_events_filter.c > +++ b/kernel/trace/trace_events_filter.c > @@ -60,36 +60,6 @@ static struct filter_op filter_ops[] = { > { OP_OPEN_PAREN, "(", 0 }, > }; > > -enum { > - FILT_ERR_NONE, > - FILT_ERR_INVALID_OP, > - FILT_ERR_UNBALANCED_PAREN, > - FILT_ERR_TOO_MANY_OPERANDS, > - FILT_ERR_OPERAND_TOO_LONG, > - FILT_ERR_FIELD_NOT_FOUND, > - FILT_ERR_ILLEGAL_FIELD_OP, > - FILT_ERR_ILLEGAL_INTVAL, > - FILT_ERR_BAD_SUBSYS_FILTER, > - FILT_ERR_TOO_MANY_PREDS, > - FILT_ERR_MISSING_FIELD, > - FILT_ERR_INVALID_FILTER, > -}; > - > -static char *err_text[] = { > - "No error", > - "Invalid operator", > - "Unbalanced parens", > - "Too many operands", > - "Operand too long", > - "Field not found", > - "Illegal operation for field type", > - "Illegal integer value", > - "Couldn't find or set field in one of a subsystem's events", > - "Too many terms in predicate expression", > - "Missing field name and/or value", > - "Meaningless filter expression", > -}; > - > struct opstack_op { > int op; > struct list_head list; > @@ -105,8 +75,6 @@ struct filter_parse_state { > struct filter_op *ops; > struct list_head opstack; > struct list_head postfix; > - int lasterr; > - int lasterr_pos; > > struct { > char *string; > @@ -223,12 +191,6 @@ int filter_match_preds(struct ftrace_event_call *call, void *rec) > } > EXPORT_SYMBOL_GPL(filter_match_preds); > > -static void parse_error(struct filter_parse_state *ps, int err, int pos) > -{ > - ps->lasterr = err; > - ps->lasterr_pos = pos; > -} > - > static void remove_filter_string(struct event_filter *filter) > { > kfree(filter->filter_string); > @@ -269,48 +231,6 @@ static void free_old_filter(struct event_filter *filter) > filter->old_filter = NULL; > } > > -static int append_filter_string(struct event_filter *filter, > - char *string) > -{ > - int newlen; > - char *new_filter_string; > - > - BUG_ON(!filter->filter_string); > - newlen = strlen(filter->filter_string) + strlen(string) + 1; > - new_filter_string = kmalloc(newlen, GFP_KERNEL); > - if (!new_filter_string) > - return -ENOMEM; > - > - strcpy(new_filter_string, filter->filter_string); > - strcat(new_filter_string, string); > - kfree(filter->filter_string); > - filter->filter_string = new_filter_string; > - > - return 0; > -} > - > -static void append_filter_err(struct filter_parse_state *ps, > - struct event_filter *filter) > -{ > - int pos = ps->lasterr_pos; > - char *buf, *pbuf; > - > - buf = (char *)__get_free_page(GFP_TEMPORARY); > - if (!buf) > - return; > - > - append_filter_string(filter, "\n"); > - memset(buf, ' ', PAGE_SIZE); > - if (pos > PAGE_SIZE - 128) > - pos = 0; > - buf[pos] = '^'; > - pbuf = &buf[pos] + 1; > - > - sprintf(pbuf, "\nparse_error: %s\n", err_text[ps->lasterr]); > - append_filter_string(filter, buf); > - free_page((unsigned long) buf); > -} > - > void print_event_filter(struct ftrace_event_call *call, struct trace_seq *s) > { > struct event_filter *filter = call->filter; > @@ -478,18 +398,15 @@ static void filter_free_subsystem_preds(struct event_subsystem *system, > } > } > > -static int filter_add_pred_fn(struct filter_parse_state *ps, > - struct ftrace_event_call *call, > +static int filter_add_pred_fn(struct ftrace_event_call *call, > struct filter_pred *pred, > filter_pred_fn_t fn) > { > struct event_filter *filter = call->filter; > int idx, err; > > - if (filter->n_preds == MAX_FILTER_PRED) { > - parse_error(ps, FILT_ERR_TOO_MANY_PREDS, 0); > + if (filter->n_preds == MAX_FILTER_PRED) > return -ENOSPC; > - } > > idx = filter->n_preds; > filter_clear_pred(filter->preds[idx]); > @@ -570,8 +487,7 @@ static filter_pred_fn_t select_comparison_fn(int op, int field_size, > return fn; > } > > -static int filter_add_pred(struct filter_parse_state *ps, > - struct ftrace_event_call *call, > +static int filter_add_pred(struct ftrace_event_call *call, > struct filter_pred *pred) > { > struct ftrace_event_field *field; > @@ -584,24 +500,20 @@ static int filter_add_pred(struct filter_parse_state *ps, > > if (pred->op == OP_AND) { > pred->pop_n = 2; > - return filter_add_pred_fn(ps, call, pred, filter_pred_and); > + return filter_add_pred_fn(call, pred, filter_pred_and); > } else if (pred->op == OP_OR) { > pred->pop_n = 2; > - return filter_add_pred_fn(ps, call, pred, filter_pred_or); > + return filter_add_pred_fn(call, pred, filter_pred_or); > } > > field = find_event_field(call, pred->field_name); > - if (!field) { > - parse_error(ps, FILT_ERR_FIELD_NOT_FOUND, 0); > + if (!field) > return -EINVAL; > - } > > pred->offset = field->offset; > > - if (!is_legal_op(field, pred->op)) { > - parse_error(ps, FILT_ERR_ILLEGAL_FIELD_OP, 0); > + if (!is_legal_op(field, pred->op)) > return -EINVAL; > - } > > string_type = is_string_field(field->type); > if (string_type) { > @@ -612,33 +524,28 @@ static int filter_add_pred(struct filter_parse_state *ps, > pred->str_len = field->size; > if (pred->op == OP_NE) > pred->not = 1; > - return filter_add_pred_fn(ps, call, pred, fn); > + return filter_add_pred_fn(call, pred, fn); > } else { > if (field->is_signed) > ret = strict_strtoll(pred->str_val, 0, &val); > else > ret = strict_strtoull(pred->str_val, 0, &val); > - if (ret) { > - parse_error(ps, FILT_ERR_ILLEGAL_INTVAL, 0); > + if (ret) > return -EINVAL; > - } > pred->val = val; > } > > fn = select_comparison_fn(pred->op, field->size, field->is_signed); > - if (!fn) { > - parse_error(ps, FILT_ERR_INVALID_OP, 0); > + if (!fn) > return -EINVAL; > - } > > if (pred->op == OP_NE) > pred->not = 1; > > - return filter_add_pred_fn(ps, call, pred, fn); > + return filter_add_pred_fn(call, pred, fn); > } > > -static int filter_add_subsystem_pred(struct filter_parse_state *ps, > - struct event_subsystem *system, > +static int filter_add_subsystem_pred(struct event_subsystem *system, > struct filter_pred *pred, > char *filter_string) > { > @@ -655,10 +562,8 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps, > return -ENOMEM; > } > > - if (filter->n_preds == MAX_FILTER_PRED) { > - parse_error(ps, FILT_ERR_TOO_MANY_PREDS, 0); > + if (filter->n_preds == MAX_FILTER_PRED) > return -ENOSPC; > - } > > filter->preds[filter->n_preds] = pred; > filter->n_preds++; > @@ -676,7 +581,7 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps, > if (call_filter->need_reset) > continue; > > - err = filter_add_pred(ps, call, pred); > + err = filter_add_pred(call, pred); > if (err) > call_filter->need_reset = true; > } > @@ -906,10 +811,8 @@ static int filter_parse(struct filter_parse_state *ps) > > if (is_op_char(ps, ch)) { > op = infix_get_op(ps, ch); > - if (op == OP_NONE) { > - parse_error(ps, FILT_ERR_INVALID_OP, 0); > + if (op == OP_NONE) > return -EINVAL; > - } > > if (strlen(curr_operand(ps))) { > postfix_append_operand(ps, curr_operand(ps)); > @@ -948,17 +851,13 @@ static int filter_parse(struct filter_parse_state *ps) > postfix_append_op(ps, top_op); > top_op = filter_opstack_pop(ps); > } > - if (top_op == OP_NONE) { > - parse_error(ps, FILT_ERR_UNBALANCED_PAREN, 0); > + if (top_op == OP_NONE) > return -EINVAL; > - } > continue; > } > parse_operand: > - if (append_operand_char(ps, ch)) { > - parse_error(ps, FILT_ERR_OPERAND_TOO_LONG, 0); > + if (append_operand_char(ps, ch)) > return -EINVAL; > - } > } > > if (strlen(curr_operand(ps))) > @@ -968,10 +867,8 @@ parse_operand: > top_op = filter_opstack_pop(ps); > if (top_op == OP_NONE) > break; > - if (top_op == OP_OPEN_PAREN) { > - parse_error(ps, FILT_ERR_UNBALANCED_PAREN, 0); > + if (top_op == OP_OPEN_PAREN) > return -EINVAL; > - } > postfix_append_op(ps, top_op); > } > > @@ -1029,10 +926,8 @@ static int check_preds(struct filter_parse_state *ps) > n_normal_preds++; > } > > - if (!n_normal_preds || n_logical_preds >= n_normal_preds) { > - parse_error(ps, FILT_ERR_INVALID_FILTER, 0); > + if (!n_normal_preds || n_logical_preds >= n_normal_preds) > return -EINVAL; > - } > > return 0; > } > @@ -1057,21 +952,19 @@ static int replace_preds(struct event_subsystem *system, > operand1 = elt->operand; > else if (!operand2) > operand2 = elt->operand; > - else { > - parse_error(ps, FILT_ERR_TOO_MANY_OPERANDS, 0); > + else > return -EINVAL; > - } > continue; > } > > if (elt->op == OP_AND || elt->op == OP_OR) { > pred = create_logical_pred(elt->op); > if (call) { > - err = filter_add_pred(ps, call, pred); > + err = filter_add_pred(call, pred); > filter_free_pred(pred); > } else > - err = filter_add_subsystem_pred(ps, system, > - pred, filter_string); > + err = filter_add_subsystem_pred(system, pred, > + filter_string); > if (err) > return err; > > @@ -1079,17 +972,15 @@ static int replace_preds(struct event_subsystem *system, > continue; > } > > - if (!operand1 || !operand2) { > - parse_error(ps, FILT_ERR_MISSING_FIELD, 0); > + if (!operand1 || !operand2) > return -EINVAL; > - } > > pred = create_pred(elt->op, operand1, operand2); > if (call) { > - err = filter_add_pred(ps, call, pred); > + err = filter_add_pred(call, pred); > filter_free_pred(pred); > } else > - err = filter_add_subsystem_pred(ps, system, pred, > + err = filter_add_subsystem_pred(system, pred, > filter_string); > if (err) > return err; > @@ -1121,15 +1012,10 @@ int __apply_event_filter(struct ftrace_event_call *call, char *filter_string) > > parse_init(ps, filter_ops, filter_string); > err = filter_parse(ps); > - if (err) { > - append_filter_err(ps, call->filter); > + if (err) > goto out; > - } > > err = replace_preds(NULL, call, ps, filter_string); > - if (err) > - append_filter_err(ps, call->filter); > - > out: > filter_opstack_clear(ps); > postfix_clear(ps); > -- 1.5.4.rc3 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] tracing/filters: remove error messages 2009-06-17 12:02 ` Frederic Weisbecker @ 2009-06-18 1:17 ` Li Zefan 2009-06-18 6:10 ` Tom Zanussi 0 siblings, 1 reply; 10+ messages in thread From: Li Zefan @ 2009-06-18 1:17 UTC (permalink / raw) To: Frederic Weisbecker; +Cc: Steven Rostedt, Ingo Molnar, Tom Zanussi, LKML Frederic Weisbecker wrote: > On Wed, Jun 17, 2009 at 04:47:15PM +0800, Li Zefan wrote: >> Now we restore original filter is the new one can't be applied, >> and no long show error messages, so we can remove them totally. > > Why? > These messages are very powerful to point a user to its mistakes > in filters syntaxes or semantics. > > I really think we are removing a very useful feature in this patch. > I think it's better done by providing a user-space program/script. So what's the criterion to decide what should be in kernel and what should be in user-space? btw, this feature is not full-fledged, that it can't point to the exact position where error occured, and the implementation will add complexity and I'm sure it's worthy or not. > May be we can keep the previous filter in case of new filter string > inserting failure, though if the user wanted to insert a new one, there > are few chances that the previous one is still relevant for him. > I don't know. > >> Another reason is, I don't think it's good to show error messages >> when reading a control file. > > So, why not create a filter_error file in this case? One for each > event and subsys that would print the last error? > Yeah, if we do want to keep this feature in the kernel. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] tracing/filters: remove error messages 2009-06-18 1:17 ` Li Zefan @ 2009-06-18 6:10 ` Tom Zanussi 2009-06-19 5:03 ` Li Zefan 0 siblings, 1 reply; 10+ messages in thread From: Tom Zanussi @ 2009-06-18 6:10 UTC (permalink / raw) To: Li Zefan; +Cc: Frederic Weisbecker, Steven Rostedt, Ingo Molnar, LKML On Thu, 2009-06-18 at 09:17 +0800, Li Zefan wrote: > Frederic Weisbecker wrote: > > On Wed, Jun 17, 2009 at 04:47:15PM +0800, Li Zefan wrote: > >> Now we restore original filter is the new one can't be applied, > >> and no long show error messages, so we can remove them totally. > > > > Why? > > These messages are very powerful to point a user to its mistakes > > in filters syntaxes or semantics. > > > > I really think we are removing a very useful feature in this patch. > > > > I think it's better done by providing a user-space program/script. > > So what's the criterion to decide what should be in kernel and > what should be in user-space? > I thought there wasn't supposed to be any userspace for non-binary tracing. If there is, then yeah, you can get rid of the error messages in the kernel because your userspace program can arrange to never submit an erroneous filter. > btw, this feature is not full-fledged, that it can't point to the > exact position where error occured, and the implementation will add > complexity and I'm sure it's worthy or not. > It could, it just doesn't yet. But even without exact position information, the messages should still be useful in most cases. Tom > > May be we can keep the previous filter in case of new filter string > > inserting failure, though if the user wanted to insert a new one, there > > are few chances that the previous one is still relevant for him. > > I don't know. > > > >> Another reason is, I don't think it's good to show error messages > >> when reading a control file. > > > > So, why not create a filter_error file in this case? One for each > > event and subsys that would print the last error? > > > > Yeah, if we do want to keep this feature in the kernel. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] tracing/filters: remove error messages 2009-06-18 6:10 ` Tom Zanussi @ 2009-06-19 5:03 ` Li Zefan 0 siblings, 0 replies; 10+ messages in thread From: Li Zefan @ 2009-06-19 5:03 UTC (permalink / raw) To: Tom Zanussi; +Cc: Frederic Weisbecker, Steven Rostedt, Ingo Molnar, LKML Tom Zanussi wrote: > On Thu, 2009-06-18 at 09:17 +0800, Li Zefan wrote: >> Frederic Weisbecker wrote: >>> On Wed, Jun 17, 2009 at 04:47:15PM +0800, Li Zefan wrote: >>>> Now we restore original filter is the new one can't be applied, >>>> and no long show error messages, so we can remove them totally. >>> Why? >>> These messages are very powerful to point a user to its mistakes >>> in filters syntaxes or semantics. >>> >>> I really think we are removing a very useful feature in this patch. >>> >> I think it's better done by providing a user-space program/script. >> >> So what's the criterion to decide what should be in kernel and >> what should be in user-space? >> > > I thought there wasn't supposed to be any userspace for non-binary > tracing. This is a good point, and I agree on this direction. > If there is, then yeah, you can get rid of the error messages > in the kernel because your userspace program can arrange to never submit > an erroneous filter. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable 2009-06-17 8:46 [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable Li Zefan 2009-06-17 8:47 ` [PATCH 2/2] tracing/filters: remove error messages Li Zefan @ 2009-06-18 5:58 ` Tom Zanussi 2009-06-18 23:18 ` Steven Rostedt 1 sibling, 1 reply; 10+ messages in thread From: Tom Zanussi @ 2009-06-18 5:58 UTC (permalink / raw) To: Li Zefan; +Cc: Steven Rostedt, Ingo Molnar, Frederic Weisbecker, LKML Hi, On Wed, 2009-06-17 at 16:46 +0800, Li Zefan wrote: > | commit 7ce7e4249921d5073e764f7ff7ad83cfa9894bd7 > | Author: Tom Zanussi <tzanussi@gmail.com> > | Date: Sun Mar 22 03:31:04 2009 -0500 > | > | tracing: add per-event filtering > | ... > | > | Filters can also be set or cleared for a complete subsystem by writing > | the same filter as would be written to an individual event to the > | filter file at the root of the subsytem. Note however, that if any > | event in the subsystem lacks a field specified in the filter being > | set, the set will fail and all filters in the subsytem are > | automatically cleared. This change from the previous version was made > | because using only the fields that happen to exist for a given event > | would most likely result in a meaningless filter. > > I really don't like this change. It is inconvenient, and makes subsystem > filter much less useful: > > # echo 'vec == 1' > irq/softirq_entry/filter > # echo 'irq == 5' > irq/filter > bash: echo: write error: Invalid argument > # cat irq/softirq_entry/filter > none > > I'd expect this will set the filter for irq_handler_entry and > irq_handler_exit, and won't touch softirq_entry and softirq_exit. > > But it just failed, and what's worse, each event's filter was > cleared. > The idea behind the change was that after setting a subsystem filter, you'd be guaranteed that all or none of the events in the subsystem would have the same filter at that point, and not some mix of different filters depending on which ones failed or not, which to me seemed nonintuitive. If I set a filter like "vec == 1 && irq == 5", which really has no overall meaning, I wouldn't expect softirq_entry to get "vec == 1" and irq_handler_entry to get "irq == 5" - I'd rather get an error, but that's just me. So if it makes more sense to users to have subsystem filters propagate to whichever events will take them, this patch would be fine with me. Tom > The basic idea of this patch is, save the filter before applying > the new one, and fall back to it if the new one can't be applied: > > # echo 'vec == 1' > softirq_entry/filter > # echo 'irq == 5' > filter > # cat irq_handler_entry/filter > irq == 5 > # cat softirq_entry/filter > vec == 1 > > [ Impact: allow subsystem filter be applied to parts of the subsys ] > > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com> > --- > kernel/trace/trace.h | 2 + > kernel/trace/trace_events_filter.c | 138 +++++++++++++++++++++++++++++------- > 2 files changed, 114 insertions(+), 26 deletions(-) > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > index 7d5cc37..b5153e9 100644 > --- a/kernel/trace/trace.h > +++ b/kernel/trace/trace.h > @@ -763,6 +763,8 @@ struct event_filter { > int n_preds; > struct filter_pred **preds; > char *filter_string; > + char *old_filter; > + bool need_reset; > }; > > struct event_subsystem { > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c > index 936c621..2f706e5 100644 > --- a/kernel/trace/trace_events_filter.c > +++ b/kernel/trace/trace_events_filter.c > @@ -246,6 +246,29 @@ static int replace_filter_string(struct event_filter *filter, > return 0; > } > > +static int save_old_filter(struct event_filter *filter) > +{ > + char *old = filter->old_filter; > + > + kfree(old); > + > + if (filter->filter_string) > + old = kstrdup(filter->filter_string, GFP_KERNEL); > + else > + old = kstrdup("0", GFP_KERNEL); > + filter->old_filter = old; > + > + if (!old) > + return -ENOMEM; > + return 0; > +} > + > +static void free_old_filter(struct event_filter *filter) > +{ > + kfree(filter->old_filter); > + filter->old_filter = NULL; > +} > + > static int append_filter_string(struct event_filter *filter, > char *string) > { > @@ -374,6 +397,8 @@ void destroy_preds(struct ftrace_event_call *call) > struct event_filter *filter = call->filter; > int i; > > + WARN_ON(filter->old_filter); > + > for (i = 0; i < MAX_FILTER_PRED; i++) { > if (filter->preds[i]) > filter_free_pred(filter->preds[i]); > @@ -418,10 +443,9 @@ oom: > } > EXPORT_SYMBOL_GPL(init_preds); > > -static void filter_free_subsystem_preds(struct event_subsystem *system) > +static void __filter_free_subsystem_preds(struct event_subsystem *system) > { > struct event_filter *filter = system->filter; > - struct ftrace_event_call *call; > int i; > > if (filter->n_preds) { > @@ -431,11 +455,22 @@ static void filter_free_subsystem_preds(struct event_subsystem *system) > filter->preds = NULL; > filter->n_preds = 0; > } > +} > + > +static void filter_free_subsystem_preds(struct event_subsystem *system, > + bool save_old) > +{ > + struct ftrace_event_call *call; > + > + __filter_free_subsystem_preds(system); > > list_for_each_entry(call, &ftrace_events, list) { > if (!call->define_fields) > continue; > > + if (save_old) > + save_old_filter(call->filter); > + > if (!strcmp(call->system, system->name)) { > filter_disable_preds(call); > remove_filter_string(call->filter); > @@ -608,8 +643,9 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps, > char *filter_string) > { > struct event_filter *filter = system->filter; > + struct event_filter *call_filter; > struct ftrace_event_call *call; > - int err = 0; > + int err; > > if (!filter->preds) { > filter->preds = kzalloc(MAX_FILTER_PRED * sizeof(pred), > @@ -629,22 +665,23 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps, > > list_for_each_entry(call, &ftrace_events, list) { > > + call_filter = call->filter; > + > if (!call->define_fields) > continue; > > if (strcmp(call->system, system->name)) > continue; > > + if (call_filter->need_reset) > + continue; > + > err = filter_add_pred(ps, call, pred); > - if (err) { > - filter_free_subsystem_preds(system); > - parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0); > - goto out; > - } > - replace_filter_string(call->filter, filter_string); > + if (err) > + call_filter->need_reset = true; > } > -out: > - return err; > + > + return 0; > } > > static void parse_init(struct filter_parse_state *ps, > @@ -1063,25 +1100,21 @@ static int replace_preds(struct event_subsystem *system, > return 0; > } > > -int apply_event_filter(struct ftrace_event_call *call, char *filter_string) > +int __apply_event_filter(struct ftrace_event_call *call, char *filter_string) > { > int err; > > struct filter_parse_state *ps; > > - mutex_lock(&event_mutex); > - > if (!strcmp(strstrip(filter_string), "0")) { > filter_disable_preds(call); > remove_filter_string(call->filter); > - mutex_unlock(&event_mutex); > return 0; > } > > - err = -ENOMEM; > ps = kzalloc(sizeof(*ps), GFP_KERNEL); > if (!ps) > - goto out_unlock; > + return -ENOMEM; > > filter_disable_preds(call); > replace_filter_string(call->filter, filter_string); > @@ -1101,23 +1134,52 @@ out: > filter_opstack_clear(ps); > postfix_clear(ps); > kfree(ps); > -out_unlock: > - mutex_unlock(&event_mutex); > > return err; > } > > +void __apply_event_old_filter(struct ftrace_event_call *call) > +{ > + struct event_filter *filter = call->filter; > + int ret; > + > + ret = __apply_event_filter(call, filter->old_filter); > + if (ret) { > + ret = __apply_event_filter(call, "0"); > + WARN_ON(ret); > + } > +} > + > +int apply_event_filter(struct ftrace_event_call *call, char *filter_string) > +{ > + int ret; > + > + ret = save_old_filter(call->filter); > + if (ret) > + return ret; > + > + mutex_lock(&event_mutex); > + ret = __apply_event_filter(call, filter_string); > + if (ret) > + __apply_event_old_filter(call); > + mutex_unlock(&event_mutex); > + > + free_old_filter(call->filter); > + return ret; > +} > + > int apply_subsystem_event_filter(struct event_subsystem *system, > char *filter_string) > { > - int err; > - > + struct event_filter *filter; > + struct ftrace_event_call *call; > struct filter_parse_state *ps; > + int err; > > mutex_lock(&event_mutex); > > if (!strcmp(strstrip(filter_string), "0")) { > - filter_free_subsystem_preds(system); > + filter_free_subsystem_preds(system, false); > remove_filter_string(system->filter); > mutex_unlock(&event_mutex); > return 0; > @@ -1128,24 +1190,48 @@ int apply_subsystem_event_filter(struct event_subsystem *system, > if (!ps) > goto out_unlock; > > - filter_free_subsystem_preds(system); > - replace_filter_string(system->filter, filter_string); > + filter = system->filter; > + filter_free_subsystem_preds(system, true); > + save_old_filter(filter); > + replace_filter_string(filter, filter_string); > > parse_init(ps, filter_ops, filter_string); > err = filter_parse(ps); > if (err) { > - append_filter_err(ps, system->filter); > + replace_filter_string(filter, filter->old_filter); > goto out; > } > > err = replace_preds(system, NULL, ps, filter_string); > if (err) > - append_filter_err(ps, system->filter); > + replace_filter_string(filter, filter->old_filter); > + > + /* > + * For those events that the filter can't be applied to, > + * reset them with their original filters. > + */ > + list_for_each_entry(call, &ftrace_events, list) { > + if (!call->define_fields) > + continue; > + > + if (strcmp(call->system, system->name) != 0) > + continue; > + > + filter = call->filter; > + if (filter->need_reset || err) { > + filter->need_reset = false; > + __apply_event_old_filter(call); > + } else > + replace_filter_string(filter, filter_string); > + > + free_old_filter(filter); > + } > > out: > filter_opstack_clear(ps); > postfix_clear(ps); > kfree(ps); > + free_old_filter(system->filter); > out_unlock: > mutex_unlock(&event_mutex); > > -- 1.5.4.rc3 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable 2009-06-18 5:58 ` [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable Tom Zanussi @ 2009-06-18 23:18 ` Steven Rostedt 2009-06-19 4:46 ` Tom Zanussi 0 siblings, 1 reply; 10+ messages in thread From: Steven Rostedt @ 2009-06-18 23:18 UTC (permalink / raw) To: Tom Zanussi; +Cc: Li Zefan, Ingo Molnar, Frederic Weisbecker, LKML On Thu, 18 Jun 2009, Tom Zanussi wrote: > Hi, > > On Wed, 2009-06-17 at 16:46 +0800, Li Zefan wrote: > > | commit 7ce7e4249921d5073e764f7ff7ad83cfa9894bd7 > > | Author: Tom Zanussi <tzanussi@gmail.com> > > | Date: Sun Mar 22 03:31:04 2009 -0500 > > | > > | tracing: add per-event filtering > > | ... > > | > > | Filters can also be set or cleared for a complete subsystem by writing > > | the same filter as would be written to an individual event to the > > | filter file at the root of the subsytem. Note however, that if any > > | event in the subsystem lacks a field specified in the filter being > > | set, the set will fail and all filters in the subsytem are > > | automatically cleared. This change from the previous version was made > > | because using only the fields that happen to exist for a given event > > | would most likely result in a meaningless filter. > > > > I really don't like this change. It is inconvenient, and makes subsystem > > filter much less useful: > > > > # echo 'vec == 1' > irq/softirq_entry/filter > > # echo 'irq == 5' > irq/filter > > bash: echo: write error: Invalid argument > > # cat irq/softirq_entry/filter > > none > > > > I'd expect this will set the filter for irq_handler_entry and > > irq_handler_exit, and won't touch softirq_entry and softirq_exit. > > > > But it just failed, and what's worse, each event's filter was > > cleared. > > > > The idea behind the change was that after setting a subsystem filter, > you'd be guaranteed that all or none of the events in the subsystem > would have the same filter at that point, and not some mix of different > filters depending on which ones failed or not, which to me seemed > nonintuitive. > > If I set a filter like "vec == 1 && irq == 5", which really has no > overall meaning, I wouldn't expect softirq_entry to get "vec == 1" and > irq_handler_entry to get "irq == 5" - I'd rather get an error, but > that's just me. > > So if it makes more sense to users to have subsystem filters propagate > to whichever events will take them, this patch would be fine with me. > I disagree. If a subsystem filter is set, it should be valid for all filters underneath, if it is not, it should error. But Li has a point, if you get an error, it should not reset all filters underneath. That is, if the irq/filter setting took an error, then irq/softirq_entry/filter should still stay the same. Perhaps you need to run through it twice. See if the setting of a filter is valid for all filters underneath, if it is not, then fail. If it is, then reset all of them, and assign the filter. -- Steve ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable 2009-06-18 23:18 ` Steven Rostedt @ 2009-06-19 4:46 ` Tom Zanussi 2009-06-19 5:02 ` Li Zefan 0 siblings, 1 reply; 10+ messages in thread From: Tom Zanussi @ 2009-06-19 4:46 UTC (permalink / raw) To: Steven Rostedt; +Cc: Li Zefan, Ingo Molnar, Frederic Weisbecker, LKML On Thu, 2009-06-18 at 19:18 -0400, Steven Rostedt wrote: > > > On Thu, 18 Jun 2009, Tom Zanussi wrote: > > > Hi, > > > > On Wed, 2009-06-17 at 16:46 +0800, Li Zefan wrote: > > > | commit 7ce7e4249921d5073e764f7ff7ad83cfa9894bd7 > > > | Author: Tom Zanussi <tzanussi@gmail.com> > > > | Date: Sun Mar 22 03:31:04 2009 -0500 > > > | > > > | tracing: add per-event filtering > > > | ... > > > | > > > | Filters can also be set or cleared for a complete subsystem by writing > > > | the same filter as would be written to an individual event to the > > > | filter file at the root of the subsytem. Note however, that if any > > > | event in the subsystem lacks a field specified in the filter being > > > | set, the set will fail and all filters in the subsytem are > > > | automatically cleared. This change from the previous version was made > > > | because using only the fields that happen to exist for a given event > > > | would most likely result in a meaningless filter. > > > > > > I really don't like this change. It is inconvenient, and makes subsystem > > > filter much less useful: > > > > > > # echo 'vec == 1' > irq/softirq_entry/filter > > > # echo 'irq == 5' > irq/filter > > > bash: echo: write error: Invalid argument > > > # cat irq/softirq_entry/filter > > > none > > > > > > I'd expect this will set the filter for irq_handler_entry and > > > irq_handler_exit, and won't touch softirq_entry and softirq_exit. > > > > > > But it just failed, and what's worse, each event's filter was > > > cleared. > > > > > > > The idea behind the change was that after setting a subsystem filter, > > you'd be guaranteed that all or none of the events in the subsystem > > would have the same filter at that point, and not some mix of different > > filters depending on which ones failed or not, which to me seemed > > nonintuitive. > > > > If I set a filter like "vec == 1 && irq == 5", which really has no > > overall meaning, I wouldn't expect softirq_entry to get "vec == 1" and > > irq_handler_entry to get "irq == 5" - I'd rather get an error, but > > that's just me. > > > > So if it makes more sense to users to have subsystem filters propagate > > to whichever events will take them, this patch would be fine with me. > > > > I disagree. If a subsystem filter is set, it should be valid for all > filters underneath, if it is not, it should error. > > But Li has a point, if you get an error, it should not reset all filters > underneath. That is, if the irq/filter setting took an error, then > irq/softirq_entry/filter should still stay the same. > > Perhaps you need to run through it twice. See if the setting of a filter > is valid for all filters underneath, if it is not, then fail. If it is, > then reset all of them, and assign the filter. > Yeah, I agree that this is better than just clearing them all on an error, but it still means that a subsystem filter will succeed only when it names common_ (or commonly named) fields. I think what Li is saying is that that restriction makes the subsystem filters less useful, and you should for convenience' sake be allowed to propagate a filter to a subset and ignore the ones that don't make sense. Note that there's a danger in this case that a filter might be applied but not really make sense e.g. two events might have a 'vec' field that mean completely different things but the filter would be applied to both just because they have the same name. The only way to ensure that they would always make sense would be to restrict the subsystem filter to just the common_ fields. But that's less useful, and maybe it would be better to leave the choice up the user... Tom > -- Steve > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable 2009-06-19 4:46 ` Tom Zanussi @ 2009-06-19 5:02 ` Li Zefan 0 siblings, 0 replies; 10+ messages in thread From: Li Zefan @ 2009-06-19 5:02 UTC (permalink / raw) To: Tom Zanussi; +Cc: Steven Rostedt, Ingo Molnar, Frederic Weisbecker, LKML Tom Zanussi wrote: > On Thu, 2009-06-18 at 19:18 -0400, Steven Rostedt wrote: >> >> On Thu, 18 Jun 2009, Tom Zanussi wrote: >> >>> Hi, >>> >>> On Wed, 2009-06-17 at 16:46 +0800, Li Zefan wrote: >>>> | commit 7ce7e4249921d5073e764f7ff7ad83cfa9894bd7 >>>> | Author: Tom Zanussi <tzanussi@gmail.com> >>>> | Date: Sun Mar 22 03:31:04 2009 -0500 >>>> | >>>> | tracing: add per-event filtering >>>> | ... >>>> | >>>> | Filters can also be set or cleared for a complete subsystem by writing >>>> | the same filter as would be written to an individual event to the >>>> | filter file at the root of the subsytem. Note however, that if any >>>> | event in the subsystem lacks a field specified in the filter being >>>> | set, the set will fail and all filters in the subsytem are >>>> | automatically cleared. This change from the previous version was made >>>> | because using only the fields that happen to exist for a given event >>>> | would most likely result in a meaningless filter. >>>> >>>> I really don't like this change. It is inconvenient, and makes subsystem >>>> filter much less useful: >>>> >>>> # echo 'vec == 1' > irq/softirq_entry/filter >>>> # echo 'irq == 5' > irq/filter >>>> bash: echo: write error: Invalid argument >>>> # cat irq/softirq_entry/filter >>>> none >>>> >>>> I'd expect this will set the filter for irq_handler_entry and >>>> irq_handler_exit, and won't touch softirq_entry and softirq_exit. >>>> >>>> But it just failed, and what's worse, each event's filter was >>>> cleared. >>>> >>> The idea behind the change was that after setting a subsystem filter, >>> you'd be guaranteed that all or none of the events in the subsystem >>> would have the same filter at that point, and not some mix of different >>> filters depending on which ones failed or not, which to me seemed >>> nonintuitive. >>> >>> If I set a filter like "vec == 1 && irq == 5", which really has no >>> overall meaning, I wouldn't expect softirq_entry to get "vec == 1" and >>> irq_handler_entry to get "irq == 5" - I'd rather get an error, but >>> that's just me. >>> >>> So if it makes more sense to users to have subsystem filters propagate >>> to whichever events will take them, this patch would be fine with me. >>> >> I disagree. If a subsystem filter is set, it should be valid for all >> filters underneath, if it is not, it should error. >> >> But Li has a point, if you get an error, it should not reset all filters >> underneath. That is, if the irq/filter setting took an error, then >> irq/softirq_entry/filter should still stay the same. >> >> Perhaps you need to run through it twice. See if the setting of a filter >> is valid for all filters underneath, if it is not, then fail. If it is, >> then reset all of them, and assign the filter. >> > > Yeah, I agree that this is better than just clearing them all on an > error, but it still means that a subsystem filter will succeed only when > it names common_ (or commonly named) fields. > > I think what Li is saying is that that restriction makes the subsystem > filters less useful, and you should for convenience' sake be allowed to > propagate a filter to a subset and ignore the ones that don't make > sense. > Yeah, being able to do this should be very useful: # echo 'irq == 1' > irq/filter # echo 'vec == 5' > irq/filter Otherwise I have to set each filter one by one. After setting subsystem/filter, one can change a single event's filter, and then the subsys/filter is not consistent with it's members' filter. So subsystem/filter is much more useful in writing but not reading. > Note that there's a danger in this case that a filter might be applied > but not really make sense e.g. two events might have a 'vec' field that > mean completely different things but the filter would be applied to both > just because they have the same name. The only way to ensure that they This should be rare. In a subsys, if 2 events have a field with the same name, they normally means the same thing. And even in this case, it's not that dangerous I think. ;) > would always make sense would be to restrict the subsystem filter to > just the common_ fields. > > But that's less useful, and maybe it would be better to leave the choice > up the user... > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2009-06-19 5:02 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-06-17 8:46 [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable Li Zefan 2009-06-17 8:47 ` [PATCH 2/2] tracing/filters: remove error messages Li Zefan 2009-06-17 12:02 ` Frederic Weisbecker 2009-06-18 1:17 ` Li Zefan 2009-06-18 6:10 ` Tom Zanussi 2009-06-19 5:03 ` Li Zefan 2009-06-18 5:58 ` [PATCH 1/2] tracing/filters: restore orignal filter when new filter isn't applicable Tom Zanussi 2009-06-18 23:18 ` Steven Rostedt 2009-06-19 4:46 ` Tom Zanussi 2009-06-19 5:02 ` Li Zefan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.