tracing: Add support for in-kernel synthetic event API
Subsystems (2)
cover tracing: Add support for in-kernel synthetic event API
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 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.
Special thanks to Artem Bityutskiy, who worked with me over several iterations of the API, and who had many great suggestions on the details of the interface, and pointed out several problems with the code itself.
The following changes since commit d783b3c08c14fccbc4d5ef33a38288ec9b264df7:
tracing: Have the histogram compare functions convert to u64 first (2019-12-11 15:47:14 -0500)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/synth-event-gen-v1
Tom Zanussi (7):
tracing: Add trace_array_find() to find instance trace arrays
tracing: Add get/put_event_file()
tracing: Add delete_synth_event()
tracing: Add create_synth_event()
tracing: Add generate_synth_event() and related functions
tracing: Add synth event generation test module
tracing: Documentation for in-kernel synthetic event API
Documentation/trace/events.rst | 268 +++++++++++++ include/linux/trace_events.h | 53 +++ kernel/trace/Kconfig | 13 + kernel/trace/Makefile | 1 + kernel/trace/synth_event_gen_test.c | 330 ++++++++++++++++ kernel/trace/trace.c | 30 +- kernel/trace/trace.h | 1 + kernel/trace/trace_events.c | 130 +++++++ kernel/trace/trace_events_hist.c | 722 +++++++++++++++++++++++++++++++++++- 9 files changed, 1521 insertions(+), 27 deletions(-) create mode 100644 kernel/trace/synth_event_gen_test.c
Signature · 1 line
2.14.1
PATCH 1/7 tracing: Add trace_array_find() to find instance trace arrays
Add a new trace_array_find() function that can be used to find a trace array given the instance name, and replace existing code that does the same thing with it.
Also make it available for use outside of trace.c.
--- kernel/trace/trace.c | 30 ++++++++++++++++++++---------- kernel/trace/trace.h | 1 + 2 files changed, 21 insertions(+), 10 deletions(-)
kernel/trace/trace.c +20 -10
@@ -8455,6 +8455,21 @@ static void update_tracer_options(struct trace_array *tr)mutex_unlock (&trace_types_lock); } +/* Must have event_mutex and trace_types_lock held */ +struct trace_array *trace_array_find (const char *name) +{ +struct trace_array *tr, *found =NULL ; + +list_for_each_entry (tr, &ftrace_trace_arrays, list) { +if (tr->name &&strcmp (tr->name, name) ==0 ) { + found = tr; +break ; + } + } + +return found; +} +static struct trace_array *trace_array_create (const char *name) {struct trace_array *tr;
@@ -8531,10 +8546,8 @@ static int instance_mkdir(const char *name)mutex_lock (&trace_types_lock); ret = -EEXIST ; -list_for_each_entry (tr, &ftrace_trace_arrays, list) { -if (tr->name &&strcmp (tr->name, name) ==0 ) - goto out_unlock; - } +if (trace_array_find (name)) + goto out_unlock; tr =trace_array_create (name);
@@ -8658,12 +8671,9 @@ static int instance_rmdir(const char *name)mutex_lock (&trace_types_lock); ret = -ENODEV ; -list_for_each_entry (tr, &ftrace_trace_arrays, list) { -if (tr->name &&strcmp (tr->name, name) ==0 ) { - ret =__remove_instance (tr); -break ; - } - } + tr =trace_array_find (name); +if (tr) + ret =__remove_instance (tr);mutex_unlock (&trace_types_lock);mutex_unlock (&event_mutex);
kernel/trace/trace.h +1 -0
@@ -346,6 +346,7 @@ extern struct mutex trace_types_lock;extern int trace_array_get (struct trace_array *tr);extern int tracing_check_open_get_tr (struct trace_array *tr); +extern struct trace_array *trace_array_find (const char *name);extern int tracing_set_time_stamp_abs (struct trace_array *tr,bool abs);extern int tracing_set_clock (struct trace_array *tr,const char *clockstr);
Signature · 1 line
2.14.1
PATCH 2/7 tracing: Add get/put_event_file()
In reply to Tom ZanussiAdd a function to get an event file and prevent it from going away on module or instance removal.
get_event_file() will find an event file in a given instance (if instance is NULL, it assumes the top trace array) and return it, pinning the instance's trace array as well as the event's module, if applicable, so they won't go away while in use.
put_event_file() does the matching release.
Also included are _nolock() versions, which can be used if event_mutex is already held.
--- include/linux/trace_events.h | 10 ++++ kernel/trace/trace_events.c | 130 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+)
include/linux/trace_events.h +10 -0
@@ -348,6 +348,16 @@ enum { EVENT_FILE_FL_WAS_ENABLED_BIT, }; +extern struct trace_event_file *get_event_file (const char *instance, +const char *system, +const char *event); +extern struct trace_event_file *get_event_file_nolock (const char *instance, +const char *system, +const char *event); + +extern void put_event_file (struct trace_event_file *file); +extern void put_event_file_nolock (struct trace_event_file *file); +/* * Event file flags: * ENABLED - The event is enabled
kernel/trace/trace_events.c +130 -0
136 lines not on this page · raw message
Signature · 1 line
2.14.1
PATCH 3/7 tracing: Add delete_synth_event()
In reply to Tom Zanussicreate_or_delete_synth_event() contains code to delete a synthetic event, which would be useful on its own - specifically, it would be useful to allow event-creating modules to call it separately.
Separate out the delete code from that function and create an exported function named delete_synth_event().
--- include/linux/trace_events.h | 2 ++ kernel/trace/trace_events_hist.c | 57 +++++++++++++++++++++++++++++----------- 2 files changed, 43 insertions(+), 16 deletions(-)
include/linux/trace_events.h +2 -0
@@ -358,6 +358,8 @@ extern struct trace_event_file *get_event_file_nolock(const char *instance,extern void put_event_file (struct trace_event_file *file);extern void put_event_file_nolock (struct trace_event_file *file); +extern int delete_synth_event (const char *name); +/* * Event file flags: * ENABLED - The event is enabled
kernel/trace/trace_events_hist.c +41 -16
70 lines not on this page · raw message
Signature · 1 line
2.14.1
PATCH 4/7 tracing: Add create_synth_event()
In reply to Tom ZanussiAdd an exported function named create_synth_event(), allowing modules or other kernel code to create a synthetic event.
create_synth_event() is actually a higher-level function composed of the subfunctions create_empty_synth_event(), finalize_synth_event(), and add_synth_field(). These functions and the related add_synth_fields() are also exported so that users who want to dynamically create an event can do so.
If the event passed to delete_synth_event() is associated with a module, it also resets the trace buffer as similar functionality that removes trace events does elsewhere.
--- include/linux/trace_events.h | 21 +++ kernel/trace/trace_events_hist.c | 297 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 316 insertions(+), 2 deletions(-)
include/linux/trace_events.h +21 -0
@@ -358,8 +358,29 @@ extern struct trace_event_file *get_event_file_nolock(const char *instance,extern void put_event_file (struct trace_event_file *file);extern void put_event_file_nolock (struct trace_event_file *file); +struct synth_field_desc { +const char *type; +const char *name; +}; + +struct synth_event ; + +extern int create_synth_event (const char *name, +struct synth_field_desc *fields, +unsigned int n_fields, +struct module *mod); +extern void free_synth_event (struct synth_event *event);extern int delete_synth_event (const char *name); +extern struct synth_event *create_empty_synth_event (const char *name, +struct module *mod); +extern int add_synth_field (struct synth_event *event,const char *field_type, +const char *field_name); +extern int add_synth_fields (struct synth_event *event, +struct synth_field_desc *fields, +unsigned int n_fields); +extern int finalize_synth_event (struct synth_event *event); +/* * Event file flags: * ENABLED - The event is enabled
kernel/trace/trace_events_hist.c +295 -2
334 lines not on this page · raw message
Signature · 1 line
2.14.1
PATCH 5/7 tracing: Add generate_synth_event() and related functions
In reply to Tom ZanussiAdd an exported function named generate_synth_event(), allowing modules or other kernel code to generate synthetic events.
Also added are several functions that allow the same functionality to be broken out in a piecewise fashion, which are useful in situations where generating an event from a full array of values would be cumbersome. Those functions are generate_synth_event_start/end() and add_(next)_synth_val().
--- include/linux/trace_events.h | 20 +++ kernel/trace/trace_events_hist.c | 370 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 390 insertions(+)
include/linux/trace_events.h +20 -0
@@ -365,6 +365,18 @@ struct synth_field_desc {struct synth_event ; +struct synth_gen_state { +struct trace_event_buffer fbuffer; +struct synth_trace_event *entry; +struct ring_buffer *buffer; +struct synth_event *event; +unsigned int cur_field; +unsigned int n_u64; +bool enabled; +bool add_next; +bool add_name; +}; + extern int create_synth_event(const char *name, struct synth_field_desc *fields, unsigned int n_fields,
@@ -380,6 +392,14 @@ extern int add_synth_fields(struct synth_event *event,struct synth_field_desc *fields, unsigned int n_fields);extern int finalize_synth_event (struct synth_event *event); +extern int generate_synth_event (struct trace_event_file *file,u64 *vals, +unsigned int n_vals); +extern int generate_synth_event_start (struct trace_event_file *file, +struct synth_gen_state *gen_state); +extern int add_next_synth_val (u64 val,struct synth_gen_state *gen_state); +extern int add_synth_val (const char *field_name,u64 val, +struct synth_gen_state *gen_state); +extern int generate_synth_event_end (struct synth_gen_state *gen_state);/* * Event file flags:
kernel/trace/trace_events_hist.c +370 -0
394 lines not on this page · raw message
Signature · 1 line
2.14.1
PATCH 6/7 tracing: Add synth event generation test module
In reply to Tom ZanussiAdd a test module that checks the basic functionality of the in-kernel synthetic event generation API by creating and generating synthetic events from a module.
--- kernel/trace/Kconfig | 13 ++ kernel/trace/Makefile | 1 + kernel/trace/synth_event_gen_test.c | 330 ++++++++++++++++++++++++++++++++++++ 3 files changed, 344 insertions(+) create mode 100644 kernel/trace/synth_event_gen_test.c
kernel/trace/Kconfig +13 -0
@@ -775,6 +775,19 @@ config PREEMPTIRQ_DELAY_TEST If unsure, say N +config SYNTH_EVENT_GEN_TEST + tristate "Test module for in-kernel synthetic event generation" + depends on HIST_TRIGGERS + help + This option creates a test module to check the base + functionality of in-kernel synthetic event definition and + generation. + + To test, insert the module, and then check the trace buffer + for the generated sample events. + + If unsure, say N. + config TRACE_EVAL_MAP_FILE bool "Show eval mappings for trace events" depends on TRACING
kernel/trace/Makefile +1 -0
@@ -44,6 +44,7 @@ obj-$(CONFIG_TRACING) += trace_stat.o obj-$(CONFIG_TRACING) += trace_printk.o obj-$(CONFIG_TRACING_MAP) += tracing_map.o obj-$(CONFIG_PREEMPTIRQ_DELAY_TEST) += preemptirq_delay_test.o +obj-$(CONFIG_SYNTH_EVENT_GEN_TEST) += synth_event_gen_test.o obj-$(CONFIG_CONTEXT_SWITCH_TRACER) += trace_sched_switch.o obj-$(CONFIG_FUNCTION_TRACER) += trace_functions.o obj-$(CONFIG_PREEMPTIRQ_TRACEPOINTS) += trace_preemptirq.o
kernel/trace/synth_event_gen_test.c new file +330 -0
330 lines not on this page · raw message
Signature · 1 line
2.14.1
PATCH 7/7 tracing: Documentation for in-kernel synthetic event API
In reply to Tom ZanussiAdd Documentation for creating and generating synthetic events from modules.
--- Documentation/trace/events.rst | 268 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 268 insertions(+)
Documentation/trace/events.rst +268 -0
@@ -525,3 +525,271 @@ The following commands are supported: event counts (hitcount). See Documentation/trace/histogram.rst for details and examples. + +6.3 In-kernel trace event API +----------------------------- + +In most cases, the command-line interface to trace events is more than +sufficient. Sometimes, however, applications might find the need for +more complex relationships than can be expressed through a simple +series of linked command-line expressions, or putting together sets of +commands may be simply too cumbersome. An example might be an +application that needs to 'listen' to the trace stream in order to +maintain an in-kernel state machine detecting, for instance, when an +illegal kernel state occurs in the scheduler. + +The trace event subsystem provides an in-kernel API allowing modules +or other kernel code to generate user-defined 'synthetic' events at +will, which can be used to either augment the existing trace stream +and/or signal that a particular important state has occurred. + +The API provided for these purposes is describe below and allows the +following: + + - dynamically creating synthetic event definitions + - generating synthetic events from in-kernel code + +6.3.1 Dyamically creating synthetic event definitions +----------------------------------------------------- + +There are a couple ways to create a new synthetic event from a kernel +module or other kernel code. + +The first creates the event in one step, using create_synth_event(). +In this method, the name of the event to create and an array defining +the fields is supplied to create_synth_event(). If successful, a +synthetic event with that name and fields will exist following that +call. For example, to create a new "schedtest" synthetic event: + + ret = create_synth_event("schedtest", sched_fields, + ARRAY_SIZE(sched_fields), THIS_MODULE); + +The sched_fields param in this example points to an array of struct +synth_field_desc, each of which describes an event field by type and +name: + + static struct synth_field_desc sched_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" }, + }; + +See synth_field_size() for available types. If field_name contains [n] +the field is considered to be an array. + +If the event is created from within a module, a pointer to the module +must be passed to create_synth_event(). This will ensure that the +trace buffer won't contain unreadable events when the module is +removed. + +At this point, the event object is ready to be used for generating new +events. + +In the second method, the event is created in several steps. This +allows events to be created dynamically and without the need to create +and populate an array of fields beforehand. + +To use this method, an empty synthetic event should first be created +using create_empty_synth_event(). The name of the event should be +supplied to this function. For example, to create a new "schedtest" +synthetic event: + + struct synth_event *se = create_empty_synth_event("schedtest", THIS_MODULE); + +Once the synthetic event object has been created, it can then be +populated with fields. Fields are added one by one using +add_synth_field(), supplying the new synthetic event object, a field +type, and a field name. For example, to add a new int field named +"intfield", the following call should be made: + + ret = add_synth_field(se, "int", "intfield"); + +See synth_field_size() for available types. If field_name contains [n] +the field is considered to be an array. + +A group of fields can also be added all at once using an array of +synth_field_desc with add_synth_fields(). For example, this would add +just the first four sched_fields: + + ret = add_synth_fields(se, sched_fields, 4); + +Once all the fields have been added, the event should be finalized and +registered by calling the finalize_synth_event() function: + + ret = finalize_synth_event(se); + +At this point, the event object is ready to be used for generating new +events. + +6.3.3 Generating synthetic events from in-kernel code +----------------------------------------------------- + +To generate a synthetic event, there are a couple of options. The +first option is to generate the event in one call, using +generate_synth_event() with an array of values to be set. A second +option can be used to avoid the need for a pre-formed array of values, +using generate_synth_event_start() and generate_synth_event_end() +along with add_next_synth_val() or add_synth_val() to add the values +piecewise. + +6.3.3.1 Generating a synthetic event all at once +------------------------------------------------ + +To generate a synthetic event all at once, the generate_synth_event() +function is used. It's passed the trace_event_file representing the +synthetic event (which can be retrieved using get_event_file() using +the synthetic event name, "synthetic" as the system name, and the +trace instance name (NULL if using the global trace array)), along +with an array of u64, one for each synthetic event field. + +So, to generate an event corresponding to the synthetic event +definition above, code like the following could be used: + + u64 vals[7]; + + vals[0] = 777; /* next_pid_field */ + vals[1] = (u64)"tiddlywinks"; /* next_comm_field */ + vals[2] = 1000000; /* ts_ns */ + vals[3] = 1000; /* ts_ms */ + vals[4] = smp_processor_id(); /* cpu */ + vals[5] = (u64)"thneed"; /* my_string_field */ + vals[6] = 398; /* my_int_field */ + +The 'vals' array is just an array of u64, the number of which must +match the number of field in the synthetic event, and which must be in +the same order as the synthetic event fields. + +All vals should be cast to u64, and string vals are just pointers to +strings, cast to u64. Strings will be copied into space reserved in +the event for the string, using these pointers. + +In order to generate a synthetic event, a pointer to the trace event +file is needed. The get_event_file() function can be used to get it - +it will find the file in the given trace instance (in this case NULL +since the top trace array is being used) while at the same time +preventing the instance containing it from going away: + + schedtest_event_file = get_event_file(NULL, "synthetic", + "schedtest"); + +Before generating the event, it should be enabled in some way, +otherwise the synthetic event won't actually show up in the trace +buffer. + +To enable a synthetic event from the kernel, trace_array_set_clr_event() +can be used (which is not specific to synthetic events, so does need +the "synthetic" system name to be specified explicitly). + +To enable the event, pass 'true' to it: + + trace_array_set_clr_event(schedtest_event_file->tr, + "synthetic", "schedtest", true); + +To disable it pass false: + + trace_array_set_clr_event(schedtest_event_file->tr, + "synthetic", "schedtest", false); + +Finally, generate_synth_event() can be used to actually generate the +event, which should be visible in the trace buffer afterwards: + + ret = generate_synth_event(schedtest_event_file, vals, + ARRAY_SIZE(vals)); + +To remove the synthetic event, the event should be disabled, and the +trace instance should be 'put' back using put_event_file(): + + trace_array_set_clr_event(schedtest_event_file->tr, + "synthetic", "schedtest", false); + put_event_file(schedtest_event_file); + +If those have been successful, delete_synth_event() can be called to +remove the event: + + ret = delete_synth_event("schedtest"); + +6.3.3.1 Generating a synthetic event piecewise +---------------------------------------------- + +To generate a synthetic using the piecewise method described above, +the generate_synth_event_start() function is used to 'open' the +synthetic event generation: + + struct synth_gen_state gen_state; + + ret = generate_synth_event_start(schedtest_event_file, &gen_state); + +It's passed the trace_event_file representing the synthetic event +using the same methods as described above, along with a pointer to a +struct synth_gen_state object, which will be zeroed before use and +used to maintain state between this and following calls. + +Once the event has been opened, which means space for it has been +reserved in the trace buffer, the individual fields can be set. There +are two ways to do that, either one after another for each field in +the event, which requires no lookups, or by name, which does. The +tradeoff is flexibility in doing the assignments vs the cost of a +lookup per field. + +To assign the values one after the other without lookups, +add_next_synth_val() should be used. Each call is passed the same +synth_gen_state object used in the generate_synth_event_start(), along +with the value to set the next field in the event. After each field +is set, the 'cursor' points to the next field, which will be set by +the subsequent call, continuing until all the fields have been set in +order. The same sequence of calls as in the above examples using this +method would be (without error-handling code): + + /* next_pid_field */ + ret = add_next_synth_val(777, &gen_state); + + /* next_comm_field */ + ret = add_next_synth_val((u64)"slinky", &gen_state); + + /* ts_ns */ + ret = add_next_synth_val(1000000, &gen_state); + + /* ts_ms */ + ret = add_next_synth_val(1000, &gen_state); + + /* cpu */ + ret = add_next_synth_val(smp_processor_id(), &gen_state); + + /* my_string_field */ + ret = add_next_synth_val((u64)"thneed_2.01", &gen_state); + + /* my_int_field */ + ret = add_next_synth_val(395, &gen_state); + +To assign the values in any order, add_synth_val() should be used. +Each call is passed the same synth_gen_state object used in the +generate_synth_event_start(), along with the field name of the field +to set and the value to set it to. The same sequence of calls as in +the above examples using this method would be (without error-handling +code): + + ret = add_synth_val("next_pid_field", 777, &gen_state); + ret = add_synth_val("next_comm_field", (u64)"silly putty", &gen_state); + ret = add_synth_val("ts_ns", 1000000, &gen_state); + ret = add_synth_val("ts_ms", 1000, &gen_state); + ret = add_synth_val("cpu", smp_processor_id(), &gen_state); + ret = add_synth_val("my_string_field", (u64)"thneed_9", &gen_state); + ret = add_synth_val("my_int_field", 3999, &gen_state); + +Note that add_next_synth_val() and add_synth_val() are incompatible if +used within the same generation of an event - either one can be used +but not both at the same time. + +Finally, the event won't be actually generated until it's 'closed', +which is done using generate_synth_event_end(), which takes only the +struct synth_gen_state object used in the previous calls: + + ret = generate_synth_event_end(&gen_state); + +Note that generate_synth_event_end() must be called at the end +regardless of whether any of the add calls failed (say due to a bad +field name being passed in).
Signature · 1 line
2.14.1
Masami Hiramatsu
maintainerHello Tom,
Quoted 6 lines · On Wed, 18 Dec 2019 09:27:36 -0600 Tom Zanussi <[email protected]> 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.stgit@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.
Quoted 5 lines
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.
Thank you,
Signature · 1 line
Masami Hiramatsu <[email protected]>
Hi Masami,
Quoted 28 lines · Masami Hiramatsu
Hello Tom,
Quoted 8 lines · On Wed, 18 Dec 2019 09:27:36 -0600 Tom Zanussi <[email protected]> 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@devnote2Above 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.
Quoted 9 lines
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.
Quoted 10 lines
Quoted 9 lines
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.
Quoted 3 lines
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
Quoted 4 lines
Thank you,
Masami Hiramatsu
maintainerHi Tom,
Quoted 41 lines · On Thu, 19 Dec 2019 10:24:27 -0600 Tom Zanussi <[email protected]> wrote:
Hi Masami,
Quoted 28 lines · Masami Hiramatsu
Hello Tom,
Quoted 8 lines · On Wed, 18 Dec 2019 09:27:36 -0600 Tom Zanussi <[email protected]> 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@devnote2Above 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.Quoted 9 lines
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.
Quoted 4 lines
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.
Quoted 14 lines
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" },};
Quoted 7 lines
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.
Quoted 3 lines
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.
Quoted 2 lines
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 14 lines
Quoted 10 lines
Quoted 9 lines
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.Quoted 3 lines
Could you also check the locks are correctly acquired? It seems that your APIs doesn't lock it.
Quoted 6 lines
I did notice that I said that
trace_types_lockandevent_mutexshould be held fortrace_array_find()but it should only betrace_types_lock, and then I missed doing that inget_event_file()in one place. And I also don't really need the nolock versions, so will simplify and remove them. I think elsewhereevent_mutexis taken where needed. But if talking about something else, please let me know.
Yes, that is what I found.
Thank you,
Signature · 1 line
Masami Hiramatsu <[email protected]>
Hi Masami,
Quoted 82 lines · Masami Hiramatsu
Hi Tom,
Quoted 47 lines · On Thu, 19 Dec 2019 10:24:27 -0600 Tom Zanussi <[email protected]> wrote:
Hi Masami,
Quoted 37 lines · Masami Hiramatsu
Hello Tom,
Quoted 10 lines · On Wed, 18 Dec 2019 09:27:36 -0600 Tom Zanussi <[email protected]> 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@devnote2Above 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)Quoted 6 lines
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.Quoted 7 lines
I'll have to look at your patches more closely, but I think it should
be possible to generate the command stringsynth_event_run_command()
needs, or the equivalent const char *args[] array you mentioned, from
the higher-level event definition in my patches.Agreed.
Quoted 20 lines
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:
Quoted 7 lines
Quoted 3 lines
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.
Quoted 39 lines
Quoted 3 lines
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.
----
structdynevent_command{
char*cmd_buf;
enumdynevent_typetype; /* Set by gen_*_cmd and checked on each
API */
};int
gen_synth_cmd(struct dynevent_command *, const char *name, ...); intadd_synth_field(struct dynevent_command *, const char *type, const char *var);
intgen_kprobe_cmd(struct dynevent_command *, const char *name, const char *loc, ....);
intgen_kretprobe_cmd(struct dynevent_command *, const char *name, const char *loc, ....);
intadd_probe_fields(struct dynevent_command *, const char *field, ...);
intcreate_dynevent(struct dynevent_command *);struct
dynevent_commandcmd;
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);
Quoted 8 lines
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.
Quoted 6 lines
Quoted 2 lines
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