[PATCH v3 0/2] perf test: Add event group test

[PATCH v3 0/2] perf test: Add event group test

[Ravi Bangoria]

Multiple events in a group can belong to one or more pmus, however
there are some limitations to it. One of the limitation is, perf
doesn't allow creating a group of events from different hw pmus.
Write a simple test to create various combinations of hw, sw and
uncore pmu events and verify group creation succeeds or fails as
expected.

v2: https://lore.kernel.org/r/20221129111946.409-1-ravi.bangoria@amd.com
v2->v3:
 - Define a set of uncore pmus that supports more than 3 events. This
   will prevent false negative results by not picking random uncore
   pmu. Test will be skipped if no uncore pmu found.

Ravi Bangoria (2):
  perf tool: Move pmus list variable to new a file
  perf test: Add event group test

 tools/perf/tests/Build          |   1 +
 tools/perf/tests/builtin-test.c |   1 +
 tools/perf/tests/event_groups.c | 127 ++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h        |   1 +
 tools/perf/util/Build           |   1 +
 tools/perf/util/pmu.c           |   2 +-
 tools/perf/util/pmus.c          |   5 ++
 tools/perf/util/pmus.h          |   9 +++
 8 files changed, 146 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/tests/event_groups.c
 create mode 100644 tools/perf/util/pmus.c
 create mode 100644 tools/perf/util/pmus.h

-- 
2.38.1

[PATCH v3 1/2] perf tool: Move pmus list variable to new a file

[Ravi Bangoria]

pmus list variable is defined as static variable under pmu.c file.
Introduce new file pmus.c and migrate this variable to it. Also make
it non static so that it can be accessed from outside.

Suggested-by: Ian Rogers <irogers@google.com>
Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 tools/perf/util/Build  | 1 +
 tools/perf/util/pmu.c  | 2 +-
 tools/perf/util/pmus.c | 5 +++++
 tools/perf/util/pmus.h | 9 +++++++++
 4 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/util/pmus.c
 create mode 100644 tools/perf/util/pmus.h

diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index ab37f588ee8b..d04802bfa23f 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -73,6 +73,7 @@ perf-y += trace-event-parse.o
 perf-y += parse-events-flex.o
 perf-y += parse-events-bison.o
 perf-y += pmu.o
+perf-y += pmus.o
 perf-y += pmu-flex.o
 perf-y += pmu-bison.o
 perf-y += pmu-hybrid.o
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index e9a4f31926bf..f5e10f41c042 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -22,6 +22,7 @@
 #include "debug.h"
 #include "evsel.h"
 #include "pmu.h"
+#include "pmus.h"
 #include "parse-events.h"
 #include "print-events.h"
 #include "header.h"
@@ -58,7 +59,6 @@ struct perf_pmu_format {
 int perf_pmu_parse(struct list_head *list, char *name);
 extern FILE *perf_pmu_in;
 
-static LIST_HEAD(pmus);
 static bool hybrid_scanned;
 
 /*
diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
new file mode 100644
index 000000000000..7f3b93c4d229
--- /dev/null
+++ b/tools/perf/util/pmus.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/list.h>
+#include <pmus.h>
+
+LIST_HEAD(pmus);
diff --git a/tools/perf/util/pmus.h b/tools/perf/util/pmus.h
new file mode 100644
index 000000000000..5ec12007eb5c
--- /dev/null
+++ b/tools/perf/util/pmus.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __PMUS_H
+#define __PMUS_H
+
+extern struct list_head pmus;
+
+#define perf_pmus__for_each_pmu(pmu) list_for_each_entry(pmu, &pmus, list)
+
+#endif /* __PMUS_H */
-- 
2.38.1

[PATCH v3 2/2] perf test: Add event group test

[Ravi Bangoria]

Multiple events in a group can belong to one or more pmus, however
there are some limitations to it. One of the limitation is, perf
doesn't allow creating a group of events from different hw pmus.
Write a simple test to create various combinations of hw, sw and
uncore pmu events and verify group creation succeeds or fails as
expected.

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 tools/perf/tests/Build          |   1 +
 tools/perf/tests/builtin-test.c |   1 +
 tools/perf/tests/event_groups.c | 127 ++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h        |   1 +
 4 files changed, 130 insertions(+)
 create mode 100644 tools/perf/tests/event_groups.c

diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index 11b69023011b..658b5052c24d 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -67,6 +67,7 @@ perf-y += expand-cgroup.o
 perf-y += perf-time-to-tsc.o
 perf-y += dlfilter-test.o
 perf-y += sigtrap.o
+perf-y += event_groups.o
 
 $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
 	$(call rule_mkdir)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 4c6ae59a4dfd..ddd8262bfa26 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -110,6 +110,7 @@ static struct test_suite *generic_tests[] = {
 	&suite__perf_time_to_tsc,
 	&suite__dlfilter,
 	&suite__sigtrap,
+	&suite__event_groups,
 	NULL,
 };
 
diff --git a/tools/perf/tests/event_groups.c b/tools/perf/tests/event_groups.c
new file mode 100644
index 000000000000..612c0444aaa8
--- /dev/null
+++ b/tools/perf/tests/event_groups.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <string.h>
+#include <unistd.h>
+#include <stdio.h>
+#include "linux/perf_event.h"
+#include "tests.h"
+#include "debug.h"
+#include "pmu.h"
+#include "pmus.h"
+#include "header.h"
+#include "../perf-sys.h"
+
+/* hw: cycles, sw: context-switch, uncore: [arch dependent] */
+static int types[] = {0, 1, -1};
+static unsigned long configs[] = {0, 3, 0};
+
+#define NR_UNCORE_PMUS 5
+
+/* Uncore pmus that support more than 3 counters */
+static struct uncore_pmus {
+	const char *name;
+	__u64 config;
+} uncore_pmus[NR_UNCORE_PMUS] = {
+	{ "amd_l3", 0x0 },
+	{ "amd_df", 0x0 },
+	{ "uncore_imc_0", 0x1 },         /* Intel */
+	{ "core_imc", 0x318 },           /* PowerPC: core_imc/CPM_STCX_FIN/ */
+	{ "hv_24x7", 0x22000000003 },    /* PowerPC: hv_24x7/CPM_STCX_FIN/ */
+};
+
+static int event_open(int type, unsigned long config, int group_fd)
+{
+	struct perf_event_attr attr;
+
+	memset(&attr, 0, sizeof(struct perf_event_attr));
+	attr.type = type;
+	attr.size = sizeof(struct perf_event_attr);
+	attr.config = config;
+	/*
+	 * When creating an event group, typically the group leader is
+	 * initialized with disabled set to 1 and any child events are
+	 * initialized with disabled set to 0. Despite disabled being 0,
+	 * the child events will not start until the group leader is
+	 * enabled.
+	 */
+	attr.disabled = group_fd == -1 ? 1 : 0;
+
+	return sys_perf_event_open(&attr, -1, 0, group_fd, 0);
+}
+
+static int setup_uncore_event(void)
+{
+	struct perf_pmu *pmu;
+	int i;
+
+	if (list_empty(&pmus))
+		perf_pmu__scan(NULL);
+
+	perf_pmus__for_each_pmu(pmu) {
+		for (i = 0; i < NR_UNCORE_PMUS; i++) {
+			if (!strcmp(uncore_pmus[i].name, pmu->name)) {
+				pr_debug("Using %s for uncore pmu event\n", pmu->name);
+				types[2] = pmu->type;
+				configs[2] = uncore_pmus[i].config;
+				return 0;
+			}
+		}
+	}
+	return -1;
+}
+
+static int run_test(int i, int j, int k)
+{
+	int erroneous = ((((1 << i) | (1 << j) | (1 << k)) & 5) == 5);
+	int group_fd, sibling_fd1, sibling_fd2;
+
+	group_fd = event_open(types[i], configs[i], -1);
+	if (group_fd == -1)
+		return -1;
+
+	sibling_fd1 = event_open(types[j], configs[j], group_fd);
+	if (sibling_fd1 == -1) {
+		close(group_fd);
+		return erroneous ? 0 : -1;
+	}
+
+	sibling_fd2 = event_open(types[k], configs[k], group_fd);
+	if (sibling_fd2 == -1) {
+		close(sibling_fd1);
+		close(group_fd);
+		return erroneous ? 0 : -1;
+	}
+
+	close(sibling_fd2);
+	close(sibling_fd1);
+	close(group_fd);
+	return erroneous ? -1 : 0;
+}
+
+static int test__event_groups(struct test_suite *text __maybe_unused, int subtest __maybe_unused)
+{
+	int i, j, k;
+	int ret;
+	int r;
+
+	ret = setup_uncore_event();
+	if (ret || types[2] == -1)
+		return TEST_SKIP;
+
+	ret = TEST_OK;
+	for (i = 0; i < 3; i++) {
+		for (j = 0; j < 3; j++) {
+			for (k = 0; k < 3; k++) {
+				r = run_test(i, j, k);
+				if (r)
+					ret = TEST_FAIL;
+
+				pr_debug("0x%x 0x%lx, 0x%x 0x%lx, 0x%x 0x%lx: %s\n",
+					 types[i], configs[i], types[j], configs[j],
+					 types[k], configs[k], r ? "Fail" : "Pass");
+			}
+		}
+	}
+	return ret;
+}
+
+DEFINE_SUITE("Event groups", event_groups);
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index e15f24cfc909..fb4b5ad4dd0f 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -147,6 +147,7 @@ DECLARE_SUITE(expand_cgroup_events);
 DECLARE_SUITE(perf_time_to_tsc);
 DECLARE_SUITE(dlfilter);
 DECLARE_SUITE(sigtrap);
+DECLARE_SUITE(event_groups);
 
 /*
  * PowerPC and S390 do not support creation of instruction breakpoints using the
-- 
2.38.1

Re: [PATCH v3 2/2] perf test: Add event group test

[Madhavan Srinivasan]

On 12/6/22 10:02 AM, Ravi Bangoria wrote:
> Multiple events in a group can belong to one or more pmus, however
> there are some limitations to it. One of the limitation is, perf
> doesn't allow creating a group of events from different hw pmus.
> Write a simple test to create various combinations of hw, sw and
> uncore pmu events and verify group creation succeeds or fails as
> expected.
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
>   tools/perf/tests/Build          |   1 +
>   tools/perf/tests/builtin-test.c |   1 +
>   tools/perf/tests/event_groups.c | 127 ++++++++++++++++++++++++++++++++
>   tools/perf/tests/tests.h        |   1 +
>   4 files changed, 130 insertions(+)
>   create mode 100644 tools/perf/tests/event_groups.c
>
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index 11b69023011b..658b5052c24d 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -67,6 +67,7 @@ perf-y += expand-cgroup.o
>   perf-y += perf-time-to-tsc.o
>   perf-y += dlfilter-test.o
>   perf-y += sigtrap.o
> +perf-y += event_groups.o
>   
>   $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
>   	$(call rule_mkdir)
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 4c6ae59a4dfd..ddd8262bfa26 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -110,6 +110,7 @@ static struct test_suite *generic_tests[] = {
>   	&suite__perf_time_to_tsc,
>   	&suite__dlfilter,
>   	&suite__sigtrap,
> +	&suite__event_groups,
>   	NULL,
>   };
>   
> diff --git a/tools/perf/tests/event_groups.c b/tools/perf/tests/event_groups.c
> new file mode 100644
> index 000000000000..612c0444aaa8
> --- /dev/null
> +++ b/tools/perf/tests/event_groups.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <string.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +#include "linux/perf_event.h"
> +#include "tests.h"
> +#include "debug.h"
> +#include "pmu.h"
> +#include "pmus.h"
> +#include "header.h"
> +#include "../perf-sys.h"
> +
> +/* hw: cycles, sw: context-switch, uncore: [arch dependent] */
> +static int types[] = {0, 1, -1};
> +static unsigned long configs[] = {0, 3, 0};
> +
> +#define NR_UNCORE_PMUS 5
> +
> +/* Uncore pmus that support more than 3 counters */
> +static struct uncore_pmus {
> +	const char *name;
> +	__u64 config;
> +} uncore_pmus[NR_UNCORE_PMUS] = {
> +	{ "amd_l3", 0x0 },
> +	{ "amd_df", 0x0 },
> +	{ "uncore_imc_0", 0x1 },         /* Intel */
> +	{ "core_imc", 0x318 },           /* PowerPC: core_imc/CPM_STCX_FIN/ */
> +	{ "hv_24x7", 0x22000000003 },    /* PowerPC: hv_24x7/CPM_STCX_FIN/ */


For Powerpc event config values
Acked-by: Madhavan Srinivasan <maddy@linux.ibm.com>

> +};
> +
> +static int event_open(int type, unsigned long config, int group_fd)
> +{
> +	struct perf_event_attr attr;
> +
> +	memset(&attr, 0, sizeof(struct perf_event_attr));
> +	attr.type = type;
> +	attr.size = sizeof(struct perf_event_attr);
> +	attr.config = config;
> +	/*
> +	 * When creating an event group, typically the group leader is
> +	 * initialized with disabled set to 1 and any child events are
> +	 * initialized with disabled set to 0. Despite disabled being 0,
> +	 * the child events will not start until the group leader is
> +	 * enabled.
> +	 */
> +	attr.disabled = group_fd == -1 ? 1 : 0;
> +
> +	return sys_perf_event_open(&attr, -1, 0, group_fd, 0);
> +}
> +
> +static int setup_uncore_event(void)
> +{
> +	struct perf_pmu *pmu;
> +	int i;
> +
> +	if (list_empty(&pmus))
> +		perf_pmu__scan(NULL);
> +
> +	perf_pmus__for_each_pmu(pmu) {
> +		for (i = 0; i < NR_UNCORE_PMUS; i++) {
> +			if (!strcmp(uncore_pmus[i].name, pmu->name)) {
> +				pr_debug("Using %s for uncore pmu event\n", pmu->name);
> +				types[2] = pmu->type;
> +				configs[2] = uncore_pmus[i].config;
> +				return 0;
> +			}
> +		}
> +	}
> +	return -1;
> +}
> +
> +static int run_test(int i, int j, int k)
> +{
> +	int erroneous = ((((1 << i) | (1 << j) | (1 << k)) & 5) == 5);
> +	int group_fd, sibling_fd1, sibling_fd2;
> +
> +	group_fd = event_open(types[i], configs[i], -1);
> +	if (group_fd == -1)
> +		return -1;
> +
> +	sibling_fd1 = event_open(types[j], configs[j], group_fd);
> +	if (sibling_fd1 == -1) {
> +		close(group_fd);
> +		return erroneous ? 0 : -1;
> +	}
> +
> +	sibling_fd2 = event_open(types[k], configs[k], group_fd);
> +	if (sibling_fd2 == -1) {
> +		close(sibling_fd1);
> +		close(group_fd);
> +		return erroneous ? 0 : -1;
> +	}
> +
> +	close(sibling_fd2);
> +	close(sibling_fd1);
> +	close(group_fd);
> +	return erroneous ? -1 : 0;
> +}
> +
> +static int test__event_groups(struct test_suite *text __maybe_unused, int subtest __maybe_unused)
> +{
> +	int i, j, k;
> +	int ret;
> +	int r;
> +
> +	ret = setup_uncore_event();
> +	if (ret || types[2] == -1)
> +		return TEST_SKIP;
> +
> +	ret = TEST_OK;
> +	for (i = 0; i < 3; i++) {
> +		for (j = 0; j < 3; j++) {
> +			for (k = 0; k < 3; k++) {
> +				r = run_test(i, j, k);
> +				if (r)
> +					ret = TEST_FAIL;
> +
> +				pr_debug("0x%x 0x%lx, 0x%x 0x%lx, 0x%x 0x%lx: %s\n",
> +					 types[i], configs[i], types[j], configs[j],
> +					 types[k], configs[k], r ? "Fail" : "Pass");
> +			}
> +		}
> +	}
> +	return ret;
> +}
> +
> +DEFINE_SUITE("Event groups", event_groups);
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index e15f24cfc909..fb4b5ad4dd0f 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -147,6 +147,7 @@ DECLARE_SUITE(expand_cgroup_events);
>   DECLARE_SUITE(perf_time_to_tsc);
>   DECLARE_SUITE(dlfilter);
>   DECLARE_SUITE(sigtrap);
> +DECLARE_SUITE(event_groups);
>   
>   /*
>    * PowerPC and S390 do not support creation of instruction breakpoints using the

Re: [PATCH v3 2/2] perf test: Add event group test

[Athira Rajeev]

> On 06-Dec-2022, at 10:02 AM, Ravi Bangoria <ravi.bangoria@amd.com> wrote:
> 
> Multiple events in a group can belong to one or more pmus, however
> there are some limitations to it. One of the limitation is, perf
> doesn't allow creating a group of events from different hw pmus.
> Write a simple test to create various combinations of hw, sw and
> uncore pmu events and verify group creation succeeds or fails as
> expected.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
> tools/perf/tests/Build          |   1 +
> tools/perf/tests/builtin-test.c |   1 +
> tools/perf/tests/event_groups.c | 127 ++++++++++++++++++++++++++++++++
> tools/perf/tests/tests.h        |   1 +
> 4 files changed, 130 insertions(+)
> create mode 100644 tools/perf/tests/event_groups.c
> 
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index 11b69023011b..658b5052c24d 100644
> --- a/tools/perf/tests/Build
> +++ b/tools/perf/tests/Build
> @@ -67,6 +67,7 @@ perf-y += expand-cgroup.o
> perf-y += perf-time-to-tsc.o
> perf-y += dlfilter-test.o
> perf-y += sigtrap.o
> +perf-y += event_groups.o
> 
> $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
> 	$(call rule_mkdir)
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 4c6ae59a4dfd..ddd8262bfa26 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -110,6 +110,7 @@ static struct test_suite *generic_tests[] = {
> 	&suite__perf_time_to_tsc,
> 	&suite__dlfilter,
> 	&suite__sigtrap,
> +	&suite__event_groups,
> 	NULL,
> };
> 
> diff --git a/tools/perf/tests/event_groups.c b/tools/perf/tests/event_groups.c
> new file mode 100644
> index 000000000000..612c0444aaa8
> --- /dev/null
> +++ b/tools/perf/tests/event_groups.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <string.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +#include "linux/perf_event.h"
> +#include "tests.h"
> +#include "debug.h"
> +#include "pmu.h"
> +#include "pmus.h"
> +#include "header.h"
> +#include "../perf-sys.h"
> +
> +/* hw: cycles, sw: context-switch, uncore: [arch dependent] */
> +static int types[] = {0, 1, -1};
> +static unsigned long configs[] = {0, 3, 0};
> +
> +#define NR_UNCORE_PMUS 5
> +
> +/* Uncore pmus that support more than 3 counters */
> +static struct uncore_pmus {
> +	const char *name;
> +	__u64 config;
> +} uncore_pmus[NR_UNCORE_PMUS] = {
> +	{ "amd_l3", 0x0 },
> +	{ "amd_df", 0x0 },
> +	{ "uncore_imc_0", 0x1 },         /* Intel */
> +	{ "core_imc", 0x318 },           /* PowerPC: core_imc/CPM_STCX_FIN/ */
> +	{ "hv_24x7", 0x22000000003 },    /* PowerPC: hv_24x7/CPM_STCX_FIN/ */
> +};
> +
> +static int event_open(int type, unsigned long config, int group_fd)
> +{
> +	struct perf_event_attr attr;
> +
> +	memset(&attr, 0, sizeof(struct perf_event_attr));
> +	attr.type = type;
> +	attr.size = sizeof(struct perf_event_attr);
> +	attr.config = config;
> +	/*
> +	 * When creating an event group, typically the group leader is
> +	 * initialized with disabled set to 1 and any child events are
> +	 * initialized with disabled set to 0. Despite disabled being 0,
> +	 * the child events will not start until the group leader is
> +	 * enabled.
> +	 */
> +	attr.disabled = group_fd == -1 ? 1 : 0;
> +
> +	return sys_perf_event_open(&attr, -1, 0, group_fd, 0);
> +}
> +
> +static int setup_uncore_event(void)
> +{
> +	struct perf_pmu *pmu;
> +	int i;
> +
> +	if (list_empty(&pmus))
> +		perf_pmu__scan(NULL);
> +
> +	perf_pmus__for_each_pmu(pmu) {
> +		for (i = 0; i < NR_UNCORE_PMUS; i++) {
> +			if (!strcmp(uncore_pmus[i].name, pmu->name)) {
> +				pr_debug("Using %s for uncore pmu event\n", pmu->name);
> +				types[2] = pmu->type;
> +				configs[2] = uncore_pmus[i].config;

Hi Ravi,

Observed failure while running the test on powerpc. It is because the uncore PMU ie hv_24x7 needs
performance monitoring to be enabled in powerpc. So to handle such cases, can we add an “event_open" check before
proceeding with the test. Below is the change on top of “tmp.perf/core” .


From 8b33fb900c26beafc28f75b6f64631f8fdd045c2 Mon Sep 17 00:00:00 2001
From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Date: Tue, 6 Dec 2022 20:17:25 +0530
Subject: [PATCH] perf test: Update event group check for support of uncore
 event

Event group test checks group creation for combinations of
hw, sw and uncore PMU events. Some of the uncore pmu event
requires performance enablement explicitly. Example, hv_24x7
event in powerpc. Hence add a check to see if event_open
succeeds before proceeding.

Fixes: 5c88101b797d ("perf test: Add event group test for events in multiple PMUs")
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/tests/event_groups.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/event_groups.c b/tools/perf/tests/event_groups.c
index 612c0444aaa8..ad52e1da259a 100644
--- a/tools/perf/tests/event_groups.c
+++ b/tools/perf/tests/event_groups.c
@@ -51,7 +51,7 @@ static int event_open(int type, unsigned long config, int group_fd)
 static int setup_uncore_event(void)
 {
 	struct perf_pmu *pmu;
-	int i;
+	int i, fd;
 
 	if (list_empty(&pmus))
 		perf_pmu__scan(NULL);
@@ -62,6 +62,10 @@ static int setup_uncore_event(void)
 				pr_debug("Using %s for uncore pmu event\n", pmu->name);
 				types[2] = pmu->type;
 				configs[2] = uncore_pmus[i].config;
+				fd = event_open(types[2], configs[2], -1);
+				if (fd < 0)
+					return -1;
+				close(fd);
 				return 0;
 			}
 		}
-- 
2.31.1


Ravi, Please share your comments on this. If this looks fine, I can post this as separate fix patch.

Thanks
Athira
> +				return 0;
> +			}
> +		}
> +	}
> +	return -1;
> +}
> +
> +static int run_test(int i, int j, int k)
> +{
> +	int erroneous = ((((1 << i) | (1 << j) | (1 << k)) & 5) == 5);
> +	int group_fd, sibling_fd1, sibling_fd2;
> +
> +	group_fd = event_open(types[i], configs[i], -1);
> +	if (group_fd == -1)
> +		return -1;
> +
> +	sibling_fd1 = event_open(types[j], configs[j], group_fd);
> +	if (sibling_fd1 == -1) {
> +		close(group_fd);
> +		return erroneous ? 0 : -1;
> +	}
> +
> +	sibling_fd2 = event_open(types[k], configs[k], group_fd);
> +	if (sibling_fd2 == -1) {
> +		close(sibling_fd1);
> +		close(group_fd);
> +		return erroneous ? 0 : -1;
> +	}
> +
> +	close(sibling_fd2);
> +	close(sibling_fd1);
> +	close(group_fd);
> +	return erroneous ? -1 : 0;
> +}
> +
> +static int test__event_groups(struct test_suite *text __maybe_unused, int subtest __maybe_unused)
> +{
> +	int i, j, k;
> +	int ret;
> +	int r;
> +
> +	ret = setup_uncore_event();
> +	if (ret || types[2] == -1)
> +		return TEST_SKIP;
> +
> +	ret = TEST_OK;
> +	for (i = 0; i < 3; i++) {
> +		for (j = 0; j < 3; j++) {
> +			for (k = 0; k < 3; k++) {
> +				r = run_test(i, j, k);
> +				if (r)
> +					ret = TEST_FAIL;
> +
> +				pr_debug("0x%x 0x%lx, 0x%x 0x%lx, 0x%x 0x%lx: %s\n",
> +					 types[i], configs[i], types[j], configs[j],
> +					 types[k], configs[k], r ? "Fail" : "Pass");
> +			}
> +		}
> +	}
> +	return ret;
> +}
> +
> +DEFINE_SUITE("Event groups", event_groups);
> diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> index e15f24cfc909..fb4b5ad4dd0f 100644
> --- a/tools/perf/tests/tests.h
> +++ b/tools/perf/tests/tests.h
> @@ -147,6 +147,7 @@ DECLARE_SUITE(expand_cgroup_events);
> DECLARE_SUITE(perf_time_to_tsc);
> DECLARE_SUITE(dlfilter);
> DECLARE_SUITE(sigtrap);
> +DECLARE_SUITE(event_groups);
> 
> /*
>  * PowerPC and S390 do not support creation of instruction breakpoints using the
> -- 
> 2.38.1
> 

Re: [PATCH v3 2/2] perf test: Add event group test

[Ravi Bangoria]

Hi Athira,

>> +static int setup_uncore_event(void)
>> +{
>> +	struct perf_pmu *pmu;
>> +	int i;
>> +
>> +	if (list_empty(&pmus))
>> +		perf_pmu__scan(NULL);
>> +
>> +	perf_pmus__for_each_pmu(pmu) {
>> +		for (i = 0; i < NR_UNCORE_PMUS; i++) {
>> +			if (!strcmp(uncore_pmus[i].name, pmu->name)) {
>> +				pr_debug("Using %s for uncore pmu event\n", pmu->name);
>> +				types[2] = pmu->type;
>> +				configs[2] = uncore_pmus[i].config;
> 
> Hi Ravi,
> 
> Observed failure while running the test on powerpc. It is because the uncore PMU ie hv_24x7 needs
> performance monitoring to be enabled in powerpc. So to handle such cases, can we add an “event_open" check before
> proceeding with the test. Below is the change on top of “tmp.perf/core” .
> 
> 
> From 8b33fb900c26beafc28f75b6f64631f8fdd045c2 Mon Sep 17 00:00:00 2001
> From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Date: Tue, 6 Dec 2022 20:17:25 +0530
> Subject: [PATCH] perf test: Update event group check for support of uncore
>  event
> 
> Event group test checks group creation for combinations of
> hw, sw and uncore PMU events. Some of the uncore pmu event
> requires performance enablement explicitly.

You need to open an event to activate hv_24x7 pmu?

> Example, hv_24x7
> event in powerpc. Hence add a check to see if event_open
> succeeds before proceeding.
> 
> Fixes: 5c88101b797d ("perf test: Add event group test for events in multiple PMUs")
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
>  tools/perf/tests/event_groups.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/tests/event_groups.c b/tools/perf/tests/event_groups.c
> index 612c0444aaa8..ad52e1da259a 100644
> --- a/tools/perf/tests/event_groups.c
> +++ b/tools/perf/tests/event_groups.c
> @@ -51,7 +51,7 @@ static int event_open(int type, unsigned long config, int group_fd)
>  static int setup_uncore_event(void)
>  {
>  	struct perf_pmu *pmu;
> -	int i;
> +	int i, fd;
>  
>  	if (list_empty(&pmus))
>  		perf_pmu__scan(NULL);
> @@ -62,6 +62,10 @@ static int setup_uncore_event(void)
>  				pr_debug("Using %s for uncore pmu event\n", pmu->name);
>  				types[2] = pmu->type;
>  				configs[2] = uncore_pmus[i].config;

Sure. Just add a comment here to explain why are we opening a
standalone event here.

> +				fd = event_open(types[2], configs[2], -1);
> +				if (fd < 0)
> +					return -1;
> +				close(fd);
>  				return 0;
>  			}
>  		}

Thanks,
Ravi

Re: [PATCH v3 2/2] perf test: Add event group test

[Madhavan Srinivasan]

On 12/7/22 9:56 AM, Ravi Bangoria wrote:
> Hi Athira,
>
>>> +static int setup_uncore_event(void)
>>> +{
>>> +	struct perf_pmu *pmu;
>>> +	int i;
>>> +
>>> +	if (list_empty(&pmus))
>>> +		perf_pmu__scan(NULL);
>>> +
>>> +	perf_pmus__for_each_pmu(pmu) {
>>> +		for (i = 0; i < NR_UNCORE_PMUS; i++) {
>>> +			if (!strcmp(uncore_pmus[i].name, pmu->name)) {
>>> +				pr_debug("Using %s for uncore pmu event\n", pmu->name);
>>> +				types[2] = pmu->type;
>>> +				configs[2] = uncore_pmus[i].config;
>> Hi Ravi,
>>
>> Observed failure while running the test on powerpc. It is because the uncore PMU ie hv_24x7 needs
>> performance monitoring to be enabled in powerpc. So to handle such cases, can we add an “event_open" check before
>> proceeding with the test. Below is the change on top of “tmp.perf/core” .
>>
>>
>>  From 8b33fb900c26beafc28f75b6f64631f8fdd045c2 Mon Sep 17 00:00:00 2001
>> From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> Date: Tue, 6 Dec 2022 20:17:25 +0530
>> Subject: [PATCH] perf test: Update event group check for support of uncore
>>   event
>>
>> Event group test checks group creation for combinations of
>> hw, sw and uncore PMU events. Some of the uncore pmu event
>> requires performance enablement explicitly.
> You need to open an event to activate hv_24x7 pmu?
hv_24x7 pmu supports events which can also provide system-wide resource data
and partition should have permissions to access those, hence the check.

Maddy
>
>> Example, hv_24x7
>> event in powerpc. Hence add a check to see if event_open
>> succeeds before proceeding.
>>
>> Fixes: 5c88101b797d ("perf test: Add event group test for events in multiple PMUs")
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>>   tools/perf/tests/event_groups.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/tests/event_groups.c b/tools/perf/tests/event_groups.c
>> index 612c0444aaa8..ad52e1da259a 100644
>> --- a/tools/perf/tests/event_groups.c
>> +++ b/tools/perf/tests/event_groups.c
>> @@ -51,7 +51,7 @@ static int event_open(int type, unsigned long config, int group_fd)
>>   static int setup_uncore_event(void)
>>   {
>>   	struct perf_pmu *pmu;
>> -	int i;
>> +	int i, fd;
>>   
>>   	if (list_empty(&pmus))
>>   		perf_pmu__scan(NULL);
>> @@ -62,6 +62,10 @@ static int setup_uncore_event(void)
>>   				pr_debug("Using %s for uncore pmu event\n", pmu->name);
>>   				types[2] = pmu->type;
>>   				configs[2] = uncore_pmus[i].config;
> Sure. Just add a comment here to explain why are we opening a
> standalone event here.
>
>> +				fd = event_open(types[2], configs[2], -1);
>> +				if (fd < 0)
>> +					return -1;
>> +				close(fd);
>>   				return 0;
>>   			}
>>   		}
> Thanks,
> Ravi

Re: [PATCH v3 2/2] perf test: Add event group test

[Ravi Bangoria]

>>> Event group test checks group creation for combinations of
>>> hw, sw and uncore PMU events. Some of the uncore pmu event
>>> requires performance enablement explicitly.
>> You need to open an event to activate hv_24x7 pmu?
> 
> hv_24x7 pmu supports events which can also provide system-wide resource data
> and partition should have permissions to access those, hence the check.

Ah ok. Got it.

Thanks,
Ravi

Re: [PATCH v3 0/2] perf test: Add event group test

[Ian Rogers]

On Mon, Dec 5, 2022 at 8:33 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> Multiple events in a group can belong to one or more pmus, however
> there are some limitations to it. One of the limitation is, perf
> doesn't allow creating a group of events from different hw pmus.
> Write a simple test to create various combinations of hw, sw and
> uncore pmu events and verify group creation succeeds or fails as
> expected.
>
> v2: https://lore.kernel.org/r/20221129111946.409-1-ravi.bangoria@amd.com
> v2->v3:
>  - Define a set of uncore pmus that supports more than 3 events. This
>    will prevent false negative results by not picking random uncore
>    pmu. Test will be skipped if no uncore pmu found.

Acked-by: Ian Rogers <irogers@google.com>

Thanks!
Ian

> Ravi Bangoria (2):
>   perf tool: Move pmus list variable to new a file
>   perf test: Add event group test
>
>  tools/perf/tests/Build          |   1 +
>  tools/perf/tests/builtin-test.c |   1 +
>  tools/perf/tests/event_groups.c | 127 ++++++++++++++++++++++++++++++++
>  tools/perf/tests/tests.h        |   1 +
>  tools/perf/util/Build           |   1 +
>  tools/perf/util/pmu.c           |   2 +-
>  tools/perf/util/pmus.c          |   5 ++
>  tools/perf/util/pmus.h          |   9 +++
>  8 files changed, 146 insertions(+), 1 deletion(-)
>  create mode 100644 tools/perf/tests/event_groups.c
>  create mode 100644 tools/perf/util/pmus.c
>  create mode 100644 tools/perf/util/pmus.h
>
> --
> 2.38.1
>

Re: [PATCH v3 0/2] perf test: Add event group test

[Liang, Kan]

On 2022-12-05 11:32 p.m., Ravi Bangoria wrote:
> Multiple events in a group can belong to one or more pmus, however
> there are some limitations to it. One of the limitation is, perf
> doesn't allow creating a group of events from different hw pmus.
> Write a simple test to create various combinations of hw, sw and
> uncore pmu events and verify group creation succeeds or fails as
> expected.
> 
> v2: https://lore.kernel.org/r/20221129111946.409-1-ravi.bangoria@amd.com
> v2->v3:
>  - Define a set of uncore pmus that supports more than 3 events. This
>    will prevent false negative results by not picking random uncore
>    pmu. Test will be skipped if no uncore pmu found.
> 
> Ravi Bangoria (2):
>   perf tool: Move pmus list variable to new a file

a new file

Other than the above typo, the patch series looks good to me.

Acked-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan

>   perf test: Add event group test
> 
>  tools/perf/tests/Build          |   1 +
>  tools/perf/tests/builtin-test.c |   1 +
>  tools/perf/tests/event_groups.c | 127 ++++++++++++++++++++++++++++++++
>  tools/perf/tests/tests.h        |   1 +
>  tools/perf/util/Build           |   1 +
>  tools/perf/util/pmu.c           |   2 +-
>  tools/perf/util/pmus.c          |   5 ++
>  tools/perf/util/pmus.h          |   9 +++
>  8 files changed, 146 insertions(+), 1 deletion(-)
>  create mode 100644 tools/perf/tests/event_groups.c
>  create mode 100644 tools/perf/util/pmus.c
>  create mode 100644 tools/perf/util/pmus.h
> 

Re: [PATCH v3 0/2] perf test: Add event group test

[Arnaldo Carvalho de Melo]

Em Tue, Dec 06, 2022 at 09:47:57AM -0500, Liang, Kan escreveu:
> On 2022-12-05 11:32 p.m., Ravi Bangoria wrote:
> > Multiple events in a group can belong to one or more pmus, however
> > there are some limitations to it. One of the limitation is, perf
> > doesn't allow creating a group of events from different hw pmus.
> > Write a simple test to create various combinations of hw, sw and
> > uncore pmu events and verify group creation succeeds or fails as
> > expected.
> > 
> > v2: https://lore.kernel.org/r/20221129111946.409-1-ravi.bangoria@amd.com
> > v2->v3:
> >  - Define a set of uncore pmus that supports more than 3 events. This
> >    will prevent false negative results by not picking random uncore
> >    pmu. Test will be skipped if no uncore pmu found.
> > 
> > Ravi Bangoria (2):
> >   perf tool: Move pmus list variable to new a file
> 
> a new file
> 
> Other than the above typo, the patch series looks good to me.
> 
> Acked-by: Kan Liang <kan.liang@linux.intel.com>

Fixed that up, added your Acked-by, thanks.

- Arnaldo