Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1637559 > unrolled thread

[PATCH] perf, tools, script: Allow adding and removing fields

Back to article view | Back to linux.kernel

Contents

  [PATCH] perf, tools, script: Allow adding and removing fields Andi Kleen <andi@firstfloor.org> - 2017-05-08 18:30 +0200
    Re: [PATCH] perf, tools, script: Allow adding and removing fields Jiri Olsa <jolsa@redhat.com> - 2017-05-09 14:30 +0200
      Re: [PATCH] perf, tools, script: Allow adding and removing fields Andi Kleen <ak@linux.intel.com> - 2017-05-09 14:40 +0200
        Re: [PATCH] perf, tools, script: Allow adding and removing fields Jiri Olsa <jolsa@redhat.com> - 2017-05-09 15:50 +0200
          Re: [PATCH] perf, tools, script: Allow adding and removing fields Andi Kleen <andi@firstfloor.org> - 2017-05-09 16:40 +0200

#1637559 — [PATCH] perf, tools, script: Allow adding and removing fields

From: Andi Kleen <ak@linux.intel.com>

With perf script it is common that we just want to add or remove a field.
Currently this requires figuring out the long list of default fields and
specifying them first, and then adding/removing the new field.

This patch adds a new + - syntax to merely add or remove fields,
that allows more succint and clearer command lines

For example to remove the comm field from PMU samples:

Previously

perf script -F tid,cpu,time,event,sym,ip,dso,period
	    0 [000] 504345.383126:          1 cycles:  ffffffff90060c66 native_write_msr ([kernel.kallsyms])

with the new syntax

perf script -F -comm
	    0 [000] 504345.383126:          1 cycles:  ffffffff90060c66 native_write_msr ([kernel.kallsyms])

The new syntax cannot be mixed with normal overriding.

v2: Fix example in description. Use tid vs pid. No functional
changes.
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/Documentation/perf-script.txt |  8 ++++++
 tools/perf/builtin-script.c              | 42 +++++++++++++++++++++++++++++---
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/tools/perf/Documentation/perf-script.txt b/tools/perf/Documentation/perf-script.txt
index cb0eda3925e6..4547120c6ad3 100644
--- a/tools/perf/Documentation/perf-script.txt
+++ b/tools/perf/Documentation/perf-script.txt
@@ -130,6 +130,14 @@ OPTIONS
 	i.e., the specified fields apply to all event types if the type string
 	is not given.
 
+	In addition to overriding fields, it is also possible to add or remove
+	fields from the defaults. For example
+
+		-F -cpu,+insn
+
+	removes the cpu field and adds the insn field. Adding/removing fields
+	cannot be mixed with normal overriding.
+
 	The arguments are processed in the order received. A later usage can
 	reset a prior request. e.g.:
 
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index d05aec491cff..f2970b118cd2 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1727,6 +1727,7 @@ static int parse_output_fields(const struct option *opt __maybe_unused,
 	int rc = 0;
 	char *str = strdup(arg);
 	int type = -1;
+	enum { DEFAULT, SET, ADD, REMOVE } change = DEFAULT;
 
 	if (!str)
 		return -ENOMEM;
@@ -1755,6 +1756,10 @@ static int parse_output_fields(const struct option *opt __maybe_unused,
 			goto out;
 		}
 
+		/* Don't override defaults for +- */
+		if (strchr(str, '+') || strchr(str, '-'))
+			goto parse;
+
 		if (output[type].user_set)
 			pr_warning("Overriding previous field request for %s events.\n",
 				   event_type(type));
@@ -1772,6 +1777,10 @@ static int parse_output_fields(const struct option *opt __maybe_unused,
 			goto out;
 		}
 
+		/* Don't override defaults for +- */
+		if (strchr(str, '+') || strchr(str, '-'))
+			goto parse;
+
 		if (output_set_by_user())
 			pr_warning("Overriding previous field request for all events.\n");
 
@@ -1782,13 +1791,30 @@ static int parse_output_fields(const struct option *opt __maybe_unused,
 		}
 	}
 
+parse:
 	for (tok = strtok_r(tok, ",", &strtok_saveptr); tok; tok = strtok_r(NULL, ",", &strtok_saveptr)) {
+		if (*tok == '+') {
+			if (change == SET)
+				goto out_badmix;
+			change = ADD;
+			tok++;
+		} else if (*tok == '-') {
+			if (change == SET)
+				goto out_badmix;
+			change = REMOVE;
+			tok++;
+		} else {
+			if (change != SET && change != DEFAULT)
+				goto out_badmix;
+			change = SET;
+		}
+
 		for (i = 0; i < imax; ++i) {
 			if (strcmp(tok, all_output_options[i].str) == 0)
 				break;
 		}
 		if (i == imax && strcmp(tok, "flags") == 0) {
-			print_flags = true;
+			print_flags = change == REMOVE ? false : true;
 			continue;
 		}
 		if (i == imax) {
@@ -1805,8 +1831,12 @@ static int parse_output_fields(const struct option *opt __maybe_unused,
 				if (output[j].invalid_fields & all_output_options[i].field) {
 					pr_warning("\'%s\' not valid for %s events. Ignoring.\n",
 						   all_output_options[i].str, event_type(j));
-				} else
-					output[j].fields |= all_output_options[i].field;
+				} else {
+					if (change == REMOVE)
+						output[j].fields &= ~all_output_options[i].field;
+					else
+						output[j].fields |= all_output_options[i].field;
+				}
 			}
 		} else {
 			if (output[type].invalid_fields & all_output_options[i].field) {
@@ -1826,10 +1856,15 @@ static int parse_output_fields(const struct option *opt __maybe_unused,
 				 "Events will not be displayed.\n", event_type(type));
 		}
 	}
+	goto out;
 
+out_badmix:
+	fprintf(stderr, "Cannot mix +-field with overridden fields\n");
+	rc = -EINVAL;
 out:
 	free(str);
 	return rc;
+
 }
 
 /* Helper function for filesystems that return a dent->d_type DT_UNKNOWN */
@@ -2444,6 +2479,7 @@ int cmd_script(int argc, const char **argv)
 		     symbol__config_symfs),
 	OPT_CALLBACK('F', "fields", NULL, "str",
 		     "comma separated output fields prepend with 'type:'. "
+		     "+field to add and -field to remove."
 		     "Valid types: hw,sw,trace,raw. "
 		     "Fields: comm,tid,pid,time,cpu,event,trace,ip,sym,dso,"
 		     "addr,symoff,period,iregs,brstack,brstacksym,flags,"
-- 
2.9.3

[toc] | [next] | [standalone]

#1638086

On Mon, May 08, 2017 at 09:21:34AM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> With perf script it is common that we just want to add or remove a field.
> Currently this requires figuring out the long list of default fields and
> specifying them first, and then adding/removing the new field.
> 
> This patch adds a new + - syntax to merely add or remove fields,
> that allows more succint and clearer command lines
> 
> For example to remove the comm field from PMU samples:
> 
> Previously
> 
> perf script -F tid,cpu,time,event,sym,ip,dso,period
> 	    0 [000] 504345.383126:          1 cycles:  ffffffff90060c66 native_write_msr ([kernel.kallsyms])
> 
> with the new syntax
> 
> perf script -F -comm
> 	    0 [000] 504345.383126:          1 cycles:  ffffffff90060c66 native_write_msr ([kernel.kallsyms])
> 
> The new syntax cannot be mixed with normal overriding.

so the -F option for 'type:' does not have default set,
and it looks like the +- don't make sense there:

[jolsa@krava perf]$ ./perf script -F hw:+comm | head -3
              ex 
              ex 
              ex 


jirka

[toc] | [prev] | [next] | [standalone]

#1638094

> so the -F option for 'type:' does not have default set,
> and it looks like the +- don't make sense there:

hw:+comm is just equivalent to hw:comm

Seems fine to me?

hw:-comm won't do anything, but I guess that's expected.

Don't see any need to change this?

> 
> [jolsa@krava perf]$ ./perf script -F hw:+comm | head -3
>               ex 
>               ex 
>               ex 


-Andi

[toc] | [prev] | [next] | [standalone]

#1638127

On Tue, May 09, 2017 at 05:29:24AM -0700, Andi Kleen wrote:
> > so the -F option for 'type:' does not have default set,
> > and it looks like the +- don't make sense there:
> 
> hw:+comm is just equivalent to hw:comm
> 
> Seems fine to me?
> 
> hw:-comm won't do anything, but I guess that's expected.
> 
> Don't see any need to change this?

why did you add it in the first place then?

jirka

[toc] | [prev] | [next] | [standalone]

#1638165

On Tue, May 09, 2017 at 03:45:03PM +0200, Jiri Olsa wrote:
> On Tue, May 09, 2017 at 05:29:24AM -0700, Andi Kleen wrote:
> > > so the -F option for 'type:' does not have default set,
> > > and it looks like the +- don't make sense there:
> > 
> > hw:+comm is just equivalent to hw:comm
> > 
> > Seems fine to me?
> > 
> > hw:-comm won't do anything, but I guess that's expected.
> > 
> > Don't see any need to change this?
> 
> why did you add it in the first place then?

You mean the Don't override defaults check in the hw types specified case?

I guess it could be removed yes.

-Andi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web