[PATCH] perf test: Add event group test

[PATCH] 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. Basically, 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>
---
Note: Uncore pmu event detail for Intel, Arm, PowerPC and s390 needs
to be verified/fixed. These are marked as XXX in the patch.

 tools/perf/tests/Build          |   1 +
 tools/perf/tests/builtin-test.c |   1 +
 tools/perf/tests/event_groups.c | 126 ++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h        |   1 +
 4 files changed, 129 insertions(+)
 create mode 100644 tools/perf/tests/event_groups.c

diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index 2064a640facb..9a59d451ca44 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 7122eae1d98d..48cd0c809cb1 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..92486b68cadb
--- /dev/null
+++ b/tools/perf/tests/event_groups.c
@@ -0,0 +1,126 @@
+// 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 "header.h"
+#include "../perf-sys.h"
+
+static int event_open(int type, unsigned long config, int g_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;
+	attr.disabled = g_fd == -1 ? 1 : 0;
+
+	return sys_perf_event_open(&attr, -1, 0, g_fd, 0);
+}
+
+/* hw: cycles, sw: context-switch, uncore: [arch dependent] */
+int type[] = {0, 1, -1};
+unsigned long config[] = {0, 3, -1};
+
+static int setup_uncore_event(void)
+{
+	char pmu_name[25] = {0};
+	struct perf_pmu *pmu;
+
+#if defined(__x86_64__) || defined(__i386__)
+{
+	char buf[128] = {0};
+
+	if (get_cpuid(buf, sizeof(buf)))
+		return -1;
+	if (strstr(buf, "Intel")) {
+		strcpy(pmu_name, "uncore_imc_0"); /* XXX */
+		config[2] = 0xff04;
+	} else {
+		strcpy(pmu_name, "amd_l3");
+		config[2] = 0xff04; /* l3_cache_accesses */
+	}
+}
+#elif defined(__arm__) || defined(__aarch64__)
+	strcpy(pmu_name, "l2c_220"); /* XXX */
+	config[2] = 0x0;
+#elif defined(__powerpc__)
+	strcpy(pmu_name, "hv_24x7"); /* XXX */
+	config[2] = 0x0;
+#elif defined(__s390x__)
+	strcpy(pmu_name, "pai_crypto"); /* XXX */
+	config[2] = 0x0;
+#else
+	pr_debug("No uncore pmu event found\n");
+	return -1;
+#endif
+
+	pmu = perf_pmu__find(pmu_name);
+	if (!pmu) {
+		pr_debug("Can not find uncore pmu\n");
+		return -1;
+	}
+	type[2] = pmu->type;
+	return 0;
+}
+
+static int run_test(int i, int j, int k)
+{
+	int erroneous = ((((1 << i) | (1 << j) | (1 << k)) & 5) == 5);
+	int fd1, fd2, fd3;
+
+	fd1 = event_open(type[i], config[i], -1);
+	if (fd1 == -1)
+		return -1;
+
+	fd2 = event_open(type[j], config[j], fd1);
+	if (fd2 == -1) {
+		close(fd1);
+		return erroneous ? 0 : -1;
+	}
+
+	fd3 = event_open(type[k], config[k], fd1);
+	if (fd3 == -1) {
+		close(fd1);
+		close(fd2);
+		return erroneous ? 0 : -1;
+	}
+
+	close(fd1);
+	close(fd2);
+	close(fd3);
+	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 || type[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",
+					 type[i], config[i], type[j], config[j],
+					 type[k], config[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 5bbb8f6a48fc..08570b5505d7 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] perf test: Add event group test

[Ian Rogers]

On Thu, Nov 24, 2022 at 7:21 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. Basically, 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.

Awesome, thanks! Some comments below.

> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
> Note: Uncore pmu event detail for Intel, Arm, PowerPC and s390 needs
> to be verified/fixed. These are marked as XXX in the patch.
>
>  tools/perf/tests/Build          |   1 +
>  tools/perf/tests/builtin-test.c |   1 +
>  tools/perf/tests/event_groups.c | 126 ++++++++++++++++++++++++++++++++
>  tools/perf/tests/tests.h        |   1 +
>  4 files changed, 129 insertions(+)
>  create mode 100644 tools/perf/tests/event_groups.c
>
> diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> index 2064a640facb..9a59d451ca44 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 7122eae1d98d..48cd0c809cb1 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..92486b68cadb
> --- /dev/null
> +++ b/tools/perf/tests/event_groups.c
> @@ -0,0 +1,126 @@
> +// 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 "header.h"
> +#include "../perf-sys.h"
> +
> +static int event_open(int type, unsigned long config, int g_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;

Could you add a comment for the line below?

> +       attr.disabled = g_fd == -1 ? 1 : 0;
> +
> +       return sys_perf_event_open(&attr, -1, 0, g_fd, 0);
> +}
> +
> +/* hw: cycles, sw: context-switch, uncore: [arch dependent] */

static?

> +int type[] = {0, 1, -1};
> +unsigned long config[] = {0, 3, -1};
> +
> +static int setup_uncore_event(void)
> +{
> +       char pmu_name[25] = {0};
> +       struct perf_pmu *pmu;
> +

I think the below finding of an uncore PMU is clunky. On my tigerlake
Intel laptop, for example, I don't have an uncore_imc_0 but do have
uncore_imc_free_running_0. I think the real fix here is that we should
start a new "pmus.h" and "pmus.c", moving the pmus static variable
from pmu.c to pmus.c. In pmus.h we should have an every PMU iterator,
like we do with perf_pmu__for_each_hybrid_pmu. I'd like to go further
with a pmus.h, as the computation of the perf_pmu struct should be
done a lot more lazily than it is now. But for now you can just
iterate the pmus looking for one saying beginning with "uncore_" as a
name.

> +#if defined(__x86_64__) || defined(__i386__)
> +{
> +       char buf[128] = {0};
> +
> +       if (get_cpuid(buf, sizeof(buf)))
> +               return -1;
> +       if (strstr(buf, "Intel")) {
> +               strcpy(pmu_name, "uncore_imc_0"); /* XXX */
> +               config[2] = 0xff04;
> +       } else {
> +               strcpy(pmu_name, "amd_l3");
> +               config[2] = 0xff04; /* l3_cache_accesses */
> +       }
> +}
> +#elif defined(__arm__) || defined(__aarch64__)
> +       strcpy(pmu_name, "l2c_220"); /* XXX */
> +       config[2] = 0x0;
> +#elif defined(__powerpc__)
> +       strcpy(pmu_name, "hv_24x7"); /* XXX */
> +       config[2] = 0x0;
> +#elif defined(__s390x__)
> +       strcpy(pmu_name, "pai_crypto"); /* XXX */
> +       config[2] = 0x0;
> +#else
> +       pr_debug("No uncore pmu event found\n");
> +       return -1;
> +#endif
> +
> +       pmu = perf_pmu__find(pmu_name);
> +       if (!pmu) {
> +               pr_debug("Can not find uncore pmu\n");
> +               return -1;
> +       }
> +       type[2] = pmu->type;
> +       return 0;
> +}
> +
> +static int run_test(int i, int j, int k)
> +{
> +       int erroneous = ((((1 << i) | (1 << j) | (1 << k)) & 5) == 5);
> +       int fd1, fd2, fd3;
> +
> +       fd1 = event_open(type[i], config[i], -1);

nit: a name like "event_group_leader_fd" would be more intention
revealing than fd1.

Thanks!
Ian

> +       if (fd1 == -1)
> +               return -1;
> +
> +       fd2 = event_open(type[j], config[j], fd1);
> +       if (fd2 == -1) {
> +               close(fd1);
> +               return erroneous ? 0 : -1;
> +       }
> +
> +       fd3 = event_open(type[k], config[k], fd1);
> +       if (fd3 == -1) {
> +               close(fd1);
> +               close(fd2);
> +               return erroneous ? 0 : -1;
> +       }
> +
> +       close(fd1);
> +       close(fd2);
> +       close(fd3);
> +       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 || type[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",
> +                                        type[i], config[i], type[j], config[j],
> +                                        type[k], config[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 5bbb8f6a48fc..08570b5505d7 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] perf test: Add event group test

[Ravi Bangoria]

On 27-Nov-22 8:58 AM, Ian Rogers wrote:
> On Thu, Nov 24, 2022 at 7:21 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. Basically, 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.
> 
> Awesome, thanks! Some comments below.

Thanks Ian!

>> +static int event_open(int type, unsigned long config, int g_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;
> 
> Could you add a comment for the line below?

Although this test exercises perf_event_open() and never enables any event,
I'm following standard practices. Snippet from man perf_event_open:

   disabled
       The disabled bit specifies whether the counter starts  out  dis‐
       abled  or  enabled.  If disabled, the event can later be enabled
       by ioctl(2), prctl(2), or enable_on_exec.

       When creating an event group, typically the group leader is ini‐
       tialized  with  disabled  set to 1 and any child events are ini‐
       tialized with disabled set to 0.  Despite disabled being 0,  the
       child events will not start until the group leader is enabled.

So it's well documented. I can probably put the same as comment.

> 
>> +       attr.disabled = g_fd == -1 ? 1 : 0;
>> +
>> +       return sys_perf_event_open(&attr, -1, 0, g_fd, 0);
>> +}
>> +
>> +/* hw: cycles, sw: context-switch, uncore: [arch dependent] */
> 
> static?

+1

> 
>> +int type[] = {0, 1, -1};
>> +unsigned long config[] = {0, 3, -1};
>> +
>> +static int setup_uncore_event(void)
>> +{
>> +       char pmu_name[25] = {0};
>> +       struct perf_pmu *pmu;
>> +
> 
> I think the below finding of an uncore PMU is clunky.

Agree.

> On my tigerlake
> Intel laptop, for example, I don't have an uncore_imc_0 but do have
> uncore_imc_free_running_0. I think the real fix here is that we should
> start a new "pmus.h" and "pmus.c", moving the pmus static variable
> from pmu.c to pmus.c. In pmus.h we should have an every PMU iterator,
> like we do with perf_pmu__for_each_hybrid_pmu.

I see only one variable that can be moved from pmu.c to pmus.c:
  LIST_HEAD(pmus)
So introducing new file pmus.c with just one list variable and a macro to
iterate over it seems overkill. Or are you suggesting to also migrate all
pmu.c functions which iterates over pmus list?

> I'd like to go further
> with a pmus.h, as the computation of the perf_pmu struct should be
> done a lot more lazily than it is now. But for now you can just
> iterate the pmus looking for one saying beginning with "uncore_" as a
> name.

Ok. I can probably introduce perf_pmu__starts_with(const char *name).

>> +static int run_test(int i, int j, int k)
>> +{
>> +       int erroneous = ((((1 << i) | (1 << j) | (1 << k)) & 5) == 5);
>> +       int fd1, fd2, fd3;
>> +
>> +       fd1 = event_open(type[i], config[i], -1);
> 
> nit: a name like "event_group_leader_fd" would be more intention
> revealing than fd1.

hmm, but that's too long :). Are you ok with:
  s/fd1/group_fd/
  s/fd2/sibling_fd1/
  s/fd3/sibling_fd2/

Thanks,
Ravi

Re: [PATCH] perf test: Add event group test

[Ian Rogers]

On Sun, Nov 27, 2022 at 11:02 PM Ravi Bangoria <ravi.bangoria@amd.com> wrote:
>
> On 27-Nov-22 8:58 AM, Ian Rogers wrote:
> > On Thu, Nov 24, 2022 at 7:21 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. Basically, 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.
> >
> > Awesome, thanks! Some comments below.
>
> Thanks Ian!
>
> >> +static int event_open(int type, unsigned long config, int g_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;
> >
> > Could you add a comment for the line below?
>
> Although this test exercises perf_event_open() and never enables any event,
> I'm following standard practices. Snippet from man perf_event_open:
>
>    disabled
>        The disabled bit specifies whether the counter starts  out  dis‐
>        abled  or  enabled.  If disabled, the event can later be enabled
>        by ioctl(2), prctl(2), or enable_on_exec.
>
>        When creating an event group, typically the group leader is ini‐
>        tialized  with  disabled  set to 1 and any child events are ini‐
>        tialized with disabled set to 0.  Despite disabled being 0,  the
>        child events will not start until the group leader is enabled.
>
> So it's well documented. I can probably put the same as comment.
>
> >
> >> +       attr.disabled = g_fd == -1 ? 1 : 0;
> >> +
> >> +       return sys_perf_event_open(&attr, -1, 0, g_fd, 0);
> >> +}
> >> +
> >> +/* hw: cycles, sw: context-switch, uncore: [arch dependent] */
> >
> > static?
>
> +1
>
> >
> >> +int type[] = {0, 1, -1};
> >> +unsigned long config[] = {0, 3, -1};
> >> +
> >> +static int setup_uncore_event(void)
> >> +{
> >> +       char pmu_name[25] = {0};
> >> +       struct perf_pmu *pmu;
> >> +
> >
> > I think the below finding of an uncore PMU is clunky.
>
> Agree.
>
> > On my tigerlake
> > Intel laptop, for example, I don't have an uncore_imc_0 but do have
> > uncore_imc_free_running_0. I think the real fix here is that we should
> > start a new "pmus.h" and "pmus.c", moving the pmus static variable
> > from pmu.c to pmus.c. In pmus.h we should have an every PMU iterator,
> > like we do with perf_pmu__for_each_hybrid_pmu.
>
> I see only one variable that can be moved from pmu.c to pmus.c:
>   LIST_HEAD(pmus)
> So introducing new file pmus.c with just one list variable and a macro to
> iterate over it seems overkill. Or are you suggesting to also migrate all
> pmu.c functions which iterates over pmus list?
>
> > I'd like to go further
> > with a pmus.h, as the computation of the perf_pmu struct should be
> > done a lot more lazily than it is now. But for now you can just
> > iterate the pmus looking for one saying beginning with "uncore_" as a
> > name.
>
> Ok. I can probably introduce perf_pmu__starts_with(const char *name).

uncore_ works for Intel, but... I agree having pmus.c for 1 variable
is overkill. I'd put an iterator in pmus.h like:

#define perf_pmu__for_each_pmu(pmu) list_for_each_entry(pmu, &pmus, list)

and also not have pmus be static in pmu.c. That way in the test you
can iterate the pmus and find the uncore ones.

> >> +static int run_test(int i, int j, int k)
> >> +{
> >> +       int erroneous = ((((1 << i) | (1 << j) | (1 << k)) & 5) == 5);
> >> +       int fd1, fd2, fd3;
> >> +
> >> +       fd1 = event_open(type[i], config[i], -1);
> >
> > nit: a name like "event_group_leader_fd" would be more intention
> > revealing than fd1.
>
> hmm, but that's too long :). Are you ok with:
>   s/fd1/group_fd/
>   s/fd2/sibling_fd1/
>   s/fd3/sibling_fd2/
>
> Thanks,
> Ravi

I'm fine with the other changes. I've done too much Java to be
allergic to long variable names ;-)

Thanks,
Ian