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
