Re: [PATCH v2] tracing/filters: Improve subsystem filter

[Frederic Weisbecker]

On Mon, Jul 20, 2009 at 12:08:52PM -0400, Steven Rostedt wrote:
> 
> 
> On Mon, 20 Jul 2009, Li Zefan wrote:
> 
> > Currently a subsystem filter should be applicable to all events
> > under the subsystem, and if it failed, all the event filters
> > will be cleared. Those behaviors make subsys 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 it set the filter for irq_handler_entry/exit, and
> > not touch softirq_entry/exit.
> > 
> > The basic idea is, try to see if the filter can be applied
> > to which events, and then just apply to the those events:
> > 
> >   # echo 'vec == 1' > softirq_entry/filter
> >   # echo 'irq == 5' > filter
> >   # cat irq_handler_entry/filter
> >   irq == 5
> >   # cat softirq_entry/filter
> >   vec == 1
> > 
> > Changelog for v2:
> > - do some cleanups to address Frederic's comments.
> > 
> > Inspied-by: Steven Rostedt <srostedt@xxxxxxxxxx>
> > Signed-off-by: Li Zefan <lizf@xxxxxxxxxxxxxx>
> 
> Nice work Li!
> 
> I'll pull it in and give it some tests. I believe Frederic is traveling 
> today, but if it works well I'll get it ready to push. Unless Tom has any 
> rejections to it.



Not yet traveling but very soon :)

No problem for me anyway.

Acked-by: Frederic Weisbecker <fweisbec@xxxxxxxxx>

Thanks.

Frederic.


> Thanks!
> 
> -- Steve
> 
> 
> > ---
> >  include/linux/ftrace_event.h       |    4 +-
> >  kernel/trace/trace.h               |    3 +-
> >  kernel/trace/trace_events_filter.c |  124 ++++++++++++++++++++++++------------
> >  3 files changed, 87 insertions(+), 44 deletions(-)
> > 
> > diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> > index 5c093ff..26d3673 100644
> > --- a/include/linux/ftrace_event.h
> > +++ b/include/linux/ftrace_event.h
> > @@ -101,6 +101,8 @@ void trace_current_buffer_discard_commit(struct ring_buffer_event *event);
> >  
> >  void tracing_record_cmdline(struct task_struct *tsk);
> >  
> > +struct event_filter;
> > +
> >  struct ftrace_event_call {
> >  	struct list_head	list;
> >  	char			*name;
> > @@ -116,7 +118,7 @@ struct ftrace_event_call {
> >  	int			(*define_fields)(void);
> >  	struct list_head	fields;
> >  	int			filter_active;
> > -	void			*filter;
> > +	struct event_filter	*filter;
> >  	void			*mod;
> >  
> >  #ifdef CONFIG_EVENT_PROFILE
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 06886a0..3a87d46 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -768,13 +768,14 @@ struct event_filter {
> >  	int			n_preds;
> >  	struct filter_pred	**preds;
> >  	char			*filter_string;
> > +	bool			no_reset;
> >  };
> >  
> >  struct event_subsystem {
> >  	struct list_head	list;
> >  	const char		*name;
> >  	struct dentry		*entry;
> > -	void			*filter;
> > +	struct event_filter	*filter;
> >  	int			nr_events;
> >  };
> >  
> > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> > index b9aae72..c7ed427 100644
> > --- a/kernel/trace/trace_events_filter.c
> > +++ b/kernel/trace/trace_events_filter.c
> > @@ -418,7 +418,14 @@ oom:
> >  }
> >  EXPORT_SYMBOL_GPL(init_preds);
> >  
> > -static void filter_free_subsystem_preds(struct event_subsystem *system)
> > +enum {
> > +	FILTER_DISABLE_ALL,
> > +	FILTER_INIT_NO_RESET,
> > +	FILTER_SKIP_NO_RESET,
> > +};
> > +
> > +static void filter_free_subsystem_preds(struct event_subsystem *system,
> > +					int flag)
> >  {
> >  	struct ftrace_event_call *call;
> >  
> > @@ -426,6 +433,14 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
> >  		if (!call->define_fields)
> >  			continue;
> >  
> > +		if (flag == FILTER_INIT_NO_RESET) {
> > +			call->filter->no_reset = false;
> > +			continue;
> > +		}
> > +
> > +		if (flag == FILTER_SKIP_NO_RESET && call->filter->no_reset)
> > +			continue;
> > +
> >  		if (!strcmp(call->system, system->name)) {
> >  			filter_disable_preds(call);
> >  			remove_filter_string(call->filter);
> > @@ -527,7 +542,8 @@ static filter_pred_fn_t select_comparison_fn(int op, int field_size,
> >  
> >  static int filter_add_pred(struct filter_parse_state *ps,
> >  			   struct ftrace_event_call *call,
> > -			   struct filter_pred *pred)
> > +			   struct filter_pred *pred,
> > +			   bool dry_run)
> >  {
> >  	struct ftrace_event_field *field;
> >  	filter_pred_fn_t fn;
> > @@ -539,10 +555,12 @@ 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);
> > +		fn = filter_pred_and;
> > +		goto add_pred_fn;
> >  	} else if (pred->op == OP_OR) {
> >  		pred->pop_n = 2;
> > -		return filter_add_pred_fn(ps, call, pred, filter_pred_or);
> > +		fn = filter_pred_or;
> > +		goto add_pred_fn;
> >  	}
> >  
> >  	field = find_event_field(call, pred->field_name);
> > @@ -565,9 +583,6 @@ static int filter_add_pred(struct filter_parse_state *ps,
> >  		else
> >  			fn = filter_pred_strloc;
> >  		pred->str_len = field->size;
> > -		if (pred->op == OP_NE)
> > -			pred->not = 1;
> > -		return filter_add_pred_fn(ps, call, pred, fn);
> >  	} else {
> >  		if (field->is_signed)
> >  			ret = strict_strtoll(pred->str_val, 0, &val);
> > @@ -578,27 +593,33 @@ static int filter_add_pred(struct filter_parse_state *ps,
> >  			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);
> > -		return -EINVAL;
> > +		fn = select_comparison_fn(pred->op, field->size,
> > +					  field->is_signed);
> > +		if (!fn) {
> > +			parse_error(ps, FILT_ERR_INVALID_OP, 0);
> > +			return -EINVAL;
> > +		}
> >  	}
> >  
> >  	if (pred->op == OP_NE)
> >  		pred->not = 1;
> >  
> > -	return filter_add_pred_fn(ps, call, pred, fn);
> > +add_pred_fn:
> > +	if (!dry_run)
> > +		return filter_add_pred_fn(ps, call, pred, fn);
> > +	return 0;
> >  }
> >  
> >  static int filter_add_subsystem_pred(struct filter_parse_state *ps,
> >  				     struct event_subsystem *system,
> >  				     struct filter_pred *pred,
> > -				     char *filter_string)
> > +				     char *filter_string,
> > +				     bool dry_run)
> >  {
> >  	struct ftrace_event_call *call;
> >  	int err = 0;
> > +	bool fail = true;
> >  
> >  	list_for_each_entry(call, &ftrace_events, list) {
> >  
> > @@ -608,16 +629,24 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
> >  		if (strcmp(call->system, system->name))
> >  			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 (call->filter->no_reset)
> > +			continue;
> > +
> > +		err = filter_add_pred(ps, call, pred, dry_run);
> > +		if (err)
> > +			call->filter->no_reset = true;
> > +		else
> > +			fail = false;
> > +
> > +		if (!dry_run)
> > +			replace_filter_string(call->filter, filter_string);
> >  	}
> > -out:
> > -	return err;
> > +
> > +	if (fail) {
> > +		parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
> > +		return err;
> > +	}
> > +	return 0;
> >  }
> >  
> >  static void parse_init(struct filter_parse_state *ps,
> > @@ -976,12 +1005,14 @@ static int check_preds(struct filter_parse_state *ps)
> >  static int replace_preds(struct event_subsystem *system,
> >  			 struct ftrace_event_call *call,
> >  			 struct filter_parse_state *ps,
> > -			 char *filter_string)
> > +			 char *filter_string,
> > +			 bool dry_run)
> >  {
> >  	char *operand1 = NULL, *operand2 = NULL;
> >  	struct filter_pred *pred;
> >  	struct postfix_elt *elt;
> >  	int err;
> > +	int n_preds = 0;
> >  
> >  	err = check_preds(ps);
> >  	if (err)
> > @@ -1000,19 +1031,14 @@ static int replace_preds(struct event_subsystem *system,
> >  			continue;
> >  		}
> >  
> > +		if (n_preds++ == MAX_FILTER_PRED) {
> > +			parse_error(ps, FILT_ERR_TOO_MANY_PREDS, 0);
> > +			return -ENOSPC;
> > +		}
> > +
> >  		if (elt->op == OP_AND || elt->op == OP_OR) {
> >  			pred = create_logical_pred(elt->op);
> > -			if (call)
> > -				err = filter_add_pred(ps, call, pred);
> > -			else
> > -				err = filter_add_subsystem_pred(ps, system,
> > -							pred, filter_string);
> > -			filter_free_pred(pred);
> > -			if (err)
> > -				return err;
> > -
> > -			operand1 = operand2 = NULL;
> > -			continue;
> > +			goto add_pred;
> >  		}
> >  
> >  		if (!operand1 || !operand2) {
> > @@ -1021,11 +1047,12 @@ static int replace_preds(struct event_subsystem *system,
> >  		}
> >  
> >  		pred = create_pred(elt->op, operand1, operand2);
> > +add_pred:
> >  		if (call)
> > -			err = filter_add_pred(ps, call, pred);
> > +			err = filter_add_pred(ps, call, pred, false);
> >  		else
> >  			err = filter_add_subsystem_pred(ps, system, pred,
> > -							filter_string);
> > +						filter_string, dry_run);
> >  		filter_free_pred(pred);
> >  		if (err)
> >  			return err;
> > @@ -1066,7 +1093,7 @@ int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
> >  		goto out;
> >  	}
> >  
> > -	err = replace_preds(NULL, call, ps, filter_string);
> > +	err = replace_preds(NULL, call, ps, filter_string, false);
> >  	if (err)
> >  		append_filter_err(ps, call->filter);
> >  
> > @@ -1090,7 +1117,7 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
> >  	mutex_lock(&event_mutex);
> >  
> >  	if (!strcmp(strstrip(filter_string), "0")) {
> > -		filter_free_subsystem_preds(system);
> > +		filter_free_subsystem_preds(system, FILTER_DISABLE_ALL);
> >  		remove_filter_string(system->filter);
> >  		mutex_unlock(&event_mutex);
> >  		return 0;
> > @@ -1101,7 +1128,6 @@ 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);
> >  
> >  	parse_init(ps, filter_ops, filter_string);
> > @@ -1111,9 +1137,23 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
> >  		goto out;
> >  	}
> >  
> > -	err = replace_preds(system, NULL, ps, filter_string);
> > -	if (err)
> > +	filter_free_subsystem_preds(system, FILTER_INIT_NO_RESET);
> > +
> > +	/* try to see the filter can be applied to which events */
> > +	err = replace_preds(system, NULL, ps, filter_string, true);
> > +	if (err) {
> > +		append_filter_err(ps, system->filter);
> > +		goto out;
> > +	}
> > +
> > +	filter_free_subsystem_preds(system, FILTER_SKIP_NO_RESET);
> > +
> > +	/* really apply the filter to the events */
> > +	err = replace_preds(system, NULL, ps, filter_string, false);
> > +	if (err) {
> >  		append_filter_err(ps, system->filter);
> > +		filter_free_subsystem_preds(system, 2);
> > +	}
> >  
> >  out:
> >  	filter_opstack_clear(ps);
> > -- 
> > 1.6.3
> > 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/