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

Groups > linux.kernel > #1633836 > unrolled thread

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

Started byAndi Kleen <andi@firstfloor.org>
First post2017-05-01 21:50 +0200
Last post2017-05-08 00:30 +0200
Articles 6 — 2 participants

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-01 21:50 +0200
    Re: [PATCH] perf, tools, script: Allow adding and removing fields Jiri Olsa <jolsa@redhat.com> - 2017-05-02 08:50 +0200
      Re: [PATCH] perf, tools, script: Allow adding and removing fields Andi Kleen <andi@firstfloor.org> - 2017-05-05 00:30 +0200
        Re: [PATCH] perf, tools, script: Allow adding and removing fields Jiri Olsa <jolsa@redhat.com> - 2017-05-05 10:00 +0200
          Re: [PATCH] perf, tools, script: Allow adding and removing fields Andi Kleen <andi@firstfloor.org> - 2017-05-05 21:50 +0200
            Re: [PATCH] perf, tools, script: Allow adding and removing fields Jiri Olsa <jolsa@redhat.com> - 2017-05-08 00:30 +0200

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

FromAndi Kleen <andi@firstfloor.org>
Date2017-05-01 21:50 +0200
Subject[PATCH] perf, tools, script: Allow adding and removing fields
Message-ID<tCpux-4NU-3@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1634169

FromJiri Olsa <jolsa@redhat.com>
Date2017-05-02 08:50 +0200
Message-ID<tCzNf-2NG-5@gated-at.bofh.it>
In reply to#1633836
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

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

#1636064

FromAndi Kleen <andi@firstfloor.org>
Date2017-05-05 00:30 +0200
Message-ID<tDxq1-1bL-1@gated-at.bofh.it>
In reply to#1634169
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

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

#1636219

FromJiri Olsa <jolsa@redhat.com>
Date2017-05-05 10:00 +0200
Message-ID<tDGjD-7cy-3@gated-at.bofh.it>
In reply to#1636064
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

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

#1636688

FromAndi Kleen <andi@firstfloor.org>
Date2017-05-05 21:50 +0200
Message-ID<tDRoK-69K-13@gated-at.bofh.it>
In reply to#1636219
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

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

#1637109

FromJiri Olsa <jolsa@redhat.com>
Date2017-05-08 00:30 +0200
Message-ID<tECQG-3x1-9@gated-at.bofh.it>
In reply to#1636688
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web