perf record: Allow to ignore missing pid

Removing extra '--' prefix.

Signed-off-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

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

tools/perf/builtin-mem.c +2 -2

index d1ce29be560e..cd7bc4d104e2 100644
@@ -70,8 +70,8 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
 	OPT_UINTEGER(0, "ldlat", &perf_mem_events__loads_ldlat, "mem-loads latency"),
 	OPT_INCR('v', "verbose", &verbose,
 		 "be more verbose (show counter open errors, etc)"),
-	OPT_BOOLEAN('U', "--all-user", &all_user, "collect only user level data"),
-	OPT_BOOLEAN('K', "--all-kernel", &all_kernel, "collect only kernel level data"),
+	OPT_BOOLEAN('U', "all-user", &all_user, "collect only user level data"),
+	OPT_BOOLEAN('K', "all-kernel", &all_kernel, "collect only kernel level data"),
 	OPT_END()
 	};
 

Signature · 1 line

2.7.4

Em Mon, Dec 12, 2016 at 11:35:39AM +0100, Jiri Olsa escreveu:

Quoted 1 line

Removing extra '--' prefix.

Please add a Fixes: tag in such cases, as this helps backporters to figure out what affects packages they published, in this case I'm adding:

Fixes ad16511b0e40 ("perf mem: Add -U/-K (--all-user/--all-kernel)

options")

And I just added this to the Todo[1] list:

* Improve git pre-commit hook script to check for OPTION() with -- in its name, to fix bugs like ad16511b0e40 ("perf mem: Add -U/-K (--all-user/--all-kernel) options") (acme)

:-)

Thanks,

- Arnaldo

[1] https://perf.wiki.kernel.org/index.php/Todo

Quoted 26 lines

Signed-off-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

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

tools/perf/builtin-mem.c +2 -2

index d1ce29be560e..cd7bc4d104e2 100644
@@ -70,8 +70,8 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
 	OPT_UINTEGER(0, "ldlat", &perf_mem_events__loads_ldlat, "mem-loads latency"),
 	OPT_INCR('v', "verbose", &verbose,
 		 "be more verbose (show counter open errors, etc)"),
-	OPT_BOOLEAN('U', "--all-user", &all_user, "collect only user level data"),
-	OPT_BOOLEAN('K', "--all-kernel", &all_kernel, "collect only kernel level data"),
+	OPT_BOOLEAN('U', "all-user", &all_user, "collect only user level data"),
+	OPT_BOOLEAN('K', "all-kernel", &all_kernel, "collect only kernel level data"),
 	OPT_END()
 	};
 

Signature · 1 line

2.7.4

Adding thread_map__remove function to remove thread from thread map. Adding automated test also.

Signed-off-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

---
 tools/perf/tests/builtin-test.c |  4 ++++
 tools/perf/tests/tests.h        |  1 +
 tools/perf/tests/thread-map.c   | 43 +++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/thread_map.c    | 22 +++++++++++++++++++++
 tools/perf/util/thread_map.h    |  1 +
 5 files changed, 71 insertions(+)

tools/perf/tests/builtin-test.c +4 -0

index 23605202d4a1..a77dcc0d24e3 100644
@@ -186,6 +186,10 @@ static struct test generic_tests[] = {
 		.func = test__thread_map_synthesize,
 	},
 	{
+		.desc = "Remove thread map",
+		.func = test__thread_map_remove,
+	},
+	{
 		.desc = "Synthesize cpu map",
 		.func = test__cpu_map_synthesize,
 	},

tools/perf/tests/tests.h +1 -0

index 0d7b251305af..a512f0c8ff5b 100644
@@ -80,6 +80,7 @@ const char *test__bpf_subtest_get_desc(int subtest);
 int test__bpf_subtest_get_nr(void);
 int test_session_topology(int subtest);
 int test__thread_map_synthesize(int subtest);
+int test__thread_map_remove(int subtest);
 int test__cpu_map_synthesize(int subtest);
 int test__synthesize_stat_config(int subtest);
 int test__synthesize_stat(int subtest);

tools/perf/tests/thread-map.c +43 -0

index cee2a2cdc933..e227e06ed7be 100644
@@ -93,3 +93,46 @@ int test__thread_map_synthesize(int subtest __maybe_unused)
 
 	return 0;
 }
+
+int test__thread_map_remove(int subtest __maybe_unused)
+{
+	struct thread_map *threads;
+	char *str;
+	int i;
+
+	TEST_ASSERT_VAL("failed to allocate map string",
+			asprintf(&str, "%d,%d", getpid(), getppid()) >= 0);
+
+	threads = thread_map__new_str(str, NULL, 0);
+
+	TEST_ASSERT_VAL("failed to allocate thread_map",
+			threads);
+
+	if (verbose)
+		thread_map__fprintf(threads, stderr);
+
+	TEST_ASSERT_VAL("failed to remove thread",
+			!thread_map__remove(threads, 0));
+
+	TEST_ASSERT_VAL("thread_map count != 1", threads->nr == 1);
+
+	if (verbose)
+		thread_map__fprintf(threads, stderr);
+
+	TEST_ASSERT_VAL("failed to remove thread",
+			!thread_map__remove(threads, 0));
+
+	TEST_ASSERT_VAL("thread_map count != 0", threads->nr == 0);
+
+	if (verbose)
+		thread_map__fprintf(threads, stderr);
+
+	TEST_ASSERT_VAL("failed to not remove thread",
+			thread_map__remove(threads, 0));
+
+	for (i = 0; i < threads->nr; i++)
+		free(threads->map[i].comm);
+
+	free(threads);
+	return 0;
+}

tools/perf/util/thread_map.c +22 -0

index 40585f5b7027..f9eab200fd75 100644
@@ -448,3 +448,25 @@ bool thread_map__has(struct thread_map *threads, pid_t pid)
 
 	return false;
 }
+
+int thread_map__remove(struct thread_map *threads, int idx)
+{
+	int i;
+
+	if (threads->nr < 1)
+		return -EINVAL;
+
+	if (idx >= threads->nr)
+		return -EINVAL;
+
+	/*
+	 * Free the 'idx' item and shift the rest up.
+	 */
+	free(threads->map[idx].comm);
+
+	for (i = idx; i < threads->nr - 1; i++)
+		threads->map[i] = threads->map[i + 1];
+
+	threads->nr--;
+	return 0;
+}

tools/perf/util/thread_map.h +1 -0

index bd3b971588da..ea0ef08c6303 100644
@@ -58,4 +58,5 @@ static inline char *thread_map__comm(struct thread_map *map, int thread)
 
 void thread_map__read_comms(struct thread_map *threads);
 bool thread_map__has(struct thread_map *threads, pid_t pid);
+int thread_map__remove(struct thread_map *threads, int idx);
 #endif	/* __PERF_THREAD_MAP_H */

Signature · 1 line

2.7.4

Em Mon, Dec 12, 2016 at 11:35:41AM +0100, Jiri Olsa escreveu:

Quoted 2 lines

Adding thread_map__remove function to remove thread from thread map. Adding automated test also.

I'm fixing this, caught on debian:experimental-x-mips64

CC /tmp/build/perf/tests/thread-map.o
CC /tmp/build/perf/tests/llvm.o
tests/thread-map.c: In function 'test__thread_map_remove':
tests/thread-map.c:134:3: error: implicit declaration of function 'free' [-Werror=implicit-function-declaration]

   free(threads->map[i].comm);
   ^~~~

tests/thread-map.c:134:3: error: incompatible implicit declaration of built-in function 'free' [-Werror] tests/thread-map.c:134:3: note: include '<stdlib.h>' or provide a declaration of 'free' tests/thread-map.c:136:2: error: incompatible implicit declaration of built-in function 'free' [-Werror]
free(threads);
^~~~
tests/thread-map.c:136:2: note: include '<stdlib.h>' or provide a declaration of 'free'
CC /tmp/build/perf/tests/bpf.o

Quoted 133 lines

Signed-off-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

---
 tools/perf/tests/builtin-test.c |  4 ++++
 tools/perf/tests/tests.h        |  1 +
 tools/perf/tests/thread-map.c   | 43 +++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/thread_map.c    | 22 +++++++++++++++++++++
 tools/perf/util/thread_map.h    |  1 +
 5 files changed, 71 insertions(+)

tools/perf/tests/builtin-test.c +4 -0

index 23605202d4a1..a77dcc0d24e3 100644
@@ -186,6 +186,10 @@ static struct test generic_tests[] = {
 		.func = test__thread_map_synthesize,
 	},
 	{
+		.desc = "Remove thread map",
+		.func = test__thread_map_remove,
+	},
+	{
 		.desc = "Synthesize cpu map",
 		.func = test__cpu_map_synthesize,
 	},

tools/perf/tests/tests.h +1 -0

index 0d7b251305af..a512f0c8ff5b 100644
@@ -80,6 +80,7 @@ const char *test__bpf_subtest_get_desc(int subtest);
 int test__bpf_subtest_get_nr(void);
 int test_session_topology(int subtest);
 int test__thread_map_synthesize(int subtest);
+int test__thread_map_remove(int subtest);
 int test__cpu_map_synthesize(int subtest);
 int test__synthesize_stat_config(int subtest);
 int test__synthesize_stat(int subtest);

tools/perf/tests/thread-map.c +43 -0

index cee2a2cdc933..e227e06ed7be 100644
@@ -93,3 +93,46 @@ int test__thread_map_synthesize(int subtest __maybe_unused)
 
 	return 0;
 }
+
+int test__thread_map_remove(int subtest __maybe_unused)
+{
+	struct thread_map *threads;
+	char *str;
+	int i;
+
+	TEST_ASSERT_VAL("failed to allocate map string",
+			asprintf(&str, "%d,%d", getpid(), getppid()) >= 0);
+
+	threads = thread_map__new_str(str, NULL, 0);
+
+	TEST_ASSERT_VAL("failed to allocate thread_map",
+			threads);
+
+	if (verbose)
+		thread_map__fprintf(threads, stderr);
+
+	TEST_ASSERT_VAL("failed to remove thread",
+			!thread_map__remove(threads, 0));
+
+	TEST_ASSERT_VAL("thread_map count != 1", threads->nr == 1);
+
+	if (verbose)
+		thread_map__fprintf(threads, stderr);
+
+	TEST_ASSERT_VAL("failed to remove thread",
+			!thread_map__remove(threads, 0));
+
+	TEST_ASSERT_VAL("thread_map count != 0", threads->nr == 0);
+
+	if (verbose)
+		thread_map__fprintf(threads, stderr);
+
+	TEST_ASSERT_VAL("failed to not remove thread",
+			thread_map__remove(threads, 0));
+
+	for (i = 0; i < threads->nr; i++)
+		free(threads->map[i].comm);
+
+	free(threads);
+	return 0;
+}

tools/perf/util/thread_map.c +22 -0

index 40585f5b7027..f9eab200fd75 100644
@@ -448,3 +448,25 @@ bool thread_map__has(struct thread_map *threads, pid_t pid)
 
 	return false;
 }
+
+int thread_map__remove(struct thread_map *threads, int idx)
+{
+	int i;
+
+	if (threads->nr < 1)
+		return -EINVAL;
+
+	if (idx >= threads->nr)
+		return -EINVAL;
+
+	/*
+	 * Free the 'idx' item and shift the rest up.
+	 */
+	free(threads->map[idx].comm);
+
+	for (i = idx; i < threads->nr - 1; i++)
+		threads->map[i] = threads->map[i + 1];
+
+	threads->nr--;
+	return 0;
+}

tools/perf/util/thread_map.h +1 -0

index bd3b971588da..ea0ef08c6303 100644
@@ -58,4 +58,5 @@ static inline char *thread_map__comm(struct thread_map *map, int thread)
 
 void thread_map__read_comms(struct thread_map *threads);
 bool thread_map__has(struct thread_map *threads, pid_t pid);
+int thread_map__remove(struct thread_map *threads, int idx);
 #endif	/* __PERF_THREAD_MAP_H */

Signature · 1 line

2.7.4

Adding perf_evsel::ignore_missing_cpu_thread bool.

When set true, it allows perf to ignore error of missing pid of perf event syscall.

We remove missing thread id from the thread_map, so the rest of the processing like ioctl and mmap won't get disturbed with -1 fd

The reason for supporting this is to ease up monitoring group of pids, that 'disappear' before perf opens their event. This currently leads perf to report error and exit and makes perf record's -u option unusable under certain setup.

With this change we will allow this race and ignore such failure with following warning:

WARNING: Ignored open failure for pid 8605

Signed-off-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

---
 tools/perf/perf.h       |  1 +
 tools/perf/util/evsel.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/evsel.h |  1 +
 3 files changed, 46 insertions(+)

tools/perf/perf.h +1 -0

index 9a0236a4cf95..1c27d947c2fe 100644
@@ -55,6 +55,7 @@ struct record_opts {
 	bool	     all_user;
 	bool	     tail_synthesize;
 	bool	     overwrite;
+	bool	     ignore_missing_thread;
 	unsigned int freq;
 	unsigned int mmap_pages;
 	unsigned int auxtrace_mmap_pages;

tools/perf/util/evsel.c +44 -0

index fd61ebd77c26..ac87f1637ae9 100644
@@ -990,6 +990,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
 	 * it overloads any global configuration.
 	 */
 	apply_config_terms(evsel, opts);
+
+	evsel->ignore_missing_thread = opts->ignore_missing_thread;
 }
 
 static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)

@@ -1419,6 +1421,33 @@ static int __open_attr__fprintf(FILE *fp, const char *name, const char *val,
 	return fprintf(fp, "  %-32s %s\n", name, val);
 }
 
+static bool ignore_missing_thread(struct perf_evsel *evsel,
+				  struct thread_map *threads,
+				  int thread, int err)
+{
+	if (!evsel->ignore_missing_thread)
+		return false;
+
+	/* The system wide setup does not work with threads. */
+	if (!evsel->system_wide)
+		return false;
+
+	/* The -ESRCH is perf event syscall errno for pid's not found. */
+	if (err != -ESRCH)
+		return false;
+
+	/* If there's only one thread, let it fail. */
+	if (threads->nr == 1)
+		return false;
+
+	if (thread_map__remove(threads, thread))
+		return false;
+
+	pr_warning("WARNING: Ignored open failure for pid %d\n",
+		   thread_map__pid(threads, thread));
+	return true;
+}
+
 static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 			      struct thread_map *threads)
 {

@@ -1491,6 +1520,21 @@ static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 
 			if (fd < 0) {
 				err = -errno;
+
+				if (ignore_missing_thread(evsel, threads, thread, err)) {
+					/*
+					 * We just removed 1 thread, so take a step
+					 * back on thread index and lower the upper
+					 * nthreads limit.
+					 */
+					nthreads--;
+					thread--;
+
+					/* ... and pretend like nothing have happened. */
+					err = 0;
+					continue;
+				}
+
 				pr_debug2("\nsys_perf_event_open failed, error %d\n",
 					  err);
 				goto try_fallback;

tools/perf/util/evsel.h +1 -0

index 6abb89cd27f9..06ef6f29efa1 100644
@@ -120,6 +120,7 @@ struct perf_evsel {
 	bool			tracking;
 	bool			per_pkg;
 	bool			precise_max;
+	bool			ignore_missing_thread;
 	/* parse modifier helper */
 	int			exclude_GH;
 	int			nr_members;

Signature · 1 line

2.7.4

Em Mon, Dec 12, 2016 at 11:35:42AM +0100, Jiri Olsa escreveu:

Quoted 8 lines

Adding perf_evsel::ignore_missing_cpu_thread bool.

When set true, it allows perf to ignore error of missing pid of perf event syscall.

We remove missing thread id from the thread_map, so the rest of the processing like ioctl and mmap won't get disturbed with -1 fd

What was the test performed? I tried it here the following while doing a 'make -j4 allmodconfig' in the background, i.e. tons of threads coming and going, and I sometimes get:

[root@jouet ~]# perf record -u acme
Error:
The sys_perf_event_open() syscall returned with 3 (No such process) for event (cycles:ppp).
/bin/dmesg may provide additional information.
No CONFIG_PERF_EVENTS=y kernel support configured? [root@jouet ~]#

Quoted 117 lines

The reason for supporting this is to ease up monitoring group of pids, that 'disappear' before perf opens their event. This currently leads perf to report error and exit and makes perf record's -u option unusable under certain setup.

With this change we will allow this race and ignore such failure with following warning:

WARNING: Ignored open failure for pid 8605

Signed-off-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

---
 tools/perf/perf.h       |  1 +
 tools/perf/util/evsel.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/evsel.h |  1 +
 3 files changed, 46 insertions(+)

tools/perf/perf.h +1 -0

index 9a0236a4cf95..1c27d947c2fe 100644
@@ -55,6 +55,7 @@ struct record_opts {
 	bool	     all_user;
 	bool	     tail_synthesize;
 	bool	     overwrite;
+	bool	     ignore_missing_thread;
 	unsigned int freq;
 	unsigned int mmap_pages;
 	unsigned int auxtrace_mmap_pages;

tools/perf/util/evsel.c +44 -0

index fd61ebd77c26..ac87f1637ae9 100644
@@ -990,6 +990,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
 	 * it overloads any global configuration.
 	 */
 	apply_config_terms(evsel, opts);
+
+	evsel->ignore_missing_thread = opts->ignore_missing_thread;
 }
 
 static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)

@@ -1419,6 +1421,33 @@ static int __open_attr__fprintf(FILE *fp, const char *name, const char *val,
 	return fprintf(fp, "  %-32s %s\n", name, val);
 }
 
+static bool ignore_missing_thread(struct perf_evsel *evsel,
+				  struct thread_map *threads,
+				  int thread, int err)
+{
+	if (!evsel->ignore_missing_thread)
+		return false;
+
+	/* The system wide setup does not work with threads. */
+	if (!evsel->system_wide)
+		return false;
+
+	/* The -ESRCH is perf event syscall errno for pid's not found. */
+	if (err != -ESRCH)
+		return false;
+
+	/* If there's only one thread, let it fail. */
+	if (threads->nr == 1)
+		return false;
+
+	if (thread_map__remove(threads, thread))
+		return false;
+
+	pr_warning("WARNING: Ignored open failure for pid %d\n",
+		   thread_map__pid(threads, thread));
+	return true;
+}
+
 static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 			      struct thread_map *threads)
 {

@@ -1491,6 +1520,21 @@ static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 
 			if (fd < 0) {
 				err = -errno;
+
+				if (ignore_missing_thread(evsel, threads, thread, err)) {
+					/*
+					 * We just removed 1 thread, so take a step
+					 * back on thread index and lower the upper
+					 * nthreads limit.
+					 */
+					nthreads--;
+					thread--;
+
+					/* ... and pretend like nothing have happened. */
+					err = 0;
+					continue;
+				}
+
 				pr_debug2("\nsys_perf_event_open failed, error %d\n",
 					  err);
 				goto try_fallback;

tools/perf/util/evsel.h +1 -0

index 6abb89cd27f9..06ef6f29efa1 100644
@@ -120,6 +120,7 @@ struct perf_evsel {
 	bool			tracking;
 	bool			per_pkg;
 	bool			precise_max;
+	bool			ignore_missing_thread;
 	/* parse modifier helper */
 	int			exclude_GH;
 	int			nr_members;

Signature · 1 line

2.7.4

Em Mon, Dec 12, 2016 at 11:35:42AM +0100, Jiri Olsa escreveu:

Quoted 8 lines

Adding perf_evsel::ignore_missing_cpu_thread bool.

When set true, it allows perf to ignore error of missing pid of perf event syscall.

We remove missing thread id from the thread_map, so the rest of the processing like ioctl and mmap won't get disturbed with -1 fd

What was the test performed? I tried it here the following while doing a 'make -j4 allmodconfig' in the background, i.e. tons of threads coming and going, and I sometimes get:

[root@jouet ~]# perf record -u acme
Error:
The sys_perf_event_open() syscall returned with 3 (No such process) for event (cycles:ppp).
/bin/dmesg may provide additional information.
No CONFIG_PERF_EVENTS=y kernel support configured?

Quoted 14 lines

Em Mon, Dec 12, 2016 at 11:35:42AM +0100, Jiri Olsa escreveu:

Quoted 8 lines

Adding perf_evsel::ignore_missing_cpu_thread bool.

When set true, it allows perf to ignore error of missing pid of perf event syscall.

We remove missing thread id from the thread_map, so the rest of the processing like ioctl and mmap won't get disturbed with -1 fd

What was the test performed? I tried it here the following while doing a 'make -j4 allmodconfig' in the background, i.e. tons of threads coming and going, and I sometimes get:

Quoted 6 lines

[root@jouet ~]# perf record -u acme
Error:
The sys_perf_event_open() syscall returned with 3 (No such process) for event (cycles:ppp).
/bin/dmesg may provide additional information.
No CONFIG_PERF_EVENTS=y kernel support configured?

I dont think it's fast enough (start/stop) to hit the race, I've got best results with having on background this:

while :; do /usr/bin/true; done

Em Mon, Dec 12, 2016 at 03:53:12PM +0100, Jiri Olsa escreveu:

Quoted 22 lines · Arnaldo Carvalho de Melo

Quoted 20 lines

Em Mon, Dec 12, 2016 at 11:35:42AM +0100, Jiri Olsa escreveu:

Quoted 8 lines

Adding perf_evsel::ignore_missing_cpu_thread bool.

When set true, it allows perf to ignore error of missing pid of perf event syscall.

We remove missing thread id from the thread_map, so the rest of the processing like ioctl and mmap won't get disturbed with -1 fd

What was the test performed? I tried it here the following while doing a 'make -j4 allmodconfig' in the background, i.e. tons of threads coming and going, and I sometimes get:

[root@jouet ~]# perf record -u acme
Error:
The sys_perf_event_open() syscall returned with 3 (No such process) for event (cycles:ppp).
/bin/dmesg may provide additional information.
No CONFIG_PERF_EVENTS=y kernel support configured?

Quoted 4 lines

I dont think it's fast enough (start/stop) to hit the race, I've got best results with having on background this:

while :; do /usr/bin/true; done

Ok, but the focus of my message wasn't on generating threads, was in having the above happen with your patch applied.

Quoted 26 lines

Em Mon, Dec 12, 2016 at 03:53:12PM +0100, Jiri Olsa escreveu:

Quoted 25 lines · Arnaldo Carvalho de Melo

Quoted 14 lines

Em Mon, Dec 12, 2016 at 11:35:42AM +0100, Jiri Olsa escreveu:

Quoted 8 lines

Adding perf_evsel::ignore_missing_cpu_thread bool.

When set true, it allows perf to ignore error of missing pid of perf event syscall.

We remove missing thread id from the thread_map, so the rest of the processing like ioctl and mmap won't get disturbed with -1 fd

What was the test performed? I tried it here the following while doing a 'make -j4 allmodconfig' in the background, i.e. tons of threads coming and going, and I sometimes get:

Quoted 6 lines

[root@jouet ~]# perf record -u acme
Error:
The sys_perf_event_open() syscall returned with 3 (No such process) for event (cycles:ppp).
/bin/dmesg may provide additional information.
No CONFIG_PERF_EVENTS=y kernel support configured?

I dont think it's fast enough (start/stop) to hit the race, I've got best results with having on background this:

Quoted 4 lines

Quoted 1 line

while :; do /usr/bin/true; done

Ok, but the focus of my message wasn't on generating threads, was in having the above happen with your patch applied.

hum, actualy the patch 5 enables that for -u option.. this patch just adds the logic

Adding perf_evsel::ignore_missing_cpu_thread bool.

When set true, it allows perf to ignore error of missing pid of perf event syscall.

We remove missing thread id from the thread_map, so the rest of the processing like ioctl and mmap won't get disturbed with -1 fd.

The reason for supporting this is to ease up monitoring group of pids, that 'disappear' before perf opens their event. This currently leads perf to report error and exit and makes perf record's -u option unusable under certain setup.

With this change we will allow this race and ignore such failure with following warning:

WARNING: Ignored open failure for pid 8605

Signed-off-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

---
 tools/perf/perf.h       |  1 +
 tools/perf/util/evsel.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/evsel.h |  1 +
 3 files changed, 46 insertions(+)

tools/perf/perf.h +1 -0

index 9a0236a4cf95..1c27d947c2fe 100644
@@ -55,6 +55,7 @@ struct record_opts {
 	bool	     all_user;
 	bool	     tail_synthesize;
 	bool	     overwrite;
+	bool	     ignore_missing_thread;
 	unsigned int freq;
 	unsigned int mmap_pages;
 	unsigned int auxtrace_mmap_pages;

tools/perf/util/evsel.c +2 -0

index fd61ebd77c26..ac87f1637ae9 100644
@@ -990,6 +990,8 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
 	 * it overloads any global configuration.
 	 */
 	apply_config_terms(evsel, opts);
+
+	evsel->ignore_missing_thread = opts->ignore_missing_thread;
 }
 
 static int perf_evsel__alloc_fd(struct perf_evsel *evsel, int ncpus, int nthreads)

@@ -1419,6 +1421,33 @@ static int __open_attr__fprintf(FILE *fp, const char *name, const char *val,
 	return fprintf(fp, "  %-32s %s\n", name, val);
 }
 

tools/perf/util/evsel.c +32 -0

+
+	/* The -ESRCH is perf event syscall errno for pid's not found. */
+	if (err != -ESRCH)
+		return false;
+
+	/* If there's only one thread, let it fail. */
+	if (threads->nr == 1)
+		return false;
+
+	if (thread_map__remove(threads, thread))
+		return false;
+
+	pr_warning("WARNING: Ignored open failure for pid %d\n",
+		   thread_map__pid(threads, thread));
+	return true;
+}
+
 static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 			      struct thread_map *threads)
 {

@@ -1491,6 +1520,21 @@ static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 
 			if (fd < 0) {
 				err = -errno;
+
+				if (ignore_missing_thread(evsel, threads, thread, err)) {
+					/*
+					 * We just removed 1 thread, so take a step
+					 * back on thread index and lower the upper
+					 * nthreads limit.
+					 */
+					nthreads--;
+					thread--;
+
+					/* ... and pretend like nothing have happened. */
+					err = 0;
+					continue;
+				}
+
 				pr_debug2("\nsys_perf_event_open failed, error %d\n",
 					  err);
 				goto try_fallback;

tools/perf/util/evsel.h +1 -0

index 6abb89cd27f9..06ef6f29efa1 100644
@@ -120,6 +120,7 @@ struct perf_evsel {
 	bool			tracking;
 	bool			per_pkg;
 	bool			precise_max;
+	bool			ignore_missing_thread;
 	/* parse modifier helper */
 	int			exclude_GH;
 	int			nr_members;

Signature · 1 line

2.7.4

Review

Quoted 12 lines

}

(quoted diff) +10 -0

+static bool ignore_missing_thread(struct perf_evsel *evsel,
+				  struct thread_map *threads,
+				  int thread, int err)
+{
+	if (!evsel->ignore_missing_thread)
+		return false;
+
+	/* The system wide setup does not work with threads. */
+	if (!evsel->system_wide)
+		return false;

I'm bit confused with this. Shouldn't the condition be positive?