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. 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 that should all be possible using your patches, and maybe generalizable to not just synth events by removing _synth_ from everything? Let me know what you think. If that's correct, I can go and rewrite the event creation part on top of your patches. > > This patchset adds support for that. The first three patches add > > some > > minor preliminary setup, followed by the two main patches that add > > the > > ability to create and generate synthetic events from the > > kernel. The > > next patch adds a test module that demonstrates actual use of the > > API > > and verifies that it works as intended, followed by Documentation. > > Could you also check the locks are correctly acquired? It seems that > your APIs doesn't lock it. > I did notice that I said that trace_types_lock and event_mutex should be held for trace_array_find() but it should only be trace_types_lock, and then I missed doing that in get_event_file() in one place. And I also don't really need the nolock versions, so will simplify and remove them. I think elsewhere event_mutex is taken where needed. But if talking about something else, please let me know. Thanks, Tom > > Thank you, > >
