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

[Tom Zanussi]

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,
> 
>