Re: [PATCH v1 09/25] perf pmu: Make the loading of formats lazy

From: Arnaldo Carvalho de Melo
Date: Wed Aug 23 2023 - 07:54:23 EST

Next message: Martin Kaiser: "Re: [PATCH] char: hw_random: bcm2835-rng: removed call to platform_set_drvdata()"
Previous message: Rob Herring: "Re: [PATCH] mailbox: Explicitly include correct DT includes"
In reply to: Ian Rogers: "[PATCH v1 09/25] perf pmu: Make the loading of formats lazy"
Next in thread: Ian Rogers: "Re: [PATCH v1 09/25] perf pmu: Make the loading of formats lazy"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Em Wed, Aug 23, 2023 at 01:08:12AM -0700, Ian Rogers escreveu:
> The sysfs format files are loaded eagerly in a PMU. Add a flag so that
> we create the format but only load the contents when necessary.
>
> Reduce the size of the value in struct perf_pmu_format and avoid holes
> so there is no additional space requirement.
>
> For "perf stat -e cycles true" this reduces the number of openat calls
> from 648 to 573 (about 12%). The benchmark pmu scan speed is improved
> by roughly 5%.
>
> Before: $ perf bench internals pmu-scan
> Computing performance of sysfs PMU event scan for 100 times
> Average core PMU scanning took: 1061.100 usec (+- 9.965 usec)
> Average PMU scanning took: 4725.300 usec (+- 260.599 usec)
>
> After: $ perf bench internals pmu-scan
> Computing performance of sysfs PMU event scan for 100 times
> Average core PMU scanning took: 989.170 usec (+- 6.873 usec)
> Average PMU scanning took: 4520.960 usec (+- 251.272 usec)
>
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
> tools/perf/tests/pmu.c | 2 +-
> tools/perf/util/pmu.c | 140 +++++++++++++++++++++++++++--------------
> tools/perf/util/pmu.h | 5 +-
> tools/perf/util/pmu.y | 20 +++---
> 4 files changed, 102 insertions(+), 65 deletions(-)
>
> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> index 2c1c349a42e2..c204ed1f1a8b 100644
> --- a/tools/perf/tests/pmu.c
> +++ b/tools/perf/tests/pmu.c
> @@ -171,7 +171,7 @@ static int test__pmu(struct test_suite *test __maybe_unused, int subtest __maybe
> }
>
> pmu->name = strdup("perf-pmu-test");
> - ret = perf_pmu__format_parse(pmu, fd);
> + ret = perf_pmu__format_parse(pmu, fd, /*eager_load=*/true);
> if (ret)
> goto out;
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 42f3249994ab..7c3de51bab08 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -40,6 +40,10 @@ struct perf_pmu perf_pmu__fake;
> * value=PERF_PMU_FORMAT_VALUE_CONFIG and bits 0 to 7 will be set.
> */
> struct perf_pmu_format {
> + /** @list: Element on list within struct perf_pmu. */
> + struct list_head list;
> + /** @bits: Which config bits are set by this format value. */
> + DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
> /** @name: The modifier/file name. */
> char *name;
> /**
> @@ -47,18 +51,75 @@ struct perf_pmu_format {
> * are from PERF_PMU_FORMAT_VALUE_CONFIG to
> * PERF_PMU_FORMAT_VALUE_CONFIG_END.
> */
> - int value;
> - /** @bits: Which config bits are set by this format value. */
> - DECLARE_BITMAP(bits, PERF_PMU_FORMAT_BITS);
> - /** @list: Element on list within struct perf_pmu. */
> - struct list_head list;
> + u16 value;
> + /** @loaded: Has the contents been loaded/parsed. */
> + bool loaded;
> };
>
> +static struct perf_pmu_format *perf_pmu__new_format(struct list_head *list, char *name)
> +{
> + struct perf_pmu_format *format;
> +
> + format = zalloc(sizeof(*format));
> + if (!format)
> + return NULL;
> +
> + format->name = strdup(name);

We need to check this for failure, later you assume it is non-NULL,
calling strlen() on it, etc.

I applied all the previous patches, will review the others later.

- Arnaldo

> + list_add_tail(&format->list, list);
> + return format;
> +}
> +
> +/* Called at the end of parsing a format. */
> +void perf_pmu_format__set_value(void *vformat, int config, unsigned long *bits)
> +{
> + struct perf_pmu_format *format = vformat;
> +
> + format->value = config;
> + memcpy(format->bits, bits, sizeof(format->bits));
> +}
> +
> +static void __perf_pmu_format__load(struct perf_pmu_format *format, FILE *file)
> +{
> + void *scanner;
> + int ret;
> +
> + ret = perf_pmu_lex_init(&scanner);
> + if (ret)
> + return;
> +
> + perf_pmu_set_in(file, scanner);
> + ret = perf_pmu_parse(format, scanner);
> + perf_pmu_lex_destroy(scanner);
> + format->loaded = true;
> +}
> +
> +static void perf_pmu_format__load(struct perf_pmu *pmu, struct perf_pmu_format *format)
> +{
> + char path[PATH_MAX];
> + FILE *file = NULL;
> +
> + if (format->loaded)
> + return;
> +
> + if (!perf_pmu__pathname_scnprintf(path, sizeof(path), pmu->name, "format"))
> + return;
> +
> + assert(strlen(path) + strlen(format->name) + 2 < sizeof(path));
> + strcat(path, "/");
> + strcat(path, format->name);
> +
> + file = fopen(path, "r");
> + if (!file)
> + return;
> + __perf_pmu_format__load(format, file);
> + fclose(file);
> +}
> +
> /*
> * Parse & process all the sysfs attributes located under
> * the directory specified in 'dir' parameter.
> */
> -int perf_pmu__format_parse(struct perf_pmu *pmu, int dirfd)
> +int perf_pmu__format_parse(struct perf_pmu *pmu, int dirfd, bool eager_load)
> {
> struct dirent *evt_ent;
> DIR *format_dir;
> @@ -68,37 +129,35 @@ int perf_pmu__format_parse(struct perf_pmu *pmu, int dirfd)
> if (!format_dir)
> return -EINVAL;
>
> - while (!ret && (evt_ent = readdir(format_dir))) {
> + while ((evt_ent = readdir(format_dir)) != NULL) {
> + struct perf_pmu_format *format;
> char *name = evt_ent->d_name;
> - int fd;
> - void *scanner;
> - FILE *file;
>
> if (!strcmp(name, ".") || !strcmp(name, ".."))
> continue;
>
> -
> - ret = -EINVAL;
> - fd = openat(dirfd, name, O_RDONLY);
> - if (fd < 0)
> - break;
> -
> - file = fdopen(fd, "r");
> - if (!file) {
> - close(fd);
> + format = perf_pmu__new_format(&pmu->format, name);
> + if (!format) {
> + ret = -ENOMEM;
> break;
> }
>
> - ret = perf_pmu_lex_init(&scanner);
> - if (ret) {
> + if (eager_load) {
> + FILE *file;
> + int fd = openat(dirfd, name, O_RDONLY);
> +
> + if (fd < 0) {
> + ret = -errno;
> + break;
> + }
> + file = fdopen(fd, "r");
> + if (!file) {
> + close(fd);
> + break;
> + }
> + __perf_pmu_format__load(format, file);
> fclose(file);
> - break;
> }
> -
> - perf_pmu_set_in(file, scanner);
> - ret = perf_pmu_parse(&pmu->format, name, scanner);
> - perf_pmu_lex_destroy(scanner);
> - fclose(file);
> }
>
> closedir(format_dir);
> @@ -119,7 +178,7 @@ static int pmu_format(struct perf_pmu *pmu, int dirfd, const char *name)
> return 0;
>
> /* it'll close the fd */
> - if (perf_pmu__format_parse(pmu, fd))
> + if (perf_pmu__format_parse(pmu, fd, /*eager_load=*/false))
> return -1;
>
> return 0;
> @@ -962,13 +1021,15 @@ void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu)
> if (pmu == &perf_pmu__fake)
> return;
>
> - list_for_each_entry(format, &pmu->format, list)
> + list_for_each_entry(format, &pmu->format, list) {
> + perf_pmu_format__load(pmu, format);
> if (format->value >= PERF_PMU_FORMAT_VALUE_CONFIG_END) {
> pr_warning("WARNING: '%s' format '%s' requires 'perf_event_attr::config%d'"
> "which is not supported by this version of perf!\n",
> pmu->name, format->name, format->value);
> return;
> }
> + }
> }
>
> bool evsel__is_aux_event(const struct evsel *evsel)
> @@ -1041,6 +1102,7 @@ int perf_pmu__format_type(struct perf_pmu *pmu, const char *name)
> if (!format)
> return -1;
>
> + perf_pmu_format__load(pmu, format);
> return format->value;
> }
>
> @@ -1177,7 +1239,7 @@ static int pmu_config_term(struct perf_pmu *pmu,
> free(pmu_term);
> return -EINVAL;
> }
> -
> + perf_pmu_format__load(pmu, format);
> switch (format->value) {
> case PERF_PMU_FORMAT_VALUE_CONFIG:
> vp = &attr->config;
> @@ -1403,24 +1465,6 @@ struct perf_pmu_alias *perf_pmu__find_alias(struct perf_pmu *pmu, const char *ev
>
> return NULL;
> }
> -
> -int perf_pmu__new_format(struct list_head *list, char *name,
> - int config, unsigned long *bits)
> -{
> - struct perf_pmu_format *format;
> -
> - format = zalloc(sizeof(*format));
> - if (!format)
> - return -ENOMEM;
> -
> - format->name = strdup(name);
> - format->value = config;
> - memcpy(format->bits, bits, sizeof(format->bits));
> -
> - list_add_tail(&format->list, list);
> - return 0;
> -}
> -
> static void perf_pmu__del_formats(struct list_head *formats)
> {
> struct perf_pmu_format *fmt, *tmp;
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index c4268053c979..675c9b97f7bf 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -227,9 +227,8 @@ int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
> struct perf_pmu_info *info);
> struct perf_pmu_alias *perf_pmu__find_alias(struct perf_pmu *pmu, const char *event);
>
> -int perf_pmu__new_format(struct list_head *list, char *name,
> - int config, unsigned long *bits);
> -int perf_pmu__format_parse(struct perf_pmu *pmu, int dirfd);
> +int perf_pmu__format_parse(struct perf_pmu *pmu, int dirfd, bool eager_load);
> +void perf_pmu_format__set_value(void *format, int config, unsigned long *bits);
> bool perf_pmu__has_format(const struct perf_pmu *pmu, const char *name);
>
> bool is_pmu_core(const char *name);
> diff --git a/tools/perf/util/pmu.y b/tools/perf/util/pmu.y
> index d861a5bfa3bd..600c8c158c8e 100644
> --- a/tools/perf/util/pmu.y
> +++ b/tools/perf/util/pmu.y
> @@ -1,6 +1,5 @@
> %define api.pure full
> -%parse-param {struct list_head *format}
> -%parse-param {char *name}
> +%parse-param {void *format}
> %parse-param {void *scanner}
> %lex-param {void* scanner}
>
> @@ -21,7 +20,7 @@ do { \
> YYABORT; \
> } while (0)
>
> -static void perf_pmu_error(struct list_head *list, char *name, void *scanner, char const *msg);
> +static void perf_pmu_error(void *format, void *scanner, const char *msg);
>
> static void perf_pmu__set_format(unsigned long *bits, long from, long to)
> {
> @@ -59,16 +58,12 @@ format_term
> format_term:
> PP_CONFIG ':' bits
> {
> - ABORT_ON(perf_pmu__new_format(format, name,
> - PERF_PMU_FORMAT_VALUE_CONFIG,
> - $3));
> + perf_pmu_format__set_value(format, PERF_PMU_FORMAT_VALUE_CONFIG, $3);
> }
> |
> PP_CONFIG PP_VALUE ':' bits
> {
> - ABORT_ON(perf_pmu__new_format(format, name,
> - $2,
> - $4));
> + perf_pmu_format__set_value(format, $2, $4);
> }
>
> bits:
> @@ -95,9 +90,8 @@ PP_VALUE
>
> %%
>
> -static void perf_pmu_error(struct list_head *list __maybe_unused,
> - char *name __maybe_unused,
> - void *scanner __maybe_unused,
> - char const *msg __maybe_unused)
> +static void perf_pmu_error(void *format __maybe_unused,
> + void *scanner __maybe_unused,
> + const char *msg __maybe_unused)
> {
> }
> --
> 2.42.0.rc1.204.g551eb34607-goog
>

--

- Arnaldo

Next message: Martin Kaiser: "Re: [PATCH] char: hw_random: bcm2835-rng: removed call to platform_set_drvdata()"
Previous message: Rob Herring: "Re: [PATCH] mailbox: Explicitly include correct DT includes"
In reply to: Ian Rogers: "[PATCH v1 09/25] perf pmu: Make the loading of formats lazy"
Next in thread: Ian Rogers: "Re: [PATCH v1 09/25] perf pmu: Make the loading of formats lazy"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]