Re: [PATCH 2/2] tracing: Merge struct event_trigger_ops into struct event_command

[Tom Zanussi <zanussi@kernel.org>]

Hi Steve,

On Tue, 2025-11-18 at 22:10 -0500, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
> 
> Now that there's pretty much a one to one mapping between the struct
> event_trigger_ops and struct event_command, there's no reason to have two
> different structures. Merge the function pointers of event_trigger_ops
> into event_command.
> 
> There's one exception in trace_events_hist.c for the
> event_hist_trigger_named_ops. This has special logic for the init and free
> function pointers for "named histograms". In this case, allocate the
> cmd_ops of the event_trigger_data and set it to the proper init and free
> functions, which are used to initialize and free the event_trigger_data
> respectively. Have the free function and the init function (on failure)
> free the cmd_ops of the data element.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Very nice as well, just one tiny note below..

Reviewed-by: Tom Zanussi <zanussi@kernel.org>


> ---
>  kernel/trace/trace.h                | 126 +++++++++++-----------------
>  kernel/trace/trace_eprobe.c         |  13 +--
>  kernel/trace/trace_events_hist.c    |  93 ++++++++++----------
>  kernel/trace/trace_events_trigger.c | 121 +++++++++++---------------
>  4 files changed, 152 insertions(+), 201 deletions(-)
> 

[...]

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index f9cc8d6a215b..1e03398b9e91 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -5694,7 +5694,7 @@ static void hist_trigger_show(struct seq_file *m,
>  		seq_puts(m, "\n\n");
>  
>  	seq_puts(m, "# event histogram\n#\n# trigger info: ");
> -	data->ops->print(m, data);
> +	data->cmd_ops->print(m, data);
>  	seq_puts(m, "#\n\n");
>  
>  	hist_data = data->private_data;
> @@ -6016,7 +6016,7 @@ static void hist_trigger_debug_show(struct seq_file *m,
>  		seq_puts(m, "\n\n");
>  
>  	seq_puts(m, "# event histogram\n#\n# trigger info: ");
> -	data->ops->print(m, data);
> +	data->cmd_ops->print(m, data);
>  	seq_puts(m, "#\n\n");
>  
>  	hist_data = data->private_data;
> @@ -6326,20 +6326,23 @@ static void event_hist_trigger_free(struct event_trigger_data *data)
>  	free_hist_pad();
>  }
>  
> -static const struct event_trigger_ops event_hist_trigger_ops = {
> -	.trigger		= event_hist_trigger,
> -	.print			= event_hist_trigger_print,
> -	.init			= event_hist_trigger_init,
> -	.free			= event_hist_trigger_free,
> -};
> +static struct event_command trigger_hist_cmd;
>  

I don't think this is needed, since the same declaration already
appears just above the event_hist_trigger_parse() declaration.

Thanks,

Tom