Messages in this thread | | | Date | Wed, 23 Aug 2023 08:54:15 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH v1 09/25] perf pmu: Make the loading of formats lazy |
| |
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.
- 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
| |