perf/core improvements and fixes

Hi Ingo,

This one has the thread reference counting, that I tested using 'perf probe':

perf probe -x ~/bin/perf 'thread__delete:4 thread refcnt=thread->refcnt tid=thread->tid'
perf probe -x ~/bin/perf 'thread__get:1 thread refcnt=thread->refcnt tid=thread->tid'
perf probe -x ~/bin/perf 'thread__put:6 thread refcnt=thread->refcnt tid=thread->tid'
perf record -o thread_refcnt.data -g -e probe_perf:thread__put,probe_perf:thread__get_1,probe_perf:thread__delete perf top

with that I checked and in the end the refcount reaches zero and
thread__delete is called, as expected, using 'perf script', looking at the callchains, etc, did the same for 'perf sched lat' and 'trace' also seems to work.

David, Namhyung, please holler if you find something fishy with this
thread refcnt stuff, as it is something that is related to previous/current work by you guys,

Ah, I also merged perf/urgent so that it is buildable in more systems.o

There is also the revert for that is_power_of_2 "simplification" in perf_mmap,
that hasn't made it to perf/urgent nor upstream, its something only in perf/core, sorry about that one, should have caught that :-\

Please consider pulling,

- Arnaldo
The following changes since commit 33be4ef116511f1079c4c3bf4b5547faf7439301:

Merge 'tip/perf/urgent' into perf/core to pick fixes (2015-03-02 11:45:49 -0300)

are available in the git repository at:

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

for you to fetch changes up to ae536acfacb65a4a9858c32b12361e09f84f4157:

perf sched: No need to keep the session around (2015-03-03 00:17:12 -0300)

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

User visible:

- Warn if given uprobe event accesses memory on older kernel (Masami Hiramatsu)

- 'perf record' Documentation fixes (Namhyung Kim)

- Report unsupported events properly in 'perf stat' (Suzuki K. Poulose)

Infrastructure:

- Avoid FORK after COMM when synthesizing records for pre-existing threads (Arnaldo Carvalho de Melo)

- Reference count struct thread (Arnaldo Carvalho de Melo)

- No need to keep the session around in 'perf sched', thread refcounting removes that need (Arnaldo Carvalho de Melo)

- Initialize cpu set in pthread_attr_setaffinity_np feature test (Adrian Hunter)

- Only include tsc file for x86 (David Ahern)

- Compare JOBS to 0 after grep (David Ahern)

- Improve feature detection messages (Ingo Molnar)

- Revert "perf: Remove the extra validity check on nr_pages" (Kan Liang)

- Remove bias offset to find probe point by address (Masami Hiramatsu)

- Fix build error on ARCH=i386/x86_64/sparc64 )Namhyung Kim)

----------------------------------------------------------------
Adrian Hunter (1):
perf tools: Initialize cpu set in pthread_attr_setaffinity_np feature test

Arnaldo Carvalho de Melo (3):
perf tools: Fix FORK after COMM when synthesizing records for pre-existing threads perf tools: Reference count struct thread
perf sched: No need to keep the session around

David Ahern (2):
perf tools: Only include tsc file for x86
perf tools: Compare JOBS to 0 after grep

Ingo Molnar (7):
perf tools: Add PERF-FEATURES to the .gitignore file perf tools: Remove annoying extra message from the features build perf tools: Improve Python feature detection messages perf tools: Improve libperl detection message
perf tools: Improve libbfd detection message
perf tools: Improve feature test debuggability
perf tools: Improve 'libbabel' feature check failure message

Kan Liang (1):
Revert "perf: Remove the extra validity check on nr_pages"

Masami Hiramatsu (2):
perf probe: Warn if given uprobe event accesses memory on older kernel perf probe: Remove bias offset to find probe point by address

Namhyung Kim (3):

      perf tools: Fix build error on ARCH=i386/x86_64/sparc64
      perf record: Get rid of -l option from Documentation
      perf record: Document --group option

Suzuki K. Poulose (1):
perf stat: Report unsupported events properly

 kernel/events/core.c                               |  2 +-
 tools/perf/.gitignore                              |  1 +
 tools/perf/Documentation/perf-record.txt           | 12 ++++--
 tools/perf/Makefile                                |  2 +-
 tools/perf/builtin-sched.c                         | 26 +++++--------
 tools/perf/builtin-stat.c                          |  5 ++-
 tools/perf/builtin-trace.c                         |  7 +++-
 tools/perf/config/Makefile                         | 17 ++++-----
 tools/perf/config/Makefile.arch                    | 27 +++----------
 tools/perf/config/feature-checks/Makefile          | 16 ++++----
 .../test-pthread-attr-setaffinity-np.c             |  4 +-
 tools/perf/config/utilities.mak                    |  3 +-
 tools/perf/ui/browsers/hists.c                     |  6 +--
 tools/perf/util/Build                              |  2 +-
 tools/perf/util/build-id.c                         |  5 ++-
 tools/perf/util/event.c                            | 34 ++++++++++++-----
 tools/perf/util/hist.c                             |  2 +
 tools/perf/util/hist.h                             |  2 +-
 tools/perf/util/machine.c                          | 44 +++++++++++-----------
 tools/perf/util/machine.h                          |  1 -
 tools/perf/util/probe-event.c                      | 23 +++++++++++
 tools/perf/util/probe-finder.c                     |  5 +--
 tools/perf/util/session.c                          |  6 ---
 tools/perf/util/thread.c                           | 14 +++++++
 tools/perf/util/thread.h                           | 13 +++++++
 25 files changed, 164 insertions(+), 115 deletions(-)

From: David Ahern <[email protected]>

The perf_time_to_tsc and tsc_to_perf_time functions are only used for x86.

Make inclusion of tsc.c dependent on x86 as well.

---
 tools/perf/util/Build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a2c8047d25f7..972a6e0da7ad 100644
@@ -71,7 +71,7 @@ libperf-y += stat.o
 libperf-y += record.o
 libperf-y += srcline.o
 libperf-y += data.o
-libperf-y += tsc.o
+libperf-$(CONFIG_X86) += tsc.o
 libperf-y += cloexec.o
 libperf-y += thread-stack.o
 

1.9.3

From: David Ahern <[email protected]>

If JOBS is not by user perf tries to autodetect the number by grepping the number of CPUs from /proc/cpuinfo. 'grep -c' will always return an integer so after this command JOBS should be compared to 0, not "".

---
 tools/perf/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index cb2e5868c8e8..d5020aeb5626 100644
@@ -25,7 +25,7 @@ unexport MAKEFLAGS
 #
 ifeq ($(JOBS),)
   JOBS := $(shell grep -c ^processor /proc/cpuinfo 2>/dev/null)
-  ifeq ($(JOBS),)
+  ifeq ($(JOBS),0)
     JOBS := 1
   endif
 endif

1.9.3

From: "Suzuki K. Poulose" <[email protected]>

Commit 1971f59 (perf stat: Use read_counter in read_counter_aggr ) broke the perf stat output for unsupported counters.

 $ perf stat -v -a -C 0 -e CCI_400/config=24/ sleep 1
 Warning:
 CCI_400/config=24/ event is not supported by the kernel.

Performance counter stats for 'system wide':

                  0      CCI_400/config=24/

        1.080265400 seconds time elapsed

Where it used to be :

$ perf stat -v -a -C 0 -e CCI_400/config=24/ sleep 1
 Warning:
 CCI_400/config=24/ event is not supported by the kernel.

Performance counter stats for 'system wide':

    <not supported>      CCI_400/config=24/

        1.083840675 seconds time elapsed

This patch fixes the issues by checking if the counter is supported, before reading and logging the counter value.

---
 tools/perf/builtin-stat.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index e598e4e98170..d28949d210cc 100644
@@ -510,6 +510,9 @@ static int read_counter(struct perf_evsel *counter)
 	int ncpus = perf_evsel__nr_cpus(counter);
 	int cpu, thread;
 
+	if (!counter->supported)
+		return -ENOENT;
+
 	if (counter->system_wide)
 		nthreads = 1;
 

@@ -1285,7 +1288,7 @@ static void print_counter_aggr(struct perf_evsel *counter, char *prefix)
 	if (prefix)
 		fprintf(output, "%s", prefix);
 
-	if (scaled == -1) {
+	if (scaled == -1 || !counter->supported) {
 		fprintf(output, "%*s%s",
 			csv_output ? 0 : 18,
 			counter->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED,

1.9.3

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

In this commit:

commit 363b785f3805a2632eb09a8b430842461c21a640
Author: Don Zickus <[email protected]>
Date: Fri Mar 14 10:43:44 2014 -0400

perf tools: Speed up thread map generation

We ended up emitting PERF_RECORD_FORK events after their corresponding PERF_RECORD_COMM, so the code below will remove the "existing thread" and then recreates it, unnecessarily:

[root@ssdandy ~]# perf probe -x ~/bin/perf -L machine__process_fork_event <machine__process_fork_event@/home/acme/git/linux/tools/perf/util/machine.c:0>

      0  int machine__process_fork_event(struct machine *machine, union perf_event *event,
                                        struct perf_sample *sample)
      2  {
      3         struct thread *thread = machine__find_thread(machine,
                                                             event->fork.pid,
                                                             event->fork.tid);
      6         struct thread *parent = machine__findnew_thread(machine,
                                                                event->fork.ppid,
                                                                event->fork.ptid);

                /* if a thread currently exists for the thread id remove it */
                if (thread != NULL)
     12                 machine__remove_thread(machine, thread);

     14         thread = machine__findnew_thread(machine, event->fork.pid,
                                                 event->fork.tid);
     16         if (dump_trace)
     17                 perf_event__fprintf_task(event, stdout);

     19         if (thread == NULL || parent == NULL ||
     20             thread__fork(thread, parent, sample->time) < 0) {
     21                 dump_printf("problem processing PERF_RECORD_FORK, skipping event.\n");
     22                 return -1;
                }

     25         return 0;
     26  }

[root@ssdandy ~]# perf probe -x ~/bin/perf fork_after_comm=machine__process_fork_event:12 Added new event:
probe_perf:fork_after_comm (on machine__process_fork_event:12 in /home/acme/bin/perf)

You can now use it in all perf tools, such as:

perf record -e probe_perf:fork_after_comm -aR sleep 1

[root@ssdandy ~]#

[root@ssdandy ~]# perf record -g -e probe_perf:* trace -o /tmp/bla ^C[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.021 MB perf.data (30 samples) ] Terminated
[root@ssdandy ~]#

  [root@ssdandy ~]# perf report --no-children --show-total-period --stdio
  # To display the perf.data header info, please use --header/--header-only options.
  #
  # Samples: 30  of event 'probe_perf:fork_after_comm'
  # Event count (approx.): 30
  #
  # Overhead        Period  Command  Shared Object  Symbol
  # ........  ............  .......  .............  ...............................
  #
     100.00%            30  trace    trace          [.] machine__process_fork_event
                |
                ---machine__process_fork_event
                   __event__synthesize_thread.part.2
                   perf_event__synthesize_threads
                   cmd_trace
                   main
                   __libc_start_main

[root@ssdandy ~]#

And Looking at 'perf report -D' output we see it:

0 0 0x8698 [0x30]: PERF_RECORD_COMM: auditd:703/707 0 0 0x86c8 [0x38]: PERF_RECORD_FORK(703:707):(703:703)

Fix it by more closely mimicking how the kernel generates those records when a new fork happens, i.e. first a PERF_RECORD_FORK, then a PERF_RECORD_COMM.

---
 tools/perf/util/event.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

index 9e806d855b04..d5efa5092ce6 100644
@@ -95,9 +95,7 @@ static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len)
 	return tgid;
 }
 
-static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
-					 union perf_event *event, pid_t pid,
-					 perf_event__handler_t process,
+static pid_t perf_event__prepare_comm(union perf_event *event, pid_t pid,
 					 struct machine *machine)
 {
 	size_t size;

@@ -124,6 +122,19 @@ static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
 				(sizeof(event->comm.comm) - size) +
 				machine->id_hdr_size);
 	event->comm.tid = pid;
+out:
+	return tgid;
+}
+
+static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
+					 union perf_event *event, pid_t pid,
+					 perf_event__handler_t process,
+					 struct machine *machine)
+{
+	pid_t tgid = perf_event__prepare_comm(event, pid, machine);
+
+	if (tgid == -1)
+		goto out;
 
 	if (process(tool, event, &synth_sample, machine) != 0)
 		return -1;

@@ -139,7 +150,6 @@ static int perf_event__synthesize_fork(struct perf_tool *tool,
 {
 	memset(&event->fork, 0, sizeof(event->fork) + machine->id_hdr_size);
 
-	/* this is really a clone event but we use fork to synthesize it */
 	event->fork.ppid = tgid;
 	event->fork.ptid = tgid;
 	event->fork.pid  = tgid;

@@ -368,19 +378,23 @@ static int __event__synthesize_thread(union perf_event *comm_event,
 		if (*end)
 			continue;
 
-		tgid = perf_event__synthesize_comm(tool, comm_event, _pid,
-						   process, machine);
+		tgid = perf_event__prepare_comm(comm_event, _pid, machine);
 		if (tgid == -1)
 			return -1;
 
+		if (perf_event__synthesize_fork(tool, fork_event, _pid, tgid,
+						process, machine) < 0)
+			return -1;
+		/*
+		 * Send the prepared comm event
+		 */
+		if (process(tool, comm_event, &synth_sample, machine) != 0)
+			return -1;
+
 		if (_pid == pid) {
 			/* process the parent's maps too */
 			rc = perf_event__synthesize_mmap_events(tool, mmap_event, pid, tgid,
 						process, machine, mmap_data);
-		} else {
-			/* only fork the tid's map, to save time */
-			rc = perf_event__synthesize_fork(tool, fork_event, _pid, tgid,
-						 process, machine);
 		}
 
 		if (rc)

1.9.3

From: Namhyung Kim <[email protected]>

He Kuang reported that current perf tools failed to build when ARCH variable was given like above.

It was because the name is different that internal directory name. I can see that David's sparc64 build has same problem.

So fix it by applying the sed conversion script to the command line ARCH variable also, and fixing the converted name there (i.e. i386/x86_64 -> x86, sparc64 -> sparc).

Acked: Jiri Olsa <[email protected]>

---
 tools/perf/config/Makefile.arch | 27 +++++----------------------
 1 file changed, 5 insertions(+), 22 deletions(-)

index ac8721ffa6c8..e9720571341d 100644
@@ -1,32 +1,15 @@
+ifndef ARCH
+ARCH := $(shell uname -m 2>/dev/null || echo not)
+endif
 
-uname_M := $(shell uname -m 2>/dev/null || echo not)
-
-RAW_ARCH := $(shell echo $(uname_M) | sed -e s/i.86/i386/ -e s/sun4u/sparc64/ \
+ARCH := $(shell echo $(ARCH) | sed -e s/i.86/x86/ -e s/x86_64/x86/ \
+                                  -e s/sun4u/sparc/ -e s/sparc64/sparc/ \
                                   -e s/arm.*/arm/ -e s/sa110/arm/ \
                                   -e s/s390x/s390/ -e s/parisc64/parisc/ \
                                   -e s/ppc.*/powerpc/ -e s/mips.*/mips/ \
                                   -e s/sh[234].*/sh/ -e s/aarch64.*/arm64/ \
                                   -e s/tile.*/tile/ )
 
-# Additional ARCH settings for x86
-ifeq ($(RAW_ARCH),i386)
-  ARCH ?= x86
-endif
-
-ifeq ($(RAW_ARCH),x86_64)
-  ARCH ?= x86
-
-  ifneq (, $(findstring m32,$(CFLAGS)))
-    RAW_ARCH := x86_32
-  endif
-endif
-
-ifeq ($(RAW_ARCH),sparc64)
-  ARCH ?= sparc
-endif
-
-ARCH ?= $(RAW_ARCH)
-
 LP64 := $(shell echo __LP64__ | ${CC} ${CFLAGS} -E -x c - | tail -n 1)
 ifeq ($(LP64), 1)
   IS_64_BIT := 1

1.9.3

From: Namhyung Kim <[email protected]>

The perf record does not support -l option anymore, so nuke it.

---
 tools/perf/Documentation/perf-record.txt | 3 ---
 1 file changed, 3 deletions(-)

index cae75c11120f..4d668945dfe6 100644
@@ -62,9 +62,6 @@ OPTIONS
 --all-cpus::
         System-wide collection from all CPUs.
 
--l::
-        Scale counter values.
-
 -p::
 --pid=::
 	Record events on existing process ID (comma separated list).

1.9.3

From: Namhyung Kim <[email protected]>

The 'perf record --group' option lacks documentation and confuses users. As -e/--event option already supports group spec, it should not be used anymore.

Also add a short description of event group itself.

---
 tools/perf/Documentation/perf-record.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

index 4d668945dfe6..355c4f5569b5 100644
@@ -55,6 +55,11 @@ OPTIONS
           If you want to profile write accesses in [0x1000~1008), just set
           'mem:0x1000/8:w'.
 
+	- a group of events surrounded by a pair of brace ("{event1,event2,...}").
+	  Each event is separated by commas and the group should be quoted to
+	  prevent the shell interpretation.  You also need to use --group on
+	  "perf report" to view group events together.
+
 --filter=<filter>::
         Event filter.
 

@@ -104,6 +109,10 @@ OPTIONS
 	specification with appended unit character - B/K/M/G. The
 	size is rounded up to have nearest pages power of two value.
 
+--group::
+	Put all events in a single event group.  This precedes the --event
+	option and remains only for backward compatibility.  See --event.
+
 -g::
 	Enables call-graph (stack chain/backtrace) recording.
 

1.9.3

From: Ingo Molnar <[email protected]>

It's an auto-generated file.

---
 tools/perf/.gitignore | 1 +
 1 file changed, 1 insertion(+)

index 40399c3d97d6..68328f517a2e 100644
@@ -1,6 +1,7 @@
 PERF-CFLAGS
 PERF-GUI-VARS
 PERF-VERSION-FILE
+PERF-FEATURES
 perf
 perf-read-vdso32
 perf-read-vdsox32

1.9.3

From: Ingo Molnar <[email protected]>

This message:

Makefile:153: The path 'python-config' is not executable.

Appears on every perf build that does not have a sufficient python environment installed. It's really just an internal detail of python configuration pass and users should not see it - and it's pretty meaningless to them in any case because the message is not very helpful. (So it's not executable. Why does that matter? What can the user do about it?)

Remove the warning, the missing python feature warning is sufficient:

config/Makefile:566: No python-config tool was found config/Makefile:566: Python support will not be built

although even that one isn't very helpful to users: so no Python support will be built, what can the user do to fix that? Most other such warnings give package install suggestions.

---
 tools/perf/config/utilities.mak | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 7076a62d0ff7..c16ce833079c 100644
@@ -175,6 +175,5 @@ _ge-abspath = $(if $(is-executable),$(1))
 define get-executable-or-default
 $(if $($(1)),$(call _ge_attempt,$($(1)),$(1)),$(call _ge_attempt,$(2)))
 endef
-_ge_attempt = $(if $(get-executable),$(get-executable),$(_gea_warn)$(call _gea_err,$(2)))
-_gea_warn = $(warning The path '$(1)' is not executable.)
+_ge_attempt = $(if $(get-executable),$(get-executable),$(call _gea_err,$(2)))
 _gea_err  = $(if $(1),$(error Please set '$(1)' appropriately))

1.9.3

From: Ingo Molnar <[email protected]>

Change the Python detection message from:

config/Makefile:566: No python-config tool was found config/Makefile:566: Python support will not be built

config/Makefile:565: No 'python-config' tool was found: disables Python support - please install python-devel/python-dev

It's now a standard one-line message with a package install suggestion, and it also uses the standard language used by other feature detection messages.

---
 tools/perf/config/Makefile | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

index c3570b5f3bf3..d3efeef58ded 100644
@@ -548,22 +548,21 @@ endif
 disable-python = $(eval $(disable-python_code))
 define disable-python_code
   CFLAGS += -DNO_LIBPYTHON
-  $(if $(1),$(warning No $(1) was found))
-  $(warning Python support will not be built)
+  $(warning $1)
   NO_LIBPYTHON := 1
 endef
 
 ifdef NO_LIBPYTHON
-  $(call disable-python)
+  $(call disable-python,Python support disabled by user)
 else
 
   ifndef PYTHON
-    $(call disable-python,python interpreter)
+    $(call disable-python,No python interpreter was found: disables Python support - please install python-devel/python-dev)
   else
     PYTHON_WORD := $(call shell-wordify,$(PYTHON))
 
     ifndef PYTHON_CONFIG
-      $(call disable-python,python-config tool)
+      $(call disable-python,No 'python-config' tool was found: disables Python support - please install python-devel/python-dev)
     else
 
       PYTHON_CONFIG_SQ := $(call shell-sq,$(PYTHON_CONFIG))

@@ -575,7 +574,7 @@ else
       FLAGS_PYTHON_EMBED := $(PYTHON_EMBED_CCOPTS) $(PYTHON_EMBED_LDOPTS)
 
       ifneq ($(feature-libpython), 1)
-        $(call disable-python,Python.h (for Python 2.x))
+        $(call disable-python,No 'Python.h' (for Python 2.x support) was found: disables Python support - please install python-devel/python-dev)
       else
 
         ifneq ($(feature-libpython-version), 1)

1.9.3

From: Ingo Molnar <[email protected]>

Before:

Missing perl devel files. Disabling perl scripting support, consider installing perl-ExtUtils-Embed

After:

Missing perl devel files. Disabling perl scripting support, please install perl-ExtUtils-Embed/libperl-dev

Change the message to the standard 'please install' language and adds Debian-ish package suggestion.

---
 tools/perf/config/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d3efeef58ded..aa2f0aa97e5e 100644
@@ -531,7 +531,7 @@ else
   ifneq ($(feature-libperl), 1)
     CFLAGS += -DNO_LIBPERL
     NO_LIBPERL := 1
-    msg := $(warning Missing perl devel files. Disabling perl scripting support, consider installing perl-ExtUtils-Embed);
+    msg := $(warning Missing perl devel files. Disabling perl scripting support, please install perl-ExtUtils-Embed/libperl-dev);
   else
     LDFLAGS += $(PERL_EMBED_LDFLAGS)
     EXTLIBS += $(PERL_EMBED_LIBADD)

1.9.3

From: Ingo Molnar <[email protected]>

Before:

No bfd.h/libbfd found, install binutils-dev[el]/zlib-static to gain symbol demangling

After:

No bfd.h/libbfd found, please install binutils-dev[el]/zlib-static/libiberty-dev to gain symbol demangling

Change the message to the standard 'please install' language and also add libiberty-dev suggestion for Ubuntu systems.

---
 tools/perf/config/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index aa2f0aa97e5e..e2350ada6ccd 100644
@@ -635,7 +635,7 @@ else
             EXTLIBS += -liberty
             CFLAGS += -DHAVE_CPLUS_DEMANGLE_SUPPORT
           else
-            msg := $(warning No bfd.h/libbfd found, install binutils-dev[el]/zlib-static to gain symbol demangling)
+            msg := $(warning No bfd.h/libbfd found, please install binutils-dev[el]/zlib-static/libiberty-dev to gain symbol demangling)
             CFLAGS += -DNO_DEMANGLE
           endif
         endif

1.9.3

From: Ingo Molnar <[email protected]>

Certain feature tests fail with link errors:

triton:~/tip/tools/perf/config/feature-checks> make test-libbabeltrace.bin
gcc -MD -o test-libbabeltrace.bin test-libbabeltrace.c # -lbabeltrace provided by /tmp/cc6dRSqd.o: In function `main':
test-libbabeltrace.c:(.text+0xf): undefined reference to `bt_ctf_stream_class_get_packet_context_type'

although they should already fail with a build error due to lack of a proper prototype for the function. Due to this I first tried to find which library was missing - while it was the whole feature that was missing from the .h file already.

To solve this, propagate -Wall -Werror to all testcases and remove them from testcase Makefile rules that used them explicitly.

A missing feature now outputs:

triton:~/tip/tools/perf/config/feature-checks> make test-libbabeltrace.bin
gcc -MD -Wall -Werror -o test-libbabeltrace.bin test-libbabeltrace.c # -lbabeltrace provided by
test-libbabeltrace.c: In function ‘main’:
test-libbabeltrace.c:6:2: error: implicit declaration of function ‘bt_ctf_stream_class_get_packet_context_type’ [-Werror=implicit-function-declaration]

---
 tools/perf/config/feature-checks/Makefile | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

index 70c9aebe9da3..8fe067864957 100644
@@ -39,24 +39,24 @@ PKG_CONFIG := $(CROSS_COMPILE)pkg-config
 
 all: $(FILES)
 
-BUILD = $(CC) $(CFLAGS) -o $(OUTPUT)$@ $(patsubst %.bin,%.c,$@) $(LDFLAGS)
+BUILD = $(CC) $(CFLAGS) -Wall -Werror -o $(OUTPUT)$@ $(patsubst %.bin,%.c,$@) $(LDFLAGS)
 
 ###############################
 
 test-all.bin:
-	$(BUILD) -Werror -fstack-protector-all -O2 -Werror -D_FORTIFY_SOURCE=2 -ldw -lelf -lnuma -lelf -laudit -I/usr/include/slang -lslang $(shell $(PKG_CONFIG) --libs --cflags gtk+-2.0 2>/dev/null) $(FLAGS_PERL_EMBED) $(FLAGS_PYTHON_EMBED) -DPACKAGE='"perf"' -lbfd -ldl -lz -lbabeltrace
+	$(BUILD) -fstack-protector-all -O2 -D_FORTIFY_SOURCE=2 -ldw -lelf -lnuma -lelf -laudit -I/usr/include/slang -lslang $(shell $(PKG_CONFIG) --libs --cflags gtk+-2.0 2>/dev/null) $(FLAGS_PERL_EMBED) $(FLAGS_PYTHON_EMBED) -DPACKAGE='"perf"' -lbfd -ldl -lz -lbabeltrace
 
 test-hello.bin:
 	$(BUILD)
 
 test-pthread-attr-setaffinity-np.bin:
-	$(BUILD) -D_GNU_SOURCE -Werror -lpthread
+	$(BUILD) -D_GNU_SOURCE -lpthread
 
 test-stackprotector-all.bin:
-	$(BUILD) -Werror -fstack-protector-all
+	$(BUILD) -fstack-protector-all
 
 test-fortify-source.bin:
-	$(BUILD) -O2 -Werror -D_FORTIFY_SOURCE=2
+	$(BUILD) -O2 -D_FORTIFY_SOURCE=2
 
 test-bionic.bin:
 	$(BUILD)

@@ -119,10 +119,10 @@ test-libbfd.bin:
 	$(BUILD) -DPACKAGE='"perf"' -lbfd -lz -liberty -ldl
 
 test-liberty.bin:
-	$(CC) -o $(OUTPUT)$@ test-libbfd.c -DPACKAGE='"perf"' -lbfd -ldl -liberty
+	$(CC) -Wall -Werror -o $(OUTPUT)$@ test-libbfd.c -DPACKAGE='"perf"' -lbfd -ldl -liberty
 
 test-liberty-z.bin:
-	$(CC) -o $(OUTPUT)$@ test-libbfd.c -DPACKAGE='"perf"' -lbfd -ldl -liberty -lz
+	$(CC) -Wall -Werror -o $(OUTPUT)$@ test-libbfd.c -DPACKAGE='"perf"' -lbfd -ldl -liberty -lz
 
 test-cplus-demangle.bin:
 	$(BUILD) -liberty

@@ -140,7 +140,7 @@ test-libbabeltrace.bin:
 	$(BUILD) # -lbabeltrace provided by $(FEATURE_CHECK_LDFLAGS-libbabeltrace)
 
 test-sync-compare-and-swap.bin:
-	$(BUILD) -Werror
+	$(BUILD)
 
 test-compile-32.bin:
 	$(CC) -m32 -o $(OUTPUT)$@ test-compile.c

1.9.3

From: Ingo Molnar <[email protected]>

On Debian-ish systems libbabeltrace-dev should be suggested as a package install as well.

---
 tools/perf/config/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e2350ada6ccd..d44c64d64465 100644
@@ -706,7 +706,7 @@ endif
 
 ifndef NO_LIBBABELTRACE
   ifeq ($(feature-libbabeltrace), 0)
-    msg := $(warning No libbabeltrace found, disables 'perf data' CTF format support, please install libbabeltrace-devel/libbabeltrace-ctf-dev);
+    msg := $(warning No libbabeltrace found, disables 'perf data' CTF format support, please install libbabeltrace-dev[el]/libbabeltrace-ctf-dev);
     NO_LIBBABELTRACE := 1
   else
     CFLAGS += -DHAVE_LIBBABELTRACE_SUPPORT $(LIBBABELTRACE_CFLAGS)

1.9.3

From: Masami Hiramatsu <[email protected]>

Warn if given uprobe event accesses memory on older kernel.

Until 3.14, uprobe event only supports accessing registers so this warns to upgrade kernel if uprobe-event returns -EINVAL and an argument of the event accesses memory ($stack, @+offset, and +|-offs() symtax).

With this patch (on 3.10.0-123.13.2.el7.x86_64);
-----

  # ./perf probe -x ./perf warn_uprobe_event_compat stack=-0\(%sp\)
  Added new event:
  Failed to write event: Invalid argument
  Please upgrade your kernel to at least 3.14 to have access to feature -0(%sp)
    Error: Failed to add events.
  -----

---
 tools/perf/util/probe-event.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

index 7c0e765fa2e3..1c570c2fa7cc 100644
@@ -2199,6 +2199,27 @@ static int get_new_event_name(char *buf, size_t len, const char *base,
 	return ret;
 }
 
+/* Warn if the current kernel's uprobe implementation is old */
+static void warn_uprobe_event_compat(struct probe_trace_event *tev)
+{
+	int i;
+	char *buf = synthesize_probe_trace_command(tev);
+
+	/* Old uprobe event doesn't support memory dereference */
+	if (!tev->uprobes || tev->nargs == 0 || !buf)
+		goto out;
+
+	for (i = 0; i < tev->nargs; i++)
+		if (strglobmatch(tev->args[i].value, "[$@+-]*")) {
+			pr_warning("Please upgrade your kernel to at least "
+				   "3.14 to have access to feature %s\n",
+				   tev->args[i].value);
+			break;
+		}
+out:
+	free(buf);
+}
+
 static int __add_probe_trace_events(struct perf_probe_event *pev,
 				     struct probe_trace_event *tevs,
 				     int ntevs, bool allow_suffix)

@@ -2295,6 +2316,8 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
 		 */
 		allow_suffix = true;
 	}
+	if (ret == -EINVAL && pev->uprobes)
+		warn_uprobe_event_compat(tev);
 
 	/* Note that it is possible to skip all events because of blacklist */
 	if (ret >= 0 && tev->event) {

1.9.3

From: Masami Hiramatsu <[email protected]>

Remove bias offset to find probe point by address.

Without this patch, probe points on kernel and executables are shown correctly, but do not work with libraries:

  # ./perf probe -l
    probe:do_fork        (on do_fork@kernel/fork.c)
    probe_libc:malloc    (on malloc in /usr/lib64/libc-2.17.so)
    probe_perf:strlist__new (on strlist__new@util/strlist.c in /home/mhiramat/ksrc/linux-3/tools/perf/perf)

Removing bias allows it to show it as real place:

  # ./perf probe -l
    probe:do_fork        (on do_fork@kernel/fork.c)
    probe_libc:malloc    (on __libc_malloc@malloc/malloc.c in /usr/lib64/libc-2.17.so)
    probe_perf:strlist__new (on strlist__new@util/strlist.c in /home/mhiramat/ksrc/linux-3/tools/perf/perf)

---
 tools/perf/util/probe-finder.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index d14193518e4d..46f009aa486c 100644
@@ -1345,11 +1345,8 @@ int debuginfo__find_probe_point(struct debuginfo *dbg, unsigned long addr,
 	const char *fname = NULL, *func = NULL, *basefunc = NULL, *tmp;
 	int baseline = 0, lineno = 0, ret = 0;
 
-	/* Adjust address with bias */
-	addr += dbg->bias;
-
 	/* Find cu die */
-	if (!dwarf_addrdie(dbg->dbg, (Dwarf_Addr)addr - dbg->bias, &cudie)) {
+	if (!dwarf_addrdie(dbg->dbg, (Dwarf_Addr)addr, &cudie)) {
 		pr_warning("Failed to find debug information for address %lx\n",
 			   addr);
 		ret = -EINVAL;

1.9.3

From: Adrian Hunter <[email protected]>

Feature tests are compiled but not executed, however it might avoid a future uninitialized variable warning, so initialize the cpu set.

---
 tools/perf/config/feature-checks/test-pthread-attr-setaffinity-np.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 2b81b72eca23..fdada5e8d454 100644
@@ -1,5 +1,6 @@
 #include <stdint.h>
 #include <pthread.h>
+#include <sched.h>
 
 int main(void)
 {

@@ -8,7 +9,8 @@ int main(void)
 	cpu_set_t cs;
 
 	pthread_attr_init(&thread_attr);
-	/* don't care abt exact args, just the API itself in libpthread */
+	CPU_ZERO(&cs);
+
 	ret = pthread_attr_setaffinity_np(&thread_attr, sizeof(cs), &cs);
 
 	return ret;

1.9.3

From: Kan Liang <[email protected]>

This reverts commit 74390aa55678 ("perf: Remove the extra validity check on nr_pages")

nr_pages equals to number of pages - 1 in perf_mmap. So nr_pages = 0 is valid.

So the nr_pages != 0 && !is_power_of_2(nr_pages) are all needed for checking. Otherwise, for example, perf test 6 failed.

 # perf test 6
  6: x86 rdpmc test                                         :Error:
 mmap() syscall returned with (Invalid argument)
 FAILED!

---
 kernel/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index af924bc38121..8bb20cc39a92 100644
@@ -4446,7 +4446,7 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
 	 * If we have rb pages ensure they're a power-of-two number, so we
 	 * can do bitmasks instead of modulo.
 	 */
-	if (!is_power_of_2(nr_pages))
+	if (nr_pages != 0 && !is_power_of_2(nr_pages))
 		return -EINVAL;
 
 	if (vma_size != PAGE_SIZE * (1 + nr_pages))

1.9.3

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

We need to do that to stop accumulating entries in the dead_threads linked list, i.e. we were keeping references to threads in struct hists that continue to exist even after a thread exited and was removed from the machine threads rbtree.

We still keep the dead_threads list, but just for debugging, allowing us to iterate at any given point over the threads that still are referenced by things like struct hist_entry.

---
 tools/perf/builtin-sched.c     |  2 +-
 tools/perf/builtin-trace.c     |  7 ++++++-
 tools/perf/ui/browsers/hists.c |  6 +++---
 tools/perf/util/build-id.c     |  5 +++--
 tools/perf/util/hist.c         |  2 ++
 tools/perf/util/hist.h         |  2 +-
 tools/perf/util/machine.c      | 44 ++++++++++++++++++++++--------------------
 tools/perf/util/machine.h      |  1 -
 tools/perf/util/session.c      |  6 ------
 tools/perf/util/thread.c       | 14 ++++++++++++++
 tools/perf/util/thread.h       | 13 +++++++++++++
 11 files changed, 66 insertions(+), 36 deletions(-)

index 7ce296618717..e00e2eaf89da 100644
@@ -831,7 +831,7 @@ static int thread_atoms_insert(struct perf_sched *sched, struct thread *thread)
 		return -1;
 	}
 
-	atoms->thread = thread;
+	atoms->thread = thread__get(thread);
 	INIT_LIST_HEAD(&atoms->work_list);
 	__thread_latency_insert(&sched->atom_root, atoms, &sched->cmp_pid);
 	return 0;

index d95a8f4d988c..211614fba217 100644
@@ -1741,7 +1741,10 @@ static int trace__sys_enter(struct trace *trace, struct perf_evsel *evsel,
 	} else
 		ttrace->entry_pending = true;
 
-	trace->current = thread;
+	if (trace->current != thread) {
+		thread__put(trace->current);
+		trace->current = thread__get(thread);
+	}
 
 	return 0;
 }

@@ -2274,6 +2277,8 @@ next_event:
 	}
 
 out_disable:
+	thread__zput(trace->current);
+
 	perf_evlist__disable(evlist);
 
 	if (!err) {

index 788506eef567..ad312d91caed 100644
@@ -1467,7 +1467,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		perf_hpp__set_user_width(symbol_conf.col_width_list_str);
 
 	while (1) {
-		const struct thread *thread = NULL;
+		struct thread *thread = NULL;
 		const struct dso *dso = NULL;
 		int choice = 0,
 		    annotate = -2, zoom_dso = -2, zoom_thread = -2,

@@ -1754,13 +1754,13 @@ zoom_thread:
 				pstack__remove(fstack, &browser->hists->thread_filter);
 zoom_out_thread:
 				ui_helpline__pop();
-				browser->hists->thread_filter = NULL;
+				thread__zput(browser->hists->thread_filter);
 				perf_hpp__set_elide(HISTC_THREAD, false);
 			} else {
 				ui_helpline__fpush("To zoom out press <- or -> + \"Zoom out of %s(%d) thread\"",
 						   thread->comm_set ? thread__comm_str(thread) : "",
 						   thread->tid);
-				browser->hists->thread_filter = thread;
+				browser->hists->thread_filter = thread__get(thread);
 				perf_hpp__set_elide(HISTC_THREAD, false);
 				pstack__push(fstack, &browser->hists->thread_filter);
 			}

index ffdc338df925..a19674666b4e 100644
@@ -61,8 +61,9 @@ static int perf_event__exit_del_thread(struct perf_tool *tool __maybe_unused,
 
 	if (thread) {
 		rb_erase(&thread->rb_node, &machine->threads);
-		machine->last_match = NULL;
-		thread__delete(thread);
+		if (machine->last_match == thread)
+			thread__zput(machine->last_match);
+		thread__put(thread);
 	}
 
 	return 0;

index 70b48a65064c..95f5ab707b74 100644
@@ -355,6 +355,7 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template,
 			callchain_init(he->callchain);
 
 		INIT_LIST_HEAD(&he->pairs.node);
+		thread__get(he->thread);
 	}
 
 	return he;

@@ -941,6 +942,7 @@ hist_entry__collapse(struct hist_entry *left, struct hist_entry *right)
 
 void hist_entry__delete(struct hist_entry *he)
 {
+	thread__zput(he->thread);
 	zfree(&he->branch_info);
 	zfree(&he->mem_info);
 	zfree(&he->stat_acc);

index 2b690d028907..e988c9fcd1bc 100644
@@ -60,7 +60,7 @@ struct hists {
 	struct rb_root		entries_collapsed;
 	u64			nr_entries;
 	u64			nr_non_filtered_entries;
-	const struct thread	*thread_filter;
+	struct thread		*thread_filter;
 	const struct dso	*dso_filter;
 	const char		*uid_filter_str;
 	const char		*symbol_filter_str;

index e8b7779a0a3f..e2faf3b47e7b 100644
@@ -118,7 +118,6 @@ void machines__set_comm_exec(struct machines *machines, bool comm_exec);
 struct machine *machine__new_host(void);
 int machine__init(struct machine *machine, const char *root_dir, pid_t pid);
 void machine__exit(struct machine *machine);
-void machine__delete_dead_threads(struct machine *machine);
 void machine__delete_threads(struct machine *machine);
 void machine__delete(struct machine *machine);
 

index e4f166981ff0..ed4e5cf2bd9d 100644
@@ -138,11 +138,6 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 	return NULL;
 }
 
-static void perf_session__delete_dead_threads(struct perf_session *session)
-{
-	machine__delete_dead_threads(&session->machines.host);
-}
-
 static void perf_session__delete_threads(struct perf_session *session)
 {
 	machine__delete_threads(&session->machines.host);

@@ -167,7 +162,6 @@ static void perf_session_env__delete(struct perf_session_env *env)
 void perf_session__delete(struct perf_session *session)
 {
 	perf_session__destroy_kernel_maps(session);
-	perf_session__delete_dead_threads(session);
 	perf_session__delete_threads(session);
 	perf_session_env__delete(&session->header.env);
 	machines__exit(&session->machines);

index 9ebc8b1f9be5..a5dbba95107f 100644
@@ -82,6 +82,20 @@ void thread__delete(struct thread *thread)
 	free(thread);
 }
 
+struct thread *thread__get(struct thread *thread)
+{
+	++thread->refcnt;
+	return thread;
+}
+
+void thread__put(struct thread *thread)
+{
+	if (thread && --thread->refcnt == 0) {
+		list_del_init(&thread->node);
+		thread__delete(thread);
+	}
+}
+
 struct comm *thread__comm(const struct thread *thread)
 {
 	if (list_empty(&thread->comm_list))

index 160fd066a7d1..783b6688d2f7 100644
@@ -20,6 +20,7 @@ struct thread {
 	pid_t			tid;
 	pid_t			ppid;
 	int			cpu;
+	int			refcnt;
 	char			shortname[3];
 	bool			comm_set;
 	bool			dead; /* if set thread has exited */

@@ -37,6 +38,18 @@ struct comm;
 struct thread *thread__new(pid_t pid, pid_t tid);
 int thread__init_map_groups(struct thread *thread, struct machine *machine);
 void thread__delete(struct thread *thread);
+
+struct thread *thread__get(struct thread *thread);
+void thread__put(struct thread *thread);
+
+static inline void __thread__zput(struct thread **thread)
+{
+	thread__put(*thread);
+	*thread = NULL;
+}
+
+#define thread__zput(thread) __thread__zput(&thread)
+
 static inline void thread__exited(struct thread *thread)
 {
 	thread->dead = true;

1.9.3

Hi Arnaldo,

[SNIP]

[SNIP]

Em Tue, Mar 03, 2015 at 10:42:14PM +0900, Namhyung Kim escreveu:

Sure

Reference counting is a step in that direction, as to if the integer inc/dec is enough or is already surrounded by existing serialization needs to be checked.

- Arnaldo

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

We were keeping the session around just because we kept pointers to struct thread instances, but now we reference count them, so no need for deferring the perf_session__delete call to after we traverse the work_list entries.

---
 tools/perf/builtin-sched.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

index e00e2eaf89da..a3ebf1d3c29d 100644
@@ -1439,8 +1439,7 @@ static int perf_sched__process_tracepoint_sample(struct perf_tool *tool __maybe_
 	return err;
 }
 
-static int perf_sched__read_events(struct perf_sched *sched,
-				   struct perf_session **psession)
+static int perf_sched__read_events(struct perf_sched *sched)
 {
 	const struct perf_evsel_str_handler handlers[] = {
 		{ "sched:sched_switch",	      process_sched_switch_event, },

@@ -1454,6 +1453,7 @@ static int perf_sched__read_events(struct perf_sched *sched,
 		.path = input_name,
 		.mode = PERF_DATA_MODE_READ,
 	};
+	int rc = -1;
 
 	session = perf_session__new(&file, false, &sched->tool);
 	if (session == NULL) {

@@ -1478,16 +1478,10 @@ static int perf_sched__read_events(struct perf_sched *sched,
 		sched->nr_lost_chunks = session->evlist->stats.nr_events[PERF_RECORD_LOST];
 	}
 
-	if (psession)
-		*psession = session;
-	else
-		perf_session__delete(session);
-
-	return 0;
-
+	rc = 0;
 out_delete:
 	perf_session__delete(session);
-	return -1;
+	return rc;
 }
 
 static void print_bad_events(struct perf_sched *sched)

@@ -1515,12 +1509,10 @@ static void print_bad_events(struct perf_sched *sched)
 static int perf_sched__lat(struct perf_sched *sched)
 {
 	struct rb_node *next;
-	struct perf_session *session;
 
 	setup_pager();
 
-	/* save session -- references to threads are held in work_list */
-	if (perf_sched__read_events(sched, &session))
+	if (perf_sched__read_events(sched))
 		return -1;
 
 	perf_sched__sort_lat(sched);

@@ -1537,6 +1529,7 @@ static int perf_sched__lat(struct perf_sched *sched)
 		work_list = rb_entry(next, struct work_atoms, node);
 		output_lat_thread(sched, work_list);
 		next = rb_next(next);
+		thread__zput(work_list->thread);
 	}
 
 	printf(" -----------------------------------------------------------------------------------------------------------------\n");

@@ -1548,7 +1541,6 @@ static int perf_sched__lat(struct perf_sched *sched)
 	print_bad_events(sched);
 	printf("\n");
 
-	perf_session__delete(session);
 	return 0;
 }
 

@@ -1557,7 +1549,7 @@ static int perf_sched__map(struct perf_sched *sched)
 	sched->max_cpu = sysconf(_SC_NPROCESSORS_CONF);
 
 	setup_pager();
-	if (perf_sched__read_events(sched, NULL))
+	if (perf_sched__read_events(sched))
 		return -1;
 	print_bad_events(sched);
 	return 0;

@@ -1572,7 +1564,7 @@ static int perf_sched__replay(struct perf_sched *sched)
 
 	test_calibrations(sched);
 
-	if (perf_sched__read_events(sched, NULL))
+	if (perf_sched__read_events(sched))
 		return -1;
 
 	printf("nr_run_events:        %ld\n", sched->nr_run_events);

1.9.3

Pulled, thanks a lot Arnaldo!

Ingo

So I got this error today:

                                                                  ┌─Warning:───────────────────────────┐
                                                                  │The vmlinux file can't be used.     │
                                                                  │Kernel samples will not be resolved.│
                                                                  │                                    │
                                                                  │                                    │
                                                                  │Press any key...                    │
                                                                  └────────────────────────────────────┘

... and sadly perf is being passive-aggressive again: being negative but refusing to say why! :-)

Is there a way to figure out why it did not like the vmlinux?

Thanks,

Ingo

Em Tue, Mar 10, 2015 at 11:03:31AM +0100, Ingo Molnar escreveu:

I'll check and improve the message.

But you must've noticed that perf is going to a therapist, aka improving error messages... ;-)

- Arnaldo

Absolutely! ;-)

Also, on the positive side, today I was able to build and run perf on ancient user-space: a Fedora Core 6 installation, with very few hacks (see the attached hacks).

The HAVE_NEW_FLAX hack results in a non-working -e option.

Thanks,

Ingo

Index: tip/tools/perf/util/parse-events.c
===================================================================
@@ -968,6 +968,13 @@ perf_pmu__parse_check(const char *name)
 	return r ? r->type : PMU_EVENT_SYMBOL_ERR;
 }
 
+#ifndef HAVE_NEW_FLEX
+static int parse_events_lex_init_extra(int start_token __maybe_unused, void **scanner __maybe_unused)
+{
+	return -1;
+}
+#endif
+
 static int parse_events__scanner(const char *str, void *data, int start_token)
 {
 	YY_BUFFER_STATE buffer;

Index: tip/tools/perf/util/symbol-elf.c
===================================================================
@@ -53,6 +53,13 @@ static int elf_getphdrnum(Elf *elf, size
 }
 #endif
 
+#ifndef HAVE_ELF_GETPHDRNUM
+static int elf_getphdrnum (Elf *__elf __maybe_unused, size_t *__dst __maybe_unused)
+{
+	return 1;
+}
+#endif
+
 #ifndef NT_GNU_BUILD_ID
 #define NT_GNU_BUILD_ID 3
 #endif

Em Tue, Mar 10, 2015 at 11:03:31AM +0100, Ingo Molnar escreveu:

So, we need some infrastructure for that, i.e. a strerror like routine like we have for other classes, because there are lots of things that can go wrong while loading a file: ELF error, decompression stuff, you name it, so, with the patch below it ends up as:

# perf top --vmlinux /dev/null

           ┌─Warning:───────────────────────────────────────────┐
           │The /tmp/passwd file can't be used: Invalid ELF file│
           │Kernel samples will not be resolved.                │
           │                                                    │
           │                                                    │
           │Press any key...                                    │
           └────────────────────────────────────────────────────┘

Basically save the errno, that may be an elf_errno (in this case I just came up with a DSO_LOAD_ERRNO__INVALID_ELF), an errno or some other error that needs propagating.

Just a heads up, I'll polish it a bit more before submitting, sounds OK?

Jiri, Namhyung, this would go down to the decompression code, etc.

Right now some of this can be obtained via the pr_debug calls, but that is too ä passive aggressive, and we don't want to do the error report syncronously, this is up for the tools/UI used, so saving it and providing a strerror routine looks sane, right?

- Arnaldo

index 5fb8723c7128..1cb3436276d1 100644
@@ -757,8 +757,10 @@ static void perf_event__process_sample(struct perf_tool *tool,
 		    al.map == machine->vmlinux_maps[MAP__FUNCTION] &&
 		    RB_EMPTY_ROOT(&al.map->dso->symbols[MAP__FUNCTION])) {
 			if (symbol_conf.vmlinux_name) {
-				ui__warning("The %s file can't be used.\n%s",
-					    symbol_conf.vmlinux_name, msg);
+				char serr[256];
+				dso__strerror_load(al.map->dso, serr, sizeof(serr));
+				ui__warning("The %s file can't be used: %s\n%s",
+					    symbol_conf.vmlinux_name, serr, msg);
 			} else {
 				ui__warning("A vmlinux file was not found.\n%s",
 					    msg);

index 0d3667f92023..4e33be8f92ba 100644
@@ -1137,3 +1137,34 @@ enum dso_type dso__type(struct dso *dso, struct machine *machine)
 
 	return dso__type_fd(fd);
 }
+
+int dso__strerror_load(struct dso *dso, char *buf, size_t buflen)
+{
+	int idx, errnum = dso->load_errno;
+	/*
+	 * This must have a same ordering as the enum dso_load_errno.
+	 */
+	static const char *dso_load__error_str[] = {
+	"Internal tools/perf/ library error",
+	"Invalid ELF file",
+	"Decompression failure",
+	};
+
+	BUG_ON(buflen == 0);
+
+	if (errnum >= 0) {
+		const char *err = strerror_r(errnum, buf, buflen);
+
+		if (err != buf)
+			scnprintf(buf, buflen, "%s", err);
+
+		return 0;
+	}
+
+	if (errnum <  __DSO_LOAD_ERRNO__START || errnum >= __DSO_LOAD_ERRNO__END)
+		return -1;
+
+	idx = errnum - __DSO_LOAD_ERRNO__START;
+	scnprintf(buf, buflen, "%s", dso_load__error_str[idx]);
+	return 0;
+}

index 88f345cc5be2..25f78bb15819 100644
@@ -60,6 +60,29 @@ enum dso_type {
 	DSO__TYPE_X32BIT,
 };
 
+enum dso_load_errno {
+	DSO_LOAD_ERRNO__SUCCESS		= 0,
+
+	/*
+	 * Choose an arbitrary negative big number not to clash with standard
+	 * errno since SUS requires the errno has distinct positive values.
+	 * See 'Issue 6' in the link below.
+	 *
+	 * http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html
+	 */
+	__DSO_LOAD_ERRNO__START		= -10000,
+
+	DSO_LOAD_ERRNO__INTERNAL_ERROR	= __DSO_LOAD_ERRNO__START,
+
+	/* for symsrc__init() */
+	DSO_LOAD_ERRNO__INVALID_ELF,
+
+	/* for decompress_kmodule */
+	DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE,
+
+	__DSO_LOAD_ERRNO__END,
+};
+
 #define DSO__SWAP(dso, type, val)			\
 ({							\
 	type ____r = val;				\

@@ -113,6 +136,7 @@ struct dso {
 	enum dso_swap_type	needs_swap;
 	enum dso_binary_type	symtab_type;
 	enum dso_binary_type	binary_type;
+	enum dso_load_errno	load_errno;
 	u8		 adjust_symbols:1;
 	u8		 has_build_id:1;
 	u8		 has_srcline:1;

@@ -294,4 +318,6 @@ void dso__free_a2l(struct dso *dso);
 
 enum dso_type dso__type(struct dso *dso, struct machine *machine);
 
+int dso__strerror_load(struct dso *dso, char *buf, size_t buflen);
+
 #endif /* __PERF_DSO */

index 78ffde9df9bf..2a464273a43b 100644
@@ -595,10 +595,13 @@ static int decompress_kmodule(struct dso *dso, const char *name,
 		return -1;
 
 	fd = mkstemp(tmpbuf);
-	if (fd < 0)
+	if (fd < 0) {
+		dso->load_errno = errno;
 		goto out;
+	}
 
 	if (!decompress_to_file(m.ext, name, fd)) {
+		dso->load_errno = DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE;
 		close(fd);
 		fd = -1;
 	}

@@ -635,27 +638,35 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 	Elf *elf;
 	int fd;
 
-	if (dso__needs_decompress(dso))
+	if (dso__needs_decompress(dso)) {
 		fd = decompress_kmodule(dso, name, type);
-	else
+		if (fd < 0)
+			return -1;
+	} else {
 		fd = open(name, O_RDONLY);
-
-	if (fd < 0)
-		return -1;
+		if (fd < 0) {
+			dso->load_errno = errno;
+			return -1;
+		}
+	}
 
 	elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL);
 	if (elf == NULL) {
 		pr_debug("%s: cannot read %s ELF file.\n", __func__, name);
+		dso->load_errno = DSO_LOAD_ERRNO__INVALID_ELF;
 		goto out_close;
 	}
 
 	if (gelf_getehdr(elf, &ehdr) == NULL) {
+		dso->load_errno = DSO_LOAD_ERRNO__INVALID_ELF;
 		pr_debug("%s: cannot get elf header.\n", __func__);
 		goto out_elf_end;
 	}
 
-	if (dso__swap_init(dso, ehdr.e_ident[EI_DATA]))
+	if (dso__swap_init(dso, ehdr.e_ident[EI_DATA])) {
+		dso->load_errno = DSO_LOAD_ERRNO__INTERNAL_ERROR;
 		goto out_elf_end;
+	}
 
 	/* Always reject images with a mismatched build-id: */
 	if (dso->has_build_id) {

@@ -701,8 +712,10 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 	}
 
 	ss->name   = strdup(name);
-	if (!ss->name)
+	if (!ss->name) {
+		dso->load_errno = errno;
 		goto out_elf_end;
+	}
 
 	ss->elf    = elf;
 	ss->fd     = fd;

right, DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE seems ok

jirka

Em Tue, Mar 24, 2015 at 02:16:56PM +0100, Jiri Olsa escreveu:

Ok, set dso->load_errno in a few more places, introduced a couple of build id errnos and sticked an "Acked-by: jirka" to it, thanks!

- Arnaldo