Hi Steve, On Tue, 2025-11-18 at 22:10 -0500, Steven Rostedt wrote: > From: Steven Rostedt <[email protected]> > > 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) <[email protected]>
Very nice as well, just one tiny note below.. Reviewed-by: Tom Zanussi <[email protected]> > --- > 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
