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

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/[email protected] open here

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

Quoted 12 lines · 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.

Quoted 2 lines

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

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

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

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/[email protected] open here

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

Quoted 14 lines · 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.

Quoted 4 lines

Setting it explicitly now.

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

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

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

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/[email protected] open here

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

---
 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(-)

tools/perf/util/env.c +4 -1

index 49a11d9d8b8f..153985977475 100644
@@ -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);

tools/perf/util/env.h +8 -0

index 56cffb60a0b4..027d0f4e6bff 100644
@@ -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;

tools/perf/util/env.h +2 -1

 	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;

tools/perf/util/header.c +27 -51

index c5cd2698281f..af5f275462b3 100644
@@ -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;
 }
 

Signature · 1 line

2.4.11

Quoted 12 lines

tools/perf/util/env.h +7 -0

+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 {

tools/perf/util/env.h struct perf_env { · line 36

Quoted 4 lines

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

 	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

Quoted 14 lines

tools/perf/util/env.h +2 -1

 	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;
 };

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

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

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

---
 tools/perf/Documentation/perf-test.txt |  4 +++
 tools/perf/tests/builtin-test.c        | 55 ++++++++++++++++++++--------------
 2 files changed, 37 insertions(+), 22 deletions(-)

Quoted 10 lines

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

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

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

---
 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.

Em Wed, Jun 29, 2016 at 11:04:01AM -0500, Nilay Vaish escreveu:

Quoted 12 lines · Jiri Olsa

Quoted 10 lines

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

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

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

---
 tools/perf/Documentation/perf-test.txt |  4 +++
 tools/perf/tests/builtin-test.c        | 55 ++++++++++++++++++++--------------
 2 files changed, 37 insertions(+), 22 deletions(-)

Quoted 1 line

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:

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

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

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

---
 tools/perf/Documentation/perf-test.txt |  4 +++
 tools/perf/tests/builtin-test.c        | 55 ++++++++++++++++++++--------------
 2 files changed, 37 insertions(+), 22 deletions(-)

tools/perf/Documentation/perf-test.txt +4 -0

index 31a5c3ea7f74..b329c65d7f40 100644
@@ -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.

tools/perf/tests/builtin-test.c +33 -22

index 0e95c20ecf6e..5781c1640eae 100644
@@ -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 };

Signature · 1 line

2.4.11