Re: [PATCH] tracing: Restore system filter behavior

[Li Zefan <lizf@cn.fujitsu.com>]

Steven Rostedt wrote:
> On Wed, 2011-11-02 at 14:51 -0400, Steven Rostedt wrote:
>> On Wed, 2011-11-02 at 14:22 -0400, Steven Rostedt wrote:
>>
>>> The above shows how things are just ambiguous. I have no problem in
>>> using the top filter to set multiple events. But the top filter should
>>> not keep the state of what was set. Perhaps just have system event
>>> filters always show default text. Like:
>>>
>>> # cat /debug/tracing/events/sched/filter
>>> ### global filter ###
>>> # Use this to set multiple event filters
>>> # Only affects events that have the event fields specified in the filter
>>>
>>>
>>> I'll add your patch, but are you OK with the above always printing for
>>> system event filters?  Just to remove the ambiguous state.
>>
>>
>> As the filter is also used to show errors, I'll only have it print the
>> "message" if the filter worked. If there's an error, then the error
>> message will display instead.
>>
> 
> What's your thought on the below patch?
> 

I'm fine with this idea. 

a comment below..

> -- Steve
> 
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 86040d9..6dee2b5 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -27,6 +27,12 @@
>  #include "trace.h"
>  #include "trace_output.h"
>  
> +#define DEFAULT_SYS_FILTER_MESSAGE					\
> +	"### global filter ###\n"					\
> +	"# Use this to set filters for multiple events.\n"		\
> +	"# Only events with the given fields will be affected.\n"	\
> +	"# If no events are modified, an error message will be displayed here"
> +
>  enum filter_op_ids
>  {
>  	OP_OR,
> @@ -646,7 +652,7 @@ void print_subsystem_event_filter(struct event_subsystem *system,
>  	if (filter && filter->filter_string)
>  		trace_seq_printf(s, "%s\n", filter->filter_string);
>  	else
> -		trace_seq_printf(s, "none\n");
> +		trace_seq_printf(s, DEFAULT_SYS_FILTER_MESSAGE "\n");
>  	mutex_unlock(&event_mutex);
>  }
>  
> @@ -1838,7 +1844,8 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
>  	if (!filter)
>  		goto out;
>  
> -	replace_filter_string(filter, filter_string);
> +	/* System filters just show a default message */
> +	replace_filter_string(filter, DEFAULT_SYS_FILTER_MESSAGE);

Seems we can call remove_filter_string() instead to save memory.

>  	/*
>  	 * No event actually uses the system filter
>  	 * we can free it without synchronize_sched().
> @@ -1848,14 +1855,12 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
>  
>  	parse_init(ps, filter_ops, filter_string);
>  	err = filter_parse(ps);
> -	if (err) {
> -		append_filter_err(ps, system->filter);
> -		goto out;
> -	}
> +	if (err)
> +		goto err_filter;
>  
>  	err = replace_system_preds(system, ps, filter_string);
>  	if (err)
> -		append_filter_err(ps, system->filter);
> +		goto err_filter;
>  
>  out:
>  	filter_opstack_clear(ps);
> @@ -1865,6 +1870,11 @@ out_unlock:
>  	mutex_unlock(&event_mutex);
>  
>  	return err;
> +
> +err_filter:
> +	replace_filter_string(filter, filter_string);
> +	append_filter_err(ps, system->filter);
> +	goto out;
>  }
>  
>  #ifdef CONFIG_PERF_EVENTS
> 
> 
>