* [PATCH 0/2] tracing: Clean up trigger code my merging structures
@ 2025-11-19 3:10 Steven Rostedt
2025-11-19 3:10 ` [PATCH 1/2] tracing: Remove get_trigger_ops() and add count_func() from trigger ops Steven Rostedt
2025-11-19 3:10 ` [PATCH 2/2] tracing: Merge struct event_trigger_ops into struct event_command Steven Rostedt
0 siblings, 2 replies; 6+ messages in thread
From: Steven Rostedt @ 2025-11-19 3:10 UTC (permalink / raw)
To: linux-kernel, linux-trace-kernel
Cc: Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton,
Tom Zanussi
The struct event_command has a callback function called get_trigger_ops().
This callback returns the "trigger_ops" to use for the trigger. These ops
define the trigger function, how to init the trigger, how to print the
trigger and how to free it.
The only reason there's a callback function to get these ops is because
some triggers have two types of operations. One is an "always on"
operation, and the other is a "count down" operation. If a user passes in
a parameter to say how many times the trigger should execute. For example:
echo stacktrace:5 > events/kmem/kmem_cache_alloc/trigger
It will trigger the stacktrace for the first 5 times the kmem_cache_alloc
event is hit.
Instead of having two different trigger_ops since the only difference
between them is the tigger itself (the print, init and free functions are
all the same), just use a single ops that the event_command points to and
add a function field to the trigger_ops to have a count_func.
When a trigger is added to an event, if there's a count attached to it and
the trigger ops has the count_func field, the data allocated to represent
this trigger will have a new flag set called COUNT.
Then when the trigger executes, it will check if the COUNT data flag is
set, and if so, it will call the ops count_func(). If that returns false,
it returns without executing the trigger.
After making the struct event_trigger_ops be mapped one to one with the
struct event_command, merge the former into the latter.
Steven Rostedt (2):
tracing: Remove get_trigger_ops() and add count_func() from trigger ops
tracing: Merge struct event_trigger_ops into struct event_command
----
kernel/trace/trace.h | 124 ++++++-------
kernel/trace/trace_eprobe.c | 19 +-
kernel/trace/trace_events_hist.c | 143 +++++----------
kernel/trace/trace_events_trigger.c | 344 +++++++++++++-----------------------
4 files changed, 231 insertions(+), 399 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] tracing: Remove get_trigger_ops() and add count_func() from trigger ops 2025-11-19 3:10 [PATCH 0/2] tracing: Clean up trigger code my merging structures Steven Rostedt @ 2025-11-19 3:10 ` Steven Rostedt 2025-11-25 12:54 ` Tom Zanussi 2025-11-19 3:10 ` [PATCH 2/2] tracing: Merge struct event_trigger_ops into struct event_command Steven Rostedt 1 sibling, 1 reply; 6+ messages in thread From: Steven Rostedt @ 2025-11-19 3:10 UTC (permalink / raw) To: linux-kernel, linux-trace-kernel Cc: Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton, Tom Zanussi From: Steven Rostedt <rostedt@goodmis.org> The struct event_command has a callback function called get_trigger_ops(). This callback returns the "trigger_ops" to use for the trigger. These ops define the trigger function, how to init the trigger, how to print the trigger and how to free it. The only reason there's a callback function to get these ops is because some triggers have two types of operations. One is an "always on" operation, and the other is a "count down" operation. If a user passes in a parameter to say how many times the trigger should execute. For example: echo stacktrace:5 > events/kmem/kmem_cache_alloc/trigger It will trigger the stacktrace for the first 5 times the kmem_cache_alloc event is hit. Instead of having two different trigger_ops since the only difference between them is the tigger itself (the print, init and free functions are all the same), just use a single ops that the event_command points to and add a function field to the trigger_ops to have a count_func. When a trigger is added to an event, if there's a count attached to it and the trigger ops has the count_func field, the data allocated to represent this trigger will have a new flag set called COUNT. Then when the trigger executes, it will check if the COUNT data flag is set, and if so, it will call the ops count_func(). If that returns false, it returns without executing the trigger. This removes the need for duplicate event_trigger_ops structures. Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> --- kernel/trace/trace.h | 26 ++- kernel/trace/trace_eprobe.c | 8 +- kernel/trace/trace_events_hist.c | 60 +------ kernel/trace/trace_events_trigger.c | 257 ++++++++++------------------ 4 files changed, 116 insertions(+), 235 deletions(-) diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h index 58be6d741d72..036019ffc407 100644 --- a/kernel/trace/trace.h +++ b/kernel/trace/trace.h @@ -1791,6 +1791,7 @@ extern void clear_event_triggers(struct trace_array *tr); enum { EVENT_TRIGGER_FL_PROBE = BIT(0), + EVENT_TRIGGER_FL_COUNT = BIT(1), }; struct event_trigger_data { @@ -1822,6 +1823,10 @@ struct enable_trigger_data { bool hist; }; +bool event_trigger_count(struct event_trigger_data *data, + struct trace_buffer *buffer, void *rec, + struct ring_buffer_event *event); + extern int event_enable_trigger_print(struct seq_file *m, struct event_trigger_data *data); extern void event_enable_trigger_free(struct event_trigger_data *data); @@ -1909,6 +1914,11 @@ extern void event_file_put(struct trace_event_file *file); * registered the trigger (see struct event_command) along with * the trace record, rec. * + * @count_func: If defined and a numeric parameter is passed to the + * trigger, then this function will be called before @trigger + * is called. If this function returns false, then @trigger is not + * executed. + * * @init: An optional initialization function called for the trigger * when the trigger is registered (via the event_command reg() * function). This can be used to perform per-trigger @@ -1936,6 +1946,10 @@ struct event_trigger_ops { struct trace_buffer *buffer, void *rec, struct ring_buffer_event *rbe); + bool (*count_func)(struct event_trigger_data *data, + struct trace_buffer *buffer, + void *rec, + struct ring_buffer_event *rbe); int (*init)(struct event_trigger_data *data); void (*free)(struct event_trigger_data *data); int (*print)(struct seq_file *m, @@ -1962,6 +1976,9 @@ struct event_trigger_ops { * @name: The unique name that identifies the event command. This is * the name used when setting triggers via trigger files. * + * @trigger_ops: The event_trigger_ops implementation associated with + * the command. + * * @trigger_type: A unique id that identifies the event command * 'type'. This value has two purposes, the first to ensure that * only one trigger of the same type can be set at a given time @@ -2013,17 +2030,11 @@ struct event_trigger_ops { * event command, filters set by the user for the command will be * ignored. This is usually implemented by the generic utility * function @set_trigger_filter() (see trace_event_triggers.c). - * - * @get_trigger_ops: The callback function invoked to retrieve the - * event_trigger_ops implementation associated with the command. - * This callback function allows a single event_command to - * support multiple trigger implementations via different sets of - * event_trigger_ops, depending on the value of the @param - * string. */ struct event_command { struct list_head list; char *name; + const struct event_trigger_ops *trigger_ops; enum event_trigger_type trigger_type; int flags; int (*parse)(struct event_command *cmd_ops, @@ -2040,7 +2051,6 @@ struct event_command { int (*set_filter)(char *filter_str, struct event_trigger_data *data, struct trace_event_file *file); - const struct event_trigger_ops *(*get_trigger_ops)(char *cmd, char *param); }; /** diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c index a1d402124836..14ae896dbe75 100644 --- a/kernel/trace/trace_eprobe.c +++ b/kernel/trace/trace_eprobe.c @@ -513,21 +513,15 @@ static void eprobe_trigger_unreg_func(char *glob, } -static const struct event_trigger_ops *eprobe_trigger_get_ops(char *cmd, - char *param) -{ - return &eprobe_trigger_ops; -} - static struct event_command event_trigger_cmd = { .name = "eprobe", .trigger_type = ETT_EVENT_EPROBE, .flags = EVENT_CMD_FL_NEEDS_REC, + .trigger_ops = &eprobe_trigger_ops, .parse = eprobe_trigger_cmd_parse, .reg = eprobe_trigger_reg_func, .unreg = eprobe_trigger_unreg_func, .unreg_all = NULL, - .get_trigger_ops = eprobe_trigger_get_ops, .set_filter = NULL, }; diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 1d536219b624..f9cc8d6a215b 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -6363,12 +6363,6 @@ static const struct event_trigger_ops event_hist_trigger_named_ops = { .free = event_hist_trigger_named_free, }; -static const struct event_trigger_ops *event_hist_get_trigger_ops(char *cmd, - char *param) -{ - return &event_hist_trigger_ops; -} - static void hist_clear(struct event_trigger_data *data) { struct hist_trigger_data *hist_data = data->private_data; @@ -6908,11 +6902,11 @@ static struct event_command trigger_hist_cmd = { .name = "hist", .trigger_type = ETT_EVENT_HIST, .flags = EVENT_CMD_FL_NEEDS_REC, + .trigger_ops = &event_hist_trigger_ops, .parse = event_hist_trigger_parse, .reg = hist_register_trigger, .unreg = hist_unregister_trigger, .unreg_all = hist_unreg_all, - .get_trigger_ops = event_hist_get_trigger_ops, .set_filter = set_trigger_filter, }; @@ -6945,29 +6939,9 @@ hist_enable_trigger(struct event_trigger_data *data, } } -static void -hist_enable_count_trigger(struct event_trigger_data *data, - struct trace_buffer *buffer, void *rec, - struct ring_buffer_event *event) -{ - if (!data->count) - return; - - if (data->count != -1) - (data->count)--; - - hist_enable_trigger(data, buffer, rec, event); -} - static const struct event_trigger_ops hist_enable_trigger_ops = { .trigger = hist_enable_trigger, - .print = event_enable_trigger_print, - .init = event_trigger_init, - .free = event_enable_trigger_free, -}; - -static const struct event_trigger_ops hist_enable_count_trigger_ops = { - .trigger = hist_enable_count_trigger, + .count_func = event_trigger_count, .print = event_enable_trigger_print, .init = event_trigger_init, .free = event_enable_trigger_free, @@ -6975,36 +6949,12 @@ static const struct event_trigger_ops hist_enable_count_trigger_ops = { static const struct event_trigger_ops hist_disable_trigger_ops = { .trigger = hist_enable_trigger, + .count_func = event_trigger_count, .print = event_enable_trigger_print, .init = event_trigger_init, .free = event_enable_trigger_free, }; -static const struct event_trigger_ops hist_disable_count_trigger_ops = { - .trigger = hist_enable_count_trigger, - .print = event_enable_trigger_print, - .init = event_trigger_init, - .free = event_enable_trigger_free, -}; - -static const struct event_trigger_ops * -hist_enable_get_trigger_ops(char *cmd, char *param) -{ - const struct event_trigger_ops *ops; - bool enable; - - enable = (strcmp(cmd, ENABLE_HIST_STR) == 0); - - if (enable) - ops = param ? &hist_enable_count_trigger_ops : - &hist_enable_trigger_ops; - else - ops = param ? &hist_disable_count_trigger_ops : - &hist_disable_trigger_ops; - - return ops; -} - static void hist_enable_unreg_all(struct trace_event_file *file) { struct event_trigger_data *test, *n; @@ -7023,22 +6973,22 @@ static void hist_enable_unreg_all(struct trace_event_file *file) static struct event_command trigger_hist_enable_cmd = { .name = ENABLE_HIST_STR, .trigger_type = ETT_HIST_ENABLE, + .trigger_ops = &hist_enable_trigger_ops, .parse = event_enable_trigger_parse, .reg = event_enable_register_trigger, .unreg = event_enable_unregister_trigger, .unreg_all = hist_enable_unreg_all, - .get_trigger_ops = hist_enable_get_trigger_ops, .set_filter = set_trigger_filter, }; static struct event_command trigger_hist_disable_cmd = { .name = DISABLE_HIST_STR, .trigger_type = ETT_HIST_ENABLE, + .trigger_ops = &hist_disable_trigger_ops, .parse = event_enable_trigger_parse, .reg = event_enable_register_trigger, .unreg = event_enable_unregister_trigger, .unreg_all = hist_enable_unreg_all, - .get_trigger_ops = hist_enable_get_trigger_ops, .set_filter = set_trigger_filter, }; diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c index cbfc306c0159..576bad18bcdb 100644 --- a/kernel/trace/trace_events_trigger.c +++ b/kernel/trace/trace_events_trigger.c @@ -28,6 +28,20 @@ void trigger_data_free(struct event_trigger_data *data) kfree(data); } +static inline void data_ops_trigger(struct event_trigger_data *data, + struct trace_buffer *buffer, void *rec, + struct ring_buffer_event *event) +{ + const struct event_trigger_ops *ops = data->ops; + + if (data->flags & EVENT_TRIGGER_FL_COUNT) { + if (!ops->count_func(data, buffer, rec, event)) + return; + } + + ops->trigger(data, buffer, rec, event); +} + /** * event_triggers_call - Call triggers associated with a trace event * @file: The trace_event_file associated with the event @@ -70,7 +84,7 @@ event_triggers_call(struct trace_event_file *file, if (data->paused) continue; if (!rec) { - data->ops->trigger(data, buffer, rec, event); + data_ops_trigger(data, buffer, rec, event); continue; } filter = rcu_dereference_sched(data->filter); @@ -80,7 +94,7 @@ event_triggers_call(struct trace_event_file *file, tt |= data->cmd_ops->trigger_type; continue; } - data->ops->trigger(data, buffer, rec, event); + data_ops_trigger(data, buffer, rec, event); } return tt; } @@ -122,7 +136,7 @@ event_triggers_post_call(struct trace_event_file *file, if (data->paused) continue; if (data->cmd_ops->trigger_type & tt) - data->ops->trigger(data, NULL, NULL, NULL); + data_ops_trigger(data, NULL, NULL, NULL); } } EXPORT_SYMBOL_GPL(event_triggers_post_call); @@ -377,6 +391,36 @@ __init int unregister_event_command(struct event_command *cmd) return -ENODEV; } +/** + * event_trigger_count - Optional count function for event triggers + * @data: Trigger-specific data + * @buffer: The ring buffer that the event is being written to + * @rec: The trace entry for the event, NULL for unconditional invocation + * @event: The event meta data in the ring buffer + * + * For triggers that can take a count parameter that doesn't do anything + * special, they can use this function to assign to their .count_func + * field. + * + * This simply does a count down of the @data->count field. + * + * If the @data->count is greater than zero, it will decrement it. + * + * Returns false if @data->count is zero, otherwise true. + */ +bool event_trigger_count(struct event_trigger_data *data, + struct trace_buffer *buffer, void *rec, + struct ring_buffer_event *event) +{ + if (!data->count) + return false; + + if (data->count != -1) + (data->count)--; + + return true; +} + /** * event_trigger_print - Generic event_trigger_ops @print implementation * @name: The name of the event trigger @@ -807,9 +851,13 @@ int event_trigger_separate_filter(char *param_and_filter, char **param, * @private_data: User data to associate with the event trigger * * Allocate an event_trigger_data instance and initialize it. The - * @cmd_ops are used along with the @cmd and @param to get the - * trigger_ops to assign to the event_trigger_data. @private_data can - * also be passed in and associated with the event_trigger_data. + * @cmd_ops defines how the trigger will operate. If @param is set, + * and @cmd_ops->trigger_ops->count_func is non NULL, then the + * data->count is set to @param and before the trigger is executed, the + * @cmd_ops->trigger_ops->count_func() is called. If that function returns + * false, the @cmd_ops->trigger_ops->trigger() function will not be called. + * @private_data can also be passed in and associated with the + * event_trigger_data. * * Use trigger_data_free() to free an event_trigger_data object. * @@ -821,18 +869,17 @@ struct event_trigger_data *trigger_data_alloc(struct event_command *cmd_ops, void *private_data) { struct event_trigger_data *trigger_data; - const struct event_trigger_ops *trigger_ops; - - trigger_ops = cmd_ops->get_trigger_ops(cmd, param); trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL); if (!trigger_data) return NULL; trigger_data->count = -1; - trigger_data->ops = trigger_ops; + trigger_data->ops = cmd_ops->trigger_ops; trigger_data->cmd_ops = cmd_ops; trigger_data->private_data = private_data; + if (param && cmd_ops->trigger_ops->count_func) + trigger_data->flags |= EVENT_TRIGGER_FL_COUNT; INIT_LIST_HEAD(&trigger_data->list); INIT_LIST_HEAD(&trigger_data->named_list); @@ -1271,31 +1318,28 @@ traceon_trigger(struct event_trigger_data *data, tracing_on(); } -static void -traceon_count_trigger(struct event_trigger_data *data, - struct trace_buffer *buffer, void *rec, - struct ring_buffer_event *event) +static bool +traceon_count_func(struct event_trigger_data *data, + struct trace_buffer *buffer, void *rec, + struct ring_buffer_event *event) { struct trace_event_file *file = data->private_data; if (file) { if (tracer_tracing_is_on(file->tr)) - return; + return false; } else { if (tracing_is_on()) - return; + return false; } if (!data->count) - return; + return false; if (data->count != -1) (data->count)--; - if (file) - tracer_tracing_on(file->tr); - else - tracing_on(); + return true; } static void @@ -1319,31 +1363,28 @@ traceoff_trigger(struct event_trigger_data *data, tracing_off(); } -static void -traceoff_count_trigger(struct event_trigger_data *data, - struct trace_buffer *buffer, void *rec, - struct ring_buffer_event *event) +static bool +traceoff_count_func(struct event_trigger_data *data, + struct trace_buffer *buffer, void *rec, + struct ring_buffer_event *event) { struct trace_event_file *file = data->private_data; if (file) { if (!tracer_tracing_is_on(file->tr)) - return; + return false; } else { if (!tracing_is_on()) - return; + return false; } if (!data->count) - return; + return false; if (data->count != -1) (data->count)--; - if (file) - tracer_tracing_off(file->tr); - else - tracing_off(); + return true; } static int @@ -1362,13 +1403,7 @@ traceoff_trigger_print(struct seq_file *m, struct event_trigger_data *data) static const struct event_trigger_ops traceon_trigger_ops = { .trigger = traceon_trigger, - .print = traceon_trigger_print, - .init = event_trigger_init, - .free = event_trigger_free, -}; - -static const struct event_trigger_ops traceon_count_trigger_ops = { - .trigger = traceon_count_trigger, + .count_func = traceon_count_func, .print = traceon_trigger_print, .init = event_trigger_init, .free = event_trigger_free, @@ -1376,41 +1411,19 @@ static const struct event_trigger_ops traceon_count_trigger_ops = { static const struct event_trigger_ops traceoff_trigger_ops = { .trigger = traceoff_trigger, + .count_func = traceoff_count_func, .print = traceoff_trigger_print, .init = event_trigger_init, .free = event_trigger_free, }; -static const struct event_trigger_ops traceoff_count_trigger_ops = { - .trigger = traceoff_count_trigger, - .print = traceoff_trigger_print, - .init = event_trigger_init, - .free = event_trigger_free, -}; - -static const struct event_trigger_ops * -onoff_get_trigger_ops(char *cmd, char *param) -{ - const struct event_trigger_ops *ops; - - /* we register both traceon and traceoff to this callback */ - if (strcmp(cmd, "traceon") == 0) - ops = param ? &traceon_count_trigger_ops : - &traceon_trigger_ops; - else - ops = param ? &traceoff_count_trigger_ops : - &traceoff_trigger_ops; - - return ops; -} - static struct event_command trigger_traceon_cmd = { .name = "traceon", .trigger_type = ETT_TRACE_ONOFF, + .trigger_ops = &traceon_trigger_ops, .parse = event_trigger_parse, .reg = register_trigger, .unreg = unregister_trigger, - .get_trigger_ops = onoff_get_trigger_ops, .set_filter = set_trigger_filter, }; @@ -1418,10 +1431,10 @@ static struct event_command trigger_traceoff_cmd = { .name = "traceoff", .trigger_type = ETT_TRACE_ONOFF, .flags = EVENT_CMD_FL_POST_TRIGGER, + .trigger_ops = &traceoff_trigger_ops, .parse = event_trigger_parse, .reg = register_trigger, .unreg = unregister_trigger, - .get_trigger_ops = onoff_get_trigger_ops, .set_filter = set_trigger_filter, }; @@ -1439,20 +1452,6 @@ snapshot_trigger(struct event_trigger_data *data, tracing_snapshot(); } -static void -snapshot_count_trigger(struct event_trigger_data *data, - struct trace_buffer *buffer, void *rec, - struct ring_buffer_event *event) -{ - if (!data->count) - return; - - if (data->count != -1) - (data->count)--; - - snapshot_trigger(data, buffer, rec, event); -} - static int register_snapshot_trigger(char *glob, struct event_trigger_data *data, @@ -1486,31 +1485,19 @@ snapshot_trigger_print(struct seq_file *m, struct event_trigger_data *data) static const struct event_trigger_ops snapshot_trigger_ops = { .trigger = snapshot_trigger, + .count_func = event_trigger_count, .print = snapshot_trigger_print, .init = event_trigger_init, .free = event_trigger_free, }; -static const struct event_trigger_ops snapshot_count_trigger_ops = { - .trigger = snapshot_count_trigger, - .print = snapshot_trigger_print, - .init = event_trigger_init, - .free = event_trigger_free, -}; - -static const struct event_trigger_ops * -snapshot_get_trigger_ops(char *cmd, char *param) -{ - return param ? &snapshot_count_trigger_ops : &snapshot_trigger_ops; -} - static struct event_command trigger_snapshot_cmd = { .name = "snapshot", .trigger_type = ETT_SNAPSHOT, + .trigger_ops = &snapshot_trigger_ops, .parse = event_trigger_parse, .reg = register_snapshot_trigger, .unreg = unregister_snapshot_trigger, - .get_trigger_ops = snapshot_get_trigger_ops, .set_filter = set_trigger_filter, }; @@ -1558,20 +1545,6 @@ stacktrace_trigger(struct event_trigger_data *data, trace_dump_stack(STACK_SKIP); } -static void -stacktrace_count_trigger(struct event_trigger_data *data, - struct trace_buffer *buffer, void *rec, - struct ring_buffer_event *event) -{ - if (!data->count) - return; - - if (data->count != -1) - (data->count)--; - - stacktrace_trigger(data, buffer, rec, event); -} - static int stacktrace_trigger_print(struct seq_file *m, struct event_trigger_data *data) { @@ -1581,32 +1554,20 @@ stacktrace_trigger_print(struct seq_file *m, struct event_trigger_data *data) static const struct event_trigger_ops stacktrace_trigger_ops = { .trigger = stacktrace_trigger, + .count_func = event_trigger_count, .print = stacktrace_trigger_print, .init = event_trigger_init, .free = event_trigger_free, }; -static const struct event_trigger_ops stacktrace_count_trigger_ops = { - .trigger = stacktrace_count_trigger, - .print = stacktrace_trigger_print, - .init = event_trigger_init, - .free = event_trigger_free, -}; - -static const struct event_trigger_ops * -stacktrace_get_trigger_ops(char *cmd, char *param) -{ - return param ? &stacktrace_count_trigger_ops : &stacktrace_trigger_ops; -} - static struct event_command trigger_stacktrace_cmd = { .name = "stacktrace", .trigger_type = ETT_STACKTRACE, + .trigger_ops = &stacktrace_trigger_ops, .flags = EVENT_CMD_FL_POST_TRIGGER, .parse = event_trigger_parse, .reg = register_trigger, .unreg = unregister_trigger, - .get_trigger_ops = stacktrace_get_trigger_ops, .set_filter = set_trigger_filter, }; @@ -1642,24 +1603,24 @@ event_enable_trigger(struct event_trigger_data *data, set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &enable_data->file->flags); } -static void -event_enable_count_trigger(struct event_trigger_data *data, - struct trace_buffer *buffer, void *rec, - struct ring_buffer_event *event) +static bool +event_enable_count_func(struct event_trigger_data *data, + struct trace_buffer *buffer, void *rec, + struct ring_buffer_event *event) { struct enable_trigger_data *enable_data = data->private_data; if (!data->count) - return; + return false; /* Skip if the event is in a state we want to switch to */ if (enable_data->enable == !(enable_data->file->flags & EVENT_FILE_FL_SOFT_DISABLED)) - return; + return false; if (data->count != -1) (data->count)--; - event_enable_trigger(data, buffer, rec, event); + return true; } int event_enable_trigger_print(struct seq_file *m, @@ -1706,13 +1667,7 @@ void event_enable_trigger_free(struct event_trigger_data *data) static const struct event_trigger_ops event_enable_trigger_ops = { .trigger = event_enable_trigger, - .print = event_enable_trigger_print, - .init = event_trigger_init, - .free = event_enable_trigger_free, -}; - -static const struct event_trigger_ops event_enable_count_trigger_ops = { - .trigger = event_enable_count_trigger, + .count_func = event_enable_count_func, .print = event_enable_trigger_print, .init = event_trigger_init, .free = event_enable_trigger_free, @@ -1720,13 +1675,7 @@ static const struct event_trigger_ops event_enable_count_trigger_ops = { static const struct event_trigger_ops event_disable_trigger_ops = { .trigger = event_enable_trigger, - .print = event_enable_trigger_print, - .init = event_trigger_init, - .free = event_enable_trigger_free, -}; - -static const struct event_trigger_ops event_disable_count_trigger_ops = { - .trigger = event_enable_count_trigger, + .count_func = event_enable_count_func, .print = event_enable_trigger_print, .init = event_trigger_init, .free = event_enable_trigger_free, @@ -1906,45 +1855,23 @@ void event_enable_unregister_trigger(char *glob, data->ops->free(data); } -static const struct event_trigger_ops * -event_enable_get_trigger_ops(char *cmd, char *param) -{ - const struct event_trigger_ops *ops; - bool enable; - -#ifdef CONFIG_HIST_TRIGGERS - enable = ((strcmp(cmd, ENABLE_EVENT_STR) == 0) || - (strcmp(cmd, ENABLE_HIST_STR) == 0)); -#else - enable = strcmp(cmd, ENABLE_EVENT_STR) == 0; -#endif - if (enable) - ops = param ? &event_enable_count_trigger_ops : - &event_enable_trigger_ops; - else - ops = param ? &event_disable_count_trigger_ops : - &event_disable_trigger_ops; - - return ops; -} - static struct event_command trigger_enable_cmd = { .name = ENABLE_EVENT_STR, .trigger_type = ETT_EVENT_ENABLE, + .trigger_ops = &event_enable_trigger_ops, .parse = event_enable_trigger_parse, .reg = event_enable_register_trigger, .unreg = event_enable_unregister_trigger, - .get_trigger_ops = event_enable_get_trigger_ops, .set_filter = set_trigger_filter, }; static struct event_command trigger_disable_cmd = { .name = DISABLE_EVENT_STR, .trigger_type = ETT_EVENT_ENABLE, + .trigger_ops = &event_disable_trigger_ops, .parse = event_enable_trigger_parse, .reg = event_enable_register_trigger, .unreg = event_enable_unregister_trigger, - .get_trigger_ops = event_enable_get_trigger_ops, .set_filter = set_trigger_filter, }; -- 2.51.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] tracing: Remove get_trigger_ops() and add count_func() from trigger ops 2025-11-19 3:10 ` [PATCH 1/2] tracing: Remove get_trigger_ops() and add count_func() from trigger ops Steven Rostedt @ 2025-11-25 12:54 ` Tom Zanussi 0 siblings, 0 replies; 6+ messages in thread From: Tom Zanussi @ 2025-11-25 12:54 UTC (permalink / raw) To: Steven Rostedt, linux-kernel, linux-trace-kernel Cc: Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton Hi Steve, On Tue, 2025-11-18 at 22:10 -0500, Steven Rostedt wrote: > From: Steven Rostedt <rostedt@goodmis.org> > > The struct event_command has a callback function called get_trigger_ops(). > This callback returns the "trigger_ops" to use for the trigger. These ops > define the trigger function, how to init the trigger, how to print the > trigger and how to free it. > > The only reason there's a callback function to get these ops is because > some triggers have two types of operations. One is an "always on" > operation, and the other is a "count down" operation. If a user passes in > a parameter to say how many times the trigger should execute. For example: > > echo stacktrace:5 > events/kmem/kmem_cache_alloc/trigger > > It will trigger the stacktrace for the first 5 times the kmem_cache_alloc > event is hit. > > Instead of having two different trigger_ops since the only difference > between them is the tigger itself (the print, init and free functions are > all the same), just use a single ops that the event_command points to and > add a function field to the trigger_ops to have a count_func. > > When a trigger is added to an event, if there's a count attached to it and > the trigger ops has the count_func field, the data allocated to represent > this trigger will have a new flag set called COUNT. > > Then when the trigger executes, it will check if the COUNT data flag is > set, and if so, it will call the ops count_func(). If that returns false, > it returns without executing the trigger. > > This removes the need for duplicate event_trigger_ops structures. > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> Very nice cleanup, thanks! Reviewed-by: Tom Zanussi <zanussi@kernel.org> > --- > kernel/trace/trace.h | 26 ++- > kernel/trace/trace_eprobe.c | 8 +- > kernel/trace/trace_events_hist.c | 60 +------ > kernel/trace/trace_events_trigger.c | 257 ++++++++++------------------ > 4 files changed, 116 insertions(+), 235 deletions(-) > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > index 58be6d741d72..036019ffc407 100644 > --- a/kernel/trace/trace.h > +++ b/kernel/trace/trace.h > @@ -1791,6 +1791,7 @@ extern void clear_event_triggers(struct trace_array *tr); > > enum { > EVENT_TRIGGER_FL_PROBE = BIT(0), > + EVENT_TRIGGER_FL_COUNT = BIT(1), > }; > > struct event_trigger_data { > @@ -1822,6 +1823,10 @@ struct enable_trigger_data { > bool hist; > }; > > +bool event_trigger_count(struct event_trigger_data *data, > + struct trace_buffer *buffer, void *rec, > + struct ring_buffer_event *event); > + > extern int event_enable_trigger_print(struct seq_file *m, > struct event_trigger_data *data); > extern void event_enable_trigger_free(struct event_trigger_data *data); > @@ -1909,6 +1914,11 @@ extern void event_file_put(struct trace_event_file *file); > * registered the trigger (see struct event_command) along with > * the trace record, rec. > * > + * @count_func: If defined and a numeric parameter is passed to the > + * trigger, then this function will be called before @trigger > + * is called. If this function returns false, then @trigger is not > + * executed. > + * > * @init: An optional initialization function called for the trigger > * when the trigger is registered (via the event_command reg() > * function). This can be used to perform per-trigger > @@ -1936,6 +1946,10 @@ struct event_trigger_ops { > struct trace_buffer *buffer, > void *rec, > struct ring_buffer_event *rbe); > + bool (*count_func)(struct event_trigger_data *data, > + struct trace_buffer *buffer, > + void *rec, > + struct ring_buffer_event *rbe); > int (*init)(struct event_trigger_data *data); > void (*free)(struct event_trigger_data *data); > int (*print)(struct seq_file *m, > @@ -1962,6 +1976,9 @@ struct event_trigger_ops { > * @name: The unique name that identifies the event command. This is > * the name used when setting triggers via trigger files. > * > + * @trigger_ops: The event_trigger_ops implementation associated with > + * the command. > + * > * @trigger_type: A unique id that identifies the event command > * 'type'. This value has two purposes, the first to ensure that > * only one trigger of the same type can be set at a given time > @@ -2013,17 +2030,11 @@ struct event_trigger_ops { > * event command, filters set by the user for the command will be > * ignored. This is usually implemented by the generic utility > * function @set_trigger_filter() (see trace_event_triggers.c). > - * > - * @get_trigger_ops: The callback function invoked to retrieve the > - * event_trigger_ops implementation associated with the command. > - * This callback function allows a single event_command to > - * support multiple trigger implementations via different sets of > - * event_trigger_ops, depending on the value of the @param > - * string. > */ > struct event_command { > struct list_head list; > char *name; > + const struct event_trigger_ops *trigger_ops; > enum event_trigger_type trigger_type; > int flags; > int (*parse)(struct event_command *cmd_ops, > @@ -2040,7 +2051,6 @@ struct event_command { > int (*set_filter)(char *filter_str, > struct event_trigger_data *data, > struct trace_event_file *file); > - const struct event_trigger_ops *(*get_trigger_ops)(char *cmd, char *param); > }; > > /** > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c > index a1d402124836..14ae896dbe75 100644 > --- a/kernel/trace/trace_eprobe.c > +++ b/kernel/trace/trace_eprobe.c > @@ -513,21 +513,15 @@ static void eprobe_trigger_unreg_func(char *glob, > > } > > -static const struct event_trigger_ops *eprobe_trigger_get_ops(char *cmd, > - char *param) > -{ > - return &eprobe_trigger_ops; > -} > - > static struct event_command event_trigger_cmd = { > .name = "eprobe", > .trigger_type = ETT_EVENT_EPROBE, > .flags = EVENT_CMD_FL_NEEDS_REC, > + .trigger_ops = &eprobe_trigger_ops, > .parse = eprobe_trigger_cmd_parse, > .reg = eprobe_trigger_reg_func, > .unreg = eprobe_trigger_unreg_func, > .unreg_all = NULL, > - .get_trigger_ops = eprobe_trigger_get_ops, > .set_filter = NULL, > }; > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index 1d536219b624..f9cc8d6a215b 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -6363,12 +6363,6 @@ static const struct event_trigger_ops event_hist_trigger_named_ops = { > .free = event_hist_trigger_named_free, > }; > > -static const struct event_trigger_ops *event_hist_get_trigger_ops(char *cmd, > - char *param) > -{ > - return &event_hist_trigger_ops; > -} > - > static void hist_clear(struct event_trigger_data *data) > { > struct hist_trigger_data *hist_data = data->private_data; > @@ -6908,11 +6902,11 @@ static struct event_command trigger_hist_cmd = { > .name = "hist", > .trigger_type = ETT_EVENT_HIST, > .flags = EVENT_CMD_FL_NEEDS_REC, > + .trigger_ops = &event_hist_trigger_ops, > .parse = event_hist_trigger_parse, > .reg = hist_register_trigger, > .unreg = hist_unregister_trigger, > .unreg_all = hist_unreg_all, > - .get_trigger_ops = event_hist_get_trigger_ops, > .set_filter = set_trigger_filter, > }; > > @@ -6945,29 +6939,9 @@ hist_enable_trigger(struct event_trigger_data *data, > } > } > > -static void > -hist_enable_count_trigger(struct event_trigger_data *data, > - struct trace_buffer *buffer, void *rec, > - struct ring_buffer_event *event) > -{ > - if (!data->count) > - return; > - > - if (data->count != -1) > - (data->count)--; > - > - hist_enable_trigger(data, buffer, rec, event); > -} > - > static const struct event_trigger_ops hist_enable_trigger_ops = { > .trigger = hist_enable_trigger, > - .print = event_enable_trigger_print, > - .init = event_trigger_init, > - .free = event_enable_trigger_free, > -}; > - > -static const struct event_trigger_ops hist_enable_count_trigger_ops = { > - .trigger = hist_enable_count_trigger, > + .count_func = event_trigger_count, > .print = event_enable_trigger_print, > .init = event_trigger_init, > .free = event_enable_trigger_free, > @@ -6975,36 +6949,12 @@ static const struct event_trigger_ops hist_enable_count_trigger_ops = { > > static const struct event_trigger_ops hist_disable_trigger_ops = { > .trigger = hist_enable_trigger, > + .count_func = event_trigger_count, > .print = event_enable_trigger_print, > .init = event_trigger_init, > .free = event_enable_trigger_free, > }; > > -static const struct event_trigger_ops hist_disable_count_trigger_ops = { > - .trigger = hist_enable_count_trigger, > - .print = event_enable_trigger_print, > - .init = event_trigger_init, > - .free = event_enable_trigger_free, > -}; > - > -static const struct event_trigger_ops * > -hist_enable_get_trigger_ops(char *cmd, char *param) > -{ > - const struct event_trigger_ops *ops; > - bool enable; > - > - enable = (strcmp(cmd, ENABLE_HIST_STR) == 0); > - > - if (enable) > - ops = param ? &hist_enable_count_trigger_ops : > - &hist_enable_trigger_ops; > - else > - ops = param ? &hist_disable_count_trigger_ops : > - &hist_disable_trigger_ops; > - > - return ops; > -} > - > static void hist_enable_unreg_all(struct trace_event_file *file) > { > struct event_trigger_data *test, *n; > @@ -7023,22 +6973,22 @@ static void hist_enable_unreg_all(struct trace_event_file *file) > static struct event_command trigger_hist_enable_cmd = { > .name = ENABLE_HIST_STR, > .trigger_type = ETT_HIST_ENABLE, > + .trigger_ops = &hist_enable_trigger_ops, > .parse = event_enable_trigger_parse, > .reg = event_enable_register_trigger, > .unreg = event_enable_unregister_trigger, > .unreg_all = hist_enable_unreg_all, > - .get_trigger_ops = hist_enable_get_trigger_ops, > .set_filter = set_trigger_filter, > }; > > static struct event_command trigger_hist_disable_cmd = { > .name = DISABLE_HIST_STR, > .trigger_type = ETT_HIST_ENABLE, > + .trigger_ops = &hist_disable_trigger_ops, > .parse = event_enable_trigger_parse, > .reg = event_enable_register_trigger, > .unreg = event_enable_unregister_trigger, > .unreg_all = hist_enable_unreg_all, > - .get_trigger_ops = hist_enable_get_trigger_ops, > .set_filter = set_trigger_filter, > }; > > diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c > index cbfc306c0159..576bad18bcdb 100644 > --- a/kernel/trace/trace_events_trigger.c > +++ b/kernel/trace/trace_events_trigger.c > @@ -28,6 +28,20 @@ void trigger_data_free(struct event_trigger_data *data) > kfree(data); > } > > +static inline void data_ops_trigger(struct event_trigger_data *data, > + struct trace_buffer *buffer, void *rec, > + struct ring_buffer_event *event) > +{ > + const struct event_trigger_ops *ops = data->ops; > + > + if (data->flags & EVENT_TRIGGER_FL_COUNT) { > + if (!ops->count_func(data, buffer, rec, event)) > + return; > + } > + > + ops->trigger(data, buffer, rec, event); > +} > + > /** > * event_triggers_call - Call triggers associated with a trace event > * @file: The trace_event_file associated with the event > @@ -70,7 +84,7 @@ event_triggers_call(struct trace_event_file *file, > if (data->paused) > continue; > if (!rec) { > - data->ops->trigger(data, buffer, rec, event); > + data_ops_trigger(data, buffer, rec, event); > continue; > } > filter = rcu_dereference_sched(data->filter); > @@ -80,7 +94,7 @@ event_triggers_call(struct trace_event_file *file, > tt |= data->cmd_ops->trigger_type; > continue; > } > - data->ops->trigger(data, buffer, rec, event); > + data_ops_trigger(data, buffer, rec, event); > } > return tt; > } > @@ -122,7 +136,7 @@ event_triggers_post_call(struct trace_event_file *file, > if (data->paused) > continue; > if (data->cmd_ops->trigger_type & tt) > - data->ops->trigger(data, NULL, NULL, NULL); > + data_ops_trigger(data, NULL, NULL, NULL); > } > } > EXPORT_SYMBOL_GPL(event_triggers_post_call); > @@ -377,6 +391,36 @@ __init int unregister_event_command(struct event_command *cmd) > return -ENODEV; > } > > +/** > + * event_trigger_count - Optional count function for event triggers > + * @data: Trigger-specific data > + * @buffer: The ring buffer that the event is being written to > + * @rec: The trace entry for the event, NULL for unconditional invocation > + * @event: The event meta data in the ring buffer > + * > + * For triggers that can take a count parameter that doesn't do anything > + * special, they can use this function to assign to their .count_func > + * field. > + * > + * This simply does a count down of the @data->count field. > + * > + * If the @data->count is greater than zero, it will decrement it. > + * > + * Returns false if @data->count is zero, otherwise true. > + */ > +bool event_trigger_count(struct event_trigger_data *data, > + struct trace_buffer *buffer, void *rec, > + struct ring_buffer_event *event) > +{ > + if (!data->count) > + return false; > + > + if (data->count != -1) > + (data->count)--; > + > + return true; > +} > + > /** > * event_trigger_print - Generic event_trigger_ops @print implementation > * @name: The name of the event trigger > @@ -807,9 +851,13 @@ int event_trigger_separate_filter(char *param_and_filter, char **param, > * @private_data: User data to associate with the event trigger > * > * Allocate an event_trigger_data instance and initialize it. The > - * @cmd_ops are used along with the @cmd and @param to get the > - * trigger_ops to assign to the event_trigger_data. @private_data can > - * also be passed in and associated with the event_trigger_data. > + * @cmd_ops defines how the trigger will operate. If @param is set, > + * and @cmd_ops->trigger_ops->count_func is non NULL, then the > + * data->count is set to @param and before the trigger is executed, the > + * @cmd_ops->trigger_ops->count_func() is called. If that function returns > + * false, the @cmd_ops->trigger_ops->trigger() function will not be called. > + * @private_data can also be passed in and associated with the > + * event_trigger_data. > * > * Use trigger_data_free() to free an event_trigger_data object. > * > @@ -821,18 +869,17 @@ struct event_trigger_data *trigger_data_alloc(struct event_command *cmd_ops, > void *private_data) > { > struct event_trigger_data *trigger_data; > - const struct event_trigger_ops *trigger_ops; > - > - trigger_ops = cmd_ops->get_trigger_ops(cmd, param); > > trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL); > if (!trigger_data) > return NULL; > > trigger_data->count = -1; > - trigger_data->ops = trigger_ops; > + trigger_data->ops = cmd_ops->trigger_ops; > trigger_data->cmd_ops = cmd_ops; > trigger_data->private_data = private_data; > + if (param && cmd_ops->trigger_ops->count_func) > + trigger_data->flags |= EVENT_TRIGGER_FL_COUNT; > > INIT_LIST_HEAD(&trigger_data->list); > INIT_LIST_HEAD(&trigger_data->named_list); > @@ -1271,31 +1318,28 @@ traceon_trigger(struct event_trigger_data *data, > tracing_on(); > } > > -static void > -traceon_count_trigger(struct event_trigger_data *data, > - struct trace_buffer *buffer, void *rec, > - struct ring_buffer_event *event) > +static bool > +traceon_count_func(struct event_trigger_data *data, > + struct trace_buffer *buffer, void *rec, > + struct ring_buffer_event *event) > { > struct trace_event_file *file = data->private_data; > > if (file) { > if (tracer_tracing_is_on(file->tr)) > - return; > + return false; > } else { > if (tracing_is_on()) > - return; > + return false; > } > > if (!data->count) > - return; > + return false; > > if (data->count != -1) > (data->count)--; > > - if (file) > - tracer_tracing_on(file->tr); > - else > - tracing_on(); > + return true; > } > > static void > @@ -1319,31 +1363,28 @@ traceoff_trigger(struct event_trigger_data *data, > tracing_off(); > } > > -static void > -traceoff_count_trigger(struct event_trigger_data *data, > - struct trace_buffer *buffer, void *rec, > - struct ring_buffer_event *event) > +static bool > +traceoff_count_func(struct event_trigger_data *data, > + struct trace_buffer *buffer, void *rec, > + struct ring_buffer_event *event) > { > struct trace_event_file *file = data->private_data; > > if (file) { > if (!tracer_tracing_is_on(file->tr)) > - return; > + return false; > } else { > if (!tracing_is_on()) > - return; > + return false; > } > > if (!data->count) > - return; > + return false; > > if (data->count != -1) > (data->count)--; > > - if (file) > - tracer_tracing_off(file->tr); > - else > - tracing_off(); > + return true; > } > > static int > @@ -1362,13 +1403,7 @@ traceoff_trigger_print(struct seq_file *m, struct event_trigger_data *data) > > static const struct event_trigger_ops traceon_trigger_ops = { > .trigger = traceon_trigger, > - .print = traceon_trigger_print, > - .init = event_trigger_init, > - .free = event_trigger_free, > -}; > - > -static const struct event_trigger_ops traceon_count_trigger_ops = { > - .trigger = traceon_count_trigger, > + .count_func = traceon_count_func, > .print = traceon_trigger_print, > .init = event_trigger_init, > .free = event_trigger_free, > @@ -1376,41 +1411,19 @@ static const struct event_trigger_ops traceon_count_trigger_ops = { > > static const struct event_trigger_ops traceoff_trigger_ops = { > .trigger = traceoff_trigger, > + .count_func = traceoff_count_func, > .print = traceoff_trigger_print, > .init = event_trigger_init, > .free = event_trigger_free, > }; > > -static const struct event_trigger_ops traceoff_count_trigger_ops = { > - .trigger = traceoff_count_trigger, > - .print = traceoff_trigger_print, > - .init = event_trigger_init, > - .free = event_trigger_free, > -}; > - > -static const struct event_trigger_ops * > -onoff_get_trigger_ops(char *cmd, char *param) > -{ > - const struct event_trigger_ops *ops; > - > - /* we register both traceon and traceoff to this callback */ > - if (strcmp(cmd, "traceon") == 0) > - ops = param ? &traceon_count_trigger_ops : > - &traceon_trigger_ops; > - else > - ops = param ? &traceoff_count_trigger_ops : > - &traceoff_trigger_ops; > - > - return ops; > -} > - > static struct event_command trigger_traceon_cmd = { > .name = "traceon", > .trigger_type = ETT_TRACE_ONOFF, > + .trigger_ops = &traceon_trigger_ops, > .parse = event_trigger_parse, > .reg = register_trigger, > .unreg = unregister_trigger, > - .get_trigger_ops = onoff_get_trigger_ops, > .set_filter = set_trigger_filter, > }; > > @@ -1418,10 +1431,10 @@ static struct event_command trigger_traceoff_cmd = { > .name = "traceoff", > .trigger_type = ETT_TRACE_ONOFF, > .flags = EVENT_CMD_FL_POST_TRIGGER, > + .trigger_ops = &traceoff_trigger_ops, > .parse = event_trigger_parse, > .reg = register_trigger, > .unreg = unregister_trigger, > - .get_trigger_ops = onoff_get_trigger_ops, > .set_filter = set_trigger_filter, > }; > > @@ -1439,20 +1452,6 @@ snapshot_trigger(struct event_trigger_data *data, > tracing_snapshot(); > } > > -static void > -snapshot_count_trigger(struct event_trigger_data *data, > - struct trace_buffer *buffer, void *rec, > - struct ring_buffer_event *event) > -{ > - if (!data->count) > - return; > - > - if (data->count != -1) > - (data->count)--; > - > - snapshot_trigger(data, buffer, rec, event); > -} > - > static int > register_snapshot_trigger(char *glob, > struct event_trigger_data *data, > @@ -1486,31 +1485,19 @@ snapshot_trigger_print(struct seq_file *m, struct event_trigger_data *data) > > static const struct event_trigger_ops snapshot_trigger_ops = { > .trigger = snapshot_trigger, > + .count_func = event_trigger_count, > .print = snapshot_trigger_print, > .init = event_trigger_init, > .free = event_trigger_free, > }; > > -static const struct event_trigger_ops snapshot_count_trigger_ops = { > - .trigger = snapshot_count_trigger, > - .print = snapshot_trigger_print, > - .init = event_trigger_init, > - .free = event_trigger_free, > -}; > - > -static const struct event_trigger_ops * > -snapshot_get_trigger_ops(char *cmd, char *param) > -{ > - return param ? &snapshot_count_trigger_ops : &snapshot_trigger_ops; > -} > - > static struct event_command trigger_snapshot_cmd = { > .name = "snapshot", > .trigger_type = ETT_SNAPSHOT, > + .trigger_ops = &snapshot_trigger_ops, > .parse = event_trigger_parse, > .reg = register_snapshot_trigger, > .unreg = unregister_snapshot_trigger, > - .get_trigger_ops = snapshot_get_trigger_ops, > .set_filter = set_trigger_filter, > }; > > @@ -1558,20 +1545,6 @@ stacktrace_trigger(struct event_trigger_data *data, > trace_dump_stack(STACK_SKIP); > } > > -static void > -stacktrace_count_trigger(struct event_trigger_data *data, > - struct trace_buffer *buffer, void *rec, > - struct ring_buffer_event *event) > -{ > - if (!data->count) > - return; > - > - if (data->count != -1) > - (data->count)--; > - > - stacktrace_trigger(data, buffer, rec, event); > -} > - > static int > stacktrace_trigger_print(struct seq_file *m, struct event_trigger_data *data) > { > @@ -1581,32 +1554,20 @@ stacktrace_trigger_print(struct seq_file *m, struct event_trigger_data *data) > > static const struct event_trigger_ops stacktrace_trigger_ops = { > .trigger = stacktrace_trigger, > + .count_func = event_trigger_count, > .print = stacktrace_trigger_print, > .init = event_trigger_init, > .free = event_trigger_free, > }; > > -static const struct event_trigger_ops stacktrace_count_trigger_ops = { > - .trigger = stacktrace_count_trigger, > - .print = stacktrace_trigger_print, > - .init = event_trigger_init, > - .free = event_trigger_free, > -}; > - > -static const struct event_trigger_ops * > -stacktrace_get_trigger_ops(char *cmd, char *param) > -{ > - return param ? &stacktrace_count_trigger_ops : &stacktrace_trigger_ops; > -} > - > static struct event_command trigger_stacktrace_cmd = { > .name = "stacktrace", > .trigger_type = ETT_STACKTRACE, > + .trigger_ops = &stacktrace_trigger_ops, > .flags = EVENT_CMD_FL_POST_TRIGGER, > .parse = event_trigger_parse, > .reg = register_trigger, > .unreg = unregister_trigger, > - .get_trigger_ops = stacktrace_get_trigger_ops, > .set_filter = set_trigger_filter, > }; > > @@ -1642,24 +1603,24 @@ event_enable_trigger(struct event_trigger_data *data, > set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &enable_data->file->flags); > } > > -static void > -event_enable_count_trigger(struct event_trigger_data *data, > - struct trace_buffer *buffer, void *rec, > - struct ring_buffer_event *event) > +static bool > +event_enable_count_func(struct event_trigger_data *data, > + struct trace_buffer *buffer, void *rec, > + struct ring_buffer_event *event) > { > struct enable_trigger_data *enable_data = data->private_data; > > if (!data->count) > - return; > + return false; > > /* Skip if the event is in a state we want to switch to */ > if (enable_data->enable == !(enable_data->file->flags & EVENT_FILE_FL_SOFT_DISABLED)) > - return; > + return false; > > if (data->count != -1) > (data->count)--; > > - event_enable_trigger(data, buffer, rec, event); > + return true; > } > > int event_enable_trigger_print(struct seq_file *m, > @@ -1706,13 +1667,7 @@ void event_enable_trigger_free(struct event_trigger_data *data) > > static const struct event_trigger_ops event_enable_trigger_ops = { > .trigger = event_enable_trigger, > - .print = event_enable_trigger_print, > - .init = event_trigger_init, > - .free = event_enable_trigger_free, > -}; > - > -static const struct event_trigger_ops event_enable_count_trigger_ops = { > - .trigger = event_enable_count_trigger, > + .count_func = event_enable_count_func, > .print = event_enable_trigger_print, > .init = event_trigger_init, > .free = event_enable_trigger_free, > @@ -1720,13 +1675,7 @@ static const struct event_trigger_ops event_enable_count_trigger_ops = { > > static const struct event_trigger_ops event_disable_trigger_ops = { > .trigger = event_enable_trigger, > - .print = event_enable_trigger_print, > - .init = event_trigger_init, > - .free = event_enable_trigger_free, > -}; > - > -static const struct event_trigger_ops event_disable_count_trigger_ops = { > - .trigger = event_enable_count_trigger, > + .count_func = event_enable_count_func, > .print = event_enable_trigger_print, > .init = event_trigger_init, > .free = event_enable_trigger_free, > @@ -1906,45 +1855,23 @@ void event_enable_unregister_trigger(char *glob, > data->ops->free(data); > } > > -static const struct event_trigger_ops * > -event_enable_get_trigger_ops(char *cmd, char *param) > -{ > - const struct event_trigger_ops *ops; > - bool enable; > - > -#ifdef CONFIG_HIST_TRIGGERS > - enable = ((strcmp(cmd, ENABLE_EVENT_STR) == 0) || > - (strcmp(cmd, ENABLE_HIST_STR) == 0)); > -#else > - enable = strcmp(cmd, ENABLE_EVENT_STR) == 0; > -#endif > - if (enable) > - ops = param ? &event_enable_count_trigger_ops : > - &event_enable_trigger_ops; > - else > - ops = param ? &event_disable_count_trigger_ops : > - &event_disable_trigger_ops; > - > - return ops; > -} > - > static struct event_command trigger_enable_cmd = { > .name = ENABLE_EVENT_STR, > .trigger_type = ETT_EVENT_ENABLE, > + .trigger_ops = &event_enable_trigger_ops, > .parse = event_enable_trigger_parse, > .reg = event_enable_register_trigger, > .unreg = event_enable_unregister_trigger, > - .get_trigger_ops = event_enable_get_trigger_ops, > .set_filter = set_trigger_filter, > }; > > static struct event_command trigger_disable_cmd = { > .name = DISABLE_EVENT_STR, > .trigger_type = ETT_EVENT_ENABLE, > + .trigger_ops = &event_disable_trigger_ops, > .parse = event_enable_trigger_parse, > .reg = event_enable_register_trigger, > .unreg = event_enable_unregister_trigger, > - .get_trigger_ops = event_enable_get_trigger_ops, > .set_filter = set_trigger_filter, > }; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] tracing: Merge struct event_trigger_ops into struct event_command 2025-11-19 3:10 [PATCH 0/2] tracing: Clean up trigger code my merging structures Steven Rostedt 2025-11-19 3:10 ` [PATCH 1/2] tracing: Remove get_trigger_ops() and add count_func() from trigger ops Steven Rostedt @ 2025-11-19 3:10 ` Steven Rostedt 2025-11-25 13:01 ` Tom Zanussi 1 sibling, 1 reply; 6+ messages in thread From: Steven Rostedt @ 2025-11-19 3:10 UTC (permalink / raw) To: linux-kernel, linux-trace-kernel Cc: Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton, Tom Zanussi 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> --- 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.h b/kernel/trace/trace.h index 036019ffc407..5863800b1ab3 100644 --- a/kernel/trace/trace.h +++ b/kernel/trace/trace.h @@ -1798,7 +1798,6 @@ struct event_trigger_data { unsigned long count; int ref; int flags; - const struct event_trigger_ops *ops; struct event_command *cmd_ops; struct event_filter __rcu *filter; char *filter_str; @@ -1889,73 +1888,6 @@ extern void event_trigger_unregister(struct event_command *cmd_ops, extern void event_file_get(struct trace_event_file *file); extern void event_file_put(struct trace_event_file *file); -/** - * struct event_trigger_ops - callbacks for trace event triggers - * - * The methods in this structure provide per-event trigger hooks for - * various trigger operations. - * - * The @init and @free methods are used during trigger setup and - * teardown, typically called from an event_command's @parse() - * function implementation. - * - * The @print method is used to print the trigger spec. - * - * The @trigger method is the function that actually implements the - * trigger and is called in the context of the triggering event - * whenever that event occurs. - * - * All the methods below, except for @init() and @free(), must be - * implemented. - * - * @trigger: The trigger 'probe' function called when the triggering - * event occurs. The data passed into this callback is the data - * that was supplied to the event_command @reg() function that - * registered the trigger (see struct event_command) along with - * the trace record, rec. - * - * @count_func: If defined and a numeric parameter is passed to the - * trigger, then this function will be called before @trigger - * is called. If this function returns false, then @trigger is not - * executed. - * - * @init: An optional initialization function called for the trigger - * when the trigger is registered (via the event_command reg() - * function). This can be used to perform per-trigger - * initialization such as incrementing a per-trigger reference - * count, for instance. This is usually implemented by the - * generic utility function @event_trigger_init() (see - * trace_event_triggers.c). - * - * @free: An optional de-initialization function called for the - * trigger when the trigger is unregistered (via the - * event_command @reg() function). This can be used to perform - * per-trigger de-initialization such as decrementing a - * per-trigger reference count and freeing corresponding trigger - * data, for instance. This is usually implemented by the - * generic utility function @event_trigger_free() (see - * trace_event_triggers.c). - * - * @print: The callback function invoked to have the trigger print - * itself. This is usually implemented by a wrapper function - * that calls the generic utility function @event_trigger_print() - * (see trace_event_triggers.c). - */ -struct event_trigger_ops { - void (*trigger)(struct event_trigger_data *data, - struct trace_buffer *buffer, - void *rec, - struct ring_buffer_event *rbe); - bool (*count_func)(struct event_trigger_data *data, - struct trace_buffer *buffer, - void *rec, - struct ring_buffer_event *rbe); - int (*init)(struct event_trigger_data *data); - void (*free)(struct event_trigger_data *data); - int (*print)(struct seq_file *m, - struct event_trigger_data *data); -}; - /** * struct event_command - callbacks and data members for event commands * @@ -1976,9 +1908,6 @@ struct event_trigger_ops { * @name: The unique name that identifies the event command. This is * the name used when setting triggers via trigger files. * - * @trigger_ops: The event_trigger_ops implementation associated with - * the command. - * * @trigger_type: A unique id that identifies the event command * 'type'. This value has two purposes, the first to ensure that * only one trigger of the same type can be set at a given time @@ -2008,7 +1937,7 @@ struct event_trigger_ops { * * @reg: Adds the trigger to the list of triggers associated with the * event, and enables the event trigger itself, after - * initializing it (via the event_trigger_ops @init() function). + * initializing it (via the event_command @init() function). * This is also where commands can use the @trigger_type value to * make the decision as to whether or not multiple instances of * the trigger should be allowed. This is usually implemented by @@ -2017,7 +1946,7 @@ struct event_trigger_ops { * * @unreg: Removes the trigger from the list of triggers associated * with the event, and disables the event trigger itself, after - * initializing it (via the event_trigger_ops @free() function). + * initializing it (via the event_command @free() function). * This is usually implemented by the generic utility function * @unregister_trigger() (see trace_event_triggers.c). * @@ -2030,11 +1959,46 @@ struct event_trigger_ops { * event command, filters set by the user for the command will be * ignored. This is usually implemented by the generic utility * function @set_trigger_filter() (see trace_event_triggers.c). + * + * All the methods below, except for @init() and @free(), must be + * implemented. + * + * @trigger: The trigger 'probe' function called when the triggering + * event occurs. The data passed into this callback is the data + * that was supplied to the event_command @reg() function that + * registered the trigger (see struct event_command) along with + * the trace record, rec. + * + * @count_func: If defined and a numeric parameter is passed to the + * trigger, then this function will be called before @trigger + * is called. If this function returns false, then @trigger is not + * executed. + * + * @init: An optional initialization function called for the trigger + * when the trigger is registered (via the event_command reg() + * function). This can be used to perform per-trigger + * initialization such as incrementing a per-trigger reference + * count, for instance. This is usually implemented by the + * generic utility function @event_trigger_init() (see + * trace_event_triggers.c). + * + * @free: An optional de-initialization function called for the + * trigger when the trigger is unregistered (via the + * event_command @reg() function). This can be used to perform + * per-trigger de-initialization such as decrementing a + * per-trigger reference count and freeing corresponding trigger + * data, for instance. This is usually implemented by the + * generic utility function @event_trigger_free() (see + * trace_event_triggers.c). + * + * @print: The callback function invoked to have the trigger print + * itself. This is usually implemented by a wrapper function + * that calls the generic utility function @event_trigger_print() + * (see trace_event_triggers.c). */ struct event_command { struct list_head list; char *name; - const struct event_trigger_ops *trigger_ops; enum event_trigger_type trigger_type; int flags; int (*parse)(struct event_command *cmd_ops, @@ -2051,6 +2015,18 @@ struct event_command { int (*set_filter)(char *filter_str, struct event_trigger_data *data, struct trace_event_file *file); + void (*trigger)(struct event_trigger_data *data, + struct trace_buffer *buffer, + void *rec, + struct ring_buffer_event *rbe); + bool (*count_func)(struct event_trigger_data *data, + struct trace_buffer *buffer, + void *rec, + struct ring_buffer_event *rbe); + int (*init)(struct event_trigger_data *data); + void (*free)(struct event_trigger_data *data); + int (*print)(struct seq_file *m, + struct event_trigger_data *data); }; /** @@ -2071,7 +2047,7 @@ struct event_command { * either committed or discarded. At that point, if any commands * have deferred their triggers, those commands are finally * invoked following the close of the current event. In other - * words, if the event_trigger_ops @func() probe implementation + * words, if the event_command @func() probe implementation * itself logs to the trace buffer, this flag should be set, * otherwise it can be left unspecified. * diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c index 14ae896dbe75..f3e0442c3b96 100644 --- a/kernel/trace/trace_eprobe.c +++ b/kernel/trace/trace_eprobe.c @@ -484,13 +484,6 @@ static void eprobe_trigger_func(struct event_trigger_data *data, __eprobe_trace_func(edata, rec); } -static const struct event_trigger_ops eprobe_trigger_ops = { - .trigger = eprobe_trigger_func, - .print = eprobe_trigger_print, - .init = eprobe_trigger_init, - .free = eprobe_trigger_free, -}; - static int eprobe_trigger_cmd_parse(struct event_command *cmd_ops, struct trace_event_file *file, char *glob, char *cmd, @@ -517,12 +510,15 @@ static struct event_command event_trigger_cmd = { .name = "eprobe", .trigger_type = ETT_EVENT_EPROBE, .flags = EVENT_CMD_FL_NEEDS_REC, - .trigger_ops = &eprobe_trigger_ops, .parse = eprobe_trigger_cmd_parse, .reg = eprobe_trigger_reg_func, .unreg = eprobe_trigger_unreg_func, .unreg_all = NULL, .set_filter = NULL, + .trigger = eprobe_trigger_func, + .print = eprobe_trigger_print, + .init = eprobe_trigger_init, + .free = eprobe_trigger_free, }; static struct event_trigger_data * @@ -542,7 +538,6 @@ new_eprobe_trigger(struct trace_eprobe *ep, struct trace_event_file *file) trigger->flags = EVENT_TRIGGER_FL_PROBE; trigger->count = -1; - trigger->ops = &eprobe_trigger_ops; /* * EVENT PROBE triggers are not registered as commands with 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; static int event_hist_trigger_named_init(struct event_trigger_data *data) { + int ret; + data->ref++; save_named_trigger(data->named_data->name, data); - return event_hist_trigger_init(data->named_data); + ret = event_hist_trigger_init(data->named_data); + if (ret < 0) { + kfree(data->cmd_ops); + data->cmd_ops = &trigger_hist_cmd; + } + + return ret; } static void event_hist_trigger_named_free(struct event_trigger_data *data) @@ -6351,18 +6354,14 @@ static void event_hist_trigger_named_free(struct event_trigger_data *data) data->ref--; if (!data->ref) { + struct event_command *cmd_ops = data->cmd_ops; + del_named_trigger(data); trigger_data_free(data); + kfree(cmd_ops); } } -static const struct event_trigger_ops event_hist_trigger_named_ops = { - .trigger = event_hist_trigger, - .print = event_hist_trigger_print, - .init = event_hist_trigger_named_init, - .free = event_hist_trigger_named_free, -}; - static void hist_clear(struct event_trigger_data *data) { struct hist_trigger_data *hist_data = data->private_data; @@ -6556,13 +6555,24 @@ static int hist_register_trigger(char *glob, data->paused = true; if (named_data) { + struct event_command *cmd_ops; + data->private_data = named_data->private_data; set_named_trigger_data(data, named_data); - data->ops = &event_hist_trigger_named_ops; + /* Copy the command ops and update some of the functions */ + cmd_ops = kmalloc(sizeof(*cmd_ops), GFP_KERNEL); + if (!cmd_ops) { + ret = -ENOMEM; + goto out; + } + *cmd_ops = *data->cmd_ops; + cmd_ops->init = event_hist_trigger_named_init; + cmd_ops->free = event_hist_trigger_named_free; + data->cmd_ops = cmd_ops; } - if (data->ops->init) { - ret = data->ops->init(data); + if (data->cmd_ops->init) { + ret = data->cmd_ops->init(data); if (ret < 0) goto out; } @@ -6676,8 +6686,8 @@ static void hist_unregister_trigger(char *glob, } } - if (test && test->ops->free) - test->ops->free(test); + if (test && test->cmd_ops->free) + test->cmd_ops->free(test); if (hist_data->enable_timestamps) { if (!hist_data->remove || test) @@ -6729,8 +6739,8 @@ static void hist_unreg_all(struct trace_event_file *file) update_cond_flag(file); if (hist_data->enable_timestamps) tracing_set_filter_buffering(file->tr, false); - if (test->ops->free) - test->ops->free(test); + if (test->cmd_ops->free) + test->cmd_ops->free(test); } } } @@ -6902,12 +6912,15 @@ static struct event_command trigger_hist_cmd = { .name = "hist", .trigger_type = ETT_EVENT_HIST, .flags = EVENT_CMD_FL_NEEDS_REC, - .trigger_ops = &event_hist_trigger_ops, .parse = event_hist_trigger_parse, .reg = hist_register_trigger, .unreg = hist_unregister_trigger, .unreg_all = hist_unreg_all, .set_filter = set_trigger_filter, + .trigger = event_hist_trigger, + .print = event_hist_trigger_print, + .init = event_hist_trigger_init, + .free = event_hist_trigger_free, }; __init int register_trigger_hist_cmd(void) @@ -6939,22 +6952,6 @@ hist_enable_trigger(struct event_trigger_data *data, } } -static const struct event_trigger_ops hist_enable_trigger_ops = { - .trigger = hist_enable_trigger, - .count_func = event_trigger_count, - .print = event_enable_trigger_print, - .init = event_trigger_init, - .free = event_enable_trigger_free, -}; - -static const struct event_trigger_ops hist_disable_trigger_ops = { - .trigger = hist_enable_trigger, - .count_func = event_trigger_count, - .print = event_enable_trigger_print, - .init = event_trigger_init, - .free = event_enable_trigger_free, -}; - static void hist_enable_unreg_all(struct trace_event_file *file) { struct event_trigger_data *test, *n; @@ -6964,8 +6961,8 @@ static void hist_enable_unreg_all(struct trace_event_file *file) list_del_rcu(&test->list); update_cond_flag(file); trace_event_trigger_enable_disable(file, 0); - if (test->ops->free) - test->ops->free(test); + if (test->cmd_ops->free) + test->cmd_ops->free(test); } } } @@ -6973,23 +6970,31 @@ static void hist_enable_unreg_all(struct trace_event_file *file) static struct event_command trigger_hist_enable_cmd = { .name = ENABLE_HIST_STR, .trigger_type = ETT_HIST_ENABLE, - .trigger_ops = &hist_enable_trigger_ops, .parse = event_enable_trigger_parse, .reg = event_enable_register_trigger, .unreg = event_enable_unregister_trigger, .unreg_all = hist_enable_unreg_all, .set_filter = set_trigger_filter, + .trigger = hist_enable_trigger, + .count_func = event_trigger_count, + .print = event_enable_trigger_print, + .init = event_trigger_init, + .free = event_enable_trigger_free, }; static struct event_command trigger_hist_disable_cmd = { .name = DISABLE_HIST_STR, .trigger_type = ETT_HIST_ENABLE, - .trigger_ops = &hist_disable_trigger_ops, .parse = event_enable_trigger_parse, .reg = event_enable_register_trigger, .unreg = event_enable_unregister_trigger, .unreg_all = hist_enable_unreg_all, .set_filter = set_trigger_filter, + .trigger = hist_enable_trigger, + .count_func = event_trigger_count, + .print = event_enable_trigger_print, + .init = event_trigger_init, + .free = event_enable_trigger_free, }; static __init void unregister_trigger_hist_enable_disable_cmds(void) diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c index 576bad18bcdb..7795af600466 100644 --- a/kernel/trace/trace_events_trigger.c +++ b/kernel/trace/trace_events_trigger.c @@ -32,14 +32,14 @@ static inline void data_ops_trigger(struct event_trigger_data *data, struct trace_buffer *buffer, void *rec, struct ring_buffer_event *event) { - const struct event_trigger_ops *ops = data->ops; + const struct event_command *cmd_ops = data->cmd_ops; if (data->flags & EVENT_TRIGGER_FL_COUNT) { - if (!ops->count_func(data, buffer, rec, event)) + if (!cmd_ops->count_func(data, buffer, rec, event)) return; } - ops->trigger(data, buffer, rec, event); + cmd_ops->trigger(data, buffer, rec, event); } /** @@ -205,7 +205,7 @@ static int trigger_show(struct seq_file *m, void *v) } data = list_entry(v, struct event_trigger_data, list); - data->ops->print(m, data); + data->cmd_ops->print(m, data); return 0; } @@ -422,7 +422,7 @@ bool event_trigger_count(struct event_trigger_data *data, } /** - * event_trigger_print - Generic event_trigger_ops @print implementation + * event_trigger_print - Generic event_command @print implementation * @name: The name of the event trigger * @m: The seq_file being printed to * @data: Trigger-specific data @@ -457,7 +457,7 @@ event_trigger_print(const char *name, struct seq_file *m, } /** - * event_trigger_init - Generic event_trigger_ops @init implementation + * event_trigger_init - Generic event_command @init implementation * @data: Trigger-specific data * * Common implementation of event trigger initialization. @@ -474,7 +474,7 @@ int event_trigger_init(struct event_trigger_data *data) } /** - * event_trigger_free - Generic event_trigger_ops @free implementation + * event_trigger_free - Generic event_command @free implementation * @data: Trigger-specific data * * Common implementation of event trigger de-initialization. @@ -536,8 +536,8 @@ clear_event_triggers(struct trace_array *tr) list_for_each_entry_safe(data, n, &file->triggers, list) { trace_event_trigger_enable_disable(file, 0); list_del_rcu(&data->list); - if (data->ops->free) - data->ops->free(data); + if (data->cmd_ops->free) + data->cmd_ops->free(data); } } } @@ -600,8 +600,8 @@ static int register_trigger(char *glob, return -EEXIST; } - if (data->ops->init) { - ret = data->ops->init(data); + if (data->cmd_ops->init) { + ret = data->cmd_ops->init(data); if (ret < 0) return ret; } @@ -639,8 +639,8 @@ static bool try_unregister_trigger(char *glob, } if (data) { - if (data->ops->free) - data->ops->free(data); + if (data->cmd_ops->free) + data->cmd_ops->free(data); return true; } @@ -875,10 +875,9 @@ struct event_trigger_data *trigger_data_alloc(struct event_command *cmd_ops, return NULL; trigger_data->count = -1; - trigger_data->ops = cmd_ops->trigger_ops; trigger_data->cmd_ops = cmd_ops; trigger_data->private_data = private_data; - if (param && cmd_ops->trigger_ops->count_func) + if (param && cmd_ops->count_func) trigger_data->flags |= EVENT_TRIGGER_FL_COUNT; INIT_LIST_HEAD(&trigger_data->list); @@ -1401,41 +1400,33 @@ traceoff_trigger_print(struct seq_file *m, struct event_trigger_data *data) data->filter_str); } -static const struct event_trigger_ops traceon_trigger_ops = { - .trigger = traceon_trigger, - .count_func = traceon_count_func, - .print = traceon_trigger_print, - .init = event_trigger_init, - .free = event_trigger_free, -}; - -static const struct event_trigger_ops traceoff_trigger_ops = { - .trigger = traceoff_trigger, - .count_func = traceoff_count_func, - .print = traceoff_trigger_print, - .init = event_trigger_init, - .free = event_trigger_free, -}; - static struct event_command trigger_traceon_cmd = { .name = "traceon", .trigger_type = ETT_TRACE_ONOFF, - .trigger_ops = &traceon_trigger_ops, .parse = event_trigger_parse, .reg = register_trigger, .unreg = unregister_trigger, .set_filter = set_trigger_filter, + .trigger = traceon_trigger, + .count_func = traceon_count_func, + .print = traceon_trigger_print, + .init = event_trigger_init, + .free = event_trigger_free, }; static struct event_command trigger_traceoff_cmd = { .name = "traceoff", .trigger_type = ETT_TRACE_ONOFF, .flags = EVENT_CMD_FL_POST_TRIGGER, - .trigger_ops = &traceoff_trigger_ops, .parse = event_trigger_parse, .reg = register_trigger, .unreg = unregister_trigger, .set_filter = set_trigger_filter, + .trigger = traceoff_trigger, + .count_func = traceoff_count_func, + .print = traceoff_trigger_print, + .init = event_trigger_init, + .free = event_trigger_free, }; #ifdef CONFIG_TRACER_SNAPSHOT @@ -1483,22 +1474,18 @@ snapshot_trigger_print(struct seq_file *m, struct event_trigger_data *data) data->filter_str); } -static const struct event_trigger_ops snapshot_trigger_ops = { - .trigger = snapshot_trigger, - .count_func = event_trigger_count, - .print = snapshot_trigger_print, - .init = event_trigger_init, - .free = event_trigger_free, -}; - static struct event_command trigger_snapshot_cmd = { .name = "snapshot", .trigger_type = ETT_SNAPSHOT, - .trigger_ops = &snapshot_trigger_ops, .parse = event_trigger_parse, .reg = register_snapshot_trigger, .unreg = unregister_snapshot_trigger, .set_filter = set_trigger_filter, + .trigger = snapshot_trigger, + .count_func = event_trigger_count, + .print = snapshot_trigger_print, + .init = event_trigger_init, + .free = event_trigger_free, }; static __init int register_trigger_snapshot_cmd(void) @@ -1552,23 +1539,19 @@ stacktrace_trigger_print(struct seq_file *m, struct event_trigger_data *data) data->filter_str); } -static const struct event_trigger_ops stacktrace_trigger_ops = { - .trigger = stacktrace_trigger, - .count_func = event_trigger_count, - .print = stacktrace_trigger_print, - .init = event_trigger_init, - .free = event_trigger_free, -}; - static struct event_command trigger_stacktrace_cmd = { .name = "stacktrace", .trigger_type = ETT_STACKTRACE, - .trigger_ops = &stacktrace_trigger_ops, .flags = EVENT_CMD_FL_POST_TRIGGER, .parse = event_trigger_parse, .reg = register_trigger, .unreg = unregister_trigger, .set_filter = set_trigger_filter, + .trigger = stacktrace_trigger, + .count_func = event_trigger_count, + .print = stacktrace_trigger_print, + .init = event_trigger_init, + .free = event_trigger_free, }; static __init int register_trigger_stacktrace_cmd(void) @@ -1665,22 +1648,6 @@ void event_enable_trigger_free(struct event_trigger_data *data) } } -static const struct event_trigger_ops event_enable_trigger_ops = { - .trigger = event_enable_trigger, - .count_func = event_enable_count_func, - .print = event_enable_trigger_print, - .init = event_trigger_init, - .free = event_enable_trigger_free, -}; - -static const struct event_trigger_ops event_disable_trigger_ops = { - .trigger = event_enable_trigger, - .count_func = event_enable_count_func, - .print = event_enable_trigger_print, - .init = event_trigger_init, - .free = event_enable_trigger_free, -}; - int event_enable_trigger_parse(struct event_command *cmd_ops, struct trace_event_file *file, char *glob, char *cmd, char *param_and_filter) @@ -1810,8 +1777,8 @@ int event_enable_register_trigger(char *glob, } } - if (data->ops->init) { - ret = data->ops->init(data); + if (data->cmd_ops->init) { + ret = data->cmd_ops->init(data); if (ret < 0) return ret; } @@ -1851,28 +1818,36 @@ void event_enable_unregister_trigger(char *glob, } } - if (data && data->ops->free) - data->ops->free(data); + if (data && data->cmd_ops->free) + data->cmd_ops->free(data); } static struct event_command trigger_enable_cmd = { .name = ENABLE_EVENT_STR, .trigger_type = ETT_EVENT_ENABLE, - .trigger_ops = &event_enable_trigger_ops, .parse = event_enable_trigger_parse, .reg = event_enable_register_trigger, .unreg = event_enable_unregister_trigger, .set_filter = set_trigger_filter, + .trigger = event_enable_trigger, + .count_func = event_enable_count_func, + .print = event_enable_trigger_print, + .init = event_trigger_init, + .free = event_enable_trigger_free, }; static struct event_command trigger_disable_cmd = { .name = DISABLE_EVENT_STR, .trigger_type = ETT_EVENT_ENABLE, - .trigger_ops = &event_disable_trigger_ops, .parse = event_enable_trigger_parse, .reg = event_enable_register_trigger, .unreg = event_enable_unregister_trigger, .set_filter = set_trigger_filter, + .trigger = event_enable_trigger, + .count_func = event_enable_count_func, + .print = event_enable_trigger_print, + .init = event_trigger_init, + .free = event_enable_trigger_free, }; static __init void unregister_trigger_enable_disable_cmds(void) -- 2.51.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] tracing: Merge struct event_trigger_ops into struct event_command 2025-11-19 3:10 ` [PATCH 2/2] tracing: Merge struct event_trigger_ops into struct event_command Steven Rostedt @ 2025-11-25 13:01 ` Tom Zanussi 2025-11-25 20:05 ` Steven Rostedt 0 siblings, 1 reply; 6+ messages in thread From: Tom Zanussi @ 2025-11-25 13:01 UTC (permalink / raw) To: Steven Rostedt, linux-kernel, linux-trace-kernel Cc: Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] tracing: Merge struct event_trigger_ops into struct event_command 2025-11-25 13:01 ` Tom Zanussi @ 2025-11-25 20:05 ` Steven Rostedt 0 siblings, 0 replies; 6+ messages in thread From: Steven Rostedt @ 2025-11-25 20:05 UTC (permalink / raw) To: Tom Zanussi Cc: Steven Rostedt, linux-kernel, linux-trace-kernel, Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton On Tue, 25 Nov 2025 07:01:43 -0600 Tom Zanussi <zanussi@kernel.org> wrote: > > -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. No it doesn't. Nice catch. I'll send a v2. Thanks for the review. -- Steve ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-11-25 20:04 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-11-19 3:10 [PATCH 0/2] tracing: Clean up trigger code my merging structures Steven Rostedt 2025-11-19 3:10 ` [PATCH 1/2] tracing: Remove get_trigger_ops() and add count_func() from trigger ops Steven Rostedt 2025-11-25 12:54 ` Tom Zanussi 2025-11-19 3:10 ` [PATCH 2/2] tracing: Merge struct event_trigger_ops into struct event_command Steven Rostedt 2025-11-25 13:01 ` Tom Zanussi 2025-11-25 20:05 ` Steven Rostedt
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).