Hi Tom,

On Thu, 19 Dec 2019 10:24:27 -0600
Tom Zanussi [off-list ref] wrote:

Hi Masami,

On Thu, 2019-12-19 at 23:45 +0900, Masami Hiramatsu wrote:

quoted

Hello Tom,

On Wed, 18 Dec 2019 09:27:36 -0600
Tom Zanussi [off-list ref] wrote:

quoted

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.

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.

 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.

quoted

quoted

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.

Yes, that is what I found.

Thank you,

-- 
Masami Hiramatsu [off-list ref]