[PATCH] perf test: Fix parse-events tests to skip parametrized events

[PATCH] perf test: Fix parse-events tests to skip parametrized events

[Athira Rajeev]

Testcase "Parsing of all PMU events from sysfs" parse events for
all PMUs, and not just cpu. In case of powerpc, the PowerVM
environment supports events from hv_24x7 and hv_gpci PMU which
is of example format like below:

- hv_24x7/CPM_ADJUNCT_INST,domain=?,core=?/
- hv_gpci/event,partition_id=?/

The value for "?" needs to be filled in depending on system
configuration. It is better to skip these parametrized events
in this test as it is done in:
'commit b50d691e50e6 ("perf test: Fix "all PMU test" to skip
parametrized events")' which handled a simialr instance with
"all PMU test".

Fix parse-events test to skip parametrized events since
it needs proper setup of the parameters.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/tests/parse-events.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index b2f82847e4c3..605373c7d005 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -2504,7 +2504,11 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
 	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
 		struct stat st;
 		char path[PATH_MAX];
+		char pmu_event[PATH_MAX + 256];
+		char *buf = NULL;
+		FILE *file;
 		struct dirent *ent;
+		size_t len = 0;
 		DIR *dir;
 		int err;
 
@@ -2528,11 +2532,39 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
 			struct evlist_test e = { .name = NULL, };
 			char name[2 * NAME_MAX + 1 + 12 + 3];
 			int test_ret;
+			int skip = 0;
 
 			/* Names containing . are special and cannot be used directly */
 			if (strchr(ent->d_name, '.'))
 				continue;
 
+			/* exclude parametrized ones (name contains '?') */
+			snprintf(pmu_event, PATH_MAX + 256, "%s%s", path, ent->d_name);
+			file = fopen(pmu_event, "r");
+			if (!file) {
+				pr_debug("can't open pmu event file for '%s'\n", ent->d_name);
+				ret = combine_test_results(ret, TEST_FAIL);
+				continue;
+			}
+
+			if (getline(&buf, &len, file) < 0) {
+				pr_debug(" pmu event: %s is a null event\n", ent->d_name);
+				ret = combine_test_results(ret, TEST_FAIL);
+				continue;
+			}
+
+			if (strchr(buf, '?'))
+				skip = 1;
+
+			free(buf);
+			buf = NULL;
+			fclose(file);
+
+			if (skip == 1) {
+				pr_debug("skipping parametrized PMU event: %s which contains ?\n", pmu_event);
+				continue;
+			}
+
 			snprintf(name, sizeof(name), "%s/event=%s/u", pmu->name, ent->d_name);
 
 			e.name  = name;
-- 
2.31.1

Re: [PATCH] perf test: Fix parse-events tests to skip parametrized events

[Sachin Sant]

> On 07-Aug-2023, at 10:20 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> 
> Testcase "Parsing of all PMU events from sysfs" parse events for
> all PMUs, and not just cpu. In case of powerpc, the PowerVM
> environment supports events from hv_24x7 and hv_gpci PMU which
> is of example format like below:
> 
> - hv_24x7/CPM_ADJUNCT_INST,domain=?,core=?/
> - hv_gpci/event,partition_id=?/
> 
> The value for "?" needs to be filled in depending on system
> configuration. It is better to skip these parametrized events
> in this test as it is done in:
> 'commit b50d691e50e6 ("perf test: Fix "all PMU test" to skip
> parametrized events")' which handled a simialr instance with
> "all PMU test".
> 
> Fix parse-events test to skip parametrized events since
> it needs proper setup of the parameters.
> 
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> —

Thanks Athira for the fix. With this fix applied the reported problem
Is fixed.

6.1: Test event parsing                                            : Ok
  6.2: Parsing of all PMU events from sysfs             : Ok
  6.3: Parsing of given PMU events from sysfs        : Ok

Tested-by: Sachin Sant <sachinp@linux.ibm.com>

- Sachin

Re: [PATCH] perf test: Fix parse-events tests to skip parametrized events

[Athira Rajeev]

> On 07-Aug-2023, at 11:40 AM, Sachin Sant <sachinp@linux.ibm.com> wrote:
> 
> 
> 
>> On 07-Aug-2023, at 10:20 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
>> 
>> Testcase "Parsing of all PMU events from sysfs" parse events for
>> all PMUs, and not just cpu. In case of powerpc, the PowerVM
>> environment supports events from hv_24x7 and hv_gpci PMU which
>> is of example format like below:
>> 
>> - hv_24x7/CPM_ADJUNCT_INST,domain=?,core=?/
>> - hv_gpci/event,partition_id=?/
>> 
>> The value for "?" needs to be filled in depending on system
>> configuration. It is better to skip these parametrized events
>> in this test as it is done in:
>> 'commit b50d691e50e6 ("perf test: Fix "all PMU test" to skip
>> parametrized events")' which handled a simialr instance with
>> "all PMU test".
>> 
>> Fix parse-events test to skip parametrized events since
>> it needs proper setup of the parameters.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> —
> 
> Thanks Athira for the fix. With this fix applied the reported problem
> Is fixed.
> 
> 6.1: Test event parsing                                            : Ok
>  6.2: Parsing of all PMU events from sysfs             : Ok
>  6.3: Parsing of given PMU events from sysfs        : Ok
> 
> Tested-by: Sachin Sant <sachinp@linux.ibm.com>
> 
> - Sachin

Hi All,

Looking for review comments on this patch

Thanks
Athira

Re: [PATCH] perf test: Fix parse-events tests to skip parametrized events

[Ian Rogers]

On Sun, Aug 6, 2023 at 9:50 PM Athira Rajeev
<atrajeev@linux.vnet.ibm.com> wrote:
>
> Testcase "Parsing of all PMU events from sysfs" parse events for
> all PMUs, and not just cpu. In case of powerpc, the PowerVM
> environment supports events from hv_24x7 and hv_gpci PMU which
> is of example format like below:
>
> - hv_24x7/CPM_ADJUNCT_INST,domain=?,core=?/
> - hv_gpci/event,partition_id=?/
>
> The value for "?" needs to be filled in depending on system
> configuration. It is better to skip these parametrized events
> in this test as it is done in:
> 'commit b50d691e50e6 ("perf test: Fix "all PMU test" to skip
> parametrized events")' which handled a simialr instance with
> "all PMU test".

I'd say this is different, the "?" is really ugly. On other
architectures the problem is solved by having >1 PMU, domain and core
can be meta-data associated with the PMU. If we want to aggregate
based on domain and core in the perf tool, it will need a different
way of solving the problem for Power. Skipping the test is just
pushing this problem down the road.

> Fix parse-events test to skip parametrized events since
> it needs proper setup of the parameters.
>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
>  tools/perf/tests/parse-events.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index b2f82847e4c3..605373c7d005 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -2504,7 +2504,11 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
>         while ((pmu = perf_pmus__scan(pmu)) != NULL) {
>                 struct stat st;
>                 char path[PATH_MAX];
> +               char pmu_event[PATH_MAX + 256];

By definition paths can't be longer than PATH_MAX.

> +               char *buf = NULL;
> +               FILE *file;
>                 struct dirent *ent;
> +               size_t len = 0;
>                 DIR *dir;
>                 int err;
>
> @@ -2528,11 +2532,39 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
>                         struct evlist_test e = { .name = NULL, };
>                         char name[2 * NAME_MAX + 1 + 12 + 3];
>                         int test_ret;
> +                       int skip = 0;

Prefer a boolean. Prefer is_event_parameterized over skip to make
variable name more intention revealing.

>
>                         /* Names containing . are special and cannot be used directly */
>                         if (strchr(ent->d_name, '.'))
>                                 continue;
>
> +                       /* exclude parametrized ones (name contains '?') */
> +                       snprintf(pmu_event, PATH_MAX + 256, "%s%s", path, ent->d_name);

Use sizeof(pmu_event) rather than "PATH_MAX + 256".

Thanks,
Ian

> +                       file = fopen(pmu_event, "r");
> +                       if (!file) {
> +                               pr_debug("can't open pmu event file for '%s'\n", ent->d_name);
> +                               ret = combine_test_results(ret, TEST_FAIL);
> +                               continue;
> +                       }
> +
> +                       if (getline(&buf, &len, file) < 0) {
> +                               pr_debug(" pmu event: %s is a null event\n", ent->d_name);
> +                               ret = combine_test_results(ret, TEST_FAIL);
> +                               continue;
> +                       }
> +
> +                       if (strchr(buf, '?'))
> +                               skip = 1;
> +
> +                       free(buf);
> +                       buf = NULL;
> +                       fclose(file);
> +
> +                       if (skip == 1) {
> +                               pr_debug("skipping parametrized PMU event: %s which contains ?\n", pmu_event);
> +                               continue;
> +                       }
> +
>                         snprintf(name, sizeof(name), "%s/event=%s/u", pmu->name, ent->d_name);
>
>                         e.name  = name;
> --
> 2.31.1
>

Re: [PATCH] perf test: Fix parse-events tests to skip parametrized events

[Athira Rajeev]

> On 18-Aug-2023, at 12:07 AM, Ian Rogers <irogers@google.com> wrote:
> 
> On Sun, Aug 6, 2023 at 9:50 PM Athira Rajeev
> <atrajeev@linux.vnet.ibm.com> wrote:
>> 
>> Testcase "Parsing of all PMU events from sysfs" parse events for
>> all PMUs, and not just cpu. In case of powerpc, the PowerVM
>> environment supports events from hv_24x7 and hv_gpci PMU which
>> is of example format like below:
>> 
>> - hv_24x7/CPM_ADJUNCT_INST,domain=?,core=?/
>> - hv_gpci/event,partition_id=?/
>> 
>> The value for "?" needs to be filled in depending on system
>> configuration. It is better to skip these parametrized events
>> in this test as it is done in:
>> 'commit b50d691e50e6 ("perf test: Fix "all PMU test" to skip
>> parametrized events")' which handled a simialr instance with
>> "all PMU test".
> 
> I'd say this is different, the "?" is really ugly. On other
> architectures the problem is solved by having >1 PMU, domain and core
> can be meta-data associated with the PMU. If we want to aggregate
> based on domain and core in the perf tool, it will need a different
> way of solving the problem for Power. Skipping the test is just
> pushing this problem down the road.

Hi Ian

Thanks for the comments. We will check on this for powerpc.
> 
>> Fix parse-events test to skip parametrized events since
>> it needs proper setup of the parameters.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> tools/perf/tests/parse-events.c | 32 ++++++++++++++++++++++++++++++++
>> 1 file changed, 32 insertions(+)
>> 
>> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
>> index b2f82847e4c3..605373c7d005 100644
>> --- a/tools/perf/tests/parse-events.c
>> +++ b/tools/perf/tests/parse-events.c
>> @@ -2504,7 +2504,11 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
>>        while ((pmu = perf_pmus__scan(pmu)) != NULL) {
>>                struct stat st;
>>                char path[PATH_MAX];
>> +               char pmu_event[PATH_MAX + 256];
> 
> By definition paths can't be longer than PATH_MAX.
Yeah, my bad. Will fix this.

> 
>> +               char *buf = NULL;
>> +               FILE *file;
>>                struct dirent *ent;
>> +               size_t len = 0;
>>                DIR *dir;
>>                int err;
>> 
>> @@ -2528,11 +2532,39 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
>>                        struct evlist_test e = { .name = NULL, };
>>                        char name[2 * NAME_MAX + 1 + 12 + 3];
>>                        int test_ret;
>> +                       int skip = 0;
> 
> Prefer a boolean. Prefer is_event_parameterized over skip to make
> variable name more intention revealing.
Sure
> 
>> 
>>                        /* Names containing . are special and cannot be used directly */
>>                        if (strchr(ent->d_name, '.'))
>>                                continue;
>> 
>> +                       /* exclude parametrized ones (name contains '?') */
>> +                       snprintf(pmu_event, PATH_MAX + 256, "%s%s", path, ent->d_name);
> 
> Use sizeof(pmu_event) rather than "PATH_MAX + 256".

Will send V2 with these changes
> 
> Thanks,
> Ian
> 
>> +                       file = fopen(pmu_event, "r");
>> +                       if (!file) {
>> +                               pr_debug("can't open pmu event file for '%s'\n", ent->d_name);
>> +                               ret = combine_test_results(ret, TEST_FAIL);
>> +                               continue;
>> +                       }
>> +
>> +                       if (getline(&buf, &len, file) < 0) {
>> +                               pr_debug(" pmu event: %s is a null event\n", ent->d_name);
>> +                               ret = combine_test_results(ret, TEST_FAIL);
>> +                               continue;
>> +                       }
>> +
>> +                       if (strchr(buf, '?'))
>> +                               skip = 1;
>> +
>> +                       free(buf);
>> +                       buf = NULL;
>> +                       fclose(file);
>> +
>> +                       if (skip == 1) {
>> +                               pr_debug("skipping parametrized PMU event: %s which contains ?\n", pmu_event);
>> +                               continue;
>> +                       }
>> +
>>                        snprintf(name, sizeof(name), "%s/event=%s/u", pmu->name, ent->d_name);
>> 
>>                        e.name  = name;
>> --
>> 2.31.1