Re: [PATCH 1/5] tracing: use union to simplify the trace_event_functions initialization

[Wei Yang]

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