Message-Id: <20250324152945.e47bc6d1e491658cfc6924fe@kernel.org>
Date: Mon, 24 Mar 2025 15:29:45 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Douglas Raillard <douglas.raillard@....com>
Cc: rostedt@...dmis.org, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] tracing: Rename trace_synth() to
 synth_event_trace2()

On Wed, 19 Mar 2025 14:51:42 +0000
Douglas Raillard <douglas.raillard@....com> wrote:

> On 19-03-2025 13:37, Masami Hiramatsu (Google) wrote:
> > On Tue, 18 Mar 2025 18:08:12 +0000
> > Douglas RAILLARD <douglas.raillard@....com> wrote:
> > 
> >> From: Douglas Raillard <douglas.raillard@....com>
> >>
> >> Rename the frehsly exposed trace_synth() to synth_event_trace2() to
> >> comply with the existing naming convention. Since synth_event_trace()
> >> already exists (and operates on a "struct trace_event_file *"), use a
> >> new name for it.
> >>
> > 
> > I don't like this '2' and similar version digit naming for the functions.
> > Can you choose another better name?
> 
> I was hoping for some suggestions as I don't like it either :)
> 
> The natural prefix for functions operating on "struct synth_event *" would by "synth_event_*",
> but most of the existing API already uses the "synth_event_*" prefix, and is using
> "struct trace_event_file*".
> 
> > BTW, can you also write a cover mail so that what is the goal of this
> > series, background and results? 
> 
> Ok, I'll respin the series with a proper cover letter. The gist is I was following the doc [1] on how
> to create a synthetic event, trying to apply that to a kernel module we have that needs to create new events.
> 
> Unfortunately, it turned out that all the exposed APIs to emit the events (such as synth_event_trace()) require
> getting a "struct trace_event_file*" before the call. This breaks when a user starts creating instances in tracefs,
> as each instance will have its own "struct trace_event_file*".

Yeah, because those are mainly for the tests, and we are expecting that if
any modules wants to emit its events, it will define new trace-events and
use it instead of synthetic events. The synthetic events are for
programming via tracefs, not reporting from the kernel modules.
It is confusing if any synthetic events are reported without any origin of
real trace event. (so, it is an intermadiate event type.) IOW, We expect
that synthetic event is reported by other events via event trigger.
The current APIs are just for testing.

Hmm, I should hide those by CONFIG_SYNTH_EVENT_TESTS.

> The way this is done for normal trace events is
> that each instance registers a probe on the tracepoint with the struct trace_event_file* as the void *data pointer.
> Then when the tracepoint is called, all the probes are called and the event data is copied in all instances in which
> it was enabled.
> 
> Although the synthetic event API does create that tracepoint and has an appropriate probe function, none of the exposed API
> I could find make use of it. The exception is trace_events_hist.c had its own private version of it that actually calls all
> the probes of the tracepoint "manually", so that the event is correctly emitted in all the instances it was enabled in. This
> is the function touched by this patch.

No, please don't touch the synthetic events for reporting any events via
kernel modules. Those should use normal trace events for avoiding the
confusion.

Would you have any reason not to use normal trace events?

Thank you,

> 
> So that means that as it stands:
> 1. The exposed API is only really usable with the "NULL" struct trace_event_file*, which maps to the top-level one.
> 2. If a user creates an instance and enables the event in it using tracefs, the code that emits the event using the existing API
>     will completely ignore that and keep emitting the event in the top-level instance that it was wired to do.
> 
> Approximately nothing in the synth event API that takes a "struct trace_event_file*" will work properly with user-created instances.
> 
> [1] https://docs.kernel.org/trace/events.html#dyamically-creating-synthetic-event-definitions
> 
> > 
> > Thank you,
> > 
> >> Signed-off-by: Douglas Raillard <douglas.raillard@....com>
> >> ---
> >>   include/linux/trace_events.h      | 2 +-
> >>   kernel/trace/trace_events_hist.c  | 2 +-
> >>   kernel/trace/trace_events_synth.c | 4 ++--
> >>   3 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> >> index e069d84a73f0..753ce8aecfe4 100644
> >> --- a/include/linux/trace_events.h
> >> +++ b/include/linux/trace_events.h
> >> @@ -521,7 +521,7 @@ struct synth_event;
> >>   
> >>   extern struct synth_event *synth_event_find(const char *name);
> >>   
> >> -extern void trace_synth(struct synth_event *event, u64 *var_ref_vals,
> >> +extern void synth_event_trace2(struct synth_event *event, u64 *var_ref_vals,
> >>   			       unsigned int *var_ref_idx);
> >>   
> >>   extern int synth_event_delete(const char *name);
> >> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> >> index 7067f6fedb1a..ee0fee123c91 100644
> >> --- a/kernel/trace/trace_events_hist.c
> >> +++ b/kernel/trace/trace_events_hist.c
> >> @@ -822,7 +822,7 @@ static void action_trace(struct hist_trigger_data *hist_data,
> >>   {
> >>   	struct synth_event *event = data->synth_event;
> >>   
> >> -	trace_synth(event, var_ref_vals, data->var_ref_idx);
> >> +	synth_event_trace2(event, var_ref_vals, data->var_ref_idx);
> >>   }
> >>   
> >>   struct hist_var_data {
> >> diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
> >> index 4a9a44d37ffc..8837aa258479 100644
> >> --- a/kernel/trace/trace_events_synth.c
> >> +++ b/kernel/trace/trace_events_synth.c
> >> @@ -850,7 +850,7 @@ EXPORT_SYMBOL_GPL(synth_event_find);
> >>   typedef void (*synth_probe_func_t) (void *__data, u64 *var_ref_vals,
> >>   				    unsigned int *var_ref_idx);
> >>   
> >> -void trace_synth(struct synth_event *event, u64 *var_ref_vals,
> >> +void synth_event_trace2(struct synth_event *event, u64 *var_ref_vals,
> >>   			       unsigned int *var_ref_idx)
> >>   {
> >>   	struct tracepoint *tp = event->tp;
> >> @@ -873,7 +873,7 @@ void trace_synth(struct synth_event *event, u64 *var_ref_vals,
> >>   		}
> >>   	}
> >>   }
> >> -EXPORT_SYMBOL_GPL(trace_synth);
> >> +EXPORT_SYMBOL_GPL(synth_event_trace2);
> >>   
> >>   static struct trace_event_fields synth_event_fields_array[] = {
> >>   	{ .type = TRACE_FUNCTION_TYPE,
> >> -- 
> >> 2.43.0
> >>
> > 
> > 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists