perf/core improvements and fixes

Hi Ingo,

Please consider pulling,

- Arnaldo

The following changes since commit 028c63b56795206464263fa3bc47094704c2a840:

Merge tag 'perf-core-for-mingo' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/core (2015-06-09 11:46:04 +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

for you to fetch changes up to c8ad7063626406181a7ebab10cb31b4f741b13d4:

perf tools: Update MANIFEST per files removed from kernel (2015-06-11 22:54:23 -0300)

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

User visible:

- Beautify perf_event_open syscall in 'perf trace'. (Arnaldo Carvalho de Melo)

- Error out unsupported group leader immediately in 'perf stat'. (Kan Liang)

- Amend some 'perf record' option summaries (period, etc) (Peter Zijlstra)

- Avoid possible race condition in copyfile() in 'perf buildid-cache'. (Milos Vyletel)

Infrastructure:

- Display 0x for hex values when printing the attribute. (Adrian Hunter)

- Update MANIFEST per files removed from kernel. (David Ahern)

Build fixes:

- Fix PRIu64 printf related failure on 32-bit arch. (He Kuang)

----------------------------------------------------------------
Adrian Hunter (1):

      perf evsel: Display 0x for hex values when printing the attribute

Arnaldo Carvalho de Melo (1):
trace: Beautify perf_event_open syscall

David Ahern (1):
perf tools: Update MANIFEST per files removed from kernel

He Kuang (1):
perf tools: Fix build failure on 32-bit arch

Kan Liang (1):
perf stat: Error out unsupported group leader immediately

Milos Vyletel (1):
perf tools: Avoid possible race condition in copyfile()

Peter Zijlstra (1):
perf record: Amend option summaries

 tools/perf/Documentation/perf-record.txt | 10 ++++--
 tools/perf/MANIFEST                      |  4 ---
 tools/perf/builtin-record.c              |  7 ++--
 tools/perf/builtin-report.c              |  2 +-
 tools/perf/builtin-stat.c                |  5 ++-
 tools/perf/builtin-trace.c               | 58 ++++++++++++++++++++++++++++++++
 tools/perf/util/evsel.c                  |  2 +-
 tools/perf/util/session.c                |  2 +-
 tools/perf/util/util.c                   | 46 ++++++++++++++++---------
 9 files changed, 106 insertions(+), 30 deletions(-)

From: Milos Vyletel <[email protected]>

Use unique temporary files when copying to buildid dir to prevent races in case multiple instances are trying to copy same file. This is done by

- creating template in form <path>/.<filename>.XXXXXX where the suffix is used by mkstemp() to create unique file
- change file mode
- copy content
- if successful link temp file to target file
- unlink temp file

At this point the only file left at target path should be the desired one either created by us or other instance if we raced. This should also prevent not yet fully copied files to be visible to to other perf instances that could try to parse them.

On top of that slow_copyfile no longer needs to deal with file mode when creating file since temporary file is already created and mode is set.

Succesfully tested by myself by running perf record, archive and reading the data on other system and by running perf buildid-cache on perf binary itself. I also did revert fix from 0635b0f that to exposes previously fixed race with EEXIST and recreator test passed sucessfully.

---
 tools/perf/util/util.c | 46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

index 0c264bc685ac..edc2d633b332 100644
@@ -115,20 +115,17 @@ int rm_rf(char *path)
 	return rmdir(path);
 }
 
-static int slow_copyfile(const char *from, const char *to, mode_t mode)
+static int slow_copyfile(const char *from, const char *to)
 {
 	int err = -1;
 	char *line = NULL;
 	size_t n;
 	FILE *from_fp = fopen(from, "r"), *to_fp;
-	mode_t old_umask;
 
 	if (from_fp == NULL)
 		goto out;
 
-	old_umask = umask(mode ^ 0777);
 	to_fp = fopen(to, "w");
-	umask(old_umask);
 	if (to_fp == NULL)
 		goto out_fclose_from;
 

@@ -178,29 +175,48 @@ int copyfile_mode(const char *from, const char *to, mode_t mode)
 	int fromfd, tofd;
 	struct stat st;
 	int err = -1;
+	char *tmp = NULL, *ptr = NULL;
 
 	if (stat(from, &st))
 		goto out;
 
-	if (st.st_size == 0) /* /proc? do it slowly... */
-		return slow_copyfile(from, to, mode);
-
-	fromfd = open(from, O_RDONLY);
-	if (fromfd < 0)
+	/* extra 'x' at the end is to reserve space for '.' */
+	if (asprintf(&tmp, "%s.XXXXXXx", to) < 0) {
+		tmp = NULL;
 		goto out;
+	}
+	ptr = strrchr(tmp, '/');
+	if (!ptr)
+		goto out;
+	ptr = memmove(ptr + 1, ptr, strlen(ptr) - 1);
+	*ptr = '.';
 
-	tofd = creat(to, mode);
+	tofd = mkstemp(tmp);
 	if (tofd < 0)
-		goto out_close_from;
+		goto out;
+
+	if (fchmod(tofd, mode))
+		goto out_close_to;
+
+	if (st.st_size == 0) { /* /proc? do it slowly... */
+		err = slow_copyfile(from, tmp);
+		goto out_close_to;
+	}
+
+	fromfd = open(from, O_RDONLY);
+	if (fromfd < 0)
+		goto out_close_to;
 
 	err = copyfile_offset(fromfd, 0, tofd, 0, st.st_size);
 
-	close(tofd);
-	if (err)
-		unlink(to);
-out_close_from:
 	close(fromfd);
+out_close_to:
+	close(tofd);
+	if (!err)
+		err = link(tmp, to);
+	unlink(tmp);
 out:
+	free(tmp);
 	return err;
 }
 

2.1.0

From: Peter Zijlstra <[email protected]>

Because there's too many options and I cannot read, I frequently get confused between -c and -P, and try to do things like:

perf record -P 50000 -- foo

Which does not work; try and make the option description slightly longer and hopefully less confusing.

---
 tools/perf/Documentation/perf-record.txt | 10 +++++++---
 tools/perf/builtin-record.c              |  7 +++----
 2 files changed, 10 insertions(+), 7 deletions(-)

index 280533ebf9df..6fdf78625c51 100644
@@ -152,12 +152,16 @@ OPTIONS
 
 -d::
 --data::
-	Sample addresses.
+	Record the sample addresses.
 
 -T::
 --timestamp::
-	Sample timestamps. Use it with 'perf report -D' to see the timestamps,
-	for instance.
+	Record the sample timestamps. Use it with 'perf report -D' to see the
+	timestamps, for instance.
+
+-P::
+--period::
+	Record the sample period.
 
 -n::
 --no-samples::

index d3731cce7c1c..4d6cdeb94fe1 100644
@@ -1027,10 +1027,9 @@ struct option __record_options[] = {
 	OPT_BOOLEAN('q', "quiet", &quiet, "don't print any message"),
 	OPT_BOOLEAN('s', "stat", &record.opts.inherit_stat,
 		    "per thread counts"),
-	OPT_BOOLEAN('d', "data", &record.opts.sample_address,
-		    "Sample addresses"),
-	OPT_BOOLEAN('T', "timestamp", &record.opts.sample_time, "Sample timestamps"),
-	OPT_BOOLEAN('P', "period", &record.opts.period, "Sample period"),
+	OPT_BOOLEAN('d', "data", &record.opts.sample_address, "Record the sample addresses"),
+	OPT_BOOLEAN('T', "timestamp", &record.opts.sample_time, "Record the sample timestamps"),
+	OPT_BOOLEAN('P', "period", &record.opts.period, "Record the sample period"),
 	OPT_BOOLEAN('n', "no-samples", &record.opts.no_samples,
 		    "don't sample"),
 	OPT_BOOLEAN('N', "no-buildid-cache", &record.no_buildid_cache,

2.1.0

From: Adrian Hunter <[email protected]>

Need to display '0x' prefix for hex values otherwise it is not obvious they are hex.

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

index a3e36fc634dc..d4f9994ae47f 100644
@@ -1058,7 +1058,7 @@ static void __p_read_format(char *buf, size_t size, u64 value)
 
 #define BUF_SIZE		1024
 
-#define p_hex(val)		snprintf(buf, BUF_SIZE, "%"PRIx64, (uint64_t)(val))
+#define p_hex(val)		snprintf(buf, BUF_SIZE, "%#"PRIx64, (uint64_t)(val))
 #define p_unsigned(val)		snprintf(buf, BUF_SIZE, "%"PRIu64, (uint64_t)(val))
 #define p_signed(val)		snprintf(buf, BUF_SIZE, "%"PRId64, (int64_t)(val))
 #define p_sample_type(val)	__p_sample_type(buf, BUF_SIZE, val)

2.1.0

From: Kan Liang <[email protected]>

perf stat ignores the unsupported event and continue to count supported event. But if the unsupported event is group leader, perf tool will crash. After applying this patch, the unsupported group leader will error out immediately.

Without this patch:

  $ perf stat -x, -e '{node-prefetch-refs,cycles}' --  sleep 1
  perf: util/evsel.c:1009: get_group_fd: Assertion `!(fd == -1)' failed.
  Aborted (core dumped)

With this patch:

  $ perf stat -x, -e '{node-prefetch-refs,cycles}' --  sleep 1
  Error:
  The node-prefetch-refs event is not supported.

Commiter note: Here I got a different output, but no core dump:

[acme@zoo linux]$ perf stat -x, -e '{node-prefetch-refs,cycles}' -- sleep 1 Error:
The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (node-prefetch-refs).
/bin/dmesg may provide additional information.
No CONFIG_PERF_EVENTS=y kernel support configured?

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

index ff3d25803400..b24ecee95fec 100644
@@ -549,7 +549,10 @@ static int __run_perf_stat(int argc, const char **argv)
 					ui__warning("%s event is not supported by the kernel.\n",
 						    perf_evsel__name(counter));
 				counter->supported = false;
-				continue;
+
+				if ((counter->leader != counter) ||
+				    !(counter->leader->nr_members > 1))
+					continue;
 			}
 
 			perf_evsel__open_strerror(counter, &target,

2.1.0

From: He Kuang <[email protected]>

Failed in 32bit arch build like this:

    CC       /opt/h00206996/output/perf/arm32/builtin-record.o

util/session.c: In function ‘perf_session__warn_about_errors’:
util/session.c:1304:9: error: format ‘%lu’ expects argument of type ‘long unsigned int’,
but argument 2 has type ‘long long unsigned int’ [-Werror=format=]

builtin-report.c: In function ‘perf_evlist__tty_browse_hists’:
builtin-report.c:323:2: error: format ‘%lu’ expects argument of type ‘long unsigned int’,
but argument 3 has type ‘u64’ [-Werror=format=]

Replace %lu format strings in warning message with PRIu64 for u64 'total_lost_samples' to fix this problem.

---
 tools/perf/builtin-report.c | 2 +-
 tools/perf/util/session.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 628090b478ab..32626ea3e227 100644
@@ -320,7 +320,7 @@ static int perf_evlist__tty_browse_hists(struct perf_evlist *evlist,
 {
 	struct perf_evsel *pos;
 
-	fprintf(stdout, "#\n# Total Lost Samples: %lu\n#\n", evlist->stats.total_lost_samples);
+	fprintf(stdout, "#\n# Total Lost Samples: %" PRIu64 "\n#\n", evlist->stats.total_lost_samples);
 	evlist__for_each(evlist, pos) {
 		struct hists *hists = evsel__hists(pos);
 		const char *evname = perf_evsel__name(pos);

index 88d87bf3049f..f31e024ddf7d 100644
@@ -1299,7 +1299,7 @@ static void perf_session__warn_about_errors(const struct perf_session *session)
 		drop_rate = (double)stats->total_lost_samples /
 			    (double) (stats->nr_events[PERF_RECORD_SAMPLE] + stats->total_lost_samples);
 		if (drop_rate > 0.05) {
-			ui__warning("Processed %lu samples and lost %3.2f%% samples!\n\n",
+			ui__warning("Processed %" PRIu64 " samples and lost %3.2f%% samples!\n\n",
 				    stats->nr_events[PERF_RECORD_SAMPLE] + stats->total_lost_samples,
 				    drop_rate * 100.0);
 		}

2.1.0

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

Syswide tracing and then running 'stat' and 'trace':

 $ perf trace -e perf_event_open
 1034.649 (0.019 ms): perf/6133 perf_event_open(attr_uptr: 0x36f0360, pid: 16134, cpu: -1, group_fd: -1, flags: FD_CLOEXEC) = -1 EINVAL Invalid argument
 1034.670 (0.008 ms): perf/6133 perf_event_open(attr_uptr: 0x36f0360, pid: 16134, cpu: -1, group_fd: -1) = -1 EINVAL Invalid argument
 1034.681 (0.007 ms): perf/6133 perf_event_open(attr_uptr: 0x36f0360, pid: 16134, cpu: -1, group_fd: -1) = -1 EINVAL Invalid argument
 1034.692 (0.007 ms): perf/6133 perf_event_open(attr_uptr: 0x36f0360, pid: 16134, cpu: -1, group_fd: -1) = -1 EINVAL Invalid argument
 9986.983 (0.014 ms): trace/6139 perf_event_open(attr_uptr: 0x7ffd9c629320, pid: -1, group_fd: -1, flags: FD_CLOEXEC) = 3
 9987.026 (0.016 ms): trace/6139 perf_event_open(attr_uptr: 0x37c7e70, pid: -1, group_fd: -1, flags: FD_CLOEXEC) = 3
 9987.041 (0.008 ms): trace/6139 perf_event_open(attr_uptr: 0x37c7e70, pid: -1, group_fd: -1, flags: FD_CLOEXEC) = 3
 9987.489 (0.092 ms): trace/6139 perf_event_open(attr_uptr: 0x3795ee0, pid: 16140, group_fd: -1, flags: FD_CLOEXEC) = 3
 9987.536 (0.044 ms): trace/6139 perf_event_open(attr_uptr: 0x3795ee0, pid: 16140, cpu: 1, group_fd: -1, flags: FD_CLOEXEC) = 4
 9987.580 (0.041 ms): trace/6139 perf_event_open(attr_uptr: 0x3795ee0, pid: 16140, cpu: 2, group_fd: -1, flags: FD_CLOEXEC) = 5
 9987.620 (0.037 ms): trace/6139 perf_event_open(attr_uptr: 0x3795ee0, pid: 16140, cpu: 3, group_fd: -1, flags: FD_CLOEXEC) = 7
 9987.659 (0.035 ms): trace/6139 perf_event_open(attr_uptr: 0x37975d0, pid: 16140, group_fd: -1, flags: FD_CLOEXEC) = 8
 9987.692 (0.031 ms): trace/6139 perf_event_open(attr_uptr: 0x37975d0, pid: 16140, cpu: 1, group_fd: -1, flags: FD_CLOEXEC) = 9
 9987.727 (0.032 ms): trace/6139 perf_event_open(attr_uptr: 0x37975d0, pid: 16140, cpu: 2, group_fd: -1, flags: FD_CLOEXEC) = 10
 9987.761 (0.031 ms): trace/6139 perf_event_open(attr_uptr: 0x37975d0, pid: 16140, cpu: 3, group_fd: -1, flags: FD_CLOEXEC) = 11

Need to intercept perf_copy_attr() with a kprobe or with eBPF...

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

index a05490d06374..4bf805b2fbf6 100644
@@ -68,6 +68,23 @@
 # define MSG_CMSG_CLOEXEC	0x40000000
 #endif
 
+#ifndef PERF_FLAG_FD_NO_GROUP
+# define PERF_FLAG_FD_NO_GROUP		(1UL << 0)
+#endif
+
+#ifndef PERF_FLAG_FD_OUTPUT
+# define PERF_FLAG_FD_OUTPUT		(1UL << 1)
+#endif
+
+#ifndef PERF_FLAG_PID_CGROUP
+# define PERF_FLAG_PID_CGROUP		(1UL << 2) /* pid=cgroup id, per-cpu mode only */
+#endif
+
+#ifndef PERF_FLAG_FD_CLOEXEC
+# define PERF_FLAG_FD_CLOEXEC		(1UL << 3) /* O_CLOEXEC */
+#endif
+
+
 struct tp_field {
 	int offset;
 	union {

@@ -358,6 +375,14 @@ static size_t syscall_arg__scnprintf_hex(char *bf, size_t size,
 
 #define SCA_HEX syscall_arg__scnprintf_hex
 
+static size_t syscall_arg__scnprintf_int(char *bf, size_t size,
+					 struct syscall_arg *arg)
+{
+	return scnprintf(bf, size, "%d", arg->val);
+}
+
+#define SCA_INT syscall_arg__scnprintf_int
+
 static size_t syscall_arg__scnprintf_mmap_prot(char *bf, size_t size,
 					       struct syscall_arg *arg)
 {

@@ -810,6 +835,34 @@ static size_t syscall_arg__scnprintf_open_flags(char *bf, size_t size,
 
 #define SCA_OPEN_FLAGS syscall_arg__scnprintf_open_flags
 
+static size_t syscall_arg__scnprintf_perf_flags(char *bf, size_t size,
+						struct syscall_arg *arg)
+{
+	int printed = 0, flags = arg->val;
+
+	if (flags == 0)
+		return 0;
+
+#define	P_FLAG(n) \
+	if (flags & PERF_FLAG_##n) { \
+		printed += scnprintf(bf + printed, size - printed, "%s%s", printed ? "|" : "", #n); \
+		flags &= ~PERF_FLAG_##n; \
+	}
+
+	P_FLAG(FD_NO_GROUP);
+	P_FLAG(FD_OUTPUT);
+	P_FLAG(PID_CGROUP);
+	P_FLAG(FD_CLOEXEC);
+#undef P_FLAG
+
+	if (flags)
+		printed += scnprintf(bf + printed, size - printed, "%s%#x", printed ? "|" : "", flags);
+
+	return printed;
+}
+
+#define SCA_PERF_FLAGS syscall_arg__scnprintf_perf_flags
+
 static size_t syscall_arg__scnprintf_eventfd_flags(char *bf, size_t size,
 						   struct syscall_arg *arg)
 {

@@ -1077,6 +1130,11 @@ static struct syscall_fmt {
 	{ .name	    = "openat",	    .errmsg = true,
 	  .arg_scnprintf = { [0] = SCA_FDAT, /* dfd */
 			     [2] = SCA_OPEN_FLAGS, /* flags */ }, },
+	{ .name	    = "perf_event_open", .errmsg = true,
+	  .arg_scnprintf = { [1] = SCA_INT, /* pid */
+			     [2] = SCA_INT, /* cpu */
+			     [3] = SCA_FD,  /* group_fd */
+			     [4] = SCA_PERF_FLAGS,  /* flags */ }, },
 	{ .name	    = "pipe2",	    .errmsg = true,
 	  .arg_scnprintf = { [1] = SCA_PIPE_FLAGS, /* flags */ }, },
 	{ .name	    = "poll",	    .errmsg = true, .timeout = true, },

2.1.0

From: David Ahern <[email protected]>

Building perf out of kernel tree is currently broken because the MANIFEST file refers to kernel files that have been removed. With this patch make perf-targz-src-pkg succeeds as does building perf using the generated tarfile.

---
 tools/perf/MANIFEST | 4 ----
 1 file changed, 4 deletions(-)

index a0bdd6124583..fe50a1b34aa0 100644
@@ -50,24 +50,20 @@ include/asm-generic/bitops/const_hweight.h
 include/asm-generic/bitops/fls64.h
 include/asm-generic/bitops/__fls.h
 include/asm-generic/bitops/fls.h
-include/linux/const.h
 include/linux/perf_event.h
 include/linux/rbtree.h
 include/linux/list.h
 include/linux/hash.h
 include/linux/stringify.h
-lib/find_next_bit.c
 lib/hweight.c
 lib/rbtree.c
 include/linux/swab.h
 arch/*/include/asm/unistd*.h
-arch/*/include/asm/perf_regs.h
 arch/*/include/uapi/asm/unistd*.h
 arch/*/include/uapi/asm/perf_regs.h
 arch/*/lib/memcpy*.S
 arch/*/lib/memset*.S
 include/linux/poison.h
-include/linux/magic.h
 include/linux/hw_breakpoint.h
 include/linux/rbtree_augmented.h
 include/uapi/linux/perf_event.h

2.1.0

Pulled, thanks a lot Arnaldo!

Ingo