[PATCH 1/5] perf test: Add -F/--dont-fork option

[PATCH 1/5] perf test: Add -F/--dont-fork option

[Jiri Olsa]

Adding -F/--dont-fork option to bypass forking
for each test. It's useful for debugging test.

Link: http://lkml.kernel.org/n/tip-yq9gy0fcr8nl70986gwnl3dh@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/Documentation/perf-test.txt |  4 +++
 tools/perf/tests/builtin-test.c        | 55 ++++++++++++++++++++--------------
 2 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/tools/perf/Documentation/perf-test.txt b/tools/perf/Documentation/perf-test.txt
index 31a5c3ea7f74..b329c65d7f40 100644
--- a/tools/perf/Documentation/perf-test.txt
+++ b/tools/perf/Documentation/perf-test.txt
@@ -30,3 +30,7 @@ OPTIONS
 -v::
 --verbose::
 	Be more verbose.
+
+-F::
+--dont-fork::
+	Do not fork child for each test, run all tests within single process.
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 0e95c20ecf6e..5781c1640eae 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -14,6 +14,8 @@
 #include <subcmd/parse-options.h>
 #include "symbol.h"
 
+static bool dont_fork;
+
 struct test __weak arch_tests[] = {
 	{
 		.func = NULL,
@@ -247,7 +249,7 @@ static bool perf_test__matches(struct test *test, int curr, int argc, const char
 
 static int run_test(struct test *test, int subtest)
 {
-	int status, err = -1, child = fork();
+	int status, err = -1, child = dont_fork ? 0 : fork();
 	char sbuf[STRERR_BUFSIZE];
 
 	if (child < 0) {
@@ -257,34 +259,41 @@ static int run_test(struct test *test, int subtest)
 	}
 
 	if (!child) {
-		pr_debug("test child forked, pid %d\n", getpid());
-		if (!verbose) {
-			int nullfd = open("/dev/null", O_WRONLY);
-			if (nullfd >= 0) {
-				close(STDERR_FILENO);
-				close(STDOUT_FILENO);
-
-				dup2(nullfd, STDOUT_FILENO);
-				dup2(STDOUT_FILENO, STDERR_FILENO);
-				close(nullfd);
+		if (!dont_fork) {
+			pr_debug("test child forked, pid %d\n", getpid());
+
+			if (!verbose) {
+				int nullfd = open("/dev/null", O_WRONLY);
+
+				if (nullfd >= 0) {
+					close(STDERR_FILENO);
+					close(STDOUT_FILENO);
+
+					dup2(nullfd, STDOUT_FILENO);
+					dup2(STDOUT_FILENO, STDERR_FILENO);
+					close(nullfd);
+				}
+			} else {
+				signal(SIGSEGV, sighandler_dump_stack);
+				signal(SIGFPE, sighandler_dump_stack);
 			}
-		} else {
-			signal(SIGSEGV, sighandler_dump_stack);
-			signal(SIGFPE, sighandler_dump_stack);
 		}
 
 		err = test->func(subtest);
-		exit(err);
+		if (!dont_fork)
+			exit(err);
 	}
 
-	wait(&status);
+	if (!dont_fork) {
+		wait(&status);
 
-	if (WIFEXITED(status)) {
-		err = (signed char)WEXITSTATUS(status);
-		pr_debug("test child finished with %d\n", err);
-	} else if (WIFSIGNALED(status)) {
-		err = -1;
-		pr_debug("test child interrupted\n");
+		if (WIFEXITED(status)) {
+			err = (signed char)WEXITSTATUS(status);
+			pr_debug("test child finished with %d\n", err);
+		} else if (WIFSIGNALED(status)) {
+			err = -1;
+			pr_debug("test child interrupted\n");
+		}
 	}
 
 	return err;
@@ -425,6 +434,8 @@ int cmd_test(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_STRING('s', "skip", &skip, "tests", "tests to skip"),
 	OPT_INCR('v', "verbose", &verbose,
 		    "be more verbose (show symbol address, etc)"),
+	OPT_BOOLEAN('F', "dont-fork", &dont_fork,
+		    "Do not fork for testcase"),
 	OPT_END()
 	};
 	const char * const test_subcommands[] = { "list", NULL };
-- 
2.4.11

[PATCH 2/5] perf tools: Allow to reset open files counter

[Jiri Olsa]

I hit a bug when running test suite without forking
each test (-F option):

  $ perf test -F dso
   8: Test dso data read                                       : Ok
   9: Test dso data cache                                      : FAILED!
  10: Test dso data reopen                                     : FAILED!

The reason the session file limit is set just once for
perf process so we need to reset it for each test,
otherwise wrong limit is taken into account.

Link: http://lkml.kernel.org/n/tip-bqapag0ljaiwmb7hlkw09zk5@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/dso-data.c |  6 ++++++
 tools/perf/util/dso.c       | 22 ++++++++++++++++------
 tools/perf/util/dso.h       |  2 ++
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index 8cf0d9e189a8..13725e09ba22 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -251,6 +251,9 @@ int test__dso_data_cache(int subtest __maybe_unused)
 	long nr_end, nr = open_files_cnt();
 	int dso_cnt, limit, i, fd;
 
+	/* Rest the internal dso open counter limit. */
+	reset_fd_limit();
+
 	memset(&machine, 0, sizeof(machine));
 
 	/* set as system limit */
@@ -312,6 +315,9 @@ int test__dso_data_reopen(int subtest __maybe_unused)
 #define dso_1 (dsos[1])
 #define dso_2 (dsos[2])
 
+	/* Rest the internal dso open counter limit. */
+	reset_fd_limit();
+
 	memset(&machine, 0, sizeof(machine));
 
 	/*
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 5d286f5d7906..e1de6cc4863e 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -442,17 +442,27 @@ static rlim_t get_fd_limit(void)
 	return limit;
 }
 
-static bool may_cache_fd(void)
+static rlim_t fd_limit;
+
+/*
+ * Used only by tests/dso-data.c to reset the environment
+ * for tests. I dont expect we should change this during
+ * standard runtime.
+ */
+void reset_fd_limit(void)
 {
-	static rlim_t limit;
+	fd_limit = 0;
+}
 
-	if (!limit)
-		limit = get_fd_limit();
+static bool may_cache_fd(void)
+{
+	if (!fd_limit)
+		fd_limit = get_fd_limit();
 
-	if (limit == RLIM_INFINITY)
+	if (fd_limit == RLIM_INFINITY)
 		return true;
 
-	return limit > (rlim_t) dso__data_open_cnt;
+	return fd_limit > (rlim_t) dso__data_open_cnt;
 }
 
 /*
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 76d79d070e21..a571f24895ca 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -360,4 +360,6 @@ enum dso_type dso__type(struct dso *dso, struct machine *machine);
 
 int dso__strerror_load(struct dso *dso, char *buf, size_t buflen);
 
+void reset_fd_limit(void);
+
 #endif /* __PERF_DSO */
-- 
2.4.11

Re: [PATCH 2/5] perf tools: Allow to reset open files counter

[Nilay Vaish]

On 28 June 2016 at 06:29, Jiri Olsa <jolsa@kernel.org> wrote:
> I hit a bug when running test suite without forking
> each test (-F option):
>
>   $ perf test -F dso
>    8: Test dso data read                                       : Ok
>    9: Test dso data cache                                      : FAILED!
>   10: Test dso data reopen                                     : FAILED!
>
> The reason the session file limit is set just once for
> perf process so we need to reset it for each test,
> otherwise wrong limit is taken into account.
>
> Link: http://lkml.kernel.org/n/tip-bqapag0ljaiwmb7hlkw09zk5@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>


The patch seems fine to me.  I confirmed that we see those failed
tests without this patch and applying it fixes them.

--
Nilay

Re: [PATCH 2/5] perf tools: Allow to reset open files counter

[Nilay Vaish]

On 29 June 2016 at 11:05, Nilay Vaish <nilayvaish@gmail.com> wrote:
> On 28 June 2016 at 06:29, Jiri Olsa <jolsa@kernel.org> wrote:
>> I hit a bug when running test suite without forking
>> each test (-F option):
>>
>>   $ perf test -F dso
>>    8: Test dso data read                                       : Ok
>>    9: Test dso data cache                                      : FAILED!
>>   10: Test dso data reopen                                     : FAILED!
>>
>> The reason the session file limit is set just once for
>> perf process so we need to reset it for each test,
>> otherwise wrong limit is taken into account.
>>
>> Link: http://lkml.kernel.org/n/tip-bqapag0ljaiwmb7hlkw09zk5@git.kernel.org
>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>
>
> The patch seems fine to me.  I confirmed that we see those failed
> tests without this patch and applying it fixes them.
>

Tested-by: Nilay Vaish <nilayvaish@gmail.com>

[tip:perf/core] perf tools: Allow to reset open files counter

[tip-bot for Jiri Olsa]

Commit-ID:  f3069249e9e6b0ce303c3547dfa2960ee2e95b61
Gitweb:     http://git.kernel.org/tip/f3069249e9e6b0ce303c3547dfa2960ee2e95b61
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Tue, 28 Jun 2016 13:29:02 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 30 Jun 2016 18:27:44 -0300

perf tools: Allow to reset open files counter

I hit a bug when running test suite without forking
each test (-F option):

  $ perf test -F dso
   8: Test dso data read                                       : Ok
   9: Test dso data cache                                      : FAILED!
  10: Test dso data reopen                                     : FAILED!

The reason the session file limit is set just once for
perf process so we need to reset it for each test,
otherwise wrong limit is taken into account.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested-by: Nilay Vaish <nilayvaish@gmail.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1467113345-12669-2-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/dso-data.c |  6 ++++++
 tools/perf/util/dso.c       | 22 ++++++++++++++++------
 tools/perf/util/dso.h       |  2 ++
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index 8cf0d9e..13725e0 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -251,6 +251,9 @@ int test__dso_data_cache(int subtest __maybe_unused)
 	long nr_end, nr = open_files_cnt();
 	int dso_cnt, limit, i, fd;
 
+	/* Rest the internal dso open counter limit. */
+	reset_fd_limit();
+
 	memset(&machine, 0, sizeof(machine));
 
 	/* set as system limit */
@@ -312,6 +315,9 @@ int test__dso_data_reopen(int subtest __maybe_unused)
 #define dso_1 (dsos[1])
 #define dso_2 (dsos[2])
 
+	/* Rest the internal dso open counter limit. */
+	reset_fd_limit();
+
 	memset(&machine, 0, sizeof(machine));
 
 	/*
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 5d286f5..e1de6cc 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -442,17 +442,27 @@ static rlim_t get_fd_limit(void)
 	return limit;
 }
 
-static bool may_cache_fd(void)
+static rlim_t fd_limit;
+
+/*
+ * Used only by tests/dso-data.c to reset the environment
+ * for tests. I dont expect we should change this during
+ * standard runtime.
+ */
+void reset_fd_limit(void)
 {
-	static rlim_t limit;
+	fd_limit = 0;
+}
 
-	if (!limit)
-		limit = get_fd_limit();
+static bool may_cache_fd(void)
+{
+	if (!fd_limit)
+		fd_limit = get_fd_limit();
 
-	if (limit == RLIM_INFINITY)
+	if (fd_limit == RLIM_INFINITY)
 		return true;
 
-	return limit > (rlim_t) dso__data_open_cnt;
+	return fd_limit > (rlim_t) dso__data_open_cnt;
 }
 
 /*
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 76d79d0..a571f24 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -360,4 +360,6 @@ enum dso_type dso__type(struct dso *dso, struct machine *machine);
 
 int dso__strerror_load(struct dso *dso, char *buf, size_t buflen);
 
+void reset_fd_limit(void);
+
 #endif /* __PERF_DSO */

[PATCH 3/5] perf tests: Fix thread map test for -F option

[Jiri Olsa]

I hit a bug when running test suite without forking
each test (-F option):

  $ perf test -Fv
  ...
  34: Test thread map                                          :
  --- start ---
  FAILED tests/thread-map.c:24 wrong comm
  ---- end ----
  Test thread map: FAILED!

The reason was the process name wasn't 'perf' as expected
by the test, because other tests set the name as well.

Setting it explicitly now.

Link: http://lkml.kernel.org/n/tip-bqapag0ljaiwmb7hlkw09zk5@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/thread-map.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/thread-map.c b/tools/perf/tests/thread-map.c
index fccde848fe9c..cee2a2cdc933 100644
--- a/tools/perf/tests/thread-map.c
+++ b/tools/perf/tests/thread-map.c
@@ -1,13 +1,20 @@
 #include <sys/types.h>
 #include <unistd.h>
+#include <sys/prctl.h>
 #include "tests.h"
 #include "thread_map.h"
 #include "debug.h"
 
+#define NAME	(const char *) "perf"
+#define NAMEUL	(unsigned long) NAME
+
 int test__thread_map(int subtest __maybe_unused)
 {
 	struct thread_map *map;
 
+	TEST_ASSERT_VAL("failed to set process name",
+			!prctl(PR_SET_NAME, NAMEUL, 0, 0, 0));
+
 	/* test map on current pid */
 	map = thread_map__new_by_pid(getpid());
 	TEST_ASSERT_VAL("failed to alloc map", map);
@@ -19,7 +26,7 @@ int test__thread_map(int subtest __maybe_unused)
 			thread_map__pid(map, 0) == getpid());
 	TEST_ASSERT_VAL("wrong comm",
 			thread_map__comm(map, 0) &&
-			!strcmp(thread_map__comm(map, 0), "perf"));
+			!strcmp(thread_map__comm(map, 0), NAME));
 	TEST_ASSERT_VAL("wrong refcnt",
 			atomic_read(&map->refcnt) == 1);
 	thread_map__put(map);
@@ -51,7 +58,7 @@ static int process_event(struct perf_tool *tool __maybe_unused,
 
 	TEST_ASSERT_VAL("wrong nr",   map->nr == 1);
 	TEST_ASSERT_VAL("wrong pid",  map->entries[0].pid == (u64) getpid());
-	TEST_ASSERT_VAL("wrong comm", !strcmp(map->entries[0].comm, "perf"));
+	TEST_ASSERT_VAL("wrong comm", !strcmp(map->entries[0].comm, NAME));
 
 	threads = thread_map__new_event(&event->thread_map);
 	TEST_ASSERT_VAL("failed to alloc map", threads);
@@ -61,7 +68,7 @@ static int process_event(struct perf_tool *tool __maybe_unused,
 			thread_map__pid(threads, 0) == getpid());
 	TEST_ASSERT_VAL("wrong comm",
 			thread_map__comm(threads, 0) &&
-			!strcmp(thread_map__comm(threads, 0), "perf"));
+			!strcmp(thread_map__comm(threads, 0), NAME));
 	TEST_ASSERT_VAL("wrong refcnt",
 			atomic_read(&threads->refcnt) == 1);
 	thread_map__put(threads);
@@ -72,6 +79,9 @@ int test__thread_map_synthesize(int subtest __maybe_unused)
 {
 	struct thread_map *threads;
 
+	TEST_ASSERT_VAL("failed to set process name",
+			!prctl(PR_SET_NAME, NAMEUL, 0, 0, 0));
+
 	/* test map on current pid */
 	threads = thread_map__new_by_pid(getpid());
 	TEST_ASSERT_VAL("failed to alloc map", threads);
-- 
2.4.11

Re: [PATCH 3/5] perf tests: Fix thread map test for -F option

[Nilay Vaish]

On 28 June 2016 at 06:29, Jiri Olsa <jolsa@kernel.org> wrote:
> I hit a bug when running test suite without forking
> each test (-F option):
>
>   $ perf test -Fv
>   ...
>   34: Test thread map                                          :
>   --- start ---
>   FAILED tests/thread-map.c:24 wrong comm
>   ---- end ----
>   Test thread map: FAILED!
>
> The reason was the process name wasn't 'perf' as expected
> by the test, because other tests set the name as well.
>
> Setting it explicitly now.
>
> Link: http://lkml.kernel.org/n/tip-bqapag0ljaiwmb7hlkw09zk5@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>


This patch also seems fine.  Again, I confirmed that test 34 fails
without the patch and that this patch fixed it.

--
Nilay

Re: [PATCH 3/5] perf tests: Fix thread map test for -F option

[Nilay Vaish]

On 29 June 2016 at 11:06, Nilay Vaish <nilayvaish@gmail.com> wrote:
> On 28 June 2016 at 06:29, Jiri Olsa <jolsa@kernel.org> wrote:
>> I hit a bug when running test suite without forking
>> each test (-F option):
>>
>>   $ perf test -Fv
>>   ...
>>   34: Test thread map                                          :
>>   --- start ---
>>   FAILED tests/thread-map.c:24 wrong comm
>>   ---- end ----
>>   Test thread map: FAILED!
>>
>> The reason was the process name wasn't 'perf' as expected
>> by the test, because other tests set the name as well.
>>
>> Setting it explicitly now.
>>
>> Link: http://lkml.kernel.org/n/tip-bqapag0ljaiwmb7hlkw09zk5@git.kernel.org
>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>
>
> This patch also seems fine.  Again, I confirmed that test 34 fails
> without the patch and that this patch fixed it.
>

Tested-by: Nilay Vaish <nilayvaish@gmail.com>

[tip:perf/core] perf tests: Fix thread map test for -F option

[tip-bot for Jiri Olsa]

Commit-ID:  8fbc38aaaf542433ef195e5fae704c56d1c071e6
Gitweb:     http://git.kernel.org/tip/8fbc38aaaf542433ef195e5fae704c56d1c071e6
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Tue, 28 Jun 2016 13:29:03 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 30 Jun 2016 18:27:44 -0300

perf tests: Fix thread map test for -F option

I hit a bug when running test suite without forking each test (-F
option):

  $ perf test -Fv
  ...
  34: Test thread map                                          :
  --- start ---
  FAILED tests/thread-map.c:24 wrong comm
  ---- end ----
  Test thread map: FAILED!

The reason was the process name wasn't 'perf' as expected by the test,
because other tests set the name as well.

Setting it explicitly now.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested-by: Nilay Vaish <nilayvaish@gmail.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1467113345-12669-3-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/thread-map.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/thread-map.c b/tools/perf/tests/thread-map.c
index fccde84..cee2a2c 100644
--- a/tools/perf/tests/thread-map.c
+++ b/tools/perf/tests/thread-map.c
@@ -1,13 +1,20 @@
 #include <sys/types.h>
 #include <unistd.h>
+#include <sys/prctl.h>
 #include "tests.h"
 #include "thread_map.h"
 #include "debug.h"
 
+#define NAME	(const char *) "perf"
+#define NAMEUL	(unsigned long) NAME
+
 int test__thread_map(int subtest __maybe_unused)
 {
 	struct thread_map *map;
 
+	TEST_ASSERT_VAL("failed to set process name",
+			!prctl(PR_SET_NAME, NAMEUL, 0, 0, 0));
+
 	/* test map on current pid */
 	map = thread_map__new_by_pid(getpid());
 	TEST_ASSERT_VAL("failed to alloc map", map);
@@ -19,7 +26,7 @@ int test__thread_map(int subtest __maybe_unused)
 			thread_map__pid(map, 0) == getpid());
 	TEST_ASSERT_VAL("wrong comm",
 			thread_map__comm(map, 0) &&
-			!strcmp(thread_map__comm(map, 0), "perf"));
+			!strcmp(thread_map__comm(map, 0), NAME));
 	TEST_ASSERT_VAL("wrong refcnt",
 			atomic_read(&map->refcnt) == 1);
 	thread_map__put(map);
@@ -51,7 +58,7 @@ static int process_event(struct perf_tool *tool __maybe_unused,
 
 	TEST_ASSERT_VAL("wrong nr",   map->nr == 1);
 	TEST_ASSERT_VAL("wrong pid",  map->entries[0].pid == (u64) getpid());
-	TEST_ASSERT_VAL("wrong comm", !strcmp(map->entries[0].comm, "perf"));
+	TEST_ASSERT_VAL("wrong comm", !strcmp(map->entries[0].comm, NAME));
 
 	threads = thread_map__new_event(&event->thread_map);
 	TEST_ASSERT_VAL("failed to alloc map", threads);
@@ -61,7 +68,7 @@ static int process_event(struct perf_tool *tool __maybe_unused,
 			thread_map__pid(threads, 0) == getpid());
 	TEST_ASSERT_VAL("wrong comm",
 			thread_map__comm(threads, 0) &&
-			!strcmp(thread_map__comm(threads, 0), "perf"));
+			!strcmp(thread_map__comm(threads, 0), NAME));
 	TEST_ASSERT_VAL("wrong refcnt",
 			atomic_read(&threads->refcnt) == 1);
 	thread_map__put(threads);
@@ -72,6 +79,9 @@ int test__thread_map_synthesize(int subtest __maybe_unused)
 {
 	struct thread_map *threads;
 
+	TEST_ASSERT_VAL("failed to set process name",
+			!prctl(PR_SET_NAME, NAMEUL, 0, 0, 0));
+
 	/* test map on current pid */
 	threads = thread_map__new_by_pid(getpid());
 	TEST_ASSERT_VAL("failed to alloc map", threads);

[PATCH 4/5] perf tools: Change cpu_map__fprintf output

[Jiri Olsa]

Display cpu map in standard list form.
(perf report -D output on perf stat data).

before:
  0x590 [0x18]: PERF_RECORD_CPU_MAP nr: 4 cpus: 0, 1, 2, 3

after:
  0x590 [0x18]: PERF_RECORD_CPU_MAP: 0-3

Adding automated testcase.

Link: http://lkml.kernel.org/n/tip-zwt9oh6xqts2yrb2qk5ehy3c@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/tests/builtin-test.c |  4 +++
 tools/perf/tests/cpumap.c       | 24 ++++++++++++++++++
 tools/perf/tests/tests.h        |  1 +
 tools/perf/util/cpumap.c        | 54 ++++++++++++++++++++++++++++++++++++-----
 tools/perf/util/cpumap.h        |  1 +
 tools/perf/util/event.c         |  2 +-
 6 files changed, 79 insertions(+), 7 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 5781c1640eae..07c14e9f6546 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -214,6 +214,10 @@ static struct test generic_tests[] = {
 		.func = test__backward_ring_buffer,
 	},
 	{
+		.desc = "Test cpu map print",
+		.func = test__cpu_map_print,
+	},
+	{
 		.func = NULL,
 	},
 };
diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
index 4cb6418a8ffc..c9ec5f83e42c 100644
--- a/tools/perf/tests/cpumap.c
+++ b/tools/perf/tests/cpumap.c
@@ -86,3 +86,27 @@ int test__cpu_map_synthesize(int subtest __maybe_unused)
 	cpu_map__put(cpus);
 	return 0;
 }
+
+static int cpu_map_print(const char *str)
+{
+	struct cpu_map *map = cpu_map__new(str);
+	char buf[100];
+
+	if (!map)
+		return -1;
+
+	cpu_map__snprint(map, buf, sizeof(buf));
+	return !strcmp(buf, str);
+}
+
+int test__cpu_map_print(int subtest __maybe_unused)
+{
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1"));
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1,5"));
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1,3,5,7,9,11,13,15,17,19,21-40"));
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("2-5"));
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1,3-6,8-10,24,35-37"));
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1,3-6,8-10,24,35-37"));
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1-10,12-20,22-30,32-40"));
+	return 0;
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index c57e72c826d2..52f969570c97 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -87,6 +87,7 @@ int test__synthesize_stat_round(int subtest);
 int test__event_update(int subtest);
 int test__event_times(int subtest);
 int test__backward_ring_buffer(int subtest);
+int test__cpu_map_print(int subtest);
 
 #if defined(__arm__) || defined(__aarch64__)
 #ifdef HAVE_DWARF_UNWIND_SUPPORT
diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 02d801670f30..15f83acac1b8 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -236,13 +236,12 @@ struct cpu_map *cpu_map__new_data(struct cpu_map_data *data)
 
 size_t cpu_map__fprintf(struct cpu_map *map, FILE *fp)
 {
-	int i;
-	size_t printed = fprintf(fp, "%d cpu%s: ",
-				 map->nr, map->nr > 1 ? "s" : "");
-	for (i = 0; i < map->nr; ++i)
-		printed += fprintf(fp, "%s%d", i ? ", " : "", map->map[i]);
+#define BUFSIZE 1024
+	char buf[BUFSIZE];
 
-	return printed + fprintf(fp, "\n");
+	cpu_map__snprint(map, buf, sizeof(buf));
+	return fprintf(fp, "%s\n", buf);
+#undef BUFSIZE
 }
 
 struct cpu_map *cpu_map__dummy_new(void)
@@ -599,3 +598,46 @@ bool cpu_map__has(struct cpu_map *cpus, int cpu)
 
 	return false;
 }
+
+size_t cpu_map__snprint(struct cpu_map *map, char *buf, size_t size)
+{
+	int i, cpu, start = -1;
+	bool first = true;
+	size_t ret = 0;
+
+#define COMMA first ? "" : ","
+
+	for (i = 0; i < map->nr + 1; i++) {
+		bool last = i == map->nr;
+
+		cpu = last ? INT_MAX : map->map[i];
+
+		if (start == -1) {
+			start = i;
+			if (last) {
+				ret += snprintf(buf + ret, size - ret,
+						"%s%d", COMMA,
+						map->map[i]);
+			}
+		} else if (((i - start) != (cpu - map->map[start])) || last) {
+			int end = i - 1;
+
+			if (start == end) {
+				ret += snprintf(buf + ret, size - ret,
+						"%s%d", COMMA,
+						map->map[start]);
+			} else {
+				ret += snprintf(buf + ret, size - ret,
+						"%s%d-%d", COMMA,
+						map->map[start], map->map[end]);
+			}
+			first = false;
+			start = i;
+		}
+	}
+
+#undef COMMA
+
+	pr_debug("cpumask list: %s\n", buf);
+	return ret;
+}
diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
index 1a0a35073ce1..206dc550354a 100644
--- a/tools/perf/util/cpumap.h
+++ b/tools/perf/util/cpumap.h
@@ -19,6 +19,7 @@ struct cpu_map *cpu_map__empty_new(int nr);
 struct cpu_map *cpu_map__dummy_new(void);
 struct cpu_map *cpu_map__new_data(struct cpu_map_data *data);
 struct cpu_map *cpu_map__read(FILE *file);
+size_t cpu_map__snprint(struct cpu_map *map, char *buf, size_t size);
 size_t cpu_map__fprintf(struct cpu_map *map, FILE *fp);
 int cpu_map__get_socket_id(int cpu);
 int cpu_map__get_socket(struct cpu_map *map, int idx, void *data);
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 9b141f12329e..e20438b784be 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -1092,7 +1092,7 @@ size_t perf_event__fprintf_cpu_map(union perf_event *event, FILE *fp)
 	struct cpu_map *cpus = cpu_map__new_data(&event->cpu_map.data);
 	size_t ret;
 
-	ret = fprintf(fp, " nr: ");
+	ret = fprintf(fp, ": ");
 
 	if (cpus)
 		ret += cpu_map__fprintf(cpus, fp);
-- 
2.4.11

[tip:perf/core] perf tools: Change cpu_map__fprintf output

[tip-bot for Jiri Olsa]

Commit-ID:  a24020e6b7cf6eb8b75d8bca6b89870b1cee6ba7
Gitweb:     http://git.kernel.org/tip/a24020e6b7cf6eb8b75d8bca6b89870b1cee6ba7
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Tue, 28 Jun 2016 13:29:04 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 30 Jun 2016 18:27:45 -0300

perf tools: Change cpu_map__fprintf output

Display cpu map in standard list form.  (perf report -D output on perf stat data).

before:
  0x590 [0x18]: PERF_RECORD_CPU_MAP nr: 4 cpus: 0, 1, 2, 3

after:
  0x590 [0x18]: PERF_RECORD_CPU_MAP: 0-3

Adding automated testcase.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1467113345-12669-4-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/builtin-test.c |  4 +++
 tools/perf/tests/cpumap.c       | 24 ++++++++++++++++++
 tools/perf/tests/tests.h        |  1 +
 tools/perf/util/cpumap.c        | 54 ++++++++++++++++++++++++++++++++++++-----
 tools/perf/util/cpumap.h        |  1 +
 tools/perf/util/event.c         |  2 +-
 6 files changed, 79 insertions(+), 7 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 5781c16..07c14e9 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -214,6 +214,10 @@ static struct test generic_tests[] = {
 		.func = test__backward_ring_buffer,
 	},
 	{
+		.desc = "Test cpu map print",
+		.func = test__cpu_map_print,
+	},
+	{
 		.func = NULL,
 	},
 };
diff --git a/tools/perf/tests/cpumap.c b/tools/perf/tests/cpumap.c
index 4cb6418..c9ec5f8 100644
--- a/tools/perf/tests/cpumap.c
+++ b/tools/perf/tests/cpumap.c
@@ -86,3 +86,27 @@ int test__cpu_map_synthesize(int subtest __maybe_unused)
 	cpu_map__put(cpus);
 	return 0;
 }
+
+static int cpu_map_print(const char *str)
+{
+	struct cpu_map *map = cpu_map__new(str);
+	char buf[100];
+
+	if (!map)
+		return -1;
+
+	cpu_map__snprint(map, buf, sizeof(buf));
+	return !strcmp(buf, str);
+}
+
+int test__cpu_map_print(int subtest __maybe_unused)
+{
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1"));
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1,5"));
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1,3,5,7,9,11,13,15,17,19,21-40"));
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("2-5"));
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1,3-6,8-10,24,35-37"));
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1,3-6,8-10,24,35-37"));
+	TEST_ASSERT_VAL("failed to convert map", cpu_map_print("1-10,12-20,22-30,32-40"));
+	return 0;
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index c57e72c..52f9695 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -87,6 +87,7 @@ int test__synthesize_stat_round(int subtest);
 int test__event_update(int subtest);
 int test__event_times(int subtest);
 int test__backward_ring_buffer(int subtest);
+int test__cpu_map_print(int subtest);
 
 #if defined(__arm__) || defined(__aarch64__)
 #ifdef HAVE_DWARF_UNWIND_SUPPORT
diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 02d8016..15f83ac 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -236,13 +236,12 @@ struct cpu_map *cpu_map__new_data(struct cpu_map_data *data)
 
 size_t cpu_map__fprintf(struct cpu_map *map, FILE *fp)
 {
-	int i;
-	size_t printed = fprintf(fp, "%d cpu%s: ",
-				 map->nr, map->nr > 1 ? "s" : "");
-	for (i = 0; i < map->nr; ++i)
-		printed += fprintf(fp, "%s%d", i ? ", " : "", map->map[i]);
+#define BUFSIZE 1024
+	char buf[BUFSIZE];
 
-	return printed + fprintf(fp, "\n");
+	cpu_map__snprint(map, buf, sizeof(buf));
+	return fprintf(fp, "%s\n", buf);
+#undef BUFSIZE
 }
 
 struct cpu_map *cpu_map__dummy_new(void)
@@ -599,3 +598,46 @@ bool cpu_map__has(struct cpu_map *cpus, int cpu)
 
 	return false;
 }
+
+size_t cpu_map__snprint(struct cpu_map *map, char *buf, size_t size)
+{
+	int i, cpu, start = -1;
+	bool first = true;
+	size_t ret = 0;
+
+#define COMMA first ? "" : ","
+
+	for (i = 0; i < map->nr + 1; i++) {
+		bool last = i == map->nr;
+
+		cpu = last ? INT_MAX : map->map[i];
+
+		if (start == -1) {
+			start = i;
+			if (last) {
+				ret += snprintf(buf + ret, size - ret,
+						"%s%d", COMMA,
+						map->map[i]);
+			}
+		} else if (((i - start) != (cpu - map->map[start])) || last) {
+			int end = i - 1;
+
+			if (start == end) {
+				ret += snprintf(buf + ret, size - ret,
+						"%s%d", COMMA,
+						map->map[start]);
+			} else {
+				ret += snprintf(buf + ret, size - ret,
+						"%s%d-%d", COMMA,
+						map->map[start], map->map[end]);
+			}
+			first = false;
+			start = i;
+		}
+	}
+
+#undef COMMA
+
+	pr_debug("cpumask list: %s\n", buf);
+	return ret;
+}
diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
index 1a0a350..206dc55 100644
--- a/tools/perf/util/cpumap.h
+++ b/tools/perf/util/cpumap.h
@@ -19,6 +19,7 @@ struct cpu_map *cpu_map__empty_new(int nr);
 struct cpu_map *cpu_map__dummy_new(void);
 struct cpu_map *cpu_map__new_data(struct cpu_map_data *data);
 struct cpu_map *cpu_map__read(FILE *file);
+size_t cpu_map__snprint(struct cpu_map *map, char *buf, size_t size);
 size_t cpu_map__fprintf(struct cpu_map *map, FILE *fp);
 int cpu_map__get_socket_id(int cpu);
 int cpu_map__get_socket(struct cpu_map *map, int idx, void *data);
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 9b141f1..e20438b 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -1092,7 +1092,7 @@ size_t perf_event__fprintf_cpu_map(union perf_event *event, FILE *fp)
 	struct cpu_map *cpus = cpu_map__new_data(&event->cpu_map.data);
 	size_t ret;
 
-	ret = fprintf(fp, " nr: ");
+	ret = fprintf(fp, ": ");
 
 	if (cpus)
 		ret += cpu_map__fprintf(cpus, fp);

[PATCH 5/5] perf tools: Transform nodes string info to struct

[Jiri Olsa]

Storing NUMA info within struct numa_node instead
of strings. This way it's usable in future patches.

Also it turned out it's slightly less code involved
than using strings.

Link: http://lkml.kernel.org/n/tip-ka37sax3gfaxwvytfxi0ycy1@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/env.c    |  5 +++-
 tools/perf/util/env.h    | 12 ++++++--
 tools/perf/util/header.c | 78 +++++++++++++++++-------------------------------
 3 files changed, 41 insertions(+), 54 deletions(-)

diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index 49a11d9d8b8f..153985977475 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -18,10 +18,13 @@ void perf_env__exit(struct perf_env *env)
 	zfree(&env->cmdline_argv);
 	zfree(&env->sibling_cores);
 	zfree(&env->sibling_threads);
-	zfree(&env->numa_nodes);
 	zfree(&env->pmu_mappings);
 	zfree(&env->cpu);
 
+	for (i = 0; i < env->numa_nodes_cnt; i++)
+		cpu_map__put(env->numa_nodes[i].map);
+	zfree(&env->numa_nodes);
+
 	for (i = 0; i < env->caches_cnt; i++)
 		cpu_cache_level__free(&env->caches[i]);
 	zfree(&env->caches);
diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
index 56cffb60a0b4..027d0f4e6bff 100644
--- a/tools/perf/util/env.h
+++ b/tools/perf/util/env.h
@@ -2,6 +2,7 @@
 #define __PERF_ENV_H
 
 #include <linux/types.h>
+#include "cpumap.h"
 
 struct cpu_topology_map {
 	int	socket_id;
@@ -18,6 +19,13 @@ struct cpu_cache_level {
 	char	*map;
 };
 
+struct numa_node {
+	u32		 node;
+	u64		 mem_total;
+	u64		 mem_free;
+	struct cpu_map	*map;
+};
+
 struct perf_env {
 	char			*hostname;
 	char			*os_release;
@@ -33,18 +41,18 @@ struct perf_env {
 	int			nr_cmdline;
 	int			nr_sibling_cores;
 	int			nr_sibling_threads;
-	int			nr_numa_nodes;
 	int			nr_pmu_mappings;
 	int			nr_groups;
 	char			*cmdline;
 	const char		**cmdline_argv;
 	char			*sibling_cores;
 	char			*sibling_threads;
-	char			*numa_nodes;
 	char			*pmu_mappings;
 	struct cpu_topology_map	*cpu;
 	struct cpu_cache_level	*caches;
 	int			 caches_cnt;
+	struct numa_node	*numa_nodes;
+	int			 numa_nodes_cnt;
 };
 
 extern struct perf_env perf_env;
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index c5cd2698281f..af5f275462b3 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1306,42 +1306,19 @@ static void print_total_mem(struct perf_header *ph, int fd __maybe_unused,
 static void print_numa_topology(struct perf_header *ph, int fd __maybe_unused,
 				FILE *fp)
 {
-	u32 nr, c, i;
-	char *str, *tmp;
-	uint64_t mem_total, mem_free;
-
-	/* nr nodes */
-	nr = ph->env.nr_numa_nodes;
-	str = ph->env.numa_nodes;
-
-	for (i = 0; i < nr; i++) {
-		/* node number */
-		c = strtoul(str, &tmp, 0);
-		if (*tmp != ':')
-			goto error;
-
-		str = tmp + 1;
-		mem_total = strtoull(str, &tmp, 0);
-		if (*tmp != ':')
-			goto error;
+	int i;
+	struct numa_node *n;
 
-		str = tmp + 1;
-		mem_free = strtoull(str, &tmp, 0);
-		if (*tmp != ':')
-			goto error;
+	for (i = 0; i < ph->env.numa_nodes_cnt; i++) {
+		n = &ph->env.numa_nodes[i];
 
 		fprintf(fp, "# node%u meminfo  : total = %"PRIu64" kB,"
 			    " free = %"PRIu64" kB\n",
-			c, mem_total, mem_free);
+			n->node, n->mem_total, n->mem_free);
 
-		str = tmp + 1;
-		fprintf(fp, "# node%u cpu list : %s\n", c, str);
-
-		str += strlen(str) + 1;
+		fprintf(fp, "# node%u cpu list : ", n->node);
+		cpu_map__fprintf(n->map, fp);
 	}
-	return;
-error:
-	fprintf(fp, "# numa topology : not available\n");
 }
 
 static void print_cpuid(struct perf_header *ph, int fd __maybe_unused, FILE *fp)
@@ -1906,11 +1883,10 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
 				 struct perf_header *ph, int fd,
 				 void *data __maybe_unused)
 {
+	struct numa_node *nodes, *n;
 	ssize_t ret;
-	u32 nr, node, i;
+	u32 nr, i;
 	char *str;
-	uint64_t mem_total, mem_free;
-	struct strbuf sb;
 
 	/* nr nodes */
 	ret = readn(fd, &nr, sizeof(nr));
@@ -1920,48 +1896,48 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
 	if (ph->needs_swap)
 		nr = bswap_32(nr);
 
-	ph->env.nr_numa_nodes = nr;
-	if (strbuf_init(&sb, 256) < 0)
-		return -1;
+	ph->env.numa_nodes_cnt = nr;
+	nodes = zalloc(sizeof(*nodes) * nr);
+	if (!nodes)
+		return -ENOMEM;
 
 	for (i = 0; i < nr; i++) {
+		n = &nodes[i];
+
 		/* node number */
-		ret = readn(fd, &node, sizeof(node));
-		if (ret != sizeof(node))
+		ret = readn(fd, &n->node, sizeof(u32));
+		if (ret != sizeof(n->node))
 			goto error;
 
-		ret = readn(fd, &mem_total, sizeof(u64));
+		ret = readn(fd, &n->mem_total, sizeof(u64));
 		if (ret != sizeof(u64))
 			goto error;
 
-		ret = readn(fd, &mem_free, sizeof(u64));
+		ret = readn(fd, &n->mem_free, sizeof(u64));
 		if (ret != sizeof(u64))
 			goto error;
 
 		if (ph->needs_swap) {
-			node = bswap_32(node);
-			mem_total = bswap_64(mem_total);
-			mem_free = bswap_64(mem_free);
+			n->node      = bswap_32(n->node);
+			n->mem_total = bswap_64(n->mem_total);
+			n->mem_free  = bswap_64(n->mem_free);
 		}
 
-		if (strbuf_addf(&sb, "%u:%"PRIu64":%"PRIu64":",
-				node, mem_total, mem_free) < 0)
-			goto error;
-
 		str = do_read_string(fd, ph);
 		if (!str)
 			goto error;
 
-		/* include a NULL character at the end */
-		if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
+		n->map = cpu_map__new(str);
+		if (!n->map)
 			goto error;
+
 		free(str);
 	}
-	ph->env.numa_nodes = strbuf_detach(&sb, NULL);
+	ph->env.numa_nodes = nodes;
 	return 0;
 
 error:
-	strbuf_release(&sb);
+	free(nodes);
 	return -1;
 }
 
-- 
2.4.11

Re: [PATCH 5/5] perf tools: Transform nodes string info to struct

[Arnaldo Carvalho de Melo]

Em Tue, Jun 28, 2016 at 01:29:05PM +0200, Jiri Olsa escreveu:
> Storing NUMA info within struct numa_node instead
> of strings. This way it's usable in future patches.
> 
> Also it turned out it's slightly less code involved
> than using strings.
> 
> Link: http://lkml.kernel.org/n/tip-ka37sax3gfaxwvytfxi0ycy1@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/env.c    |  5 +++-
>  tools/perf/util/env.h    | 12 ++++++--
>  tools/perf/util/header.c | 78 +++++++++++++++++-------------------------------
>  3 files changed, 41 insertions(+), 54 deletions(-)
> 
> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index 49a11d9d8b8f..153985977475 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
> @@ -18,10 +18,13 @@ void perf_env__exit(struct perf_env *env)
>  	zfree(&env->cmdline_argv);
>  	zfree(&env->sibling_cores);
>  	zfree(&env->sibling_threads);
> -	zfree(&env->numa_nodes);
>  	zfree(&env->pmu_mappings);
>  	zfree(&env->cpu);
>  
> +	for (i = 0; i < env->numa_nodes_cnt; i++)
> +		cpu_map__put(env->numa_nodes[i].map);
> +	zfree(&env->numa_nodes);
> +
>  	for (i = 0; i < env->caches_cnt; i++)
>  		cpu_cache_level__free(&env->caches[i]);
>  	zfree(&env->caches);
> diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
> index 56cffb60a0b4..027d0f4e6bff 100644
> --- a/tools/perf/util/env.h
> +++ b/tools/perf/util/env.h
> @@ -2,6 +2,7 @@
>  #define __PERF_ENV_H
>  
>  #include <linux/types.h>
> +#include "cpumap.h"
>  
>  struct cpu_topology_map {
>  	int	socket_id;
> @@ -18,6 +19,13 @@ struct cpu_cache_level {
>  	char	*map;
>  };
>  
> +struct numa_node {
> +	u32		 node;
> +	u64		 mem_total;
> +	u64		 mem_free;
> +	struct cpu_map	*map;
> +};
> +
>  struct perf_env {
>  	char			*hostname;
>  	char			*os_release;
> @@ -33,18 +41,18 @@ struct perf_env {
>  	int			nr_cmdline;
>  	int			nr_sibling_cores;
>  	int			nr_sibling_threads;
> -	int			nr_numa_nodes;

why rename it from nr_numa_nodes to numa_nodes_cnt? Seems gratuitous and
potentially introduces up to three 4 byte holes into 'struct perf_env'
:-\

Applied the other patches in this series,

Thanks,

- Arnaldo

>  	int			nr_pmu_mappings;
>  	int			nr_groups;
>  	char			*cmdline;
>  	const char		**cmdline_argv;
>  	char			*sibling_cores;
>  	char			*sibling_threads;
> -	char			*numa_nodes;
>  	char			*pmu_mappings;
>  	struct cpu_topology_map	*cpu;
>  	struct cpu_cache_level	*caches;
>  	int			 caches_cnt;
> +	struct numa_node	*numa_nodes;
> +	int			 numa_nodes_cnt;
>  };
>  
>  extern struct perf_env perf_env;
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index c5cd2698281f..af5f275462b3 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1306,42 +1306,19 @@ static void print_total_mem(struct perf_header *ph, int fd __maybe_unused,
>  static void print_numa_topology(struct perf_header *ph, int fd __maybe_unused,
>  				FILE *fp)
>  {
> -	u32 nr, c, i;
> -	char *str, *tmp;
> -	uint64_t mem_total, mem_free;
> -
> -	/* nr nodes */
> -	nr = ph->env.nr_numa_nodes;
> -	str = ph->env.numa_nodes;
> -
> -	for (i = 0; i < nr; i++) {
> -		/* node number */
> -		c = strtoul(str, &tmp, 0);
> -		if (*tmp != ':')
> -			goto error;
> -
> -		str = tmp + 1;
> -		mem_total = strtoull(str, &tmp, 0);
> -		if (*tmp != ':')
> -			goto error;
> +	int i;
> +	struct numa_node *n;
>  
> -		str = tmp + 1;
> -		mem_free = strtoull(str, &tmp, 0);
> -		if (*tmp != ':')
> -			goto error;
> +	for (i = 0; i < ph->env.numa_nodes_cnt; i++) {
> +		n = &ph->env.numa_nodes[i];
>  
>  		fprintf(fp, "# node%u meminfo  : total = %"PRIu64" kB,"
>  			    " free = %"PRIu64" kB\n",
> -			c, mem_total, mem_free);
> +			n->node, n->mem_total, n->mem_free);
>  
> -		str = tmp + 1;
> -		fprintf(fp, "# node%u cpu list : %s\n", c, str);
> -
> -		str += strlen(str) + 1;
> +		fprintf(fp, "# node%u cpu list : ", n->node);
> +		cpu_map__fprintf(n->map, fp);
>  	}
> -	return;
> -error:
> -	fprintf(fp, "# numa topology : not available\n");
>  }
>  
>  static void print_cpuid(struct perf_header *ph, int fd __maybe_unused, FILE *fp)
> @@ -1906,11 +1883,10 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
>  				 struct perf_header *ph, int fd,
>  				 void *data __maybe_unused)
>  {
> +	struct numa_node *nodes, *n;
>  	ssize_t ret;
> -	u32 nr, node, i;
> +	u32 nr, i;
>  	char *str;
> -	uint64_t mem_total, mem_free;
> -	struct strbuf sb;
>  
>  	/* nr nodes */
>  	ret = readn(fd, &nr, sizeof(nr));
> @@ -1920,48 +1896,48 @@ static int process_numa_topology(struct perf_file_section *section __maybe_unuse
>  	if (ph->needs_swap)
>  		nr = bswap_32(nr);
>  
> -	ph->env.nr_numa_nodes = nr;
> -	if (strbuf_init(&sb, 256) < 0)
> -		return -1;
> +	ph->env.numa_nodes_cnt = nr;
> +	nodes = zalloc(sizeof(*nodes) * nr);
> +	if (!nodes)
> +		return -ENOMEM;
>  
>  	for (i = 0; i < nr; i++) {
> +		n = &nodes[i];
> +
>  		/* node number */
> -		ret = readn(fd, &node, sizeof(node));
> -		if (ret != sizeof(node))
> +		ret = readn(fd, &n->node, sizeof(u32));
> +		if (ret != sizeof(n->node))
>  			goto error;
>  
> -		ret = readn(fd, &mem_total, sizeof(u64));
> +		ret = readn(fd, &n->mem_total, sizeof(u64));
>  		if (ret != sizeof(u64))
>  			goto error;
>  
> -		ret = readn(fd, &mem_free, sizeof(u64));
> +		ret = readn(fd, &n->mem_free, sizeof(u64));
>  		if (ret != sizeof(u64))
>  			goto error;
>  
>  		if (ph->needs_swap) {
> -			node = bswap_32(node);
> -			mem_total = bswap_64(mem_total);
> -			mem_free = bswap_64(mem_free);
> +			n->node      = bswap_32(n->node);
> +			n->mem_total = bswap_64(n->mem_total);
> +			n->mem_free  = bswap_64(n->mem_free);
>  		}
>  
> -		if (strbuf_addf(&sb, "%u:%"PRIu64":%"PRIu64":",
> -				node, mem_total, mem_free) < 0)
> -			goto error;
> -
>  		str = do_read_string(fd, ph);
>  		if (!str)
>  			goto error;
>  
> -		/* include a NULL character at the end */
> -		if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
> +		n->map = cpu_map__new(str);
> +		if (!n->map)
>  			goto error;
> +
>  		free(str);
>  	}
> -	ph->env.numa_nodes = strbuf_detach(&sb, NULL);
> +	ph->env.numa_nodes = nodes;
>  	return 0;
>  
>  error:
> -	strbuf_release(&sb);
> +	free(nodes);
>  	return -1;
>  }
>  
> -- 
> 2.4.11

Re: [PATCH 5/5] perf tools: Transform nodes string info to struct

[Jiri Olsa]

On Thu, Jun 30, 2016 at 06:20:09PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> >  
> > +struct numa_node {
> > +	u32		 node;
> > +	u64		 mem_total;
> > +	u64		 mem_free;
> > +	struct cpu_map	*map;
> > +};
> > +
> >  struct perf_env {
> >  	char			*hostname;
> >  	char			*os_release;
> > @@ -33,18 +41,18 @@ struct perf_env {
> >  	int			nr_cmdline;
> >  	int			nr_sibling_cores;
> >  	int			nr_sibling_threads;
> > -	int			nr_numa_nodes;
> 
> why rename it from nr_numa_nodes to numa_nodes_cnt? Seems gratuitous and
> potentially introduces up to three 4 byte holes into 'struct perf_env'
> :-\

it goes along nicely with the caches/chaches_cnt ;-)

I'll change it back, np

jirka

> 
> Applied the other patches in this series,
> 
> Thanks,
> 
> - Arnaldo
> 
> >  	int			nr_pmu_mappings;
> >  	int			nr_groups;
> >  	char			*cmdline;
> >  	const char		**cmdline_argv;
> >  	char			*sibling_cores;
> >  	char			*sibling_threads;
> > -	char			*numa_nodes;
> >  	char			*pmu_mappings;
> >  	struct cpu_topology_map	*cpu;
> >  	struct cpu_cache_level	*caches;
> >  	int			 caches_cnt;
> > +	struct numa_node	*numa_nodes;
> > +	int			 numa_nodes_cnt;
> >  };

SNIP

Re: [PATCH 1/5] perf test: Add -F/--dont-fork option

[Nilay Vaish]

On 28 June 2016 at 06:29, Jiri Olsa <jolsa@kernel.org> wrote:
> Adding -F/--dont-fork option to bypass forking
> for each test. It's useful for debugging test.
>
> Link: http://lkml.kernel.org/n/tip-yq9gy0fcr8nl70986gwnl3dh@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/Documentation/perf-test.txt |  4 +++
>  tools/perf/tests/builtin-test.c        | 55 ++++++++++++++++++++--------------
>  2 files changed, 37 insertions(+), 22 deletions(-)
>

This patch seems fine to me.

--
Nilay

Re: [PATCH 1/5] perf test: Add -F/--dont-fork option

[Arnaldo Carvalho de Melo]

Em Wed, Jun 29, 2016 at 11:04:01AM -0500, Nilay Vaish escreveu:
> On 28 June 2016 at 06:29, Jiri Olsa <jolsa@kernel.org> wrote:
> > Adding -F/--dont-fork option to bypass forking
> > for each test. It's useful for debugging test.
> >
> > Link: http://lkml.kernel.org/n/tip-yq9gy0fcr8nl70986gwnl3dh@git.kernel.org
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/Documentation/perf-test.txt |  4 +++
> >  tools/perf/tests/builtin-test.c        | 55 ++++++++++++++++++++--------------
> >  2 files changed, 37 insertions(+), 22 deletions(-)
> >
> 
> This patch seems fine to me.

In these cases, please state that more formally, i.e. please read
Documentation/SubmittingPatches, specially this section:

12) When to use Acked-by:

and:

13) Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and
Fixes:

- Arnaldo

Re: [PATCH 1/5] perf test: Add -F/--dont-fork option

[Nilay Vaish]

On 29 June 2016 at 11:07, Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
> Em Wed, Jun 29, 2016 at 11:04:01AM -0500, Nilay Vaish escreveu:
>> On 28 June 2016 at 06:29, Jiri Olsa <jolsa@kernel.org> wrote:
>> > Adding -F/--dont-fork option to bypass forking
>> > for each test. It's useful for debugging test.
>> >
>> > Link: http://lkml.kernel.org/n/tip-yq9gy0fcr8nl70986gwnl3dh@git.kernel.org
>> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>> > ---
>> >  tools/perf/Documentation/perf-test.txt |  4 +++
>> >  tools/perf/tests/builtin-test.c        | 55 ++++++++++++++++++++--------------
>> >  2 files changed, 37 insertions(+), 22 deletions(-)
>> >
>>
>> This patch seems fine to me.
>
> In these cases, please state that more formally, i.e. please read
> Documentation/SubmittingPatches, specially this section:
>
> 12) When to use Acked-by:
>
> and:
>
> 13) Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and
> Fixes:
>

Thanks for the info.

--
Nilay

Re: [PATCH 1/5] perf test: Add -F/--dont-fork option

[Arnaldo Carvalho de Melo]

Em Tue, Jun 28, 2016 at 01:29:01PM +0200, Jiri Olsa escreveu:
> Adding -F/--dont-fork option to bypass forking
> for each test. It's useful for debugging test.

Applied, but moved 1/5 to after the csets fixing things that breaks with
it, I tested those without this one and they work, then with -F and they
continue working, no bisect breakage that way.

- Arnaldo
 
> Link: http://lkml.kernel.org/n/tip-yq9gy0fcr8nl70986gwnl3dh@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/Documentation/perf-test.txt |  4 +++
>  tools/perf/tests/builtin-test.c        | 55 ++++++++++++++++++++--------------
>  2 files changed, 37 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-test.txt b/tools/perf/Documentation/perf-test.txt
> index 31a5c3ea7f74..b329c65d7f40 100644
> --- a/tools/perf/Documentation/perf-test.txt
> +++ b/tools/perf/Documentation/perf-test.txt
> @@ -30,3 +30,7 @@ OPTIONS
>  -v::
>  --verbose::
>  	Be more verbose.
> +
> +-F::
> +--dont-fork::
> +	Do not fork child for each test, run all tests within single process.
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 0e95c20ecf6e..5781c1640eae 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -14,6 +14,8 @@
>  #include <subcmd/parse-options.h>
>  #include "symbol.h"
>  
> +static bool dont_fork;
> +
>  struct test __weak arch_tests[] = {
>  	{
>  		.func = NULL,
> @@ -247,7 +249,7 @@ static bool perf_test__matches(struct test *test, int curr, int argc, const char
>  
>  static int run_test(struct test *test, int subtest)
>  {
> -	int status, err = -1, child = fork();
> +	int status, err = -1, child = dont_fork ? 0 : fork();
>  	char sbuf[STRERR_BUFSIZE];
>  
>  	if (child < 0) {
> @@ -257,34 +259,41 @@ static int run_test(struct test *test, int subtest)
>  	}
>  
>  	if (!child) {
> -		pr_debug("test child forked, pid %d\n", getpid());
> -		if (!verbose) {
> -			int nullfd = open("/dev/null", O_WRONLY);
> -			if (nullfd >= 0) {
> -				close(STDERR_FILENO);
> -				close(STDOUT_FILENO);
> -
> -				dup2(nullfd, STDOUT_FILENO);
> -				dup2(STDOUT_FILENO, STDERR_FILENO);
> -				close(nullfd);
> +		if (!dont_fork) {
> +			pr_debug("test child forked, pid %d\n", getpid());
> +
> +			if (!verbose) {
> +				int nullfd = open("/dev/null", O_WRONLY);
> +
> +				if (nullfd >= 0) {
> +					close(STDERR_FILENO);
> +					close(STDOUT_FILENO);
> +
> +					dup2(nullfd, STDOUT_FILENO);
> +					dup2(STDOUT_FILENO, STDERR_FILENO);
> +					close(nullfd);
> +				}
> +			} else {
> +				signal(SIGSEGV, sighandler_dump_stack);
> +				signal(SIGFPE, sighandler_dump_stack);
>  			}
> -		} else {
> -			signal(SIGSEGV, sighandler_dump_stack);
> -			signal(SIGFPE, sighandler_dump_stack);
>  		}
>  
>  		err = test->func(subtest);
> -		exit(err);
> +		if (!dont_fork)
> +			exit(err);
>  	}
>  
> -	wait(&status);
> +	if (!dont_fork) {
> +		wait(&status);
>  
> -	if (WIFEXITED(status)) {
> -		err = (signed char)WEXITSTATUS(status);
> -		pr_debug("test child finished with %d\n", err);
> -	} else if (WIFSIGNALED(status)) {
> -		err = -1;
> -		pr_debug("test child interrupted\n");
> +		if (WIFEXITED(status)) {
> +			err = (signed char)WEXITSTATUS(status);
> +			pr_debug("test child finished with %d\n", err);
> +		} else if (WIFSIGNALED(status)) {
> +			err = -1;
> +			pr_debug("test child interrupted\n");
> +		}
>  	}
>  
>  	return err;
> @@ -425,6 +434,8 @@ int cmd_test(int argc, const char **argv, const char *prefix __maybe_unused)
>  	OPT_STRING('s', "skip", &skip, "tests", "tests to skip"),
>  	OPT_INCR('v', "verbose", &verbose,
>  		    "be more verbose (show symbol address, etc)"),
> +	OPT_BOOLEAN('F', "dont-fork", &dont_fork,
> +		    "Do not fork for testcase"),
>  	OPT_END()
>  	};
>  	const char * const test_subcommands[] = { "list", NULL };
> -- 
> 2.4.11

[tip:perf/core] perf test: Add -F/--dont-fork option

[tip-bot for Jiri Olsa]

Commit-ID:  7fa9b8fba0b55edd1ff5b8ea696ec75fc5f6194c
Gitweb:     http://git.kernel.org/tip/7fa9b8fba0b55edd1ff5b8ea696ec75fc5f6194c
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Tue, 28 Jun 2016 13:29:01 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 30 Jun 2016 18:27:45 -0300

perf test: Add -F/--dont-fork option

Adding -F/--dont-fork option to bypass forking for each test. It's
useful for debugging test.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested-by: Nilay Vaish <nilayvaish@gmail.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1467113345-12669-1-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Documentation/perf-test.txt |  4 +++
 tools/perf/tests/builtin-test.c        | 55 ++++++++++++++++++++--------------
 2 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/tools/perf/Documentation/perf-test.txt b/tools/perf/Documentation/perf-test.txt
index 31a5c3e..b329c65 100644
--- a/tools/perf/Documentation/perf-test.txt
+++ b/tools/perf/Documentation/perf-test.txt
@@ -30,3 +30,7 @@ OPTIONS
 -v::
 --verbose::
 	Be more verbose.
+
+-F::
+--dont-fork::
+	Do not fork child for each test, run all tests within single process.
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 0e95c20..5781c16 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -14,6 +14,8 @@
 #include <subcmd/parse-options.h>
 #include "symbol.h"
 
+static bool dont_fork;
+
 struct test __weak arch_tests[] = {
 	{
 		.func = NULL,
@@ -247,7 +249,7 @@ static bool perf_test__matches(struct test *test, int curr, int argc, const char
 
 static int run_test(struct test *test, int subtest)
 {
-	int status, err = -1, child = fork();
+	int status, err = -1, child = dont_fork ? 0 : fork();
 	char sbuf[STRERR_BUFSIZE];
 
 	if (child < 0) {
@@ -257,34 +259,41 @@ static int run_test(struct test *test, int subtest)
 	}
 
 	if (!child) {
-		pr_debug("test child forked, pid %d\n", getpid());
-		if (!verbose) {
-			int nullfd = open("/dev/null", O_WRONLY);
-			if (nullfd >= 0) {
-				close(STDERR_FILENO);
-				close(STDOUT_FILENO);
-
-				dup2(nullfd, STDOUT_FILENO);
-				dup2(STDOUT_FILENO, STDERR_FILENO);
-				close(nullfd);
+		if (!dont_fork) {
+			pr_debug("test child forked, pid %d\n", getpid());
+
+			if (!verbose) {
+				int nullfd = open("/dev/null", O_WRONLY);
+
+				if (nullfd >= 0) {
+					close(STDERR_FILENO);
+					close(STDOUT_FILENO);
+
+					dup2(nullfd, STDOUT_FILENO);
+					dup2(STDOUT_FILENO, STDERR_FILENO);
+					close(nullfd);
+				}
+			} else {
+				signal(SIGSEGV, sighandler_dump_stack);
+				signal(SIGFPE, sighandler_dump_stack);
 			}
-		} else {
-			signal(SIGSEGV, sighandler_dump_stack);
-			signal(SIGFPE, sighandler_dump_stack);
 		}
 
 		err = test->func(subtest);
-		exit(err);
+		if (!dont_fork)
+			exit(err);
 	}
 
-	wait(&status);
+	if (!dont_fork) {
+		wait(&status);
 
-	if (WIFEXITED(status)) {
-		err = (signed char)WEXITSTATUS(status);
-		pr_debug("test child finished with %d\n", err);
-	} else if (WIFSIGNALED(status)) {
-		err = -1;
-		pr_debug("test child interrupted\n");
+		if (WIFEXITED(status)) {
+			err = (signed char)WEXITSTATUS(status);
+			pr_debug("test child finished with %d\n", err);
+		} else if (WIFSIGNALED(status)) {
+			err = -1;
+			pr_debug("test child interrupted\n");
+		}
 	}
 
 	return err;
@@ -425,6 +434,8 @@ int cmd_test(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_STRING('s', "skip", &skip, "tests", "tests to skip"),
 	OPT_INCR('v', "verbose", &verbose,
 		    "be more verbose (show symbol address, etc)"),
+	OPT_BOOLEAN('F', "dont-fork", &dont_fork,
+		    "Do not fork for testcase"),
 	OPT_END()
 	};
 	const char * const test_subcommands[] = { "list", NULL };