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

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

[Andi Kleen]

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

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

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

[Jiri Olsa]

On Mon, May 01, 2017 at 12:47:46PM -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 pid,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])


I haven't checked deeply yet, but I'm getting different pids
with the new syntax, perhaps some mixing with tids column?

     0 [002] 42615.004240:       2172 cycles:  ffffffff86060c66 native_write_msr ([kernel.kallsyms])
     0 [002] 42615.004242:      19857 cycles:  ffffffff860abee0 irq_exit ([kernel.kallsyms])
     0 [002] 42615.004250:     412618 cycles:  ffffffff860d1429 ttwu_do_wakeup ([kernel.kallsyms])
- 1798 [001] 42615.008219:      46557 cycles:  ffffffff860dc718 update_blocked_averages ([kernel.kallsyms])
- 1798 [001] 42615.008239:      46557 cycles:      7f313dff0684 [unknown] (/usr/lib64/firefox/libxul.so)
- 1798 [001] 42615.008258:     112387 cycles:      7f313e32c71a [unknown] (/usr/lib64/firefox/libxul.so)
+ 1861 [001] 42615.008219:      46557 cycles:  ffffffff860dc718 update_blocked_averages ([kernel.kallsyms])
+ 1861 [001] 42615.008239:      46557 cycles:      7f313dff0684 [unknown] (/usr/lib64/firefox/libxul.so)
+ 1861 [001] 42615.008258:     112387 cycles:      7f313e32c71a [unknown] (/usr/lib64/firefox/libxul.so)
  1687 [002] 42615.008268:    1711528 cycles:  ffffffffc01e74c7 i915_gem_set_domain_ioctl ([i915])
- 1798 [001] 42615.008303:     281652 cycles:  ffffffff860dc169 account_entity_dequeue ([kernel.kallsyms])
+ 1861 [001] 42615.008303:     281652 cycles:  ffffffff860dc169 account_entity_dequeue ([kernel.kallsyms])
  1798 [001] 42615.008440:     442473 cycles:      7f3147c6935b g_slice_free1 (/usr/lib64/libglib-2.0.so.0.5000.3)
- 1935 [001] 42615.009462:     487718 cycles:  ffffffff860f25bc cpuacct_charge ([kernel.kallsyms])
- 1935 [002] 42615.009955:    1711528 cycles:      7fa2a2ac748f [unknown] (/usr/lib64/firefox/libmozavcodec.so)
- 1935 [001] 42615.009989:     441676 cycles:      559627e901da [unknown] (/usr/lib64/firefox/firefox)
- 1935 [000] 42615.010233:      79099 cycles:      7fa2c2a11fa9 [unknown] (/usr/lib64/firefox/libxul.so)
+24128 [001] 42615.009462:     487718 cycles:  ffffffff860f25bc cpuacct_charge ([kernel.kallsyms])
+24182 [002] 42615.009955:    1711528 cycles:      7fa2a2ac748f [unknown] (/usr/lib64/firefox/libmozavcodec.so)
+24181 [001] 42615.009989:     441676 cycles:      559627e901da [unknown] (/usr/lib64/firefox/firefox)
+24112 [000] 42615.010233:      79099 cycles:      7fa2c2a11fa9 [unknown] (/usr/lib64/firefox/libxul.so)

thanks,
jirka

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

[Andi Kleen]

On Tue, May 02, 2017 at 08:41:47AM +0200, Jiri Olsa wrote:
> On Mon, May 01, 2017 at 12:47:46PM -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 pid,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])
> 
> 
> I haven't checked deeply yet, but I'm getting different pids
> with the new syntax, perhaps some mixing with tids column?

Cannot reproduce. Do you have an exact command line?

The patch shouldn't really change any columns.

-Andi

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

[Jiri Olsa]

On Thu, May 04, 2017 at 03:26:20PM -0700, Andi Kleen wrote:
> On Tue, May 02, 2017 at 08:41:47AM +0200, Jiri Olsa wrote:
> > On Mon, May 01, 2017 at 12:47:46PM -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 pid,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])
> > 
> > 
> > I haven't checked deeply yet, but I'm getting different pids
> > with the new syntax, perhaps some mixing with tids column?
> 
> Cannot reproduce. Do you have an exact command line?
> 
> The patch shouldn't really change any columns.
>

[jolsa@krava perf]$ ./perf record -a
^C[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.527 MB perf.data (2533 samples) ]

[jolsa@krava perf]$ ./perf script -F pid,cpu,time,event,sym,ip,dso,period > 1
Failed to open /tmp/perf-1841.map, continuing without symbols
/run/user/1000/orcexec.1yoPtl was updated (is prelink enabled?). Restart the long running apps that use it!
[vdso] with build id 0b94eba680f6a3bb2e3efbde9a2551c1f5b27e34 not found, continuing without symbols
[jolsa@krava perf]$ ./perf script -F -comm > 2
Failed to open /tmp/perf-1841.map, continuing without symbols
/run/user/1000/orcexec.1yoPtl was updated (is prelink enabled?). Restart the long running apps that use it!
[vdso] with build id 0b94eba680f6a3bb2e3efbde9a2551c1f5b27e34 not found, continuing without symbols
[jolsa@krava perf]$ diff -puw  1 2 | head -20
--- 1   2017-05-05 09:56:00.088148174 +0200
+++ 2   2017-05-05 09:56:04.469166203 +0200
@@ -6,16 +6,16 @@
     0 [000] 10374.768173:       1680 cycles:  ffffffff94116904 __next_timer_interrupt ([kernel.kallsyms])
     0 [000] 10374.768174:      32136 cycles:  ffffffff94116904 __next_timer_interrupt ([kernel.kallsyms])
     0 [000] 10374.768187:     846579 cycles:  ffffffff9484f3f6 __schedule ([kernel.kallsyms])
- 1706 [000] 10374.774848:    2417537 cycles:      7f362deaaf5e [unknown] (/usr/lib64/firefox/libxul.so)
- 1706 [003] 10374.775161:      31151 cycles:  ffffffff940dc520 update_blocked_averages ([kernel.kallsyms])
- 1841 [002] 10374.775163:      41524 cycles:  ffffffff940f3ec9 queued_spin_lock_slowpath ([kernel.kallsyms])
+ 1769 [000] 10374.774848:    2417537 cycles:      7f362deaaf5e [unknown] (/usr/lib64/firefox/libxul.so)
+ 1723 [003] 10374.775161:      31151 cycles:  ffffffff940dc520 update_blocked_averages ([kernel.kallsyms])
+ 1844 [002] 10374.775163:      41524 cycles:  ffffffff940f3ec9 queued_spin_lock_slowpath ([kernel.kallsyms])
     0 [001] 10374.775168:      24203 cycles:  ffffffff940d7e24 sched_clock_idle_wakeup_event ([kernel.kallsyms])
- 1706 [003] 10374.775175:      31151 cycles:  ffffffff94704227 copy_msghdr_from_user ([kernel.kallsyms])
+ 1723 [003] 10374.775175:      31151 cycles:  ffffffff94704227 copy_msghdr_from_user ([kernel.kallsyms])
  1841 [001] 10374.775178:      24203 cycles:      7f298cdb1d1b [unknown] (/usr/lib64/firefox/libxul.so)
- 1841 [002] 10374.775181:      41524 cycles:  ffffffff940ead8e __wake_up_common ([kernel.kallsyms])
- 1706 [003] 10374.775188:      94430 cycles:  ffffffff9438a7cb selinux_file_permission ([kernel.kallsyms])
+ 1844 [002] 10374.775181:      41524 cycles:  ffffffff940ead8e __wake_up_common ([kernel.kallsyms])
+ 1723 [003] 10374.775188:      94430 cycles:  ffffffff9438a7cb selinux_file_permission ([kernel.kallsyms])
[jolsa@krava perf]$  


jirka

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

[Andi Kleen]

On Fri, May 05, 2017 at 09:57:54AM +0200, Jiri Olsa wrote:
> On Thu, May 04, 2017 at 03:26:20PM -0700, Andi Kleen wrote:
> > On Tue, May 02, 2017 at 08:41:47AM +0200, Jiri Olsa wrote:
> > > On Mon, May 01, 2017 at 12:47:46PM -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 pid,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])
> > > 
> > > 
> > > I haven't checked deeply yet, but I'm getting different pids
> > > with the new syntax, perhaps some mixing with tids column?
> > 
> > Cannot reproduce. Do you have an exact command line?
> > 
> > The patch shouldn't really change any columns.
> >
> 
> [jolsa@krava perf]$ ./perf record -a

I looked at this, and I don't think it's different with my patch. 

Any time you set fields you get different output versus default:


% perf script -F pid,cpu,time,event,sym,ip,dso,period,comm 

vs 

% perf script

First gives PID second TID.

It would be good to fix, but I don't think it should block my patch.

-Andi

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

[Jiri Olsa]

On Fri, May 05, 2017 at 12:43:40PM -0700, Andi Kleen wrote:
> On Fri, May 05, 2017 at 09:57:54AM +0200, Jiri Olsa wrote:
> > On Thu, May 04, 2017 at 03:26:20PM -0700, Andi Kleen wrote:
> > > On Tue, May 02, 2017 at 08:41:47AM +0200, Jiri Olsa wrote:
> > > > On Mon, May 01, 2017 at 12:47:46PM -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 pid,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])
> > > > 
> > > > 
> > > > I haven't checked deeply yet, but I'm getting different pids
> > > > with the new syntax, perhaps some mixing with tids column?
> > > 
> > > Cannot reproduce. Do you have an exact command line?
> > > 
> > > The patch shouldn't really change any columns.
> > >
> > 
> > [jolsa@krava perf]$ ./perf record -a
> 
> I looked at this, and I don't think it's different with my patch. 
> 
> Any time you set fields you get different output versus default:
> 
> 
> % perf script -F pid,cpu,time,event,sym,ip,dso,period,comm 
> 
> vs 
> 
> % perf script
> 
> First gives PID second TID.
> 
> It would be good to fix, but I don't think it should block my patch.

your changelog says those 2 commands give same output,
so either fix the changelog or provide related fix

thanks,
jirka

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

[Andi Kleen]

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

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

[Jiri Olsa]

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

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

[Andi Kleen]

> 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

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

[Jiri Olsa]

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

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

[Andi Kleen]

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

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

[Andi Kleen]

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.
v3: Don't skip initialization when user specified explicit type.
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/Documentation/perf-script.txt |  8 +++++++
 tools/perf/builtin-script.c              | 38 +++++++++++++++++++++++++++++---
 2 files changed, 43 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..de29a0f7beaa 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;
@@ -1772,6 +1773,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 +1787,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 +1827,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 +1852,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 +2475,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

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

[Jiri Olsa]

On Tue, May 09, 2017 at 07:36:37AM -0700, Andi Kleen wrote:

SNIP

> +				} 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 +1852,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;
> +
>  }

extra new line, but I guess Arnaldo could remove that ;-)

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

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

[Andi Kleen]

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.
v3: Don't skip initialization when user specified explicit type.
v4: Rebase. Remove empty line.
Acked-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/Documentation/perf-script.txt |  8 +++++++
 tools/perf/builtin-script.c              | 37 +++++++++++++++++++++++++++++---
 2 files changed, 42 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..85c83cc3e994 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;
@@ -1772,6 +1773,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 +1787,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 +1827,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,7 +1852,11 @@ 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;
@@ -2444,6 +2474,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.4

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

[Milian Wolff]

[-- Attachment #1: Type: text/plain, Size: 2410 bytes --]

On Friday, June 2, 2017 5:48:10 PM CEST 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.

Tested-by: Milian Wolff <milian.wolff@kdab.com>

Works a charm.

But I notice that this functionality is missing in other places too. Most 
notably, I would like to be able to configure `perf stat` in a similar way. 
Such that one could do:

perf stat -e +cache-misses

Instead of

perf stat -e <whatever the defaults are>,cache-misses

While at it, I also think it would be tremendously useful to get wildcard 
matching behavior in all perf tools. Currently, I can do:

perf list "topdown-*"

List of pre-defined events (to be used in -e):

  topdown-fetch-bubbles OR cpu/topdown-fetch-bubbles/ [Kernel PMU event]
  topdown-recovery-bubbles OR cpu/topdown-recovery-bubbles/ [Kernel PMU event]
  topdown-slots-issued OR cpu/topdown-slots-issued/  [Kernel PMU event]
  topdown-slots-retired OR cpu/topdown-slots-retired/ [Kernel PMU event]
  topdown-total-slots OR cpu/topdown-total-slots/    [Kernel PMU event]

But I cannot do:

$ perf record -e "topdown-*" ls
event syntax error: 'topdown-*'
                            \___ parser error
Run 'perf list' for a list of valid events

But of course I'm aware that these are all separate issues. But Andi, you seem 
to be working a lot on polishing perf. I'd really appreciate it if you could 
also implement the above two suggestions. Otherwise I'll try to see when I get 
the time to do it myself.

Cheers

-- 
Milian Wolff | milian.wolff@kdab.com | Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3826 bytes --]

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

[Andi Kleen]

On Thu, Jun 08, 2017 at 02:59:58PM +0200, Milian Wolff wrote:
> But I notice that this functionality is missing in other places too. Most 
> notably, I would like to be able to configure `perf stat` in a similar way. 
> Such that one could do:
> 
> perf stat -e +cache-misses
> 
> Instead of
> 
> perf stat -e <whatever the defaults are>,cache-misses

The defaults are not great, so I'm not sure that is super useful.

It's probably better to assemble reasonable groups, perhaps
with groups of metrics.

> But I cannot do:
> 
> $ perf record -e "topdown-*" ls
> event syntax error: 'topdown-*'

That's actually good because the current topdown events are not useful to sample

Usually you need to have at least some idea about the events you're collecting,
and also for non trivial collections you need groups to get good results.

I've been thinking about adding MetricGroups to the json files, that
would allow to assemble reasonable groups. But it still wouldn't be wildcard.

For a few things wildcards are useful, e.g. I implemented it recently
for PMUs so that uncore PMUs are easier to handle.

-Andi

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

[Milian Wolff]

On Freitag, 9. Juni 2017 04:52:43 CEST Andi Kleen wrote:
> On Thu, Jun 08, 2017 at 02:59:58PM +0200, Milian Wolff wrote:
> > But I notice that this functionality is missing in other places too. Most
> > notably, I would like to be able to configure `perf stat` in a similar
> > way.
> > Such that one could do:
> > 
> > perf stat -e +cache-misses
> > 
> > Instead of
> > 
> > perf stat -e <whatever the defaults are>,cache-misses
> 
> The defaults are not great, so I'm not sure that is super useful.
> 
> It's probably better to assemble reasonable groups, perhaps
> with groups of metrics.
> 
> > But I cannot do:
> > 
> > $ perf record -e "topdown-*" ls
> > event syntax error: 'topdown-*'
> 
> That's actually good because the current topdown events are not useful to
> sample

Can you elaborate? I assume it's because you actually want to sample on 
instructions, and then group it together with the topdown events and 
potentially other counters like instructions?

> Usually you need to have at least some idea about the events you're
> collecting, and also for non trivial collections you need groups to get
> good results.

Yes, sure. But replace `record` with `stat` in the above and my point still 
stands.

> I've been thinking about adding MetricGroups to the json files, that
> would allow to assemble reasonable groups. But it still wouldn't be
> wildcard.
> 
> For a few things wildcards are useful, e.g. I implemented it recently
> for PMUs so that uncore PMUs are easier to handle.

I just noticed that I can actually use wildcards for tracepoints:

perf trace --no-syscalls --event "ext4:*"

And I think the same should be doable for PMU events with perf stat, but 
currently isn't:

$ perf stat -e "topdown*" ls
invalid or unsupported event: 'topdown*'
$ perf stat -e "branch*" ls
invalid or unsupported event: 'branch*'
$ perf stat -e "cache*" ls
invalid or unsupported event: 'cache*'

Bye

-- 
Milian Wolff | milian.wolff@kdab.com | Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

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

[Andi Kleen]

On Fri, Jun 09, 2017 at 11:13:11AM +0200, Milian Wolff wrote:
> > > But I cannot do:
> > > 
> > > $ perf record -e "topdown-*" ls
> > > event syntax error: 'topdown-*'
> > 
> > That's actually good because the current topdown events are not useful to
> > sample
> 
> Can you elaborate? I assume it's because you actually want to sample on 
> instructions, and then group it together with the topdown events and 
> potentially other counters like instructions?

The topdown-* events are inputs to a formula. But you cannot directly
sample for the formula.

What you can do is to compute the formuals from counts, determine 
the bottlenecks and then sample for events which look for the
bottlebeck. For example FRONTEND_* for Frontend Bound.
These events are generally different.

toplev in pmu-tools implements this automatically, but it's a bit
too complicated for standard perf.

> 
> > Usually you need to have at least some idea about the events you're
> > collecting, and also for non trivial collections you need groups to get
> > good results.
> 
> Yes, sure. But replace `record` with `stat` in the above and my point still 
> stands

The comment was for stat.

-Andi

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

[Arnaldo Carvalho de Melo]

Em Fri, Jun 02, 2017 at 08:48:10AM -0700, Andi Kleen escreveu:
> 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])

Humm, its a nice feature, but the output used as an example doesn't show
any diff, cut'n'paste error?

Anyway, I tested it and will update that example with this:

---------------

  # perf record -a usleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 1.748 MB perf.data (14 samples) ]

Without a explicit field list specified via -F, defaults to:

  # perf script | head -2
              perf  6338 [000] 18467.058607:          1 cycles:  ffffffff89060c36 native_write_msr (/lib/modules/4.11.0-rc8+/build/vmlinux)
           swapper     0 [001] 18467.058617:          1 cycles:  ffffffff89060c36 native_write_msr (/lib/modules/4.11.0-rc8+/build/vmlinux)

Which is equivalent to:

  # perf script -F comm,tid,cpu,time,period,event,ip,sym,dso | head -2
              perf  6338 [000] 18467.058607:          1 cycles:  ffffffff89060c36 native_write_msr (/lib/modules/4.11.0-rc8+/build/vmlinux)
           swapper     0 [001] 18467.058617:          1 cycles:  ffffffff89060c36 native_write_msr (/lib/modules/4.11.0-rc8+/build/vmlinux)
  #

So if we want to remove the comm, as in your original example, we would have to 
figure out the default field list and remove ' comm' from it:

  # perf script -F tid,cpu,time,period,event,ip,sym,dso | head -2
   6338 [000] 18467.058607:          1 cycles:  ffffffff89060c36 native_write_msr (/lib/modules/4.11.0-rc8+/build/vmlinux)
      0 [001] 18467.058617:          1 cycles:  ffffffff89060c36 native_write_msr (/lib/modules/4.11.0-rc8+/build/vmlinux)
  # 

With your patch this becomes simpler, one can remove fields by prefixing them
with '-':

  # perf script -F -comm | head -2
  6338 [000] 18467.058607:          1 cycles:  ffffffff89060c36 native_write_msr (/lib/modules/4.11.0-rc8+/build/vmlinux)
     0 [001] 18467.058617:          1 cycles:  ffffffff89060c36 native_write_msr (/lib/modules/4.11.0-rc8+/build/vmlinux)
  # 

-------------------

BTW, the '+' syntax is present in 'perf report -F', but we lack handling
'-', and it works for the whole list, i.e. it is not possible to add
some fields and remove others, would be good to look at both and
consolidate at some time, using your syntax, which is more powerful.

- Arnaldo
 
> The new syntax cannot be mixed with normal overriding.
> 
> v2: Fix example in description. Use tid vs pid. No functional
> changes.
> v3: Don't skip initialization when user specified explicit type.
> v4: Rebase. Remove empty line.
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  tools/perf/Documentation/perf-script.txt |  8 +++++++
>  tools/perf/builtin-script.c              | 37 +++++++++++++++++++++++++++++---
>  2 files changed, 42 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..85c83cc3e994 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;
> @@ -1772,6 +1773,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 +1787,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 +1827,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,7 +1852,11 @@ 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;
> @@ -2444,6 +2474,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.4