There are for 4 fields in trace_event_functions with the same type of
trace_print_func. Initialize them in register_trace_event() one by one
looks redundant.
Let's take advantage of union to simplify the procedure.
Signed-off-by: Wei Yang <[email protected]>
---
include/linux/trace_events.h | 13 +++++++++----
kernel/trace/trace_output.c | 14 +++++---------
2 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 5c6943354049..1a421246f4a2 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -122,10 +122,15 @@ typedef enum print_line_t (*trace_print_func)(struct trace_iterator *iter,
int flags, struct trace_event *event);
struct trace_event_functions {
- trace_print_func trace;
- trace_print_func raw;
- trace_print_func hex;
- trace_print_func binary;
+ union {
+ struct {
+ trace_print_func trace;
+ trace_print_func raw;
+ trace_print_func hex;
+ trace_print_func binary;
+ };
+ trace_print_func print_funcs[4];
+ };
};
struct trace_event {
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index 73976de7f8cc..47bf9f042b97 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -728,7 +728,7 @@ void trace_event_read_unlock(void)
int register_trace_event(struct trace_event *event)
{
unsigned key;
- int ret = 0;
+ int i, ret = 0;
down_write(&trace_event_sem);
@@ -770,14 +770,10 @@ int register_trace_event(struct trace_event *event)
goto out;
}
- if (event->funcs->trace == NULL)
- event->funcs->trace = trace_nop_print;
- if (event->funcs->raw == NULL)
- event->funcs->raw = trace_nop_print;
- if (event->funcs->hex == NULL)
- event->funcs->hex = trace_nop_print;
- if (event->funcs->binary == NULL)
- event->funcs->binary = trace_nop_print;
+ for (i = 0; i < ARRAY_SIZE(event->funcs->print_funcs); i++) {
+ if (!event->funcs->print_funcs[i])
+ event->funcs->print_funcs[i] = trace_nop_print;
+ }
key = event->type & (EVENT_HASHSIZE - 1);
--
2.20.1 (Apple Git-117)
The value to be used and compared in trace_search_list() is "last + 1".
Let's just define next to be "last + 1" instead of doing the addition
each time.
Signed-off-by: Wei Yang <[email protected]>
---
kernel/trace/trace_output.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index 47bf9f042b97..b704b3ef4264 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -675,11 +675,11 @@ static LIST_HEAD(ftrace_event_list);
static int trace_search_list(struct list_head **list)
{
struct trace_event *e;
- int last = __TRACE_LAST_TYPE;
+ int next = __TRACE_LAST_TYPE + 1;
if (list_empty(&ftrace_event_list)) {
*list = &ftrace_event_list;
- return last + 1;
+ return next;
}
/*
@@ -687,17 +687,17 @@ static int trace_search_list(struct list_head **list)
* lets see if somebody freed one.
*/
list_for_each_entry(e, &ftrace_event_list, list) {
- if (e->type != last + 1)
+ if (e->type != next)
break;
- last++;
+ next++;
}
/* Did we used up all 65 thousand events??? */
- if ((last + 1) > TRACE_EVENT_TYPE_MAX)
+ if (next > TRACE_EVENT_TYPE_MAX)
return 0;
*list = &e->list;
- return last + 1;
+ return next;
}
void trace_event_read_lock(void)
--
2.20.1 (Apple Git-117)
Static defined trace_event->type stops at (__TRACE_LAST_TYPE - 1) and
dynamic trace_event->type starts from (__TRACE_LAST_TYPE + 1).
To save one trace_event->type index, let's use __TRACE_LAST_TYPE.
Signed-off-by: Wei Yang <[email protected]>
---
kernel/trace/trace_output.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index b704b3ef4264..818f0c9d10c5 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -20,7 +20,7 @@ DECLARE_RWSEM(trace_event_sem);
static struct hlist_head event_hash[EVENT_HASHSIZE] __read_mostly;
-static int next_event_type = __TRACE_LAST_TYPE + 1;
+static int next_event_type = __TRACE_LAST_TYPE;
enum print_line_t trace_print_bputs_msg_only(struct trace_iterator *iter)
{
@@ -675,7 +675,7 @@ static LIST_HEAD(ftrace_event_list);
static int trace_search_list(struct list_head **list)
{
struct trace_event *e;
- int next = __TRACE_LAST_TYPE + 1;
+ int next = __TRACE_LAST_TYPE;
if (list_empty(&ftrace_event_list)) {
*list = &ftrace_event_list;
--
2.20.1 (Apple Git-117)
Currently we have following call flow:
tracer_init_tracefs()
tracing_init_dentry()
event_trace_init()
tracing_init_dentry()
This shows tracing_init_dentry() is called twice in this flow and this
is not necessary.
Let's remove the second one when it is for sure be properly initialized.
Signed-off-by: Wei Yang <[email protected]>
---
kernel/trace/trace_events.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 8b3aa57dcea6..76879b29cf33 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -3434,7 +3434,6 @@ early_initcall(event_trace_enable_again);
__init int event_trace_init(void)
{
struct trace_array *tr;
- struct dentry *d_tracer;
struct dentry *entry;
int ret;
@@ -3442,10 +3441,6 @@ __init int event_trace_init(void)
if (!tr)
return -ENODEV;
- d_tracer = tracing_init_dentry();
- if (IS_ERR(d_tracer))
- return 0;
-
entry = tracefs_create_file("available_events", 0444, NULL,
tr, &ftrace_avail_fops);
if (!entry)
--
2.20.1 (Apple Git-117)
tracing_init_dentry() has two types of return value:
* NULL if succeed
* IS_ERR() if failed
If the function return error, the following check would return from the
function. So we are sure d_tracer passed in here is NULL.
This is a preparation for following cleanup.
Signed-off-by: Wei Yang <[email protected]>
---
kernel/trace/trace_events.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index f6f55682d3e2..8b3aa57dcea6 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -3446,7 +3446,7 @@ __init int event_trace_init(void)
if (IS_ERR(d_tracer))
return 0;
- entry = tracefs_create_file("available_events", 0444, d_tracer,
+ entry = tracefs_create_file("available_events", 0444, NULL,
tr, &ftrace_avail_fops);
if (!entry)
pr_warn("Could not create tracefs 'available_events' entry\n");
@@ -3457,7 +3457,7 @@ __init int event_trace_init(void)
if (trace_define_common_fields())
pr_warn("tracing: Failed to allocate common fields");
- ret = early_event_add_tracer(d_tracer, tr);
+ ret = early_event_add_tracer(NULL, tr);
if (ret)
return ret;
--
2.20.1 (Apple Git-117)
On Fri, 3 Jul 2020 10:06:08 +0800
Wei Yang <[email protected]> wrote:
> There are for 4 fields in trace_event_functions with the same type of
> trace_print_func. Initialize them in register_trace_event() one by one
> looks redundant.
I have mixed emotions about this patch. Yeah, it consolidates it a bit,
but it also makes it less easy to know what it is doing.
All this patch is doing is optimizing the initialization path, which is
done once when an event is registered. It's error prone, as you would
need to make sure to map the array with the functions. Something like
this is only reasonable if it is used more often, which here it's a
single spot.
So no, I can't take this patch.
-- Steve
>
> Let's take advantage of union to simplify the procedure.
>
> Signed-off-by: Wei Yang <[email protected]>
> ---
> include/linux/trace_events.h | 13 +++++++++----
> kernel/trace/trace_output.c | 14 +++++---------
> 2 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> index 5c6943354049..1a421246f4a2 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -122,10 +122,15 @@ typedef enum print_line_t (*trace_print_func)(struct trace_iterator *iter,
> int flags, struct trace_event *event);
>
> struct trace_event_functions {
> - trace_print_func trace;
> - trace_print_func raw;
> - trace_print_func hex;
> - trace_print_func binary;
> + union {
> + struct {
> + trace_print_func trace;
> + trace_print_func raw;
> + trace_print_func hex;
> + trace_print_func binary;
> + };
> + trace_print_func print_funcs[4];
> + };
> };
>
> struct trace_event {
> diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
> index 73976de7f8cc..47bf9f042b97 100644
> --- a/kernel/trace/trace_output.c
> +++ b/kernel/trace/trace_output.c
> @@ -728,7 +728,7 @@ void trace_event_read_unlock(void)
> int register_trace_event(struct trace_event *event)
> {
> unsigned key;
> - int ret = 0;
> + int i, ret = 0;
>
> down_write(&trace_event_sem);
>
> @@ -770,14 +770,10 @@ int register_trace_event(struct trace_event *event)
> goto out;
> }
>
> - if (event->funcs->trace == NULL)
> - event->funcs->trace = trace_nop_print;
> - if (event->funcs->raw == NULL)
> - event->funcs->raw = trace_nop_print;
> - if (event->funcs->hex == NULL)
> - event->funcs->hex = trace_nop_print;
> - if (event->funcs->binary == NULL)
> - event->funcs->binary = trace_nop_print;
> + for (i = 0; i < ARRAY_SIZE(event->funcs->print_funcs); i++) {
> + if (!event->funcs->print_funcs[i])
> + event->funcs->print_funcs[i] = trace_nop_print;
> + }
>
> key = event->type & (EVENT_HASHSIZE - 1);
>
On Fri, 3 Jul 2020 10:06:09 +0800
Wei Yang <[email protected]> wrote:
> The value to be used and compared in trace_search_list() is "last + 1".
> Let's just define next to be "last + 1" instead of doing the addition
> each time.
Yeah, this is a nice clean up. I'll take this one.
-- Steve
>
> Signed-off-by: Wei Yang <[email protected]>
> ---
> kernel/trace/trace_output.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
> index 47bf9f042b97..b704b3ef4264 100644
> --- a/kernel/trace/trace_output.c
> +++ b/kernel/trace/trace_output.c
> @@ -675,11 +675,11 @@ static LIST_HEAD(ftrace_event_list);
> static int trace_search_list(struct list_head **list)
> {
> struct trace_event *e;
> - int last = __TRACE_LAST_TYPE;
> + int next = __TRACE_LAST_TYPE + 1;
>
> if (list_empty(&ftrace_event_list)) {
> *list = &ftrace_event_list;
> - return last + 1;
> + return next;
> }
>
> /*
> @@ -687,17 +687,17 @@ static int trace_search_list(struct list_head **list)
> * lets see if somebody freed one.
> */
> list_for_each_entry(e, &ftrace_event_list, list) {
> - if (e->type != last + 1)
> + if (e->type != next)
> break;
> - last++;
> + next++;
> }
>
> /* Did we used up all 65 thousand events??? */
> - if ((last + 1) > TRACE_EVENT_TYPE_MAX)
> + if (next > TRACE_EVENT_TYPE_MAX)
> return 0;
>
> *list = &e->list;
> - return last + 1;
> + return next;
> }
>
> void trace_event_read_lock(void)
On Fri, 3 Jul 2020 10:06:10 +0800
Wei Yang <[email protected]> wrote:
> Static defined trace_event->type stops at (__TRACE_LAST_TYPE - 1) and
> dynamic trace_event->type starts from (__TRACE_LAST_TYPE + 1).
>
> To save one trace_event->type index, let's use __TRACE_LAST_TYPE.
When I wrote this code, I purposely had a gap there. But I guess it's
not really needed. I'll take your patch.
Thanks,
-- Steve
>
> Signed-off-by: Wei Yang <[email protected]>
> ---
> kernel/trace/trace_output.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
> index b704b3ef4264..818f0c9d10c5 100644
> --- a/kernel/trace/trace_output.c
> +++ b/kernel/trace/trace_output.c
> @@ -20,7 +20,7 @@ DECLARE_RWSEM(trace_event_sem);
>
> static struct hlist_head event_hash[EVENT_HASHSIZE] __read_mostly;
>
> -static int next_event_type = __TRACE_LAST_TYPE + 1;
> +static int next_event_type = __TRACE_LAST_TYPE;
>
> enum print_line_t trace_print_bputs_msg_only(struct trace_iterator *iter)
> {
> @@ -675,7 +675,7 @@ static LIST_HEAD(ftrace_event_list);
> static int trace_search_list(struct list_head **list)
> {
> struct trace_event *e;
> - int next = __TRACE_LAST_TYPE + 1;
> + int next = __TRACE_LAST_TYPE;
>
> if (list_empty(&ftrace_event_list)) {
> *list = &ftrace_event_list;
On Fri, 3 Jul 2020 10:06:12 +0800
Wei Yang <[email protected]> wrote:
> Currently we have following call flow:
>
> tracer_init_tracefs()
> tracing_init_dentry()
> event_trace_init()
> tracing_init_dentry()
>
> This shows tracing_init_dentry() is called twice in this flow and this
> is not necessary.
There's no reason to have patch 4 and 5 separate. Fold the two together.
If you want, you can create another patch that changes
tracing_init_dentry() to return a integer, as you point out, it never
returns an actual dentry. No reason for having it return a pointer then.
-- Steve
>
> Let's remove the second one when it is for sure be properly initialized.
>
> Signed-off-by: Wei Yang <[email protected]>
> ---
> kernel/trace/trace_events.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 8b3aa57dcea6..76879b29cf33 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -3434,7 +3434,6 @@ early_initcall(event_trace_enable_again);
> __init int event_trace_init(void)
> {
> struct trace_array *tr;
> - struct dentry *d_tracer;
> struct dentry *entry;
> int ret;
>
> @@ -3442,10 +3441,6 @@ __init int event_trace_init(void)
> if (!tr)
> return -ENODEV;
>
> - d_tracer = tracing_init_dentry();
> - if (IS_ERR(d_tracer))
> - return 0;
> -
> entry = tracefs_create_file("available_events", 0444, NULL,
> tr, &ftrace_avail_fops);
> if (!entry)
On Thu, Jul 09, 2020 at 05:57:39PM -0400, Steven Rostedt wrote:
>On Fri, 3 Jul 2020 10:06:08 +0800
>Wei Yang <[email protected]> wrote:
>
>> There are for 4 fields in trace_event_functions with the same type of
>> trace_print_func. Initialize them in register_trace_event() one by one
>> looks redundant.
>
>I have mixed emotions about this patch. Yeah, it consolidates it a bit,
>but it also makes it less easy to know what it is doing.
>
>All this patch is doing is optimizing the initialization path, which is
>done once when an event is registered. It's error prone, as you would
>need to make sure to map the array with the functions. Something like
>this is only reasonable if it is used more often, which here it's a
>single spot.
>
>So no, I can't take this patch.
>
Sure, I think you get the point.
>-- Steve
>
>
>
>>
>> Let's take advantage of union to simplify the procedure.
>>
>> Signed-off-by: Wei Yang <[email protected]>
>> ---
>> include/linux/trace_events.h | 13 +++++++++----
>> kernel/trace/trace_output.c | 14 +++++---------
>> 2 files changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
>> index 5c6943354049..1a421246f4a2 100644
>> --- a/include/linux/trace_events.h
>> +++ b/include/linux/trace_events.h
>> @@ -122,10 +122,15 @@ typedef enum print_line_t (*trace_print_func)(struct trace_iterator *iter,
>> int flags, struct trace_event *event);
>>
>> struct trace_event_functions {
>> - trace_print_func trace;
>> - trace_print_func raw;
>> - trace_print_func hex;
>> - trace_print_func binary;
>> + union {
>> + struct {
>> + trace_print_func trace;
>> + trace_print_func raw;
>> + trace_print_func hex;
>> + trace_print_func binary;
>> + };
>> + trace_print_func print_funcs[4];
>> + };
>> };
>>
>> struct trace_event {
>> diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
>> index 73976de7f8cc..47bf9f042b97 100644
>> --- a/kernel/trace/trace_output.c
>> +++ b/kernel/trace/trace_output.c
>> @@ -728,7 +728,7 @@ void trace_event_read_unlock(void)
>> int register_trace_event(struct trace_event *event)
>> {
>> unsigned key;
>> - int ret = 0;
>> + int i, ret = 0;
>>
>> down_write(&trace_event_sem);
>>
>> @@ -770,14 +770,10 @@ int register_trace_event(struct trace_event *event)
>> goto out;
>> }
>>
>> - if (event->funcs->trace == NULL)
>> - event->funcs->trace = trace_nop_print;
>> - if (event->funcs->raw == NULL)
>> - event->funcs->raw = trace_nop_print;
>> - if (event->funcs->hex == NULL)
>> - event->funcs->hex = trace_nop_print;
>> - if (event->funcs->binary == NULL)
>> - event->funcs->binary = trace_nop_print;
>> + for (i = 0; i < ARRAY_SIZE(event->funcs->print_funcs); i++) {
>> + if (!event->funcs->print_funcs[i])
>> + event->funcs->print_funcs[i] = trace_nop_print;
>> + }
>>
>> key = event->type & (EVENT_HASHSIZE - 1);
>>
--
Wei Yang
Help you, Help me
On Thu, Jul 09, 2020 at 06:13:59PM -0400, Steven Rostedt wrote:
>On Fri, 3 Jul 2020 10:06:12 +0800
>Wei Yang <[email protected]> wrote:
>
>> Currently we have following call flow:
>>
>> tracer_init_tracefs()
>> tracing_init_dentry()
>> event_trace_init()
>> tracing_init_dentry()
>>
>> This shows tracing_init_dentry() is called twice in this flow and this
>> is not necessary.
>
>There's no reason to have patch 4 and 5 separate. Fold the two together.
>
Yep, if you think there is no need.
Do you want me to send v2 based on you comments?
>If you want, you can create another patch that changes
>tracing_init_dentry() to return a integer, as you point out, it never
>returns an actual dentry. No reason for having it return a pointer then.
>
>-- Steve
>
>
>>
>> Let's remove the second one when it is for sure be properly initialized.
>>
>> Signed-off-by: Wei Yang <[email protected]>
>> ---
>> kernel/trace/trace_events.c | 5 -----
>> 1 file changed, 5 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>> index 8b3aa57dcea6..76879b29cf33 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -3434,7 +3434,6 @@ early_initcall(event_trace_enable_again);
>> __init int event_trace_init(void)
>> {
>> struct trace_array *tr;
>> - struct dentry *d_tracer;
>> struct dentry *entry;
>> int ret;
>>
>> @@ -3442,10 +3441,6 @@ __init int event_trace_init(void)
>> if (!tr)
>> return -ENODEV;
>>
>> - d_tracer = tracing_init_dentry();
>> - if (IS_ERR(d_tracer))
>> - return 0;
>> -
>> entry = tracefs_create_file("available_events", 0444, NULL,
>> tr, &ftrace_avail_fops);
>> if (!entry)
--
Wei Yang
Help you, Help me
On Fri, 10 Jul 2020 09:11:19 +0800
Wei Yang <[email protected]> wrote:
> Yep, if you think there is no need.
>
> Do you want me to send v2 based on you comments?
I already applied patch 2 and 3. Please send a v2 with the 4-5 folded,
and you can also include a patch to remove the dentry of
tracing_init_dentry() return.
-- Steve