Re: [PATCH 0/7] tracing: Add support for in-kernel synthetic event API

From: Tom Zanussi
Date: Fri Dec 20 2019 - 11:24:23 EST


Hi Masami,

On Fri, 2019-12-20 at 17:41 +0900, Masami Hiramatsu wrote:
> Hi Tom,
>
> On Thu, 19 Dec 2019 10:24:27 -0600
> Tom Zanussi <zanussi@xxxxxxxxxx> wrote:
>
> > Hi Masami,
> >
> > On Thu, 2019-12-19 at 23:45 +0900, Masami Hiramatsu wrote:
> > > Hello Tom,
> > >
> > > On Wed, 18 Dec 2019 09:27:36 -0600
> > > Tom Zanussi <zanussi@xxxxxxxxxx> wrote:
> > >
> > > > Hi,
> > > >
> > > > I've recently had several requests and suggestions from users
> > > > to
> > > > add
> > > > support for the creation and generation of synthetic events
> > > > from
> > > > kernel code such as modules, and not just from the available
> > > > command
> > > > line commands.
> > >
> > > This reminds me my recent series of patches.
> > >
> > > Could you use synth_event_run_command() for this purpose as I did
> > > in boot-time tracing series?
> > >
> > > https://lkml.kernel.org/r/157528179955.22451.16317363776831311868
> > > .stg
> > > it@devnote2
> > >
> > > Above example uses a command string as same as command string,
> > > but I
> > > think
> > > we can introduce some macros to construct the command string for
> > > easier
> > > definition.
> > >
> > > Or maybe it is possible to pass the const char *args[] array to
> > > that
> > > API,
> > > instead of single char *cmd. (for dynamic event definiton)
> > >
> > > Maybe we should consider more generic APIs for modules to
> > > create/delete
> > > dynamic-event including synthetic and probes, and those might
> > > reuse
> > > existing command parsers.
> > >
> >
> > I'll have to look at your patches more closely, but I think it
> > should
> > be possible to generate the command string
> > synth_event_run_command()
> > needs, or the equivalent const char *args[] array you mentioned,
> > from
> > the higher-level event definition in my patches.
>
> Agreed.
>
> >
> > So the two ways of defining an event in my patches is 1) from a
> > static
> > array known at build-time defined like this:
> >
> > static struct synth_field_desc synthtest_fields[] = {
> > { .type = "pid_t", .name = "next_pid_field" },
> > { .type = "char[16]", .name = "next_comm_field" },
> > { .type = "u64", .name = "ts_ns" },
> > { .type = "u64", .name = "ts_ms" },
> > { .type = "unsigned int", .name = "cpu" },
> > { .type = "char[64]", .name = "my_string_field" },
> > { .type = "int", .name = "my_int_field" },
> > };
> >
> > which is then passed to create_synth_event(&synthtest_fields)
> >
> > or 2) at run-time by adding fields individually as they become
> > known:
> >
> > add_synth_field("type", "name")
> >
> > which requires some sort of start/end("event name").
>
> I think the 1) API seems a bit redundant IF we can expose the
> single comamnd string interface.
>

It may be redundant, but I think it might be a preferred interface for
some users. In any case, supporting 1) would just a simple matter of
providing a wrapper interface around your string interface.

> > I think that should all be possible using your patches, and maybe
> > generalizable to not just synth events by removing _synth_ from
> > everything?
>
> If the implementation is enough generic, I think it is better to keep
> "synth" for better usability.
>
> For example, if the API is just generating a command string,
> it would be easy to be reused by probe-events too.
>
> ----
> struct dynevent_command {
> char *cmd_buf;
> enum dynevent_type type; /* Set by gen_*_cmd and checked on each
> API */
> };
>
> int gen_synth_cmd(struct dynevent_command *, const char *name, ...);
> int add_synth_field(struct dynevent_command *, const char *type,
> const char *var);
> int gen_kprobe_cmd(struct dynevent_command *, const char *name, const
> char *loc, ....);
> int gen_kretprobe_cmd(struct dynevent_command *, const char *name,
> const char *loc, ....);
> int add_probe_fields(struct dynevent_command *, const char *field,
> ...);
> int create_dynevent(struct dynevent_command *);
>
> struct dynevent_command cmd;
>
> gen_synth_cmd(&cmd, "synthtest", "pid_t", "next_pid_field");
> add_synth_field(&cmd, "char[16]", "next_comm_field");
> ...
> create_dynevent(&cmd);
>
> gen_kprobe_cmd(&cmd, "myprobe", "vfs_read+5", "%ax");
> add_probe_fields(&cmd, "%bx", "%dx");
> create_dynevent(&cmd);
>
> gen_kretprobe_cmd(&cmd, "myretprobe", "vfs_write", "$retval");
> create_dynevent(&cmd);
> ----
>
> Actually, these are just wrappers of type verifier & strcat() :P
> And it can provide similar user-experience and generic interface
> with simplar implementation.
>

Good suggestions - I'll start implementing something like this and
rebase my patches on top of this.

> > Let me know what you think. If that's correct, I can go
> > and rewrite the event creation part on top of your patches.
>
> No need to move onto my series. Mine focuses on tracing
> boot process, but your's are providing APIs for modules.
>

OK, yeah I guess synth_event_run_command() et al are very simple.

Thanks,

Tom