From: Namhyung Kim <namhyung@kernel.org>
To: Tom Zanussi <tom.zanussi@linux.intel.com>
Cc: rostedt@goodmis.org, tglx@linutronix.de, mhiramat@kernel.org,
vedang.patel@intel.com, bigeasy@linutronix.de,
joel.opensrc@gmail.com, joelaf@google.com,
mathieu.desnoyers@efficios.com, baohong.liu@intel.com,
rajvi.jingar@intel.com, julia@ni.com, fengguang.wu@intel.com,
linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org,
kernel-team@lge.com
Subject: Re: [PATCH v6 24/37] tracing: Add support for 'synthetic' events
Date: Tue, 21 Nov 2017 20:32:11 +0900 [thread overview]
Message-ID: <20171121113211.GB17299@sejong> (raw)
In-Reply-To: <2bbfcf24f73a41b9c7140d49cfc5e3874ac96432.1510948725.git.tom.zanussi@linux.intel.com>
On Fri, Nov 17, 2017 at 02:33:03PM -0600, Tom Zanussi wrote:
> Synthetic events are user-defined events generated from hist trigger
> variables saved from one or more other events.
>
> To define a synthetic event, the user writes a simple specification
> consisting of the name of the new event along with one or more
> variables and their type(s), to the tracing/synthetic_events file.
>
> For instance, the following creates a new event named 'wakeup_latency'
> with 3 fields: lat, pid, and prio:
>
> # echo 'wakeup_latency u64 lat; pid_t pid; int prio' >> \
> /sys/kernel/debug/tracing/synthetic_events
>
> Reading the tracing/synthetic_events file lists all the
> currently-defined synthetic events, in this case the event we defined
> above:
>
> # cat /sys/kernel/debug/tracing/synthetic_events
> wakeup_latency u64 lat; pid_t pid; int prio
>
> At this point, the synthetic event is ready to use, and a histogram
> can be defined using it:
>
> # echo 'hist:keys=pid,prio,lat.log2:sort=pid,lat' >> \
> /sys/kernel/debug/tracing/events/synthetic/wakeup_latency/trigger
>
> The new event is created under the tracing/events/synthetic/ directory
> and looks and behaves just like any other event:
>
> # ls /sys/kernel/debug/tracing/events/synthetic/wakeup_latency
> enable filter format hist id trigger
>
> Although a histogram can be defined for it, nothing will happen until
> an action tracing that event via the trace_synth() function occurs.
> The trace_synth() function is very similar to all the other trace_*
> invocations spread throughout the kernel, except in this case the
> trace_ function and its corresponding tracepoint isn't statically
> generated but defined by the user at run-time.
>
> How this can be automatically hooked up via a hist trigger 'action' is
> discussed in a subsequent patch.
>
> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> [fix noderef.cocci warnings, sizeof pointer for kcalloc of event->fields]
> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> ---
[SNIP]
> +static int synth_field_size(char *type)
> +{
> + int size = 0;
> +
> + if (strcmp(type, "s64") == 0)
> + size = sizeof(s64);
> + else if (strcmp(type, "u64") == 0)
> + size = sizeof(u64);
> + else if (strcmp(type, "s32") == 0)
> + size = sizeof(s32);
> + else if (strcmp(type, "u32") == 0)
> + size = sizeof(u32);
> + else if (strcmp(type, "s16") == 0)
> + size = sizeof(s16);
> + else if (strcmp(type, "u16") == 0)
> + size = sizeof(u16);
> + else if (strcmp(type, "s8") == 0)
> + size = sizeof(s8);
> + else if (strcmp(type, "u8") == 0)
> + size = sizeof(u8);
> + else if (strcmp(type, "char") == 0)
> + size = sizeof(char);
> + else if (strcmp(type, "unsigned char") == 0)
> + size = sizeof(unsigned char);
> + else if (strcmp(type, "int") == 0)
> + size = sizeof(int);
> + else if (strcmp(type, "unsigned int") == 0)
> + size = sizeof(unsigned int);
> + else if (strcmp(type, "long") == 0)
> + size = sizeof(long);
> + else if (strcmp(type, "unsigned long") == 0)
> + size = sizeof(unsigned long);
> + else if (strcmp(type, "pid_t") == 0)
> + size = sizeof(pid_t);
> + else if (synth_field_is_string(type))
> + size = synth_field_string_size(type);
> +
> + return size;
> +}
> +
> +static const char *synth_field_fmt(char *type)
> +{
> + const char *fmt = "%llu";
> +
> + if (strcmp(type, "s64") == 0)
> + fmt = "%lld";
> + else if (strcmp(type, "u64") == 0)
> + fmt = "%llu";
> + else if (strcmp(type, "s32") == 0)
> + fmt = "%d";
> + else if (strcmp(type, "u32") == 0)
> + fmt = "%u";
> + else if (strcmp(type, "s16") == 0)
> + fmt = "%d";
> + else if (strcmp(type, "u16") == 0)
> + fmt = "%u";
> + else if (strcmp(type, "s8") == 0)
> + fmt = "%d";
> + else if (strcmp(type, "u8") == 0)
> + fmt = "%u";
> + else if (strcmp(type, "char") == 0)
> + fmt = "%d";
> + else if (strcmp(type, "unsigned char") == 0)
> + fmt = "%u";
> + else if (strcmp(type, "int") == 0)
> + fmt = "%d";
> + else if (strcmp(type, "unsigned int") == 0)
> + fmt = "%u";
> + else if (strcmp(type, "long") == 0)
> + fmt = "%ld";
> + else if (strcmp(type, "unsigned long") == 0)
> + fmt = "%lu";
> + else if (strcmp(type, "pid_t") == 0)
> + fmt = "%d";
> + else if (strchr(type, '[') != NULL)
Why not using the same function here?
else if (synth_field_is_string(type))
> + fmt = "%s";
> +
> + return fmt;
> +}
> +
[SNIP]
> +static notrace void trace_event_raw_event_synth(void *__data,
> + u64 *var_ref_vals,
> + unsigned int var_ref_idx)
> +{
> + struct trace_event_file *trace_file = __data;
> + struct synth_trace_event *entry;
> + struct trace_event_buffer fbuffer;
> + struct synth_event *event;
> + unsigned int i, n_u64;
> + int fields_size = 0;
> +
> + event = trace_file->event_call->data;
> +
> + if (trace_trigger_soft_disabled(trace_file))
> + return;
> +
> + fields_size = event->n_u64 * sizeof(u64);
> +
> + entry = trace_event_buffer_reserve(&fbuffer, trace_file,
> + sizeof(*entry) + fields_size);
> + if (!entry)
> + return;
> +
> + for (i = 0, n_u64 = 0; i < event->n_fields; i++) {
> + if (event->fields[i]->is_string) {
> + char *str_val = (char *)(long)var_ref_vals[var_ref_idx + i];
> + char *str_field = (char *)&entry->fields[n_u64];
> +
> + strncpy(str_field, str_val, STR_VAR_LEN_MAX);
> + n_u64 += STR_VAR_LEN_MAX / sizeof(u64);
> + } else {
> + entry->fields[i] = var_ref_vals[var_ref_idx + i];
I think it should be 'entry->fields[n_u64]' like for string.
> + n_u64++;
> + }
> + }
> +
> + trace_event_buffer_commit(&fbuffer);
> +}
> +
[SNIP]
> +static int create_synth_event(int argc, char **argv)
> +{
> + struct synth_field *field, *fields[SYNTH_FIELDS_MAX];
> + struct synth_event *event = NULL;
> + bool delete_event = false;
> + int i, n_fields = 0, ret = 0;
> + char *name;
> +
> + mutex_lock(&synth_event_mutex);
> +
> + /*
> + * Argument syntax:
> + * - Add synthetic event: <event_name> field[;field] ...
> + * - Remove synthetic event: !<event_name> field[;field] ...
> + * where 'field' = type field_name
> + */
> + if (argc < 1) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + name = argv[0];
> + if (name[0] == '!') {
> + delete_event = true;
> + name++;
> + }
> +
> + event = find_synth_event(name);
> + if (event) {
> + if (delete_event) {
> + if (event->ref) {
> + event = NULL;
> + ret = -EBUSY;
> + goto out;
> + }
> + list_del(&event->list);
> + goto out;
> + }
> + event = NULL;
> + ret = -EEXIST;
> + goto out;
> + } else if (delete_event)
> + goto out;
> +
> + if (argc < 2) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + for (i = 1; i < argc - 1; i++) {
> + if (strcmp(argv[i], ";") == 0)
> + continue;
> + if (n_fields == SYNTH_FIELDS_MAX) {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + field = parse_synth_field(argv[i], argv[i + 1]);
> + if (IS_ERR(field)) {
> + ret = PTR_ERR(field);
> + goto err;
> + }
> + fields[n_fields] = field;
> + i++; n_fields++;
> + }
> +
> + if (i < argc) {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + event = alloc_synth_event(name, n_fields, fields);
> + if (IS_ERR(event)) {
> + ret = PTR_ERR(event);
> + event = NULL;
> + goto err;
> + }
> + out:
> + mutex_unlock(&synth_event_mutex);
> +
> + if (event) {
> + if (delete_event) {
> + ret = unregister_synth_event(event);
> + if (!ret)
> + free_synth_event(event);
> + else {
> + mutex_lock(&synth_event_mutex);
> + if (!find_synth_event(event->name))
> + list_add(&event->list, &synth_event_list);
> + mutex_unlock(&synth_event_mutex);
> + }
> + } else {
> + ret = register_synth_event(event);
> + if (!ret) {
> + mutex_lock(&synth_event_mutex);
> + if (!find_synth_event(event->name))
> + list_add(&event->list, &synth_event_list);
> + mutex_unlock(&synth_event_mutex);
> + } else
> + free_synth_event(event);
Hmm.. this code is repeated 3 times (see below). How about making a
separate function like free_or_add_back_event()?
> + }
> + }
> +
> + return ret;
> + err:
> + mutex_unlock(&synth_event_mutex);
> +
> + for (i = 0; i < n_fields; i++)
> + free_synth_field(fields[i]);
> + free_synth_event(event);
> +
> + return ret;
> +}
> +
> +static int release_all_synth_events(void)
> +{
> + struct list_head release_events;
> + struct synth_event *event, *e;
> + int ret = 0, err = 0;
> +
> + INIT_LIST_HEAD(&release_events);
> +
> + mutex_lock(&synth_event_mutex);
> +
> + list_for_each_entry(event, &synth_event_list, list) {
> + if (event->ref) {
> + mutex_unlock(&synth_event_mutex);
> + return -EBUSY;
> + }
> + }
> +
> + list_for_each_entry_safe(event, e, &synth_event_list, list) {
> + list_del(&event->list);
> + list_add(&event->list, &release_events);
list_move() ?
> + }
> +
> + mutex_unlock(&synth_event_mutex);
> +
> + list_for_each_entry_safe(event, e, &release_events, list) {
> + list_del(&event->list);
> +
> + ret = unregister_synth_event(event);
> + if (ret == 0)
> + free_synth_event(event);
> + else {
> + err = ret;
> + mutex_lock(&synth_event_mutex);
> + if (!find_synth_event(event->name))
> + list_add(&event->list, &synth_event_list);
> + mutex_unlock(&synth_event_mutex);
> + }
Also repeated here..
Thanks,
Namhyung
> + }
> +
> + if (err)
> + ret = err;
> +
> + return ret;
> +}
next prev parent reply other threads:[~2017-11-21 11:32 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-17 20:32 [PATCH v6 00/37] tracing: Inter-event (e.g. latency) support Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 01/37] tracing: Move hist trigger Documentation to histogram.txt Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 02/37] tracing: Add Documentation for log2 modifier Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 03/37] tracing: Add support to detect and avoid duplicates Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 04/37] tracing: Remove code which merges duplicates Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 05/37] ring-buffer: Add interface for setting absolute time stamps Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 06/37] ring-buffer: Redefine the unimplemented RINGBUF_TYPE_TIME_STAMP Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 07/37] tracing: Add timestamp_mode trace file Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 08/37] tracing: Give event triggers access to ring_buffer_event Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 09/37] tracing: Add ring buffer event param to hist field functions Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 10/37] tracing: Break out hist trigger assignment parsing Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 11/37] tracing: Add hist trigger timestamp support Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 12/37] tracing: Add per-element variable support to tracing_map Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 13/37] tracing: Add hist_data member to hist_field Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 14/37] tracing: Add usecs modifier for hist trigger timestamps Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 15/37] tracing: Add variable support to hist triggers Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 16/37] tracing: Account for variables in named trigger compatibility Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 17/37] tracing: Move get_hist_field_flags() Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 18/37] tracing: Add simple expression support to hist triggers Tom Zanussi
2017-11-20 6:05 ` Namhyung Kim
2017-11-17 20:32 ` [PATCH v6 19/37] tracing: Generalize per-element hist trigger data Tom Zanussi
2017-11-17 20:32 ` [PATCH v6 20/37] tracing: Pass tracing_map_elt to hist_field accessor functions Tom Zanussi
2017-11-17 20:33 ` [PATCH v6 21/37] tracing: Add hist_field 'type' field Tom Zanussi
2017-11-17 20:33 ` [PATCH v6 22/37] tracing: Add variable reference handling to hist triggers Tom Zanussi
2017-11-21 10:53 ` Namhyung Kim
2017-11-17 20:33 ` [PATCH v6 23/37] tracing: Add hist trigger action hook Tom Zanussi
2017-11-17 20:33 ` [PATCH v6 24/37] tracing: Add support for 'synthetic' events Tom Zanussi
2017-11-21 11:32 ` Namhyung Kim [this message]
2017-11-17 20:33 ` [PATCH v6 25/37] tracing: Add support for 'field variables' Tom Zanussi
2017-11-21 12:08 ` Namhyung Kim
2017-11-22 6:05 ` Namhyung Kim
2017-11-17 20:33 ` [PATCH v6 26/37] tracing: Add 'onmatch' hist trigger action support Tom Zanussi
2017-11-22 9:49 ` Namhyung Kim
2017-11-17 20:33 ` [PATCH v6 27/37] tracing: Add 'onmax' " Tom Zanussi
2017-11-23 5:09 ` Namhyung Kim
2017-11-29 20:53 ` Tom Zanussi
2017-11-17 20:33 ` [PATCH v6 28/37] tracing: Allow whitespace to surround hist trigger filter Tom Zanussi
2017-11-17 20:33 ` [PATCH v6 29/37] tracing: Add cpu field for hist triggers Tom Zanussi
2017-11-23 5:25 ` Namhyung Kim
2017-11-29 21:02 ` Tom Zanussi
2017-11-17 20:33 ` [PATCH v6 30/37] tracing: Add hist trigger support for variable reference aliases Tom Zanussi
2017-11-17 20:33 ` [PATCH v6 31/37] tracing: Add 'last error' error facility for hist triggers Tom Zanussi
2017-11-23 6:22 ` Namhyung Kim
2017-11-17 20:33 ` [PATCH v6 32/37] tracing: Add inter-event hist trigger Documentation Tom Zanussi
2017-11-17 20:33 ` [PATCH v6 33/37] tracing: Make tracing_set_clock() non-static Tom Zanussi
2017-11-17 20:33 ` [PATCH v6 34/37] tracing: Add a clock attribute for hist triggers Tom Zanussi
2017-11-17 20:33 ` [PATCH v6 35/37] tracing: Increase trace_recursive_lock() limit for synthetic events Tom Zanussi
2017-11-17 20:33 ` [PATCH v6 36/37] tracing: Add inter-event blurb to HIST_TRIGGERS config option Tom Zanussi
2017-11-23 6:57 ` Namhyung Kim
2017-11-17 20:33 ` [PATCH v6 37/37] selftests: ftrace: Add inter-event hist triggers testcases Tom Zanussi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171121113211.GB17299@sejong \
--to=namhyung@kernel.org \
--cc=baohong.liu@intel.com \
--cc=bigeasy@linutronix.de \
--cc=fengguang.wu@intel.com \
--cc=joel.opensrc@gmail.com \
--cc=joelaf@google.com \
--cc=julia@ni.com \
--cc=kernel-team@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rt-users@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mhiramat@kernel.org \
--cc=rajvi.jingar@intel.com \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=tom.zanussi@linux.intel.com \
--cc=vedang.patel@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).