2022-12-06 04:37:51

by Ravi Bangoria

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

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


2022-12-06 07:01:29

by Madhavan Srinivasan

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


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

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

2022-12-06 17:58:25

by Athira Rajeev

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



> On 06-Dec-2022, at 10:02 AM, Ravi Bangoria <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
---
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
>

2022-12-07 04:43:24

by Ravi Bangoria

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

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

2022-12-07 06:26:10

by Ravi Bangoria

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

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

2022-12-07 06:53:55

by Madhavan Srinivasan

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


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