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

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

[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 <lizefan@huawei.com>
---

this was sent 4 months ago.

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

diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 8f2ac73..1de2ba6 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -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;
-- 
1.8.0.2

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

[Li Zefan]

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

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 kernel/trace/trace.h        | 8 ++++----
 kernel/trace/trace_events.c | 5 +++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 9e01458..a3365a4 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -916,10 +916,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 {
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 53582e9..67aee85 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -111,6 +111,11 @@ static int __trace_define_field(struct list_head *head, const char *type,
 	field->size = size;
 	field->is_signed = is_signed;
 
+	WARN_ON(offset >= (1 << 12));
+	WARN_ON(size >= (1 << 12));
+	WARN_ON(is_signed >= (1 << 1));
+	WARN_ON(field->filter_type >= (1 << 4));
+
 	list_add(&field->link, head);
 
 	return 0;
-- 1.8.0.2

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

[Steven Rostedt]

On Thu, 2013-07-25 at 10:27 +0800, Li Zefan wrote:
> 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 <lizefan@huawei.com>
> ---
> 
> 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

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

[Steven Rostedt]

On Thu, 2013-07-25 at 10:28 +0800, Li Zefan wrote:
>  
>  struct event_filter {
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 53582e9..67aee85 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -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.

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

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

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

[Li Zefan]

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

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

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

Much better!

> and then have:
> 
> 	VERIFY_SIZE(offset);
> 	VERIFY_SIZE(size);
> 	VERIFY_SIZE(filter_type);

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

[Li Zefan]

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 <lizefan@huawei.com>
---
 kernel/trace/trace.h        |  8 ++++----
 kernel/trace/trace_events.c | 14 ++++++++++----
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 4a4f6e1..3e8c97f 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -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 {
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 7d85429..d72694d 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -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);
 
-- 
1.8.0.2

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

[Steven Rostedt]

On Sat, 2013-07-27 at 11:32 +0800, Li Zefan wrote:
 
>  struct event_filter {
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 7d85429..d72694d 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -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

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

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

[Borislav Petkov]

On Fri, Jul 26, 2013 at 11:47:22PM -0400, Steven Rostedt wrote:
> On Sat, 2013-07-27 at 11:32 +0800, Li Zefan wrote:
>  
> >  struct event_filter {
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 7d85429..d72694d 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -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
> 
> >  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);

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.

-- 
Regards/Gruss,
    Boris.

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

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

[Steven Rostedt]

On Sat, 2013-07-27 at 10:45 +0200, Borislav Petkov wrote:
>  
> > > +	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.

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

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

[Borislav Petkov]

On Sat, Jul 27, 2013 at 07:35:38AM -0400, Steven Rostedt wrote:
> 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.

:-)

-- 
Regards/Gruss,
    Boris.

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

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

[Li Zefan]

>>>  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);
> 
> 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.
> 

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

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

[Li Zefan]

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 <lizefan@huawei.com>
---
 kernel/trace/trace.h        |  8 ++++----
 kernel/trace/trace_events.c | 12 ++++++++----
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 4a4f6e1..3e8c97f 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -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 {
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 7d85429..6509afe 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -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);
 
-- 
1.8.0.2