> On 18-Aug-2023, at 12:07 AM, Ian Rogers <irog...@google.com> wrote:
>
> On Sun, Aug 6, 2023 at 9:50 PM Athira Rajeev
> <atraj...@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 <atraj...@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
