tracing/filters: Defer pred allocation

PATCH Applied Li Zefan 5 participants 11 messages Aug 31, 2009 – Sep 6, 2009

Subsystems (3)

Tracing ABI/API ftrace

lore

Share & tools

lore

Li Zefan

author

2009-08-31 08:51 UTC #0 lore raw

PATCH 1/1 tracing/filters: Defer pred allocation

init_preds() allocates about 5392 bytes of memory (on x86_32) for a TRACE_EVENT. With my config, at system boot total memory occupied is:

	5392 * (642 + 15) == 3459KB

642 == cat available_events | wc -l
15 == number of dirs in events/ftrace

That's quite a lot, so we'd better defer memory allocation util it's needed, that's when filter is used.

Signed-off-by Li Zefan <[email protected]>

---

tracing/kprobe needs rebase after this patch..

---
 include/linux/ftrace_event.h       |    1 -
 include/linux/syscalls.h           |    2 -
 include/trace/ftrace.h             |    1 -
 kernel/trace/trace_events_filter.c |   50 +++++++++++++++++++++++++++++------
 kernel/trace/trace_export.c        |    1 -
 5 files changed, 41 insertions(+), 14 deletions(-)

include/linux/ftrace_event.h +0 -1

index ace2da9..7554804 100644
@@ -133,7 +133,6 @@ struct ftrace_event_call {
 #define MAX_FILTER_PRED		32
 #define MAX_FILTER_STR_VAL	128
 
-extern int init_preds(struct ftrace_event_call *call);
 extern void destroy_preds(struct ftrace_event_call *call);
 extern int filter_match_preds(struct ftrace_event_call *call, void *rec);
 extern int filter_current_check_discard(struct ftrace_event_call *call,

include/linux/syscalls.h +0 -2

index f124c89..a8e3782 100644
@@ -177,7 +177,6 @@ static void prof_sysexit_disable_##sname(struct ftrace_event_call *event_call) \
 		event_enter_##sname.id = id;				\
 		set_syscall_enter_id(num, id);				\
 		INIT_LIST_HEAD(&event_enter_##sname.fields);		\
-		init_preds(&event_enter_##sname);			\
 		return 0;						\
 	}								\
 	TRACE_SYS_ENTER_PROFILE(sname);					\

@@ -214,7 +213,6 @@ static void prof_sysexit_disable_##sname(struct ftrace_event_call *event_call) \
 		event_exit_##sname.id = id;				\
 		set_syscall_exit_id(num, id);				\
 		INIT_LIST_HEAD(&event_exit_##sname.fields);		\
-		init_preds(&event_exit_##sname);			\
 		return 0;						\
 	}								\
 	TRACE_SYS_EXIT_PROFILE(sname);					\

include/trace/ftrace.h +0 -1

index 57c56a9..bfbc842 100644
@@ -622,7 +622,6 @@ static int ftrace_raw_init_event_##call(void)				\
 		return -ENODEV;						\
 	event_##call.id = id;						\
 	INIT_LIST_HEAD(&event_##call.fields);				\
-	init_preds(&event_##call);					\
 	return 0;							\
 }									\
 									\

kernel/trace/trace_events_filter.c +41 -9

110 lines not on this page · raw message

kernel/trace/trace_export.c +0 -1

index 029a91f..df1bf6e 100644
@@ -135,7 +135,6 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
 static int ftrace_raw_init_event_##call(void)				\
 {									\
 	INIT_LIST_HEAD(&event_##call.fields);				\
-	init_preds(&event_##call);					\
 	return 0;							\
 }									\

Signature · 1 line

1.6.3

Ingo Molnar

Applied

2009-08-31 08:59 UTC #1 lore raw

Quoted 6 lines · Li Zefan

init_preds() allocates about 5392 bytes of memory (on x86_32) for a TRACE_EVENT. With my config, at system boot total memory occupied is:
	5392 * (642 + 15) == 3459KB

Quoted 7 lines

642 == cat available_events | wc -l
15 == number of dirs in events/ftrace
That's quite a lot, so we'd better defer memory allocation util it's needed, that's when filter is used.
Signed-off-by Li Zefan <[email protected]>

Applied, thanks!

Quoted 1 line

tracing/kprobe needs rebase after this patch..

You mean with many probes registered it has a lot of memory footprint? Instead of a rebase a merge of tracing/core into tracing/kprobes would be less intrusive.

Ingo

Li Zefan

author

2009-08-31 09:08 UTC #2 lore raw

Quoted 14 lines · Ingo Molnar

Quoted 12 lines · Li Zefan
init_preds() allocates about 5392 bytes of memory (on x86_32) for a TRACE_EVENT. With my config, at system boot total memory occupied is:
	5392 * (642 + 15) == 3459KB
642 == cat available_events | wc -l
15 == number of dirs in events/ftrace
That's quite a lot, so we'd better defer memory allocation util it's needed, that's when filter is used.

Quoted 7 lines

Quoted 1 line
Signed-off-by Li Zefan <[email protected]>
Applied, thanks!
Quoted 1 line
tracing/kprobe needs rebase after this patch..
You mean with many probes registered it has a lot of memory

I think so, if filter is used.

Quoted 3 lines

footprint? Instead of a rebase a merge of tracing/core into tracing/kprobes would be less intrusive.

Yeah, I meant this patch conflicts with some patches in tracing/kprobe. :)

Masami Hiramatsu

2009-09-01 12:57 UTC #3 lore raw

Quoted 23 lines · Li Zefan

Quoted 14 lines · Ingo Molnar
Quoted 12 lines · Li Zefan
init_preds() allocates about 5392 bytes of memory (on x86_32) for a TRACE_EVENT. With my config, at system boot total memory occupied is:
	5392 * (642 + 15) == 3459KB
642 == cat available_events | wc -l
15 == number of dirs in events/ftrace
That's quite a lot, so we'd better defer memory allocation util it's needed, that's when filter is used.
Quoted 7 lines
Quoted 1 line
Signed-off-by Li Zefan <[email protected]>
Applied, thanks!
Quoted 1 line
tracing/kprobe needs rebase after this patch..
You mean with many probes registered it has a lot of memory

Quoted 8 lines

I think so, if filter is used.
Quoted 3 lines
footprint? Instead of a rebase a merge of tracing/core into tracing/kprobes would be less intrusive.
Yeah, I meant this patch conflicts with some patches in tracing/kprobe. :)

Sure, that should be a small change. :-)

Thank you!

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: [email protected]

tip-bot for Li Zefan

author

2009-08-31 09:01 UTC #4 lore raw

In reply to Li Zefan

Commit-ID:  8e254c1d183f0225ad21f9049641529e56cce4da
Gitweb:     http://git.kernel.org/tip/8e254c1d183f0225ad21f9049641529e56cce4da
Author:     Li Zefan <[email protected]>
AuthorDate: Mon, 31 Aug 2009 16:49:41 +0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 31 Aug 2009 10:58:08 +0200

tracing/filters: Defer pred allocation

init_preds() allocates about 5392 bytes of memory (on x86_32) for a TRACE_EVENT. With my config, at system boot total memory occupied is:

	5392 * (642 + 15) == 3459KB

642 == cat available_events | wc -l
15 == number of dirs in events/ftrace

That's quite a lot, so we'd better defer memory allocation util it's needed, that's when filter is used.

Signed-off-by Li Zefan <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Tom Zanussi <[email protected]>

Cc Masami Hiramatsu <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 include/linux/ftrace_event.h       |    1 -
 include/linux/syscalls.h           |    2 -
 include/trace/ftrace.h             |    1 -
 kernel/trace/trace_events_filter.c |   50 +++++++++++++++++++++++++++++------
 kernel/trace/trace_export.c        |    1 -
 5 files changed, 41 insertions(+), 14 deletions(-)

include/linux/ftrace_event.h +0 -1

index ace2da9..7554804 100644
@@ -133,7 +133,6 @@ struct ftrace_event_call {
 #define MAX_FILTER_PRED		32
 #define MAX_FILTER_STR_VAL	128
 
-extern int init_preds(struct ftrace_event_call *call);
 extern void destroy_preds(struct ftrace_event_call *call);
 extern int filter_match_preds(struct ftrace_event_call *call, void *rec);
 extern int filter_current_check_discard(struct ftrace_event_call *call,

include/linux/syscalls.h +0 -2

index f124c89..a8e3782 100644
@@ -177,7 +177,6 @@ static void prof_sysexit_disable_##sname(struct ftrace_event_call *event_call) \
 		event_enter_##sname.id = id;				\
 		set_syscall_enter_id(num, id);				\
 		INIT_LIST_HEAD(&event_enter_##sname.fields);		\
-		init_preds(&event_enter_##sname);			\
 		return 0;						\
 	}								\
 	TRACE_SYS_ENTER_PROFILE(sname);					\

@@ -214,7 +213,6 @@ static void prof_sysexit_disable_##sname(struct ftrace_event_call *event_call) \
 		event_exit_##sname.id = id;				\
 		set_syscall_exit_id(num, id);				\
 		INIT_LIST_HEAD(&event_exit_##sname.fields);		\
-		init_preds(&event_exit_##sname);			\
 		return 0;						\
 	}								\
 	TRACE_SYS_EXIT_PROFILE(sname);					\

include/trace/ftrace.h +0 -1

index 57c56a9..bfbc842 100644
@@ -622,7 +622,6 @@ static int ftrace_raw_init_event_##call(void)				\
 		return -ENODEV;						\
 	event_##call.id = id;						\
 	INIT_LIST_HEAD(&event_##call.fields);				\
-	init_preds(&event_##call);					\
 	return 0;							\
 }									\
 									\

kernel/trace/trace_events_filter.c +41 -9

110 lines not on this page · raw message

kernel/trace/trace_export.c +0 -1

index 029a91f..df1bf6e 100644
@@ -135,7 +135,6 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
 static int ftrace_raw_init_event_##call(void)				\
 {									\
 	INIT_LIST_HEAD(&event_##call.fields);				\
-	init_preds(&event_##call);					\
 	return 0;							\
 }									\

Frederic Weisbecker

2009-08-31 17:41 UTC #5 lore raw

In reply to Li Zefan

kernel/trace/trace_events_filter.c int apply_event_filter(struct ftrace_event_call *call, char *filter_string) · line 1123

Quoted 7 lines · Li Zefan

kernel/trace/trace_events_filter.c +3 -0

@@ -1094,6 +1118,10 @@ int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
 
 	mutex_lock(&event_mutex);
 
+	err = init_preds(call);
+	if (err)
+		goto out_unlock;

Hmm, but what happens if the filter already has its preds initialized by a previous filter?

The first thing that init_preds() does is:

filter = call->filter = kzalloc(sizeof(*filter), GFP_KERNEL);

That looks like a memory leak.

kernel/trace/trace_events_filter.c int apply_subsystem_event_filter(struct event_subsystem *system, · line 1170

Quoted 8 lines

kernel/trace/trace_events_filter.c +1 -0

 	if (!strcmp(strstrip(filter_string), "0")) {
 		filter_disable_preds(call);
 		remove_filter_string(call->filter);

@@ -1139,6 +1167,10 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
 
 	mutex_lock(&event_mutex);
 
+	err = init_subsystem_preds(system);

Whereas this one has a check:

	if (!call->filter)
		alloc...

Quoted 21 lines

kernel/trace/trace_events_filter.c +3 -0

+	if (err)
+		goto out_unlock;
+
 	if (!strcmp(strstrip(filter_string), "0")) {
 		filter_free_subsystem_preds(system, FILTER_DISABLE_ALL);
 		remove_filter_string(system->filter);

kernel/trace/trace_export.c +0 -1

index 029a91f..df1bf6e 100644
@@ -135,7 +135,6 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
 static int ftrace_raw_init_event_##call(void)				\
 {									\
 	INIT_LIST_HEAD(&event_##call.fields);				\
-	init_preds(&event_##call);					\
 	return 0;							\
 }									\

Signature · 1 line

1.6.3

Other than the above doubts, that looks good.

Li Zefan

author

2009-09-01 00:52 UTC #6 lore raw

Quoted 13 lines

kernel/trace/trace_events_filter.c int apply_event_filter(struct ftrace_event_call *call, char *filter_string) · line 1123
Quoted 7 lines
kernel/trace/trace_events_filter.c +3 -0
@@ -1094,6 +1118,10 @@ int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
 
 	mutex_lock(&event_mutex);
 
+	err = init_preds(call);
+	if (err)
+		goto out_unlock;
Hmm, but what happens if the filter already has its preds initialized by a previous filter?

Quoted 6 lines

The first thing that init_preds() does is:
filter = call->filter = kzalloc(sizeof(*filter), GFP_KERNEL);
That looks like a memory leak.

Oops! I thought I had added a check in it.

Thanks.

Li Zefan

author

2009-09-01 05:32 UTC #7 lore raw

PATCH 1/1 tracing/filters: Defer pred allocation, fix memory leak

In reply to Frederic Weisbecker

Check if we have already allocated memory for filter preds.

Spotted-by Frederic Weisbecker <[email protected]>

Signed-off-by Li Zefan <[email protected]>

---
 kernel/trace/trace_events_filter.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

kernel/trace/trace_events_filter.c +6 -5

index c6b2edf..93660fb 100644
@@ -409,6 +409,9 @@ static int init_preds(struct ftrace_event_call *call)
 	struct filter_pred *pred;
 	int i;
 
+	if (call->filter)
+		return 0;
+
 	filter = call->filter = kzalloc(sizeof(*filter), GFP_KERNEL);
 	if (!call->filter)
 		return -ENOMEM;

@@ -447,11 +450,9 @@ static int init_subsystem_preds(struct event_subsystem *system)
 		if (strcmp(call->system, system->name) != 0)
 			continue;
 
-		if (!call->filter) {
-			err = init_preds(call);
-			if (err)
-				return err;
-		}
+		err = init_preds(call);
+		if (err)
+			return err;
 	}
 
 	return 0;

Signature · 1 line

1.6.3

Frederic Weisbecker

Acked-by on v1 1/1

2009-09-01 09:32 UTC #8 lore raw

Re: [PATCH] tracing/filters: Defer pred allocation, fix memory leak

Quoted 4 lines · Li Zefan

Check if we have already allocated memory for filter preds.
Spotted-by Frederic Weisbecker <[email protected]>
Signed-off-by Li Zefan <[email protected]>

Acked-by Frederic Weisbecker <[email protected]>

Quoted 36 lines

---
 kernel/trace/trace_events_filter.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

kernel/trace/trace_events_filter.c +6 -5

index c6b2edf..93660fb 100644
@@ -409,6 +409,9 @@ static int init_preds(struct ftrace_event_call *call)
 	struct filter_pred *pred;
 	int i;
 
+	if (call->filter)
+		return 0;
+
 	filter = call->filter = kzalloc(sizeof(*filter), GFP_KERNEL);
 	if (!call->filter)
 		return -ENOMEM;

@@ -447,11 +450,9 @@ static int init_subsystem_preds(struct event_subsystem *system)
 		if (strcmp(call->system, system->name) != 0)
 			continue;
 
-		if (!call->filter) {
-			err = init_preds(call);
-			if (err)
-				return err;
-		}
+		err = init_preds(call);
+		if (err)
+			return err;
 	}
 
 	return 0;

Signature · 1 line

1.6.3

tip-bot for Li Zefan

author

2009-09-06 04:37 UTC #9 lore raw

[tip:tracing/core] tracing/filters: Defer pred allocation, fix memory leak

In reply to Li Zefan

Commit-ID:  c58b43218c1a04a0bcf338ea47406c759ac28e11
Gitweb:     http://git.kernel.org/tip/c58b43218c1a04a0bcf338ea47406c759ac28e11
Author:     Li Zefan <[email protected]>
AuthorDate: Tue, 1 Sep 2009 13:31:38 +0800
Committer:  Frederic Weisbecker <[email protected]>
CommitDate: Fri, 4 Sep 2009 23:22:33 +0200

tracing/filters: Defer pred allocation, fix memory leak

The predicates of an event and their filter structure are allocated when we create an event filter for the first time.

These objects must be created once but each time we come with a new filter, we overwrite such pre-existing allocation, if any.

Thus, this patch checks if the filter has already been allocated before going ahead.

Spotted-by Frederic Weisbecker <[email protected]>

Signed-off-by Li Zefan <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Tom Zanussi <[email protected]>

Cc Masami Hiramatsu <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

---
 kernel/trace/trace_events_filter.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

kernel/trace/trace_events_filter.c +6 -5

index c6b2edf..93660fb 100644
@@ -409,6 +409,9 @@ static int init_preds(struct ftrace_event_call *call)
 	struct filter_pred *pred;
 	int i;
 
+	if (call->filter)
+		return 0;
+
 	filter = call->filter = kzalloc(sizeof(*filter), GFP_KERNEL);
 	if (!call->filter)
 		return -ENOMEM;

@@ -447,11 +450,9 @@ static int init_subsystem_preds(struct event_subsystem *system)
 		if (strcmp(call->system, system->name) != 0)
 			continue;
 
-		if (!call->filter) {
-			err = init_preds(call);
-			if (err)
-				return err;
-		}
+		err = init_preds(call);
+		if (err)
+			return err;
 	}
 
 	return 0;

Tom Zanussi

Acked-by on v1 1/1

2009-09-01 03:28 UTC #10 lore raw

In reply to Li Zefan

Quoted 6 lines · Li Zefan

init_preds() allocates about 5392 bytes of memory (on x86_32) for a TRACE_EVENT. With my config, at system boot total memory occupied is:
	5392 * (642 + 15) == 3459KB

Quoted 7 lines

642 == cat available_events | wc -l
15 == number of dirs in events/ftrace
That's quite a lot, so we'd better defer memory allocation util it's needed, that's when filter is used.
Signed-off-by Li Zefan <[email protected]>

Other than the leak mentioned by Frederic, it looks good to me.

Acked-by Tom Zanussi <[email protected]>