[PATCH] tracing: Restore system filter behavior

[PATCH] tracing: Restore system filter behavior

[Li Zefan]

Though not all events have field 'prev_pid', it was allowed to do this:

  # echo 'prev_pid == 100' > events/sched/filter

but commit 75b8e98263fdb0bfbdeba60d4db463259f1fe8a2 (tracing/filter: Swap
entire filter of events) broke it without any reason.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 include/linux/ftrace_event.h       |    2 ++
 kernel/trace/trace_events_filter.c |    7 ++++++-
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 96efa67..c3da42d 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -172,6 +172,7 @@ enum {
 	TRACE_EVENT_FL_FILTERED_BIT,
 	TRACE_EVENT_FL_RECORDED_CMD_BIT,
 	TRACE_EVENT_FL_CAP_ANY_BIT,
+	TRACE_EVENT_FL_NO_SET_FILTER_BIT,
 };
 
 enum {
@@ -179,6 +180,7 @@ enum {
 	TRACE_EVENT_FL_FILTERED		= (1 << TRACE_EVENT_FL_FILTERED_BIT),
 	TRACE_EVENT_FL_RECORDED_CMD	= (1 << TRACE_EVENT_FL_RECORDED_CMD_BIT),
 	TRACE_EVENT_FL_CAP_ANY		= (1 << TRACE_EVENT_FL_CAP_ANY_BIT),
+	TRACE_EVENT_FL_NO_SET_FILTER	= (1 << TRACE_EVENT_FL_NO_SET_FILTER_BIT),
 };
 
 struct ftrace_event_call {
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 816d3d0..86040d9 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -1649,7 +1649,9 @@ static int replace_system_preds(struct event_subsystem *system,
 		 */
 		err = replace_preds(call, NULL, ps, filter_string, true);
 		if (err)
-			goto fail;
+			call->flags |= TRACE_EVENT_FL_NO_SET_FILTER;
+		else
+			call->flags &= ~TRACE_EVENT_FL_NO_SET_FILTER;
 	}
 
 	list_for_each_entry(call, &ftrace_events, list) {
@@ -1658,6 +1660,9 @@ static int replace_system_preds(struct event_subsystem *system,
 		if (strcmp(call->class->system, system->name) != 0)
 			continue;
 
+		if (call->flags & TRACE_EVENT_FL_NO_SET_FILTER)
+			continue;
+
 		filter_item = kzalloc(sizeof(*filter_item), GFP_KERNEL);
 		if (!filter_item)
 			goto fail_mem;
-- 
1.7.3.1

Re: [PATCH] tracing: Restore system filter behavior

[Steven Rostedt]

On Tue, 2011-11-01 at 09:09 +0800, Li Zefan wrote:
> Though not all events have field 'prev_pid', it was allowed to do this:
> 
>   # echo 'prev_pid == 100' > events/sched/filter
> 
> but commit 75b8e98263fdb0bfbdeba60d4db463259f1fe8a2 (tracing/filter: Swap
> entire filter of events) broke it without any reason.

I wouldn't say "without any reason" as there was a reason.

But I'm not pleased with the way this all works still.

# echo 'prev_pid == 100' > /debug/tracing/events/sched/filter
# cat /debug/tracing/events/sched/filter
prev_pid == 100

# cat /debug/tracing/events/sched/sched_migrate_task/filter
none

# echo 'orig_cpu == 1' > /debug/tracing/events/sched/sched_migrate_task/filter
# cat /debug/tracing/events/sched/sched_migrate_task/filter
orig_cpu == 1

# cat /debug/tracing/events/sched/filter
prev_pid == 100


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.

-- Steve

Re: [PATCH] tracing: Restore system filter behavior

[Steven Rostedt]

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.

-- Steve

Re: [PATCH] tracing: Restore system filter behavior

[Steven Rostedt]

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?

-- 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);
 	/*
 	 * 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

Re: [PATCH] tracing: Restore system filter behavior

[Li Zefan]

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

Re: [PATCH] tracing: Restore system filter behavior

[Steven Rostedt]

On Thu, 2011-11-03 at 09:25 +0800, Li Zefan wrote:
>  
> > -	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.

I originally had it that way, but for some reason I had to change it.
Not sure why. I'll put it back and see if that reason pops up again ;)

Thanks,

-- Steve

[tip:perf/core] tracing: Restore system filter behavior

[tip-bot for Li Zefan]

Commit-ID:  ed0449af5373abd766c79fbf83254bebc996bd23
Gitweb:     http://git.kernel.org/tip/ed0449af5373abd766c79fbf83254bebc996bd23
Author:     Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Tue, 1 Nov 2011 09:09:35 +0800
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Wed, 2 Nov 2011 13:56:25 -0400

tracing: Restore system filter behavior

Though not all events have field 'prev_pid', it was allowed to do this:

  # echo 'prev_pid == 100' > events/sched/filter

but commit 75b8e98263fdb0bfbdeba60d4db463259f1fe8a2 (tracing/filter: Swap
entire filter of events) broke it without any reason.

Link: http://lkml.kernel.org/r/4EAF46CF.8040408@cn.fujitsu.com

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace_event.h       |    2 ++
 kernel/trace/trace_events_filter.c |    7 ++++++-
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 96efa67..c3da42d 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -172,6 +172,7 @@ enum {
 	TRACE_EVENT_FL_FILTERED_BIT,
 	TRACE_EVENT_FL_RECORDED_CMD_BIT,
 	TRACE_EVENT_FL_CAP_ANY_BIT,
+	TRACE_EVENT_FL_NO_SET_FILTER_BIT,
 };
 
 enum {
@@ -179,6 +180,7 @@ enum {
 	TRACE_EVENT_FL_FILTERED		= (1 << TRACE_EVENT_FL_FILTERED_BIT),
 	TRACE_EVENT_FL_RECORDED_CMD	= (1 << TRACE_EVENT_FL_RECORDED_CMD_BIT),
 	TRACE_EVENT_FL_CAP_ANY		= (1 << TRACE_EVENT_FL_CAP_ANY_BIT),
+	TRACE_EVENT_FL_NO_SET_FILTER	= (1 << TRACE_EVENT_FL_NO_SET_FILTER_BIT),
 };
 
 struct ftrace_event_call {
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 816d3d0..86040d9 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -1649,7 +1649,9 @@ static int replace_system_preds(struct event_subsystem *system,
 		 */
 		err = replace_preds(call, NULL, ps, filter_string, true);
 		if (err)
-			goto fail;
+			call->flags |= TRACE_EVENT_FL_NO_SET_FILTER;
+		else
+			call->flags &= ~TRACE_EVENT_FL_NO_SET_FILTER;
 	}
 
 	list_for_each_entry(call, &ftrace_events, list) {
@@ -1658,6 +1660,9 @@ static int replace_system_preds(struct event_subsystem *system,
 		if (strcmp(call->class->system, system->name) != 0)
 			continue;
 
+		if (call->flags & TRACE_EVENT_FL_NO_SET_FILTER)
+			continue;
+
 		filter_item = kzalloc(sizeof(*filter_item), GFP_KERNEL);
 		if (!filter_item)
 			goto fail_mem;

[tip:perf/core] tracing: Restore system filter behavior

[tip-bot for Li Zefan]

Commit-ID:  27b14b56af081ec7edeefb3a38b2c9577cc5ef48
Gitweb:     http://git.kernel.org/tip/27b14b56af081ec7edeefb3a38b2c9577cc5ef48
Author:     Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Tue, 1 Nov 2011 09:09:35 +0800
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Mon, 5 Dec 2011 13:28:45 -0500

tracing: Restore system filter behavior

Though not all events have field 'prev_pid', it was allowed to do this:

  # echo 'prev_pid == 100' > events/sched/filter

but commit 75b8e98263fdb0bfbdeba60d4db463259f1fe8a2 (tracing/filter: Swap
entire filter of events) broke it without any reason.

Link: http://lkml.kernel.org/r/4EAF46CF.8040408@cn.fujitsu.com

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace_event.h       |    2 ++
 kernel/trace/trace_events_filter.c |    7 ++++++-
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 96efa67..c3da42d 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -172,6 +172,7 @@ enum {
 	TRACE_EVENT_FL_FILTERED_BIT,
 	TRACE_EVENT_FL_RECORDED_CMD_BIT,
 	TRACE_EVENT_FL_CAP_ANY_BIT,
+	TRACE_EVENT_FL_NO_SET_FILTER_BIT,
 };
 
 enum {
@@ -179,6 +180,7 @@ enum {
 	TRACE_EVENT_FL_FILTERED		= (1 << TRACE_EVENT_FL_FILTERED_BIT),
 	TRACE_EVENT_FL_RECORDED_CMD	= (1 << TRACE_EVENT_FL_RECORDED_CMD_BIT),
 	TRACE_EVENT_FL_CAP_ANY		= (1 << TRACE_EVENT_FL_CAP_ANY_BIT),
+	TRACE_EVENT_FL_NO_SET_FILTER	= (1 << TRACE_EVENT_FL_NO_SET_FILTER_BIT),
 };
 
 struct ftrace_event_call {
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index d6e7926..95dc31e 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -1649,7 +1649,9 @@ static int replace_system_preds(struct event_subsystem *system,
 		 */
 		err = replace_preds(call, NULL, ps, filter_string, true);
 		if (err)
-			goto fail;
+			call->flags |= TRACE_EVENT_FL_NO_SET_FILTER;
+		else
+			call->flags &= ~TRACE_EVENT_FL_NO_SET_FILTER;
 	}
 
 	list_for_each_entry(call, &ftrace_events, list) {
@@ -1658,6 +1660,9 @@ static int replace_system_preds(struct event_subsystem *system,
 		if (strcmp(call->class->system, system->name) != 0)
 			continue;
 
+		if (call->flags & TRACE_EVENT_FL_NO_SET_FILTER)
+			continue;
+
 		filter_item = kzalloc(sizeof(*filter_item), GFP_KERNEL);
 		if (!filter_item)
 			goto fail_mem;