[PATCH 0/7] Make more tests skip rather than fail

[PATCH 0/7] Make more tests skip rather than fail

[Ian Rogers]

A long standing niggle has been that tests that cannot pass are marked
as failing rather than skip.  John Garry mentioned a similar concern
in [1]. These changes fix this behavior so that as root, or not, at
least the first 10 tests are passing or skipping.

[1] https://lore.kernel.org/lkml/d32376b5-5538-ff00-6620-e74ad4b4abf2@huawei.com/

Ian Rogers (7):
  perf test: Skip reason for suites with 1 test
  perf test: Use skip in vmlinux kallsyms
  perf test: Use skip in openat syscall
  perf test: Basic mmap use skip
  perf test: Parse events tidy terms_test
  perf test: Parse events tidy evlist_test
  perf test: Parse events break apart tests

 tools/perf/tests/builtin-test.c            |   6 +-
 tools/perf/tests/mmap-basic.c              |  22 +-
 tools/perf/tests/openat-syscall-all-cpus.c |  23 +-
 tools/perf/tests/openat-syscall.c          |  20 +-
 tools/perf/tests/parse-events.c            | 492 +++++++++++----------
 tools/perf/tests/vmlinux-kallsyms.c        |  12 +-
 6 files changed, 329 insertions(+), 246 deletions(-)

-- 
2.36.0.550.gb090851708-goog

[PATCH 1/7] perf test: Skip reason for suites with 1 test

[Ian Rogers]

When a suite has just 1 subtest, the subtest number is given as -1 to
avoid indented printing. When this subtest number is seen for the skip
reason, use the reason of the first test.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/builtin-test.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index fac3717d9ba1..33fcafa0fa79 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -137,10 +137,10 @@ static bool has_subtests(const struct test_suite *t)
 
 static const char *skip_reason(const struct test_suite *t, int subtest)
 {
-	if (t->test_cases && subtest >= 0)
-		return t->test_cases[subtest].skip_reason;
+	if (!t->test_cases)
+		return NULL;
 
-	return NULL;
+	return t->test_cases[subtest >= 0 ? subtest : 0].skip_reason;
 }
 
 static const char *test_description(const struct test_suite *t, int subtest)
-- 
2.36.0.550.gb090851708-goog

[PATCH 2/7] perf test: Use skip in vmlinux kallsyms

[Ian Rogers]

Currently failures in reading vmlinux or kallsyms result in a test
failure. However, the failure is typically permission related. Prefer to
flag these failures as skip.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/vmlinux-kallsyms.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
index 93dee542a177..983f32964749 100644
--- a/tools/perf/tests/vmlinux-kallsyms.c
+++ b/tools/perf/tests/vmlinux-kallsyms.c
@@ -114,7 +114,7 @@ static bool is_ignored_symbol(const char *name, char type)
 static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused,
 					int subtest __maybe_unused)
 {
-	int err = -1;
+	int err = TEST_FAIL;
 	struct rb_node *nd;
 	struct symbol *sym;
 	struct map *kallsyms_map, *vmlinux_map, *map;
@@ -142,7 +142,8 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
 	 * and find the .ko files that match them in /lib/modules/`uname -r`/.
 	 */
 	if (machine__create_kernel_maps(&kallsyms) < 0) {
-		pr_debug("machine__create_kernel_maps ");
+		pr_info("machine__create_kernel_maps failed");
+		err = TEST_SKIP;
 		goto out;
 	}
 
@@ -158,7 +159,8 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
 	 * code and with the one got from /proc/modules from the "kallsyms" code.
 	 */
 	if (machine__load_kallsyms(&kallsyms, "/proc/kallsyms") <= 0) {
-		pr_debug("dso__load_kallsyms ");
+		pr_debug("machine__load_kallsyms failed");
+		err = TEST_SKIP;
 		goto out;
 	}
 
@@ -178,7 +180,7 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
 	 * Now repeat step 2, this time for the vmlinux file we'll auto-locate.
 	 */
 	if (machine__create_kernel_maps(&vmlinux) < 0) {
-		pr_debug("machine__create_kernel_maps ");
+		pr_info("machine__create_kernel_maps failed");
 		goto out;
 	}
 
@@ -196,7 +198,7 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
 	 * to fixup the symbols.
 	 */
 	if (machine__load_vmlinux_path(&vmlinux) <= 0) {
-		pr_debug("Couldn't find a vmlinux that matches the kernel running on this machine, skipping test\n");
+		pr_info("Couldn't find a vmlinux that matches the kernel running on this machine, skipping test\n");
 		err = TEST_SKIP;
 		goto out;
 	}
-- 
2.36.0.550.gb090851708-goog

[PATCH 3/7] perf test: Use skip in openat syscall

[Ian Rogers]

Failures to open the tracepoint cause this test to fail, however,
typically such failures are permission related. Lower the failure to
just skipping the test in those cases and add a skip reason.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/openat-syscall-all-cpus.c | 23 +++++++++++++++++-----
 tools/perf/tests/openat-syscall.c          | 20 +++++++++++++++----
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/tools/perf/tests/openat-syscall-all-cpus.c b/tools/perf/tests/openat-syscall-all-cpus.c
index 1ab362323d25..90828ae03ef5 100644
--- a/tools/perf/tests/openat-syscall-all-cpus.c
+++ b/tools/perf/tests/openat-syscall-all-cpus.c
@@ -22,7 +22,7 @@
 static int test__openat_syscall_event_on_all_cpus(struct test_suite *test __maybe_unused,
 						  int subtest __maybe_unused)
 {
-	int err = -1, fd, idx;
+	int err = TEST_FAIL, fd, idx;
 	struct perf_cpu cpu;
 	struct perf_cpu_map *cpus;
 	struct evsel *evsel;
@@ -49,6 +49,7 @@ static int test__openat_syscall_event_on_all_cpus(struct test_suite *test __mayb
 	if (IS_ERR(evsel)) {
 		tracing_path__strerror_open_tp(errno, errbuf, sizeof(errbuf), "syscalls", "sys_enter_openat");
 		pr_debug("%s\n", errbuf);
+		err = TEST_SKIP;
 		goto out_cpu_map_delete;
 	}
 
@@ -56,6 +57,7 @@ static int test__openat_syscall_event_on_all_cpus(struct test_suite *test __mayb
 		pr_debug("failed to open counter: %s, "
 			 "tweak /proc/sys/kernel/perf_event_paranoid?\n",
 			 str_error_r(errno, sbuf, sizeof(sbuf)));
+		err = TEST_SKIP;
 		goto out_evsel_delete;
 	}
 
@@ -88,7 +90,7 @@ static int test__openat_syscall_event_on_all_cpus(struct test_suite *test __mayb
 
 	evsel->core.cpus = perf_cpu_map__get(cpus);
 
-	err = 0;
+	err = TEST_OK;
 
 	perf_cpu_map__for_each_cpu(cpu, idx, cpus) {
 		unsigned int expected;
@@ -98,7 +100,7 @@ static int test__openat_syscall_event_on_all_cpus(struct test_suite *test __mayb
 
 		if (evsel__read_on_cpu(evsel, idx, 0) < 0) {
 			pr_debug("evsel__read_on_cpu\n");
-			err = -1;
+			err = TEST_FAIL;
 			break;
 		}
 
@@ -106,7 +108,7 @@ static int test__openat_syscall_event_on_all_cpus(struct test_suite *test __mayb
 		if (perf_counts(evsel->counts, idx, 0)->val != expected) {
 			pr_debug("evsel__read_on_cpu: expected to intercept %d calls on cpu %d, got %" PRIu64 "\n",
 				 expected, cpu.cpu, perf_counts(evsel->counts, idx, 0)->val);
-			err = -1;
+			err = TEST_FAIL;
 		}
 	}
 
@@ -122,4 +124,15 @@ static int test__openat_syscall_event_on_all_cpus(struct test_suite *test __mayb
 	return err;
 }
 
-DEFINE_SUITE("Detect openat syscall event on all cpus", openat_syscall_event_on_all_cpus);
+
+static struct test_case tests__openat_syscall_event_on_all_cpus[] = {
+	TEST_CASE_REASON("Detect openat syscall event on all cpus",
+			 openat_syscall_event_on_all_cpus,
+			 "permissions"),
+	{	.name = NULL, }
+};
+
+struct test_suite suite__openat_syscall_event_on_all_cpus = {
+	.desc = "Detect openat syscall event on all cpus",
+	.test_cases = tests__openat_syscall_event_on_all_cpus,
+};
diff --git a/tools/perf/tests/openat-syscall.c b/tools/perf/tests/openat-syscall.c
index 7f4c13c4b14d..7e05b8b5cc95 100644
--- a/tools/perf/tests/openat-syscall.c
+++ b/tools/perf/tests/openat-syscall.c
@@ -16,7 +16,7 @@
 static int test__openat_syscall_event(struct test_suite *test __maybe_unused,
 				      int subtest __maybe_unused)
 {
-	int err = -1, fd;
+	int err = TEST_FAIL, fd;
 	struct evsel *evsel;
 	unsigned int nr_openat_calls = 111, i;
 	struct perf_thread_map *threads = thread_map__new(-1, getpid(), UINT_MAX);
@@ -25,13 +25,14 @@ static int test__openat_syscall_event(struct test_suite *test __maybe_unused,
 
 	if (threads == NULL) {
 		pr_debug("thread_map__new\n");
-		return -1;
+		return TEST_FAIL;
 	}
 
 	evsel = evsel__newtp("syscalls", "sys_enter_openat");
 	if (IS_ERR(evsel)) {
 		tracing_path__strerror_open_tp(errno, errbuf, sizeof(errbuf), "syscalls", "sys_enter_openat");
 		pr_debug("%s\n", errbuf);
+		err = TEST_SKIP;
 		goto out_thread_map_delete;
 	}
 
@@ -39,6 +40,7 @@ static int test__openat_syscall_event(struct test_suite *test __maybe_unused,
 		pr_debug("failed to open counter: %s, "
 			 "tweak /proc/sys/kernel/perf_event_paranoid?\n",
 			 str_error_r(errno, sbuf, sizeof(sbuf)));
+		err = TEST_SKIP;
 		goto out_evsel_delete;
 	}
 
@@ -58,7 +60,7 @@ static int test__openat_syscall_event(struct test_suite *test __maybe_unused,
 		goto out_close_fd;
 	}
 
-	err = 0;
+	err = TEST_OK;
 out_close_fd:
 	perf_evsel__close_fd(&evsel->core);
 out_evsel_delete:
@@ -68,4 +70,14 @@ static int test__openat_syscall_event(struct test_suite *test __maybe_unused,
 	return err;
 }
 
-DEFINE_SUITE("Detect openat syscall event", openat_syscall_event);
+static struct test_case tests__openat_syscall_event[] = {
+	TEST_CASE_REASON("Detect openat syscall event",
+			 openat_syscall_event,
+			 "permissions"),
+	{	.name = NULL, }
+};
+
+struct test_suite suite__openat_syscall_event = {
+	.desc = "Detect openat syscall event",
+	.test_cases = tests__openat_syscall_event,
+};
-- 
2.36.0.550.gb090851708-goog

[PATCH 4/7] perf test: Basic mmap use skip

[Ian Rogers]

If opening the first event fails for basic mmap it is more likely
permission related that a true error. Mark the test as skip in this
case and add a skip reason.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/mmap-basic.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
index c3c17600f29c..32f0a63fa157 100644
--- a/tools/perf/tests/mmap-basic.c
+++ b/tools/perf/tests/mmap-basic.c
@@ -31,7 +31,7 @@
  */
 static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
 {
-	int err = -1;
+	int err = TEST_FAIL;
 	union perf_event *event;
 	struct perf_thread_map *threads;
 	struct perf_cpu_map *cpus;
@@ -83,6 +83,14 @@ static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest
 		evsels[i] = evsel__newtp("syscalls", name);
 		if (IS_ERR(evsels[i])) {
 			pr_debug("evsel__new(%s)\n", name);
+			if (i == 0) {
+				/*
+				 * Failure to open first event is more likely
+				 * related to permissions so flag the failure as
+				 * a skip.
+				 */
+				err = TEST_SKIP;
+			}
 			goto out_delete_evlist;
 		}
 
@@ -166,4 +174,14 @@ static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest
 	return err;
 }
 
-DEFINE_SUITE("Read samples using the mmap interface", basic_mmap);
+static struct test_case tests__basic_mmap[] = {
+	TEST_CASE_REASON("Read samples using the mmap interface",
+			 basic_mmap,
+			 "permissions"),
+	{	.name = NULL, }
+};
+
+struct test_suite suite__basic_mmap = {
+	.desc = "Read samples using the mmap interface",
+	.test_cases = tests__basic_mmap,
+};
-- 
2.36.0.550.gb090851708-goog

[PATCH 5/7] perf test: Parse events tidy terms_test

[Ian Rogers]

Remove an unused variables. Make structs const. Fix checkpatch issue wrt
unsigned not being with an int.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/parse-events.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index e71efadb24f5..7e802666d2d5 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -1980,11 +1980,10 @@ static struct evlist_test test__events_pmu[] = {
 
 struct terms_test {
 	const char *str;
-	__u32 type;
 	int (*check)(struct list_head *terms);
 };
 
-static struct terms_test test__terms[] = {
+static const struct terms_test test__terms[] = {
 	[0] = {
 		.str   = "config=10,config1,config2=3,umask=1,read,r0xead",
 		.check = test__checkterms_simple,
@@ -2112,7 +2111,7 @@ static int test_events(struct evlist_test *events, unsigned cnt)
 	return ret2;
 }
 
-static int test_term(struct terms_test *t)
+static int test_term(const struct terms_test *t)
 {
 	struct list_head terms;
 	int ret;
@@ -2139,13 +2138,12 @@ static int test_term(struct terms_test *t)
 	return ret;
 }
 
-static int test_terms(struct terms_test *terms, unsigned cnt)
+static int test_terms(const struct terms_test *terms, int cnt)
 {
 	int ret = 0;
-	unsigned i;
 
-	for (i = 0; i < cnt; i++) {
-		struct terms_test *t = &terms[i];
+	for (int i = 0; i < cnt; i++) {
+		const struct terms_test *t = &terms[i];
 
 		pr_debug("running test %d '%s'\n", i, t->str);
 		ret = test_term(t);
-- 
2.36.0.550.gb090851708-goog

[PATCH 6/7] perf test: Parse events tidy evlist_test

[Ian Rogers]

Remove two unused variables. Make structs const. Also fix the array
index (aka id) for the event software/r0x1a/.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/parse-events.c | 171 ++++++++++++++++----------------
 1 file changed, 84 insertions(+), 87 deletions(-)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 7e802666d2d5..0d65770bd686 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -1637,344 +1637,342 @@ static int test__hybrid_cache_event(struct evlist *evlist)
 
 struct evlist_test {
 	const char *name;
-	__u32 type;
-	const int id;
 	bool (*valid)(void);
 	int (*check)(struct evlist *evlist);
 };
 
-static struct evlist_test test__events[] = {
+static const struct evlist_test test__events[] = {
 	{
 		.name  = "syscalls:sys_enter_openat",
 		.check = test__checkevent_tracepoint,
-		.id    = 0,
+		/* 0 */
 	},
 	{
 		.name  = "syscalls:*",
 		.check = test__checkevent_tracepoint_multi,
-		.id    = 1,
+		/* 1 */
 	},
 	{
 		.name  = "r1a",
 		.check = test__checkevent_raw,
-		.id    = 2,
+		/* 2 */
 	},
 	{
 		.name  = "1:1",
 		.check = test__checkevent_numeric,
-		.id    = 3,
+		/* 3 */
 	},
 	{
 		.name  = "instructions",
 		.check = test__checkevent_symbolic_name,
-		.id    = 4,
+		/* 4 */
 	},
 	{
 		.name  = "cycles/period=100000,config2/",
 		.check = test__checkevent_symbolic_name_config,
-		.id    = 5,
+		/* 5 */
 	},
 	{
 		.name  = "faults",
 		.check = test__checkevent_symbolic_alias,
-		.id    = 6,
+		/* 6 */
 	},
 	{
 		.name  = "L1-dcache-load-miss",
 		.check = test__checkevent_genhw,
-		.id    = 7,
+		/* 7 */
 	},
 	{
 		.name  = "mem:0",
 		.check = test__checkevent_breakpoint,
-		.id    = 8,
+		/* 8 */
 	},
 	{
 		.name  = "mem:0:x",
 		.check = test__checkevent_breakpoint_x,
-		.id    = 9,
+		/* 9 */
 	},
 	{
 		.name  = "mem:0:r",
 		.check = test__checkevent_breakpoint_r,
-		.id    = 10,
+		/* 0 */
 	},
 	{
 		.name  = "mem:0:w",
 		.check = test__checkevent_breakpoint_w,
-		.id    = 11,
+		/* 1 */
 	},
 	{
 		.name  = "syscalls:sys_enter_openat:k",
 		.check = test__checkevent_tracepoint_modifier,
-		.id    = 12,
+		/* 2 */
 	},
 	{
 		.name  = "syscalls:*:u",
 		.check = test__checkevent_tracepoint_multi_modifier,
-		.id    = 13,
+		/* 3 */
 	},
 	{
 		.name  = "r1a:kp",
 		.check = test__checkevent_raw_modifier,
-		.id    = 14,
+		/* 4 */
 	},
 	{
 		.name  = "1:1:hp",
 		.check = test__checkevent_numeric_modifier,
-		.id    = 15,
+		/* 5 */
 	},
 	{
 		.name  = "instructions:h",
 		.check = test__checkevent_symbolic_name_modifier,
-		.id    = 16,
+		/* 6 */
 	},
 	{
 		.name  = "faults:u",
 		.check = test__checkevent_symbolic_alias_modifier,
-		.id    = 17,
+		/* 7 */
 	},
 	{
 		.name  = "L1-dcache-load-miss:kp",
 		.check = test__checkevent_genhw_modifier,
-		.id    = 18,
+		/* 8 */
 	},
 	{
 		.name  = "mem:0:u",
 		.check = test__checkevent_breakpoint_modifier,
-		.id    = 19,
+		/* 9 */
 	},
 	{
 		.name  = "mem:0:x:k",
 		.check = test__checkevent_breakpoint_x_modifier,
-		.id    = 20,
+		/* 0 */
 	},
 	{
 		.name  = "mem:0:r:hp",
 		.check = test__checkevent_breakpoint_r_modifier,
-		.id    = 21,
+		/* 1 */
 	},
 	{
 		.name  = "mem:0:w:up",
 		.check = test__checkevent_breakpoint_w_modifier,
-		.id    = 22,
+		/* 2 */
 	},
 	{
 		.name  = "r1,syscalls:sys_enter_openat:k,1:1:hp",
 		.check = test__checkevent_list,
-		.id    = 23,
+		/* 3 */
 	},
 	{
 		.name  = "instructions:G",
 		.check = test__checkevent_exclude_host_modifier,
-		.id    = 24,
+		/* 4 */
 	},
 	{
 		.name  = "instructions:H",
 		.check = test__checkevent_exclude_guest_modifier,
-		.id    = 25,
+		/* 5 */
 	},
 	{
 		.name  = "mem:0:rw",
 		.check = test__checkevent_breakpoint_rw,
-		.id    = 26,
+		/* 6 */
 	},
 	{
 		.name  = "mem:0:rw:kp",
 		.check = test__checkevent_breakpoint_rw_modifier,
-		.id    = 27,
+		/* 7 */
 	},
 	{
 		.name  = "{instructions:k,cycles:upp}",
 		.check = test__group1,
-		.id    = 28,
+		/* 8 */
 	},
 	{
 		.name  = "{faults:k,cache-references}:u,cycles:k",
 		.check = test__group2,
-		.id    = 29,
+		/* 9 */
 	},
 	{
 		.name  = "group1{syscalls:sys_enter_openat:H,cycles:kppp},group2{cycles,1:3}:G,instructions:u",
 		.check = test__group3,
-		.id    = 30,
+		/* 0 */
 	},
 	{
 		.name  = "{cycles:u,instructions:kp}:p",
 		.check = test__group4,
-		.id    = 31,
+		/* 1 */
 	},
 	{
 		.name  = "{cycles,instructions}:G,{cycles:G,instructions:G},cycles",
 		.check = test__group5,
-		.id    = 32,
+		/* 2 */
 	},
 	{
 		.name  = "*:*",
 		.check = test__all_tracepoints,
-		.id    = 33,
+		/* 3 */
 	},
 	{
 		.name  = "{cycles,cache-misses:G}:H",
 		.check = test__group_gh1,
-		.id    = 34,
+		/* 4 */
 	},
 	{
 		.name  = "{cycles,cache-misses:H}:G",
 		.check = test__group_gh2,
-		.id    = 35,
+		/* 5 */
 	},
 	{
 		.name  = "{cycles:G,cache-misses:H}:u",
 		.check = test__group_gh3,
-		.id    = 36,
+		/* 6 */
 	},
 	{
 		.name  = "{cycles:G,cache-misses:H}:uG",
 		.check = test__group_gh4,
-		.id    = 37,
+		/* 7 */
 	},
 	{
 		.name  = "{cycles,cache-misses,branch-misses}:S",
 		.check = test__leader_sample1,
-		.id    = 38,
+		/* 8 */
 	},
 	{
 		.name  = "{instructions,branch-misses}:Su",
 		.check = test__leader_sample2,
-		.id    = 39,
+		/* 9 */
 	},
 	{
 		.name  = "instructions:uDp",
 		.check = test__checkevent_pinned_modifier,
-		.id    = 40,
+		/* 0 */
 	},
 	{
 		.name  = "{cycles,cache-misses,branch-misses}:D",
 		.check = test__pinned_group,
-		.id    = 41,
+		/* 1 */
 	},
 	{
 		.name  = "mem:0/1",
 		.check = test__checkevent_breakpoint_len,
-		.id    = 42,
+		/* 2 */
 	},
 	{
 		.name  = "mem:0/2:w",
 		.check = test__checkevent_breakpoint_len_w,
-		.id    = 43,
+		/* 3 */
 	},
 	{
 		.name  = "mem:0/4:rw:u",
 		.check = test__checkevent_breakpoint_len_rw_modifier,
-		.id    = 44
+		/* 4 */
 	},
 #if defined(__s390x__)
 	{
 		.name  = "kvm-s390:kvm_s390_create_vm",
 		.check = test__checkevent_tracepoint,
 		.valid = kvm_s390_create_vm_valid,
-		.id    = 100,
+		/* 0 */
 	},
 #endif
 	{
 		.name  = "instructions:I",
 		.check = test__checkevent_exclude_idle_modifier,
-		.id    = 45,
+		/* 5 */
 	},
 	{
 		.name  = "instructions:kIG",
 		.check = test__checkevent_exclude_idle_modifier_1,
-		.id    = 46,
+		/* 6 */
 	},
 	{
 		.name  = "task-clock:P,cycles",
 		.check = test__checkevent_precise_max_modifier,
-		.id    = 47,
+		/* 7 */
 	},
 	{
 		.name  = "instructions/name=insn/",
 		.check = test__checkevent_config_symbol,
-		.id    = 48,
+		/* 8 */
 	},
 	{
 		.name  = "r1234/name=rawpmu/",
 		.check = test__checkevent_config_raw,
-		.id    = 49,
+		/* 9 */
 	},
 	{
 		.name  = "4:0x6530160/name=numpmu/",
 		.check = test__checkevent_config_num,
-		.id    = 50,
+		/* 0 */
 	},
 	{
 		.name  = "L1-dcache-misses/name=cachepmu/",
 		.check = test__checkevent_config_cache,
-		.id    = 51,
+		/* 1 */
 	},
 	{
 		.name  = "intel_pt//u",
 		.valid = test__intel_pt_valid,
 		.check = test__intel_pt,
-		.id    = 52,
+		/* 2 */
 	},
 	{
 		.name  = "cycles/name='COMPLEX_CYCLES_NAME:orig=cycles,desc=chip-clock-ticks'/Duk",
 		.check = test__checkevent_complex_name,
-		.id    = 53
+		/* 3 */
 	},
 	{
 		.name  = "cycles//u",
 		.check = test__sym_event_slash,
-		.id    = 54,
+		/* 4 */
 	},
 	{
 		.name  = "cycles:k",
 		.check = test__sym_event_dc,
-		.id    = 55,
+		/* 5 */
 	},
 	{
 		.name  = "instructions:uep",
 		.check = test__checkevent_exclusive_modifier,
-		.id    = 56,
+		/* 6 */
 	},
 	{
 		.name  = "{cycles,cache-misses,branch-misses}:e",
 		.check = test__exclusive_group,
-		.id    = 57,
+		/* 7 */
 	},
 };
 
-static struct evlist_test test__events_pmu[] = {
+static const struct evlist_test test__events_pmu[] = {
 	{
 		.name  = "cpu/config=10,config1,config2=3,period=1000/u",
 		.check = test__checkevent_pmu,
-		.id    = 0,
+		/* 0 */
 	},
 	{
 		.name  = "cpu/config=1,name=krava/u,cpu/config=2/u",
 		.check = test__checkevent_pmu_name,
-		.id    = 1,
+		/* 1 */
 	},
 	{
 		.name  = "cpu/config=1,call-graph=fp,time,period=100000/,cpu/config=2,call-graph=no,time=0,period=2000/",
 		.check = test__checkevent_pmu_partial_time_callgraph,
-		.id    = 2,
+		/* 2 */
 	},
 	{
 		.name  = "cpu/name='COMPLEX_CYCLES_NAME:orig=cycles,desc=chip-clock-ticks',period=0x1,event=0x2/ukp",
 		.check = test__checkevent_complex_name,
-		.id    = 3,
+		/* 3 */
 	},
 	{
 		.name  = "software/r1a/",
 		.check = test__checkevent_raw_pmu,
-		.id    = 4,
+		/* 4 */
 	},
 	{
 		.name  = "software/r0x1a/",
 		.check = test__checkevent_raw_pmu,
-		.id    = 4,
+		/* 5 */
 	},
 };
 
@@ -1990,55 +1988,55 @@ static const struct terms_test test__terms[] = {
 	},
 };
 
-static struct evlist_test test__hybrid_events[] = {
+static const struct evlist_test test__hybrid_events[] = {
 	{
 		.name  = "cpu_core/cpu-cycles/",
 		.check = test__hybrid_hw_event_with_pmu,
-		.id    = 0,
+		/* 0 */
 	},
 	{
 		.name  = "{cpu_core/cpu-cycles/,cpu_core/instructions/}",
 		.check = test__hybrid_hw_group_event,
-		.id    = 1,
+		/* 1 */
 	},
 	{
 		.name  = "{cpu-clock,cpu_core/cpu-cycles/}",
 		.check = test__hybrid_sw_hw_group_event,
-		.id    = 2,
+		/* 2 */
 	},
 	{
 		.name  = "{cpu_core/cpu-cycles/,cpu-clock}",
 		.check = test__hybrid_hw_sw_group_event,
-		.id    = 3,
+		/* 3 */
 	},
 	{
 		.name  = "{cpu_core/cpu-cycles/k,cpu_core/instructions/u}",
 		.check = test__hybrid_group_modifier1,
-		.id    = 4,
+		/* 4 */
 	},
 	{
 		.name  = "r1a",
 		.check = test__hybrid_raw1,
-		.id    = 5,
+		/* 5 */
 	},
 	{
 		.name  = "cpu_core/r1a/",
 		.check = test__hybrid_raw2,
-		.id    = 6,
+		/* 6 */
 	},
 	{
 		.name  = "cpu_core/config=10,config1,config2=3,period=1000/u",
 		.check = test__checkevent_pmu,
-		.id    = 7,
+		/* 7 */
 	},
 	{
 		.name  = "cpu_core/LLC-loads/",
 		.check = test__hybrid_cache_event,
-		.id    = 8,
+		/* 8 */
 	},
 };
 
-static int test_event(struct evlist_test *e)
+static int test_event(const struct evlist_test *e)
 {
 	struct parse_events_error err;
 	struct evlist *evlist;
@@ -2093,15 +2091,14 @@ static int test_event_fake_pmu(const char *str)
 	return ret;
 }
 
-static int test_events(struct evlist_test *events, unsigned cnt)
+static int test_events(const struct evlist_test *events, int cnt)
 {
 	int ret1, ret2 = 0;
-	unsigned i;
 
-	for (i = 0; i < cnt; i++) {
-		struct evlist_test *e = &events[i];
+	for (int i = 0; i < cnt; i++) {
+		const struct evlist_test *e = &events[i];
 
-		pr_debug("running test %d '%s'", e->id, e->name);
+		pr_debug("running test %d '%s'", i, e->name);
 		ret1 = test_event(e);
 		if (ret1)
 			ret2 = ret1;
@@ -2193,7 +2190,7 @@ static int test_pmu_events(void)
 	}
 
 	while (!ret && (ent = readdir(dir))) {
-		struct evlist_test e = { .id = 0, };
+		struct evlist_test e = { .name = NULL, };
 		char name[2 * NAME_MAX + 1 + 12 + 3];
 
 		/* Names containing . are special and cannot be used directly */
@@ -2288,7 +2285,7 @@ static int test__checkevent_pmu_events_alias(struct evlist *evlist)
 
 static int test_pmu_events_alias(char *event, char *alias)
 {
-	struct evlist_test e = { .id = 0, };
+	struct evlist_test e = { .name = NULL, };
 	char name[2 * NAME_MAX + 20];
 
 	snprintf(name, sizeof(name), "%s/event=1/,%s/event=1/",
-- 
2.36.0.550.gb090851708-goog

[PATCH 7/7] perf test: Parse events break apart tests

[Ian Rogers]

Break multiple tests in the main test into individual test cases. Make
better use of skip and add reasons. Skip also for parse event permission
issues (detected by searching the error string). Rather than break out
of tests on the first failure, keep going and logging to pr_debug.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/parse-events.c | 311 ++++++++++++++++++--------------
 1 file changed, 177 insertions(+), 134 deletions(-)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 0d65770bd686..459afdb256a1 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -56,7 +56,7 @@ static int test__checkevent_tracepoint(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong sample_type",
 		PERF_TP_SAMPLE_TYPE == evsel->core.attr.sample_type);
 	TEST_ASSERT_VAL("wrong sample_period", 1 == evsel->core.attr.sample_period);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_tracepoint_multi(struct evlist *evlist)
@@ -74,7 +74,7 @@ static int test__checkevent_tracepoint_multi(struct evlist *evlist)
 		TEST_ASSERT_VAL("wrong sample_period",
 			1 == evsel->core.attr.sample_period);
 	}
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_raw(struct evlist *evlist)
@@ -84,7 +84,7 @@ static int test__checkevent_raw(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->core.nr_entries);
 	TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong config", 0x1a == evsel->core.attr.config);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_numeric(struct evlist *evlist)
@@ -94,7 +94,7 @@ static int test__checkevent_numeric(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->core.nr_entries);
 	TEST_ASSERT_VAL("wrong type", 1 == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong config", 1 == evsel->core.attr.config);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_symbolic_name(struct evlist *evlist)
@@ -105,7 +105,7 @@ static int test__checkevent_symbolic_name(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong type", PERF_TYPE_HARDWARE == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong config",
 			PERF_COUNT_HW_INSTRUCTIONS == evsel->core.attr.config);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_symbolic_name_config(struct evlist *evlist)
@@ -126,7 +126,7 @@ static int test__checkevent_symbolic_name_config(struct evlist *evlist)
 			0 == evsel->core.attr.config1);
 	TEST_ASSERT_VAL("wrong config2",
 			1 == evsel->core.attr.config2);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_symbolic_alias(struct evlist *evlist)
@@ -137,7 +137,7 @@ static int test__checkevent_symbolic_alias(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong type", PERF_TYPE_SOFTWARE == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong config",
 			PERF_COUNT_SW_PAGE_FAULTS == evsel->core.attr.config);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_genhw(struct evlist *evlist)
@@ -147,7 +147,7 @@ static int test__checkevent_genhw(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->core.nr_entries);
 	TEST_ASSERT_VAL("wrong type", PERF_TYPE_HW_CACHE == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong config", (1 << 16) == evsel->core.attr.config);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_breakpoint(struct evlist *evlist)
@@ -161,7 +161,7 @@ static int test__checkevent_breakpoint(struct evlist *evlist)
 					 evsel->core.attr.bp_type);
 	TEST_ASSERT_VAL("wrong bp_len", HW_BREAKPOINT_LEN_4 ==
 					evsel->core.attr.bp_len);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_breakpoint_x(struct evlist *evlist)
@@ -174,7 +174,7 @@ static int test__checkevent_breakpoint_x(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong bp_type",
 			HW_BREAKPOINT_X == evsel->core.attr.bp_type);
 	TEST_ASSERT_VAL("wrong bp_len", sizeof(long) == evsel->core.attr.bp_len);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_breakpoint_r(struct evlist *evlist)
@@ -189,7 +189,7 @@ static int test__checkevent_breakpoint_r(struct evlist *evlist)
 			HW_BREAKPOINT_R == evsel->core.attr.bp_type);
 	TEST_ASSERT_VAL("wrong bp_len",
 			HW_BREAKPOINT_LEN_4 == evsel->core.attr.bp_len);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_breakpoint_w(struct evlist *evlist)
@@ -204,7 +204,7 @@ static int test__checkevent_breakpoint_w(struct evlist *evlist)
 			HW_BREAKPOINT_W == evsel->core.attr.bp_type);
 	TEST_ASSERT_VAL("wrong bp_len",
 			HW_BREAKPOINT_LEN_4 == evsel->core.attr.bp_len);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_breakpoint_rw(struct evlist *evlist)
@@ -219,7 +219,7 @@ static int test__checkevent_breakpoint_rw(struct evlist *evlist)
 		(HW_BREAKPOINT_R|HW_BREAKPOINT_W) == evsel->core.attr.bp_type);
 	TEST_ASSERT_VAL("wrong bp_len",
 			HW_BREAKPOINT_LEN_4 == evsel->core.attr.bp_len);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_tracepoint_modifier(struct evlist *evlist)
@@ -450,7 +450,7 @@ static int test__checkevent_pmu(struct evlist *evlist)
 	 */
 	TEST_ASSERT_VAL("wrong period",     0 == evsel->core.attr.sample_period);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_list(struct evlist *evlist)
@@ -489,7 +489,7 @@ static int test__checkevent_list(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong exclude_hv", !evsel->core.attr.exclude_hv);
 	TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_pmu_name(struct evlist *evlist)
@@ -510,7 +510,7 @@ static int test__checkevent_pmu_name(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong name",
 			!strcmp(evsel__name(evsel), "cpu/config=2/u"));
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_pmu_partial_time_callgraph(struct evlist *evlist)
@@ -541,7 +541,7 @@ static int test__checkevent_pmu_partial_time_callgraph(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong callgraph",  !evsel__has_callchain(evsel));
 	TEST_ASSERT_VAL("wrong time",  !(PERF_SAMPLE_TIME & evsel->core.attr.sample_type));
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_pmu_events(struct evlist *evlist)
@@ -559,7 +559,7 @@ static int test__checkevent_pmu_events(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong pinned", !evsel->core.attr.pinned);
 	TEST_ASSERT_VAL("wrong exclusive", !evsel->core.attr.exclusive);
 
-	return 0;
+	return TEST_OK;
 }
 
 
@@ -591,7 +591,7 @@ static int test__checkevent_pmu_events_mix(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong pinned", !evsel->core.attr.pinned);
 	TEST_ASSERT_VAL("wrong exclusive", !evsel->core.attr.pinned);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkterms_simple(struct list_head *terms)
@@ -662,7 +662,7 @@ static int test__checkterms_simple(struct list_head *terms)
 			term->type_val == PARSE_EVENTS__TERM_TYPE_NUM);
 	TEST_ASSERT_VAL("wrong val", term->val.num == 0xead);
 	TEST_ASSERT_VAL("wrong config", !strcmp(term->config, "config"));
-	return 0;
+	return TEST_OK;
 }
 
 static int test__group1(struct evlist *evlist)
@@ -704,7 +704,7 @@ static int test__group1(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong group_idx", evsel__group_idx(evsel) == 1);
 	TEST_ASSERT_VAL("wrong sample_read", !evsel->sample_read);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__group2(struct evlist *evlist)
@@ -759,7 +759,7 @@ static int test__group2(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong leader", evsel__is_group_leader(evsel));
 	TEST_ASSERT_VAL("wrong sample_read", !evsel->sample_read);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__group3(struct evlist *evlist __maybe_unused)
@@ -851,7 +851,7 @@ static int test__group3(struct evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong leader", evsel__is_group_leader(evsel));
 	TEST_ASSERT_VAL("wrong sample_read", !evsel->sample_read);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__group4(struct evlist *evlist __maybe_unused)
@@ -895,7 +895,7 @@ static int test__group4(struct evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong group_idx", evsel__group_idx(evsel) == 1);
 	TEST_ASSERT_VAL("wrong sample_read", !evsel->sample_read);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__group5(struct evlist *evlist __maybe_unused)
@@ -981,7 +981,7 @@ static int test__group5(struct evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
 	TEST_ASSERT_VAL("wrong leader", evsel__is_group_leader(evsel));
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__group_gh1(struct evlist *evlist)
@@ -1021,7 +1021,7 @@ static int test__group_gh1(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
 	TEST_ASSERT_VAL("wrong group_idx", evsel__group_idx(evsel) == 1);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__group_gh2(struct evlist *evlist)
@@ -1061,7 +1061,7 @@ static int test__group_gh2(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
 	TEST_ASSERT_VAL("wrong group_idx", evsel__group_idx(evsel) == 1);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__group_gh3(struct evlist *evlist)
@@ -1101,7 +1101,7 @@ static int test__group_gh3(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
 	TEST_ASSERT_VAL("wrong group_idx", evsel__group_idx(evsel) == 1);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__group_gh4(struct evlist *evlist)
@@ -1141,7 +1141,7 @@ static int test__group_gh4(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
 	TEST_ASSERT_VAL("wrong group_idx", evsel__group_idx(evsel) == 1);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__leader_sample1(struct evlist *evlist)
@@ -1194,7 +1194,7 @@ static int test__leader_sample1(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
 	TEST_ASSERT_VAL("wrong sample_read", evsel->sample_read);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__leader_sample2(struct evlist *evlist __maybe_unused)
@@ -1233,7 +1233,7 @@ static int test__leader_sample2(struct evlist *evlist __maybe_unused)
 	TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
 	TEST_ASSERT_VAL("wrong sample_read", evsel->sample_read);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_pinned_modifier(struct evlist *evlist)
@@ -1277,7 +1277,7 @@ static int test__pinned_group(struct evlist *evlist)
 			PERF_COUNT_HW_BRANCH_MISSES == evsel->core.attr.config);
 	TEST_ASSERT_VAL("wrong pinned", !evsel->core.attr.pinned);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_exclusive_modifier(struct evlist *evlist)
@@ -1321,7 +1321,7 @@ static int test__exclusive_group(struct evlist *evlist)
 			PERF_COUNT_HW_BRANCH_MISSES == evsel->core.attr.config);
 	TEST_ASSERT_VAL("wrong exclusive", !evsel->core.attr.exclusive);
 
-	return 0;
+	return TEST_OK;
 }
 static int test__checkevent_breakpoint_len(struct evlist *evlist)
 {
@@ -1335,7 +1335,7 @@ static int test__checkevent_breakpoint_len(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong bp_len", HW_BREAKPOINT_LEN_1 ==
 					evsel->core.attr.bp_len);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_breakpoint_len_w(struct evlist *evlist)
@@ -1350,7 +1350,7 @@ static int test__checkevent_breakpoint_len_w(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong bp_len", HW_BREAKPOINT_LEN_2 ==
 					evsel->core.attr.bp_len);
 
-	return 0;
+	return TEST_OK;
 }
 
 static int
@@ -1374,7 +1374,7 @@ static int test__checkevent_precise_max_modifier(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong type", PERF_TYPE_SOFTWARE == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong config",
 			PERF_COUNT_SW_TASK_CLOCK == evsel->core.attr.config);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_config_symbol(struct evlist *evlist)
@@ -1382,7 +1382,7 @@ static int test__checkevent_config_symbol(struct evlist *evlist)
 	struct evsel *evsel = evlist__first(evlist);
 
 	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "insn") == 0);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_config_raw(struct evlist *evlist)
@@ -1390,7 +1390,7 @@ static int test__checkevent_config_raw(struct evlist *evlist)
 	struct evsel *evsel = evlist__first(evlist);
 
 	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "rawpmu") == 0);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_config_num(struct evlist *evlist)
@@ -1398,7 +1398,7 @@ static int test__checkevent_config_num(struct evlist *evlist)
 	struct evsel *evsel = evlist__first(evlist);
 
 	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "numpmu") == 0);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_config_cache(struct evlist *evlist)
@@ -1406,7 +1406,7 @@ static int test__checkevent_config_cache(struct evlist *evlist)
 	struct evsel *evsel = evlist__first(evlist);
 
 	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "cachepmu") == 0);
-	return 0;
+	return TEST_OK;
 }
 
 static bool test__intel_pt_valid(void)
@@ -1419,7 +1419,7 @@ static int test__intel_pt(struct evlist *evlist)
 	struct evsel *evsel = evlist__first(evlist);
 
 	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "intel_pt//u") == 0);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_complex_name(struct evlist *evlist)
@@ -1427,7 +1427,7 @@ static int test__checkevent_complex_name(struct evlist *evlist)
 	struct evsel *evsel = evlist__first(evlist);
 
 	TEST_ASSERT_VAL("wrong complex name parsing", strcmp(evsel->name, "COMPLEX_CYCLES_NAME:orig=cycles,desc=chip-clock-ticks") == 0);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__checkevent_raw_pmu(struct evlist *evlist)
@@ -1437,7 +1437,7 @@ static int test__checkevent_raw_pmu(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->core.nr_entries);
 	TEST_ASSERT_VAL("wrong type", PERF_TYPE_SOFTWARE == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong config", 0x1a == evsel->core.attr.config);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__sym_event_slash(struct evlist *evlist)
@@ -1447,7 +1447,7 @@ static int test__sym_event_slash(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong type", evsel->core.attr.type == PERF_TYPE_HARDWARE);
 	TEST_ASSERT_VAL("wrong config", evsel->core.attr.config == PERF_COUNT_HW_CPU_CYCLES);
 	TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__sym_event_dc(struct evlist *evlist)
@@ -1457,7 +1457,7 @@ static int test__sym_event_dc(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong type", evsel->core.attr.type == PERF_TYPE_HARDWARE);
 	TEST_ASSERT_VAL("wrong config", evsel->core.attr.config == PERF_COUNT_HW_CPU_CYCLES);
 	TEST_ASSERT_VAL("wrong exclude_user", evsel->core.attr.exclude_user);
-	return 0;
+	return TEST_OK;
 }
 
 static int count_tracepoints(void)
@@ -1521,7 +1521,7 @@ static int test__hybrid_hw_event_with_pmu(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->core.nr_entries);
 	TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong config", 0x3c == evsel->core.attr.config);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__hybrid_hw_group_event(struct evlist *evlist)
@@ -1538,7 +1538,7 @@ static int test__hybrid_hw_group_event(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong config", 0xc0 == evsel->core.attr.config);
 	TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
-	return 0;
+	return TEST_OK;
 }
 
 static int test__hybrid_sw_hw_group_event(struct evlist *evlist)
@@ -1554,7 +1554,7 @@ static int test__hybrid_sw_hw_group_event(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong config", 0x3c == evsel->core.attr.config);
 	TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
-	return 0;
+	return TEST_OK;
 }
 
 static int test__hybrid_hw_sw_group_event(struct evlist *evlist)
@@ -1570,7 +1570,7 @@ static int test__hybrid_hw_sw_group_event(struct evlist *evlist)
 	evsel = evsel__next(evsel);
 	TEST_ASSERT_VAL("wrong type", PERF_TYPE_SOFTWARE == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
-	return 0;
+	return TEST_OK;
 }
 
 static int test__hybrid_group_modifier1(struct evlist *evlist)
@@ -1591,7 +1591,7 @@ static int test__hybrid_group_modifier1(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong leader", evsel__has_leader(evsel, leader));
 	TEST_ASSERT_VAL("wrong exclude_user", !evsel->core.attr.exclude_user);
 	TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__hybrid_raw1(struct evlist *evlist)
@@ -1602,7 +1602,7 @@ static int test__hybrid_raw1(struct evlist *evlist)
 		TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->core.nr_entries);
 		TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type);
 		TEST_ASSERT_VAL("wrong config", 0x1a == evsel->core.attr.config);
-		return 0;
+		return TEST_OK;
 	}
 
 	TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->core.nr_entries);
@@ -1612,7 +1612,7 @@ static int test__hybrid_raw1(struct evlist *evlist)
 	/* The type of second event is randome value */
 	evsel = evsel__next(evsel);
 	TEST_ASSERT_VAL("wrong config", 0x1a == evsel->core.attr.config);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__hybrid_raw2(struct evlist *evlist)
@@ -1622,7 +1622,7 @@ static int test__hybrid_raw2(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->core.nr_entries);
 	TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong config", 0x1a == evsel->core.attr.config);
-	return 0;
+	return TEST_OK;
 }
 
 static int test__hybrid_cache_event(struct evlist *evlist)
@@ -1632,7 +1632,7 @@ static int test__hybrid_cache_event(struct evlist *evlist)
 	TEST_ASSERT_VAL("wrong number of entries", 1 == evlist->core.nr_entries);
 	TEST_ASSERT_VAL("wrong type", PERF_TYPE_HW_CACHE == evsel->core.attr.type);
 	TEST_ASSERT_VAL("wrong config", 0x2 == (evsel->core.attr.config & 0xffffffff));
-	return 0;
+	return TEST_OK;
 }
 
 struct evlist_test {
@@ -2043,20 +2043,24 @@ static int test_event(const struct evlist_test *e)
 	int ret;
 
 	if (e->valid && !e->valid()) {
-		pr_debug("... SKIP");
-		return 0;
+		pr_debug("... SKIP\n");
+		return TEST_OK;
 	}
 
 	evlist = evlist__new();
-	if (evlist == NULL)
-		return -ENOMEM;
-
+	if (evlist == NULL) {
+		pr_err("Failed allocation");
+		return TEST_FAIL;
+	}
 	parse_events_error__init(&err);
 	ret = parse_events(evlist, e->name, &err);
 	if (ret) {
 		pr_debug("failed to parse event '%s', err %d, str '%s'\n",
 			 e->name, ret, err.str);
 		parse_events_error__print(&err, e->name);
+		ret = TEST_FAIL;
+		if (strstr(err.str, "can't access trace events"))
+			ret = TEST_SKIP;
 	} else {
 		ret = e->check(evlist);
 	}
@@ -2091,21 +2095,37 @@ static int test_event_fake_pmu(const char *str)
 	return ret;
 }
 
+static int combine_test_results(int existing, int latest)
+{
+	if (existing == TEST_FAIL)
+		return TEST_FAIL;
+	if (existing == TEST_SKIP)
+		return latest == TEST_OK ? TEST_SKIP : latest;
+	return latest;
+}
+
 static int test_events(const struct evlist_test *events, int cnt)
 {
-	int ret1, ret2 = 0;
+	int ret = TEST_OK;
 
 	for (int i = 0; i < cnt; i++) {
 		const struct evlist_test *e = &events[i];
+		int test_ret;
 
-		pr_debug("running test %d '%s'", i, e->name);
-		ret1 = test_event(e);
-		if (ret1)
-			ret2 = ret1;
-		pr_debug("\n");
+		pr_debug("running test %d '%s'\n", i, e->name);
+		test_ret = test_event(e);
+		if (test_ret != TEST_OK) {
+			pr_debug("Event test failure: test %d '%s'", i, e->name);
+			ret = combine_test_results(ret, test_ret);
+		}
 	}
 
-	return ret2;
+	return ret;
+}
+
+static int test__events2(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
+{
+	return test_events(test__events, ARRAY_SIZE(test__events));
 }
 
 static int test_term(const struct terms_test *t)
@@ -2151,6 +2171,11 @@ static int test_terms(const struct terms_test *terms, int cnt)
 	return ret;
 }
 
+static int test__terms2(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
+{
+	return test_terms(test__terms, ARRAY_SIZE(test__terms));
+}
+
 static int test_pmu(void)
 {
 	struct stat st;
@@ -2166,7 +2191,7 @@ static int test_pmu(void)
 	return !ret;
 }
 
-static int test_pmu_events(void)
+static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
 {
 	struct stat st;
 	char path[PATH_MAX];
@@ -2174,24 +2199,29 @@ static int test_pmu_events(void)
 	DIR *dir;
 	int ret;
 
+	if (!test_pmu())
+		return TEST_SKIP;
+
 	snprintf(path, PATH_MAX, "%s/bus/event_source/devices/cpu/events/",
 		 sysfs__mountpoint());
 
 	ret = stat(path, &st);
 	if (ret) {
-		pr_debug("omitting PMU cpu events tests\n");
-		return 0;
+		pr_debug("omitting PMU cpu events tests: %s\n", path);
+		return TEST_OK;
 	}
 
 	dir = opendir(path);
 	if (!dir) {
-		pr_debug("can't open pmu event dir");
-		return -1;
+		pr_debug("can't open pmu event dir: %s\n", path);
+		return TEST_FAIL;
 	}
 
-	while (!ret && (ent = readdir(dir))) {
+	ret = TEST_OK;
+	while ((ent = readdir(dir))) {
 		struct evlist_test e = { .name = NULL, };
 		char name[2 * NAME_MAX + 1 + 12 + 3];
+		int test_ret;
 
 		/* Names containing . are special and cannot be used directly */
 		if (strchr(ent->d_name, '.'))
@@ -2202,19 +2232,33 @@ static int test_pmu_events(void)
 		e.name  = name;
 		e.check = test__checkevent_pmu_events;
 
-		ret = test_event(&e);
-		if (ret)
-			break;
+		test_ret = test_event(&e);
+		if (test_ret != TEST_OK) {
+			pr_debug("Test PMU event failed for '%s'", name);
+			ret = combine_test_results(ret, test_ret);
+		}
 		snprintf(name, sizeof(name), "%s:u,cpu/event=%s/u", ent->d_name, ent->d_name);
 		e.name  = name;
 		e.check = test__checkevent_pmu_events_mix;
-		ret = test_event(&e);
+		test_ret = test_event(&e);
+		if (test_ret != TEST_OK) {
+			pr_debug("Test PMU event failed for '%s'", name);
+			ret = combine_test_results(ret, test_ret);
+		}
 	}
 
 	closedir(dir);
 	return ret;
 }
 
+static int test__pmu_events2(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
+{
+	if (!test_pmu())
+		return TEST_SKIP;
+
+	return test_events(test__events_pmu, ARRAY_SIZE(test__events_pmu));
+}
+
 static bool test_alias(char **event, char **alias)
 {
 	char path[PATH_MAX];
@@ -2273,6 +2317,14 @@ static bool test_alias(char **event, char **alias)
 	return false;
 }
 
+static int test__hybrid(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
+{
+	if (!perf_pmu__has_hybrid())
+		return TEST_SKIP;
+
+	return test_events(test__hybrid_events, ARRAY_SIZE(test__hybrid_events));
+}
+
 static int test__checkevent_pmu_events_alias(struct evlist *evlist)
 {
 	struct evsel *evsel1 = evlist__first(evlist);
@@ -2280,10 +2332,10 @@ static int test__checkevent_pmu_events_alias(struct evlist *evlist)
 
 	TEST_ASSERT_VAL("wrong type", evsel1->core.attr.type == evsel2->core.attr.type);
 	TEST_ASSERT_VAL("wrong config", evsel1->core.attr.config == evsel2->core.attr.config);
-	return 0;
+	return TEST_OK;
 }
 
-static int test_pmu_events_alias(char *event, char *alias)
+static int test__pmu_events_alias(char *event, char *alias)
 {
 	struct evlist_test e = { .name = NULL, };
 	char name[2 * NAME_MAX + 20];
@@ -2296,72 +2348,63 @@ static int test_pmu_events_alias(char *event, char *alias)
 	return test_event(&e);
 }
 
-static int test_pmu_events_alias2(void)
+static int test__alias(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
 {
-	static const char events[][30] = {
-			"event-hyphen",
-			"event-two-hyph",
-	};
-	unsigned long i;
-	int ret = 0;
+	char *event, *alias;
+	int ret;
 
-	for (i = 0; i < ARRAY_SIZE(events); i++) {
-		ret = test_event_fake_pmu(&events[i][0]);
-		if (ret) {
-			pr_err("check_parse_fake %s failed\n", &events[i][0]);
-			break;
-		}
-	}
+	if (!test_alias(&event, &alias))
+		return TEST_SKIP;
 
+	ret = test__pmu_events_alias(event, alias);
+
+	free(event);
+	free(alias);
 	return ret;
 }
 
-static int test__parse_events(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
+static int test__pmu_events_alias2(struct test_suite *test __maybe_unused,
+				   int subtest __maybe_unused)
 {
-	int ret1, ret2 = 0;
-	char *event, *alias;
-
-#define TEST_EVENTS(tests)				\
-do {							\
-	ret1 = test_events(tests, ARRAY_SIZE(tests));	\
-	if (!ret2)					\
-		ret2 = ret1;				\
-} while (0)
-
-	if (perf_pmu__has_hybrid()) {
-		TEST_EVENTS(test__hybrid_events);
-		return ret2;
-	}
-
-	TEST_EVENTS(test__events);
-
-	if (test_pmu())
-		TEST_EVENTS(test__events_pmu);
-
-	if (test_pmu()) {
-		int ret = test_pmu_events();
-		if (ret)
-			return ret;
-	}
+	static const char events[][30] = {
+			"event-hyphen",
+			"event-two-hyph",
+	};
+	int ret = TEST_OK;
 
-	if (test_alias(&event, &alias)) {
-		int ret = test_pmu_events_alias(event, alias);
+	for (unsigned int i = 0; i < ARRAY_SIZE(events); i++) {
+		int test_ret = test_event_fake_pmu(&events[i][0]);
 
-		free(event);
-		free(alias);
-		if (ret)
-			return ret;
+		if (test_ret != TEST_OK) {
+			pr_debug("check_parse_fake %s failed\n", &events[i][0]);
+			ret = combine_test_results(ret, test_ret);
+		}
 	}
 
-	ret1 = test_pmu_events_alias2();
-	if (!ret2)
-		ret2 = ret1;
-
-	ret1 = test_terms(test__terms, ARRAY_SIZE(test__terms));
-	if (!ret2)
-		ret2 = ret1;
-
-	return ret2;
+	return ret;
 }
 
-DEFINE_SUITE("Parse event definition strings", parse_events);
+static struct test_case tests__parse_events[] = {
+	TEST_CASE_REASON("Test event parsing",
+			 events2,
+			 "permissions"),
+	TEST_CASE_REASON("Test parsing of \"hybrid\" CPU events",
+			 hybrid,
+			"not hybrid"),
+	TEST_CASE_REASON("Parsing of all PMU events from sysfs",
+			 pmu_events,
+			 "permissions"),
+	TEST_CASE_REASON("Parsing of given PMU events from sysfs",
+			 pmu_events2,
+			 "permissions"),
+	TEST_CASE_REASON("Parsing of aliased events from sysfs", alias,
+			 "no aliases in sysfs"),
+	TEST_CASE("Parsing of aliased events", pmu_events_alias2),
+	TEST_CASE("Parsing of terms (event modifiers)", terms2),
+	{	.name = NULL, }
+};
+
+struct test_suite suite__parse_events = {
+	.desc = "Parse event definition strings",
+	.test_cases = tests__parse_events,
+};
-- 
2.36.0.550.gb090851708-goog

Re: [PATCH 1/7] perf test: Skip reason for suites with 1 test

[John Garry]

On 13/05/2022 05:05, Ian Rogers wrote:
> When a suite has just 1 subtest, the subtest number is given as -1 to
> avoid indented printing. When this subtest number is seen for the skip
> reason, use the reason of the first test.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/tests/builtin-test.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index fac3717d9ba1..33fcafa0fa79 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -137,10 +137,10 @@ static bool has_subtests(const struct test_suite *t)
>   
>   static const char *skip_reason(const struct test_suite *t, int subtest)
>   {
> -	if (t->test_cases && subtest >= 0)
> -		return t->test_cases[subtest].skip_reason;
> +	if (!t->test_cases)
> +		return NULL;
>   
> -	return NULL;
> +	return t->test_cases[subtest >= 0 ? subtest : 0].skip_reason;
>   }

I was not sure which suite has a single tastcase, so I experimented for 
libpfm4 by deleting a testcase so it has only 1x remaining, I get:

before your change:
john@localhost:~/acme/tools/perf> sudo ./perf test 63
63: Test libpfm4 support : Skip

after:

john@localhost:~/acme/tools/perf> sudo ./perf test 63
63: Test libpfm4 support : Skip (not compiled in)

Although it is odd to have a single sub-test, is there a reason for 
which we don't print its name? We print the name when there are multiple 
sub-tests.

Thanks,
John

Re: [PATCH 1/7] perf test: Skip reason for suites with 1 test

[Ian Rogers]

On Fri, May 13, 2022 at 8:29 AM John Garry <john.garry@huawei.com> wrote:
>
> On 13/05/2022 05:05, Ian Rogers wrote:
> > When a suite has just 1 subtest, the subtest number is given as -1 to
> > avoid indented printing. When this subtest number is seen for the skip
> > reason, use the reason of the first test.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >   tools/perf/tests/builtin-test.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index fac3717d9ba1..33fcafa0fa79 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -137,10 +137,10 @@ static bool has_subtests(const struct test_suite *t)
> >
> >   static const char *skip_reason(const struct test_suite *t, int subtest)
> >   {
> > -     if (t->test_cases && subtest >= 0)
> > -             return t->test_cases[subtest].skip_reason;
> > +     if (!t->test_cases)
> > +             return NULL;
> >
> > -     return NULL;
> > +     return t->test_cases[subtest >= 0 ? subtest : 0].skip_reason;
> >   }
>
> I was not sure which suite has a single tastcase, so I experimented for
> libpfm4 by deleting a testcase so it has only 1x remaining, I get:
>
> before your change:
> john@localhost:~/acme/tools/perf> sudo ./perf test 63
> 63: Test libpfm4 support : Skip
>
> after:
>
> john@localhost:~/acme/tools/perf> sudo ./perf test 63
> 63: Test libpfm4 support : Skip (not compiled in)
>
> Although it is odd to have a single sub-test, is there a reason for
> which we don't print its name? We print the name when there are multiple
> sub-tests.

The reason was to replicate the existing "perf test" behavior before
the kunit style transition. The main place we get tests with a single
sub-test is from the DEFINE_SUITE macro:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/tests.h?h=perf/core#n67
I agree it looks kind of weird and was inheriting the data structures
from kunit and the format of the output from perf test.

Thanks,
Ian

> Thanks,
> John

Re: [PATCH 1/7] perf test: Skip reason for suites with 1 test

[John Garry]

On 13/05/2022 16:42, Ian Rogers wrote:
>> I was not sure which suite has a single tastcase, so I experimented for
>> libpfm4 by deleting a testcase so it has only 1x remaining, I get:
>>
>> before your change:
>> john@localhost:~/acme/tools/perf> sudo ./perf test 63
>> 63: Test libpfm4 support : Skip
>>
>> after:
>>
>> john@localhost:~/acme/tools/perf> sudo ./perf test 63
>> 63: Test libpfm4 support : Skip (not compiled in)
>>
>> Although it is odd to have a single sub-test, is there a reason for
>> which we don't print its name? We print the name when there are multiple
>> sub-tests.
> The reason was to replicate the existing "perf test" behavior before
> the kunit style transition. The main place we get tests with a single
> sub-test is from the DEFINE_SUITE macro:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/tests.h?h=perf/core#n67
> I agree it looks kind of weird and was inheriting the data structures
> from kunit and the format of the output from perf test.

Out of curiosity, which suite is this that you find only has a single 
subtest? Does it possibly only have a single subtest as some others may 
be compiled out?

Thanks,
John

Re: [PATCH 1/7] perf test: Skip reason for suites with 1 test

[Ian Rogers]

On Fri, May 13, 2022 at 9:26 AM John Garry <john.garry@huawei.com> wrote:
>
> On 13/05/2022 16:42, Ian Rogers wrote:
> >> I was not sure which suite has a single tastcase, so I experimented for
> >> libpfm4 by deleting a testcase so it has only 1x remaining, I get:
> >>
> >> before your change:
> >> john@localhost:~/acme/tools/perf> sudo ./perf test 63
> >> 63: Test libpfm4 support : Skip
> >>
> >> after:
> >>
> >> john@localhost:~/acme/tools/perf> sudo ./perf test 63
> >> 63: Test libpfm4 support : Skip (not compiled in)
> >>
> >> Although it is odd to have a single sub-test, is there a reason for
> >> which we don't print its name? We print the name when there are multiple
> >> sub-tests.
> > The reason was to replicate the existing "perf test" behavior before
> > the kunit style transition. The main place we get tests with a single
> > sub-test is from the DEFINE_SUITE macro:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/tests.h?h=perf/core#n67
> > I agree it looks kind of weird and was inheriting the data structures
> > from kunit and the format of the output from perf test.
>
> Out of curiosity, which suite is this that you find only has a single
> subtest? Does it possibly only have a single subtest as some others may
> be compiled out?

I was getting it when I added a skip message to the openat syscall
tests in patch 3:
https://lore.kernel.org/lkml/20220513040519.1499333-4-irogers@google.com/

I didn't see any changes with any existing tests.

Thanks,
Ian

> Thanks,
> John

Re: [PATCH 1/7] perf test: Skip reason for suites with 1 test

[John Garry]

On 13/05/2022 17:34, Ian Rogers wrote:
>>>> john@localhost:~/acme/tools/perf> sudo ./perf test 63
>>>> 63: Test libpfm4 support : Skip (not compiled in)
>>>>
>>>> Although it is odd to have a single sub-test, is there a reason for
>>>> which we don't print its name? We print the name when there are multiple
>>>> sub-tests.
>>> The reason was to replicate the existing "perf test" behavior before
>>> the kunit style transition. The main place we get tests with a single
>>> sub-test is from the DEFINE_SUITE macro:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/tests/tests.h?h=perf/core#n67
>>> I agree it looks kind of weird and was inheriting the data structures
>>> from kunit and the format of the output from perf test.
>> Out of curiosity, which suite is this that you find only has a single
>> subtest? Does it possibly only have a single subtest as some others may
>> be compiled out?
> I was getting it when I added a skip message to the openat syscall
> tests in patch 3:
> https://lore.kernel.org/lkml/20220513040519.1499333-4-irogers@google.com/
> 
> I didn't see any changes with any existing tests.

I suppose it works when the suite has the same name as the subtest, so, 
FWIW:

Reviewed-by: John Garry <john.garry@huawei.com>

Re: [PATCH 2/7] perf test: Use skip in vmlinux kallsyms

[John Garry]

On 13/05/2022 05:05, Ian Rogers wrote:
> Currently failures in reading vmlinux or kallsyms result in a test
> failure. However, the failure is typically permission related. 

If the user requires root priviledges then should we just mention this 
would be a problem for this test?

> Prefer to
> flag these failures as skip.
> 
> Signed-off-by: Ian Rogers<irogers@google.com>

Thanks,
John

Re: [PATCH 2/7] perf test: Use skip in vmlinux kallsyms

[Ian Rogers]

On Fri, May 13, 2022 at 10:01 AM John Garry <john.garry@huawei.com> wrote:
>
> On 13/05/2022 05:05, Ian Rogers wrote:
> > Currently failures in reading vmlinux or kallsyms result in a test
> > failure. However, the failure is typically permission related.
>
> If the user requires root priviledges then should we just mention this
> would be a problem for this test?

Tbh, it wasn't clear to me and so I didn't add a reason for skipping.

Thanks,
Ian

> > Prefer to
> > flag these failures as skip.
> >
> > Signed-off-by: Ian Rogers<irogers@google.com>
>
> Thanks,
> John

Re: [PATCH 2/7] perf test: Use skip in vmlinux kallsyms

[Namhyung Kim]

Hi Ian,

On Thu, May 12, 2022 at 9:05 PM Ian Rogers <irogers@google.com> wrote:
>
> Currently failures in reading vmlinux or kallsyms result in a test
> failure. However, the failure is typically permission related. Prefer to
> flag these failures as skip.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/vmlinux-kallsyms.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
> index 93dee542a177..983f32964749 100644
> --- a/tools/perf/tests/vmlinux-kallsyms.c
> +++ b/tools/perf/tests/vmlinux-kallsyms.c
> @@ -114,7 +114,7 @@ static bool is_ignored_symbol(const char *name, char type)
>  static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused,
>                                         int subtest __maybe_unused)
>  {
> -       int err = -1;
> +       int err = TEST_FAIL;
>         struct rb_node *nd;
>         struct symbol *sym;
>         struct map *kallsyms_map, *vmlinux_map, *map;
> @@ -142,7 +142,8 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
>          * and find the .ko files that match them in /lib/modules/`uname -r`/.
>          */
>         if (machine__create_kernel_maps(&kallsyms) < 0) {
> -               pr_debug("machine__create_kernel_maps ");
> +               pr_info("machine__create_kernel_maps failed");
> +               err = TEST_SKIP;
>                 goto out;
>         }
>
> @@ -158,7 +159,8 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
>          * code and with the one got from /proc/modules from the "kallsyms" code.
>          */
>         if (machine__load_kallsyms(&kallsyms, "/proc/kallsyms") <= 0) {
> -               pr_debug("dso__load_kallsyms ");
> +               pr_debug("machine__load_kallsyms failed");

For consistency, you might want to use pr_info() here.

Thanks,
Namhyung


> +               err = TEST_SKIP;
>                 goto out;
>         }
>
> @@ -178,7 +180,7 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
>          * Now repeat step 2, this time for the vmlinux file we'll auto-locate.
>          */
>         if (machine__create_kernel_maps(&vmlinux) < 0) {
> -               pr_debug("machine__create_kernel_maps ");
> +               pr_info("machine__create_kernel_maps failed");
>                 goto out;
>         }
>
> @@ -196,7 +198,7 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
>          * to fixup the symbols.
>          */
>         if (machine__load_vmlinux_path(&vmlinux) <= 0) {
> -               pr_debug("Couldn't find a vmlinux that matches the kernel running on this machine, skipping test\n");
> +               pr_info("Couldn't find a vmlinux that matches the kernel running on this machine, skipping test\n");
>                 err = TEST_SKIP;
>                 goto out;
>         }
> --
> 2.36.0.550.gb090851708-goog
>

Re: [PATCH 4/7] perf test: Basic mmap use skip

[Namhyung Kim]

On Thu, May 12, 2022 at 9:05 PM Ian Rogers <irogers@google.com> wrote:
>
> If opening the first event fails for basic mmap it is more likely
> permission related that a true error. Mark the test as skip in this
> case and add a skip reason.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/mmap-basic.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
> index c3c17600f29c..32f0a63fa157 100644
> --- a/tools/perf/tests/mmap-basic.c
> +++ b/tools/perf/tests/mmap-basic.c
> @@ -31,7 +31,7 @@
>   */
>  static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
>  {
> -       int err = -1;
> +       int err = TEST_FAIL;
>         union perf_event *event;
>         struct perf_thread_map *threads;
>         struct perf_cpu_map *cpus;
> @@ -83,6 +83,14 @@ static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest
>                 evsels[i] = evsel__newtp("syscalls", name);
>                 if (IS_ERR(evsels[i])) {
>                         pr_debug("evsel__new(%s)\n", name);
> +                       if (i == 0) {
> +                               /*
> +                                * Failure to open first event is more likely
> +                                * related to permissions so flag the failure as
> +                                * a skip.
> +                                */
> +                               err = TEST_SKIP;

This is not about perf_event_open() but it accesses
tracefs to get the type number of the trace event.

The evsel__newtp() seems to return the errno in a
negative number.  It'd be better to check if it's
-EACCES actually.

Thanks,
Namhyung


> +                       }
>                         goto out_delete_evlist;
>                 }
>
> @@ -166,4 +174,14 @@ static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest
>         return err;
>  }
>
> -DEFINE_SUITE("Read samples using the mmap interface", basic_mmap);
> +static struct test_case tests__basic_mmap[] = {
> +       TEST_CASE_REASON("Read samples using the mmap interface",
> +                        basic_mmap,
> +                        "permissions"),
> +       {       .name = NULL, }
> +};
> +
> +struct test_suite suite__basic_mmap = {
> +       .desc = "Read samples using the mmap interface",
> +       .test_cases = tests__basic_mmap,
> +};
> --
> 2.36.0.550.gb090851708-goog
>

Re: [PATCH 1/7] perf test: Skip reason for suites with 1 test

[Namhyung Kim]

On Thu, May 12, 2022 at 9:05 PM Ian Rogers <irogers@google.com> wrote:
>
> When a suite has just 1 subtest, the subtest number is given as -1 to
> avoid indented printing. When this subtest number is seen for the skip
> reason, use the reason of the first test.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> ---
>  tools/perf/tests/builtin-test.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index fac3717d9ba1..33fcafa0fa79 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -137,10 +137,10 @@ static bool has_subtests(const struct test_suite *t)
>
>  static const char *skip_reason(const struct test_suite *t, int subtest)
>  {
> -       if (t->test_cases && subtest >= 0)
> -               return t->test_cases[subtest].skip_reason;
> +       if (!t->test_cases)
> +               return NULL;
>
> -       return NULL;
> +       return t->test_cases[subtest >= 0 ? subtest : 0].skip_reason;
>  }
>
>  static const char *test_description(const struct test_suite *t, int subtest)
> --
> 2.36.0.550.gb090851708-goog
>

Re: [PATCH 2/7] perf test: Use skip in vmlinux kallsyms

[Ian Rogers]

On Mon, May 16, 2022 at 8:22 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Ian,
>
> On Thu, May 12, 2022 at 9:05 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Currently failures in reading vmlinux or kallsyms result in a test
> > failure. However, the failure is typically permission related. Prefer to
> > flag these failures as skip.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/tests/vmlinux-kallsyms.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
> > index 93dee542a177..983f32964749 100644
> > --- a/tools/perf/tests/vmlinux-kallsyms.c
> > +++ b/tools/perf/tests/vmlinux-kallsyms.c
> > @@ -114,7 +114,7 @@ static bool is_ignored_symbol(const char *name, char type)
> >  static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused,
> >                                         int subtest __maybe_unused)
> >  {
> > -       int err = -1;
> > +       int err = TEST_FAIL;
> >         struct rb_node *nd;
> >         struct symbol *sym;
> >         struct map *kallsyms_map, *vmlinux_map, *map;
> > @@ -142,7 +142,8 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
> >          * and find the .ko files that match them in /lib/modules/`uname -r`/.
> >          */
> >         if (machine__create_kernel_maps(&kallsyms) < 0) {
> > -               pr_debug("machine__create_kernel_maps ");
> > +               pr_info("machine__create_kernel_maps failed");
> > +               err = TEST_SKIP;
> >                 goto out;
> >         }
> >
> > @@ -158,7 +159,8 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
> >          * code and with the one got from /proc/modules from the "kallsyms" code.
> >          */
> >         if (machine__load_kallsyms(&kallsyms, "/proc/kallsyms") <= 0) {
> > -               pr_debug("dso__load_kallsyms ");
> > +               pr_debug("machine__load_kallsyms failed");
>
> For consistency, you might want to use pr_info() here.

Point taken, I think pr_debug is more consistent with other tests and
so I changed the above back in v2.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> > +               err = TEST_SKIP;
> >                 goto out;
> >         }
> >
> > @@ -178,7 +180,7 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
> >          * Now repeat step 2, this time for the vmlinux file we'll auto-locate.
> >          */
> >         if (machine__create_kernel_maps(&vmlinux) < 0) {
> > -               pr_debug("machine__create_kernel_maps ");
> > +               pr_info("machine__create_kernel_maps failed");
> >                 goto out;
> >         }
> >
> > @@ -196,7 +198,7 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
> >          * to fixup the symbols.
> >          */
> >         if (machine__load_vmlinux_path(&vmlinux) <= 0) {
> > -               pr_debug("Couldn't find a vmlinux that matches the kernel running on this machine, skipping test\n");
> > +               pr_info("Couldn't find a vmlinux that matches the kernel running on this machine, skipping test\n");
> >                 err = TEST_SKIP;
> >                 goto out;
> >         }
> > --
> > 2.36.0.550.gb090851708-goog
> >

Re: [PATCH 4/7] perf test: Basic mmap use skip

[Ian Rogers]

On Mon, May 16, 2022 at 8:43 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, May 12, 2022 at 9:05 PM Ian Rogers <irogers@google.com> wrote:
> >
> > If opening the first event fails for basic mmap it is more likely
> > permission related that a true error. Mark the test as skip in this
> > case and add a skip reason.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/tests/mmap-basic.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
> > index c3c17600f29c..32f0a63fa157 100644
> > --- a/tools/perf/tests/mmap-basic.c
> > +++ b/tools/perf/tests/mmap-basic.c
> > @@ -31,7 +31,7 @@
> >   */
> >  static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> >  {
> > -       int err = -1;
> > +       int err = TEST_FAIL;
> >         union perf_event *event;
> >         struct perf_thread_map *threads;
> >         struct perf_cpu_map *cpus;
> > @@ -83,6 +83,14 @@ static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest
> >                 evsels[i] = evsel__newtp("syscalls", name);
> >                 if (IS_ERR(evsels[i])) {
> >                         pr_debug("evsel__new(%s)\n", name);
> > +                       if (i == 0) {
> > +                               /*
> > +                                * Failure to open first event is more likely
> > +                                * related to permissions so flag the failure as
> > +                                * a skip.
> > +                                */
> > +                               err = TEST_SKIP;
>
> This is not about perf_event_open() but it accesses
> tracefs to get the type number of the trace event.
>
> The evsel__newtp() seems to return the errno in a
> negative number.  It'd be better to check if it's
> -EACCES actually.

Thanks, done in v2.

Ian

> Thanks,
> Namhyung
>
>
> > +                       }
> >                         goto out_delete_evlist;
> >                 }
> >
> > @@ -166,4 +174,14 @@ static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest
> >         return err;
> >  }
> >
> > -DEFINE_SUITE("Read samples using the mmap interface", basic_mmap);
> > +static struct test_case tests__basic_mmap[] = {
> > +       TEST_CASE_REASON("Read samples using the mmap interface",
> > +                        basic_mmap,
> > +                        "permissions"),
> > +       {       .name = NULL, }
> > +};
> > +
> > +struct test_suite suite__basic_mmap = {
> > +       .desc = "Read samples using the mmap interface",
> > +       .test_cases = tests__basic_mmap,
> > +};
> > --
> > 2.36.0.550.gb090851708-goog
> >