* [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization
@ 2020-07-03 2:06 Wei Yang
2020-07-03 2:06 ` [PATCH 2/5] tracing: simplify the logic by defining next to be "lasst + 1" Wei Yang
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Wei Yang @ 2020-07-03 2:06 UTC (permalink / raw)
To: rostedt, mingo; +Cc: linux-kernel, Wei Yang
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 <richard.weiyang@linux.alibaba.com>
---
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)
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/5] tracing: simplify the logic by defining next to be "lasst + 1"
2020-07-03 2:06 [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization Wei Yang
@ 2020-07-03 2:06 ` Wei Yang
2020-07-09 21:59 ` Steven Rostedt
2020-07-03 2:06 ` [PATCH 3/5] tracing: save one trace_event->type by using __TRACE_LAST_TYPE Wei Yang
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Wei Yang @ 2020-07-03 2:06 UTC (permalink / raw)
To: rostedt, mingo; +Cc: linux-kernel, Wei Yang
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 <richard.weiyang@linux.alibaba.com>
---
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)
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/5] tracing: save one trace_event->type by using __TRACE_LAST_TYPE
2020-07-03 2:06 [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization Wei Yang
2020-07-03 2:06 ` [PATCH 2/5] tracing: simplify the logic by defining next to be "lasst + 1" Wei Yang
@ 2020-07-03 2:06 ` Wei Yang
2020-07-09 22:04 ` Steven Rostedt
2020-07-03 2:06 ` [PATCH 4/5] tracing: use NULL directly to create root level tracefs Wei Yang
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Wei Yang @ 2020-07-03 2:06 UTC (permalink / raw)
To: rostedt, mingo; +Cc: linux-kernel, Wei Yang
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 <richard.weiyang@linux.alibaba.com>
---
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)
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/5] tracing: use NULL directly to create root level tracefs
2020-07-03 2:06 [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization Wei Yang
2020-07-03 2:06 ` [PATCH 2/5] tracing: simplify the logic by defining next to be "lasst + 1" Wei Yang
2020-07-03 2:06 ` [PATCH 3/5] tracing: save one trace_event->type by using __TRACE_LAST_TYPE Wei Yang
@ 2020-07-03 2:06 ` Wei Yang
2020-07-03 2:06 ` [PATCH 5/5] tracing: toplevel d_entry already initialized Wei Yang
2020-07-09 21:57 ` [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization Steven Rostedt
4 siblings, 0 replies; 12+ messages in thread
From: Wei Yang @ 2020-07-03 2:06 UTC (permalink / raw)
To: rostedt, mingo; +Cc: linux-kernel, Wei Yang
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 <richard.weiyang@linux.alibaba.com>
---
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)
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 5/5] tracing: toplevel d_entry already initialized
2020-07-03 2:06 [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization Wei Yang
` (2 preceding siblings ...)
2020-07-03 2:06 ` [PATCH 4/5] tracing: use NULL directly to create root level tracefs Wei Yang
@ 2020-07-03 2:06 ` Wei Yang
2020-07-09 22:13 ` Steven Rostedt
2020-07-09 21:57 ` [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization Steven Rostedt
4 siblings, 1 reply; 12+ messages in thread
From: Wei Yang @ 2020-07-03 2:06 UTC (permalink / raw)
To: rostedt, mingo; +Cc: linux-kernel, Wei Yang
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 <richard.weiyang@linux.alibaba.com>
---
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)
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization
2020-07-03 2:06 [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization Wei Yang
` (3 preceding siblings ...)
2020-07-03 2:06 ` [PATCH 5/5] tracing: toplevel d_entry already initialized Wei Yang
@ 2020-07-09 21:57 ` Steven Rostedt
2020-07-10 1:10 ` Wei Yang
4 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2020-07-09 21:57 UTC (permalink / raw)
To: Wei Yang; +Cc: mingo, linux-kernel
On Fri, 3 Jul 2020 10:06:08 +0800
Wei Yang <richard.weiyang@linux.alibaba.com> 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 <richard.weiyang@linux.alibaba.com>
> ---
> 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);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/5] tracing: simplify the logic by defining next to be "lasst + 1"
2020-07-03 2:06 ` [PATCH 2/5] tracing: simplify the logic by defining next to be "lasst + 1" Wei Yang
@ 2020-07-09 21:59 ` Steven Rostedt
0 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-07-09 21:59 UTC (permalink / raw)
To: Wei Yang; +Cc: mingo, linux-kernel
On Fri, 3 Jul 2020 10:06:09 +0800
Wei Yang <richard.weiyang@linux.alibaba.com> 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 <richard.weiyang@linux.alibaba.com>
> ---
> 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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/5] tracing: save one trace_event->type by using __TRACE_LAST_TYPE
2020-07-03 2:06 ` [PATCH 3/5] tracing: save one trace_event->type by using __TRACE_LAST_TYPE Wei Yang
@ 2020-07-09 22:04 ` Steven Rostedt
0 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-07-09 22:04 UTC (permalink / raw)
To: Wei Yang; +Cc: mingo, linux-kernel
On Fri, 3 Jul 2020 10:06:10 +0800
Wei Yang <richard.weiyang@linux.alibaba.com> 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 <richard.weiyang@linux.alibaba.com>
> ---
> 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;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 5/5] tracing: toplevel d_entry already initialized
2020-07-03 2:06 ` [PATCH 5/5] tracing: toplevel d_entry already initialized Wei Yang
@ 2020-07-09 22:13 ` Steven Rostedt
2020-07-10 1:11 ` Wei Yang
0 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2020-07-09 22:13 UTC (permalink / raw)
To: Wei Yang; +Cc: mingo, linux-kernel
On Fri, 3 Jul 2020 10:06:12 +0800
Wei Yang <richard.weiyang@linux.alibaba.com> 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 <richard.weiyang@linux.alibaba.com>
> ---
> 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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization
2020-07-09 21:57 ` [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization Steven Rostedt
@ 2020-07-10 1:10 ` Wei Yang
0 siblings, 0 replies; 12+ messages in thread
From: Wei Yang @ 2020-07-10 1:10 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Wei Yang, mingo, linux-kernel
On Thu, Jul 09, 2020 at 05:57:39PM -0400, Steven Rostedt wrote:
>On Fri, 3 Jul 2020 10:06:08 +0800
>Wei Yang <richard.weiyang@linux.alibaba.com> 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 <richard.weiyang@linux.alibaba.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 5/5] tracing: toplevel d_entry already initialized
2020-07-09 22:13 ` Steven Rostedt
@ 2020-07-10 1:11 ` Wei Yang
2020-07-10 13:08 ` Steven Rostedt
0 siblings, 1 reply; 12+ messages in thread
From: Wei Yang @ 2020-07-10 1:11 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Wei Yang, mingo, linux-kernel
On Thu, Jul 09, 2020 at 06:13:59PM -0400, Steven Rostedt wrote:
>On Fri, 3 Jul 2020 10:06:12 +0800
>Wei Yang <richard.weiyang@linux.alibaba.com> 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 <richard.weiyang@linux.alibaba.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 5/5] tracing: toplevel d_entry already initialized
2020-07-10 1:11 ` Wei Yang
@ 2020-07-10 13:08 ` Steven Rostedt
0 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-07-10 13:08 UTC (permalink / raw)
To: Wei Yang; +Cc: mingo, linux-kernel
On Fri, 10 Jul 2020 09:11:19 +0800
Wei Yang <richard.weiyang@linux.alibaba.com> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-07-10 13:08 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-03 2:06 [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization Wei Yang
2020-07-03 2:06 ` [PATCH 2/5] tracing: simplify the logic by defining next to be "lasst + 1" Wei Yang
2020-07-09 21:59 ` Steven Rostedt
2020-07-03 2:06 ` [PATCH 3/5] tracing: save one trace_event->type by using __TRACE_LAST_TYPE Wei Yang
2020-07-09 22:04 ` Steven Rostedt
2020-07-03 2:06 ` [PATCH 4/5] tracing: use NULL directly to create root level tracefs Wei Yang
2020-07-03 2:06 ` [PATCH 5/5] tracing: toplevel d_entry already initialized Wei Yang
2020-07-09 22:13 ` Steven Rostedt
2020-07-10 1:11 ` Wei Yang
2020-07-10 13:08 ` Steven Rostedt
2020-07-09 21:57 ` [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization Steven Rostedt
2020-07-10 1:10 ` Wei Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).