Skip to contentSkip to conversation

tracing: Add support for in-kernel synthetic event API

PATCH Superseded Tom Zanussi 2 participants 12 messages Dec 18, 2019 – Dec 20, 2019

Subsystems (2)

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

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
#1 lore raw

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
index 23459d53d576..5483b50a11ff 100644
@@ -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
index 63bf60f79398..f37ef6524821 100644
@@ -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
#2 lore raw

PATCH 2/7 tracing: Add get/put_event_file()

In reply to Tom Zanussi

Add 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
index 4c6e15605766..cf982c7d6636 100644
@@ -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
#3 lore raw

PATCH 3/7 tracing: Add delete_synth_event()

In reply to Tom Zanussi

create_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
index cf982c7d6636..0c36a58cea43 100644
@@ -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
#4 lore raw

PATCH 4/7 tracing: Add create_synth_event()

In reply to Tom Zanussi

Add 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
index 0c36a58cea43..8b385778b2db 100644
@@ -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
#5 lore raw

PATCH 5/7 tracing: Add generate_synth_event() and related functions

In reply to Tom Zanussi

Add 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
index 8b385778b2db..8a413b9cff40 100644
@@ -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
#6 lore raw

PATCH 6/7 tracing: Add synth event generation test module

In reply to Tom Zanussi

Add 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
index 29a9c5058b62..0597d60158b4 100644
@@ -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
index 0e63db62225f..99de699c6062 100644
@@ -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
#7 lore raw

PATCH 7/7 tracing: Documentation for in-kernel synthetic event API

In reply to Tom Zanussi

Add 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
index f7e1fcc0953c..084d983df3a2 100644
@@ -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
maintainer
#8 lore raw
In reply to Tom Zanussi

Hello 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]>
#9 lore raw

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@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.

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,

maintainer
#10 lore raw

Hi 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@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.

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

Signature · 1 line
Masami Hiramatsu <[email protected]>
#11 lore raw

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@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)

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

----
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);

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