From: Namhyung Kim <namhyung@kernel.org>
To: Tom Zanussi <tom.zanussi@linux.intel.com>
Cc: <rostedt@goodmis.org>, <tglx@linutronix.de>,
<mhiramat@kernel.org>, <linux-kernel@vger.kernel.org>,
<linux-rt-users@vger.kernel.org>
Subject: Re: [RFC][PATCH 13/21] tracing: Add simple expression support to hist triggers
Date: Tue, 14 Feb 2017 11:37:26 +0900 [thread overview]
Message-ID: <20170214023726.GM14705@sejong> (raw)
In-Reply-To: <0ec36d09fff75cfa8fe241b2e3026400cf22880f.1486569306.git.tom.zanussi@linux.intel.com>
On Wed, Feb 08, 2017 at 11:25:09AM -0600, Tom Zanussi wrote:
> Add support for simple addition, subtraction, and unary expressions
> (-(expr) and expr, where expr = b-a, a+b, a+b+c) to hist triggers, in
> order to support a minimal set of useful inter-event calculations.
>
> These operations are needed for calculating latencies between events
> (timestamp1-timestamp0) and for combined latencies (latencies over 3
> or more events).
>
> In the process, factor out some common code from key and value
> parsing.
>
> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> ---
[SNIP]
> +static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
> + struct trace_event_file *file,
> + char *str, unsigned long flags,
> + char *var_name);
> +
> +static struct hist_field *parse_unary(struct hist_trigger_data *hist_data,
> + struct trace_event_file *file,
> + char *str, unsigned long flags,
> + char *var_name)
> +{
> + struct hist_field *operand1, *expr = NULL;
> + struct ftrace_event_field *field = NULL;
> + unsigned long operand_flags;
> + char *operand1_str;
> + int ret = 0;
> + char *s;
> +
> + // we support only -(xxx) i.e. explicit parens required
> +
> + str++; // skip leading '-'
> +
> + s = strchr(str, '(');
> + if (s)
> + str++;
> + else {
> + ret = -EINVAL;
> + goto free;
> + }
> +
> + s = strchr(str, ')');
> + if (s)
> + *s = '\0';
> + else {
> + ret = -EINVAL; // no closing ')'
> + goto free;
> + }
> +
> + operand1_str = strsep(&str, "(");
> + if (!operand1_str)
> + goto free;
> +
> + flags |= HIST_FIELD_FL_EXPR;
> + expr = create_hist_field(NULL, flags, var_name);
> + if (!expr) {
> + ret = -ENOMEM;
> + goto free;
> + }
> +
> + operand_flags = 0;
> + operand1 = parse_expr(hist_data, file, str, operand_flags, NULL);
Doesn't it create an unbounded recursion?
Thanks,
Namhyung
> + if (IS_ERR(operand1)) {
> + ret = PTR_ERR(operand1);
> + goto free;
> + }
> +
> + if (operand1 == NULL) {
> + operand_flags = 0;
> + field = parse_field(hist_data, file, operand1_str,
> + &operand_flags);
> + if (IS_ERR(field)) {
> + ret = PTR_ERR(field);
> + goto free;
> + }
> + operand1 = create_hist_field(field, operand_flags, NULL);
> + if (!operand1) {
> + ret = -ENOMEM;
> + goto free;
> + }
> + }
> +
> + expr->fn = hist_field_unary_minus;
> + expr->operands[0] = operand1;
> + expr->operator = FIELD_OP_UNARY_MINUS;
> + expr->name = expr_str(expr);
> +
> + return expr;
> + free:
> + return ERR_PTR(ret);
> +}
> +
> +static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
> + struct trace_event_file *file,
> + char *str, unsigned long flags,
> + char *var_name)
> +{
> + struct hist_field *operand1, *operand2, *expr = NULL;
> + struct ftrace_event_field *field = NULL;
> + unsigned long operand_flags;
> + int field_op, ret = -EINVAL;
> + char *sep, *operand1_str;
> +
> + field_op = contains_operator(str);
> + if (field_op == FIELD_OP_NONE)
> + return NULL;
> +
> + if (field_op == FIELD_OP_UNARY_MINUS)
> + return parse_unary(hist_data, file, str, flags, var_name);
> +
> + switch (field_op) {
> + case FIELD_OP_MINUS:
> + sep = "-";
> + break;
> + case FIELD_OP_PLUS:
> + sep = "+";
> + break;
> + default:
> + goto free;
> + }
> +
> + operand1_str = strsep(&str, sep);
> + if (!operand1_str || !str)
> + goto free;
> +
> + operand_flags = 0;
> + field = parse_field(hist_data, file, operand1_str, &operand_flags);
> + if (IS_ERR(field)) {
> + ret = PTR_ERR(field);
> + goto free;
> + }
> + operand1 = create_hist_field(field, operand_flags, NULL);
> + if (!operand1) {
> + ret = -ENOMEM;
> + operand1 = NULL;
> + goto free;
> + }
> +
> + // rest of string could be another expression e.g. b+c in a+b+c
> + operand_flags = 0;
> + operand2 = parse_expr(hist_data, file, str, operand_flags, NULL);
> + if (IS_ERR(operand2)) {
> + ret = PTR_ERR(operand2);
> + operand2 = NULL;
> + goto free;
> + }
> + if (!operand2) {
> + operand_flags = 0;
> + field = parse_field(hist_data, file, str, &operand_flags);
> + if (IS_ERR(field)) {
> + ret = PTR_ERR(field);
> + goto free;
> + }
> + operand2 = create_hist_field(field, operand_flags, NULL);
> + if (!operand2) {
> + ret = -ENOMEM;
> + operand2 = NULL;
> + goto free;
> + }
> + }
> +
> + flags |= HIST_FIELD_FL_EXPR;
> + expr = create_hist_field(NULL, flags, var_name);
> + if (!expr) {
> + ret = -ENOMEM;
> + goto free;
> + }
> +
> + expr->operands[0] = operand1;
> + expr->operands[1] = operand2;
> + expr->operator = field_op;
> + expr->name = expr_str(expr);
> +
> + switch (field_op) {
> + case FIELD_OP_MINUS:
> + expr->fn = hist_field_minus;
> + break;
> + case FIELD_OP_PLUS:
> + expr->fn = hist_field_plus;
> + break;
> + default:
> + goto free;
> + }
> +
> + return expr;
> + free:
> + destroy_hist_field(operand1);
> + destroy_hist_field(operand2);
> + destroy_hist_field(expr);
> +
> + return ERR_PTR(ret);
> +}
> +
> static int create_hitcount_val(struct hist_trigger_data *hist_data)
> {
> hist_data->fields[HITCOUNT_IDX] =
> @@ -529,8 +874,9 @@ static int create_val_field(struct hist_trigger_data *hist_data,
> char *field_str, char *var_name)
> {
> struct ftrace_event_field *field = NULL;
> - char *field_name, *token;
> + struct hist_field *hist_field;
> unsigned long flags = 0;
> + char *token;
> int ret = 0;
>
> if (WARN_ON(val_idx >= TRACING_MAP_VALS_MAX))
> @@ -549,32 +895,27 @@ static int create_val_field(struct hist_trigger_data *hist_data,
> flags |= HIST_FIELD_FL_VAR;
> }
>
> - field_name = strsep(&field_str, ".");
> - if (field_str) {
> - if (strcmp(field_str, "hex") == 0)
> - flags |= HIST_FIELD_FL_HEX;
> - else {
> - ret = -EINVAL;
> + hist_field = parse_expr(hist_data, file, field_str, flags, var_name);
> + if (IS_ERR(hist_field)) {
> + ret = PTR_ERR(hist_field);
> + goto out;
> + }
> +
> + if (!hist_field) {
> + field = parse_field(hist_data, file, field_str, &flags);
> + if (IS_ERR(field)) {
> + ret = PTR_ERR(field);
> goto out;
> }
> - }
>
> - if (strcmp(field_name, "common_timestamp") == 0) {
> - flags |= HIST_FIELD_FL_TIMESTAMP;
> - hist_data->enable_timestamps = true;
> - } else {
> - field = trace_find_event_field(file->event_call, field_name);
> - if (!field) {
> - ret = -EINVAL;
> + hist_field = create_hist_field(field, flags, var_name);
> + if (!hist_field) {
> + ret = -ENOMEM;
> goto out;
> }
> }
>
> - hist_data->fields[val_idx] = create_hist_field(field, flags, var_name);
> - if (!hist_data->fields[val_idx]) {
> - ret = -ENOMEM;
> - goto out;
> - }
> + hist_data->fields[val_idx] = hist_field;
>
> ++hist_data->n_vals;
>
> @@ -623,6 +964,7 @@ static int create_key_field(struct hist_trigger_data *hist_data,
> char *field_str)
> {
> struct ftrace_event_field *field = NULL;
> + struct hist_field *hist_field;
> unsigned long flags = 0;
> unsigned int key_size;
> char *var_name;
> @@ -640,53 +982,40 @@ static int create_key_field(struct hist_trigger_data *hist_data,
> if (strcmp(field_str, "stacktrace") == 0) {
> flags |= HIST_FIELD_FL_STACKTRACE;
> key_size = sizeof(unsigned long) * HIST_STACKTRACE_DEPTH;
> + hist_field = create_hist_field(field, flags, var_name);
> } else {
> - char *field_name = strsep(&field_str, ".");
> -
> - if (field_str) {
> - if (strcmp(field_str, "hex") == 0)
> - flags |= HIST_FIELD_FL_HEX;
> - else if (strcmp(field_str, "sym") == 0)
> - flags |= HIST_FIELD_FL_SYM;
> - else if (strcmp(field_str, "sym-offset") == 0)
> - flags |= HIST_FIELD_FL_SYM_OFFSET;
> - else if ((strcmp(field_str, "execname") == 0) &&
> - (strcmp(field_name, "common_pid") == 0))
> - flags |= HIST_FIELD_FL_EXECNAME;
> - else if (strcmp(field_str, "syscall") == 0)
> - flags |= HIST_FIELD_FL_SYSCALL;
> - else if (strcmp(field_str, "log2") == 0)
> - flags |= HIST_FIELD_FL_LOG2;
> - else {
> - ret = -EINVAL;
> - goto out;
> - }
> + hist_field = parse_expr(hist_data, file, field_str, flags,
> + var_name);
> + if (IS_ERR(hist_field)) {
> + ret = PTR_ERR(hist_field);
> + goto out;
> }
>
> - if (strcmp(field_name, "common_timestamp") == 0) {
> - flags |= HIST_FIELD_FL_TIMESTAMP;
> - hist_data->enable_timestamps = true;
> - key_size = sizeof(u64);
> - } else {
> - field = trace_find_event_field(file->event_call, field_name);
> - if (!field) {
> - ret = -EINVAL;
> + if (!hist_field) {
> + field = parse_field(hist_data, file, field_str,
> + &flags);
> + if (IS_ERR(field)) {
> + ret = PTR_ERR(field);
> goto out;
> }
>
> - if (is_string_field(field))
> - key_size = MAX_FILTER_STR_VAL;
> - else
> - key_size = field->size;
> + hist_field = create_hist_field(field, flags, var_name);
> + if (!hist_field) {
> + ret = -ENOMEM;
> + goto out;
> + }
> }
> - }
>
> - hist_data->fields[key_idx] = create_hist_field(field, flags, var_name);
> - if (!hist_data->fields[key_idx]) {
> - ret = -ENOMEM;
> - goto out;
> + if (flags & HIST_FIELD_FL_TIMESTAMP)
> + key_size = sizeof(u64);
> + else if (is_string_field(field))
> + key_size = MAX_FILTER_STR_VAL;
> + else
> + key_size = field->size;
> }
>
> + hist_data->fields[key_idx] = hist_field;
> +
> key_size = ALIGN(key_size, sizeof(u64));
> hist_data->fields[key_idx]->size = key_size;
> hist_data->fields[key_idx]->offset = key_offset;
> --
> 1.9.3
>
next prev parent reply other threads:[~2017-02-14 2:37 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-08 17:24 [RFC][PATCH 00/21] tracing: Inter-event (e.g. latency) support Tom Zanussi
2017-02-08 17:24 ` [RFC][PATCH 01/21] tracing: Add hist_field_name() accessor Tom Zanussi
2017-02-08 20:09 ` Steven Rostedt
2017-02-08 17:24 ` [RFC][PATCH 02/21] tracing: Reimplement log2 Tom Zanussi
2017-02-08 20:13 ` Steven Rostedt
2017-02-08 20:25 ` Tom Zanussi
2017-02-08 17:24 ` [RFC][PATCH 03/21] ring-buffer: Add TIME_EXTEND_ABS ring buffer type Tom Zanussi
2017-02-08 20:32 ` Steven Rostedt
2017-02-08 20:55 ` Tom Zanussi
2017-02-09 14:54 ` Steven Rostedt
2017-02-10 6:04 ` Namhyung Kim
2017-02-10 14:28 ` Steven Rostedt
2017-02-08 17:25 ` [RFC][PATCH 04/21] tracing: Give event triggers access to ring_buffer_event Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 05/21] tracing: Add ring buffer event param to hist field functions Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 06/21] tracing: Increase tracing map KEYS_MAX size Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 07/21] tracing: Break out hist trigger assignment parsing Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 08/21] tracing: Make traceprobe parsing code reusable Tom Zanussi
2017-02-09 20:40 ` Steven Rostedt
2017-02-08 17:25 ` [RFC][PATCH 09/21] tracing: Add hist trigger timestamp support Tom Zanussi
2017-02-10 6:14 ` Namhyung Kim
2017-02-08 17:25 ` [RFC][PATCH 10/21] tracing: Add per-element variable support to tracing_map Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 11/21] tracing: Add variable support to hist triggers Tom Zanussi
2017-02-13 6:03 ` Namhyung Kim
2017-02-14 15:25 ` Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 12/21] tracing: Account for variables in named trigger compatibility Tom Zanussi
2017-02-13 6:04 ` Namhyung Kim
2017-02-14 15:26 ` Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 13/21] tracing: Add simple expression support to hist triggers Tom Zanussi
2017-02-14 2:37 ` Namhyung Kim [this message]
2017-02-14 15:29 ` Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 14/21] tracing: Add variable reference handling " Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 15/21] tracing: Add usecs modifier for hist trigger timestamps Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 16/21] tracing: Add support for dynamic tracepoints Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 17/21] tracing: Add hist trigger action hook Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 18/21] tracing: Add support for 'synthetic' events Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 19/21] tracing: Add 'onmatch' hist trigger action support Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 20/21] tracing: Add 'onmax' " Tom Zanussi
2017-02-08 17:25 ` [RFC][PATCH 21/21] tracing: Add inter-event hist trigger Documentation Tom Zanussi
2017-02-08 20:01 ` [RFC][PATCH 00/21] tracing: Inter-event (e.g. latency) support Steven Rostedt
2017-02-08 20:19 ` Tom Zanussi
2017-02-08 23:28 ` Tom Zanussi
2017-02-09 2:14 ` Steven Rostedt
2017-02-08 23:13 ` Masami Hiramatsu
2017-02-09 1:14 ` Tom Zanussi
2017-02-09 14:18 ` Masami Hiramatsu
2017-02-09 17:18 ` Tom Zanussi
2017-02-09 19:57 ` Steven Rostedt
2017-02-09 14:46 ` Frank Ch. Eigler
2017-02-09 18:43 ` Tom Zanussi
2017-02-10 4:16 ` Namhyung Kim
2017-02-10 9:34 ` Masami Hiramatsu
2017-02-10 18:58 ` Tom Zanussi
2017-02-13 1:04 ` Namhyung Kim
2017-02-14 9:37 ` Masami Hiramatsu
2017-02-14 15:27 ` Tom Zanussi
2017-02-10 18:43 ` 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=20170214023726.GM14705@sejong \
--to=namhyung@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rt-users@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=tom.zanussi@linux.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).