Re: [PATCH] tracing: Add system trigger file to enable triggers for all the system's events

[Tom Zanussi <zanussi@kernel.org>]

Hi Steve,

On Wed, 2025-11-26 at 15:24 -0500, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
> 
> Currently a trigger can only be added to individual events. Some triggers
> (like stacktrace) can be useful to add as a bulk trigger for a set of
> system events (like interrupt or scheduling).
> 
> Add a trigger file to the system directories:
> 
>    /sys/kernel/tracing/events/*/trigger
> 
> And allow stacktrace trigger to be enabled for all those events.
> 
> Writing into the system/trigger file acts the same as writing into each of
> the system event's trigger files individually.
> 
> This also allows to remove a trigger from all events in a subsystem (even
> if it's not a subsystem trigger!).
> 

This looks very useful! Just a couple comments below..

> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> Changes since v2: https://patch.msgid.link/20251125200837.31aae207@gandalf.local.home
> 
> - Removed unneeded NULL initialization of tr (Masami Hiramatsu)
> 
> 
> 

[ ... ]

> +static ssize_t
> +event_system_trigger_write(struct file *filp, const char __user *ubuf,
> +		    size_t cnt, loff_t *ppos)
> +{
> +	struct trace_subsystem_dir *dir = filp->private_data;
> +	struct event_command *p;
> +	char *command, *next;
> +	char *buf __free(kfree) = get_user_buf(ubuf, cnt);

nit: I think you could move this up a couple lines and keep the reverse
Christmas tree.

> +	bool remove = false;
> +	bool found = false;
> +	ssize_t ret;
> +
> +	if (!buf)
> +		return 0;
> +
> +	if (IS_ERR(buf))
> +		return PTR_ERR(buf);
> +
> +	/* system triggers are not allowed to have counters */
> +	if (strchr(buf, ':'))
> +		return -EINVAL;
> +

As mentioned by Masami, I think this would preclude the hist triggers
from having this enabled.

I'm guessing you didn't see the hist commands as one of the triggers
that would be useful as system triggers, but there might be a couple
cases where they could be if using the common fields e.g.

echo hist:keys=common_pid.execname:vals=hitcount > events/syscalls/trigger 

or anywhere just

echo hist:keys=common_stacktrace > trigger

Just about everything else would error out, but these might be worth having.

Maybe you could just change this check to look for :<number> at the end of
the command?

Thanks,

Tom