tracing/syscalls: Annotate raw_init function with __init

PATCH RESEND In review Li Zefan 3 participants 12 messages Jul 25, 2013 – Jul 29, 2013 Tracing

Share & tools

Li Zefan

author

2013-07-25 02:28 UTC #0 lore raw

PATCH RESEND 1/2 tracing/syscalls: Annotate raw_init function with __init

init_syscall_trace() can be called during kernel bootup only, so we can mark it and the functions it calls as __init.

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

---

this was sent 4 months ago.

---
 kernel/trace/trace_syscalls.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

kernel/trace/trace_syscalls.c +5 -5

index 8f2ac73..1de2ba6 100644
@@ -200,8 +200,8 @@ extern char *__bad_type_size(void);
 		#type, #name, offsetof(typeof(trace), name),		\
 		sizeof(trace.name), is_signed_type(type)
 
-static
-int  __set_enter_print_fmt(struct syscall_metadata *entry, char *buf, int len)
+static int __init
+__set_enter_print_fmt(struct syscall_metadata *entry, char *buf, int len)
 {
 	int i;
 	int pos = 0;

@@ -228,7 +228,7 @@ int  __set_enter_print_fmt(struct syscall_metadata *entry, char *buf, int len)
 	return pos;
 }
 
-static int set_syscall_print_fmt(struct ftrace_event_call *call)
+static int __init set_syscall_print_fmt(struct ftrace_event_call *call)
 {
 	char *print_fmt;
 	int len;

@@ -253,7 +253,7 @@ static int set_syscall_print_fmt(struct ftrace_event_call *call)
 	return 0;
 }
 
-static void free_syscall_print_fmt(struct ftrace_event_call *call)
+static void __init free_syscall_print_fmt(struct ftrace_event_call *call)
 {
 	struct syscall_metadata *entry = call->data;

@@ -446,7 +446,7 @@ static void unreg_event_syscall_exit(struct ftrace_event_file *file,
 	mutex_unlock(&syscall_trace_lock);
 }
 
-static int init_syscall_trace(struct ftrace_event_call *call)
+static int __init init_syscall_trace(struct ftrace_event_call *call)
 {
 	int id;
 	int num;

Signature · 1 line

1.8.0.2

Steven Rostedt

maintainer

2013-07-26 14:23 UTC #11 lore raw

In reply to Li Zefan

Quoted 8 lines · Li Zefan

init_syscall_trace() can be called during kernel bootup only, so we can mark it and the functions it calls as __init.
Signed-off-by Li Zefan <[email protected]>
---
this was sent 4 months ago.

And it was still in my todo list :-/

Sorry, it was lost in the noise, as there's lots in my todo list, and things that are marked for cleanup got pushed to the bottom.

I'll add this to my 3.12 queue.

Thanks!

-- Steve

Steven Rostedt

maintainer

2013-07-26 15:09 UTC #2 lore raw

Re: [PATCH 2/2][RESEND] tracing: Shrink the size of struct ftrace_event_field

In reply to Li Zefan

kernel/trace/trace_events.c static int __trace_define_field(struct list_head *head, const char *type, · line 112

Quoted 9 lines · Li Zefan

struct event_filter {

kernel/trace/trace_events.c +0 -0

index 53582e9..67aee85 100644
@@ -111,6 +111,11 @@ static int __trace_define_field(struct list_head *head, const char *type,
 	field->size = size;
 	field->is_signed = is_signed;

I think we should just change is_signed to bool. At least the parameter. Or we can make the assignment: field->is_signed = !!is_signed; and nuke the check below.

kernel/trace/trace_events.c static int __trace_define_field(struct list_head *head, const char *type, · line 117

Quoted 5 lines

kernel/trace/trace_events.c +4 -0

+	WARN_ON(offset >= (1 << 12));
+	WARN_ON(size >= (1 << 12));
+	WARN_ON(is_signed >= (1 << 1));
+	WARN_ON(field->filter_type >= (1 << 4));

Note, the test for field->filter_type is wrong.

We should make a helper macro:

#define VERIFY_SIZE(type) WARN_ON(type > field->type)

and then have:

	VERIFY_SIZE(offset);
	VERIFY_SIZE(size);
	VERIFY_SIZE(filter_type);

-- Steve

Quoted 5 lines

kernel/trace/trace_events.c +1 -1

+
 	list_add(&field->link, head);
 
 	return 0;
-- 1.8.0.2

Li Zefan

author

2013-07-27 03:14 UTC #3 lore raw

Re: [PATCH 2/2][RESEND] tracing: Shrink the size of struct ftrace_event_field

Quoted 8 lines

kernel/trace/trace_events.c static int __trace_define_field(struct list_head *head, const char *type, · line 112
Quoted 3 lines
kernel/trace/trace_events.c +0 -0
@@ -111,6 +111,11 @@ static int __trace_define_field(struct list_head *head, const char *type,
 	field->size = size;
 	field->is_signed = is_signed;
I think we should just change is_signed to bool. At least the parameter. Or we can make the assignment: field->is_signed = !!is_signed; and nuke the check below.

Changing field->is_signed to bool won't shrink the size of the struct. I prefer: field->is_signed = !!is_signed.

Quoted 8 lines

kernel/trace/trace_events.c static int __trace_define_field(struct list_head *head, const char *type, · line 117
Quoted 5 lines
kernel/trace/trace_events.c +4 -0
+	WARN_ON(offset >= (1 << 12));
+	WARN_ON(size >= (1 << 12));
+	WARN_ON(is_signed >= (1 << 1));
+	WARN_ON(field->filter_type >= (1 << 4));
Note, the test for field->filter_type is wrong.

oops.

Quoted 4 lines

We should make a helper macro:
#define VERIFY_SIZE(type) WARN_ON(type > field->type)

Much better!

Quoted 5 lines

and then have:

	VERIFY_SIZE(offset);
	VERIFY_SIZE(size);
	VERIFY_SIZE(filter_type);

Li Zefan

author

2013-07-27 03:38 UTC #4 lore raw

PATCH v2 2/2 tracing: Shrink the size of struct ftrace_event_field

In reply to Steven Rostedt

Use bit fields, and the size of struct ftrace_event_field can be shrunk from 48 bytes to 40 bytes on 64bit kernel.

slab_name active_obj nr_obj size obj_per_slab
---------------------------------------------

ftrace_event_field   1105   1105     48   85  (before)
ftrace_event_field   1224   1224     40  102  (after)

This saves a few Kbytes: (1224 * 40) - (1105 * 48) = 4080

v2:
- use !!is_signed, and nuke the check on this field.
- use a different way to detect overflow. (both suggested by Steven)

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

---
 kernel/trace/trace.h        |  8 ++++----
 kernel/trace/trace_events.c | 14 ++++++++++----
 2 files changed, 14 insertions(+), 8 deletions(-)

kernel/trace/trace.h +4 -4

index 4a4f6e1..3e8c97f 100644
@@ -904,10 +904,10 @@ struct ftrace_event_field {
 	struct list_head	link;
 	const char		*name;
 	const char		*type;
-	int			filter_type;
-	int			offset;
-	int			size;
-	int			is_signed;
+	unsigned int		filter_type:4;
+	unsigned int		offset:12;
+	unsigned int		size:12;
+	unsigned int		is_signed:1;
 };
 
 struct event_filter {

kernel/trace/trace_events.c +10 -4

index 7d85429..d72694d 100644
@@ -106,6 +106,9 @@ trace_find_event_field(struct ftrace_event_call *call, char *name)
 	return __find_event_field(head, name);
 }
 
+/* detect bit-field overflow */
+#define VERIFY_SIZE(type) WARN_ON(type > field->type)
+
 static int __trace_define_field(struct list_head *head, const char *type,
 				const char *name, int offset, int size,
 				int is_signed, int filter_type)

@@ -120,13 +123,16 @@ static int __trace_define_field(struct list_head *head, const char *type,
 	field->type = type;
 
 	if (filter_type == FILTER_OTHER)
-		field->filter_type = filter_assign_type(type);
-	else
-		field->filter_type = filter_type;
+		filter_type = filter_assign_type(type);
 
+	field->filter_type = filter_type;
 	field->offset = offset;
 	field->size = size;
-	field->is_signed = is_signed;
+	field->is_signed = !!is_signed;
+
+	VERIFY_SIZE(filter_type);
+	VERIFY_SIZE(offset);
+	VERIFY_SIZE(size);
 
 	list_add(&field->link, head);

Signature · 1 line

1.8.0.2

Steven Rostedt

maintainer

2013-07-27 03:47 UTC #5 lore raw

Re: [PATCH v2 2/2] tracing: Shrink the size of struct ftrace_event_field

kernel/trace/trace_events.c trace_find_event_field(struct ftrace_event_call *call, char *name) · line 111

Quoted 12 lines · Li Zefan

struct event_filter {

kernel/trace/trace_events.c +3 -0

index 7d85429..d72694d 100644
@@ -106,6 +106,9 @@ trace_find_event_field(struct ftrace_event_call *call, char *name)
 	return __find_event_field(head, name);
 }
 
+/* detect bit-field overflow */
+#define VERIFY_SIZE(type) WARN_ON(type > field->type)
+

One small nit. Move this macro definition into the function itself, right above the macro usage. That way it will be much easier to review in the future, as people don't need to go search for VERIFY_SIZE(). It will be right there with the usage. The aesthetics may be a bit off, but at least the code will be obvious at first glance at what is happening, and I think that's more important than the "look" of the code.

Also, it will be obvious that the variable name must match the field name.

Thanks,

-- Steve

Quoted 24 lines

kernel/trace/trace_events.c +7 -4

 static int __trace_define_field(struct list_head *head, const char *type,
 				const char *name, int offset, int size,
 				int is_signed, int filter_type)

@@ -120,13 +123,16 @@ static int __trace_define_field(struct list_head *head, const char *type,
 	field->type = type;
 
 	if (filter_type == FILTER_OTHER)
-		field->filter_type = filter_assign_type(type);
-	else
-		field->filter_type = filter_type;
+		filter_type = filter_assign_type(type);
 
+	field->filter_type = filter_type;
 	field->offset = offset;
 	field->size = size;
-	field->is_signed = is_signed;
+	field->is_signed = !!is_signed;
+
+	VERIFY_SIZE(filter_type);
+	VERIFY_SIZE(offset);
+	VERIFY_SIZE(size);
 
 	list_add(&field->link, head);

Borislav Petkov

2013-07-27 08:45 UTC #6 lore raw

Re: [PATCH v2 2/2] tracing: Shrink the size of struct ftrace_event_field

Quoted 41 lines · Steven Rostedt

kernel/trace/trace_events.c trace_find_event_field(struct ftrace_event_call *call, char *name) · line 111
Quoted 12 lines · Li Zefan
struct event_filter {
kernel/trace/trace_events.c +3 -0
index 7d85429..d72694d 100644
@@ -106,6 +106,9 @@ trace_find_event_field(struct ftrace_event_call *call, char *name)
 	return __find_event_field(head, name);
 }
 
+/* detect bit-field overflow */
+#define VERIFY_SIZE(type) WARN_ON(type > field->type)
+
One small nit. Move this macro definition into the function itself, right above the macro usage. That way it will be much easier to review in the future, as people don't need to go search for VERIFY_SIZE(). It will be right there with the usage. The aesthetics may be a bit off, but at least the code will be obvious at first glance at what is happening, and I think that's more important than the "look" of the code.
Also, it will be obvious that the variable name must match the field name.
Thanks,
-- Steve
Quoted 12 lines
kernel/trace/trace_events.c +1 -3
 static int __trace_define_field(struct list_head *head, const char *type,
 				const char *name, int offset, int size,
 				int is_signed, int filter_type)
@@ -120,13 +123,16 @@ static int __trace_define_field(struct list_head *head, const char *type,
 	field->type = type;
 
 	if (filter_type == FILTER_OTHER)
-		field->filter_type = filter_assign_type(type);
-	else
-		field->filter_type = filter_type;
+		filter_type = filter_assign_type(type);
 

Quoted 9 lines

kernel/trace/trace_events.c static int __trace_define_field(struct list_head *head, const char *type, · line 135
Quoted 9 lines
kernel/trace/trace_events.c +6 -1
+	field->filter_type = filter_type;
 	field->offset = offset;
 	field->size = size;
-	field->is_signed = is_signed;
+	field->is_signed = !!is_signed;
+
+	VERIFY_SIZE(filter_type);
+	VERIFY_SIZE(offset);
+	VERIFY_SIZE(size);

Isn't this wrap-a-macro-with-another-more-obscure-macro not a bit too much?

I mean,
WARN_ON(filter_type > field->filter_type)

is much more readable than VERIFY_SIZE IMO.

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Steven Rostedt

maintainer

2013-07-27 11:35 UTC #7 lore raw

Re: [PATCH v2 2/2] tracing: Shrink the size of struct ftrace_event_field

Quoted 14 lines · Borislav Petkov

Quoted 10 lines
kernel/trace/trace_events.c static int __trace_define_field(struct list_head *head, const char *type, · line 135
Quoted 10 lines
kernel/trace/trace_events.c +6 -1
+	field->filter_type = filter_type;
 	field->offset = offset;
 	field->size = size;
-	field->is_signed = is_signed;
+	field->is_signed = !!is_signed;
+
+	VERIFY_SIZE(filter_type);
+	VERIFY_SIZE(offset);
+	VERIFY_SIZE(size);
Isn't this wrap-a-macro-with-another-more-obscure-macro not a bit too much?

Quoted 4 lines

I mean,
WARN_ON(filter_type > field->filter_type)
is much more readable than VERIFY_SIZE IMO.

Fair enough. My first version of the test was a bit more complex, and then I simplified it, but kept the macro. Either is fine to me. But yeah, I can see the point that the test isn't that complex, and there's only three tests. If there were more or the test a bit more complex, then I would definitely want the macro.

But you know me. I LOVE macros!

-- Steve

Borislav Petkov

2013-07-27 16:19 UTC #8 lore raw

Re: [PATCH v2 2/2] tracing: Shrink the size of struct ftrace_event_field

Quoted 1 line · Steven Rostedt

But you know me. I LOVE macros!

I'll say.

Btw, I wouldn't wonder if you start sending patches for the gcc C preprocessor because it evaluates your macro voodoo too slow.

:-)

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Li Zefan

author

2013-07-29 02:12 UTC #10 lore raw

Re: [PATCH v2 2/2] tracing: Shrink the size of struct ftrace_event_field

In reply to Borislav Petkov

Quoted 25 lines

Quoted 12 lines
Quoted 12 lines
static int __trace_define_field(struct list_head *head, const char *type,
kernel/trace/trace_events.c +1 -3
 				const char *name, int offset, int size,
 				int is_signed, int filter_type)
@@ -120,13 +123,16 @@ static int __trace_define_field(struct list_head *head, const char *type,
 	field->type = type;
 
 	if (filter_type == FILTER_OTHER)
-		field->filter_type = filter_assign_type(type);
-	else
-		field->filter_type = filter_type;
+		filter_type = filter_assign_type(type);
 
Quoted 9 lines
kernel/trace/trace_events.c static int __trace_define_field(struct list_head *head, const char *type, · line 135
Quoted 9 lines
kernel/trace/trace_events.c +6 -1
+	field->filter_type = filter_type;
 	field->offset = offset;
 	field->size = size;
-	field->is_signed = is_signed;
+	field->is_signed = !!is_signed;
+
+	VERIFY_SIZE(filter_type);
+	VERIFY_SIZE(offset);
+	VERIFY_SIZE(size);
Isn't this wrap-a-macro-with-another-more-obscure-macro not a bit too much?

Quoted 5 lines

I mean,
WARN_ON(filter_type > field->filter_type)
is much more readable than VERIFY_SIZE IMO.

Oh, right. Using macro is a bit excessive here.

Li Zefan

author

2013-07-29 02:14 UTC #9 lore raw

PATCH v3 2/2 tracing: Shrink the size of struct ftrace_event_field

In reply to Steven Rostedt

Use bit fields, and the size of struct ftrace_event_field can be shrunk from 48 bytes to 40 bytes on 64bit kernel.

slab_name active_obj nr_obj size obj_per_slab
---------------------------------------------

ftrace_event_field   1105   1105     48   85  (before)
ftrace_event_field   1224   1224     40  102  (after)

This saves a few Kbytes: (1224 * 40) - (1105 * 48) = 4080

v2:
- use !!is_signed, and nuke the check on this field.
- use a different way to detect overflow. (both suggested by Steven)

v3:
- use plain WARN_ON() instead of a macro. (suggeusted by Borislav)

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

---
 kernel/trace/trace.h        |  8 ++++----
 kernel/trace/trace_events.c | 12 ++++++++----
 2 files changed, 12 insertions(+), 8 deletions(-)

kernel/trace/trace.h +4 -4

index 4a4f6e1..3e8c97f 100644
@@ -904,10 +904,10 @@ struct ftrace_event_field {
 	struct list_head	link;
 	const char		*name;
 	const char		*type;
-	int			filter_type;
-	int			offset;
-	int			size;
-	int			is_signed;
+	unsigned int		filter_type:4;
+	unsigned int		offset:12;
+	unsigned int		size:12;
+	unsigned int		is_signed:1;
 };
 
 struct event_filter {

kernel/trace/trace_events.c +8 -4

index 7d85429..6509afe 100644
@@ -120,13 +120,17 @@ static int __trace_define_field(struct list_head *head, const char *type,
 	field->type = type;
 
 	if (filter_type == FILTER_OTHER)
-		field->filter_type = filter_assign_type(type);
-	else
-		field->filter_type = filter_type;
+		filter_type = filter_assign_type(type);
 
+	field->filter_type = filter_type;
 	field->offset = offset;
 	field->size = size;
-	field->is_signed = is_signed;
+	field->is_signed = !!is_signed;
+
+	/* detect bit-field overflow */
+	WARN_ON(filter_type > field->filter_type);
+	WARN_ON(offset > field->offset);
+	WARN_ON(size > field->size);
 
 	list_add(&field->link, head);

Signature · 1 line

1.8.0.2