Re: [PATCH v4 4/6] perf test pmu: Add an eagerly loaded event test

From: Liang, Kan
Date: Thu May 02 2024 - 13:57:48 EST




On 2024-05-02 12:01 a.m., Ian Rogers wrote:
> Allow events/aliases to be eagerly loaded for a PMU. Factor out the
> pmu_aliases_parse to allow this. Parse a test event and check it
> configures the attribute as expected. There is overlap with the
> parse-events tests, but this test is done with a PMU created in a temp
> directory and doesn't rely on PMUs in sysfs.
>
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
> tools/perf/tests/pmu.c | 77 ++++++++++++++++++++++++++++++++++++++++++
> tools/perf/util/pmu.c | 69 +++++++++++++++++++++++++------------
> 2 files changed, 125 insertions(+), 21 deletions(-)
>
> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> index 424ebdb0f09d..6e18a4c447ce 100644
> --- a/tools/perf/tests/pmu.c
> +++ b/tools/perf/tests/pmu.c
> @@ -1,4 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> +#include "evlist.h"
> +#include "evsel.h"
> #include "parse-events.h"
> #include "pmu.h"
> #include "tests.h"
> @@ -54,6 +56,9 @@ static struct perf_pmu *test_pmu_get(char *dir, size_t sz)
> { "krava22", "config2:8,18,48,58\n", },
> { "krava23", "config2:28-29,38\n", },
> };
> + const char *test_event = "krava01=15,krava02=170,krava03=1,krava11=27,krava12=1,"
> + "krava13=2,krava21=119,krava22=11,krava23=2\n";
> +
> char name[PATH_MAX];
> int dirfd, file;
> struct perf_pmu *pmu = NULL;
> @@ -116,6 +121,24 @@ static struct perf_pmu *test_pmu_get(char *dir, size_t sz)
> close(file);
> }
>
> + /* Create test event. */
> + if (mkdirat(dirfd, "perf-pmu-test/events", 0755) < 0) {
> + pr_err("Failed to mkdir PMU events directory\n");
> + goto err_out;
> + }
> + file = openat(dirfd, "perf-pmu-test/events/test-event", O_WRONLY | O_CREAT, 0600);
> + if (!file) {
> + pr_err("Failed to open for writing file \"type\"\n");
> + goto err_out;
> + }
> + len = strlen(test_event);
> + if (write(file, test_event, len) < len) {
> + close(file);
> + pr_err("Failed to write to 'test-event' file\n");
> + goto err_out;
> + }
> + close(file);
> +
> /* Make the PMU reading the files created above. */
> pmu = perf_pmus__add_test_pmu(dirfd, "perf-pmu-test");
> if (!pmu)
> @@ -176,8 +199,62 @@ static int test__pmu_format(struct test_suite *test __maybe_unused, int subtest
> return ret;
> }
>
> +static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> +{
> + char dir[PATH_MAX];
> + struct parse_events_error err;
> + struct evlist *evlist;
> + struct evsel *evsel;
> + struct perf_event_attr *attr;
> + int ret;
> + struct perf_pmu *pmu = test_pmu_get(dir, sizeof(dir));
> + const char *event = "perf-pmu-test/test-event/";
> +
> +
> + if (!pmu)
> + return TEST_FAIL;
> +
> + evlist = evlist__new();
> + if (evlist == NULL) {
> + pr_err("Failed allocation");
> + goto err_out;
> + }
> + parse_events_error__init(&err);
> + ret = parse_events(evlist, event, &err);
> + if (ret) {
> + pr_debug("failed to parse event '%s', err %d\n", event, ret);
> + parse_events_error__print(&err, event);
> + ret = TEST_FAIL;
> + if (parse_events_error__contains(&err, "can't access trace events"))
> + ret = TEST_SKIP;
> + goto err_out;
> + }
> + evsel = evlist__first(evlist);
> + attr = &evsel->core.attr;
> + if (attr->config != 0xc00000000002a823) {
> + pr_err("Unexpected config value %llx\n", attr->config);
> + goto err_out;
> + }
> + if (attr->config1 != 0x8000400000000145) {
> + pr_err("Unexpected config1 value %llx\n", attr->config1);
> + goto err_out;
> + }
> + if (attr->config2 != 0x0400000020041d07) {
> + pr_err("Unexpected config2 value %llx\n", attr->config2);
> + goto err_out;
> + }
> +
> + ret = TEST_OK;
> +err_out:
> + parse_events_error__exit(&err);
> + evlist__delete(evlist);
> + test_pmu_put(dir, pmu);
> + return ret;

Got a warning when I compile the patch.

tests/pmu.c: In function ‘test__pmu_events’:
tests/pmu.c:257:16: error: ‘ret’ may be used uninitialized in this
function [-Werror=maybe-uninitialized]
257 | return ret;
| ^~~

Thanks,
Kan

> +}
> +
> static struct test_case tests__pmu[] = {
> TEST_CASE("Parsing with PMU format directory", pmu_format),
> + TEST_CASE("Parsing with PMU event", pmu_events),
> { .name = NULL, }
> };
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index fbbc535ed93f..7849be4bfea1 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -597,33 +597,18 @@ static inline bool pmu_alias_info_file(const char *name)
> * Reading the pmu event aliases definition, which should be located at:
> * /sys/bus/event_source/devices/<dev>/events as sysfs group attributes.
> */
> -static int pmu_aliases_parse(struct perf_pmu *pmu)
> +static int __pmu_aliases_parse(struct perf_pmu *pmu, int events_dir_fd)
> {
> - char path[PATH_MAX];
> struct dirent *evt_ent;
> DIR *event_dir;
> - size_t len;
> - int fd, dir_fd;
>
> - len = perf_pmu__event_source_devices_scnprintf(path, sizeof(path));
> - if (!len)
> - return 0;
> - scnprintf(path + len, sizeof(path) - len, "%s/events", pmu->name);
> -
> - dir_fd = open(path, O_DIRECTORY);
> - if (dir_fd == -1) {
> - pmu->sysfs_aliases_loaded = true;
> - return 0;
> - }
> -
> - event_dir = fdopendir(dir_fd);
> - if (!event_dir){
> - close (dir_fd);
> + event_dir = fdopendir(events_dir_fd);
> + if (!event_dir)
> return -EINVAL;
> - }
>
> while ((evt_ent = readdir(event_dir))) {
> char *name = evt_ent->d_name;
> + int fd;
> FILE *file;
>
> if (!strcmp(name, ".") || !strcmp(name, ".."))
> @@ -635,7 +620,7 @@ static int pmu_aliases_parse(struct perf_pmu *pmu)
> if (pmu_alias_info_file(name))
> continue;
>
> - fd = openat(dir_fd, name, O_RDONLY);
> + fd = openat(events_dir_fd, name, O_RDONLY);
> if (fd == -1) {
> pr_debug("Cannot open %s\n", name);
> continue;
> @@ -653,11 +638,50 @@ static int pmu_aliases_parse(struct perf_pmu *pmu)
> }
>
> closedir(event_dir);
> - close (dir_fd);
> pmu->sysfs_aliases_loaded = true;
> return 0;
> }
>
> +static int pmu_aliases_parse(struct perf_pmu *pmu)
> +{
> + char path[PATH_MAX];
> + size_t len;
> + int events_dir_fd, ret;
> +
> + if (pmu->sysfs_aliases_loaded)
> + return 0;
> +
> + len = perf_pmu__event_source_devices_scnprintf(path, sizeof(path));
> + if (!len)
> + return 0;
> + scnprintf(path + len, sizeof(path) - len, "%s/events", pmu->name);
> +
> + events_dir_fd = open(path, O_DIRECTORY);
> + if (events_dir_fd == -1) {
> + pmu->sysfs_aliases_loaded = true;
> + return 0;
> + }
> + ret = __pmu_aliases_parse(pmu, events_dir_fd);
> + close(events_dir_fd);
> + return ret;
> +}
> +
> +static int pmu_aliases_parse_eager(struct perf_pmu *pmu, int sysfs_fd)
> +{
> + char path[FILENAME_MAX + 7];
> + int ret, events_dir_fd;
> +
> + scnprintf(path, sizeof(path), "%s/events", pmu->name);
> + events_dir_fd = openat(sysfs_fd, path, O_DIRECTORY, 0);
> + if (events_dir_fd == -1) {
> + pmu->sysfs_aliases_loaded = true;
> + return 0;
> + }
> + ret = __pmu_aliases_parse(pmu, events_dir_fd);
> + close(events_dir_fd);
> + return ret;
> +}
> +
> static int pmu_alias_terms(struct perf_pmu_alias *alias, int err_loc, struct list_head *terms)
> {
> struct parse_events_term *term, *cloned;
> @@ -1042,6 +1066,9 @@ struct perf_pmu *perf_pmu__lookup(struct list_head *pmus, int dirfd, const char
>
> perf_pmu__arch_init(pmu);
>
> + if (eager_load)
> + pmu_aliases_parse_eager(pmu, dirfd);
> +
> return pmu;
> err:
> zfree(&pmu->name);