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

[PATCH v2 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.

v1: https://lore.kernel.org/r/20221125032018.962-1-ravi.bangoria@amd.com
v1->v2:
 - #1 is new. It makes pmus list variable non-static and moves
   it to a new file.
 - Instead of hardcoded uncore pmu configuration, iterate over
   pmus list and use whichever first uncore pmu is available.

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 | 109 ++++++++++++++++++++++++++++++++
 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, 128 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 v2 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 v2 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 | 109 ++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h        |   1 +
 4 files changed, 112 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..4002b467cc8f
--- /dev/null
+++ b/tools/perf/tests/event_groups.c
@@ -0,0 +1,109 @@
+// 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"
+
+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);
+}
+
+/* hw: cycles, sw: context-switch, uncore: [arch dependent] */
+static int type[] = {0, 1, -1};
+static unsigned long config[] = {0, 3, 0};
+
+static int setup_uncore_event(void)
+{
+	struct perf_pmu *pmu;
+
+	if (list_empty(&pmus))
+		perf_pmu__scan(NULL);
+
+	perf_pmus__for_each_pmu(pmu) {
+		if (pmu->is_uncore) {
+			pr_debug("Using %s for uncore pmu event\n", pmu->name);
+			type[2] = pmu->type;
+			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(type[i], config[i], -1);
+	if (group_fd == -1)
+		return -1;
+
+	sibling_fd1 = event_open(type[j], config[j], group_fd);
+	if (sibling_fd1 == -1) {
+		close(group_fd);
+		return erroneous ? 0 : -1;
+	}
+
+	sibling_fd2 = event_open(type[k], config[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 || 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 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 v2 2/2] perf test: Add event group test

[Liang, Kan]

On 2022-11-29 6:19 a.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.
> 
> 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 | 109 ++++++++++++++++++++++++++++++++
>  tools/perf/tests/tests.h        |   1 +
>  4 files changed, 112 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..4002b467cc8f
> --- /dev/null
> +++ b/tools/perf/tests/event_groups.c
> @@ -0,0 +1,109 @@
> +// 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"
> +
> +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);
> +}
> +
> +/* hw: cycles, sw: context-switch, uncore: [arch dependent] */
> +static int type[] = {0, 1, -1};
> +static unsigned long config[] = {0, 3, 0};
> +
> +static int setup_uncore_event(void)
> +{
> +	struct perf_pmu *pmu;
> +
> +	if (list_empty(&pmus))
> +		perf_pmu__scan(NULL);
> +
> +	perf_pmus__for_each_pmu(pmu) {
> +		if (pmu->is_uncore) {

Always using the first uncore PMU may trigger false alarm on some Intel
platforms. For example, Intel has free running uncore PMUs (e.g.,
uncore_imc_free_running_0), which only supports special event encoding
0xff. The config 0 must fails.
You may want to add the below check to ignore the free running uncore PMUs.
                        if (strstr(pmu->name, "free_running"))
                                continue;


Also, some uncore PMUs only support two counters. But the test assumes
that the number of counters > 2. You may want to limit the size of the
group for 2 for a pure uncore group.


Thanks,
Kan

> +			pr_debug("Using %s for uncore pmu event\n", pmu->name);
> +			type[2] = pmu->type;
> +			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(type[i], config[i], -1);
> +	if (group_fd == -1)
> +		return -1;
> +
> +	sibling_fd1 = event_open(type[j], config[j], group_fd);
> +	if (sibling_fd1 == -1) {
> +		close(group_fd);
> +		return erroneous ? 0 : -1;
> +	}
> +
> +	sibling_fd2 = event_open(type[k], config[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 || 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 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 v2 2/2] perf test: Add event group test

[Ravi Bangoria]

Hi Kan,

Thanks for the review.

>> +static int setup_uncore_event(void)
>> +{
>> +	struct perf_pmu *pmu;
>> +
>> +	if (list_empty(&pmus))
>> +		perf_pmu__scan(NULL);
>> +
>> +	perf_pmus__for_each_pmu(pmu) {
>> +		if (pmu->is_uncore) {
> 
> Always using the first uncore PMU may trigger false alarm on some Intel
> platforms. For example, Intel has free running uncore PMUs (e.g.,
> uncore_imc_free_running_0), which only supports special event encoding
> 0xff. The config 0 must fails.
> You may want to add the below check to ignore the free running uncore PMUs.
>                         if (strstr(pmu->name, "free_running"))
>                                 continue;
> 
> 
> Also, some uncore PMUs only support two counters. But the test assumes
> that the number of counters > 2. You may want to limit the size of the
> group for 2 for a pure uncore group.

That seems hacky. Instead of ignoring, would it be possible to provide
a list of testable pmus? Example with random values:

  /* Uncore pmus that support more than 3 counters */
  static struct uncore_pmus {
      char *name;
      unsigned long config;
  } uncore_pmus[] = {
      { "amd_l3",         0x0  },
      { "amd_df",         0x0  },
      { "uncore_imc_xxx", 0xff },   /* Intel */
      { "intel_xxx_pmu2", 0xff },   /* Intel */
      { "abc_pmu1",       0x0  },   /* Arm */
      { "hv_24x7",        0xa  },   /* PowerPC */
      { ...                    },
  };

  perf_pmus__for_each_pmu(pmu) {
      if (pmu present in uncore_pmus[])
          type[2] = pmu->type;
          config[2] = pmu->config;
  }

Ofcourse, this should work for all architectures. Arm, PowerPC, s390 folks?

Thanks,
Ravi

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

[Liang, Kan]

On 2022-12-01 4:13 a.m., Ravi Bangoria wrote:
> Hi Kan,
> 
> Thanks for the review.
> 
>>> +static int setup_uncore_event(void)
>>> +{
>>> +	struct perf_pmu *pmu;
>>> +
>>> +	if (list_empty(&pmus))
>>> +		perf_pmu__scan(NULL);
>>> +
>>> +	perf_pmus__for_each_pmu(pmu) {
>>> +		if (pmu->is_uncore) {
>>
>> Always using the first uncore PMU may trigger false alarm on some Intel
>> platforms. For example, Intel has free running uncore PMUs (e.g.,
>> uncore_imc_free_running_0), which only supports special event encoding
>> 0xff. The config 0 must fails.
>> You may want to add the below check to ignore the free running uncore PMUs.
>>                         if (strstr(pmu->name, "free_running"))
>>                                 continue;
>>
>>
>> Also, some uncore PMUs only support two counters. But the test assumes
>> that the number of counters > 2. You may want to limit the size of the
>> group for 2 for a pure uncore group.
> 
> That seems hacky. Instead of ignoring, would it be possible to provide
> a list of testable pmus? Example with random values:
> 
>   /* Uncore pmus that support more than 3 counters */
>   static struct uncore_pmus {
>       char *name;
>       unsigned long config;
>   } uncore_pmus[] = {
>       { "amd_l3",         0x0  },
>       { "amd_df",         0x0  },
>       { "uncore_imc_xxx", 0xff },   /* Intel */

IMC seems a safe choice. AFAIK, we should have at least uncore_imc_0 for
all the existing Intel platforms. { "uncore_imc_0", 0x1 }

>       { "intel_xxx_pmu2", 0xff },   /* Intel */

Intel doesn't have such uncore PMUs.

>       { "abc_pmu1",       0x0  },   /* Arm */
>       { "hv_24x7",        0xa  },   /* PowerPC */
>       { ...                    },
>   };
> 
>   perf_pmus__for_each_pmu(pmu) {
>       if (pmu present in uncore_pmus[])
>           type[2] = pmu->type;
>           config[2] = pmu->config;>   }


Not sure the uncore_pmus[] can cover all possible names for all
architectures.

Maybe we should fall back to the first uncore PMU and try again if
nothing match the uncore_pmus[].

Thanks,
Kan
> 
> Ofcourse, this should work for all architectures. Arm, PowerPC, s390 folks?
> 
> Thanks,
> Ravi

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

[Ravi Bangoria]

>>   /* Uncore pmus that support more than 3 counters */
>>   static struct uncore_pmus {
>>       char *name;
>>       unsigned long config;
>>   } uncore_pmus[] = {
>>       { "amd_l3",         0x0  },
>>       { "amd_df",         0x0  },
>>       { "uncore_imc_xxx", 0xff },   /* Intel */
> 
> IMC seems a safe choice. AFAIK, we should have at least uncore_imc_0 for
> all the existing Intel platforms. { "uncore_imc_0", 0x1 }

Ok. Ian said he don't see uncore_imc_0 on his tigerlake machine. Are you
sure uncore_imc_0 should be present on all existing Intel platforms?

>>       { "intel_xxx_pmu2", 0xff },   /* Intel */
> 
> Intel doesn't have such uncore PMUs.

Yeah this was just for example purpose.

>>       { "abc_pmu1",       0x0  },   /* Arm */
>>       { "hv_24x7",        0xa  },   /* PowerPC */
>>       { ...                    },
>>   };
>>
>>   perf_pmus__for_each_pmu(pmu) {
>>       if (pmu present in uncore_pmus[])
>>           type[2] = pmu->type;
>>           config[2] = pmu->config;>   }
> 
> 
> Not sure the uncore_pmus[] can cover all possible names for all
> architectures.

It doesn't need to cover _all_ possible names. It just needs to cover
minimal set of names which can cover all platforms for that architecture.

> Maybe we should fall back to the first uncore PMU and try again if
> nothing match the uncore_pmus[].

That's a good point. However, this can endup with the same problem you
mentioned: it may trigger false alarm on some platform. So better to
skip the test(and let someone add proper pmu in this list) rather than
proving false negative result?

Thanks,
Ravi

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

[Liang, Kan]

On 2022-12-01 10:29 a.m., Ravi Bangoria wrote:
>>>   /* Uncore pmus that support more than 3 counters */
>>>   static struct uncore_pmus {
>>>       char *name;
>>>       unsigned long config;
>>>   } uncore_pmus[] = {
>>>       { "amd_l3",         0x0  },
>>>       { "amd_df",         0x0  },
>>>       { "uncore_imc_xxx", 0xff },   /* Intel */
>>
>> IMC seems a safe choice. AFAIK, we should have at least uncore_imc_0 for
>> all the existing Intel platforms. { "uncore_imc_0", 0x1 }
> 
> Ok. Ian said he don't see uncore_imc_0 on his tigerlake machine. Are you
> sure uncore_imc_0 should be present on all existing Intel platforms?

For TGL and older client platforms, there is only free running IMC
counters. For other uncore PMUs on the old client platforms, I cannot
guarantee that then always have more then 2 counters. I think you can
skip the uncore test for these old platforms if you need at least 3
counters.


> 
>>>       { "intel_xxx_pmu2", 0xff },   /* Intel */
>>
>> Intel doesn't have such uncore PMUs.
> 
> Yeah this was just for example purpose.
> 
>>>       { "abc_pmu1",       0x0  },   /* Arm */
>>>       { "hv_24x7",        0xa  },   /* PowerPC */
>>>       { ...                    },
>>>   };
>>>
>>>   perf_pmus__for_each_pmu(pmu) {
>>>       if (pmu present in uncore_pmus[])
>>>           type[2] = pmu->type;
>>>           config[2] = pmu->config;>   }
>>
>>
>> Not sure the uncore_pmus[] can cover all possible names for all
>> architectures.
> 
> It doesn't need to cover _all_ possible names. It just needs to cover
> minimal set of names which can cover all platforms for that architecture.
>>> Maybe we should fall back to the first uncore PMU and try again if
>> nothing match the uncore_pmus[].
> 
> That's a good point. However, this can endup with the same problem you
> mentioned: it may trigger false alarm on some platform. So better to
> skip the test(and let someone add proper pmu in this list) rather than
> proving false negative result?

OK. Skipping the test for this case sounds good to me.

Thanks,
Kan
> 
> Thanks,
> Ravi

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

[Ravi Bangoria]

On 01-Dec-22 9:17 PM, Liang, Kan wrote:
> 
> 
> On 2022-12-01 10:29 a.m., Ravi Bangoria wrote:
>>>>   /* Uncore pmus that support more than 3 counters */
>>>>   static struct uncore_pmus {
>>>>       char *name;
>>>>       unsigned long config;
>>>>   } uncore_pmus[] = {
>>>>       { "amd_l3",         0x0  },
>>>>       { "amd_df",         0x0  },
>>>>       { "uncore_imc_xxx", 0xff },   /* Intel */
>>>
>>> IMC seems a safe choice. AFAIK, we should have at least uncore_imc_0 for
>>> all the existing Intel platforms. { "uncore_imc_0", 0x1 }
>>
>> Ok. Ian said he don't see uncore_imc_0 on his tigerlake machine. Are you
>> sure uncore_imc_0 should be present on all existing Intel platforms?
> 
> For TGL and older client platforms, there is only free running IMC
> counters. For other uncore PMUs on the old client platforms, I cannot
> guarantee that then always have more then 2 counters. I think you can
> skip the uncore test for these old platforms if you need at least 3
> counters.
> 
> 
>>
>>>>       { "intel_xxx_pmu2", 0xff },   /* Intel */
>>>
>>> Intel doesn't have such uncore PMUs.
>>
>> Yeah this was just for example purpose.
>>
>>>>       { "abc_pmu1",       0x0  },   /* Arm */
>>>>       { "hv_24x7",        0xa  },   /* PowerPC */
>>>>       { ...                    },
>>>>   };
>>>>
>>>>   perf_pmus__for_each_pmu(pmu) {
>>>>       if (pmu present in uncore_pmus[])
>>>>           type[2] = pmu->type;
>>>>           config[2] = pmu->config;>   }
>>>
>>>
>>> Not sure the uncore_pmus[] can cover all possible names for all
>>> architectures.
>>
>> It doesn't need to cover _all_ possible names. It just needs to cover
>> minimal set of names which can cover all platforms for that architecture.
>>>> Maybe we should fall back to the first uncore PMU and try again if
>>> nothing match the uncore_pmus[].
>>
>> That's a good point. However, this can endup with the same problem you
>> mentioned: it may trigger false alarm on some platform. So better to
>> skip the test(and let someone add proper pmu in this list) rather than
>> proving false negative result?
> 
> OK. Skipping the test for this case sounds good to me.

Thanks. Will respin with this change.

Ravi