perf/core improvements and fixes

Hi Ingo,

Please consider pulling,

- Arnaldo

Test results at the end of this message, as usual.

The following changes since commit 9df9078ef2086652647248ee6e82ca8f661cb3f5:

perf/amd/uncore: Fix pr_fmt() prefix (2017-04-11 08:44:59 +0200)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tags/perf-core-for-mingo-4.12-20170411

for you to fetch changes up to 986a5bc028a84d487c354a529730b48682d1fb41:

perf annotate: Use stripped line instead of raw disassemble line (2017-04-11 16:22:23 -0300)

----------------------------------------------------------------
perf/core improvements and fixes:

User visible:

- Support s390 jump instructions in perf annotate (Christian Borntraeger)

- When failing to setup multiple events (e.g. '-e irq_vectors:*'), state which one caused the failure (Yao Jin)

- Various fixes for pipe mode, where the output of 'perf record' is written to stdout instead of to a perf.data file, fixing workloads such as: (David Carrillo-Cisneros)

    $ perf record -o - noploop | perf inject -b > perf.data

    $ perf record -o - noploop | perf annotate

Infrastructure:

- Simplify ltrim() implementation (Arnaldo Carvalho de Melo)

- Use ltrim() and rtrim() in places where ad-hoc equivalents were being used (Taeung Song)

----------------------------------------------------------------
Arnaldo Carvalho de Melo (3):
perf callchains: Switch from strtok() to strtok_r() when parsing options perf script: Use strtok_r() when parsing output field list perf string: Simplify ltrim() implementation

Christian Borntraeger (2):
perf annotate s390: Fix perf annotate error -95 (4.10 regression) perf annotate s390: Implement jump types for perf annotate

David Carrillo-Cisneros (6):
perf inject: Don't proceed if perf_session__process_event() fails perf inject: Copy events when reordering events in pipe mode perf tools: Describe pipe mode in perf.data-file-fomat.txt perf annotate: Process attr and build_id records
perf session: Don't rely on evlist in pipe mode
perf tools: Do not print missing features in pipe-mode

Jin Yao (1):
perf evsel: Return exact sub event which failed with EPERM for wildcards

Taeung Song (6):
perf stat: Refactor the code to strip csv output with ltrim() perf ui browser: Refactor the code to parse color configs with ltrim() perf pmu: Refactor wordwrap() with ltrim()
perf tools: Refactor the code to strip command name with {l,r}trim() perf annotate: Refactor the code to parse disassemble lines with {l,r}trim() perf annotate: Use stripped line instead of raw disassemble line

 tools/perf/Documentation/perf.data-file-format.txt | 19 +++++++-
 tools/perf/arch/s390/annotate/instructions.c       | 30 ++++++++++++
 tools/perf/builtin-annotate.c                      |  2 +
 tools/perf/builtin-inject.c                        |  2 +
 tools/perf/builtin-script.c                        |  4 +-
 tools/perf/builtin-stat.c                          | 10 +---
 tools/perf/ui/browser.c                            |  2 +-
 tools/perf/util/annotate.c                         | 54 +++++++---------------
 tools/perf/util/callchain.c                        |  4 +-
 tools/perf/util/event.c                            | 11 +----
 tools/perf/util/evsel.c                            |  8 +++-
 tools/perf/util/header.c                           |  3 ++
 tools/perf/util/ordered-events.c                   |  3 +-
 tools/perf/util/pmu.c                              |  3 +-
 tools/perf/util/session.c                          | 17 +++++--
 tools/perf/util/string.c                           |  6 +--
 16 files changed, 105 insertions(+), 73 deletions(-)
 create mode 100644 tools/perf/arch/s390/annotate/instructions.c

Test results:

The first ones are container (docker) based builds of tools/perf with and without libelf support, objtool where it is supported and samples/bpf/, ditto. Where clang is available, it is also used to build perf with/without libelf.

For this specific pull request the samples/bpf/ was disabled, as 'make headers_install' is failing with the following error, in this case in fedora:rawhide:

INSTALL usr/include/uapi/ (0 file)
/git/linux/scripts/Makefile.headersinst:62: *** Missing generated UAPI file ./arch/x86/include/generated/uapi/asm/unistd_32.h. Stop. make[1]: *** [/git/linux/Makefile:1151: headers_install] Error 2
make[1]: Leaving directory '/tmp/build/linux'
make: *** [Makefile:152: sub-make] Error 2
make: Leaving directory '/git/linux'

I'll investigate later, perf and objtool builds just fine, with clang and gcc.

Several are cross builds, the ones with -x-ARCH, and the android one, and those may not have all the features built, due to lack of multi-arch devel packages, available and being used so far on just a few, like debian:experimental-x-{arm64,mipsel}.

The 'perf test' one will perform a variety of tests exercising tools/perf/util/, tools/lib/{bpf,traceevent,etc}, as well as run perf commands with a variety of command line event specifications to then intercept the sys_perf_event syscall to check that the perf_event_attr fields are set up as expected, among a variety of other unit tests.

Then there is the 'make -C tools/perf build-test' ones, that build tools/perf/ with a variety of feature sets, exercising the build with an incomplete set of features as well as with a complete one. It is planned to have it run on each of the containers mentioned above, using some container orchestration infrastructure. Get in contact if interested in helping having this in place.

  # dm
   1 alpine:3.4: Ok
   2 alpine:3.5: Ok
   3 alpine:edge: Ok
   4 android-ndk:r12b-arm: Ok
   5 archlinux:latest: Ok
   6 centos:5: Ok
   7 centos:6: Ok
   8 centos:7: Ok
   9 debian:7: Ok
  10 debian:8: Ok
  11 debian:9: Ok
  12 debian:experimental: Ok
  13 debian:experimental-x-arm64: Ok
  14 debian:experimental-x-mips: Ok
  15 debian:experimental-x-mips64: Ok
  16 debian:experimental-x-mipsel: Ok
  17 fedora:20: Ok
  18 fedora:21: Ok
  19 fedora:22: Ok
  20 fedora:23: Ok
  21 fedora:24: Ok
  22 fedora:24-x-ARC-uClibc: Ok
  23 fedora:25: Ok
  24 fedora:rawhide: Ok
  25 mageia:5: Ok
  26 opensuse:13.2: Ok
  27 opensuse:42.1: Ok
  28 opensuse:tumbleweed: Ok
  29 ubuntu:12.04.5: Ok
  30 ubuntu:14.04.4: Ok
  31 ubuntu:14.04.4-x-linaro-arm64: Ok
  32 ubuntu:15.10: Ok
  33 ubuntu:16.04: Ok
  34 ubuntu:16.04-x-arm: Ok
  35 ubuntu:16.04-x-arm64: Ok
  36 ubuntu:16.04-x-powerpc: Ok
  37 ubuntu:16.04-x-powerpc64: Ok
  38 ubuntu:16.04-x-s390: Ok
  39 ubuntu:16.10: Ok
  40 ubuntu:17.04: Ok
  # 

  # uname -a
  Linux jouet 4.11.0-rc6+ #7 SMP Tue Apr 11 11:53:14 -03 2017 x86_64 x86_64 x86_64 GNU/Linux
  # perf test
   1: vmlinux symtab matches kallsyms            : Ok
   2: Detect openat syscall event                : Ok
   3: Detect openat syscall event on all cpus    : Ok
   4: Read samples using the mmap interface      : Ok
   5: Parse event definition strings             : Ok
   6: Simple expression parser                   : Ok
   7: PERF_RECORD_* events & perf_sample fields  : Ok
   8: Parse perf pmu format                      : Ok
   9: DSO data read                              : Ok
  10: DSO data cache                             : Ok
  11: DSO data reopen                            : Ok
  12: Roundtrip evsel->name                      : Ok
  13: Parse sched tracepoints fields             : Ok
  14: syscalls:sys_enter_openat event fields     : Ok
  15: Setup struct perf_event_attr               : Ok
  16: Match and link multiple hists              : Ok
  17: 'import perf' in python                    : Ok
  18: Breakpoint overflow signal handler         : Ok
  19: Breakpoint overflow sampling               : Ok
  20: Number of exit events of a simple workload : Ok
  21: Software clock events period values        : Ok
  22: Object code reading                        : Ok
  23: Sample parsing                             : Ok
  24: Use a dummy software event to keep tracking: Ok
  25: Parse with no sample_id_all bit set        : Ok
  26: Filter hist entries                        : Ok
  27: Lookup mmap thread                         : Ok
  28: Share thread mg                            : Ok
  29: Sort output of hist entries                : Ok
  30: Cumulate child hist entries                : Ok
  31: Track with sched_switch                    : Ok
  32: Filter fds with revents mask in a fdarray  : Ok
  33: Add fd to a fdarray, making it autogrow    : Ok
  34: kmod_path__parse                           : Ok
  35: Thread map                                 : Ok
  36: LLVM search and compile                    :
  36.1: Basic BPF llvm compile                    : Ok
  36.2: kbuild searching                          : Ok
  36.3: Compile source for BPF prologue generation: Ok
  36.4: Compile source for BPF relocation         : Ok
  37: Session topology                           : Ok
  38: BPF filter                                 :
  38.1: Basic BPF filtering                      : Ok
  38.2: BPF pinning                              : Ok
  38.3: BPF prologue generation                  : Ok
  38.4: BPF relocation checker                   : Ok
  39: Synthesize thread map                      : Ok
  40: Remove thread map                          : Ok
  41: Synthesize cpu map                         : Ok
  42: Synthesize stat config                     : Ok
  43: Synthesize stat                            : Ok
  44: Synthesize stat round                      : Ok
  45: Synthesize attr update                     : Ok
  46: Event times                                : Ok
  47: Read backward ring buffer                  : Ok
  48: Print cpu map                              : Ok
  49: Probe SDT events                           : Ok
  50: is_printable_array                         : Ok
  51: Print bitmap                               : Ok
  52: perf hooks                                 : Ok
  53: builtin clang support                      : Skip (not compiled in)
  54: unit_number__scnprintf                     : Ok
  55: x86 rdpmc                                  : Ok
  56: Convert perf time to TSC                   : Ok
  57: DWARF unwind                               : Ok
  58: x86 instruction decoder - new instructions : Ok
  59: Intel cqm nmi context read                 : Skip
  #

  $ make -C tools/perf build-test
  make: Entering directory '/home/acme/git/linux/tools/perf'
  - tarpkg: ./tests/perf-targz-src-pkg .
            make_no_auxtrace_O: make NO_AUXTRACE=1
            make_install_bin_O: make install-bin
                make_minimal_O: make NO_LIBPERL=1 NO_LIBPYTHON=1 NO_NEWT=1 NO_GTK2=1 NO_DEMANGLE=1 NO_LIBELF=1 NO_LIBUNWIND=1 NO_BACKTRACE=1 NO_LIBNUMA=1 NO_LIBAUDIT=1 NO_LIBBIONIC=1 NO_LIBDW_DWARF_UNWIND=1 NO_AUXTRACE=1 NO_LIBBPF=1 NO_LIBCRYPTO=1 NO_SDT=1 NO_JVMTI=1
  make_no_libdw_dwarf_unwind_O: make NO_LIBDW_DWARF_UNWIND=1
   make_install_prefix_slash_O: make install prefix=/tmp/krava/
             make_no_libperl_O: make NO_LIBPERL=1
              make_no_libelf_O: make NO_LIBELF=1
                make_no_gtk2_O: make NO_GTK2=1
                make_no_newt_O: make NO_NEWT=1
                  make_debug_O: make DEBUG=1
              make_no_libbpf_O: make NO_LIBBPF=1
             make_no_libnuma_O: make NO_LIBNUMA=1
                   make_help_O: make help
              make_clean_all_O: make clean all
           make_no_libunwind_O: make NO_LIBUNWIND=1
           make_no_libpython_O: make NO_LIBPYTHON=1
                 make_perf_o_O: make perf.o
                   make_tags_O: make tags
             make_util_map_o_O: make util/map.o
       make_util_pmu_bison_o_O: make util/pmu-bison.o
             make_no_scripts_O: make NO_LIBPYTHON=1 NO_LIBPERL=1
               make_no_slang_O: make NO_SLANG=1
           make_no_backtrace_O: make NO_BACKTRACE=1
                  make_no_ui_O: make NO_NEWT=1 NO_SLANG=1 NO_GTK2=1
         make_with_clangllvm_O: make LIBCLANGLLVM=1
        make_with_babeltrace_O: make LIBBABELTRACE=1
                   make_pure_O: make
                make_install_O: make install
           make_no_libbionic_O: make NO_LIBBIONIC=1
                 make_static_O: make LDFLAGS=-static
                    make_doc_O: make doc
            make_no_demangle_O: make NO_DEMANGLE=1
         make_install_prefix_O: make install prefix=/tmp/krava
            make_no_libaudit_O: make NO_LIBAUDIT=1
  OK
  make: Leaving directory '/home/acme/git/linux/tools/perf'
  $

From: Arnaldo Carvalho de Melo <[email protected]>

Trying to keep everything reentrant.

---
 tools/perf/util/callchain.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 3cea1fb5404b..2e5eff5abef0 100644
@@ -116,7 +116,7 @@ static int
 __parse_callchain_report_opt(const char *arg, bool allow_record_opt)
 {
 	char *tok;
-	char *endptr;
+	char *endptr, *saveptr = NULL;
 	bool minpcnt_set = false;
 	bool record_opt_set = false;
 	bool try_stack_size = false;

@@ -127,7 +127,7 @@ __parse_callchain_report_opt(const char *arg, bool allow_record_opt)
 	if (!arg)
 		return 0;
 
-	while ((tok = strtok((char *)arg, ",")) != NULL) {
+	while ((tok = strtok_r((char *)arg, ",", &saveptr)) != NULL) {
 		if (!strncmp(tok, "none", strlen(tok))) {
 			callchain_param.mode = CHAIN_NONE;
 			callchain_param.enabled = false;

2.9.3

From: Arnaldo Carvalho de Melo <[email protected]>

Just avoiding non-reentrant functions.

---
 tools/perf/builtin-script.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 46acc8ece41f..2dab70fba2ba 100644
@@ -1708,7 +1708,7 @@ static int parse_scriptname(const struct option *opt __maybe_unused,
 static int parse_output_fields(const struct option *opt __maybe_unused,
 			    const char *arg, int unset __maybe_unused)
 {
-	char *tok;
+	char *tok, *strtok_saveptr = NULL;
 	int i, imax = ARRAY_SIZE(all_output_options);
 	int j;
 	int rc = 0;

@@ -1769,7 +1769,7 @@ static int parse_output_fields(const struct option *opt __maybe_unused,
 		}
 	}
 
-	for (tok = strtok(tok, ","); tok; tok = strtok(NULL, ",")) {
+	for (tok = strtok_r(tok, ",", &strtok_saveptr); tok; tok = strtok_r(NULL, ",", &strtok_saveptr)) {
 		for (i = 0; i < imax; ++i) {
 			if (strcmp(tok, all_output_options[i].str) == 0)
 				break;

2.9.3

From: Jin Yao <[email protected]>

The kernel has a special check for a specific irq_vectors trace event.

TRACE_EVENT_PERF_PERM(irq_work_exit,

	is_sampling_event(p_event) ? -EPERM : 0);

The perf-record fails for this irq_vectors event when it is present, like when using a wildcard:

  root@skl:/tmp# perf record -a -e irq_vectors:* sleep 2
  Error:
  You may not have permission to collect system-wide stats.

Consider tweaking /proc/sys/kernel/perf_event_paranoid, which controls use of the performance events system by unprivileged users (without CAP_SYS_ADMIN).

The current value is 2:

-1: Allow use of (almost) all events by all users

To make this setting permanent, edit /etc/sysctl.conf too, e.g.:

kernel.perf_event_paranoid = -1

This patch prints out the exact sub event that failed with EPERM for wildcards to help in understanding what went wrong when this event is present:

After the patch:

  root@skl:/tmp# perf record -a -e irq_vectors:* sleep 2
  Error:
  No permission to enable irq_vectors:irq_work_exit event.

You may not have permission to collect system-wide stats.
......

Committer notes:

So we have a lot of irq_vectors events:

[root@jouet ~]# perf list irq_vectors:*

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

    irq_vectors:call_function_entry                    [Tracepoint event]
    irq_vectors:call_function_exit                     [Tracepoint event]
    irq_vectors:call_function_single_entry             [Tracepoint event]
    irq_vectors:call_function_single_exit              [Tracepoint event]
    irq_vectors:deferred_error_apic_entry              [Tracepoint event]
    irq_vectors:deferred_error_apic_exit               [Tracepoint event]
    irq_vectors:error_apic_entry                       [Tracepoint event]
    irq_vectors:error_apic_exit                        [Tracepoint event]
    irq_vectors:irq_work_entry                         [Tracepoint event]
    irq_vectors:irq_work_exit                          [Tracepoint event]
    irq_vectors:local_timer_entry                      [Tracepoint event]
    irq_vectors:local_timer_exit                       [Tracepoint event]
    irq_vectors:reschedule_entry                       [Tracepoint event]
    irq_vectors:reschedule_exit                        [Tracepoint event]
    irq_vectors:spurious_apic_entry                    [Tracepoint event]
    irq_vectors:spurious_apic_exit                     [Tracepoint event]
    irq_vectors:thermal_apic_entry                     [Tracepoint event]
    irq_vectors:thermal_apic_exit                      [Tracepoint event]
    irq_vectors:threshold_apic_entry                   [Tracepoint event]
    irq_vectors:threshold_apic_exit                    [Tracepoint event]
    irq_vectors:x86_platform_ipi_entry                 [Tracepoint event]
    irq_vectors:x86_platform_ipi_exit                  [Tracepoint event]

#

And some may be sampled:

[root@jouet ~]# perf record -e irq_vectors:local* sleep 20s [ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.020 MB perf.data (2 samples) ] [root@jouet ~]# perf report -D | egrep 'stats:|events:' Aggregated stats:
TOTAL events: 155
MMAP events: 144
COMM events: 2
EXIT events: 1
SAMPLE events: 2
MMAP2 events: 4
FINISHED_ROUND events: 1
TIME_CONV events: 1
irq_vectors:local_timer_entry stats:
TOTAL events: 1
SAMPLE events: 1
irq_vectors:local_timer_exit stats:
TOTAL events: 1
SAMPLE events: 1
[root@jouet ~]#

But, as shown in the tracepoint definition at the start of this message, some, like "irq_vectors:irq_work_exit", may not be sampled, just counted, i.e. if we try to sample, as when using 'perf record', we get an error:

[root@jouet ~]# perf record -e irq_vectors:irq_work_exit Error:
You may not have permission to collect system-wide stats.

Consider tweaking /proc/sys/kernel/perf_event_paranoid,
<SNIP>

The error message is misleading, this patch will help in pointing out what is the event causing such an error, but the error message needs improvement, i.e. we need to figure out a way to check if a tracepoint is counting only, like this one, when all we can do is to count it with 'perf stat', at most printing the delta using interval printing, as in:

[root@jouet ~]# perf stat -I 5000 -e irq_vectors:irq_work_*
# time counts unit events

       5.000168871                  0      irq_vectors:irq_work_entry
       5.000168871                  0      irq_vectors:irq_work_exit
      10.000676730                  0      irq_vectors:irq_work_entry
      10.000676730                  0      irq_vectors:irq_work_exit
      15.001122415                  0      irq_vectors:irq_work_entry
      15.001122415                  0      irq_vectors:irq_work_exit
      20.001298051                  0      irq_vectors:irq_work_entry
      20.001298051                  0      irq_vectors:irq_work_exit
      25.001485020                  1      irq_vectors:irq_work_entry
      25.001485020                  1      irq_vectors:irq_work_exit
      30.001658706                  0      irq_vectors:irq_work_entry
      30.001658706                  0      irq_vectors:irq_work_exit
  ^C    32.045711878                  0      irq_vectors:irq_work_entry
      32.045711878                  0      irq_vectors:irq_work_exit

[root@jouet ~]#

But at least, when we use a wildcard, this patch helps a bit.

---
 tools/perf/util/evsel.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index 9dc7e2d6e48a..8f5d86bd3501 100644
@@ -2457,11 +2457,17 @@ int perf_evsel__open_strerror(struct perf_evsel *evsel, struct target *target,
 			      int err, char *msg, size_t size)
 {
 	char sbuf[STRERR_BUFSIZE];
+	int printed = 0;
 
 	switch (err) {
 	case EPERM:
 	case EACCES:
-		return scnprintf(msg, size,
+		if (err == EPERM)
+			printed = scnprintf(msg, size,
+				"No permission to enable %s event.\n\n",
+				perf_evsel__name(evsel));
+
+		return scnprintf(msg + printed, size - printed,
 		 "You may not have permission to collect %sstats.\n\n"
 		 "Consider tweaking /proc/sys/kernel/perf_event_paranoid,\n"
 		 "which controls use of the performance events system by\n"

2.9.3

From: Taeung Song <[email protected]>

To strip csv output, use ltrim() instead of just while loop and isspace() at print_metric_{only}_csv().

---
 tools/perf/builtin-stat.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

index 2158ea14da57..868e086a6b59 100644
@@ -875,10 +875,7 @@ static void print_metric_csv(void *ctx,
 		return;
 	}
 	snprintf(buf, sizeof(buf), fmt, val);
-	vals = buf;
-	while (isspace(*vals))
-		vals++;
-	ends = vals;
+	ends = vals = ltrim(buf);
 	while (isdigit(*ends) || *ends == '.')
 		ends++;
 	*ends = 0;

@@ -950,10 +947,7 @@ static void print_metric_only_csv(void *ctx, const char *color __maybe_unused,
 		return;
 	unit = fixunit(tbuf, os->evsel, unit);
 	snprintf(buf, sizeof buf, fmt, val);
-	vals = buf;
-	while (isspace(*vals))
-		vals++;
-	ends = vals;
+	ends = vals = ltrim(buf);
 	while (isdigit(*ends) || *ends == '.')
 		ends++;
 	*ends = 0;

2.9.3

From: Taeung Song <[email protected]>

When parsing {fore, back} ground color configs, use ltrim() instead of just while loop and isspace().

---
 tools/perf/ui/browser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 3eb3edb307a4..9e47ccbe07f1 100644
@@ -579,7 +579,7 @@ static int ui_browser__color_config(const char *var, const char *value,
 			break;
 
 		*bg = '\0';
-		while (isspace(*++bg));
+		bg = ltrim(++bg);
 		ui_browser__colorsets[i].bg = bg;
 		ui_browser__colorsets[i].fg = fg;
 		return 0;

2.9.3

From: Taeung Song <[email protected]>

---
 tools/perf/util/pmu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 362051ea7f3d..11c752561c55 100644
@@ -1148,8 +1148,7 @@ static void wordwrap(char *s, int start, int max, int corr)
 			break;
 		s += wlen;
 		column += n;
-		while (isspace(*s))
-			s++;
+		s = ltrim(s);
 	}
 }
 

2.9.3

From: Taeung Song <[email protected]>

After reading command name from /proc/<pid>/status, use ltrim() and rtrim() to strip command name, not using just while loop, isspace() and etc.

---
 tools/perf/util/event.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

index 76b9c6bc8369..8255a26ac255 100644
@@ -106,7 +106,7 @@ static int perf_event__get_comm_ids(pid_t pid, char *comm, size_t len,
 	int fd;
 	size_t size = 0;
 	ssize_t n;
-	char *nl, *name, *tgids, *ppids;
+	char *name, *tgids, *ppids;
 
 	*tgid = -1;
 	*ppid = -1;

@@ -134,14 +134,7 @@ static int perf_event__get_comm_ids(pid_t pid, char *comm, size_t len,
 
 	if (name) {
 		name += 5;  /* strlen("Name:") */
-
-		while (*name && isspace(*name))
-			++name;
-
-		nl = strchr(name, '\n');
-		if (nl)
-			*nl = '\0';
-
+		name = rtrim(ltrim(name));
 		size = strlen(name);
 		if (size >= len)
 			size = len - 1;

2.9.3

From: Arnaldo Carvalho de Melo <[email protected]>

We don't need to use strlen(), a var, or check for the end explicitely, isspace('\0') is false:

  [acme@jouet c]$ cat ltrim.c
  #include <ctype.h>
  #include <stdio.h>

static char *ltrim(char *s)

  {
	  while (isspace(*s))
		  ++s;
	  return s;

}

int main(void)

  {
	  printf("ltrim(\"\")='%s'\n", ltrim(""));
	  return 0;

}
[acme@jouet c]$ ./ltrim
ltrim("")=''
[acme@jouet c]$

---
 tools/perf/util/string.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

index bddca519dd58..e8feb142c9c9 100644
@@ -322,12 +322,8 @@ char *strxfrchar(char *s, char from, char to)
  */
 char *ltrim(char *s)
 {
-	int len = strlen(s);
-
-	while (len && isspace(*s)) {
-		len--;
+	while (isspace(*s))
 		s++;
-	}
 
 	return s;
 }

2.9.3

From: Christian Borntraeger <[email protected]>

since 4.10 perf annotate exits on s390 with an "unknown error -95". Turns out that commit 786c1b51844d ("perf annotate: Start supporting cross arch annotation") added a hard requirement for architecture support when objdump is used but only provided x86 and arm support. Meanwhile power was added so lets add s390 as well.

While at it make sure to implement the branch and jump types.

---
 tools/perf/util/annotate.c | 6 ++++++
 1 file changed, 6 insertions(+)

index a37032bd137d..bfb2f1d393d5 100644
@@ -130,6 +130,12 @@ static struct arch architectures[] = {
 		.name = "powerpc",
 		.init = powerpc__annotate_init,
 	},
+	{
+		.name = "s390",
+		.objdump =  {
+			.comment_char = '#',
+		},
+	},
 };
 
 static void ins__delete(struct ins_operands *ops)

2.9.3

From: Christian Borntraeger <[email protected]>

Implement simple detection for all kind of jumps and branches.

---
 tools/perf/arch/s390/annotate/instructions.c | 30 ++++++++++++++++++++++++++++
 tools/perf/util/annotate.c                   |  2 ++
 2 files changed, 32 insertions(+)
 create mode 100644 tools/perf/arch/s390/annotate/instructions.c

new file mode 100644
index 000000000000..745b4b1b8b21
@@ -0,0 +1,30 @@
+static struct ins_ops *s390__associate_ins_ops(struct arch *arch, const char *name)
+{
+	struct ins_ops *ops = NULL;
+
+	/* catch all kind of jumps */
+	if (strchr(name, 'j') ||
+	    !strncmp(name, "bct", 3) ||
+	    !strncmp(name, "br", 2))
+		ops = &jump_ops;
+	/* override call/returns */
+	if (!strcmp(name, "bras") ||
+	    !strcmp(name, "brasl") ||
+	    !strcmp(name, "basr"))
+		ops = &call_ops;
+	if (!strcmp(name, "br"))
+		ops = &ret_ops;
+
+	arch__associate_ins_ops(arch, name, ops);
+	return ops;
+}
+
+static int s390__annotate_init(struct arch *arch)
+{
+	if (!arch->initialized) {
+		arch->initialized = true;
+		arch->associate_instruction_ops = s390__associate_ins_ops;
+	}
+
+	return 0;
+}

index bfb2f1d393d5..44ed6652b02f 100644
@@ -108,6 +108,7 @@ static int arch__associate_ins_ops(struct arch* arch, const char *name, struct i
 #include "arch/arm64/annotate/instructions.c"
 #include "arch/x86/annotate/instructions.c"
 #include "arch/powerpc/annotate/instructions.c"
+#include "arch/s390/annotate/instructions.c"
 
 static struct arch architectures[] = {
 	{

@@ -132,6 +133,7 @@ static struct arch architectures[] = {
 	},
 	{
 		.name = "s390",
+		.init = s390__annotate_init,
 		.objdump =  {
 			.comment_char = '#',
 		},

2.9.3

From: David Carrillo-Cisneros <[email protected]>

All paths following perf_session__process_event() in __cmd_inject() are useless if __cmd_inject() is to fail, some depend on a correct session->evlist.

First commit to add code that depends on session->evlist without checking error was commmit e558a5bd8b ("perf inject: Work with files"). It has grown since then.

Change __cmd_inject() to fail immediately after
perf_session__process_event() fails.

---
 tools/perf/builtin-inject.c | 2 ++
 1 file changed, 2 insertions(+)

index 42dff0b1375a..65e1c026a2f0 100644
@@ -694,6 +694,8 @@ static int __cmd_inject(struct perf_inject *inject)
 		lseek(fd, output_data_offset, SEEK_SET);
 
 	ret = perf_session__process_events(session);
+	if (ret)
+		return ret;
 
 	if (!file_out->is_pipe) {
 		if (inject->build_ids)

2.9.3

From: David Carrillo-Cisneros <[email protected]>

__perf_session__process_pipe_events reuses the same memory buffer to process all events in the pipe.

When reordering is needed (e.g. -b option), events are not immediately flushed, but kept around until reordering is possible, causing memory corruption.

The problem is usually observed by a "Unknown sample error" output. It can easily be reproduced by:

perf record -o - noploop | perf inject -b > output

Committer testing:

Before:

  $ perf record -o - stress -t 2 -c 2 | perf inject -b > /dev/null
  stress: info: [8297] dispatching hogs: 2 cpu, 0 io, 0 vm, 0 hdd
  stress: info: [8297] successful run completed in 2s
  [ perf record: Woken up 3 times to write data ]
  [ perf record: Captured and wrote 0.000 MB - ]
  Warning:
  Found 1 unknown events!

Is this an older tool processing a perf.data file generated by a more recent tool?

If that is not the case, consider reporting to [email protected].

$

After:

  $ perf record -o - stress -t 2 -c 2 | perf inject -b > /dev/null
  stress: info: [9027] dispatching hogs: 2 cpu, 0 io, 0 vm, 0 hdd
  stress: info: [9027] successful run completed in 2s
  [ perf record: Woken up 3 times to write data ]
  [ perf record: Captured and wrote 0.000 MB - ]
  no symbols found in /usr/bin/stress, maybe install a debug package?
  no symbols found in /usr/bin/stress, maybe install a debug package?
  $

---
 tools/perf/util/ordered-events.c | 3 ++-
 tools/perf/util/session.c        | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

index fe84df1875aa..e70e935b1841 100644
@@ -79,7 +79,7 @@ static union perf_event *dup_event(struct ordered_events *oe,
 
 static void free_dup_event(struct ordered_events *oe, union perf_event *event)
 {
-	if (oe->copy_on_queue) {
+	if (event && oe->copy_on_queue) {
 		oe->cur_alloc_size -= event->header.size;
 		free(event);
 	}

@@ -150,6 +150,7 @@ void ordered_events__delete(struct ordered_events *oe, struct ordered_event *eve
 	list_move(&event->list, &oe->cache);
 	oe->nr_events--;
 	free_dup_event(oe, event->event);
+	event->event = NULL;
 }
 
 int ordered_events__queue(struct ordered_events *oe, union perf_event *event,

index 24259bc2c598..a25302bc55a8 100644
@@ -1656,6 +1656,7 @@ static int __perf_session__process_pipe_events(struct perf_session *session)
 	buf = malloc(cur_size);
 	if (!buf)
 		return -errno;
+	ordered_events__set_copy_on_queue(oe, true);
 more:
 	event = buf;
 	err = readn(fd, event, sizeof(struct perf_event_header));

2.9.3

From: David Carrillo-Cisneros <[email protected]>

Add a minimal description of pipe's data format.

---
 tools/perf/Documentation/perf.data-file-format.txt | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

index b664b18d3991..fa2a9132f0a9 100644
@@ -11,8 +11,8 @@ All fields are in native-endian of the machine that generated the perf.data.
 
 When perf is writing to a pipe it uses a special version of the file
 format that does not rely on seeking to adjust data offsets.  This
-format is not described here. The pipe version can be converted to
-normal perf.data with perf inject.
+format is described in "Pipe-mode data" section. The pipe data version can be
+augmented with additional events using perf inject.
 
 The file starts with a perf_header:
 

@@ -411,6 +411,21 @@ An array bound by the perf_file_section size.
 
 ids points to a array of uint64_t defining the ids for event attr attr.
 
+Pipe-mode data
+
+Pipe-mode avoid seeks in the file by removing the perf_file_section and flags
+from the struct perf_header. The trimmed header is:
+
+struct perf_pipe_file_header {
+	u64				magic;
+	u64				size;
+};
+
+The information about attrs, data, and event_types is instead in the
+synthesized events PERF_RECORD_ATTR, PERF_RECORD_HEADER_TRACING_DATA and
+PERF_RECORD_HEADER_EVENT_TYPE that are generated by perf record in pipe-mode.
+
+
 References:
 
 include/uapi/linux/perf_event.h

2.9.3

From: David Carrillo-Cisneros <[email protected]>

perf annotate did not get some love for pipe-mode, and did not have .attr and .buil_id setup (while record and inject did. Fix that.

It can easily be reproduced by:

perf record -o - noploop | perf annotate

that in my system shows:

    0xd8 [0x28]: failed to process type: 9

Committer Testing:

Before:

  $ perf record -o - stress -t 2 -c 2 | perf annotate --stdio
  stress: info: [11060] dispatching hogs: 2 cpu, 0 io, 0 vm, 0 hdd
  0x4470 [0x28]: failed to process type: 9
  $ stress: info: [11060] successful run completed in 2s

$

After:

  $ perf record -o - stress -t 2 -c 2 | perf annotate --stdio
  stress: info: [11871] dispatching hogs: 2 cpu, 0 io, 0 vm, 0 hdd
  stress: info: [11871] successful run completed in 2s
  [ perf record: Woken up 2 times to write data ]
  [ perf record: Captured and wrote 0.000 MB - ]
  no symbols found in /usr/bin/stress, maybe install a debug package?
   Percent |      Source code & Disassembly of libc-2.24.so for cycles:uhH (6117 samples)
  ---------------------------------------------------------------------------------------
           :
           :      Disassembly of section .text:
           :
           :      000000000003b050 <random_r>:
           :      __random_r():
     10.56 :        3b050:       test   %rdi,%rdi
      0.00 :        3b053:       je     3b0d0 <random_r+0x80>
      0.34 :        3b055:       test   %rsi,%rsi
      0.00 :        3b058:       je     3b0d0 <random_r+0x80>
      0.46 :        3b05a:       mov    0x18(%rdi),%eax
     12.44 :        3b05d:       mov    0x10(%rdi),%r8
      0.18 :        3b061:       test   %eax,%eax
      0.00 :        3b063:       je     3b0b0 <random_r+0x60>
<SNIP>

---
 tools/perf/builtin-annotate.c | 2 ++
 1 file changed, 2 insertions(+)

index 56a7c8d210b9..b2b2722f6bb7 100644
@@ -394,6 +394,8 @@ int cmd_annotate(int argc, const char **argv)
 			.exit	= perf_event__process_exit,
 			.fork	= perf_event__process_fork,
 			.namespaces = perf_event__process_namespaces,
+			.attr	= perf_event__process_attr,
+			.build_id = perf_event__process_build_id,
 			.ordered_events = true,
 			.ordering_requires_timestamps = true,
 		},

2.9.3

From: David Carrillo-Cisneros <[email protected]>

Session sets a number parameters that rely on evlist. These parameters are not used in pipe-mode and should not be set, since evlist is unavailable. Fix that.

---
 tools/perf/util/session.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

index a25302bc55a8..7b740a73e595 100644
@@ -140,8 +140,14 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 			if (perf_session__open(session) < 0)
 				goto out_close;
 
-			perf_session__set_id_hdr_size(session);
-			perf_session__set_comm_exec(session);
+			/*
+			 * set session attributes that are present in perf.data
+			 * but not in pipe-mode.
+			 */
+			if (!file->is_pipe) {
+				perf_session__set_id_hdr_size(session);
+				perf_session__set_comm_exec(session);
+			}
 		}
 	} else  {
 		session->machines.host.env = &perf_env;

@@ -156,7 +162,11 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 			pr_warning("Cannot read kernel map\n");
 	}
 
-	if (tool && tool->ordering_requires_timestamps &&
+	/*
+	 * In pipe-mode, evlist is empty until PERF_RECORD_HEADER_ATTR is
+	 * processed, so perf_evlist__sample_id_all is not meaningful here.
+	 */
+	if ((!file || !file->is_pipe) && tool && tool->ordering_requires_timestamps &&
 	    tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
 		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
 		tool->ordered_events = false;

2.9.3

From: David Carrillo-Cisneros <[email protected]>

Pipe-mode has no perf.data header, hence no upfront knowledge of presend and missing features, hence, do not print missing features in pipe-mode.

---
 tools/perf/util/header.c | 3 +++
 1 file changed, 3 insertions(+)

index ef09f26e67da..2ccc7f06db79 100644
@@ -2270,6 +2270,9 @@ int perf_header__fprintf_info(struct perf_session *session, FILE *fp, bool full)
 	perf_header__process_sections(header, fd, &hd,
 				      perf_file_section__fprintf_info);
 
+	if (session->file->is_pipe)
+		return 0;
+
 	fprintf(fp, "# missing features: ");
 	for_each_clear_bit(bit, header->adds_features, HEADER_LAST_FEATURE) {
 		if (bit)

2.9.3

From: Taeung Song <[email protected]>

When parsing disassemble lines, use ltrim() and rtrim() to strip them, not using just while loop and isspace().

---
 tools/perf/util/annotate.c | 42 +++++++-----------------------------------
 1 file changed, 7 insertions(+), 35 deletions(-)

index 44ed6652b02f..204790db10f1 100644
@@ -387,9 +387,7 @@ static int mov__parse(struct arch *arch, struct ins_operands *ops, struct map *m
 	if (comment == NULL)
 		return 0;
 
-	while (comment[0] != '\0' && isspace(comment[0]))
-		++comment;
-
+	comment = ltrim(comment);
 	comment__symbol(ops->source.raw, comment, &ops->source.addr, &ops->source.name);
 	comment__symbol(ops->target.raw, comment, &ops->target.addr, &ops->target.name);
 

@@ -434,9 +432,7 @@ static int dec__parse(struct arch *arch __maybe_unused, struct ins_operands *ops
 	if (comment == NULL)
 		return 0;
 
-	while (comment[0] != '\0' && isspace(comment[0]))
-		++comment;
-
+	comment = ltrim(comment);
 	comment__symbol(ops->target.raw, comment, &ops->target.addr, &ops->target.name);
 
 	return 0;

@@ -785,10 +781,7 @@ static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, str
 
 static int disasm_line__parse(char *line, const char **namep, char **rawp)
 {
-	char *name = line, tmp;
-
-	while (isspace(name[0]))
-		++name;
+	char tmp, *name = ltrim(line);
 
 	if (name[0] == '\0')
 		return -1;

@@ -806,12 +799,7 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
 		goto out_free_name;
 
 	(*rawp)[0] = tmp;
-
-	if ((*rawp)[0] != '\0') {
-		(*rawp)++;
-		while (isspace((*rawp)[0]))
-			++(*rawp);
-	}
+	*rawp = ltrim(*rawp);
 
 	return 0;
 

@@ -1156,7 +1144,7 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 {
 	struct annotation *notes = symbol__annotation(sym);
 	struct disasm_line *dl;
-	char *line = NULL, *parsed_line, *tmp, *tmp2, *c;
+	char *line = NULL, *parsed_line, *tmp, *tmp2;
 	size_t line_len;
 	s64 line_ip, offset = -1;
 	regmatch_t match[2];

@@ -1167,15 +1155,8 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 	if (!line)
 		return -1;
 
-	while (line_len != 0 && isspace(line[line_len - 1]))
-		line[--line_len] = '\0';
-
-	c = strchr(line, '\n');
-	if (c)
-		*c = 0;
-
 	line_ip = -1;
-	parsed_line = line;
+	parsed_line = rtrim(line);
 
 	/* /filename:linenr ? Save line number and ignore. */
 	if (regexec(&file_lineno, line, 2, match, 0) == 0) {

@@ -1183,16 +1164,7 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 		return 0;
 	}
 
-	/*
-	 * Strip leading spaces:
-	 */
-	tmp = line;
-	while (*tmp) {
-		if (*tmp != ' ')
-			break;
-		tmp++;
-	}
-
+	tmp = ltrim(parsed_line);
 	if (*tmp) {
 		/*
 		 * Parse hexa addresses followed by ':'

2.9.3

From: Taeung Song <[email protected]>

When parsing disassemble lines for source line number, use a stripped line instead of raw line.

---
 tools/perf/util/annotate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 204790db10f1..30498a2d4a6f 100644
@@ -1159,8 +1159,8 @@ static int symbol__parse_objdump_line(struct symbol *sym, struct map *map,
 	parsed_line = rtrim(line);
 
 	/* /filename:linenr ? Save line number and ignore. */
-	if (regexec(&file_lineno, line, 2, match, 0) == 0) {
-		*line_nr = atoi(line + match[1].rm_so);
+	if (regexec(&file_lineno, parsed_line, 2, match, 0) == 0) {
+		*line_nr = atoi(parsed_line + match[1].rm_so);
 		return 0;
 	}
 

2.9.3

Pulled, thanks a lot Arnaldo!

Note, there was a conflict in tools/perf/util/annotate.c due to the s390 fix in perf/urgent. I've fixed it up, but please double check the conflict resolution I made in 43d5b075c400.

Thanks,

Ingo

The merge looks wrong:

dbdebdc53 (Ravi Bangoria            2016-11-23 21:33:46 +0530  133)     },
3c1a42795 (Christian Borntraeger    2017-04-06 09:51:51 +0200  134)     {
3c1a42795 (Christian Borntraeger    2017-04-06 09:51:51 +0200  135)             .name = "s390",
d9f8dfa9b (Christian Borntraeger    2017-04-06 09:51:52 +0200  136)             .init = s390__annotate_init,
e77852b32 (Christian Borntraeger    2017-04-06 09:51:51 +0200  137)             .objdump =  {
e77852b32 (Christian Borntraeger    2017-04-06 09:51:51 +0200  138)                     .comment_char = '#',
e77852b32 (Christian Borntraeger    2017-04-06 09:51:51 +0200  139)             },
e77852b32 (Christian Borntraeger    2017-04-06 09:51:51 +0200  140)     },
43d5b075c (Ingo Molnar              2017-04-12 07:29:13 +0200  141)     {
43d5b075c (Ingo Molnar              2017-04-12 07:29:13 +0200  142)             .name = "s390",
3c1a42795 (Christian Borntraeger    2017-04-06 09:51:51 +0200  143)             .objdump =  {
3c1a42795 (Christian Borntraeger    2017-04-06 09:51:51 +0200  144)                     .comment_char = '#',
3c1a42795 (Christian Borntraeger    2017-04-06 09:51:51 +0200  145)             },
3c1a42795 (Christian Borntraeger    2017-04-06 09:51:51 +0200  146)     },
786c1b518 (Arnaldo Carvalho de Melo 2016-11-16 15:39:50 -0300  147) };

You can delete line 141-146

Ok, I've deleted those lines and amended the merge commit - thanks!

Ingo