Skip to contentSkip to conversation

tracing: ->filter cleanups

PATCH In review Oleg Nesterov 3 participants 12 messages Jul 15, 2014 – Jul 17, 2014

Subsystems (2)
Tracing ftrace
lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

v2 cover tracing: ->filter cleanups

Hello,

To avoid the confusion, let me resend everything (except the 1st patch you already applied) with v2 tag.

Changes:

4/7 - fix the typo in changelog, add the ack from Srikar

6/7 - by discussion with you and Namhyung: pass dir rather
then system, update replace_system_preds() as well.

7/7 - new, cosmetic/trivial.

I tried to play with "filter" files, everything seems to work with the additional debugging patch below.

Oleg.
---

kernel/trace/trace_events_filter.c +4 -0
index 7a8c152..4c16229 100644
@@ -848,6 +848,7 @@ static void filter_free_subsystem_preds(struct ftrace_subsystem_dir *dir,
 	struct ftrace_event_file *file;
 
 	list_for_each_entry(file, &tr->events, list) {
+BUG_ON((file->system != dir) != !!strcmp(file->event_call->class->system, dir->subsystem->name));
 		if (file->system != dir)
 			continue;
 		__remove_filter(file);

@@ -873,6 +874,7 @@ static void filter_free_subsystem_filters(struct ftrace_subsystem_dir *dir,
 	struct ftrace_event_file *file;
 
 	list_for_each_entry(file, &tr->events, list) {
+BUG_ON((file->system != dir) != !!strcmp(file->event_call->class->system, dir->subsystem->name));
 		if (file->system != dir)
 			continue;
 		__free_subsystem_filter(file);

@@ -1731,6 +1733,7 @@ static int replace_system_preds(struct ftrace_subsystem_dir *dir,
 	int err;
 
 	list_for_each_entry(file, &tr->events, list) {
+BUG_ON((file->system != dir) != !!strcmp(file->event_call->class->system, dir->subsystem->name));
 		if (file->system != dir)
 			continue;
 

@@ -1748,6 +1751,7 @@ static int replace_system_preds(struct ftrace_subsystem_dir *dir,
 	list_for_each_entry(file, &tr->events, list) {
 		struct event_filter *filter;
 
+BUG_ON((file->system != dir) != !!strcmp(file->event_call->class->system, dir->subsystem->name));
 		if (file->system != dir)
 			continue;
author
#1 lore raw

PATCH v2 1/7 tracing: kill destroy_preds() and destroy_file_preds()

destroy_preds() makes no any sense.

The only caller, event_remove(), actually wants destroy_file_preds(). __trace_remove_event_call() does destroy_call_preds() which takes care of call->filter.

And after the previous change we can simply remove destroy_preds() from event_remove(), we are going to call remove_event_from_tracers() which in turn calls remove_event_file_dir()->free_event_filter().

Signed-off-by Oleg Nesterov <[email protected]>
---
 include/linux/ftrace_event.h       |    1 -
 kernel/trace/trace_events.c        |    1 -
 kernel/trace/trace_events_filter.c |   20 --------------------
 3 files changed, 0 insertions(+), 22 deletions(-)
include/linux/ftrace_event.h +0 -1
index cff3106..738d465 100644
@@ -404,7 +404,6 @@ enum event_trigger_type {
 	ETT_EVENT_ENABLE	= (1 << 3),
 };
 
-extern void destroy_preds(struct ftrace_event_file *file);
 extern void destroy_call_preds(struct ftrace_event_call *call);
 extern int filter_match_preds(struct event_filter *filter, void *rec);
kernel/trace/trace_events.c +0 -1
index 735608e..d049a5e 100644
@@ -1621,7 +1621,6 @@ static void event_remove(struct ftrace_event_call *call)
 		if (file->event_call != call)
 			continue;
 		ftrace_event_enable_disable(file, 0);
-		destroy_preds(file);
 		/*
 		 * The do_for_each_event_file() is
 		 * a double loop. After finding the call for this
kernel/trace/trace_events_filter.c +0 -20
index 8a86319..30fc66f 100644
@@ -810,26 +810,6 @@ void destroy_call_preds(struct ftrace_event_call *call)
 	call->filter = NULL;
 }
 
-static void destroy_file_preds(struct ftrace_event_file *file)
-{
-	__free_filter(file->filter);
-	file->filter = NULL;
-}
-
-/*
- * Called when destroying the ftrace_event_file.
- * The file is being freed, so we do not need to worry about
- * the file being currently used. This is for module code removing
- * the tracepoints from within it.
- */
-void destroy_preds(struct ftrace_event_file *file)
-{
-	if (file->event_call->flags & TRACE_EVENT_FL_USE_CALL_FILTER)
-		destroy_call_preds(file->event_call);
-	else
-		destroy_file_preds(file);
-}
-
 static struct event_filter *__alloc_filter(void)
 {
 	struct event_filter *filter;
Signature · 1 line
1.5.5.1
author
#2 lore raw

PATCH v2 2/7 tracing: kill destroy_call_preds()

In reply to Oleg Nesterov

Remove destroy_call_preds(). Its only caller, __trace_remove_event_call(), can use free_event_filter() and nullify ->filter by hand.

Perhaps we could keep this trivial helper although imo it is pointless, but then it should be static in trace_events.c.

Signed-off-by Oleg Nesterov <[email protected]>
---
 include/linux/ftrace_event.h       |    1 -
 kernel/trace/trace_events.c        |    3 ++-
 kernel/trace/trace_events_filter.c |    6 ------
 3 files changed, 2 insertions(+), 8 deletions(-)
include/linux/ftrace_event.h +0 -1
index 738d465..f434d75 100644
@@ -404,7 +404,6 @@ enum event_trigger_type {
 	ETT_EVENT_ENABLE	= (1 << 3),
 };
 
-extern void destroy_call_preds(struct ftrace_event_call *call);
 extern int filter_match_preds(struct event_filter *filter, void *rec);
 
 extern int filter_check_discard(struct ftrace_event_file *file, void *rec,
kernel/trace/trace_events.c +2 -1
index d049a5e..8690a3f 100644
@@ -1748,7 +1748,8 @@ static void __trace_remove_event_call(struct ftrace_event_call *call)
 {
 	event_remove(call);
 	trace_destroy_fields(call);
-	destroy_call_preds(call);
+	free_event_filter(call->filter);
+	call->filter = NULL;
 }
 
 static int probe_remove_event_call(struct ftrace_event_call *call)
kernel/trace/trace_events_filter.c +0 -6
index 30fc66f..1edec32 100644
@@ -804,12 +804,6 @@ void free_event_filter(struct event_filter *filter)
 	__free_filter(filter);
 }
 
-void destroy_call_preds(struct ftrace_event_call *call)
-{
-	__free_filter(call->filter);
-	call->filter = NULL;
-}
-
 static struct event_filter *__alloc_filter(void)
 {
 	struct event_filter *filter;
Signature · 1 line
1.5.5.1
author
#3 lore raw

PATCH v2 3/7 tracing: kill call_filter_disable()

In reply to Oleg Nesterov

It seems that the only purpose of call_filter_disable() is to make filter_disable() less clear and symmetrical, remove it.

Signed-off-by Oleg Nesterov <[email protected]>
---
 kernel/trace/trace_events_filter.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)
kernel/trace/trace_events_filter.c +1 -6
index 1edec32..54a125c 100644
@@ -774,17 +774,12 @@ static void __free_preds(struct event_filter *filter)
 	filter->n_preds = 0;
 }
 
-static void call_filter_disable(struct ftrace_event_call *call)
-{
-	call->flags &= ~TRACE_EVENT_FL_FILTERED;
-}
-
 static void filter_disable(struct ftrace_event_file *file)
 {
 	struct ftrace_event_call *call = file->event_call;
 
 	if (call->flags & TRACE_EVENT_FL_USE_CALL_FILTER)
-		call_filter_disable(call);
+		call->flags &= ~TRACE_EVENT_FL_FILTERED;
 	else
 		file->flags &= ~FTRACE_EVENT_FL_FILTERED;
 }
Signature · 1 line
1.5.5.1
author
#4 lore raw

PATCH v2 4/7 tracing/uprobes: kill the dead TRACE_EVENT_FL_USE_CALL_FILTER logic

In reply to Oleg Nesterov

alloc_trace_uprobe() sets TRACE_EVENT_FL_USE_CALL_FILTER for unknown reason and this is simply wrong. Fortunately this has no effect because register_uprobe_event() clears call->flags after that.

Kill both. This trace_uprobe was kzalloc'ed and we rely on this fact anyway.

Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Srikar Dronamraju <[email protected]>
---
 kernel/trace/trace_uprobe.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
kernel/trace/trace_uprobe.c +1 -2
index 3c9b97e..33ff6a2 100644
@@ -265,7 +265,6 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
 	if (is_ret)
 		tu->consumer.ret_handler = uretprobe_dispatcher;
 	init_trace_uprobe_filter(&tu->filter);
-	tu->tp.call.flags |= TRACE_EVENT_FL_USE_CALL_FILTER;
 	return tu;
 
 error:

@@ -1292,7 +1291,7 @@ static int register_uprobe_event(struct trace_uprobe *tu)
 		kfree(call->print_fmt);
 		return -ENODEV;
 	}
-	call->flags = 0;
+
 	call->class->reg = trace_uprobe_register;
 	call->data = tu;
 	ret = trace_add_event_call(call);
Signature · 1 line
1.5.5.1
author
#5 lore raw

PATCH v2 5/7 tracing: kill ftrace_event_call->files

In reply to Oleg Nesterov

Remove ftrace_event_call->files. It has no users, and in fact even the commit ae63b31e4d0e "tracing: Separate out trace events from global variables" which added this member did not use it.

Signed-off-by Oleg Nesterov <[email protected]>
---
 include/linux/ftrace_event.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
include/linux/ftrace_event.h +0 -1
index f434d75..06c6faa 100644
@@ -272,7 +272,6 @@ struct ftrace_event_call {
 	struct trace_event	event;
 	const char		*print_fmt;
 	struct event_filter	*filter;
-	struct list_head	*files;
 	void			*mod;
 	void			*data;
 	/*
Signature · 1 line
1.5.5.1
author
#6 lore raw

PATCH v2 6/7 tracing: change apply_subsystem_event_filter() paths to check file->system == dir

In reply to Oleg Nesterov

filter_free_subsystem_preds(), filter_free_subsystem_filters() and replace_system_preds() can simply check file->system->subsystem and avoid strcmp(call->class->system).

Better yet, we can pass "struct ftrace_subsystem_dir *dir" instead of event_subsystem and just check file->system == dir.

Thanks to Namhyung Kim who pointed out that replace_system_preds() can be changed too.

Signed-off-by Oleg Nesterov <[email protected]>
---
 kernel/trace/trace_events_filter.c |   39 ++++++++++++++---------------------
 1 files changed, 16 insertions(+), 23 deletions(-)
kernel/trace/trace_events_filter.c +16 -23
index 54a125c..c2ef5a5 100644
@@ -842,17 +842,14 @@ static inline void __remove_filter(struct ftrace_event_file *file)
 		remove_filter_string(file->filter);
 }
 
-static void filter_free_subsystem_preds(struct event_subsystem *system,
+static void filter_free_subsystem_preds(struct ftrace_subsystem_dir *dir,
 					struct trace_array *tr)
 {
 	struct ftrace_event_file *file;
-	struct ftrace_event_call *call;
 
 	list_for_each_entry(file, &tr->events, list) {
-		call = file->event_call;
-		if (strcmp(call->class->system, system->name) != 0)
+		if (file->system != dir)
 			continue;
-
 		__remove_filter(file);
 	}
 }

@@ -870,15 +867,13 @@ static inline void __free_subsystem_filter(struct ftrace_event_file *file)
 	}
 }
 
-static void filter_free_subsystem_filters(struct event_subsystem *system,
+static void filter_free_subsystem_filters(struct ftrace_subsystem_dir *dir,
 					  struct trace_array *tr)
 {
 	struct ftrace_event_file *file;
-	struct ftrace_event_call *call;
 
 	list_for_each_entry(file, &tr->events, list) {
-		call = file->event_call;
-		if (strcmp(call->class->system, system->name) != 0)
+		if (file->system != dir)
 			continue;
 		__free_subsystem_filter(file);
 	}

@@ -1724,13 +1719,12 @@ struct filter_list {
 	struct event_filter	*filter;
 };
 
-static int replace_system_preds(struct event_subsystem *system,
+static int replace_system_preds(struct ftrace_subsystem_dir *dir,
 				struct trace_array *tr,
 				struct filter_parse_state *ps,
 				char *filter_string)
 {
 	struct ftrace_event_file *file;
-	struct ftrace_event_call *call;
 	struct filter_list *filter_item;
 	struct filter_list *tmp;
 	LIST_HEAD(filter_list);

@@ -1738,15 +1732,15 @@ static int replace_system_preds(struct event_subsystem *system,
 	int err;
 
 	list_for_each_entry(file, &tr->events, list) {
-		call = file->event_call;
-		if (strcmp(call->class->system, system->name) != 0)
+		if (file->system != dir)
 			continue;
 
 		/*
 		 * Try to see if the filter can be applied
 		 *  (filter arg is ignored on dry_run)
 		 */
-		err = replace_preds(call, NULL, ps, filter_string, true);
+		err = replace_preds(file->event_call, NULL, ps,
+					filter_string, true);
 		if (err)
 			event_set_no_set_filter_flag(file);
 		else

@@ -1756,9 +1750,7 @@ static int replace_system_preds(struct event_subsystem *system,
 	list_for_each_entry(file, &tr->events, list) {
 		struct event_filter *filter;
 
-		call = file->event_call;
-
-		if (strcmp(call->class->system, system->name) != 0)
+		if (file->system != dir)
 			continue;
 
 		if (event_no_set_filter_flag(file))

@@ -1780,7 +1772,8 @@ static int replace_system_preds(struct event_subsystem *system,
 		if (err)
 			goto fail_mem;
 
-		err = replace_preds(call, filter, ps, filter_string, false);
+		err = replace_preds(file->event_call, filter, ps,
+					filter_string, false);
 		if (err) {
 			filter_disable(file);
 			parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);

@@ -1928,7 +1921,7 @@ int create_event_filter(struct ftrace_event_call *call,
  * Identical to create_filter() except that it creates a subsystem filter
  * and always remembers @filter_str.
  */
-static int create_system_filter(struct event_subsystem *system,
+static int create_system_filter(struct ftrace_subsystem_dir *dir,
 				struct trace_array *tr,
 				char *filter_str, struct event_filter **filterp)
 {

@@ -1938,7 +1931,7 @@ static int create_system_filter(struct event_subsystem *system,
 
 	err = create_filter_start(filter_str, true, &ps, &filter);
 	if (!err) {
-		err = replace_system_preds(system, tr, ps, filter_str);
+		err = replace_system_preds(dir, tr, ps, filter_str);
 		if (!err) {
 			/* System filters just show a default message */
 			kfree(filter->filter_string);

@@ -2022,18 +2015,18 @@ int apply_subsystem_event_filter(struct ftrace_subsystem_dir *dir,
 	}
 
 	if (!strcmp(strstrip(filter_string), "0")) {
-		filter_free_subsystem_preds(system, tr);
+		filter_free_subsystem_preds(dir, tr);
 		remove_filter_string(system->filter);
 		filter = system->filter;
 		system->filter = NULL;
 		/* Ensure all filters are no longer used */
 		synchronize_sched();
-		filter_free_subsystem_filters(system, tr);
+		filter_free_subsystem_filters(dir, tr);
 		__free_filter(filter);
 		goto out_unlock;
 	}
 
-	err = create_system_filter(system, tr, filter_string, &filter);
+	err = create_system_filter(dir, tr, filter_string, &filter);
 	if (filter) {
 		/*
 		 * No event actually uses the system filter
Signature · 1 line
1.5.5.1
maintainer
#7 lore raw

Re: [PATCH v2 6/7] tracing: change apply_subsystem_event_filter() paths to check file->system == dir

Quoted 9 lines · On Tue, 15 Jul 2014 20:48:29 +0200 Oleg Nesterov <[email protected]> wrote:

filter_free_subsystem_preds(), filter_free_subsystem_filters() and replace_system_preds() can simply check file->system->subsystem and avoid strcmp(call->class->system).

Better yet, we can pass "struct ftrace_subsystem_dir *dir" instead of event_subsystem and just check file->system == dir.

Thanks to Namhyung Kim who pointed out that replace_system_preds() can be changed too.

What? No thanks to me for suggesting the file->system == dir idea?

/me pouts like a baby

-- Steve

Quoted 3 lines
Signed-off-by Oleg Nesterov <[email protected]>

---

author
#8 lore raw

Re: [PATCH v2 6/7] tracing: change apply_subsystem_event_filter() paths to check file->system == dir

Quoted 11 lines · Steven Rostedt
Quoted 7 lines · On Tue, 15 Jul 2014 20:48:29 +0200 Oleg Nesterov <[email protected]> wrote:

filter_free_subsystem_preds(), filter_free_subsystem_filters() and replace_system_preds() can simply check file->system->subsystem and avoid strcmp(call->class->system).

Better yet, we can pass "struct ftrace_subsystem_dir *dir" instead of event_subsystem and just check file->system == dir.

Quoted 6 lines
Quoted 2 lines

Thanks to Namhyung Kim who pointed out that replace_system_preds() can be changed too.

What? No thanks to me for suggesting the file->system == dir idea?

/me pouts like a baby

OOPS, sorry, Iactually forgot to add "as suggested by" ;)

But, otoh... who cares about rostedt when it comes to tracing area?

Oleg.

maintainer
#9 lore raw

Re: [PATCH v2 6/7] tracing: change apply_subsystem_event_filter() paths to check file->system == dir

Quoted 15 lines · On Wed, 16 Jul 2014 21:12:48 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 14 lines · Steven Rostedt
Quoted 9 lines · On Tue, 15 Jul 2014 20:48:29 +0200 Oleg Nesterov <[email protected]> wrote:

filter_free_subsystem_preds(), filter_free_subsystem_filters() and replace_system_preds() can simply check file->system->subsystem and avoid strcmp(call->class->system).

Better yet, we can pass "struct ftrace_subsystem_dir *dir" instead of event_subsystem and just check file->system == dir.

Thanks to Namhyung Kim who pointed out that replace_system_preds() can be changed too.

Quoted 8 lines
Quoted 3 lines

What? No thanks to me for suggesting the file->system == dir idea?

/me pouts like a baby

OOPS, sorry, Iactually forgot to add "as suggested by" ;)

But, otoh... who cares about rostedt when it comes to tracing area?

As Paul McKenney would say...

;-) ;-) ;-)

-- Steve

author
#10 lore raw

PATCH v2 7/7 tracing: kill "filter_string" arg of replace_preds()

In reply to Oleg Nesterov

Cosmetic, but replace_preds() doesn't need/use "char *filter_string". Remove it to microsimplify the code.

Signed-off-by Oleg Nesterov <[email protected]>
---
 kernel/trace/trace_events_filter.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)
kernel/trace/trace_events_filter.c +3 -6
index c2ef5a5..7a8c152 100644
@@ -1546,7 +1546,6 @@ static int fold_pred_tree(struct event_filter *filter,
 static int replace_preds(struct ftrace_event_call *call,
 			 struct event_filter *filter,
 			 struct filter_parse_state *ps,
-			 char *filter_string,
 			 bool dry_run)
 {
 	char *operand1 = NULL, *operand2 = NULL;

@@ -1739,8 +1738,7 @@ static int replace_system_preds(struct ftrace_subsystem_dir *dir,
 		 * Try to see if the filter can be applied
 		 *  (filter arg is ignored on dry_run)
 		 */
-		err = replace_preds(file->event_call, NULL, ps,
-					filter_string, true);
+		err = replace_preds(file->event_call, NULL, ps, true);
 		if (err)
 			event_set_no_set_filter_flag(file);
 		else

@@ -1772,8 +1770,7 @@ static int replace_system_preds(struct ftrace_subsystem_dir *dir,
 		if (err)
 			goto fail_mem;
 
-		err = replace_preds(file->event_call, filter, ps,
-					filter_string, false);
+		err = replace_preds(file->event_call, filter, ps, false);
 		if (err) {
 			filter_disable(file);
 			parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);

@@ -1895,7 +1892,7 @@ static int create_filter(struct ftrace_event_call *call,
 
 	err = create_filter_start(filter_str, set_str, &ps, &filter);
 	if (!err) {
-		err = replace_preds(call, filter, ps, filter_str, false);
+		err = replace_preds(call, filter, ps, false);
 		if (err && set_str)
 			append_filter_err(ps, filter);
 	}
Signature · 1 line
1.5.5.1
#11 lore raw
In reply to Oleg Nesterov

Hi Oleg,

Quoted 5 lines · Oleg Nesterov

Hello,

To avoid the confusion, let me resend everything (except the 1st patch you already applied) with v2 tag.

Quoted 8 lines

Changes:

4/7 - fix the typo in changelog, add the ack from Srikar

6/7 - by discussion with you and Namhyung: pass dir rather
then system, update replace_system_preds() as well.

7/7 - new, cosmetic/trivial.

Looks all good to me. You can add my Acked-by if you want. :)

Thanks,
Namhyung