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@google.com>
---
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);
+ 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-googEm 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@google.com>
> ---
> 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
On Wed, Aug 23, 2023 at 4:54 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> 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@google.com>
> > ---
> > 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.
Thanks, Arnaldo I'll fix this and rebase on to tmp.perf-tools-next for v2.
> - 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);
The unchecked strdup was a copy-and-paste from here.
Thanks,
Ian
> > - 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
© 2016 - 2025 Red Hat, Inc.