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

[Steven Rostedt]

On Mon, 20 Jul 2009, 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

OK, I tried this and it worked as you described, but it also does 
something that I did not like. Basically this:

# echo 'vec == 1' > filter
# cat softirq_entry/filter
 vec == 1
# cat irq_handler_entry/filter
 none
# cat filter
 vec == 1
# echo 'irq == 5' > filter
# cat irq_handler_entry/filter
 irq == 5
# cat softirq_entry/filter
 vec == 1
# cat filter
 irq == 5


That last "cat filter" should show:
 vec == 1
 irq == 5

I can keep this version, since I do find it useful (I've now come to the 
conclusion that having the filter work only for those filters that it 
would work for is fine). But the top level filter should show all filters.

You can make a patch on top of this one, and I'll just keep this one in my 
queue.

Thanks,

-- Steve

> > 
> > 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.
> 
> 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/